[SSSD] [PATCH] AD SRV plugin

Pavel Březina pbrezina at redhat.com
Tue Apr 30 14:00:36 UTC 2013


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.

> 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.

  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.

> [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.

>> --- 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.

> 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.

> 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-add-fo_discover_servers-request.patch
Type: text/x-patch
Size: 9939 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130430/8ace2298/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-IPA-SRV-plugin-use-fo_discover_servers-request.patch
Type: text/x-patch
Size: 7003 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130430/8ace2298/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-IPA-SRV-plugin-improve-debugging.patch
Type: text/x-patch
Size: 1253 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130430/8ace2298/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004-sdap-add-sdap_connect_host-request.patch
Type: text/x-patch
Size: 7920 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130430/8ace2298/attachment-0003.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005-add-sss_ldap_encode_ndr_uint32.patch
Type: text/x-patch
Size: 1647 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130430/8ace2298/attachment-0004.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0006-DNS-sites-support-add-AD-SRV-plugin.patch
Type: text/x-patch
Size: 34691 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130430/8ace2298/attachment-0005.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0007-dns-srv-plugin-compare-domain-names-case-insensitive.patch
Type: text/x-patch
Size: 994 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130430/8ace2298/attachment-0006.bin>


More information about the sssd-devel mailing list