Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-12-20
17:16:19 gmann that is needed for the window when we remove the deprecated rule and make enforce_scope=True by default. in that window project admin should not be able to do system admin operation.
17:16:47 gmann if enforce_scope if false then scope check is not checked right.
17:17:04 johnthetubaguy yeah, I just assumed those two things happened at the same time
17:18:18 johnthetubaguy gmann: maybe we should do the admin_password API, its admin_or_owner
17:18:40 johnthetubaguy I think what works there we probably want to apply to the admin api too
17:19:22 johnthetubaguy I think the points you made above, I totally agree with you for the admin_or_owner case
17:19:36 gmann if both things happen at same time then yes i agree with your point for admin case.
17:20:21 gmann yeah admin_or_owner and any lower role like member or reader we need that string.
17:20:54 gmann so that we do not expose any project level reader to system level reader
17:21:13 johnthetubaguy what about doing the deprecation at this level: https://review.opendev.org/#/c/645452/17/nova/policies/base.py
17:21:22 gmann s/system level reader to project level reader
17:21:59 johnthetubaguy we spoke about someone wanting to use this sooner they can override the base policy rule to always to the system check, if they want to opt into the new world early
17:22:51 gmann yeah
17:23:14 gmann did not get your last point on deprecation at base level ?
17:23:33 johnthetubaguy so thinking about this: https://github.com/openstack/nova/blob/b953db09e8b9195c6a922c03a57f44cd790adcb6/nova/policies/base.py#L79
17:23:50 johnthetubaguy what if we move the APIs over to the new rule
17:24:18 johnthetubaguy but we add a deprecated bit to the new rule, so it falls back to allowing the old check in there?
17:24:33 johnthetubaguy ... i am not explaining this well
17:25:30 gmann i think i got your point and deprecate to RULE_ADMIN_API
17:25:37 johnthetubaguy yeah
17:25:58 johnthetubaguy so when an operator wants the new world, its only a few rules they change
17:26:18 gmann but what if any API rule changing from RULE_ADMIN_OR_OWNER - > SYSTEM_ADMIN ? i think no..
17:27:08 gmann it will be -> either PROJECT_MEMBER_OR_SYSTEM_ADMIN or PROJECT_READER_OR_SYSTEM_READER that what we have in table
17:27:39 johnthetubaguy yeah, that is the only option I believe
17:27:53 johnthetubaguy and it should work OK for those two
17:28:03 johnthetubaguy hopefully the unit tests will keep us honest here anyways
17:28:16 johnthetubaguy once we have added them :)
17:28:21 gmann johnthetubaguy: then all case we can do in base rules as per this tabel? -https://github.com/openstack/nova/blob/b953db09e8b9195c6a922c03a57f44cd790adcb6/nova/policies/base.py#L41
17:28:36 johnthetubaguy yeah, I hope so
17:29:01 johnthetubaguy so if we go through the cases...
17:29:08 gmann until we encounter any such policy conflicting that table but we can check that if we encounter such case
17:29:25 johnthetubaguy 1) no overrides, just use default, they keep working
17:29:43 gmann ahh 1 min
17:29:52 gmann so override would not work in that case
17:30:02 johnthetubaguy ah, why is that?
17:30:28 johnthetubaguy I think if you override, it skips doing an or with the deprecated rule
17:30:34 gmann because override is done with specific policy rules not base rules
17:30:51 johnthetubaguy right, but I think that is OK
17:31:16 johnthetubaguy in the sense that if the override it to something special, that something special stays the same
17:32:30 gmann yeah override rule is first pick in oslo policy.
17:33:03 johnthetubaguy maybe another question... what upgrade check should we when we want to make enforce_scope = true?
17:33:31 johnthetubaguy we could warn if they have any policy rules overriden that they need checking, but not sure what else we can do really
17:33:51 gmann we need deprecation at specific policy level when we do granularity where rule name change like- https://review.opendev.org/#/c/648480/23/nova/policies/services.py
17:33:53 johnthetubaguy this is more for "next" release, after we make the initial backwards compatible change
17:34:04 johnthetubaguy gmann: +1
17:34:11 johnthetubaguy that case we totally need that
17:34:58 johnthetubaguy but that is a sort of independent issue, its because we added more granularity
17:35:07 gmann yeah
17:35:11 johnthetubaguy and folks make have overriden the old rule
17:35:28 johnthetubaguy cool, does that mean we are all set?
17:36:33 gmann for upgrade check, i was thinking auto correction but it need more thought.
17:36:59 johnthetubaguy yeah, although I don't think it blocks this release's work a such
17:37:21 johnthetubaguy the simplest check is probably to see if the override is the old default
17:37:37 johnthetubaguy in case people haven't deleted their old policy files they always used to deploy
17:37:58 johnthetubaguy there is an ops meetup after christmas, I could ask around there if policy comes up
17:38:17 gmann +1, that will be great.
17:38:50 johnthetubaguy cool, happy holidays when that happens
17:38:52 gmann seems all set for now. thanks for discussion.
17:39:17 johnthetubaguy no problem, thanks for pushing this, so many of our customers are wanting to see this move forward
17:39:19 gmann you too. i will try to make other policy ready once you are back
17:39:29 johnthetubaguy cool
17:40:21 gmann also waiting for stephenfin to reply on this, i could not understand his query (if something interesting to care at initial level)- https://review.opendev.org/#/c/657698/14
17:43:30 johnthetubaguy gmann: I think stephenfin was meaning we could have left the unit tests where they were, under api, rather than "moving" to policy
17:44:12 gmann ohk
17:45:13 johnthetubaguy personally, I had assumed we were putting the unit tests under api/ before I saw your patches, but I wasn't against moving it
17:45:14 gmann i think doing under separate dir is more explicit and easy to check coverage. inside API it was very hidden kind of testing. but may be it just me ?
17:45:40 johnthetubaguy the other thing, is when you cut and paste all the tests when adding a new API, you might not see the policy tests
17:45:57 johnthetubaguy not that people do that, of course (cough)
17:45:58 gmann i actually went with the code structure. policies is not under API code
17:46:31 johnthetubaguy but we are testing the context.can call, and that is under the API code
17:46:59 johnthetubaguy I should be clear, my +2 stands, I don't mind the idea, for the reasons you just said
17:47:17 johnthetubaguy really just playing devils advocate to make sure we are sure
17:48:26 gmann ok. i can move as its not big deal. may be under ./nova/tests/unit/api or nova/tests/unit/api/openstack/compute ?
17:49:36 johnthetubaguy I think for that patch, basically it goes where those deleted tests are
17:50:13 johnthetubaguy but I would +2 either approach, until I hear a stronger argument for one or the other
17:50:37 gmann i think we should do under separate dir but under API dir or api/openstack/compute
17:50:53 johnthetubaguy oh... in some ways that is worse
17:51:06 gmann inside API test files it is very hidden honestly.
17:51:15 johnthetubaguy lets stick with what you have already, assuming stephenfin is OK with it after we tell him why
17:51:39 gmann in doc we can say 'any API change or policy, review can check the tests under this folder'
17:51:47 gmann ok
17:52:36 johnthetubaguy I think either we consider it testing the policy file or the api file, its basically testing both
17:53:04 johnthetubaguy hence I am OK with either next to all the API unit tests (like the old broken tests we delete) or in the new place you chose
17:53:50 johnthetubaguy its easy enough to move if we decide we made the wrong call
17:54:03 johnthetubaguy cool, I should run, was good to catch up
17:54:07 gmann ok. let's see what stephenfin say.
17:54:11 johnthetubaguy +1
17:54:13 gmann sure, thanks
18:49:15 openstackgerrit Merged openstack/nova stable/train: Block rebuild when NUMA topology changed https://review.opendev.org/698530
19:58:50 openstackgerrit Merged openstack/nova stable/stein: Cache security group driver https://review.opendev.org/697480
20:10:06 openstackgerrit Merged openstack/nova stable/stein: Improve metadata server performance with large security groups https://review.opendev.org/695925
21:18:09 openstackgerrit Merged openstack/nova master: FakeDriver: adding and removing instances on live migration. https://review.opendev.org/243613
21:24:00 mriedem stephenfin: some easy things to cleanup in the xvpvncproxy removal change https://review.opendev.org/#/c/687909/ - i'm sure i'll probably be poking around in gerrit next week to avoid my in-laws so if you get that updated i'm +2
21:48:15 openstackgerrit Merged openstack/nova master: Add new default roles in os-services API policies https://review.opendev.org/648480
21:55:50 openstackgerrit Dustin Cowles proposed openstack/nova master: Provider Config File: YAML file loading and schema validation https://review.opendev.org/673341
21:55:50 openstackgerrit Dustin Cowles proposed openstack/nova master: Provider Config File: Function to further validate and retrieve configs https://review.opendev.org/676029
21:55:51 openstackgerrit Dustin Cowles proposed openstack/nova master: Provider Config File: Functions to merge provider configs to provider tree https://review.opendev.org/676522
21:55:51 openstackgerrit Dustin Cowles proposed openstack/nova master: WIP: Provider Config File: Enable loading and merging of provider configs https://review.opendev.org/693460
22:38:03 mriedem gmann: nova.tests.unit.test_policy.RealRolePolicyTestCase.test_all_rules_in_sample_file is dumping a lot to the console log
22:38:35 mriedem https://zuul.opendev.org/t/openstack/build/3a837b550b2344aca69eaf747dc5a0c2/log/job-output.txt#20581

Earlier   Later