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!