Cyclical reference check seems broken

[Ben Nemec]

If I create a policy file with the following rules:

"identity:get_application_credential": "(role:reader and system_scope:all) or rule:owner"
"identity:get_application_credentials": "rule:identity:get_application_credential"

I am getting a warning from oslo.policy about them being part of a cyclical reference. I don't see how that's cyclical though. The latter rule refers to the former, but the former does not reciprocate. Unless somehow rule:owner has a nested reference to identity:get_application_credentials, but I'm pretty sure that's not the case (and if it is: why?).

This is particularly problematic as this pattern is how we handle deprecations in the generated sample policy files, so it's going to be a common thing.

[Ben Nemec (bnemec)]

Oh, actually I was mistaken. If you explicitly put both rules in the policy file then the warning is not triggered. It's only if you rely on the fact that the first rule is the default in code that you get the warning. So the problematic policy file is actually more like:

# This is the default from policy-in-code
#"identity:get_application_credential": "(role:reader and system_scope:all) or rule:owner"
"identity:get_application_credentials": "rule:identity:get_application_credential"

The rest of what I said is still valid.

[Ben Nemec (bnemec)]

Strange. If I dump out the list of rules in the cycle check, I see both

"identity:get_application_credentials": "rule:identity:get_application_credential",

and

"identity:get_application_credential": "rule:identity:get_application_credential",

If I explicitly set identity:get_application_credential then I get the expected rule. Something is implicitly adding a circular rule. :-/

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.opendev.org/682150

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/682150
Committed: https://git.openstack.org/cgit/openstack/oslo.policy/commit/?id=82a2c8d8b71896b1f8b7f33c560681367ae76755
Submitter: Zuul
Branch: master

commit 82a2c8d8b71896b1f8b7f33c560681367ae76755
Author: Ben Nemec <email address hidden>
Date: Fri Sep 13 20:04:56 2019 +0000

    Fix reference cycle caused by deprecated sample override

    In the sample policy generator, we create a rule that maps the
    deprecated name of a policy to the new rule name. For example:

    identity:old_rule: rule:identity:new_rule

    However, in the policy code, if we see an override of a deprecated
    name and no override for the new name, we apply the value of the
    deprecated name to the new name. In the above case, this results
    in us creating a rule that looks like:

    identity:new_rule: rule:identity:new_rule

    which is a circular reference and nonsense.

    To fix this, I added a check to the deprecated rule logic that looks
    for instances where the old override is just a reference to the new
    rule. If that's the case, then we don't need to do anything because
    it's already doing the right thing.

    Change-Id: Ifd14993bc84e83c13abab3456fbf670c06e5806f
    Closes-Bug: 1843931

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/stein
Review: https://review.opendev.org/684314

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/684314
Committed: https://git.openstack.org/cgit/openstack/oslo.policy/commit/?id=0a4adba1a53c25ad75a10525130d832138f4fc68
Submitter: Zuul
Branch: stable/stein

commit 0a4adba1a53c25ad75a10525130d832138f4fc68
Author: Ben Nemec <email address hidden>
Date: Fri Sep 13 20:04:56 2019 +0000

    Fix reference cycle caused by deprecated sample override

    In the sample policy generator, we create a rule that maps the
    deprecated name of a policy to the new rule name. For example:

    identity:old_rule: rule:identity:new_rule

    However, in the policy code, if we see an override of a deprecated
    name and no override for the new name, we apply the value of the
    deprecated name to the new name. In the above case, this results
    in us creating a rule that looks like:

    identity:new_rule: rule:identity:new_rule

    which is a circular reference and nonsense.

    To fix this, I added a check to the deprecated rule logic that looks
    for instances where the old override is just a reference to the new
    rule. If that's the case, then we don't need to do anything because
    it's already doing the right thing.

    Change-Id: Ifd14993bc84e83c13abab3456fbf670c06e5806f
    Closes-Bug: 1843931
    (cherry picked from commit 82a2c8d8b71896b1f8b7f33c560681367ae76755)

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: stable/train
Review: https://review.opendev.org/694841

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/oslo.policy 2.4.0 release.

[Zane Bitter (zaneb)]

Aha, I wonder if this is the thing I was complaining about in https://bugs.launchpad.net/oslo.policy/+bug/1742569/comments/3 when I said "I'm surprised if it doesn't result in cycles when using the sample policy, because if the new policy is not specified but the deprecated one is the IIUC it is supposed to fall back to the deprecated one from the file... which points to the new one."

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.opendev.org/694841
Committed: https://git.openstack.org/cgit/openstack/oslo.policy/commit/?id=3898da6d8c54229aa60c0e3571c7ec0f847ca8ea
Submitter: Zuul
Branch: stable/train

commit 3898da6d8c54229aa60c0e3571c7ec0f847ca8ea
Author: Ben Nemec <email address hidden>
Date: Fri Sep 13 20:04:56 2019 +0000

    Fix reference cycle caused by deprecated sample override

    In the sample policy generator, we create a rule that maps the
    deprecated name of a policy to the new rule name. For example:

    identity:old_rule: rule:identity:new_rule

    However, in the policy code, if we see an override of a deprecated
    name and no override for the new name, we apply the value of the
    deprecated name to the new name. In the above case, this results
    in us creating a rule that looks like:

    identity:new_rule: rule:identity:new_rule

    which is a circular reference and nonsense.

    To fix this, I added a check to the deprecated rule logic that looks
    for instances where the old override is just a reference to the new
    rule. If that's the case, then we don't need to do anything because
    it's already doing the right thing.

    Change-Id: Ifd14993bc84e83c13abab3456fbf670c06e5806f
    Closes-Bug: 1843931
    (cherry picked from commit 82a2c8d8b71896b1f8b7f33c560681367ae76755)

[Ben Nemec (bnemec)]

Probably, and I think that only got fixed around the time you made the comment.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/oslo.policy 2.3.3 release.

[OpenStack Infra (hudson-openstack)]

This issue was fixed in the openstack/oslo.policy 2.1.2 release.