Open Stack

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

I've sent a patch that makes And, Or, Not, and Rule checks public. As
for RoleCheck, we don't need it anymore since we're going to kill the
code that relied on it.

The patch is: https://review.openstack.org/#/c/176683/

Note that we will need a new library release to start neutron
migration to the library.

As for policy syntax, I'm not sure we should reimplement the wheel
with another DSL. I know, that's a bit crazy, but why not defining
rules in python itself? In that regard, policies pypi package
mentioned in the thread seems to be a step in the right direction
since it reuses (restricted) python syntax.

Ihar

On 04/23/2015 12:17 AM, Doug Hellmann wrote:
> Excerpts from Salvatore Orlando's message of 2015-04-22 23:10:01
> +0200:
>> On 22 April 2015 at 14:49, Doug Hellmann <doug at doughellmann.com>
>> wrote:
>> 
>>> 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_poli
cy/_
>>>>
>>>>> 
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.
>>> 
>> 
>> In defence of Neutron and in partial defence of the developer who
>> did this (who should be shot anyway), this happened sometime back
>> in 2012. At that time oslo-incubator was still pretty much an
>> incubator and library graduation was a thing very far in the
>> future. Should that have happened now, development will surely
>> have occurred straight into oslo_policy (and possibly rejected).
> 
> Sure, I'm not placing blame just pointing out that it wasn't
> necessary at all. The incubated code is *also* meant to be shared.
> That's why we incubate it, to take code written in one app and
> remove those app-specific aspects that make it hard to share. So
> let's move ahead with the idea that we want to bring all of the
> good features into the shared code base, and not worry too much
> about how we got to where we are now.
> 
>> I believe it's now neutron developers' problem to sort out this
>> situation. We surely don't want to implement our own policy
>> engine diverging from oslo_policy. Also, I don't think this kind
>> of "fine grained" checks can be folded into oslo_policy, because
>> of what Ihar said (it might break other consumers), and also
>> probably because oslo_policy was never meant to work this way.
> 
> We may need a new syntax, but there's no reason the feature itself
> would break any other users, is there?
> 
>> Perhaps the way forward is to define a plan to get rid of
>> attribute and sub-attribute level checks performed using the
>> policy engine, and find an alternate way for doing such checks.
>> For instance we might decide to hardcode them, and run them once
>> the "traditional" policy check is performed. The downside is that
>> by hardcoding things that were configurable beforehand we might
>> be breaking some setups. Nevertheless, it sounds like separating
>> the "standard" operation-level check from the "specific"
>> attribute-level check might be the way forward.
>> 
>> Salvatore
>> 
>> 
>> Doug
>>> 
>>> ____________________________________________________________________
______
>>>
>>> 
OpenStack Development Mailing List (not for usage questions)
>>> Unsubscribe:
>>> OpenStack-dev-request at lists.openstack.org?subject:unsubscribe 
>>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>>>
>
>>> 
> ______________________________________________________________________
____
>
> 
OpenStack Development Mailing List (not for usage questions)
> Unsubscribe:
> OpenStack-dev-request at lists.openstack.org?subject:unsubscribe 
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> 
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJVOObkAAoJEC5aWaUY1u57o1UH/0hYjCz0g+uoG/R5GkGrPLsg
PIJhXKUaSrWrlSZ+84xhp1tyG2E4B0dcgk08XWPYE+rat+44QnlFIUPqAvcP6F2h
HlnOnIozwaSzl4IoIcOPo740WNDVRBkAXnvc80GIBSL1YeDTOsmvNoPkpvZ8blde
CeQOHA9fukMO8zRBODbRywdrzNSbaTIBgAQTznWKOPqYbsdA2kf0vwMcAIv+Bi/1
/H8Lr22ciPGFEo9u9m5awrX5HWm5hd7QKWSsDY4Mt33hMZ5B5lVm3si0fBbPvd+t
t07ftWt10/cOzsVyg3HD0lBKE4X5zALzKYAZIOIWP40OL5yJ45CNdor/Re89tM0=
=Xp5N
-----END PGP SIGNATURE-----