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.


>  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. 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. 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.
IMO it is not worth it.


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

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.


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





> c) There was a use case when person actually wanted to replace a 
> section. In case of SSSD we probably do not want that but library should 
> allow it. Merge logic already supports this so it is a question of a 
> flag passed to an existing call - no extra code or complexity.
> In case of SSSD the rule would be (at least by default for now) that 
> sections can only be unique and can can be added with no overwrite. Any 
> colliding snippet that collides with a section in the main file will be 
> recorded and ignored because SSSD should be able to start and work 
> without the augmentations.
Nod

> d) I actually think that merging values, i.e. adding a value to the list 
> like in the list of the domains or providers is not a bad thing. It will 
> allow us to avoid interpreting what each section is about by 
> introspecting it. It will also avoid having stale sections that actually 
> do not have any meaning or value. Moving forward when we have syntax 
> validation we will be able to define and validate that the desired key 
> and section are actually a list that is extensible. I had this in mind 
> but this is outside of scope of the current proposal.
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!


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.

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.
I think enable/disable is a good idea but it is for SSSD to implement. INI does not have a say here.



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

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.
And as I described above there is not need to black list anything.


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

However you forget something. The change you propose would require significant semantic changes to the config file.
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)


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



Simo.


-- 
Thank you,
Dmitri Pal

Sr. Engineering Manager IdM portfolio
Red Hat, Inc.