| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-11-05 | |||
| 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 | the problem is I would get this from the checker: | |
| 16:24:30 | dansmith | testtools.matchers._impl.MismatchError: !=: | |
| 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 | actual = "Policy doesn't allow os_compute_api:servers:create_image to be performed." | |
| 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 | 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:54:05 | dansmith | gmann: yeah, maybe, or just ensure that a system token gets stopped at the parent policy check | |
| 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 | gmann | oh no, sorry | |
| 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: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 | |
| 18:00:58 | gmann | we may want to change that in that case | |
| 18:02:51 | dansmith | gmann: okay I'm confused | |
| 18:02:58 | gmann | dansmith: may be I am confusing with current and newthings. system I mean domain admin who will be allowed to do all-tenant instances | |
| 18:03:00 | dansmith | I'm working on https://review.opendev.org/c/openstack/governance/+/815158/7/goals/selected/yoga/consistent-and-secure-rbac.rst line 144 | |
| 18:03:16 | dansmith | gmann: ah, no I'm talking about system:* specifically :) | |
| 18:03:46 | gmann | dansmith: ohk, so new-system as per our discussion | |
| 18:03:54 | dansmith | yeah | |
| 18:04:33 | gmann | dansmith: say with doamin-admin, we want index to be project+domain or just project ? | |
| 18:05:03 | gmann | because we have to think on filter instance by tenant also in this case | |
| 18:05:05 | dansmith | gmann: yeah I think it'll be domain+project at that point | |
| 18:05:12 | gmann | ohk, +1 | |
| 18:05:31 | gmann | that will make sense. that what i was asking but sorry confused with system | |
| 18:05:43 | dansmith | all good, it's all very confusing :) | |
| 18:05:58 | gmann | so now onwards we talk on new things on old way:) | |
| 18:06:03 | gmann | I make note of that | |
| 18:06:14 | gmann | s/on/not | |
| 18:07:19 | dansmith | heh | |
| 18:07:27 | gmann | dansmith: I am sure these multi-policy testing stuff will be more conflicted in the 'GET server with additional attributes' | |