Open Stack

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?

> 
> 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

[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.

Doug