[SSSD] [PATCH] AD SRV plugin

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


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

Ups, I forgot to commit last changes.


-------------- 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/7490be52/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/7490be52/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/7490be52/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/7490be52/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/7490be52/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: 34494 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20130430/7490be52/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/7490be52/attachment-0006.bin>


More information about the sssd-devel mailing list