Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-11-05
13:55:41 opendevreview Lee Yarwood proposed openstack/nova master: nova-next: Deploy noVNC from source instead of packages https://review.opendev.org/c/openstack/nova/+/816738
16:07:45 dansmith gmann: if I have a policy rule with a specific scope_types= set, and then I override that rule with enforcer.set_rules({}) do you know what happens to the scope_types from the default?
16:08:26 dansmith it seems like they are being kept through the override (which I would expect), but that's preventing me from overriding some rules to @ in the tests, like check_image before check_image:allow_volume_backed
16:08:28 dansmith lbragstad: ^
16:10:55 opendevreview Merged openstack/nova master: [Trival] Fix wrong microversion in TestClass name https://review.opendev.org/c/openstack/nova/+/816778
16:13:19 lbragstad dansmith i think you're right in that it shouldn't override the scope_type
16:13:56 dansmith okay, so some of the tests we have hit two policies, and the tests try to @ the first one to make sure we test the second one,
16:14:15 dansmith but in the case of the "what happens when we enable scope checking" tests, I'm not sure how to get past that
16:14:28 lbragstad oh...
16:14:37 dansmith other than to just assume that scope violations get caught by the first one
16:15:19 lbragstad the only way i could think of doing that today would be re-register the rules so that you can reset the scope type of the one you want to pass through
16:15:26 lbragstad but that sounds clunky
16:15:42 dansmith yeah
16:16:11 lbragstad what's the scope_type for check_image, project?
16:16:14 dansmith or mock the enforce with a side_effect=[skip(), orig()]
16:16:48 dansmith it was both (of course, like all of them) but I'm moving it back to just project since it's on an instance
16:16:54 dansmith (and it's create_image, typo above)
16:17:32 lbragstad ah
16:17:42 dansmith the tests allow me to skip the check that makes sure it's the exact rule name that triggers the failure,
16:18:08 dansmith so I could just do that, but it kinda sidesteps the actual verification (although only for the system: contexts, which isn't so bad)
16:18:14 lbragstad and you want to test the second check fails with a system-scoped token?
16:18:32 dansmith I mean it would still ensure the behavior, it's just that the tests currently disable the first check with @ to hit the second for all the context
16:18:46 dansmith just trying to change as little as possible about the test behavior of course
16:18:56 dansmith maybe I'll just put a #NOTE in there and leave it for review
16:19:09 lbragstad sure - and that's not necessarily realistic given the discussions the other day
16:22:38 dansmith https://termbin.com/z7yz
16:22:40 dansmith this ^ works
16:23:14 dansmith so we keep the strict checking for the non-scope-enforcing case, but relax it (as allowed by common_policy_check()) if we are scope-obsessed
16:24:25 dansmith oh you know, I could use the base rule name instead of none to be even more explicit
16:24:30 dansmith actual = "Policy doesn't allow os_compute_api:servers:create_image to be performed."
16:24:30 dansmith reference = "Policy doesn't allow os_compute_api:servers:create_image:allow_volume_backed to be performed."
16:24:30 dansmith testtools.matchers._impl.MismatchError: !=:
16:24:30 dansmith the problem is I would get this from the checker:
16:24:46 dansmith because we failed on the earlier check before we got to create_image:allow_volume_backed
16:26:45 dansmith anyway, I'm mostly just talking to myself out loud at this point :)
16:29:42 lbragstad yeah - that makes sense
16:30:44 lbragstad i guess my question now is if we think nesting checks like that is going to be a common pattern, or a one off thing here and there
16:31:34 lbragstad feels like a one-off thing
16:32:14 dansmith it's more a result of nova's test pattern as anything
16:32:19 dansmith but yeah I'll see as I keep going through it
16:33:22 lbragstad if it's common, i think we should have a better answer for it?
16:33:30 lbragstad or potentially consolidate the checks
16:34:08 dansmith yup
16:43:22 opendevreview Sylvain Bauza proposed openstack/nova master: [doc] propose Review-Priority label for contribs https://review.opendev.org/c/openstack/nova/+/816861
16:50:49 gmann dansmith: lbragstad yeah, we have this type of multi policy enforcement in nova and I think in neutron there are many.
16:51:16 gmann but do we have mixed type of scope_type in such multi policy in single API?
16:51:32 gmann that sounds not correct if we do have
16:51:43 dansmith gmann: so far that hypervisors "give a different view for projecty people" is the only multi-scope one I know of
16:52:08 gmann dansmith: or it is different APIs like resource creation for GET test etc
16:53:22 gmann dansmith: lbragstad I am thinking to allow tests to disable (set to None) the scope_type but yes only for testing
16:53:32 dansmith gmann: yeah, let me continue through more of this to see how many are a problem
16:54:05 dansmith gmann: yeah, maybe, or just ensure that a system token gets stopped at the parent policy check
16:54:05 gmann dansmith: in nova case where it is unit tests we can handle with mocking API controller method itself if it is different APIs.
16:56:09 gmann yeah with switching the CONF.encorce_scope to false first and then true
17:06:47 dansmith gmann: yeah, index:get_all_tenants and index are in the same boat :/
17:08:47 gmann dansmith: you mean with the new direction or existing scope_type?
17:09:08 gmann but in both case anyone allow to do index:get_all_tenants should be allow to do index right?
17:09:15 dansmith gmann: I mean checking index before index:get_all_tenants and failing the scope check
17:09:32 dansmith gmann: that's not the problem
17:09:34 gmann but both are same scope_type
17:09:58 dansmith gmann: the problem is that both aren't allowed for system:* but we'll fail on the first one before we get to the second one, even if we set the check_str=*
17:09:58 gmann oh no, sorry
17:10:30 dansmith it's not actually a problem because as you mention, their scope restriction will be the same, it's just a matter of verification in the tests
17:10:39 gmann yeah
17:10:43 dansmith let me propose a change to common_policy_check and see what you think
17:10:51 gmann +1
17:15:33 gibi bottom line: ospurge works and can be used but it does not handle default security group yet.
17:15:37 gibi hups
17:15:47 gibi wrong window
17:37:26 opendevreview Dan Smith proposed openstack/nova master: WIP: Allow per-context rule in error messages https://review.opendev.org/c/openstack/nova/+/816865
17:37:32 dansmith gmann: ^
17:37:54 dansmith gmann: that lets me pass a function that will decide if we should fail on index or index:get_all_tenants
17:38:37 dansmith it means that we don't actually pass the parent check to make sure the child check is the one that fails, but only in the case where the scope_types= is what causes us to fail
17:39:02 dansmith (in the case where we have overridden the parent check to be @)
17:43:20 gmann dansmith: can you give some example of rule_name in this ^^ case?
17:45:14 dansmith gmann: https://termbin.com/0w1b
17:47:43 dansmith that's snips from multiple places, hopefully that makes sense
17:48:12 dansmith I would push up my actual test changes, but they are a TOTAL mess atm :)
17:49:44 gmann dansmith: so in case of project scoped context they will fail on all-tenant as it will index policy
17:50:06 gmann and for system reader case, it will fail on index but we want to skip that and test all-tenant right?
17:51:07 dansmith for project member, it will fail index:get_all_tenants
17:51:12 dansmith for project admin, it will succeed
17:51:32 dansmith for system * it will fail on index (because of scope check requiring project)
17:51:50 dansmith the callable makes sure we assert the right rule name in the error message for system:* and project:member
17:52:04 gmann yeah for system* so how you will test index:get_all_tenants when index will fail first
17:52:22 dansmith well, that's my point.. we won't :)
17:52:32 gmann yeah
17:52:41 dansmith in reality, it doesn't matter because they will be stopped there (operator can't override scope_types in their config file)
17:53:10 dansmith if you think we really need to, then we have to do something more complicated
17:53:41 dansmith your NOTE in there about rule_name=None, however, seems intended for this purpose, but is less specific than what I have there
17:54:39 gmann dansmith: no, I was thinking from operator perspective. so index policy has to be system+project scoped so yeah test passing/failing on system for all-tenant is ok
17:55:30 gmann dansmith: by this "for system * it will fail on index (because of scope check requiring project)" do you mean you will modify index policy to project scoped only?
17:57:25 gmann if so then we need to to little magic in multi-policy like if system scoped token then check all-tenant policy only and not index-policy.
17:57:50 dansmith gmann: yeah that's what I'm doing, working on making those project-only again
17:58:18 dansmith I think for these cases, enforce_scope=project will handle that for us
17:59:35 gmann so we want system user to do only 1. get all tenant instances and 2. get single tenant instances with all-tenant=true but NOT 3. get specific tenant instances directly
17:59:38 gmann ?
18:00:08 dansmith gmann: what we discussed on wednesday (and before) is that system users don't get to see project resources, so they can never list instances
18:00:09 gmann as filtering instances of single tenant-id is only allowed with all-tenant parameter
18:00:35 gmann dansmith: ^^ this is one way to do for them in nova imementation

Earlier   Later