On 03/25/2013 05:18 PM, Jakub Hrozek wrote:
On Fri, Mar 22, 2013 at 02:47:31PM +0100, Pavel Březina wrote:
> Hi, this patchset takes the current SRV lookup code and converts it
> into a plugin. Next waves will add custom plugin for IPA and AD.
>
> Patch 1: Introduce new plugin interface.
>
> Patch 2: First SRV lookup plugin.
>
> Patch 3: Replaces current code with a plugin code. I originally
> wanted to remove resolve_srv_* functions completely and call the
> plugin in fo_resolve_service_send() directly but that proved to be
> harder and more error sensitive than I expected.
>
> This got me thinking, we should create a refactoring ticket for
> the failover code. It has become quite complex and hard to read
> and understand because it lacks a lot of comments. Also many things
> there seems very hackish to me.
>
Feel free to create that ticket, I agree that the current failover
code has grown too much..but keep in mind that many of the "hacks"
are in fact special cases for all kinds of weird errors.
My idea was to decouple the caching logic in the failover to a
separate layer. So resolver would always talk to DNS or whatever back
end there was, then there would be a resolver cache and finally fail
over that would only manage states of servers and ports.
Is there anything else that bothers you with the current failover
architecture?
> Patch 4: Set SRV lookup plugin for all providers. This is done as a
> separate patch because it will be reverted once each provider has
> its own plugin.
>
> I also would like to discuss where we can set the plugin in the
> future. At the moment we have one failover context for the whole
> backend. But the backend can be configured to run several different
> providers at the same time. The plugin itself is made per provider
> - you have different plugin for IPA and different for LDAP.
>
> I think we should have failover context per provider. My idea is
> to create a new module constructor e.g. sssm_ipa_init(). This will
> be called only once before all other sssm_ipa_*_init() functions
> and it will initialize IPA-wise failover context.
>
In theory there may be a case where we need this functionality, yes.
I can't think of any use-case right now, but this may be a better
architecture going forward, especially as we are adding more
providers like sudo or autofs that are only implemented with some
back ends.
> We can also use it to initialize sdap_id_ctx instead of calling
> sssm_ipa_id_init from other places.
Given it more thoughts I don't think we should go this way. I like the
sssm_ipa_init() concept though and we should probably implement it in
1.11+.
But there is an advantage having only one plugin active. For example:
id_provider = ipa
sudo_provider = ldap
ldap_uri = _srv_
Even though sudo_provider is ldap we still contact ipa server so it is
logical to still use plugin with ipa sites support.
We can consider id_provider as authoritative provider and set the plugin
accordingly. Any thoughts?