| opendevreview | Mridula Joshi proposed openstack/python-glanceclient master: Add member-get command https://review.opendev.org/c/openstack/python-glanceclient/+/802890 | 04:48 |
|---|---|---|
| opendevreview | Merged openstack/glance stable/ussuri: Add housekeeping module and staging cleaner https://review.opendev.org/c/openstack/glance/+/785552 | 05:44 |
| prometheanfire | https://review.opendev.org/803113 shows that glance has some issues with the newer networkx release | 05:45 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Refactor gateway auth layer for metadef APIs https://review.opendev.org/c/openstack/glance/+/799632 | 08:46 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef namepsace policy checks in the API https://review.opendev.org/c/openstack/glance/+/799633 | 08:46 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef object policy checks in the API https://review.opendev.org/c/openstack/glance/+/799634 | 08:46 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef resource type association policy checks in the API https://review.opendev.org/c/openstack/glance/+/799637 | 08:46 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef property policy checks in the API https://review.opendev.org/c/openstack/glance/+/799635 | 08:46 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef tag policy checks in the API https://review.opendev.org/c/openstack/glance/+/799636 | 08:46 |
| opendevreview | Mridula Joshi proposed openstack/python-glanceclient master: Add member-get command https://review.opendev.org/c/openstack/python-glanceclient/+/802890 | 09:12 |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef namespaces https://review.opendev.org/c/openstack/glance/+/798700 | 11:31 |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef resource-types https://review.opendev.org/c/openstack/glance/+/799671 | 11:31 |
| opendevreview | Merged openstack/python-glanceclient master: Add member-get command https://review.opendev.org/c/openstack/python-glanceclient/+/802890 | 12:40 |
| *** abhishekk is now known as akekane|home | 13:40 | |
| akekane|home | I don't see lance is back yet | 13:42 |
| dansmith | akekane|home: I think he's back, probably working out the irc change | 13:52 |
| dansmith | I'm guessing he probably can't be consulting on everyone's patches in the first few hours of his return :P | 13:52 |
| akekane|home | dansmith, ack :D | 13:52 |
| dansmith | akekane|home: I think I'm probably getting confused on this metadef stuff | 15:12 |
| dansmith | there are so many objects, relationships and patches that I'm losing track of what we're trying to do here | 15:13 |
| dansmith | my latest comments on your metadef objects patch might be exposing that confusion, | 15:13 |
| akekane|home | dansmith, world of metadefs | 15:13 |
| akekane|home | looking | 15:13 |
| dansmith | but in there it seems like you're checking one type of rule against a different type of thing (i.e. objects vs. namespaces) | 15:13 |
| dansmith | is the point to just replicate the very coarse "admin or not" permissions checks for metadef up in the api layer, or are you actually trying to fix the coarseness itself? | 15:14 |
| akekane|home | which comment you are talking about? | 15:16 |
| dansmith | akekane|home: well, L129 for instance | 15:17 |
| akekane|home | in the namespace patch | 15:18 |
| akekane|home | ? | 15:18 |
| dansmith | https://review.opendev.org/c/openstack/glance/+/799634/10/glance/api/v2/metadef_objects.py | 15:18 |
| dansmith | sorry, I said "metadef object patch" but that's probably not very clear, sorry | 15:18 |
| akekane|home | no problem, I am also in other discussion at that moment | 15:19 |
| akekane|home | ok | 15:20 |
| akekane|home | dansmith, here reason I have avoided policy check again as our database already returns formatted list to us | 15:21 |
| akekane|home | so if I have added that check then that check will always performed only positive enforcement and not negative one | 15:22 |
| dansmith | right, but the goal is to move those checks to the api right? otherwise what's the point? | 15:22 |
| akekane|home | Ok, lets go line by line | 15:22 |
| akekane|home | at line no 123 only I will get only those objects which are visible to me | 15:23 |
| akekane|home | So the query executed on line 123 will give me those result set | 15:24 |
| akekane|home | does this mean we need to change the query as well | 15:24 |
| dansmith | akekane|home: just like L87 here: https://review.opendev.org/c/openstack/glance/+/799633/13/glance/api/v2/metadef_namespaces.py | 15:25 |
| dansmith | but we're running each through the policy check so that the operator can write policy checks that actually do something right? | 15:25 |
| akekane|home | ack, will do it that way then | 15:26 |
| akekane|home | regarding your update and delete comment | 15:26 |
| dansmith | is there a reason not to? | 15:26 |
| akekane|home | Update and delete calls are admin only | 15:26 |
| dansmith | akekane|home: okay that's kinda what I was asking.. the point there is just to decide "can this user update/delete *any* object" and not to make it more flexible | 15:27 |
| dansmith | the admin could add more roles to the policy rule, but not say "you can update these but not those" | 15:27 |
| dansmith | because your comment says the DB layer will decide if you can update something or not, which is not exactly "admin only" so it's a bit confusing | 15:28 |
| akekane|home | Yeah, I will change that comment | 15:29 |
| dansmith | okay | 15:29 |
| akekane|home | I need to remove that comment from all CUD operations as it makes no sense | 15:29 |
| akekane|home | Regarding your comment at line #170 | 15:32 |
| akekane|home | Namespace is gatekeeper object | 15:32 |
| akekane|home | objects, properties, resource_types, tags etc all associated with namespace only | 15:33 |
| dansmith | okay but does it make sense to have a "can get objects inside a namespace I already have access to" permission? | 15:34 |
| akekane|home | and each get call checks for namespace is visible or not at db layer | 15:34 |
| dansmith | like, is there a reason to give someone access to see a namespace but not the things within it, if the things within are not actually restricted or restrictable? | 15:34 |
| akekane|home | I don't think so | 15:35 |
| dansmith | so I wonder if it just makes sense to deprecate all these checks other than the namespace one(s) and not check them after the refactor? | 15:35 |
| akekane|home | I am ok for that | 15:36 |
| akekane|home | but as of now, I have tried to maintain the behavior which is as close to current behavior | 15:36 |
| akekane|home | and then we can work on deprecating these policies which are actually causing confusion | 15:37 |
| akekane|home | also if you see auth layer checks you can see those are also based on namespace only | 15:40 |
| akekane|home | e.g. is_object_mutable call | 15:41 |
| dansmith | okay I guess I need to go look at those | 15:41 |
| dansmith | this is just a big mess | 15:41 |
| akekane|home | ++ | 15:41 |
| akekane|home | is_meta_resource_type_mutable | 15:41 |
| akekane|home | is_namespace_property_mutable | 15:41 |
| akekane|home | is_tag_mutable | 15:42 |
| dansmith | I get the argument for "just replicate what we have, deprecate later, | 15:42 |
| dansmith | but if it's hard to confirm that we're really replicating what is there today, I'm not sure it's really better/safer | 15:42 |
| dansmith | but I'll go try to convince myself by looking at the metadef auth layer after my next call | 15:42 |
| akekane|home | ack | 15:43 |
| akekane|home | I guess this is again has a different reality vs our policy check | 15:43 |
| * akekane|home will be back from dinner till then | 15:48 | |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef resource-types https://review.opendev.org/c/openstack/glance/+/799671 | 16:04 |
| akekane|home | dansmith, also you can refer 4th sheet from our spread sheet so that you can get some idea related to metadef policies as well | 16:29 |
| akekane|home | lbragstad, welcome back and congratulations | 16:31 |
| lbragstad | o/ thanks :) | 16:31 |
| akekane|home | Someday will like to see photo of the little one | 16:36 |
| dansmith | akekane|home: ugh.. too complicated, but yeah nice job with that at least :) | 16:38 |
| dansmith | lbragstad: o/ | 16:39 |
| dansmith | akekane|home: I guess I'm not sure why we're even doing all this for metadef.. meaning, why can't we just refactor to "admin can CRUD, otherwise everyone can read" and kinda deprecate the multi-tenancy part? | 16:42 |
| dansmith | it just seems like a crazy amount of work (as shown by the grid) for something 99.9999% of people will never use with private namespace objects | 16:43 |
| akekane|home | dansmith, so you are saying to get rid of visibility check, right ? | 16:44 |
| dansmith | I dunno what I'm saying, I just want to burn it all down :) | 16:46 |
| akekane|home | :D | 16:46 |
| dansmith | on the "before rbac" side, that's not really the way it is now, right? | 16:47 |
| dansmith | I mean, the default policices don't allow project members to create things anymore, | 16:47 |
| dansmith | but they could until last cycle, and could still have the old default rules | 16:47 |
| akekane|home | no they do not | 16:47 |
| dansmith | the new "after rbac" side seems too complicated to me, as it's returning us to "members can create metadef things" which seems to be of limited or no value to almost everyone, | 16:48 |
| dansmith | unless you've heard of new use-cases | 16:48 |
| akekane|home | where do you see member can create metadef things ? | 16:48 |
| dansmith | well, okay maybe I'm getting my rows mixed up, | 16:49 |
| akekane|home | for other than get operation member and reader both have forbidden for POST, PUT and Delete | 16:49 |
| dansmith | but in "after rbac" you've gone back to "can view only public and tenant b namespaces" | 16:50 |
| dansmith | but you're thinking that's because the *admin* would have created something for that tenant, right? | 16:50 |
| akekane|home | right | 16:50 |
| akekane|home | and based on our read only checks we have at auth layer | 16:51 |
| akekane|home | or is_namespace_visible check at db layer | 16:51 |
| dansmith | alright | 16:55 |
| akekane|home | I think I will restructure comments that will avoid confusion | 16:57 |
| akekane|home | probably I should avoid mentioning db layer checks at API layer :D | 16:57 |
| dansmith | there's really no way to avoid confusion with this stuff I think | 16:58 |
| *** akekane_ is now known as abhishekk | 17:01 | |
| abhishekk | croelandt, if you are around could you please have a look at this change https://review.opendev.org/c/openstack/glance/+/789913/16 | 17:10 |
| opendevreview | Dan Smith proposed openstack/glance master: Refactor gateway get_repo auth layer https://review.opendev.org/c/openstack/glance/+/789913 | 17:22 |
| opendevreview | Dan Smith proposed openstack/glance master: Make image update check policy at API layer https://review.opendev.org/c/openstack/glance/+/789915 | 17:22 |
| opendevreview | Dan Smith proposed openstack/glance master: Check get_image(s) in the API https://review.opendev.org/c/openstack/glance/+/796067 | 17:22 |
| opendevreview | Dan Smith proposed openstack/glance master: Add a member field to Image when appropriate https://review.opendev.org/c/openstack/glance/+/796066 | 17:22 |
| opendevreview | Dan Smith proposed openstack/glance master: POC: Check delete_image policy in the API https://review.opendev.org/c/openstack/glance/+/798073 | 17:22 |
| opendevreview | Dan Smith proposed openstack/glance master: POC: Check deactivate, reactivate policy in the API https://review.opendev.org/c/openstack/glance/+/798266 | 17:22 |
| opendevreview | Rajat Dhasmana proposed openstack/glance master: Refactor glance cinder tests https://review.opendev.org/c/openstack/glance/+/803240 | 17:43 |
| dansmith | croelandt: you commented on PS16 but I actually just updated PS17 with more words in that docstring.. maybe that clarifies it a bit (also replied), but if not I can add some more to that comment | 17:48 |
| croelandt | dansmith: damn, lemme check | 17:50 |
| akekane_ | may be I have shared the link with croelandt before your updates to PS | 17:50 |
| croelandt | oh yeah there was /16 at the end | 17:51 |
| croelandt | :D | 17:51 |
| croelandt | oh ok so legacy code implicitely passes True | 17:52 |
| croelandt | I see | 17:52 |
| croelandt | ok let's merge this one | 17:52 |
| dansmith | right, and then when everything is passing =False, we actually remove the flag entirely | 17:52 |
| akekane_ | thank you croelandt | 17:54 |
| *** akekane_ is now known as abhishekk | 17:54 | |
| dansmith | \o/ | 17:56 |
| croelandt | 3rd one looks good as well | 18:05 |
| dansmith | woohoo | 18:06 |
| abhishekk | croelandt, super | 18:08 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef namepsace policy checks in the API https://review.opendev.org/c/openstack/glance/+/799633 | 18:11 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef object policy checks in the API https://review.opendev.org/c/openstack/glance/+/799634 | 18:11 |
| abhishekk | dansmith, ^^ probably this will be less confusing :P | 18:11 |
| croelandt | dansmith: are the POCs patches you need comments on, or just work in progress not ready to be reviewed yet? | 18:12 |
| dansmith | croelandt: well, comments always welcome, but I've just not gotten to finishing those yet.. just wanting to get some of the stack flushed before I keep going | 18:14 |
| abhishekk | croelandt, comment will be good IMO so that we will not forget if there is any objection | 18:14 |
| dansmith | always open for comments though | 18:14 |
| dansmith | makes later review faster for you too :) | 18:14 |
| abhishekk | ++ | 18:15 |
| abhishekk | dansmith, based on above two patches I will work on other patches tomorrow my time | 18:15 |
| abhishekk | so if you have any suggestion please comment as per your convenient time | 18:15 |
| dansmith | okay will try to look over those in a bit | 18:17 |
| dansmith | I already see one GLARING -1 | 18:17 |
| abhishekk | aah | 18:18 |
| abhishekk | will stop further work then :D | 18:18 |
| dansmith | hehe | 18:18 |
| croelandt | The first two POC patches are fairly small | 18:21 |
| croelandt | which is nice | 18:21 |
| dansmith | because they're missing tests :) | 18:21 |
| abhishekk | :D | 18:22 |
| croelandt | I almost wish we could have the whole api_policy.ImageApiPolicy(req.context, image, self.policy).<method>() as a decorator, but that would make us call get_repo() and image_repo.get() twice, which would not be super efficient | 18:23 |
| dansmith | yeah, I mean we could make the decorator pass in the repo it got, but I think there are cases where we're checking things in different order and such | 18:23 |
| dansmith | and I figured too much magic hurts readability, especially while we're changing things from enforcement in one layer to another | 18:23 |
| croelandt | yeah | 18:24 |
| croelandt | in a way, having @check_policy_here would *help* readability | 18:25 |
| croelandt | but yeah sometimes we have to run checks in weird places | 18:25 |
| croelandt | so that might end up being worse | 18:25 |
| * abhishekk signing out for the day | 18:33 | |
| abhishekk | good night/day all | 18:34 |
| dansmith | croelandt: could always refactor to that once everything is uniform | 18:36 |
| croelandt | yeah | 18:37 |
| croelandt | let's get something workign first | 18:37 |
| dansmith | right now checks happen all over the place, but once everything is at the API layer and generally in the same format, we could then clean up to that | 18:37 |
| croelandt | we'll make it beautiful then | 18:37 |
| dansmith | like when we yank the auth layer for good | 18:37 |
| * lbragstad wanders in late | 19:17 | |
| lbragstad | fwiw, i got that same impression when i worked on the project personas in Wallaby | 19:18 |
| lbragstad | getting everything into one place would be nice | 19:18 |
| lbragstad | i kept running into issues getting things into a single place *and* making it work for all cases | 19:19 |
| croelandt | yeah | 19:28 |
| croelandt | there is already one case where we do the checks in an if/else | 19:29 |
| croelandt | so that would be weird with a generic decorator | 19:29 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!