Open Stack

> I tracked down the cause of the check-grenade-dsvm failure on
> https://review.openstack.org/#/c/167370 . As I understand it, grenade is
> taking the previous stable release, deploying it, then upgrading to the
> current master (plus the proposed changeset) without changing any of the
> config from the stable deployment. Thus the policy.json file used in that
> test is the file from stable-juno. Then if we look at oslo_policy/policy.py
> we see that if the rule being looked for is missing then the default rule
> will be used, but then if that default rule is also missing a KeyError is
> thrown. Since the default rule was missing with ceilometer's policy.json
> file in Juno, that's what would happen here. I assume that KeyError then
> gets turned into the 403 Forbidden that is causing check-grenade-dsvm
> failure.
> 
> I suspect the author of the already-merged
> https://review.openstack.org/#/c/115717 did what they did in
> ceilometer/api/rbac.py rather than what is proposed in
> https://review.openstack.org/#/c/167370 just to get the grenade tests to
> pass. I think they got lucky (unlucky for us), too, because I think they
> actually did break what the grenade tests are meant to catch. The patch set
> which was merged under https://review.openstack.org/#/c/115717 changed the
> rule that is checked in get_limited_to() from "context_is_admin" to
> "segregation". But the "segregation" rule didn't exist in the Juno version
> of ceilometer's policy.json, so if a method that calls get_limited_to() was
> tested after an upgrade, I believe it would fail with a 403 Forbidden
> tracing back to a KeyError looking for the "segregation" rule... very
> similar to what we're seeing in https://review.openstack.org/#/c/167370
> 
> Am I on the right track here? How should we handle this? Is there a way to
> maintain backward compatibility while fixing what is currently broken (as a
> result of https://review.openstack.org/#/c/115717 ) and allowing for a fix
> for https://bugs.launchpad.net/ceilometer/+bug/1435855 (the goal of
> https://review.openstack.org/#/c/167370 )? Or will we need to document in
> the release notes that the manual step of modifying ceilometer's policy.json
> is required when upgrading from Juno, and then correspondingly modify
> grenade's upgrade_ceilometer file?

Thanks for raising this issue.

IIUC the idea behind the unconventional approach taken by the original
RBAC patch that landed in juno was to ensure that API calls continued to
be allowed by default, as was previously the case.

However, you correctly point out that this missed a case where the new
logic is run against a completely unchanged policy.json from Juno or
before.

As we just discussed on #os-ceilometer IRC, we can achieve the following
three goals with a relatively minor change:

 1. allow API operations if no matching rule *and* no default rule

 2. apply the default rule *if* present

 3. tolerate the absence of the segregation rule

This would require:

 (a) explicitly checking for 'default' in _ENFORCER.rules.keys() before
     applying the enforcement approach in [1], otherwise falling back
     to the prior enforcement approach in [2]

 (b) explicitly checking for 'segregation' in _ENFORCER.rules.keys()
     before [3] otherwise falling back to checking for the literal
     'context_as_admin' as before.

If https://review.openstack.org/167370 is updated to follow this approach,
I think we can land it for kilo-rc1 without an upgrade exception.
 
Cheers,
Eoghan
 
[1] https://review.openstack.org/#/c/167370/5/ceilometer/api/rbac.py line 49

[2] https://review.openstack.org/#/c/115717/18/ceilometer/api/rbac.py line 51

[3] https://review.openstack.org/#/c/115717/18/ceilometer/api/rbac.py line 81