[SSSD] [PATCH] AD SRV plugin

Jakub Hrozek jhrozek at redhat.com
Thu May 2 12:25:12 UTC 2013


On Tue, Apr 30, 2013 at 04:21:32PM +0200, Pavel Březina wrote:
> On 04/30/2013 04:00 PM, Pavel Březina wrote:
> >On 04/30/2013 12:37 AM, Jakub Hrozek wrote:
> >>On Mon, Apr 29, 2013 at 06:58:41PM +0200, Pavel Březina wrote:
> >>>On 04/17/2013 02:44 PM, Pavel Březina wrote:
> >>>>https://fedorahosted.org/sssd/ticket/1032
> >>>>
> >>>>Here it is.
> >>>>
> >>>
> >>>This is new patch set that addresses few issues raised by Jakub via IRC.
> >>>
> >>>The most important was that I should treat domain names as case
> >>>insensitive. So I changed strcmp() to strcasecmp() in
> >>>fo_discover_servers_primary_done() which was part of the the original
> >>>patch set and also included a new patch that makes the change in dns srv
> >>>plugin.
> >>
> >>tl;dr - looking mostly good. Let's fix some nitpicks and push the work
> >>to master.
> >>
> >>I performed testing using the dssite.msc MMC -- without any specific
> >>site, I
> >>got redirected to the Default-First-Site-Name site, defining the site
> >>got me redirected to the proper site.
> >>
> >>[PATCH 1/7] add fo_discover_servers request
> >>Mostly good, but I'd like to discuss returning backup servers as primary
> >>servers. I haven't tested that particular case, but can you confirm that
> >>if any records were added on the server that would resolve to primary
> >>servers, the client would pick them up after the SRV query expires
> >>without restarting the client completely?
> >
> >When the SRV fo_server collapse, all associated primary and backup
> >servers are removed from the list. The plugin doesn't have any state
> >from the previous run, so it will contact both primary and backup domain
> >again.
> >

Thanks for checking 

> >>Otherwise ack.
> >>
> >>[PATCH 2/7] IPA SRV plugin: use fo_discover_servers request
> >>Ack. I did a quick sanity test of the IPA DNS discovery and it still
> >>works.
> >>
> >>[PATCH 3/7] IPA SRV plugin: improve debugging
> >>Ack
> >>
> >>[PATCH 4/7] sdap: add sdap_connect_host request
> >>I tested this patch by checking with netstat that there was only one
> >>connection after the discovery used the request.
> >>
> >>I just wonder if using "struct be_resolv_ctx" would make more sense
> >>rather
> >>than using both "struct resolv_ctx" and "enum restrict_family".
> >
> >Hmm, the whole sdap async code doesn't have any knowledge of be what so
> >ever and I'd stick with that.
> >

OK

> >  It would also
> >>make sense to cancel the resolv request after "dns_resolver_timeout"
> >>passes
> >>(and read its value from "struct be_resolv_ctx"), although I would be OK
> >>with filing a ticket for that. This functionality is not needed for
> >>the beta.
> >
> >If it takes too long then it will hit a timeout for service resolver in
> >fail over, which already is dns_resolver_timeout.
> >

Right, I didn't realize we were in the resolve_service code already when
the plugin is called. Sorry.

> >>[PATCH 5/7] add sss_ldap_filter_binary_value
> >>Have you looked at ldap_encode_ndr_uint32() from Samba? At the very least
> >>I would prefer if the name made it clear that the function is converting
> >>uint32_t to char. FTR the current implementation of
> >>ldap_encode_ndr_uint32()
> >>looks like:
> >>
> >>char *ldap_encode_ndr_uint32(TALLOC_CTX *mem_ctx, uint32_t value)
> >>{
> >>     uint8_t buf[4];
> >>     struct ldb_val val;
> >>     SIVAL(buf, 0, value);
> >>     val.data = buf;
> >>     val.length = 4;
> >>     return ldb_binary_encode(mem_ctx, val);
> >>}
> >>
> >>So what about a very similar sss_ldap_encode_ndr_uint32() ?
> >
> >Done.
> >
> >>[PATCH 6/7] DNS sites support - add AD SRV plugin
> >>This patch makes samba4-devel a hard requirement of the AD provider.
> >>Given that we're considering dropping support of RHEL5 upstream now, I
> >>don't think this should be a blocker in accepting these patches, but I
> >>would prefer to file a ticket to discover if we can #ifdef the support
> >>out to keep the support of older releases upstream.
> >
> >>It might not be practical after all given the other AD provider features
> >>coming up in 1.10, but still, I think we should track the samba4-devel
> >>requirement and decide on implementing or dropping the ifdefs later
> >>during
> >>1.10.0 cycle when all the features are in.
> >
> >If we decide to #ifdef it, it will be quite simple, I think.
> >

Yes, since we dropped RHEL5 support in upstream, I no longer think it's
needed. We can ifdef it later if someone really needs it.

> >>>--- a/src/man/sssd-ad.5.xml
> >>>+++ b/src/man/sssd-ad.5.xml
> >>>@@ -130,6 +130,26 @@ ldap_id_mapping = False
> >>>                          </para>
> >>>                      </listitem>
> >>>                  </varlistentry>
> >>>+
> >>>+                <varlistentry>
> >>>+                    <term>ad_enable_dns_sites (boolean)</term>
> >>>+                    <listitem>
> >>>+                        <para>
> >>>+                            Enables DNS sites - location based
> >>>+                            service discovery.
> >>>+                        </para>
> >>>+                        <para>
> >>>+                            If true and service discovery (see Service
> >>>+                            Discovery paragraph at the bottom of the
> >>>man page)
> >>>+                            is enabled, then the SSSD will first
> >>>attempt
> >>>+                            location based discovery using an Active
> >>>Directory
> >>>+                            mechanism called Sites.
> >>
> >>I was wondering for a while whether to explain a little about the AD
> >>Sites, but the sssd-ad man page is probably not the right place..
> >
> >This is a place for google and MSDN IMHO.
> >

Yes, I just wanted to explain what the option does. 

> >>Still, what about "the SSSD will first attempt to discover the Active
> >>Directory server to connect to using the Active Directory Site
> >>Discovery and fall back to the DNS SRV records if no AD site is found".
> >
> >Sounds good.
> >

This is good enough for me.

> >>Defaulting to True is correct give the presence of the
> >>Default-First-Site-Name site.
> >>
> >>The ad_get_client_site_send() function could accept "struct
> >>be_resolv_ctx".
> >
> >Done.
> >
> >>Nitpick:
> >>>+static void ad_get_client_site_done(struct tevent_req *subreq)
> >>>+{
> >>>+    struct ad_get_client_site_state *state = NULL;
> >>>+    struct tevent_req *req = NULL;
> >>>+    struct ldb_message_element *el = NULL;
> >>>+    struct sysdb_attrs **reply = NULL;
> >>>+    size_t reply_count;
> >>>+    errno_t ret;
> >>>+
> >>>+    req = tevent_req_callback_data(subreq, struct tevent_req);
> >>>+    state = tevent_req_data(req, struct ad_get_client_site_state);
> >>>+
> >>>+    ret = sdap_get_generic_recv(subreq, state, &reply_count, &reply);
> >>>+    talloc_zfree(subreq);
> >>
> >>Can you put a comment here stating that this is where the LDAP
> >>connection that was used for the location discovery is closed?
> >
> >Done.
> >
> >>
> >>>+    talloc_zfree(state->sh);
> >>>+    if (ret != EOK) {
> >>
> >>The rest looks good to me.
> >>
> >>[PATCH 7/7] dns srv plugin: compare domain names case insensitive
> >>Ack
> >
> >Thank you for the review.
> 
> Ups, I forgot to commit last changes.
> 
> 

The latest changes work for me. Ack!



More information about the sssd-devel mailing list