Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
-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 d...@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
Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
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.
Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
On Wed, 2015-04-22 at 08:49 -0400, Doug Hellmann wrote: 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. I have been thinking that I should point people to the policies package[1] I built, and this DSL comment has finally pushed me over the edge :) It's a complete clean-room implementation with a completely different syntax than oslo.policy, so I don't know how interested anyone here would be in using it, but there it is. (I have it licensed under the GPL right now, but if anyone wants to use it, I'd be happy to relicense under Apache…) [1] https://pypi.python.org/pypi/policies -- Kevin L. Mitchell kevin.mitch...@rackspace.com Rackspace __ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
On 22 April 2015 at 14:49, Doug Hellmann d...@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,
Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
Excerpts from Salvatore Orlando's message of 2015-04-22 23:10:01 +0200: On 22 April 2015 at 14:49, Doug Hellmann d...@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:
Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 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. 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
Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
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
Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
On 04/17/2015 08:45 AM, Ihar Hrachyshka wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 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 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] If we make the checks public, is that sufficient? I have no problem with most of these being public. The only one that is Keystone specific is the Role check, which I would like to rework. Instead of RoleCheck, can you build on top of the newly submitted ability to do List checks? https://review.openstack.org/#/c/169045/3/oslo_policy/_checks.py,cm Rule, And, Or, Not and Generic checks are building blocks for other logic, and should be public. == 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]]. If attribute is present in a policy entry, it applies to target iff attribute is set, and 'enforce_policy' is set in attribute map for the attribute in question, and target is not read-only (=its name does not start from get_). If subattribute is present, the rule applies to target if 'validate' is set in attribute map for the attribute, and its type is dict, plus all the requirements for :attribute described above. Note that those rules are *aggregated* into single matching rule with AndCheck, so f.e. if action is create_network, and provider is set in target, then the actual rule validated would be all the rules for create_network, create_network:provider, and e.g. create_network:provider:physical_network, joined into single rule with AndCheck (meaning, target should conform to all of those requirements). This stands for a significant extension of original oslo.policy intent. Originally, I thought
Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 04/17/2015 07:49 PM, Salvatore Orlando wrote: == 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.) Roles are normally populated by the keystone middleware. I'm not sure whether we want to drop them altogether from the context, but I would expect the policy engine to use them even after switching to oslo.policy - Plugins should never leverage this kind of information. I am indeede tempted to drop roles and other AAA-related info from the context when it's dispatched to the plugin. This should be done carefully - beyond breaking some plugins it might also impact the notifiers we use to communicate with nova. The function which artificially populates roles in the context was built for artificial contextes, which in some cases were created by plugins performing db operations at startup. I would check if we still have this requirement, and if not remove the function. Ouch, I failed in wording above (ETOOMANYWORDS?). I only meant dropping explicit admin role population from the context object. If there are any reasons to drop the whole attribute, they are irrelevant to oslo.policy adoption discussion, and are worth a separate thread, if at all. Thanks for keeping me honest on the non-sense above! == 3. aggregating core, attribute and subattribute policies == [...] Policies on subattributes are an abomination of nature and they should go. Not sure they can easily go now, without breaking existing setups. I wouldn't require existing deployments to convert their policies unless we are completely locked otherwise. The problem however is that this needs first a rethink about some API extensions - namely the one for external gateway modes. However, as you say we can't reablockedlly do without policies on attributes at the moment. Policies like the following: create_subnetpool:shared: rule:admin_only Led us to implement [1], which uses the symbols which were now made private. You probably forgot to define [1] in your email. At least [1] in the original email seems irrelevant to attribute matching in neutron. That logic is specific for Neutron, which adds semantic value to the policy target. As Ihar says, imposing this on all the other projects might not be welcome and in some cases break the project themselves. 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. Making this possilbe would be the quickest path for Neutron. However if the oslo_policy team took this decision it must have been for a solid design reasoning. It is tough to ask to revise a design decision for a single user of a library. Unfortunately a particular Neutron developer took the liberty of playing with authZ policies - I bet he was even proud of what he did: leaving the project with more technical debt. That particular developer should be dealt with appropriately, but this is another story. Do we need a separate neutron-troika gerrit group to handle those cases? I think that the only alternative to making those symbols public in oslo_policy is for Neutron to perform attribute and sub-attribute authZ checks in a different fashion (perhaps an additional engine). This will be a backward
Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
On 20 April 2015 at 10:03, Ihar Hrachyshka ihrac...@redhat.com wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 04/17/2015 07:49 PM, Salvatore Orlando wrote: == 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.) Roles are normally populated by the keystone middleware. I'm not sure whether we want to drop them altogether from the context, but I would expect the policy engine to use them even after switching to oslo.policy - Plugins should never leverage this kind of information. I am indeede tempted to drop roles and other AAA-related info from the context when it's dispatched to the plugin. This should be done carefully - beyond breaking some plugins it might also impact the notifiers we use to communicate with nova. The function which artificially populates roles in the context was built for artificial contextes, which in some cases were created by plugins performing db operations at startup. I would check if we still have this requirement, and if not remove the function. Ouch, I failed in wording above (ETOOMANYWORDS?). I only meant dropping explicit admin role population from the context object. If there are any reasons to drop the whole attribute, they are irrelevant to oslo.policy adoption discussion, and are worth a separate thread, if at all. Thanks for keeping me honest on the non-sense above! Nah it was me being ambiguous in my reply. We need roles in the context. Otherwise most policy checks would not be applicable anymore. I was making a point that a function that loads and stores them in context generated with operation like get_admin_context or ContextBase.elevated is not needed anymore. Indeed there is a note in the code pointing that out [1]. And here's the patch for doing it [2] (needs some more work) [1] http://git.openstack.org/cgit/openstack/neutron/tree/neutron/policy.py#n472 [2] https://review.openstack.org/#/c/175238/1 == 3. aggregating core, attribute and subattribute policies == [...] Policies on subattributes are an abomination of nature and they should go. Not sure they can easily go now, without breaking existing setups. I wouldn't require existing deployments to convert their policies unless we are completely locked otherwise. Sure. That was just the personal opinion of the developer who was asked to introduce them. The problem however is that this needs first a rethink about some API extensions - namely the one for external gateway modes. However, as you say we can't reablockedlly do without policies on attributes at the moment. Policies like the following: create_subnetpool:shared: rule:admin_only Led us to implement [1], which uses the symbols which were now made private. You probably forgot to define [1] in your email. At least [1] in the original email seems irrelevant to attribute matching in neutron. I was pointing out this (no reference to avoid confusion): http://git.openstack.org/cgit/openstack/neutron/tree/neutron/policy.py#n152 That logic is specific for Neutron, which adds semantic value to the policy target. As Ihar says, imposing this on all the other projects might not be welcome and in some cases break the project themselves. 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
[openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 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 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]]. If attribute is present in a policy entry, it applies to target iff attribute is set, and 'enforce_policy' is set in attribute map for the attribute in question, and target is not read-only (=its name does not start from get_). If subattribute is present, the rule applies to target if 'validate' is set in attribute map for the attribute, and its type is dict, plus all the requirements for :attribute described above. Note that those rules are *aggregated* into single matching rule with AndCheck, so f.e. if action is create_network, and provider is set in target, then the actual rule validated would be all the rules for create_network, create_network:provider, and e.g. create_network:provider:physical_network, joined into single rule with AndCheck (meaning, target should conform to all of those requirements). This stands for a significant extension of original oslo.policy intent. Originally, I thought that we would be able to introduce neutron policy semantics into oslo.policy, and just switch to it once it's there. But there is a problem with that approach. Other projects (like nova [5]) already use similar syntax for their policy targets, while not putting such semantics on top of what oslo.policy provides (which is basically nothing, for target is not interpreted in any special way). AFAIU the way those projects use this syntax does not introduce any new *meaning*, so it's used for mere convenience to
Re: [openstack-dev] [oslo][policy][neutron] oslo.policy API is not powerful enough to switch Neutron to it
Thanks for this analysis Ihar. Some comments inline. On 17 April 2015 at 14:45, Ihar Hrachyshka ihrac...@redhat.com wrote: -BEGIN PGP SIGNED MESSAGE- Hash: SHA1 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 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. Definitely not a blocker - we could as you suggest, or removing that logging altogether. == 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.) Roles are normally populated by the keystone middleware. I'm not sure whether we want to drop them altogether from the context, but I would expect the policy engine to use them even after switching to oslo.policy - Plugins should never leverage this kind of information. I am indeede tempted to drop roles and other AAA-related info from the context when it's dispatched to the plugin. This should be done carefully - beyond breaking some plugins it might also impact the notifiers we use to communicate with nova. The function which artificially populates roles in the context was built for artificial contextes, which in some cases were created by plugins performing db operations at startup. I would check if we still have this requirement, and if not remove the function. == 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]]. If attribute is present in a policy entry, it applies to target iff attribute is set, and 'enforce_policy' is set in attribute map for the attribute in question, and target is not read-only (=its name does not start from get_). If subattribute is present, the rule applies to target if 'validate' is set in attribute map for the attribute, and its type is dict,
