Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2021-11-08
17:59:59 kevko sean-k-mooney: is this related to mi issue ? :/ https://paste.opendev.org/show/810855/ ?
18:51:53 sean-k-mooney kevko: you get the error if the port has been created in ovs but the tapdevice is not present on the host in the same network namespace as ovs
18:52:21 sean-k-mooney actully no that is a slightly idffernt error
18:52:34 sean-k-mooney in this case the port has been revomed form the ovsdb.
18:52:43 sean-k-mooney this might be a race with the vm deletion
19:33:53 dansmith gmann: are you okay with this? https://pastebin.com/hGzKL2ap
19:34:12 dansmith gmann: it makes the tests much less verbose, easier to reason about (IMHO) and easier to refactor when we move things around
19:39:23 gmann dansmith: sure, looks good to me.
19:39:37 dansmith gmann: okay cool :)
19:49:20 dansmith gmann: why is this index and not show? https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/api/openstack/compute/views/servers.py#L236-L237
19:50:50 gmann dansmith: because we used single policy for listing all extraspecs in flavor APi and in server API response also.
19:51:17 gmann but with new granular one we can name them like servers:show:flavor_extraspecs
19:51:20 dansmith gmann: right, but index means "can you list" and show means "can you see the actual thing" right?
19:51:30 gmann dansmith: yes
19:51:47 dansmith seems like the server-embedded flavor is more of a "show" than "index" to me
19:53:12 dansmith ah,
19:53:30 dansmith I guess it's because a show on a flavor including extra specs means "listing" the extra specs inside
19:53:37 gmann dansmith: but that is list right? not single extraspec. we usually used show for getting single resource details and index for list
19:53:54 dansmith I think that's probably just an implementation detail about how we store those..seems confusing from the outside, but I see now
19:54:41 gmann this one https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/api/openstack/compute/flavors_extraspecs.py#L59
19:55:19 dansmith yep, I understand now
19:56:06 dansmith makes sense if you consider extra specs their own thing and a flavor as a container of them. I know that's how we do it, it just a little confusing if you think of the flavor as a whole thing :)
19:56:52 gmann yeah
19:57:37 gmann dansmith: i am thinking do we need separate policy for showing extra-specs?
19:58:01 dansmith showing individual specs?
19:59:16 gmann I mean if flavor can be seen by anyone then we show extraspecs also to them or have policy to show flavor detail itself?
19:59:53 dansmith oh, I thought that came from some people wanting to be able to exclude extra_specs from the view of regular users, without removing the rest of the flavor
19:59:54 dansmith sean-k-mooney: ?
20:00:51 sean-k-mooney dansmith: no i was suggesting we might want to allow that in the futre currently some public cloud hide all extra specs form there users there is not filterign today
20:01:28 dansmith sean-k-mooney: there's a separate rule for showing them now
20:01:58 sean-k-mooney in the server reponce or in flavor list
20:02:04 sean-k-mooney i tought there was just one for both
20:02:08 gmann in flavor and server response both
20:02:18 gmann yeah currently one for both
20:02:52 gmann and default to reader so kind of everyone as default
20:03:00 sean-k-mooney what i think woudl be nice at some point woudl be to be able to filter out any non namespace extra spec an any namespaced on related to filtering
20:03:31 sean-k-mooney that way operators that want to hide infra detail can do so but still expsoe the rest fo the extra specs
20:04:02 sean-k-mooney some extra specs ar thigns a user really want to know like does this have a gpu
20:04:24 sean-k-mooney other are really things that are important to the admin and schduler only
20:06:36 gmann that is little complex to handle but anyways let's leave them as it is then if it get updated in future
20:07:32 sean-k-mooney gmann: well that would not be implemeted as a policy it would likely need to be a new api microverions
20:07:45 sean-k-mooney or at least a code change that would implement the filtering
20:08:15 gmann sean-k-mooney: but you said that should be configurable right not hard-coded is_admin check?
20:08:25 sean-k-mooney right now if operator use extra specs for filtering and want to hide that form user there only option is to hide all the extra specs which is an interoperatty issue
20:09:15 sean-k-mooney gmann: good quetion i assuem configurble jsut for interop but admin only for some might make sense. anyway in the context of dans patch we shoudl nto change anythign form what we have today
20:09:57 sean-k-mooney gmann: i just dont really think the exsitg policy fully solve that use case but operators are not complaing so lets not change it for now
20:10:28 gmann sean-k-mooney: or they are not even know it :) as it is allow to show extra specs to everyone as default
20:10:51 gmann but yes if someone has overridden the policy then it make sense
20:11:05 gmann let's leave those as it is.
20:11:40 gmann dansmith: sean-k-mooney so in that case, we can leave it as single policy itself instead of granular i suggested earlier
20:12:22 gmann making them granular for flavor and servers response might confuse and make things complex
20:12:28 dansmith gmann: still need a different one for server/flavor but yep
20:12:38 dansmith oh, no we definitely need those two,
20:12:39 gmann dansmith: because of default ?
20:12:42 dansmith because of the scope_types
20:13:47 sean-k-mooney i can see where scope_types come into other issue by why is it relevent here? you want to not check scope types on the server endpoint?
20:14:10 gmann and with flavor one default as system-project-reader and server one as project-reader ?
20:14:25 dansmith flavor one is project,system but the embedded one is just project
20:14:43 gmann embedded in server reposne right?
20:14:55 sean-k-mooney isned the embeded on contole by the server detail policy
20:15:06 sean-k-mooney e.g. its only expose via server detail
20:15:13 gmann sean-k-mooney: no, as separate policy after detail policy
20:15:14 sean-k-mooney not /server/uuid/extra_specs right
20:15:30 dansmith sure, we could rely on that, but all these tests are disabling the parent check to obsess over the child one being right,
20:15:41 dansmith which would imply that they need to be different
20:16:11 gmann yeah, like if operator want to show server detail to their owner but not extra-specs
20:16:14 dansmith the test will thus assert that system:reader can view the embedded one, but it can't actually
20:16:43 sean-k-mooney gmann: right but if they showed the falavor name you could look it up or at least what it is now
20:17:00 sean-k-mooney you could not compare embeeded to currnt but not sure how much that is relevent
20:17:54 gmann as long as we allow project for both flavor or embedded it is ok i think
20:17:58 sean-k-mooney dansmith: i have not looked at the test so ill trust your judgement if you think havign two woudl be useful but im not sure when it would make sense to have tehm set differently
20:19:03 gmann sean-k-mooney: like we will not add system scope for embedded one as system cannot GET /servers/server-id anyways
20:19:21 dansmith getting the two to work is also making me hate life, so maybe a note in the test would be better
20:20:33 gmann so any extra things to show in sevrer reponse has to be moved to project=scoped only as parent policy is project-scoped only
20:20:35 dansmith these tests are hard to debug sometimes, figuring out which context can do a thing that shouldn't be allowed
20:22:45 gmann dansmith: I tried too be more optimize there may be. instead those can be simple/readable with more separate tests for allowed and not-allowed
20:24:01 dansmith gmann: they're very obsessive which is good, they're just hard to debug, but more infra around them can make it easier
20:25:30 gmann sure
20:34:51 opendevreview Alexey Stupnikov proposed openstack/nova master: Test aborting queued live migration https://review.opendev.org/c/openstack/nova/+/776250
20:46:25 opendevreview Alexey Stupnikov proposed openstack/nova master: Test aborting queued live migration https://review.opendev.org/c/openstack/nova/+/776250
20:53:47 dansmith gmann: why does this not include other_project_* and legacy_admin ? https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/tests/unit/policies/test_flavor_extra_specs.py#L399
20:54:18 dansmith gmann: is it because we're not passing a context to the index check so we're not *actually* preventing those users from doing the flavor extra specs index on servers, because the server rule should have stopped them?
20:56:38 dansmith gmann: by the way, this is what I'm trying to get the tests to (only showing the context setup): https://pastebin.com/UKcd62Th
20:56:51 dansmith which I think is a lot easier to read, especially with the comments
23:10:31 gmann dansmith: RE: other_project_* and legacy_admin - yes as server rule will take care of accessing server
23:11:18 dansmith gmann: ack
23:11:43 gmann and in flavor, as there is no project_id there so anyone can access
23:12:44 dansmith gmann: well, right but in the server tests, I think the casual reader (and definitely the policy-editing operator) would expect that to be the server's project_id
23:13:23 dansmith meaning, when accessing the flavor on a server
23:14:13 dansmith i.e. here https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/tests/unit/policies/test_flavor_extra_specs.py#L327
23:14:41 dansmith so I will put a note in there, but that's probably another reason to have the rules eventually be separate and to pass a target with the proper project_id in it
23:16:46 gmann dansmith: yeah, we can put project_id in current rule too for safer side (if server can be seen by anyone but extra specs not) here - https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/api/openstack/compute/views/servers.py#L236
23:17:13 dansmith yeah
23:17:22 dansmith anyway, note added
23:17:38 dansmith I will try to finish this up tomorrow and get another version posted with more of it cleaned up
23:18:31 gmann dansmith: and on this: https://pastebin.com/UKcd62Th
23:19:28 gmann in ScoptType: self.admin_authorized_contexts need to include project_admin also right?
23:20:24 dansmith gmann: no? system-only for creating flavors right?
23:20:35 gmann or you can name self.system_admin_authorized_contexts and self.project_admin_authorized_contexts
23:20:53 dansmith well, flavor extra_specs anyway

Earlier   Later