Open Stack

On 09/18/2015 03:33 PM, John Griffith wrote:
> On Fri, Sep 18, 2015 at 12:52 PM, Eric Harney <eharney at redhat.com> wrote:
> 
>> On 09/18/2015 01:01 PM, John Griffith wrote:
>>> On Fri, Sep 18, 2015 at 9:06 AM, Chris Friesen <
>> chris.friesen at windriver.com>
>>> wrote:
>>>
>>>> On 09/18/2015 06:57 AM, Eric Harney wrote:
>>>>
>>>>> On 09/17/2015 06:06 PM, John Griffith wrote:
>>>>>
>>>>
>>>> Having the "global conf" settings intermixed with the backend sections
>>>>>> caused a number of issues when we first started working on this.
>> That's
>>>>>> part of why we require the "self.configuration" usage all over in the
>>>>>> drivers.  Each driver instantiation is it's own independent entity.
>>>>>>
>>>>>>
>>>>> Yes, each driver instantiation is independent, but that would still be
>>>>> the case if these settings inherited values set in [DEFAULT] when they
>>>>> aren't set in the backend section.
>>>>>
>>>>
>>>> Agreed.  If I explicitly set something in the [DEFAULT] section, that
>>>> should carry through and apply to all the backends unless overridden in
>> the
>>>> backend-specific section.
>>>>
>>>> Chris
>>>>
>>>>
>>> Meh I don't know about the "have to modify the code", the config file
>> works
>>> you just need to add that line to your driver section and configure the
>>> backend correctly.
>>>
>>
>> My point is that there doesn't seem to be a justification for "you just
>> need to add that line to your driver section", which seems to counter
>> what most people's expectation would be.
>>
> ​There certainly is, I don't want to force the same options against all
> backends.  Perfect example is the issues with some distros in the past that
> DID use global settings and stomp over any driver; which in turn broke
> those that weren't compatible with that conf setting even though in the
> driver section they overrode it.​
> 
> 
>>
>> People can and do fail to do that, because they assume that [DEFAULT]
>> settings are treated as defaults.
>>
> 
> ​Bad assumption, we should probably document this until we fix it (making a
> very large assumption that we'll ever agree on how to fix it).​
> 
>>
>> To help people who make that assumption, yes, you have to modify the
>> code, because the code supplies a default value that you cannot supply
>> in the same way via config files.
>>
> 
> ​Or you could just fill out the config file properly:
>     [lvm-1]
>     iscsi_helper = lioadm
> 
> I didn't have to modify any code.
> ​
> 
> 

In the use case I was describing, I'm shipping a package, as a
distribution, with a default configuration file. The deployer (not me)
is the only one that knows about config sections that they want for
multi-backend. I don't think it's fair to require them to fill out
things like iscsi_helper, because there is only one correct value for
iscsi_helper on the platform I support, and defaulting to a different
one is not useful.

The fact that we don't inherit [DEFAULT] settings means that it is not
possible for me to ship a package with the correct defaults without
changing the hard-coded default value, in the code, to customize it for
my platform. I want to set iscsi_helper = lioadm in a configuration file
and have that be the default for any enabled_backend.


>>
>>> Regardless, I see your point (but I still certainly don't agree that it's
>>> "blatantly wrong").
>>>
>>
>> You can substitute "very confusing" for "blatantly wrong" but I think
>> those are about the same thing when talking about usability issues with
>> how to configure a service.
>>
> 
> ​Fair enough.  Call it whatever you like.​
> 
> 
>>
>> Look at options like:
>>  - strict_ssh_host_key_policy
>>  - sio_verify_server_certificate
>>  - driver_ssl_cert_verify
> 
> 
>> All of these default to False, and if turned on, enable protections
>> against MITM attacks.  All of them also fail to turn on for the relevant
>> drivers if set in [DEFAULT].  These should, if set in DEFAULT when using
>> multi-backend, issue a warning so the admin knows that they are not
>> getting the intended security guarantees.  Instead, nothing happens and
>> Cinder and the storage works.  Confusion is dangerous.
>>
> 
> ​Yeah, so is crappy documentation lack of understanding.​
> 
> 

I can't make my customers read documentation and test them for
understanding.  I can make software that's more robust and less prone to
misuse.  Warning people with "hey, you're using multi-backend and have
set this security-related option in a section where it will never have
an effect in your deployment" is one way to do this that we could do today.

>>
>>> Bottom line "yes", ideally in the case of drivers we would check
>>> global/default setting, and then override it if something was provided in
>>> the driver specific setting, or if the driver itself set a different
>>> default.  That seems like the right way to be doing it anyway.  I've
>> looked
>>> at that a bit this morning, the issue is that currently we don't even
>> pass
>>> any of those higher level conf settings in to the drivers init methods
>>> anywhere.  Need to figure out how to change that, then it should be a
>>> relatively simple fix.
>>>
>>
>> What I was getting at earlier though, is that I'm not really sure there
>> is a simple fix.  It may be simple to change the behavior to more
>> predictable behavior, but doing that in a way that doesn't introduce
>> upgrade problems for deployments relying on the current defaults seems
>> difficult to me.
>>
> ​Agreed, but honestly I'd like to at least try.  Especially when people use
> terms like "blatantly wrong" and "dangerous", kinda prompts one to think ​
> 
> ​that perhaps it should be looked at.  If nothing else, we shouldn't have
> driver settings in the DEFAULT section, we should just create a driver
> section, but we still need to figure out how to deal with things outside of
> the "general" section vs the backend stanza.
> 

Yeah, it should be looked at, that's why I'm talking about all of this...

Moving settings to a drivers section sounds like a good start but it
doesn't fix the issue I'm talking about without also changing how the
inheritance works.

> Also, I'd argue that the behavior your arguing for is MORE dangerous and
> troublesome.  The LIO being in the global CONF was a perfect example where
> it broke third party devices on a specific distro because it assumed that
> EVERYTHING on the system was using the lio methods and in that case you
> really couldn't do anything but modify code.
> 

I don't really know how to parse this, I was talking about dangerous
software behavior, I think you're talking about dangerous code changes?

When LIO originally landed, all we had was global conf, so I'm not sure
what you're getting at.  If the conf model was wrong for LIO, it was/is
wrong for dozens of other driver-specific options too.

If you're referring to https://bugs.launchpad.net/cinder/+bug/1400804 ,
all I can really say about that is, yeah, that kind of thing was more
likely to happen back then..?  That seems orthogonal other than the fact
that it's config-related code that I worked on once.

I don't really follow what I'm supposed to conclude about my current
proposal in that context...

> I've pinged you a number of times on IRC, maybe we can chat there a bit
> real-time and see if we can work together on a solution?
> 
> Thanks,
> John​
>