| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef objects https://review.opendev.org/c/openstack/glance/+/802054 | 05:31 |
|---|---|---|
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef properties https://review.opendev.org/c/openstack/glance/+/802055 | 05:31 |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef tags https://review.opendev.org/c/openstack/glance/+/802056 | 06:14 |
| *** mabrams is now known as mabrams|afk | 07:03 | |
| *** mabrams|afk is now known as mabrams | 07:04 | |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef objects https://review.opendev.org/c/openstack/glance/+/802054 | 09:00 |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef properties https://review.opendev.org/c/openstack/glance/+/802055 | 09:00 |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef tags https://review.opendev.org/c/openstack/glance/+/802056 | 09:05 |
| opendevreview | Rajat Dhasmana proposed openstack/glance master: DNM: check grenade gate fix https://review.opendev.org/c/openstack/glance/+/803325 | 10:28 |
| opendevreview | Rajat Dhasmana proposed openstack/glance master: DNM: check grenade gate fix https://review.opendev.org/c/openstack/glance/+/803325 | 10:30 |
| opendevreview | Rajat Dhasmana proposed openstack/glance master: DNM: check grenade gate fix https://review.opendev.org/c/openstack/glance/+/803325 | 10:38 |
| opendevreview | Rajat Dhasmana proposed openstack/glance master: DNM: check grenade gate fix https://review.opendev.org/c/openstack/glance/+/803325 | 10:43 |
| opendevreview | Mridula Joshi proposed openstack/python-glanceclient master: Fix undesirable raw Python error https://review.opendev.org/c/openstack/python-glanceclient/+/803326 | 10:46 |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef objects https://review.opendev.org/c/openstack/glance/+/802054 | 11:22 |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef properties https://review.opendev.org/c/openstack/glance/+/802055 | 11:22 |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef tags https://review.opendev.org/c/openstack/glance/+/802056 | 11:22 |
| abhishekk | @all, glance gate is blocked due to grenade failure, kindly avoid rechecking until the fix https://review.opendev.org/c/openstack/grenade/+/803317 gets merged | 13:43 |
| dansmith | abhishekk: sorry I had half-reviewed the objects one yesterday and god distracted.. just finished | 13:51 |
| abhishekk | dansmith, no problem | 13:52 |
| abhishekk | hmm, I will use comprehension, just avoided as it was going out of pep8 standard (line length) | 13:53 |
| dansmith | abhishekk: I'm not saying you definitely should, | 13:53 |
| dansmith | I'm just saying it because it would be better | 13:53 |
| dansmith | how many metadef objects are people likely to have? | 13:53 |
| abhishekk | yes go it | 13:54 |
| abhishekk | I am not sure as there is no limit | 13:54 |
| abhishekk | per namespace I guess it will be less than 10 though | 13:54 |
| dansmith | okay, well, no need to change it, it's just that api stuff where "in testing I have two objects" can start to suck for real workloads when you have 2 million :) | 13:55 |
| abhishekk | hmm, right | 13:55 |
| abhishekk | I have fixed the spell mistake though (before your comment) :D | 13:56 |
| dansmith | heh | 13:56 |
| abhishekk | just didn't pushed as gate is broken | 13:56 |
| dansmith | ack | 13:56 |
| abhishekk | today I will not be much around but will be keeping eye on mails and gate situation | 13:58 |
| dansmith | okay | 13:58 |
| abhishekk | dansmith, if you still around I want to highlight one issue | 14:15 |
| dansmith | sure | 14:16 |
| abhishekk | nah, its not a issue, sorry | 14:17 |
| * abhishekk to much over thinking at the moment | 14:18 | |
| dansmith | hah | 14:18 |
| abhishekk | update, the grenade fix is approved so we can recheck in short period of time | 14:20 |
| croelandt | Anything urgent to review once the CI works again? | 14:30 |
| dansmith | once abhishekk pushes his metadef set, that would be next I think | 14:31 |
| dansmith | waiting to push revs until the gate is fixed to avoid a bunch of useless checks | 14:31 |
| abhishekk | Yeah | 14:31 |
| croelandt | the grenade patch has just been approved | 14:32 |
| croelandt | abhishekk: can you check Rajat's email btw? | 14:32 |
| abhishekk | croelandt, ack | 14:32 |
| dansmith | abhishekk: I think you could depends-on the grenade fix on your bottom patch and get a run with the new bits | 14:32 |
| croelandt | I can do the backports both upstream and downstream, but we just need to agree with Cinder on how to do it :) | 14:33 |
| abhishekk | dansmith, ack, then I will fix the comprehension and test change as next revision | 14:33 |
| abhishekk | croelandt, right | 14:33 |
| abhishekk | croelandt, I will push two patches in 15-20 minutes | 14:39 |
| croelandt | abhishekk: on which topic? | 14:40 |
| abhishekk | policy refactor + metadef | 14:40 |
| croelandt | ok | 14:40 |
| abhishekk | will ping you once done | 14:40 |
| abhishekk | I think grenade patch will take 4-5 hours to merge | 14:49 |
| croelandt | erf :/ | 14:51 |
| croelandt | Should I do night-time reviews? :) | 14:51 |
| abhishekk | :D | 14:52 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef namespace policy checks in the API https://review.opendev.org/c/openstack/glance/+/799633 | 14:54 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef object policy checks in the API https://review.opendev.org/c/openstack/glance/+/799634 | 14:54 |
| abhishekk | croelandt, dansmith ^^ | 14:55 |
| abhishekk | Now I need to replicate these comments in other three patches that will take another hour | 14:55 |
| croelandt | which comments? | 14:57 |
| abhishekk | on above two patches dansmith has given some comments which I have fixed and proposed new PS | 14:58 |
| abhishekk | those comments I need to fix in my other patches as well as most of the code is common | 14:59 |
| croelandt | ok ok | 15:00 |
| * abhishekk going for dinner break | 16:02 | |
| abhishekk | update, grenade patch is still in the gate and will take approx 4-5 hours more to merge | 16:46 |
| dansmith | yeah, I'm sure :/ | 16:47 |
| dansmith | gmann: lbragstad: I have a API/policy question | 16:47 |
| lbragstad | o/ | 16:48 |
| dansmith | we have several places where we currently raise 403 for an operation which might be forbidden by policy | 16:48 |
| dansmith | I think we have the proper behavior today, but only because it's all hard-coded in the DB, where we'd return 404 if you're not able to see the image, | 16:49 |
| dansmith | but 403 if you can see it, but aren't allowed to delete it or something like that | 16:49 |
| dansmith | I'm realizing that converting all that to policy we might be doing the wrong thing in places, | 16:49 |
| dansmith | where we might convert the 403 to 404 to avoid exposing the image's existence to people that can't see it, | 16:50 |
| dansmith | but a consequence is that we might show someone an image, but then return 404 when they try to delete it, if not allowed | 16:50 |
| dansmith | which surely isn't right | 16:50 |
| dansmith | so I'm thinking we probably need to, in all of our Forbidden handlers have some sort of "if can_get_image: return 403 else return 404" | 16:51 |
| dansmith | right? | 16:51 |
| lbragstad | i think that makes sense | 16:52 |
| lbragstad | i think we had a very similar discussion to this a while back that focused on the inverse, which is what we did in keystone | 16:53 |
| lbragstad | (we return 403 instead of 404) | 16:53 |
| dansmith | yeah, well, if you read the glance code it looks like we return 403 in a lot of places that we don't, because it's hard-codes in the depths to do something different | 16:54 |
| lbragstad | i thought there was a utility method somewhere in the database layer that determines if a thing (like an image) is visible and then throws the right exception (404 or 403) based on that | 16:56 |
| lbragstad | is that what you mean by depths? | 16:56 |
| dansmith | I don't think there's a utility method for that, but that logic is spread all over the db yeah | 16:56 |
| lbragstad | i think i'm thinking of this - https://github.com/openstack/glance/blob/master/glance/db/sqlalchemy/api.py#L286-L290 | 16:59 |
| dansmith | yeah, that's not really the same I don't think, because that's just the image fetch path | 17:00 |
| 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 | 17:16 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef property policy checks in the API https://review.opendev.org/c/openstack/glance/+/799635 | 17:16 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move metadef tag policy checks in the API https://review.opendev.org/c/openstack/glance/+/799636 | 17:16 |
| gmann | dansmith: lbragstad +1 on checking image GET permission for other not allowed operation too. | 17:25 |
| dansmith | ack, I have a good solution that fits nicely into our new v2 layer I think | 17:26 |
| dansmith | will push that up in a sec | 17:26 |
| gmann | and this issue is for other project also not only glance. like in nova we return 404 if server does not exist to user who does not have permission for GET /servers | 17:26 |
| dansmith | right | 17:27 |
| gmann | but is there case anyone want to restrict GET but not Delete ? | 17:28 |
| gmann | in that case, checking GET permission in Delete can break them | 17:28 |
| gmann | dansmith: lbragstad on rethinking, I am not 100% sure about checking GET permission in Delete or other operation. actually if we see getting 404 does not tell 100% that image does not exist. 404 can be from url not present or so. | 17:33 |
| gmann | I think what we can do is not show the 'image not found' error message in such case | 17:34 |
| gmann | just log | 17:34 |
| dansmith | gmann: there are cases where you want to restrict GET but allow DELETE, like project cleanup scripts that should not see image metadata with license keys | 17:34 |
| dansmith | and this doesn't change delete, it just changes the return code you get if you're not allowed to delete | 17:34 |
| dansmith | if you can delete, we never check get | 17:34 |
| gmann | yeah. and as we have granular policy so we can say yes there are use case | 17:34 |
| dansmith | we only check it to determine 403/404 on fail | 17:35 |
| gmann | dansmith: ohk. still it has both point, 1. returning 403 can confuse API user and they ask for permission where actually they are requesting non-existing image 2. returning 404 can leak image non-exit info to user not having permission to GET or Delete. | 17:38 |
| gmann | some tradeoff we have to do in any ways | 17:39 |
| gmann | because we cannot check delete permission without image_id right? so in case of 'image not exist' we actually do not know if delete is allowed or not | 17:41 |
| dansmith | gmann: no, we'll still return 404 for an image that doesn't exist | 17:41 |
| gmann | so which case you will convert it to 403 | 17:42 |
| dansmith | image exists, you're allowed to see image, you are not allowed to delete | 17:42 |
| dansmith | if not exists, 404, if exists but you cannot see it, 404, if it exists, you can see it, can delete it, 200 | 17:43 |
| gmann | sorry I am confused. so in delete case what change you are proposing ? | 17:45 |
| dansmith | okay, right now what we have proposed will result in a user being able to GET an image, but if they are not allowed to DELETE, they would get 404, but then they could GET the image right after and see it still exists and be confused | 17:46 |
| dansmith | what I want is to make sure that if a user can GET an image, but not DELETE it, that they receive a 403 instead of a 404. | 17:47 |
| dansmith | if they cannot GET an image, then a DELETE on that image should still be a 404 | 17:47 |
| gmann | ohk, i thought 'able to GET an image, but if they are not allowed to DELETE, they would get 404' return 403 currently. +1. so the way how nova return. | 17:48 |
| gmann | so no GET permission check in Delete basically and keep returning 404 if resource does not exist and we cannot check delete permission too | 17:49 |
| dansmith | gmann: currently that's true in glance too I think, but only because there is hard-coded python below the policy layer that subverts the actual policy behavior in the api | 17:49 |
| dansmith | gmann: right | 17:49 |
| gmann | i see. thanks | 17:49 |
| gmann | yeah moving them to API layer is nice. | 17:50 |
| opendevreview | Dan Smith proposed openstack/glance master: Make image update check policy at API layer https://review.opendev.org/c/openstack/glance/+/789915 | 18:36 |
| opendevreview | Dan Smith proposed openstack/glance master: Check get_image(s) in the API https://review.opendev.org/c/openstack/glance/+/796067 | 18:36 |
| opendevreview | Dan Smith proposed openstack/glance master: Add a member field to Image when appropriate https://review.opendev.org/c/openstack/glance/+/796066 | 18:36 |
| opendevreview | Dan Smith proposed openstack/glance master: Check delete_image policy in the API https://review.opendev.org/c/openstack/glance/+/798073 | 18:36 |
| opendevreview | Dan Smith proposed openstack/glance master: POC: Check deactivate, reactivate policy in the API https://review.opendev.org/c/openstack/glance/+/798266 | 18:36 |
| dansmith | gmann: this summarizes the behavior: https://review.opendev.org/c/openstack/glance/+/798073/19/glance/tests/functional/v2/test_images_api_policy.py | 18:36 |
| dansmith | actually, let me add the not-exist case in there just to be sure | 18:37 |
| opendevreview | Dan Smith proposed openstack/glance master: Check delete_image policy in the API https://review.opendev.org/c/openstack/glance/+/798073 | 18:38 |
| opendevreview | Dan Smith proposed openstack/glance master: POC: Check deactivate, reactivate policy in the API https://review.opendev.org/c/openstack/glance/+/798266 | 18:38 |
| dansmith | gmann: https://review.opendev.org/c/openstack/glance/+/798073/20/glance/tests/functional/v2/test_images_api_policy.py | 18:38 |
| abhishekk | update, grenade fix is merged | 18:39 |
| dansmith | abhishekk: sweet.. note slight change to image behavior starting in the image update patch | 18:39 |
| dansmith | per discussion above | 18:39 |
| dansmith | didn't really think about it until I was writing a test for delete | 18:40 |
| abhishekk | yes, going through now | 18:40 |
| dansmith | abhishekk: fine to look tomorrow, I'm sure it's late | 18:40 |
| gmann | dansmith: L244, should not it be 403 ? | 18:40 |
| dansmith | gmann: no, because we can't see the image, so we should not be able to see that it's there by trying to delete | 18:41 |
| gmann | dansmith: but we should not check GET permission in Delete right | 18:41 |
| abhishekk | yeah | 18:41 |
| dansmith | we should, but only to determine 403/404 | 18:41 |
| abhishekk | dansmith, There are still some things that have to be resolved, like the image | 18:42 |
| dansmith | not to determine if you can delete or not, | 18:42 |
| abhishekk | repo will still check get_image even though we don't want it to. | 18:42 |
| dansmith | ONLY for the return code | 18:42 |
| abhishekk | is this still the case or it is there and need to remove from commit message ? | 18:42 |
| dansmith | abhishekk: commit msg on which patch? | 18:43 |
| gmann | dansmith: but where you are checking that https://review.opendev.org/c/openstack/glance/+/798073/20/glance/api/v2/images.py | 18:43 |
| abhishekk | https://review.opendev.org/c/openstack/glance/+/789915 | 18:43 |
| dansmith | gmann: https://review.opendev.org/c/openstack/glance/+/798073/20/glance/api/v2/policy.py line 72 | 18:44 |
| dansmith | abhishekk: yeah, image repo still does that checking very low down, so that statement is still valid right? | 18:44 |
| dansmith | abhishekk: no change from last rev that was approved, in that regard | 18:45 |
| abhishekk | ack | 18:45 |
| gmann | dansmith: I see. this looks good to me. | 18:49 |
| opendevreview | Dan Smith proposed openstack/glance master: Check delete_image policy in the API https://review.opendev.org/c/openstack/glance/+/798073 | 18:49 |
| opendevreview | Dan Smith proposed openstack/glance master: POC: Check deactivate, reactivate policy in the API https://review.opendev.org/c/openstack/glance/+/798266 | 18:49 |
| dansmith | pep8 sorry ^ | 18:49 |
| abhishekk | nicely handled | 18:50 |
| abhishekk | will go through in detail tomorrow morning | 18:50 |
| dansmith | for that delete test, I could also add a case where get_image is denied, but delete is allowed, to prove we don't check get_image unless we need to return an error | 18:50 |
| dansmith | abhishekk: ack thanks, have a good night | 18:50 |
| abhishekk | thanks, good day o/~ | 18:51 |
| abhishekk | just FYI I attended RBAC open hour but agenda was full so not able to discuss anything with lbragstad (may be next week) | 18:52 |
| opendevreview | Dan Smith proposed openstack/glance master: Check delete_image policy in the API https://review.opendev.org/c/openstack/glance/+/798073 | 18:52 |
| opendevreview | Dan Smith proposed openstack/glance master: POC: Check deactivate, reactivate policy in the API https://review.opendev.org/c/openstack/glance/+/798266 | 18:52 |
| dansmith | gmann: added that case ^ | 18:52 |
| dansmith | abhishekk: ah cool, I know the special currency needed to bribe lbragstad to answer our questions first anyway, if need be :) | 18:53 |
| abhishekk | :D | 18:53 |
| dansmith | crap, another typo | 18:54 |
| * dansmith is in the patch revision olympics | 18:54 | |
| abhishekk | I will get gold there :P | 18:55 |
| dansmith | hah | 18:55 |
| opendevreview | Dan Smith proposed openstack/glance master: Check delete_image policy in the API https://review.opendev.org/c/openstack/glance/+/798073 | 18:55 |
| * abhishekk finally shutting down | 18:55 | |
| dansmith | o/ | 18:55 |
| abhishekk | o/ | 18:55 |
| gmann | perfect. | 18:58 |
| prometheanfire | https://review.opendev.org/803113 shows that glance has some issues with the newer networkx release | 20:04 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!