Open Stack

Excerpts from Ihar Hrachyshka's message of 2015-04-22 12:33:52 +0200:
> On 04/22/2015 05:01 AM, Doug Hellmann wrote:
> > Excerpts from Ihar Hrachyshka's message of 2015-04-17 14:45:58
> > +0200:
> >> Hi,
> >> 
> >> tl;dr neutron has special semantics for policy targets that
> >> relies on private symbols from oslo.policy, and it's impossible
> >> to introduce this semantics into oslo.policy itself due to
> >> backwards compatibility concerns, meaning we need to expose some
> >> more symbols as part of public API for the library to facilitate
> >> neutron switch to it.
> >> 
> >> ===
> >> 
> >> oslo.policy was graduated during Kilo [1]. Neutron considered
> >> the switch to it [2], but failed to achieve it because some
> >> library symbols that were originally public (or at least looked
> >> like public) in policy.py from oslo-incubator, became private in
> >> oslo.policy. Specifically, Neutron policy code [3] relies on the
> >> following symbols that are now hidden inside oslo_policy._checks
> >> (note the underscore in the name of the module that suggests we
> >> cannot use the module directly):
> >> 
> >> - - RoleCheck - - RuleCheck - - AndCheck
> > 
> > I'm not sure I followed all of the rest of what you wrote below,
> > but it seems like this is the real problem. We are pretty
> > aggressive about making things private when we release a new
> > library, because it's easier to make them public later if we need
> > to than it is to make public things private.
> > 
> > In this case, it looks like we made some symbols private even
> > though they were already being used, and that seems like a mistake
> > on our part. So, if we make those public, would that address the
> > issues with neutron adopting the library?
> > 
> 
> Yes, that would help. I will also check Adam's suggestion, maybe it
> will allow us to avoid exposing RoleCheck.

Keeping stuff private is less of a priority if we can demonstrate
that having it public makes it more useful.

> >> Those symbols are used for the following matters: (all the
> >> relevant neutron code is in neutron/policy.py)
> >> 
> >> 1. debug logging in case policy does not authorize an action 
> >> (RuleCheck, AndCheck) [log_rule_list]
> >> 
> >> 2. filling in admin context with admin roles (RoleCheck,
> >> RuleCheck, AndCheck/OrCheck internals) [get_admin_roles]
> >> 
> >> 3. aggregating core, attribute and subattribute policies
> >> (RuleCheck, AndCheck) [_prepare_check]
> >> 
> >> 
> >> == 1. debug logging in case policy does not authorize an action
> >> ==
> >> 
> >> Neutron logs rules that failed to match if policy module does
> >> not authorize an action. Not sure whether Neutron developers
> >> really want to have those debug logs, and whether we cannot just
> >> kill them to avoid this specific usage of private symbols; though
> >> it also seems that we could easily use __str__ that is present
> >> for all types of Checks instead. So it does not look like a
> >> blocker for the switch.
> >> 
> >> 
> >> == 2. filling in admin context with admin roles ==
> >> 
> >> Admin context object is filled with .roles attribute that is a
> >> list of roles considered granting admin permissions [4]. The
> >> attribute would then be used by plugins that would like to do
> >> explicit policy checks. As per Salvatore, this attribute can
> >> probably be dropped now that all plugins and services don't rely
> >> on it (Salvatore mentioned lbaas mixins as the ones that
> >> previously relied on it, but are now not doing it since service
> >> split from neutron tree (?)).
> >> 
> >> The problem with dropping the .roles attribute from context
> >> object in Liberty is that we, as a responsible upstream with lots
> >> of plugins maintained out-of-tree (see the ongoing vendor
> >> decomposition effort) would need to support the attribute while
> >> it's marked as deprecated for at least one cycle, meaning that if
> >> we don't get those oslo.policy internals we rely on in Liberty,
> >> we would need to postpone the switch till Mizzle, or rely on
> >> private symbols during the switch (while a new release of
> >> oslo.policy can easily break us).
> >> 
> >> (BTW the code to extract admin roles is not really robust and
> >> has bugs, f.e. it does not handle AndChecks that could be used
> >> in context_is_admin. In theory, 'and' syntax would mean that both
> >> roles are needed to claim someone is an admin, while the code to
> >> extract admin roles handles 'and' the same way as 'or'. For the
> >> deprecation time being, we may need to document this
> >> limitation.)
> >> 
> >> 
> >> == 3. aggregating core, attribute and subattribute policies ==
> >> 
> >> That's the most interesting issue.
> >> 
> >> For oslo.policy, policies are described as "target: rule", where
> >> rule is interpreted as per registered checks, while target is
> >> opaque to the library.
> >> 
> >> Neutron extended the syntax for target as: 
> >> target[:attribute[:subattribute]].
> > 
> > This extension of the syntax is a bit more problematic. We should 
> > talk about a way to fold that into the library so it can be used 
> > consistently across projects. FTR, making it less easy to extend 
> > common behaviors in application-specific ways is one reason we
> > like to make symbols private whenever possible.
> > 
> > Although it is not well-documented, we do support chained
> > attribute names on the right-side of the expression [1]. Does that
> > do what you are describing here, or do we need to extend it to
> > support a similar syntax on the left side of the expression.
> > 
> > [1]
> > http://git.openstack.org/cgit/openstack/oslo.policy/tree/oslo_policy/_
> checks.py#n288
> 
> The
> > 
> gut of the neutron 'feature' is: applying multiple separate
> policies for the same target based on which attributes are set for the
> target. As I described in the original email, there are existing
> consumers of the library that would be broken if we add the feature to
> oslo.policy (at least with the existing syntax; we could think of
> distinguishing it somehow with new syntax rules).

That feature sounds like it could be useful outside of neutron, so let's
see if we can come up with a new syntax to make it portable. Bonus
points if the new syntax results in a proper DSL.

> 
> > 
> > [snip]
> > 
> >> So the question to oslo.policy maintainers is: whether all that
> >> is said above makes sense, and if so, whether we may now
> >> consider exposing those private symbols (+ maybe OrCheck,
> >> NotCheck, and other primitives that are logically bound to
> >> AndCheck) as part of public API. If community agrees with my
> >> analysis and justification for the change, I am happy to propose
> >> a patch that would do just that.
> > 
> > I think so, yes.
> > 
> >> PS: one more question to oslo.policy maintainers is whether
> >> consuming projects may rely on Enforcer.rules attribute being
> >> present, and following dict semantics, as it currently does.
> >> Neutron and other projects (like Nova) relies on the attribute,
> >> while it's not really documented in official library
> >> documentation. We may need clarification on whether its usage is
> >> supported, and document it appropriately.
> > 
> > I think it's a really really bad idea for any project to be
> > building application-specific behavior on top of the policy engine.
> > App-specific *policies* make sense. But making the evaluation of
> > policy rules different across the applications largely defeats the
> > purpose of having a common language for expressing those rules in
> > the first place. Come contribute changes to oslo.policy if it
> > doesn't do what you need. Oslo was started to provide a place for
> > otherwise disparate teams to collaborate. We should do more of
> > that.
> > 
> 
> I agree, the problem is that neutron is already in that position where
> we have the feature implemented in a way that would not allow to move
> it to oslo.policy without breaking someone (either neutron existing
> users or other consumers of the library).
> 
> Long term, we can consider adding some new distinguished syntax for
> attribute matching. Neutron would then switch to it, supporting both
> for a cycle or two, and then dropping the existing one.

Yes, certainly, we need to take the current state of the world into
account. My point was just that you don't have to solve this problem on
your own, in isolation.

Doug