Open Stack

On Mon, 2013-12-09 at 11:11 -0600, Ben Nemec wrote:
> On 2013-12-09 10:55, Sean Dague wrote:
> > On 12/09/2013 11:38 AM, Clint Byrum wrote:
> >> Excerpts from Sean Dague's message of 2013-12-09 08:17:45 -0800:
> >>> On 12/06/2013 05:40 PM, Ben Nemec wrote:
> >>>> On 2013-12-06 16:30, Clint Byrum wrote:
> >>>>> Excerpts from Ben Nemec's message of 2013-12-06 13:38:16 -0800:
> >>>>>> 
> >>>>>> 
> >>>>>> On 2013-12-06 15:14, Yuriy Taraday wrote:
> >>>>>> 
> >>>>>>> Hello, Sean.
> >>>>>>> 
> >>>>>>> I get the issue with upgrade path. User doesn't want to update
> >>>>>> config unless one is forced to do so.
> >>>>>>> But introducing code that weakens security and let it stay is an
> >>>>>> unconditionally bad idea.
> >>>>>>> It looks like we have to weigh two evils: having troubles 
> >>>>>>> upgrading
> >>>>>> and lessening security. That's obvious.
> >>>>>>> 
> >>>>>>> Here are my thoughts on what we can do with it:
> >>>>>>> 1. I think we should definitely force user to do appropriate
> >>>>>> configuration to let us use secure ways to do locking.
> >>>>>>> 2. We can wait one release to do so, e.g. issue a deprecation
> >>>>>> warning now and force user to do it the right way later.
> >>>>>>> 3. If we are going to do 2. we should do it in the service that 
> >>>>>>> is
> >>>>>> affected not in the library because library shouldn't track 
> >>>>>> releases
> >>>>>> of an application that uses it. It should do its thing and do it
> >>>>>> right (secure).
> >>>>>>> 
> >>>>>>> So I would suggest to deal with it in Cinder by importing
> >>>>>> 'lock_path' option after parsing configs and issuing a deprecation
> >>>>>> warning and setting it to tempfile.gettempdir() if it is still 
> >>>>>> None.
> >>>>>> 
> >>>>>> This is what Sean's change is doing, but setting lock_path to
> >>>>>> tempfile.gettempdir() is the security concern.
> >>>>> 
> >>>>> Yuriy's suggestion is that we should let Cinder override the config
> >>>>> variable's default with something insecure. Basically only 
> >>>>> deprecate
> >>>>> it in Cinder's world, not oslo's. That makes more sense from a 
> >>>>> library
> >>>>> standpoint as it keeps the library's expected interface stable.
> >>>> 
> >>>> Ah, I see the distinction now.  If we get this split off into
> >>>> oslo.lockutils (which I believe is the plan), that's probably what 
> >>>> we'd
> >>>> have to do.
> >>>> 
> >>>>>> 
> >>>>>> Since there seems to be plenty of resistance to using /tmp by 
> >>>>>> default,
> >>>>>> here is my proposal:
> >>>>>> 
> >>>>>> 1) We make Sean's change to open files in append mode. I think we 
> >>>>>> can
> >>>>>> all agree this is a good thing regardless of any config changes.
> >>>>>> 
> >>>>>> 2) Leave lockutils broken in Icehouse if lock_path is not set, as 
> >>>>>> I
> >>>>>> believe Mark suggested earlier. Log an error if we find that
> >>>>>> configuration. Users will be no worse off than they are today, and 
> >>>>>> if
> >>>>>> they're paying attention they can get the fixed lockutils behavior
> >>>>>> immediately.
> >>>>> 
> >>>>> Broken how? Broken in that it raises an exception, or broken in 
> >>>>> that it
> >>>>> carries a security risk?
> >>>> 
> >>>> Broken as in external locks don't actually lock.  If we fall back to
> >>>> using a local semaphore it might actually be a little better because
> >>>> then at least the locks work within a single process, whereas before
> >>>> there was no locking whatsoever.
> >>> 
> >>> Right, so broken as in "doesn't actually locks, potentially 
> >>> completely
> >>> scrambles the user's data, breaking them forever."
> >>> 
> >> 
> >> Things I'd like to see OpenStack do in the short term, ranked in 
> >> ascending
> >> order of importance:
> >> 
> >> 4) Upgrade smoothly.
> >> 3) Scale.
> >> 2) Help users manage external risks.
> >> 1) Not do what Sean described above.
> >> 
> >> I mean, how can we even suggest silently destroying integrity?
> >> 
> >> I suggest merging Sean's patch and putting a warning in the release
> >> notes that running without setting this will be deprecated in the next
> >> release. If that is what this is preventing this debate should not 
> >> have
> >> happened, and I sincerely apologize for having delayed it. I believe 
> >> my
> >> mistake was assuming this was something far more trivial than "without
> >> this patch we destroy users' data".
> >> 
> >> I thought we were just talking about making upgrades work. :-P
> > 
> > Honestly, I haven't looked exactly how bad the corruption would be. But
> > we are locking to handle something around simultaneous db access in
> > cinder, so I'm going to assume the worst here.
> 
> Yeah, my understanding is that this doesn't come up much in actual use 
> because lock_path is set in most production environments.  Still, 
> obviously not cool when your locks don't lock, which is why we made the 
> unpleasant change to require lock_path.  It wasn't something we did 
> lightly (I even sent something to the list before it merged, although I 
> got no responses at the time).

What would happen if we required each service to set a sane default
here?

e.g. for Nova, would a dir under $state_path work? It just needs to be a
directory that isn't world-writeable but is writeable by whatever user
Nova is running as.

Practically speaking, this just means that Cinder needs to do:

  lockutils.set_defaults(lock_path=os.path.join(CONF.state_path, 'tmp'))

and the current behaviour of lockutils.py is fine.

Hmm, that feels like I'm missing something?

Mark.