Update: I've revised the patch documenting how to configure Cinder policies to enable a read-only administrator [0] to take into account the ongoing Keystone work that Lance discussed earlier in this thread. At this point in the development cycle, I think it makes the most sense to continue with the plan to "fix" this via documentation, especially since the workaround documented immediately applies to the stable branches. The work Yikun Jiang is doing now to get "protection" tests into Cinder [1] will give us a base to work from if we decide to refactor the code to make this easier (or to implement default policies that recognize a 'reader' role out of the box, as Keystone is doing this cycle). In any case, reviews of [0] will be appreciated, especially during this festive holiday season. cheers, brian [0] https://review.openstack.org/#/c/624424/ [1] https://review.openstack.org/#/q/status:open+project:openstack/cinder+branch... On 12/13/18 12:58 PM, Lance Bragstad wrote:
Thanks for looping keystone into this, Ben.
This sounds very similar, and related, to the default roles work keystone implemented in Rocky [0]. TL;DR, keystone ensures the ``member`` and ``reader`` roles are available during ``keystone-manage bootstrap``, which is typically done during installation and sometimes on upgrade. Pre-Rocky, only the ``admin`` role existed in keystone.
The idea is to make it easier for other services, like cinder, to rely on these roles by default. As opposed to cinder changing a policy default to something like ``role:reader`` and another service using something like ``role:auditor``, but they both mean the same thing. We want to avoid the case where operators would need to make sure both roles exist in keystone and users have both if they're expected to perform read-only operations from each service.
The work keystone did in Rocky made it possible for cinder to start making those assumptions about the existence of roles called ``member`` and ``reader``.
As far as testing goes, I remember there being a cinder session in Denver about policy changes, regression, and making sure we can protect against it happening, especially late in the release. Ideally, it would be fantastic to have protection tests in each service that assert the default policies work as expected for protecting APIs. Unfortunately, testing this in its entirety makes it an integration test that requires keystone. Luckily, there are some ways for services to expand protection testing without having that dependency, which falls within the parameters of unit tests. I proposed a couple of changes to cinder that show how to do this [1], and it requires modeling an API request similar to what keystonemiddleware/oslo.context do and ensuring the actual defaults are tested by removing overridden test policies [2] that leave APIs unprotected. We attempted to document a generalized approach to this sort of testing in oslo.policy [3].
[0] http://specs.openstack.org/openstack/keystone-specs/specs/keystone/rocky/def... [1] https://review.openstack.org/#/c/602489/ [2] https://review.openstack.org/#/c/609498/ [3] https://docs.openstack.org/oslo.policy/latest/user/usage.html#testing-defaul...
On Thu, Dec 13, 2018 at 8:10 AM Ben Nemec <openstack@nemebean.com <mailto:openstack@nemebean.com>> wrote:
cc Keystone. It would be interesting to know how this fits in with the new default roles work.
On 12/12/18 8:21 PM, Brian Rosmaita wrote: > At last week's cinder meeting (5 December) we had a discussion about > policy checks at the database layer. While these checks seem to make it > difficult to perform some policy configurations, there are too many of > them to simply remove them without impacting stability given our current > test coverage (at least that's my feeling). Additionally, it's possible > to handle the proposed use case (a read-only administrator) without > making any code changes. So we decided to fix this issue by documenting > how this could work. > > I've got a patch up for the documentation. I've flagged this email for > cinder people to read for accuracy, operators to read to make sure the > instructions are clear and detailed, and docs people to read for > felicity of expression. Please leave comments on the patch: > > https://review.openstack.org/#/c/624424/ > > cheers, > brian >