[Freeipa-devel] [PATCH] 0012 Modify existing SSSD configuration instead of dropping it

[Rob Crittenden]

Alexander Bokovoy wrote:
> On Tue, 13 Sep 2011, Stephen Gallagher wrote:
>
>> On Tue, 2011-09-13 at 16:33 +0300, Alexander Bokovoy wrote:
>>> On Tue, 13 Sep 2011, Stephen Gallagher wrote:
>>>>>>    File "/usr/lib/python2.7/site-packages/SSSDConfig.py", line 1207, in import_config
>>>>>>      fd = open(configfile, 'r')
>>>>>> IOError: [Errno 2] No such file or directory: '/etc/sssd/sssd.conf'
>>>>> Right, we need to fallback to new sssd.conf in case of any exception,
>>>>> not only for ParsingError.
>>>> Actually, that's not necessarily true. Do we want to fall back on
>>>> permission error, for instance? This could result in clobbering an
>>>> existing file (if for example the existing sssd.conf's SELinux context
>>>> is wrong, preventing reading, but when we create a new one and save it
>>>> in place later we have the right context and it replaces the old one).
>>> Let's define what we want to see here.
>>>
>>> 1. There is no sssd.conf ->  create new one (unlikely for existing SSSD
>>> installation -- if we went to this path, we already found SSSD
>>> installed)
>>
>> Actually, this is fairly likely in future releases. There's so much
>> noise in the example configuration that I've decided that we're going to
>> install it as %doc instead of %config. So a raw installation of the SSSD
>> package will have no sssd.conf in place. We obviously need to consider
>> this.
>>
>>> 2. There is sssd.conf ->  modify existing one
>>>     2.1. Can't open for write ->  report error
>>
>> Agreed.
>>
>>>     2.2. Can't open and read due to parsing error ->  create new one
>>> ...
>>
>> There are two issues here. Failure to open for reading and ParseError on
>> read. That said, I think they should be handled the same way (see
>> below).
>>
>>
>> Unfortunately, we have additional issues here... The SSSDConfig API is
>> more strict with options than the SSSD itself is. Specifically, the SSSD
>> will ignore unknown options entirely, but the SSSDConfig will through a
>> ParseError on unknown options.
>>
>> The long-term goal is for SSSD to be more strict about this, but we
>> don't currently have a way to do this.
>>
>> So as I see it, we have three choices for dealing with ParseError:
>>
>> 1) Back up the existing sssd.conf and replace it with a completely new
>> one for FreeIPA. Pros: sssd.conf is guaranteed to parse cleanly. FreeIPA
>> client install completes successfully. Cons: existing configuration
>> (which may have worked) is not preserved.
>>
>> 2) Be restrictive on ParseError and throw an error telling them to fix
>> their config file. Pros: we don't break an existing setup. Cons: FreeIPA
>> installation has been broken.
>>
>> 3) Default to one of the above but provide a command-line flag to behave
>> the other way. This is probably our best bet. I'd suggest defaulting to
>> replacing the config file on ParseError (with a loud message at the END
>> of ipa-client-install pointing to the backed-up file).
> Attached patch tries to implement these ideas. It is untested as I
> wanted to get feedback on the approach first.
>

Well, in the "generate new file" option I think the output is a bit 
misleading.

+        print "New SSSD config will be generated. The old one is backed 
up and can be restored during uninstall"

There could have been no existing sssd.conf, right?

+        logging.error("Failed to parse SSSD configuration and will 
generate new one")

This could imply that an error occurred when in fact there just was no 
sssd.conf to import.

Otherwise the approach looks good.

rob