| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-11-30 | |||
| 16:38:34 | opendevmeet | Log: https://meetings.opendev.org/meetings/nova/2021/nova.2021-11-30-16.00.log.html | |
| 16:38:34 | opendevmeet | Minutes (text): https://meetings.opendev.org/meetings/nova/2021/nova.2021-11-30-16.00.txt | |
| 16:38:34 | opendevmeet | Minutes: https://meetings.opendev.org/meetings/nova/2021/nova.2021-11-30-16.00.html | |
| 16:38:34 | opendevmeet | Meeting ended Tue Nov 30 16:38:34 2021 UTC. Information about MeetBot at http://wiki.debian.org/MeetBot . (v 0.1.4) | |
| 16:38:34 | bauzas | #endmeeting* | |
| 16:38:49 | bauzas | man, I'm fed up with my typing issues | |
| 16:40:03 | bauzas | thanks all | |
| 16:40:22 | elodilles | thanks bauzas o/ | |
| 16:40:57 | bauzas | I'm a gross person, I haven't officially thanked you all at the end of the meeting | |
| 16:42:09 | elodilles | :D | |
| 17:07:13 | bauzas | gmann: can I put you assignee on https://blueprints.launchpad.net/nova/+spec/policy-defaults-refresh-2 ? | |
| 17:14:48 | opendevreview | Nicolas Parquet proposed openstack/nova master: Add v2.91 microversion, allowing @ and dot (.) characters in keypair name https://review.opendev.org/c/openstack/nova/+/781076 | |
| 18:05:52 | gmann | bauzas: sure, thanks | |
| 18:07:59 | opendevreview | Dan Smith proposed openstack/nova master: Revert project-specific APIs for servers https://review.opendev.org/c/openstack/nova/+/816206 | |
| 18:08:00 | opendevreview | Dan Smith proposed openstack/nova master: Make API fixture pass roles https://review.opendev.org/c/openstack/nova/+/819907 | |
| 18:08:26 | dansmith | gmann: this has the fixture change on top ^ so you can see it not working without it, and then working when we change that | |
| 18:08:42 | dansmith | I haven't done more digging on why this is required, but hoping it's something you can spot easily | |
| 18:08:53 | gmann | dansmith: ok, checking | |
| 18:08:54 | dansmith | I wonder if we're not really testing fake policy like we think in functional? | |
| 18:09:47 | gmann | dansmith: which is good :). I wanted to remove those fake policy from unit as well as from functional tests completely but that might need more tests modification but something we should do. | |
| 18:10:29 | dansmith | don't disagree that it would be better, I just don't know why this is required right now, | |
| 18:10:40 | dansmith | since I would expect us to at most be testing with old defaults OR'd in | |
| 18:11:13 | gmann | sure, I will check it where we are missing things. | |
| 18:11:46 | gmann | dansmith: did you see my comment https://review.opendev.org/c/openstack/nova/+/816206/9/nova/policies/base.py | |
| 18:12:12 | gmann | dansmith: if CONTEXT_ADMIN if more readable then it is fine otherwise we can ad ADMIN ? | |
| 18:12:27 | dansmith | gmann: oh yeah I did, I just got distracted by the functional failures | |
| 18:12:59 | gmann | I started converting SYSTEM_READER to SYSTEM_ADMIN on top of your patch so doing it in base patch will avoid rebase or so | |
| 18:13:00 | gmann | sure | |
| 18:57:52 | gmann | dansmith: this is reason for functional test failure https://review.opendev.org/c/openstack/nova/+/816206/comment/510a59e0_5ffbc2f5/ | |
| 18:58:12 | gmann | dansmith: functional test using the real policy helped us to capture it. | |
| 19:00:04 | dansmith | gmann: ahh, I was probably conflating that rule with the admin_or_owner below it when thinking that we'd still have the old default | |
| 19:01:40 | gmann | dansmith: yeah, and those role hierarchy fix in 819907 made test passing because of 'admin' being used as user-id for functional test https://review.opendev.org/c/openstack/nova/+/819907/comment/eba56840_d585da0a/ | |
| 19:02:17 | dansmith | gmann: you mean that's why they worked before I switched the rule... | |
| 19:03:14 | dansmith | gmann: should I use admin_or_owner for those flavor-extra-spec rules, or add the DEPRECATED_ADMIN_OR_OWNER to context_admin/ | |
| 19:05:22 | gmann | dansmith: I think DEPRECATED_ADMIN_POLICY as CONTEXT_ADMIN is going to replace SYSTEM_ADMIN only . I was trying to do it this way https://review.opendev.org/c/openstack/nova/+/819389 | |
| 19:06:07 | gmann | and PROJECT_ADMIN going to be with DEPRECATED_ADMIN_OR_OWNER which has project_id in that | |
| 19:08:22 | gmann | dansmith: let me update my patch and then you can use the new ADMIN rule for place of role:admin | |
| 19:08:56 | dansmith | gmann: ack | |
| 19:09:06 | dansmith | gmann: we still want my patch to pass proper roles from the fixture though right? | |
| 19:09:32 | dansmith | presumably we need to also let you get a fixture with no (or foo) roles for testing that member is enforced | |
| 19:14:38 | gmann | dansmith: for is_admin L1092 yeah it is ok but else part make reader also give member authority https://review.opendev.org/c/openstack/nova/+/819907/1/nova/tests/fixtures/nova.py#1092 | |
| 19:15:22 | gmann | if we remove the else part and let real role being tested what test use then it should be ok | |
| 19:15:37 | dansmith | gmann: well, it just assumes member if not admin, which all the current users of that fixture are going to expect.. I'm saying it needs to be extended to allow you to tell it what roles you want | |
| 19:15:43 | gmann | and later we can add user_id as 'member', 'reader' etc and add role accodringl;y | |
| 19:15:48 | dansmith | gmann: otherwise non-admin stuff will all fail too because roles=[] can't do aything | |
| 19:16:02 | gmann | yeah that is true | |
| 19:16:36 | dansmith | if we fix the deprecated rule bit, then this doesn't matter just yet, but before we can test with deprecated rules disabled, we'll have to fix this I think | |
| 19:17:51 | gmann | dansmith: right for No legacy tests we need that. and we can add we can add user_id == 'member' and add only member role and else part as add reader | |
| 19:18:05 | dansmith | sure, or just pass roles= to the fixture | |
| 19:18:27 | dansmith | right now, the user that is used is something other than "member" so we probably need a bunch of changes for that first | |
| 19:18:57 | gmann | +1, much better | |
| 19:30:41 | opendevreview | Dan Smith proposed openstack/nova master: Make API fixture pass roles https://review.opendev.org/c/openstack/nova/+/819907 | |
| 19:30:47 | dansmith | gmann: like this-ish ^ | |
| 19:33:45 | gmann | dansmith: +1 yeah, and you want default to reader only (lower level of access) ? here https://review.opendev.org/c/openstack/nova/+/819907/2/nova/tests/functional/api/client.py#140 | |
| 19:34:30 | dansmith | gmann: that defaults to member, not reader.. since that's what everything else would expect currently, it seemed like that would be the best default | |
| 19:34:54 | dansmith | looks like maybe it's only used in one other place though, | |
| 19:35:04 | dansmith | so I guess I could default it even lower and fix that one too | |
| 19:35:32 | dansmith | default should be roles=[] right? since that's what you get if you're not explicitly granted reader/member/admin on a project | |
| 19:35:40 | gmann | dansmith: yeah. but if it need mroe test fix then we can leave as of now and later we can do while moving func tests also to reader/mem,ber | |
| 19:35:43 | dansmith | oh nm, it's just used here | |
| 19:37:41 | opendevreview | Dan Smith proposed openstack/nova master: Make API fixture pass roles https://review.opendev.org/c/openstack/nova/+/819907 | |
| 19:37:51 | dansmith | I thought client was used a few other places directly, but it's not, so we should be good to default to roles=[] ^ | |
| 19:39:04 | gmann | +1. yeah its self.api and self.admin_api | |
| 19:39:06 | gmann | as of now | |
| 19:39:19 | dansmith | yup | |
| 19:39:27 | sean-k-mooney | well default to roles=None | |
| 19:39:39 | sean-k-mooney | rather then [] | |
| 19:39:40 | opendevreview | Ghanshyam proposed openstack/nova master: Introduce 'admin' policy base rule https://review.opendev.org/c/openstack/nova/+/819389 | |
| 19:39:49 | sean-k-mooney | sicne you should not use mutable defaults but same effect | |
| 19:39:56 | gmann | dansmith: ^^ this is for 'admin' basically renaming 'context_is_admin' to 'admin' | |
| 19:40:02 | dansmith | sean-k-mooney: did you look at the patch? :) | |
| 19:40:11 | dansmith | gmann: cool thanks | |
| 19:40:24 | sean-k-mooney | nope just saw the converstaion scroll by | |
| 19:40:43 | dansmith | sean-k-mooney: trying not to take offense that you think I don't know not to use mutable defaults :) | |
| 19:41:50 | sean-k-mooney | :) | |
| 19:42:58 | sean-k-mooney | i still see it in code often enough but ya you already did it the right way https://review.opendev.org/c/openstack/nova/+/819907/3/nova/tests/functional/api/client.py#140 | |
| 19:49:52 | gmann | I think we have hacking rule for that. | |
| 20:13:37 | dansmith | gmann: I don't think your admin patch works for me by itself, | |
| 20:13:44 | dansmith | since it's still requiring role:admin, which isn't what we have today | |
| 20:13:56 | dansmith | it needs to be is_admin in order to work as-is right? | |
| 20:24:52 | gmann | dansmith: context set is_admin based on this rule itself https://github.com/openstack/nova/blob/d630615a02469442fb50ed4aa7e092206a28166a/nova/context.py#L138 | |
| 20:24:58 | gmann | https://review.opendev.org/c/openstack/nova/+/819389/4/nova/policy.py | |
| 20:29:16 | gmann | but as it is two level deprecated rule combined I hope each one is logical ORed by oslo policy. but is it failing on your patch? if so then we can avoid to rename it for now. and add DEPRECATED_ADMIN_POLICY in is_context_admin rule only | |
| 20:39:49 | gmann | dansmith: yeah, i ran your patch with my change and it does not work. oslo policy add only one level of deprecated rule in logical OR. | |
| 20:40:56 | gmann | dansmith: I will abandon my patch and let's rename CONTEXT_ADMIN to ADMIN in your patch but keeping rule:is_context_admin. commented in https://review.opendev.org/c/openstack/nova/+/816206/comment/543216d1_15fecfb1/ | |
| 21:43:47 | dansmith | gmann: okay, but that's not enough, it needs to be "role:admin or is_admin:True" | |
| 21:44:21 | dansmith | so do you want me to do that on context_is_admin with a deprecated_rule= or add it to admin_api? | |
| 21:44:51 | gmann | dansmith: yeah that will add ORed both | |
| 21:45:59 | gmann | dansmith: yeah deprecated_rule=DEPRECATED_ADMIN_POLICY . | |
| 21:46:13 | dansmith | okay that's not enough either, I need this: https://termbin.com/bnqh | |
| 21:46:15 | dansmith | you okay with that? | |
| 21:46:27 | dansmith | oh wait | |
| 21:46:37 | dansmith | it's enough to use it as-is for today, you're right | |
| 21:46:45 | dansmith | I'm thinking this needs to work for both old and new, but not as of this patch | |
| 21:46:58 | dansmith | so yeah, I will just add the deprecated_admin_policy there | |
| 21:47:12 | gmann | if you do this then it should ADMIN = 'rule:context_is_admin' | |
| 21:47:58 | dansmith | yep, hadn't saved that before paste :) | |
| 21:48:31 | dansmith | running all the tests now, but expect this to work now | |
| 21:48:40 | gmann | dansmith: so with that you do not need this right? 'check_str='is_admin:True or role:admin',' | |
| 21:48:52 | dansmith | correct, for just this patch, I don't | |