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".
>> > 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).
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.
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.
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.
>> >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.
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.
>> >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.
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..
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.
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.
> 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.
> 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.
> 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.
--
Simo Sorce * Red Hat, Inc * New York