[SSSD] [INI] Ding-lIbs - Merging Configuration Snippets Installed in Different Locations (Design page)

Dmitri Pal dpal at redhat.com
Mon Apr 7 20:32:21 UTC 2014


On 04/07/2014 02:29 PM, Simo Sorce wrote:
> On Mon, 2014-04-07 at 13:55 -0400, Dmitri Pal wrote:
>> On 04/07/2014 08:37 AM, Simo Sorce wrote:
>>>> I disagree with several points here, sorry.
>>>>> 1) I agree that it would have been better if sssd.conf had a different
>>>>> semantics about the domains. But the situation right now is that the
>>>>> domains are listed. I agree we can go to your model but in reality there
>>>>> then will be more code and more logic in SSSD to actually identify which
>>>>> sections are domains and which are not.
>>> This is very easy, it's a string comparison on the name all domains
>>> start with domain/, but we can also add a 'type' key if people feel it
>>> is really necessary (I don't).
>> This is SSSD an specific change. The INI should be generic. It does not
>> know anything about domains.
> Yes, I understand you want to build generic features, but what I am
> saying is that I want the option of being able to tell the library to
> not do some things programmatically at initialization time, because I do
> not think we want to use some of the merging features in SSSD and we
> want to make sure the library does not merge stuff "behind SSSDs back".

Nothing will be merged unless the caller actually calls the proposed APIs.
The logic is : read the main config, apply additional snippets if the 
application wants.
So SSSD is in full control, INI just provides the capabilities that SSSD 
can elect to choose.

>
>>>>>   I am proposing to make it a part
>>>>> of the library so that we implement it once and SSSD does not need to
>>>>> change and write more custom code. The amount of code and complexity
>>>>> will be same it is just where you put it.
>>>>>
>>>>> Another thing is that domains is not the only option the provider list
>>>>> is constructed the same way. There is now two things that you have to
>>>>> change in the sssd.conf. IMO it is too much.
>>> I think we'll have to disagree, it is not a big deal on SSSD's side to
>>> change that behavior, and I retrospect I believe we should have done it
>>> that way from the start, exactly to make it easier to include sections
>>> from different files. So I would like to take the chance to do it now.
>> We can force SSSD to fix that. But this would mean a syntax change in
>> sssd.conf. This would require new version of the config to be added to
>> SSSD code.
> Nope, this is the thing, we do not need to do that, we can keep reading
> the list *and* also find additional domains that are instead enabled by
> means of an 'enabled' key in their domain definition. The configuration
> would be fully backwards compatible (must be, too much installed and
> scripted management software depends on it).
See my comments below in the previous mail.
You would also need to add keyword
domain = true
or something like to denote that this is the section that covers the 
domains or have some special convention in the name of the section
which would make the file less generic.



>
>>   While I agree that we should not have done this in SSSD IMO
>> we should leave SSSD as is and focus on making INI provide functionality
>> that will be consumed by SSSD without additional changes.
> I mentioned this change because I think it would be beneficial
> regardless.

I do not think this design should depend on whether and when SSSD 
decides to change its behavior.


>
>> This is in fact the design goal of this proposal.
>> If we take it as a design goal the change can be made in couple months
>> in INI. If we require SSSD to change it would take much more time to
>> implement.
> SSSD change is really simple, it will not take months, days at most.

Well, I tend to disagree.
Jakub, Lukas, can you chime in?

>
>> IMO it is not worth it.
> Here we strongly disagree, it is not the change per se that matters to
> me, but the effect on usability or readability/reliability of the merge
> future in INI. I think merging values in [global] would be really bad,
> as well as I think letting INI automatically merge non-conflicting
> sections from different files in conf.d would be a great feature.

I think it should really be.
We can control it by a flag.
We can add a flag to ini_mergeobj_run function to do the merges of the 
values or not. I think it will be a good compromise.
At the beginning SSSD can pass the flag to merge the values. Once it 
fixes the code to support self described sections for providers and 
domains it can change the flag to say that no need to merge values any more.

>
>>>>> 2) Include files are bad because they require you to modify the main
>>>>> config file. The use case I want to be able to support is that you can
>>>>> add domains without touching the main configuration file. Say you have a
>>>>> base image that you deploy and then you have some snippets that you drop
>>>>> in when you deploy things. Whether the configuration snippets are
>>>>> dropped in by an application or they are dropped by config tool like
>>>>> Puppet or they are created as part of Kickstart/Cloud-init or some other
>>>>> scripts run by orchestration software does not matter. What matters is
>>>>> that they should extend the configuration without touching the main
>>>>> file. That eliminates the include file approach altogether.
>>> I was not proposing to use include files, although I think they are a
>>> powerful tool that should not be dismissed.
>> include derectives do not fit into the concepts of a ini file which is
>> KVP broken into sections.
>> It adds special syntax, special parsing etc.
>> It also implies that someone have to add the include directive into the
>> main config file. And it is hard to do it automatically. Another problem
>> is that it would be position dependent as the file with includes will be
>> treated as a single stream of configuration (you just dive into the
>> included files recursively) with this you loose the control that you can
>> get with the explicit merge.
> Again I am not advocating for doing it now, or as part of this merge
> proposal, just do not dismiss it, we use it in krb5.conf and it has
> proven to be quite powerful.

I am not dismissing it. I carefully evaluated it and found flaws. :-)
I have been thinking about it since the ticket was open three years or 
so ago!
>
>> Keep in mind that merge is already possible in INI. The whole proposal
>> is about starting to use thins functionality not about actually building it.
> I know, and I am not proposing actual change, just additional
> limitations, and a slightly more conservative way of using it than you
> thought was needed, including a change in SSSD's configuration to cope
> better.

Let us separate the two. Making SSSD more restrictive should be 
independent of what the library should provide.

>
>>>>> 3) The question about merges comes again and again and IMO it is the
>>>>> time to get serious about it.
>>>>> I explicitly said that there are several rules that would prevent issues
>>>>> that you are concerned about.
>>>>> a) There is not overwrite between snippets so any collision would cause
>>>>> an error. It will be recorded and snippet will be ignored.
>>> Nod
>>>
>>>>> b) We can force the snippets to have sections though strictly it is not
>>>>> required but we can do it if people think that it is 100% necessary.
>>> What would do you do if there is no section in sssd's case ?
>>> Where would directives end up ?
>> If there is no section defined and there is not .merge file that
>> corresponds to this snippet with section key defined then ther snippet
>> will be ignored and an error will be logged.
>> ...
> On the .merge file, I do not want to depend on that in SSSD, at all.
> It's part of the reason I am proposing a slightly different (and IMO
> better) approach.

OK. Let us decouple this. All the other points actually not that 
important :-)
This is the real one. I have my concerns about the .merge files too.
Let us have a conversation about .merge files in a separate thread. I 
think it is the weakest part of the proposal. I just do not see how I 
can get rid of them but it has nothing to do with the points discussed 
here thus I will start a separate thread about it.


>
>> section = <name>
>>
>> */section/*- optional, default name of the section
>>    to use if the file does not have a section defined and just has KVPs as
>>    in the example above.
>>
>> The rule here is that the .merge file is mandatory so the snippet itself
>> might be just KVPs.
> I think making a .merge file mandatory is a deal breaker, it's not very
> usable to have to have multiple files. From an admin point of view it is
> not a good design. An admin just want to drop a small config snippet. It
> should be a self confined configuration file, multiple files are bad,
> they tend to get out of sync, forgotten etc..

Nod. If we can overcome the concerns I had in mind I am all for them 
doing away.
See other thread.

>
> If you really *need* additional control directives (I do not think you
> do, the basic functionality can be coded w/o any) then I would rather
> have a [merge] section at the top of the snippet file that defines how
> it should be interpreted. It will make "merge" a special section, but it
> is a much better approach than having multiple files.

May be. I am not sure with this one. You really blend two things 
together. We can strip the merge section though...
But let us talk in a separate thread.


>
>> A bit less of work for any script becuase you do not need to generate
>> the section line, just put KVPs both in snippet and .merge file.
> I honeslty think .merge files are a bad idea, at least for SSSD and
> GSS-Proxy cases. I really think we should avoid them completely.

+1.


>
>>> I do not understand how it helps, for the specific sssd case I think it
>>> only has downsides.
>>>
>>> a) You have to define an additional [global] section in order to define
>>> the domain list with the snippet you are deploying, which means you have
>>> to special case the global section and allow section merging, but
>>> disallow it for everything else.
>> No I think merge logic can check that
>>
>> mergesec = <section>
>> mergekey = <key>
>>
>> are not present in the merge set, if any of snippets tries to overwrite
>> this section then it will be an error. It has nothing to do with special
>> casing, it can be a generic rule enforced by the library.
>> It actually makes perfect sense to check and enforce.
>> I would need to add this to the design.
>> Very good point, thanks for bringing it up!
> I still think it is a very bad idea for SSSD, and other projects we
> have, I am not against it being a feature in the library, as long as it
> can be disabled by SSSD.

This is SSSD choice.
I think we can use it until SSSD makes the change you propose.

>
>>> b) An additional global section can end up defining other values we'd
>>> rather not merge from a snippet, but you wouldn't know how to
>>> "blacklist" values unless we deploy yet another meta-configuration
>>> file(I am strongly opposed to meta-configuration files).
>> No black listing. This is in fact will lead to insanity.
>> The rule is simple if a set of snippets contains a Key that needs
>> merging of values into this single list then the same set of snippets
>> can't contain a snippet that updates that section. I think it should be
>> treated as a global misconfiguration, reported and no snippets should be
>> applied in this case.
> I think merging values is a bad idea, as said previously, so won't argue
> more on its semantics.
>
>>> c) it will be harder for an admin to visualize how to enable/disable a
>>> domain.
>>>
>>> A key called enabled = yes|no*in*  the section, would avoid all 3 above,
>>> no need to merge global sections, no additional values, and easier to
>>> understand if a domain is enabled or not.
>>>
>>> And the same is for the providers.
>> Admin can just drop and snippet and follow the rules defined above.
>> I think it would be easy to visualize.
> Agree to disagree.
>
>> I think enable/disable is a good idea but it is for SSSD to implement.
>> INI does not have a say here.
> Yes, this is not about what INI offers, but about what we should use in
> SSSD from the features set you are offering.
>
>
>>>>> Bottom line:
>>>>> We have use case that require merges, we have SSSD with its semantics.
>>>>> The proposal covers both cases. If you think that sub dirs is a bad is a
>>>>> bad idea I will budge but I think it is a mistake.
>>> I think a conf.d subdir is a great idea.
>> You mean subdirectories under conf.d dir so that snippets from one
>> source do not have access to snippets from another source and can be
>> sort of jailed in that subdirectory? Is that what you mean?
> No, we should have just this:
> /etc/sssd/sssd.conf
> /etc/sssd/conf.d/mydomain.conf
> /etc/sssd/conf.d/other-domain.conf
>
> Also only files ending in .conf should be evaluated so that a
> domain.conf.rpmsave/.bak/.old/etc.. file would be ignored.
>
> Anything more that this becomes too complex and tips the usability
> balance.

Sigh. Too limiting IMO. What a waste of great powers of INI library!
I see you think it is too over-engineered, may be. It is created to 
address current state without requiring a change.
I want to hear from Jakub on how hard it would be to make the change you 
propose.
If it is simple as you say I will drop this part from the design 
considerations and will assume we are not required to merge the values 
at all.


>
>>> But I think merging sections is a bad idea, I would prefer to have only
>>> whole sections defined in conf.d and conflicts always be ignored w/o any
>>> exception for [global].
>> I agree. Absolutely and I think the design does exactly that. Each
>> snippet is treated as section, though its name can be recorded in the
>> corresponding .merge file.
> .merge files are a bad idea to me :-)
>
>> And as I described above there is not need to black list anything.
> no, there is no need, it was an hypothetical, in my view SSSDs snippets
> *must* always have a section name as they can only be new services or
> new domains and nothing else, and conflicting sections should simply be
> flagged and prominently reported as an error and ignored/renamed
> (perhaps returned in a separate namespace by the library in case the
> calling program wants to inspect them).
>
>>> I think merging values is generally a pretty bad idea, because you have
>>> no place to see what the final resulting value is except by cranking up
>>> sssd's log level and making it spit out the resulting config value.
>> I agree that merging values is generally bad. This is why we will check
>> sections for collisions.
>> But I think that adding a value to the list is not that bad.
> I do not think we want to depend on it in SSSD though, I can see various
> corner cases were this would lead to unexpected results, I find it much
> safer to put an 'enabled = yes|no' key in the various sections.
>
>>> I strongly prefer adding an enabled = yes|no parameter (it would be
>>> optional for backwards compatibility for the main config file, and
>>> required for the snippets).
>> This is up to sssd.
> Indeed, just communicating how I think we should use these features, in
> case the feedback is valuable for INI too and not to build wrong
> expectations.
>
>> However you forget something. The change you propose would require
>> significant semantic changes to the config file.
> No it would not. It is simple to even maintain backwards compatibility.
> All SSSD needs to do is to take the "legacy" list use it to reorder the
> domain defined there first (if they are not already ordered that way)
> and drop in their definition an enabled = yes key.
>
> Then process all of them as if the directive was never there and the
> admin actually rewrote the config file according to the new paradigm.
>
>> It would require a different traversal algorythim related to the
>> configuration in SSSD. Right now one has to just get the list of domains
>> or providers from a specific section and a specific key (pseudocode):
>>
>> Value = getKeyForSection(Section,Key)
>> List = convertToList(Value)
>>
>> for each Item in List
>>      Domain = getSection(Item)
>>      processDomain(Domain)
>>
>> In the case of the approach you suggest
>>
>> Value = getKeyForSection(Section,Key)
>> List = convertToList(Value)
>>
>> Sections = getListOfSections(Config)
>>
>> for each Sec in Section
>>       If Sec is Domain
>>           LIst+= Sec
>>
>> for each Item in List
>>      Domain = getSection(Item)
>>      processDomain(Domain)
>
> It is actually simpler:
>
> Sections = getListOfSections(Config)
> for Sec in Section:
>     if Sec.is_enabled():
>         processDomain(Sec)
>
> (excuse my pythonism, my head is in there right now :-)
>
> This after running the compatibility code I mentioned above to fixup the
> sections.
>
>> So you would have to parse the config and examine each an every section
>> to build the list anyways. But INI will do it for you.
> I know INI could do it for SSSD, I just disagree it is appropriate.
>
>>   The benefits are
>> obvious:
>> a) You do not need to change SSSD and its logic of processing data
>> b) Same approach can be used in other applications and it is OK to
>> follow SSSD example :-) . INI will do the change once and it can be reused.
> and the cons are:
> A) configuration more complex and confusing
> B) Same approach can be used by other applications, making them as
> bad :)
>
>> And keep in mind that the change to merge values will be to the in
>> memory object not to the text of the INI file itself, that would be
>> insane for sure :-)
> Sure.
> In the end it is just a matter of perspective. I do understand that this
> functionality looks "cool" from INI pov, it is just "too smart for its
> own good" in my opinion. In other words: over-engineered :)
>
> Just like trying to use a database only for config in SSSD was too smart
> when we started. We quickly got forced to instead go with config files,
> because, even though the db was more powerful it was just too
> complex/alien/difficult to manage for admins. I am just applying the
> same principle to this cofig expansion here, after learning the
> lesson :)
>
> Simo.
>
Now that we argued about less important things let us get to the core of 
the problem i.e. .merge files which in fact the concern.

-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IdM portfolio
Red Hat, Inc.




More information about the sssd-devel mailing list