Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On 06/19/2014 02:31 PM, Alexander Bokovoy wrote: > On Wed, 18 Jun 2014, Nathaniel McCallum wrote: >> On Wed, 2014-06-04 at 18:47 +0300, Alexander Bokovoy wrote: >>> On Thu, 01 May 2014, Nathaniel McCallum wrote: >>> >On Tue, 2014-03-11 at 11:09 -0400, Simo Sorce wrote: >>> >> On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote: >>> >> > On Tue, 11 Mar 2014, Jan Pazdziora wrote: >>> >> > >On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote: >>> >> > >> Before this patch, ipa-kdb would load global configuration on >>> >> > >> startup >>> >> > >> and never update it. This means that if global configuration is >>> changed, >>> >> > >> the KDC never receives the new configuration until it is restarted. >>> >> > >> >>> >> > >> This patch enables caching of the global configuration with a >>> timeout of >>> >> > >> 60 seconds. >>> >> > >> >>> >> > >> https://fedorahosted.org/freeipa/ticket/4153 >>> >> > > >>> >> > >> >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 >>> >> > >> >2001 >>> >> > >> From: Nathaniel McCallum >>> >> > >> Date: Mon, 24 Feb 2014 14:19:13 -0500 >>> >> > >> Subject: [PATCH] Periodically refresh global ipa-kdb configuration >>> >> > >> >>> >> > >> Before this patch, ipa-kdb would load global configuration on >>> startup and >>> >> > >> never update it. This means that if global configuration is changed, >>> the >>> >> > >> KDC never receives the new configuration until it is restarted. >>> >> > >> >>> >> > >> This patch enables caching of the global configuration with a >>> timeout of >>> >> > >> 60 seconds. >>> >> > >> >>> >> > >> https://fedorahosted.org/freeipa/ticket/4153 >>> >> > > >>> >> > >I have only read the code and it looks sane, so depending on how much >>> >> > >you consider my word about code-reading worth, ack. >>> >> > > >>> >> > >However, my gut feeling is that my preferred way of handling the issue >>> >> > >(without knowing much about the background of the ticket) would >>> >> > >probably be a HUP signal handler or something similar, rather than >>> >> > >polling for current values with the value timeout. This patch >>> >> > >introduces small nondeterminism to the behaviour when the usage of the >>> >> > >new values cannot really be enfoced by the admin (without the daemon >>> >> > >restart). >>> >> > Thing is, we need the update to happen when other, non-root process >>> >> > makes the changes -- in our case when IPA server running under httpd >>> >> > privileges performs series of MS-RPC calls towards smbd which lead to >>> >> > creating certain LDAP objects. httpd couldn't send SIGHUP to a process >>> >> > not owned by httpd's effective user (non-root). >>> >> >>> >> Yes but this is not really the way to go. >>> >> >>> >> The proper fix is to use syncrepl/persistent search to get a >>> >> notification of when we need to reload the configuration. >>> >> >>> >> On the patch itself I have a NACK due to this syntax used in various >>> >> places: function()->attribute >>> >> >>> >> don't. do. that. ever. >>> >> >>> >> assign whatever come from the function to a local variable and *check* >>> >> it is not null, *then* use it. >>> > >>> >Attached patch fixes the NACK issue, but does not address the question >>> >of the larger approach. Do we need to take a different approach? If so, >>> >what is it? >>> >>> I've tested the patch but was unable to spot any activity to refresh the >>> configuration after I've updated it with >>> ipa config-mod --ipaconfigstring=KDC:Disable\ Lockout >>> >>> At least, dirsrv logs didn't show me any attempt to re-read IPA config >>> past the change. >> >> I spent extensive time testing this today. I am unable to reproduce a >> failure to reload data from LDAP. If I make the change using "ipa >> config-mod", wait a minute and run a kinit the KDC always gets the new >> values. So it works for me. >> >> However, I did uncover a subtle bug with regard to ipaconfigstring which >> would cause it to never to be unset once set. Perhaps you were running >> into this? > Yes, that seems a likely cause. At least, I cannot reproduce it again. > >> >> The new attached version fixes this bug. Everything appears to work... > Thanks, ACK. Pushed to master. Martin ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On Wed, 18 Jun 2014, Nathaniel McCallum wrote: On Wed, 2014-06-04 at 18:47 +0300, Alexander Bokovoy wrote: On Thu, 01 May 2014, Nathaniel McCallum wrote: >On Tue, 2014-03-11 at 11:09 -0400, Simo Sorce wrote: >> On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote: >> > On Tue, 11 Mar 2014, Jan Pazdziora wrote: >> > >On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote: >> > >> Before this patch, ipa-kdb would load global configuration on startup >> > >> and never update it. This means that if global configuration is changed, >> > >> the KDC never receives the new configuration until it is restarted. >> > >> >> > >> This patch enables caching of the global configuration with a timeout of >> > >> 60 seconds. >> > >> >> > >> https://fedorahosted.org/freeipa/ticket/4153 >> > > >> > >> >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 2001 >> > >> From: Nathaniel McCallum >> > >> Date: Mon, 24 Feb 2014 14:19:13 -0500 >> > >> Subject: [PATCH] Periodically refresh global ipa-kdb configuration >> > >> >> > >> Before this patch, ipa-kdb would load global configuration on startup and >> > >> never update it. This means that if global configuration is changed, the >> > >> KDC never receives the new configuration until it is restarted. >> > >> >> > >> This patch enables caching of the global configuration with a timeout of >> > >> 60 seconds. >> > >> >> > >> https://fedorahosted.org/freeipa/ticket/4153 >> > > >> > >I have only read the code and it looks sane, so depending on how much >> > >you consider my word about code-reading worth, ack. >> > > >> > >However, my gut feeling is that my preferred way of handling the issue >> > >(without knowing much about the background of the ticket) would >> > >probably be a HUP signal handler or something similar, rather than >> > >polling for current values with the value timeout. This patch >> > >introduces small nondeterminism to the behaviour when the usage of the >> > >new values cannot really be enfoced by the admin (without the daemon >> > >restart). >> > Thing is, we need the update to happen when other, non-root process >> > makes the changes -- in our case when IPA server running under httpd >> > privileges performs series of MS-RPC calls towards smbd which lead to >> > creating certain LDAP objects. httpd couldn't send SIGHUP to a process >> > not owned by httpd's effective user (non-root). >> >> Yes but this is not really the way to go. >> >> The proper fix is to use syncrepl/persistent search to get a >> notification of when we need to reload the configuration. >> >> On the patch itself I have a NACK due to this syntax used in various >> places: function()->attribute >> >> don't. do. that. ever. >> >> assign whatever come from the function to a local variable and *check* >> it is not null, *then* use it. > >Attached patch fixes the NACK issue, but does not address the question >of the larger approach. Do we need to take a different approach? If so, >what is it? I've tested the patch but was unable to spot any activity to refresh the configuration after I've updated it with ipa config-mod --ipaconfigstring=KDC:Disable\ Lockout At least, dirsrv logs didn't show me any attempt to re-read IPA config past the change. I spent extensive time testing this today. I am unable to reproduce a failure to reload data from LDAP. If I make the change using "ipa config-mod", wait a minute and run a kinit the KDC always gets the new values. So it works for me. However, I did uncover a subtle bug with regard to ipaconfigstring which would cause it to never to be unset once set. Perhaps you were running into this? Yes, that seems a likely cause. At least, I cannot reproduce it again. The new attached version fixes this bug. Everything appears to work... Thanks, ACK. -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On Wed, 2014-06-04 at 18:47 +0300, Alexander Bokovoy wrote: > On Thu, 01 May 2014, Nathaniel McCallum wrote: > >On Tue, 2014-03-11 at 11:09 -0400, Simo Sorce wrote: > >> On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote: > >> > On Tue, 11 Mar 2014, Jan Pazdziora wrote: > >> > >On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote: > >> > >> Before this patch, ipa-kdb would load global configuration on startup > >> > >> and never update it. This means that if global configuration is > >> > >> changed, > >> > >> the KDC never receives the new configuration until it is restarted. > >> > >> > >> > >> This patch enables caching of the global configuration with a timeout > >> > >> of > >> > >> 60 seconds. > >> > >> > >> > >> https://fedorahosted.org/freeipa/ticket/4153 > >> > > > >> > >> >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 > >> > >> >2001 > >> > >> From: Nathaniel McCallum > >> > >> Date: Mon, 24 Feb 2014 14:19:13 -0500 > >> > >> Subject: [PATCH] Periodically refresh global ipa-kdb configuration > >> > >> > >> > >> Before this patch, ipa-kdb would load global configuration on startup > >> > >> and > >> > >> never update it. This means that if global configuration is changed, > >> > >> the > >> > >> KDC never receives the new configuration until it is restarted. > >> > >> > >> > >> This patch enables caching of the global configuration with a timeout > >> > >> of > >> > >> 60 seconds. > >> > >> > >> > >> https://fedorahosted.org/freeipa/ticket/4153 > >> > > > >> > >I have only read the code and it looks sane, so depending on how much > >> > >you consider my word about code-reading worth, ack. > >> > > > >> > >However, my gut feeling is that my preferred way of handling the issue > >> > >(without knowing much about the background of the ticket) would > >> > >probably be a HUP signal handler or something similar, rather than > >> > >polling for current values with the value timeout. This patch > >> > >introduces small nondeterminism to the behaviour when the usage of the > >> > >new values cannot really be enfoced by the admin (without the daemon > >> > >restart). > >> > Thing is, we need the update to happen when other, non-root process > >> > makes the changes -- in our case when IPA server running under httpd > >> > privileges performs series of MS-RPC calls towards smbd which lead to > >> > creating certain LDAP objects. httpd couldn't send SIGHUP to a process > >> > not owned by httpd's effective user (non-root). > >> > >> Yes but this is not really the way to go. > >> > >> The proper fix is to use syncrepl/persistent search to get a > >> notification of when we need to reload the configuration. > >> > >> On the patch itself I have a NACK due to this syntax used in various > >> places: function()->attribute > >> > >> don't. do. that. ever. > >> > >> assign whatever come from the function to a local variable and *check* > >> it is not null, *then* use it. > > > >Attached patch fixes the NACK issue, but does not address the question > >of the larger approach. Do we need to take a different approach? If so, > >what is it? > > I've tested the patch but was unable to spot any activity to refresh the > configuration after I've updated it with > ipa config-mod --ipaconfigstring=KDC:Disable\ Lockout > > At least, dirsrv logs didn't show me any attempt to re-read IPA config > past the change. I spent extensive time testing this today. I am unable to reproduce a failure to reload data from LDAP. If I make the change using "ipa config-mod", wait a minute and run a kinit the KDC always gets the new values. So it works for me. However, I did uncover a subtle bug with regard to ipaconfigstring which would cause it to never to be unset once set. Perhaps you were running into this? The new attached version fixes this bug. Everything appears to work... Nathaniel From 34bd5bd7f7e846ee302ff54891dc42e6ac777690 Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum Date: Mon, 24 Feb 2014 14:19:13 -0500 Subject: [PATCH] Periodically refresh global ipa-kdb configuration Before this patch, ipa-kdb would load global configuration on startup and never update it. This means that if global configuration is changed, the KDC never receives the new configuration until it is restarted. This patch enables caching of the global configuration with a timeout of 60 seconds. https://fedorahosted.org/freeipa/ticket/4153 --- daemons/ipa-kdb/ipa_kdb.c| 82 +--- daemons/ipa-kdb/ipa_kdb.h| 17 ++-- daemons/ipa-kdb/ipa_kdb_audit_as.c | 9 +++- daemons/ipa-kdb/ipa_kdb_mspac.c | 10 - daemons/ipa-kdb/ipa_kdb_principals.c | 11 - 5 files changed, 85 insertions(+), 44 deletions(-) diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c index 0f3996cdfa35374c005bc1ed174dea0816a27747..e5101bdd0ad880888fd58fd93a5ca8133868db98 100644 --- a/daemons/ipa-kdb/ipa_kdb.c +++ b/daemons/ipa-kdb/ipa_kdb.c @@ -25,6
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On Thu, 01 May 2014, Nathaniel McCallum wrote: On Tue, 2014-03-11 at 11:09 -0400, Simo Sorce wrote: On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote: > On Tue, 11 Mar 2014, Jan Pazdziora wrote: > >On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote: > >> Before this patch, ipa-kdb would load global configuration on startup > >> and never update it. This means that if global configuration is changed, > >> the KDC never receives the new configuration until it is restarted. > >> > >> This patch enables caching of the global configuration with a timeout of > >> 60 seconds. > >> > >> https://fedorahosted.org/freeipa/ticket/4153 > > > >> >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 2001 > >> From: Nathaniel McCallum > >> Date: Mon, 24 Feb 2014 14:19:13 -0500 > >> Subject: [PATCH] Periodically refresh global ipa-kdb configuration > >> > >> Before this patch, ipa-kdb would load global configuration on startup and > >> never update it. This means that if global configuration is changed, the > >> KDC never receives the new configuration until it is restarted. > >> > >> This patch enables caching of the global configuration with a timeout of > >> 60 seconds. > >> > >> https://fedorahosted.org/freeipa/ticket/4153 > > > >I have only read the code and it looks sane, so depending on how much > >you consider my word about code-reading worth, ack. > > > >However, my gut feeling is that my preferred way of handling the issue > >(without knowing much about the background of the ticket) would > >probably be a HUP signal handler or something similar, rather than > >polling for current values with the value timeout. This patch > >introduces small nondeterminism to the behaviour when the usage of the > >new values cannot really be enfoced by the admin (without the daemon > >restart). > Thing is, we need the update to happen when other, non-root process > makes the changes -- in our case when IPA server running under httpd > privileges performs series of MS-RPC calls towards smbd which lead to > creating certain LDAP objects. httpd couldn't send SIGHUP to a process > not owned by httpd's effective user (non-root). Yes but this is not really the way to go. The proper fix is to use syncrepl/persistent search to get a notification of when we need to reload the configuration. On the patch itself I have a NACK due to this syntax used in various places: function()->attribute don't. do. that. ever. assign whatever come from the function to a local variable and *check* it is not null, *then* use it. Attached patch fixes the NACK issue, but does not address the question of the larger approach. Do we need to take a different approach? If so, what is it? I've tested the patch but was unable to spot any activity to refresh the configuration after I've updated it with ipa config-mod --ipaconfigstring=KDC:Disable\ Lockout At least, dirsrv logs didn't show me any attempt to re-read IPA config past the change. Nathaniel -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On Mon, 2014-05-05 at 20:08 -0400, Dmitri Pal wrote: > On 05/02/2014 02:52 PM, Simo Sorce wrote: > > On Thu, 2014-05-01 at 16:22 -0400, Nathaniel McCallum wrote: > >> On Tue, 2014-03-11 at 11:09 -0400, Simo Sorce wrote: > >>> On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote: > On Tue, 11 Mar 2014, Jan Pazdziora wrote: > > On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote: > >> Before this patch, ipa-kdb would load global configuration on startup > >> and never update it. This means that if global configuration is > >> changed, > >> the KDC never receives the new configuration until it is restarted. > >> > >> This patch enables caching of the global configuration with a timeout > >> of > >> 60 seconds. > >> > >> https://fedorahosted.org/freeipa/ticket/4153 > >> >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 2001 > >> From: Nathaniel McCallum > >> Date: Mon, 24 Feb 2014 14:19:13 -0500 > >> Subject: [PATCH] Periodically refresh global ipa-kdb configuration > >> > >> Before this patch, ipa-kdb would load global configuration on startup > >> and > >> never update it. This means that if global configuration is changed, > >> the > >> KDC never receives the new configuration until it is restarted. > >> > >> This patch enables caching of the global configuration with a timeout > >> of > >> 60 seconds. > >> > >> https://fedorahosted.org/freeipa/ticket/4153 > > I have only read the code and it looks sane, so depending on how much > > you consider my word about code-reading worth, ack. > > > > However, my gut feeling is that my preferred way of handling the issue > > (without knowing much about the background of the ticket) would > > probably be a HUP signal handler or something similar, rather than > > polling for current values with the value timeout. This patch > > introduces small nondeterminism to the behaviour when the usage of the > > new values cannot really be enfoced by the admin (without the daemon > > restart). > Thing is, we need the update to happen when other, non-root process > makes the changes -- in our case when IPA server running under httpd > privileges performs series of MS-RPC calls towards smbd which lead to > creating certain LDAP objects. httpd couldn't send SIGHUP to a process > not owned by httpd's effective user (non-root). > >>> Yes but this is not really the way to go. > >>> > >>> The proper fix is to use syncrepl/persistent search to get a > >>> notification of when we need to reload the configuration. > >>> > >>> On the patch itself I have a NACK due to this syntax used in various > >>> places: function()->attribute > >>> > >>> don't. do. that. ever. > >>> > >>> assign whatever come from the function to a local variable and *check* > >>> it is not null, *then* use it. > >> Attached patch fixes the NACK issue, but does not address the question > >> of the larger approach. Do we need to take a different approach? If so, > >> what is it? > > LGTM > > I might prefer slightly more explicit names for those temp vars, but > > that's not a big deal. > > > > As for the approach, moving to something like syncrepl would be a good > > idea. As it would allow us to avoid useless polling and changes would be > > applaied as soon as they become available as the syncrepl agreement > > would push them to our client. It does mean we need a way to check that > > there aren't pending changes coming down the syncrepl pipe, but we can > > do that synchronously at every entry point in the KDB. After all we do > > not need to care about a change until the KDC needs something from the > > KDB. > > > > Simo. > > > Do we need a ticket for that then? Turns out we already have one: https://fedorahosted.org/freeipa/ticket/1302 Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On 05/02/2014 02:52 PM, Simo Sorce wrote: On Thu, 2014-05-01 at 16:22 -0400, Nathaniel McCallum wrote: On Tue, 2014-03-11 at 11:09 -0400, Simo Sorce wrote: On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote: On Tue, 11 Mar 2014, Jan Pazdziora wrote: On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote: Before this patch, ipa-kdb would load global configuration on startup and never update it. This means that if global configuration is changed, the KDC never receives the new configuration until it is restarted. This patch enables caching of the global configuration with a timeout of 60 seconds. https://fedorahosted.org/freeipa/ticket/4153 >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum Date: Mon, 24 Feb 2014 14:19:13 -0500 Subject: [PATCH] Periodically refresh global ipa-kdb configuration Before this patch, ipa-kdb would load global configuration on startup and never update it. This means that if global configuration is changed, the KDC never receives the new configuration until it is restarted. This patch enables caching of the global configuration with a timeout of 60 seconds. https://fedorahosted.org/freeipa/ticket/4153 I have only read the code and it looks sane, so depending on how much you consider my word about code-reading worth, ack. However, my gut feeling is that my preferred way of handling the issue (without knowing much about the background of the ticket) would probably be a HUP signal handler or something similar, rather than polling for current values with the value timeout. This patch introduces small nondeterminism to the behaviour when the usage of the new values cannot really be enfoced by the admin (without the daemon restart). Thing is, we need the update to happen when other, non-root process makes the changes -- in our case when IPA server running under httpd privileges performs series of MS-RPC calls towards smbd which lead to creating certain LDAP objects. httpd couldn't send SIGHUP to a process not owned by httpd's effective user (non-root). Yes but this is not really the way to go. The proper fix is to use syncrepl/persistent search to get a notification of when we need to reload the configuration. On the patch itself I have a NACK due to this syntax used in various places: function()->attribute don't. do. that. ever. assign whatever come from the function to a local variable and *check* it is not null, *then* use it. Attached patch fixes the NACK issue, but does not address the question of the larger approach. Do we need to take a different approach? If so, what is it? LGTM I might prefer slightly more explicit names for those temp vars, but that's not a big deal. As for the approach, moving to something like syncrepl would be a good idea. As it would allow us to avoid useless polling and changes would be applaied as soon as they become available as the syncrepl agreement would push them to our client. It does mean we need a way to check that there aren't pending changes coming down the syncrepl pipe, but we can do that synchronously at every entry point in the KDB. After all we do not need to care about a change until the KDC needs something from the KDB. Simo. Do we need a ticket for that then? -- Thank you, Dmitri Pal Sr. Engineering Manager IdM portfolio Red Hat, Inc. ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On Thu, 2014-05-01 at 16:22 -0400, Nathaniel McCallum wrote: > On Tue, 2014-03-11 at 11:09 -0400, Simo Sorce wrote: > > On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote: > > > On Tue, 11 Mar 2014, Jan Pazdziora wrote: > > > >On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote: > > > >> Before this patch, ipa-kdb would load global configuration on startup > > > >> and never update it. This means that if global configuration is > > > >> changed, > > > >> the KDC never receives the new configuration until it is restarted. > > > >> > > > >> This patch enables caching of the global configuration with a timeout > > > >> of > > > >> 60 seconds. > > > >> > > > >> https://fedorahosted.org/freeipa/ticket/4153 > > > > > > > >> >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 2001 > > > >> From: Nathaniel McCallum > > > >> Date: Mon, 24 Feb 2014 14:19:13 -0500 > > > >> Subject: [PATCH] Periodically refresh global ipa-kdb configuration > > > >> > > > >> Before this patch, ipa-kdb would load global configuration on startup > > > >> and > > > >> never update it. This means that if global configuration is changed, > > > >> the > > > >> KDC never receives the new configuration until it is restarted. > > > >> > > > >> This patch enables caching of the global configuration with a timeout > > > >> of > > > >> 60 seconds. > > > >> > > > >> https://fedorahosted.org/freeipa/ticket/4153 > > > > > > > >I have only read the code and it looks sane, so depending on how much > > > >you consider my word about code-reading worth, ack. > > > > > > > >However, my gut feeling is that my preferred way of handling the issue > > > >(without knowing much about the background of the ticket) would > > > >probably be a HUP signal handler or something similar, rather than > > > >polling for current values with the value timeout. This patch > > > >introduces small nondeterminism to the behaviour when the usage of the > > > >new values cannot really be enfoced by the admin (without the daemon > > > >restart). > > > Thing is, we need the update to happen when other, non-root process > > > makes the changes -- in our case when IPA server running under httpd > > > privileges performs series of MS-RPC calls towards smbd which lead to > > > creating certain LDAP objects. httpd couldn't send SIGHUP to a process > > > not owned by httpd's effective user (non-root). > > > > Yes but this is not really the way to go. > > > > The proper fix is to use syncrepl/persistent search to get a > > notification of when we need to reload the configuration. > > > > On the patch itself I have a NACK due to this syntax used in various > > places: function()->attribute > > > > don't. do. that. ever. > > > > assign whatever come from the function to a local variable and *check* > > it is not null, *then* use it. > > Attached patch fixes the NACK issue, but does not address the question > of the larger approach. Do we need to take a different approach? If so, > what is it? LGTM I might prefer slightly more explicit names for those temp vars, but that's not a big deal. As for the approach, moving to something like syncrepl would be a good idea. As it would allow us to avoid useless polling and changes would be applaied as soon as they become available as the syncrepl agreement would push them to our client. It does mean we need a way to check that there aren't pending changes coming down the syncrepl pipe, but we can do that synchronously at every entry point in the KDB. After all we do not need to care about a change until the KDC needs something from the KDB. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On Tue, 2014-03-11 at 11:09 -0400, Simo Sorce wrote:
> On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote:
> > On Tue, 11 Mar 2014, Jan Pazdziora wrote:
> > >On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote:
> > >> Before this patch, ipa-kdb would load global configuration on startup
> > >> and never update it. This means that if global configuration is changed,
> > >> the KDC never receives the new configuration until it is restarted.
> > >>
> > >> This patch enables caching of the global configuration with a timeout of
> > >> 60 seconds.
> > >>
> > >> https://fedorahosted.org/freeipa/ticket/4153
> > >
> > >> >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 2001
> > >> From: Nathaniel McCallum
> > >> Date: Mon, 24 Feb 2014 14:19:13 -0500
> > >> Subject: [PATCH] Periodically refresh global ipa-kdb configuration
> > >>
> > >> Before this patch, ipa-kdb would load global configuration on startup and
> > >> never update it. This means that if global configuration is changed, the
> > >> KDC never receives the new configuration until it is restarted.
> > >>
> > >> This patch enables caching of the global configuration with a timeout of
> > >> 60 seconds.
> > >>
> > >> https://fedorahosted.org/freeipa/ticket/4153
> > >
> > >I have only read the code and it looks sane, so depending on how much
> > >you consider my word about code-reading worth, ack.
> > >
> > >However, my gut feeling is that my preferred way of handling the issue
> > >(without knowing much about the background of the ticket) would
> > >probably be a HUP signal handler or something similar, rather than
> > >polling for current values with the value timeout. This patch
> > >introduces small nondeterminism to the behaviour when the usage of the
> > >new values cannot really be enfoced by the admin (without the daemon
> > >restart).
> > Thing is, we need the update to happen when other, non-root process
> > makes the changes -- in our case when IPA server running under httpd
> > privileges performs series of MS-RPC calls towards smbd which lead to
> > creating certain LDAP objects. httpd couldn't send SIGHUP to a process
> > not owned by httpd's effective user (non-root).
>
> Yes but this is not really the way to go.
>
> The proper fix is to use syncrepl/persistent search to get a
> notification of when we need to reload the configuration.
>
> On the patch itself I have a NACK due to this syntax used in various
> places: function()->attribute
>
> don't. do. that. ever.
>
> assign whatever come from the function to a local variable and *check*
> it is not null, *then* use it.
Attached patch fixes the NACK issue, but does not address the question
of the larger approach. Do we need to take a different approach? If so,
what is it?
Nathaniel
>From 0749ce930a3ca0dcefd7b7dc6ace5a851285a7d4 Mon Sep 17 00:00:00 2001
From: Nathaniel McCallum
Date: Mon, 24 Feb 2014 14:19:13 -0500
Subject: [PATCH] Periodically refresh global ipa-kdb configuration
Before this patch, ipa-kdb would load global configuration on startup and
never update it. This means that if global configuration is changed, the
KDC never receives the new configuration until it is restarted.
This patch enables caching of the global configuration with a timeout of
60 seconds.
https://fedorahosted.org/freeipa/ticket/4153
---
daemons/ipa-kdb/ipa_kdb.c| 65 +---
daemons/ipa-kdb/ipa_kdb.h| 17 +++---
daemons/ipa-kdb/ipa_kdb_audit_as.c | 9 +++--
daemons/ipa-kdb/ipa_kdb_mspac.c | 10 --
daemons/ipa-kdb/ipa_kdb_principals.c | 11 --
5 files changed, 75 insertions(+), 37 deletions(-)
diff --git a/daemons/ipa-kdb/ipa_kdb.c b/daemons/ipa-kdb/ipa_kdb.c
index 0f3996cdfa35374c005bc1ed174dea0816a27747..1b55735f1118ccbba2fc5d810c0171724634f9ad 100644
--- a/daemons/ipa-kdb/ipa_kdb.c
+++ b/daemons/ipa-kdb/ipa_kdb.c
@@ -25,6 +25,8 @@
#include "ipa_kdb.h"
+#define IPADB_GLOBAL_CONFIG_CACHE_TIME 60
+
struct ipadb_context *ipadb_get_context(krb5_context kcontext)
{
void *db_ctx;
@@ -41,6 +43,7 @@ struct ipadb_context *ipadb_get_context(krb5_context kcontext)
static void ipadb_context_free(krb5_context kcontext,
struct ipadb_context **ctx)
{
+struct ipadb_global_config *cfg;
size_t c;
if (*ctx != NULL) {
@@ -56,10 +59,11 @@ static void ipadb_context_free(krb5_context kcontext,
ipadb_mspac_struct_free(&(*ctx)->mspac);
krb5_free_default_realm(kcontext, (*ctx)->realm);
-for (c = 0; (*ctx)->authz_data && (*ctx)->authz_data[c]; c++) {
-free((*ctx)->authz_data[c]);
+cfg = &(*ctx)->config;
+for (c = 0; cfg->authz_data && cfg->authz_data[c]; c++) {
+free(cfg->authz_data[c]);
}
-free((*ctx)->authz_data);
+free(cfg->authz_data);
free(*ctx);
*ctx = NULL;
@@ -209,7 +213,7 @@ void ipadb_parse_user_auth(LDAP *lcontext, LDAPMessage *
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On Tue, 2014-03-11 at 16:05 +0200, Alexander Bokovoy wrote: > On Tue, 11 Mar 2014, Jan Pazdziora wrote: > >On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote: > >> Before this patch, ipa-kdb would load global configuration on startup > >> and never update it. This means that if global configuration is changed, > >> the KDC never receives the new configuration until it is restarted. > >> > >> This patch enables caching of the global configuration with a timeout of > >> 60 seconds. > >> > >> https://fedorahosted.org/freeipa/ticket/4153 > > > >> >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 2001 > >> From: Nathaniel McCallum > >> Date: Mon, 24 Feb 2014 14:19:13 -0500 > >> Subject: [PATCH] Periodically refresh global ipa-kdb configuration > >> > >> Before this patch, ipa-kdb would load global configuration on startup and > >> never update it. This means that if global configuration is changed, the > >> KDC never receives the new configuration until it is restarted. > >> > >> This patch enables caching of the global configuration with a timeout of > >> 60 seconds. > >> > >> https://fedorahosted.org/freeipa/ticket/4153 > > > >I have only read the code and it looks sane, so depending on how much > >you consider my word about code-reading worth, ack. > > > >However, my gut feeling is that my preferred way of handling the issue > >(without knowing much about the background of the ticket) would > >probably be a HUP signal handler or something similar, rather than > >polling for current values with the value timeout. This patch > >introduces small nondeterminism to the behaviour when the usage of the > >new values cannot really be enfoced by the admin (without the daemon > >restart). > Thing is, we need the update to happen when other, non-root process > makes the changes -- in our case when IPA server running under httpd > privileges performs series of MS-RPC calls towards smbd which lead to > creating certain LDAP objects. httpd couldn't send SIGHUP to a process > not owned by httpd's effective user (non-root). Yes but this is not really the way to go. The proper fix is to use syncrepl/persistent search to get a notification of when we need to reload the configuration. On the patch itself I have a NACK due to this syntax used in various places: function()->attribute don't. do. that. ever. assign whatever come from the function to a local variable and *check* it is not null, *then* use it. Simo. -- Simo Sorce * Red Hat, Inc * New York ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On Tue, 11 Mar 2014, Jan Pazdziora wrote: On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote: Before this patch, ipa-kdb would load global configuration on startup and never update it. This means that if global configuration is changed, the KDC never receives the new configuration until it is restarted. This patch enables caching of the global configuration with a timeout of 60 seconds. https://fedorahosted.org/freeipa/ticket/4153 >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 2001 From: Nathaniel McCallum Date: Mon, 24 Feb 2014 14:19:13 -0500 Subject: [PATCH] Periodically refresh global ipa-kdb configuration Before this patch, ipa-kdb would load global configuration on startup and never update it. This means that if global configuration is changed, the KDC never receives the new configuration until it is restarted. This patch enables caching of the global configuration with a timeout of 60 seconds. https://fedorahosted.org/freeipa/ticket/4153 I have only read the code and it looks sane, so depending on how much you consider my word about code-reading worth, ack. However, my gut feeling is that my preferred way of handling the issue (without knowing much about the background of the ticket) would probably be a HUP signal handler or something similar, rather than polling for current values with the value timeout. This patch introduces small nondeterminism to the behaviour when the usage of the new values cannot really be enfoced by the admin (without the daemon restart). Thing is, we need the update to happen when other, non-root process makes the changes -- in our case when IPA server running under httpd privileges performs series of MS-RPC calls towards smbd which lead to creating certain LDAP objects. httpd couldn't send SIGHUP to a process not owned by httpd's effective user (non-root). -- / Alexander Bokovoy ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On Mon, Feb 24, 2014 at 02:26:27PM -0500, Nathaniel McCallum wrote: > Before this patch, ipa-kdb would load global configuration on startup > and never update it. This means that if global configuration is changed, > the KDC never receives the new configuration until it is restarted. > > This patch enables caching of the global configuration with a timeout of > 60 seconds. > > https://fedorahosted.org/freeipa/ticket/4153 > >From 7daeae56671d7b3049b0341aad66c96877431bbe Mon Sep 17 00:00:00 2001 > From: Nathaniel McCallum > Date: Mon, 24 Feb 2014 14:19:13 -0500 > Subject: [PATCH] Periodically refresh global ipa-kdb configuration > > Before this patch, ipa-kdb would load global configuration on startup and > never update it. This means that if global configuration is changed, the > KDC never receives the new configuration until it is restarted. > > This patch enables caching of the global configuration with a timeout of > 60 seconds. > > https://fedorahosted.org/freeipa/ticket/4153 I have only read the code and it looks sane, so depending on how much you consider my word about code-reading worth, ack. However, my gut feeling is that my preferred way of handling the issue (without knowing much about the background of the ticket) would probably be a HUP signal handler or something similar, rather than polling for current values with the value timeout. This patch introduces small nondeterminism to the behaviour when the usage of the new values cannot really be enfoced by the admin (without the daemon restart). -- Jan Pazdziora Principal Software Engineer, Identity Management Engineering, Red Hat ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH 0044] Periodically refresh global ipa-kdb configuration
On Mon, 2014-02-24 at 14:26 -0500, Nathaniel McCallum wrote: > Before this patch, ipa-kdb would load global configuration on startup > and never update it. This means that if global configuration is changed, > the KDC never receives the new configuration until it is restarted. > > This patch enables caching of the global configuration with a timeout of > 60 seconds. > > https://fedorahosted.org/freeipa/ticket/4153 I still need a review of this. Nathaniel ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
