| *** whoami-rajat has quit IRC | 00:15 | |
| *** johanssone has quit IRC | 00:16 | |
| *** tosky has quit IRC | 00:17 | |
| *** johanssone has joined #openstack-glance | 00:19 | |
| *** felixhuettner[m] has quit IRC | 00:19 | |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: Implement API protection testing for images https://review.opendev.org/c/openstack/glance-tempest-plugin/+/773568 | 00:24 |
|---|---|---|
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: Add tests for image membership, deactivation, and reactivation https://review.opendev.org/c/openstack/glance-tempest-plugin/+/775742 | 00:24 |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: WIP: Lay down system persona test plumbing https://review.opendev.org/c/openstack/glance-tempest-plugin/+/778343 | 00:24 |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: Implement API protection testing for images https://review.opendev.org/c/openstack/glance-tempest-plugin/+/773568 | 00:26 |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: Add tests for image membership, deactivation, and reactivation https://review.opendev.org/c/openstack/glance-tempest-plugin/+/775742 | 00:26 |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: WIP: Lay down system persona test plumbing https://review.opendev.org/c/openstack/glance-tempest-plugin/+/778343 | 00:26 |
| *** felixhuettner[m] has joined #openstack-glance | 00:28 | |
| dansmith | lbragstad: just thought of something else.. | 00:29 |
| dansmith | er, wait, nevermind | 00:29 |
| * dansmith needs to dinner | 00:30 | |
| lbragstad | ack | 00:30 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Implement project personas for image actions https://review.opendev.org/c/openstack/glance/+/764754 | 00:31 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Update default policies for task API https://review.opendev.org/c/openstack/glance/+/763208 | 00:31 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Make secure RBAC protection job voting https://review.opendev.org/c/openstack/glance/+/778258 | 00:31 |
| lbragstad | ok - that should take care of things? assuming the gtp tests still work and i didn't fat-finger something | 00:32 |
| dansmith | lbragstad: yeah, on your copy-image comment, | 00:35 |
| dansmith | it's fine to default to admin, but I can override to include regular members with what you have there right? | 00:35 |
| dansmith | the comment is more "if we want the *default* to be different" or something right? | 00:35 |
| dansmith | okay I think the comment is about you using a simplistic check_str default, | 00:38 |
| dansmith | unlike the ADMIN_OR_PROJECT_MEMBER on most of the other rules | 00:38 |
| dansmith | also commented on the set_image_location thing | 00:40 |
| dansmith | will dig into that tomorrow.. didn't notice it before | 00:40 |
| *** Underknowledge has quit IRC | 01:08 | |
| *** Underknowledge has joined #openstack-glance | 01:08 | |
| *** gyee has quit IRC | 01:17 | |
| openstackgerrit | Dan Smith proposed openstack/glance master: Add housekeeping module and staging cleaner https://review.opendev.org/c/openstack/glance/+/777012 | 01:36 |
| *** Underknowledge2 has joined #openstack-glance | 01:51 | |
| *** Underknowledge has quit IRC | 01:54 | |
| *** Underknowledge2 is now known as Underknowledge | 01:54 | |
| *** rcernin has quit IRC | 02:00 | |
| lbragstad | yeah - you can override copy_image to be "" if you want and your nova job should still work | 02:14 |
| lbragstad | with enforce_scope = True | 02:14 |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: WIP: Lay down system persona test plumbing https://review.opendev.org/c/openstack/glance-tempest-plugin/+/778343 | 02:29 |
| *** rcernin has joined #openstack-glance | 02:29 | |
| *** rcernin has quit IRC | 02:29 | |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: WIP: Lay down system persona test plumbing https://review.opendev.org/c/openstack/glance-tempest-plugin/+/778343 | 02:29 |
| *** felixhuettner[m] has quit IRC | 02:29 | |
| *** rcernin has joined #openstack-glance | 02:30 | |
| *** felixhuettner[m] has joined #openstack-glance | 02:33 | |
| *** Underknowledge has quit IRC | 02:38 | |
| *** rcernin has quit IRC | 02:44 | |
| *** rcernin has joined #openstack-glance | 03:16 | |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Implement project personas for image actions https://review.opendev.org/c/openstack/glance/+/764754 | 03:18 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Update default policies for task API https://review.opendev.org/c/openstack/glance/+/763208 | 03:18 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Make secure RBAC protection job voting https://review.opendev.org/c/openstack/glance/+/778258 | 03:18 |
| *** dasp has quit IRC | 03:34 | |
| *** dasp has joined #openstack-glance | 03:45 | |
| *** k_mouza has joined #openstack-glance | 04:00 | |
| *** k_mouza has quit IRC | 04:05 | |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: Increase oslo.policy logging for protection tests https://review.opendev.org/c/openstack/glance-tempest-plugin/+/778356 | 04:14 |
| *** zzzeek has quit IRC | 04:17 | |
| *** zzzeek has joined #openstack-glance | 04:17 | |
| *** zzzeek has quit IRC | 04:22 | |
| *** zzzeek has joined #openstack-glance | 04:23 | |
| *** udesale has joined #openstack-glance | 04:35 | |
| *** ratailor has joined #openstack-glance | 04:54 | |
| *** whoami-rajat has joined #openstack-glance | 04:58 | |
| *** ratailor_ has joined #openstack-glance | 05:26 | |
| *** ratailor has quit IRC | 05:29 | |
| *** ratailor__ has joined #openstack-glance | 05:34 | |
| *** m75abrams has joined #openstack-glance | 05:35 | |
| *** ratailor_ has quit IRC | 05:37 | |
| *** ajitha has joined #openstack-glance | 05:48 | |
| *** zzzeek has quit IRC | 06:06 | |
| *** zzzeek has joined #openstack-glance | 06:41 | |
| *** ralonsoh has joined #openstack-glance | 07:37 | |
| *** rcernin has quit IRC | 07:44 | |
| *** tosky has joined #openstack-glance | 08:34 | |
| *** jdillaman has quit IRC | 08:44 | |
| *** jdillaman has joined #openstack-glance | 08:45 | |
| *** Underknowledge has joined #openstack-glance | 09:11 | |
| openstackgerrit | Merged openstack/python-glanceclient master: Add Python3 wallaby unit tests https://review.opendev.org/c/openstack/python-glanceclient/+/750701 | 09:37 |
| *** lpetrut has joined #openstack-glance | 10:22 | |
| *** k_mouza has joined #openstack-glance | 10:36 | |
| *** bhagyashris is now known as bhagyashris|rove | 11:30 | |
| *** bhagyashris|rove is now known as bhagyashri|rover | 11:30 | |
| *** udesale_ has joined #openstack-glance | 11:57 | |
| *** udesale has quit IRC | 12:00 | |
| *** legochen_ has joined #openstack-glance | 12:42 | |
| *** legochen_ is now known as legochen | 12:43 | |
| *** ratailor__ has quit IRC | 13:19 | |
| *** dirtwash has joined #openstack-glance | 13:40 | |
| dirtwash | so I just setup a fresh ceph nautilus cluster with a fresh victoria openstanck and again snapshost fail, this is related to https://bugs.launchpad.net/glance/+bug/1916482 | 13:42 |
| openstack | Launchpad bug 1916482 in Glance "rbd.IncompleteWriteError: RBD incomplete write (Wrote only 8388608 out of 8394566 bytes) since Victoria Upgrade, ceph v nautilus" [Undecided,New] | 13:42 |
| dirtwash | seems defo incompatiblity issue with Victoria glance release and ceph v14/nautilus | 13:42 |
| jokke | dirtwash: thanks, will try to look into the details today | 13:50 |
| *** jv_ has quit IRC | 13:55 | |
| *** jv_ has joined #openstack-glance | 14:00 | |
| dirtwash | kinda important because most big ceph users dont run latset stable (octoupus) because of early issues, all major clusters I know are still Nautilus, mine included | 14:07 |
| *** takamatsu has joined #openstack-glance | 14:17 | |
| *** jdillaman has quit IRC | 14:18 | |
| *** jdillaman has joined #openstack-glance | 14:18 | |
| *** jv_ has quit IRC | 14:33 | |
| *** jv_ has joined #openstack-glance | 14:34 | |
| openstackgerrit | Dan Smith proposed openstack/glance master: Add administrator docs for distributed-import https://review.opendev.org/c/openstack/glance/+/778072 | 14:38 |
| openstackgerrit | Dan Smith proposed openstack/glance master: Enable second glance worker for import testing https://review.opendev.org/c/openstack/glance/+/770629 | 14:38 |
| *** jv_ has quit IRC | 14:59 | |
| *** jdillaman has quit IRC | 15:00 | |
| *** jv_ has joined #openstack-glance | 15:07 | |
| *** lpetrut has quit IRC | 15:09 | |
| *** happyhemant has joined #openstack-glance | 15:13 | |
| dansmith | abhishekk: this is majorly cut down to just the important stuff and looks good to me: https://review.opendev.org/c/openstack/glance-tempest-plugin/+/773568 | 15:19 |
| dansmith | do you think we're ready to go ahead and get that merged to help keep the number of pending patches down? | 15:19 |
| jokke | dansmith: you around? | 15:33 |
| dansmith | yup | 15:33 |
| tosky | there is a small cleanup (almost typo fix) you can merge before anything else: https://review.opendev.org/c/openstack/glance-tempest-plugin/+/773562 | 15:34 |
| jokke | dansmith: about the cleanup. Did you test that without moving the create pool out of the if clause? If I'm reading the code flow correctly in multi worker environment that pool gets actually never utilized and it would exists when called if it was still within the condition | 15:35 |
| dansmith | tosky: got it :) | 15:35 |
| dansmith | jokke: yeah, I moved it because of the way we start functional workers I think... move it back and run those and I think you'll see a NameError when we go to use self.pool | 15:36 |
| dansmith | jokke: if I'm mistaken, then I can move it back (although having instance variables sometimes missing is confusing anyway) | 15:36 |
| dansmith | I also really *really* don't think it's something we need to worry much about in terms of the overhead it may bring | 15:37 |
| jokke | dansmith: sure I'll give it a try. I thought each of the workers actually set their pool as self.pool as part of their spawn process. So the self.pool should be pointing to the last worker and then all the pools are in the async pool list | 15:37 |
| jokke | dansmith: true, just thinking we might be actually creating extra pool that gets never used unless workers == 0 | 15:38 |
| dansmith | I understand, I just don't think that's a huge deal.. if it pre-allocated a thousand threads then maybe, but .. :) | 15:39 |
| jokke | yeah thankfully afaik the eventlet pools are not preallocating anything but the thread tracker. I think that's why we default like 1000 threads on them | 15:39 |
| dansmith | yeah | 15:41 |
| abhishekk | dansmith, So this https://review.opendev.org/c/openstack/glance-tempest-plugin/+/773568 is the patch which will verify that legacy rbac is working correctly and secure-rbac will fail until we adopt to new changes | 15:55 |
| dansmith | abhishekk: yep | 15:56 |
| abhishekk | cool | 15:56 |
| abhishekk | will have a look soon enough | 15:57 |
| openstackgerrit | Dan Smith proposed openstack/glance master: Add housekeeping module and staging cleaner https://review.opendev.org/c/openstack/glance/+/777012 | 16:02 |
| openstackgerrit | Dan Smith proposed openstack/glance master: Add a check on startup for staging directory https://review.opendev.org/c/openstack/glance/+/778493 | 16:02 |
| *** takamatsu has quit IRC | 16:11 | |
| *** smcginnis has quit IRC | 16:25 | |
| *** smcginnis has joined #openstack-glance | 16:34 | |
| *** m75abrams has quit IRC | 16:48 | |
| *** udesale_ has quit IRC | 16:52 | |
| *** jv_ has quit IRC | 17:01 | |
| *** ralonsoh has quit IRC | 17:01 | |
| *** hoonetorg has quit IRC | 17:01 | |
| *** stand has quit IRC | 17:01 | |
| *** lifeless_ has quit IRC | 17:01 | |
| dansmith | lbragstad: I'm really struggling with these: https://review.opendev.org/c/openstack/glance-tempest-plugin/+/775742 | 17:01 |
| dansmith | I started by offering to comment them for you as I went through figuring them out, but ... I was stopped at like the second one | 17:01 |
| *** openstackgerrit has quit IRC | 17:02 | |
| lbragstad | dansmith ack - i'll take a look here soon | 17:03 |
| dansmith | ack, sorry, I'm not very smart | 17:04 |
| *** ralonsoh has joined #openstack-glance | 17:05 | |
| *** stand has joined #openstack-glance | 17:05 | |
| lbragstad | i can understand the confusion - it's a lot to keep straight when you start dealing with multiple projects and clients | 17:05 |
| dansmith | yeah, it's more than I have fingers and toes :) | 17:06 |
| *** jv_ has joined #openstack-glance | 17:07 | |
| *** hoonetorg has joined #openstack-glance | 17:07 | |
| *** lifeless_ has joined #openstack-glance | 17:07 | |
| abhishekk | rosmaita, when you get time, please have a look at https://review.opendev.org/c/openstack/glance/+/775860 and https://review.opendev.org/c/openstack/python-glanceclient/+/776403 | 17:23 |
| rosmaita | abhishekk: ack | 17:23 |
| abhishekk | both are good to go and will reduce our M3 review list | 17:23 |
| abhishekk | thank you | 17:23 |
| dansmith | abhishekk: in order to test the cleanup of the conversion files in the functional test, I need to have them in the database | 17:46 |
| dansmith | abhishekk: is it okay to just use the existing uuids.stale as the base, and create several more files for it like .raw and .uc and make sure those get hit? | 17:47 |
| dansmith | I can create another image for those too, but it'll just make that test longer | 17:47 |
| abhishekk | I think it will be ok to use uuids.stale as base | 17:47 |
| dansmith | er, not uuids. stale, but the image I create/stage above | 17:47 |
| dansmith | okay, I think it's close enough too, just wanted to make sure | 17:48 |
| abhishekk | ack | 17:48 |
| dansmith | actually, nevermind all that I'm being stupid | 17:49 |
| dansmith | not a great day for me today :) | 17:50 |
| abhishekk | :D | 17:50 |
| dansmith | abhishekk: ^ | 18:00 |
| dansmith | er, where is the bot? :) | 18:00 |
| abhishekk | I think bot is also not having good day :D | 18:01 |
| dansmith | haha | 18:01 |
| abhishekk | I got email update though, thank you | 18:01 |
| dansmith | cool | 18:01 |
| abhishekk | dansmith, could you please verify glance-store hash | 18:08 |
| abhishekk | https://review.opendev.org/c/openstack/releases/+/777970 | 18:09 |
| abhishekk | I think it has +2 from zuul, but not merged yet | 18:09 |
| dansmith | abhishekk: done | 18:11 |
| abhishekk | thank you | 18:11 |
| dansmith | maybe those are manual merges? it completed the gate | 18:13 |
| abhishekk | I think it will revisit gate once +w hits | 18:15 |
| abhishekk | smcginnis, ^^ | 18:15 |
| dansmith | ah, I see he dropped it, gotcha | 18:16 |
| abhishekk | me signing out for the day, will revisit rbac patches tomorrow | 18:17 |
| dansmith | o/ | 18:17 |
| abhishekk | o/~ | 18:17 |
| jokke | night | 18:18 |
| *** ralonsoh has quit IRC | 18:23 | |
| jokke | lbragstad: quick follow up about the community/public etc. all that. | 18:28 |
| dansmith | lbragstad: and while fixing whatever jokke just found, remove your non-voting job from the gate queue :) https://review.opendev.org/c/openstack/glance/+/778079 | 18:29 |
| * lbragstad nods | 18:30 | |
| lbragstad | dansmith quick spot check | 18:30 |
| dansmith | I can ninja that patch when you fix | 18:31 |
| jokke | lbragstad: so yeah I saw your comment now on the add_image policy and how it kind of never hits in that specific case due to the other policy being on the way. Is it just perception thing (as in the string looks bad and like it could allow such) or is it as scary as it looks like and we need to be super careful? | 18:31 |
| lbragstad | https://review.opendev.org/c/openstack/glance-tempest-plugin/+/778538 is this the type of clarification you're asking for dansmith? | 18:32 |
| lbragstad | jokke well - it could be scary if something changes the order of operations as far as policy is concerned | 18:33 |
| dansmith | lbragstad: in general yeah, I'll go look, but... can't we have those comments in the patch where you're adding the tests to make it easier to review? | 18:33 |
| lbragstad | or if glance decides to not nest policy enforcement checks | 18:33 |
| lbragstad | dansmith yeah - i'm planning on proposing that in the review you had comments on | 18:34 |
| jokke | lbragstad: so the policy actually works like I described in my comment and we're pretty much relying something else catching it first to avoid horrible things? :P | 18:34 |
| lbragstad | i just threw that up as a wip so you could easily see the different | 18:34 |
| lbragstad | difference* | 18:34 |
| dansmith | lbragstad: ack cool | 18:34 |
| lbragstad | jokke well - yeah.. kind of | 18:36 |
| lbragstad | just about every layer of glance wants at least *some* responsibility for authorization | 18:36 |
| jokke | lbragstad: for example my delete question below was very related for that exact reason. AFAIK there is no other policy to safeguard it | 18:36 |
| lbragstad | so - if we don't trip on the twig, we'll trip on the branch in the next step | 18:37 |
| jokke | lbragstad: I know :( | 18:37 |
| lbragstad | but - i understand what you're saying | 18:37 |
| lbragstad | i think to address that | 18:37 |
| jokke | we have loads of authorization that is 100% not relying the policies to check if action is ok or not. | 18:37 |
| jokke | which is sad | 18:38 |
| lbragstad | agreed - but.. i think the goal today should be | 18:38 |
| lbragstad | write a policy check string that encompasses the ideal use case | 18:38 |
| lbragstad | and make sure the behavior doesn't break | 18:38 |
| *** lpetrut has joined #openstack-glance | 18:38 | |
| lbragstad | then we at least know the policy check isn't too restrictive | 18:39 |
| lbragstad | and as we sweep the authorization bits out of the database/authorization/controllers and into the policy layer | 18:39 |
| lbragstad | the policy check string will naturally take on more authorization responsibility | 18:39 |
| lbragstad | and do what it's supposed to do | 18:39 |
| lbragstad | for example | 18:39 |
| lbragstad | right now "get_image" has something like this for a check string | 18:40 |
| lbragstad | "role:admin or (role:reader and (project_id:%(project_id)s or project_id:%(member_id)s or "community":%(visibility)s or "public":%(visibility)s))" | 18:40 |
| lbragstad | but the community and public parts of that check string aren't *fully* tested by the policy integration tests because that enforcement still lives in the database (from what I can understand and test) | 18:41 |
| lbragstad | but - at least we have a check string that's relatively realistic and should hopefully act as a safeguard if/when we do start refactoring the database bits | 18:42 |
| jokke | lbragstad: also added clarification to https://accountantonline.ie/accountancy-fees/ | 18:47 |
| jokke | sorry | 18:48 |
| jokke | https://review.opendev.org/c/openstack/glance/+/764754/23/glance/policies/image.py#307 | 18:48 |
| jokke | ^^ right link :D | 18:48 |
| *** lpetrut has quit IRC | 18:49 | |
| lbragstad | ok - maybe i'm confused | 18:49 |
| lbragstad | i wasn't sure if we were talking about the add_image policy or the reactivate policy | 18:49 |
| jokke | lbragstad: ok that makes bit more sense in general. Not less scary looking 'though | 18:50 |
| jokke | lbragstad: oh I was going through your responses. | 18:50 |
| lbragstad | ok - so end users are allowed to deactivate images, but only administrators can reactivate them? | 18:51 |
| lbragstad | s/end users/project-members/ | 18:51 |
| jokke | the discussion here was mainly the impact of that check string, cause that looks in general scary AF. But lots of those thing you said makes more sense when you take the db restrictions and all that in to account | 18:51 |
| jokke | lbragstad: correct. as long as they are part of the owner tenant | 18:52 |
| lbragstad | so - if i decide to deactivate all images before i get fired, my manager has to contact the openstack operators to re-enable all of them after i've been escorted out? | 18:53 |
| lbragstad | s/all images/all images in my project/ | 18:53 |
| * dansmith takes note | 18:53 | |
| jokke | lbragstad: that's how it was designed ... which would explain why you got fired :P | 18:54 |
| dansmith | hah | 18:54 |
| jokke | it's still less pain in the bollox than you deleting them which you would have permissions to do as well | 18:55 |
| jokke | and would likely got you fired too | 18:55 |
| lbragstad | well - i was probably fired for other reasons, but as a disgruntled employee i could hose images for my immediate project members and know it requires a ticket-based workflow somewhere | 18:55 |
| dansmith | yeah, it seems odd to me | 18:56 |
| lbragstad | just playing devils advocate as someone really looking to stick it to the man before they get booted out the door :) | 18:58 |
| jokke | IIRC the motivation for that was somewhere around these lines. Employee X finds weird behaviour on one of their images, deactivates it so no-one boots more instances from it before it's deemed safe. Now Employee Z who actually uploaded that malicious image, would obviously notice that his image got jammed so all he had to do was to either reactivate it or delete and recreate to play little more | 18:59 |
| jokke | time for it. | 18:59 |
| jokke | Specially if you use osc and rely image names that would be super effective | 19:00 |
| lbragstad | ok - so that's assuming X and Z are both users within the same project | 19:00 |
| lbragstad | right? | 19:00 |
| jokke | so the image literally gets locked before admin gives it a pass as default policy | 19:00 |
| jokke | lbragstad: yeah part of same tenant/project/<inject string what they are called today here> | 19:01 |
| lbragstad | enemy in the wire kind of thing? | 19:01 |
| jokke | most of the time attacks comes from inside | 19:01 |
| jokke | also admin could deactivate weirdly behaving image in their cloud instead of just deleting it for investigation | 19:02 |
| lbragstad | yeah - i think the admin case makes sense | 19:02 |
| jokke | so it's really quarantine operation | 19:02 |
| lbragstad | i think it's certainly within their rights to activate and deactivate whatever they want | 19:02 |
| lbragstad | i guess i really wanted to understand the reason why project members didn't have the ability to reactivate images and require operator intervention | 19:04 |
| dansmith | jokke: is there any user doc material that describes it like that? | 19:04 |
| jokke | I think it makes more sense maybe some day if we get these scopes little bit more in place. I could see reactivation being system or project admin etc. | 19:04 |
| lbragstad | mmm - exposing reactivate/deactivate to project-admins seems reasonable | 19:05 |
| jokke | lbragstad: it was really if anyone including operator did deactivate it, so the malicious user couldn't destroy the evidence and/or drop it back into use | 19:05 |
| jokke | dansmith: the original spec for example https://wiki.openstack.org/wiki/Glance-deactivate-image | 19:08 |
| jokke | dansmith: FAQ at the very end | 19:08 |
| lbragstad | so - from an operator perspective, of which i'm not ;) | 19:09 |
| dansmith | this describes the opposite doesn't it? | 19:09 |
| dansmith | where a user can reactivate but an admin may only be able to deactivate? | 19:09 |
| lbragstad | if i notice someone is uploading malicious images within a paritcular project, i'm likely going to disable their entire project | 19:09 |
| dansmith | or have I been flipping this conversation? I thought you were saying a user could deactivate but then needs a ticket to reactivate | 19:10 |
| jokke | I think this was the actual spec the implementation was based off https://specs.openstack.org/openstack/glance-specs/specs/kilo/deactivate-image.html | 19:10 |
| lbragstad | "If an image is deemed suspicious, an admin should be able to put the image “on hold” preventing instances from being built with it until it has be properly examined and determined to be safe or found dangerous and deleted. All image properties will remain accessible and editable, but the image will not be downloadable by non-admin users." | 19:11 |
| lbragstad | i think we have that part covered - yeah? | 19:11 |
| jokke | no you're not. That original spec assumed the deactivate/reactivate was treated as single operation and described the problem exposing that flip state to the user | 19:12 |
| jokke | why it was later on split like proposed on that same statement to be behind two different policies so they can be controlled individually | 19:13 |
| jokke | lbragstad: I think it's one of those things where it's hardcoded to be admin operation only rather than utilixing policies to lock the data down when the image is deactivated | 19:14 |
| lbragstad | ok - so we have a few options | 19:15 |
| lbragstad | 1.) make deactivate/reactivate admin-only | 19:15 |
| jokke | it was just the reactivation policy we should have admin by default as I think it has been so far | 19:15 |
| lbragstad | 2.) expose deactivate to project-members and keep reactivate admin-only | 19:15 |
| lbragstad | 3.) expose deactivate and reactivate to project-members | 19:16 |
| dansmith | jokke: was that "no you're not" aimed at my wondering about having this backwards? | 19:16 |
| dansmith | sounds like the current state is opposite from what that original spec proposed, | 19:16 |
| jokke | dansmith: yeah | 19:17 |
| dansmith | although not sure if that changed along the way, or just in the implementation | 19:17 |
| dansmith | okay | 19:17 |
| jokke | dansmith: the original spec did present it backwards because it assumed it being single toggle opertion | 19:17 |
| jokke | instead of two different operations | 19:17 |
| lbragstad | i guess the case i'd like to avoid is #2 | 19:18 |
| dansmith | but the original spec described admin having perms to deactivate, and user having activate... so still split now? | 19:18 |
| dansmith | *no | 19:18 |
| jokke | it is still split and currently the default behaviour should be the option 2 lbragstad described | 19:18 |
| lbragstad | that should be the default (#2)? | 19:19 |
| dansmith | okay, like lbragstad that seems not very useful to me, but it sounds like that makes it "like it is today" - for better or worse | 19:19 |
| jokke | so as part of this work, I'd say lets keep it like that and if there is wish to change it, that should be it's own piece of work | 19:19 |
| lbragstad | as in that's how it *should* work today prior to any of the secure rbac changes? | 19:19 |
| jokke | correct ;) | 19:20 |
| dansmith | I'd like to know who is actually using this, and how... | 19:20 |
| jokke | dansmith: I've stumbled upon pretty creative use cases for the feature | 19:20 |
| lbragstad | umm | 19:20 |
| lbragstad | https://review.opendev.org/c/openstack/glance-tempest-plugin/+/775742/11/glance_tempest_plugin/tests/rbac/v2/test_images.py@1431 | 19:20 |
| lbragstad | that ^ test passes with legacy rbac... as in no new project personas | 19:20 |
| dansmith | lbragstad: meaning a regular user can reactivate yeah? | 19:21 |
| lbragstad | https://1e03df7e49d16b52a381-065293745cdcc6ecb6052ade0199e5ba.ssl.cf2.rackcdn.com/775742/11/check/glance-legacy-rbac-protection-functional/fe5e8a6/testr_results.html | 19:21 |
| lbragstad | yes | 19:22 |
| dansmith | lbragstad: see why I wanted the tests to land first? :) | 19:22 |
| dansmith | lbragstad: we get a 204 there, I wonder if it's just not actually reactivated even though it told us it was? | 19:22 |
| jokke | hmm-m that's likely due to something that has slipped when we stopped shipping default policy file, as that was one of the rare default overwrites we did ship from the permissive policy | 19:23 |
| jokke | which would be super easy to check if we still had our policy files | 19:25 |
| dansmith | jokke: https://github.com/openstack/glance/blob/stable/stein/etc/policy.json#L36 | 19:26 |
| dansmith | looks unrestricted to me | 19:26 |
| dansmith | all the way back to ocata | 19:26 |
| dansmith | (unless I'm looking at the wrong thing) | 19:28 |
| jokke | yup, looks like I did remember that wrong | 19:28 |
| jokke | now I'm wondering how we are shipping that from OoO etc. | 19:28 |
| dansmith | so, maybe the default was the intent, | 19:28 |
| dansmith | but the shipped policy didn't match | 19:28 |
| dansmith | well, no the default was default for both right? | 19:29 |
| dansmith | heh, default was "rule:default" I mean | 19:29 |
| dansmith | which is "anyone that can see the image" ? | 19:29 |
| jokke | yeah our default policy has been pretty permissive about everything for testing | 19:30 |
| jokke | like many things that we document should be admin only or restricted, are in repo with empty string so we can test it | 19:30 |
| dansmith | the tests override the policy to be unrestricted, AFAIK, so I don't think this file affects them that I know of | 19:31 |
| jokke | nowadays yes | 19:31 |
| jokke | iirc even tempest used to be like default policies and tempest tests only user functions, if it needed admin credentials it was out of scope ofr tempest | 19:32 |
| jokke | but indeed looks like this is yet another of those things that we've been shipping opposite what we expect in the docs :( | 19:33 |
| dansmith | well, that's the point of the defaults-in-code work, so ... good? :) | 19:33 |
| jokke | well no, now when it's hidden all over the place in code it's even mre difficult to chec vs. when it was one clean file ;) | 19:34 |
| dansmith | well, disagree on that, but the point is to not be shipping two copies of things that can (and clearly do) get out of sync | 19:35 |
| lbragstad | well - just because it was in the file doesn't mean what was in the file was true with what is in the rest of glance that does authorization stuff | 19:35 |
| dansmith | well, that's a whole other thing | 19:36 |
| jokke | lbragstad: well the policies being in the code does not change the out of the policies auth either ;D | 19:36 |
| lbragstad | i think i understand that bit, but i don't think having a policy file necessarily makes this better | 19:37 |
| lbragstad | theoretically - we would have hit these issues 4 or 8 cycles ago if we wrote the same tests then, no? | 19:38 |
| jokke | but at least the new definitions under /glance/policies are much more undestandable than the old policy.json or the intermediate one we're moving away now | 19:38 |
| jokke | I really liked the old file based policies. It was so clear and all package managers knew how to deal with/warn if there was changes coming from upstream on upgrade. | 19:41 |
| lbragstad | we have deprecation signaling for that now | 19:41 |
| lbragstad | the file-based approach didn't really give us a way to handle things gracefully for operators on upgrade | 19:42 |
| dansmith | yeah | 19:42 |
| dansmith | but regardless, here we are, not much point in discussing that here and now | 19:43 |
| jokke | well at least it won't change anytihng | 19:43 |
| dansmith | sounds like to be the most compatible, we should keep both operations open to everyone and maybe have a reno to alert operators if they for some reason find that surprising? | 19:44 |
| lbragstad | so approach #3? | 19:44 |
| dansmith | not really sure it should be surprising to anyone, I guess, given all the defaults were open/open AFAICT | 19:44 |
| dansmith | yeah | 19:44 |
| lbragstad | ok - so i don't think i need to update that specific policy in question then | 19:45 |
| jokke | dansmith: not _all_ :P | 19:45 |
| jokke | "publicize_image": "role:admin", | 19:45 |
| jokke | "manage_image_cache": "role:admin", | 19:45 |
| jokke | "tasks_api_access": "role:admin", | 19:45 |
| dansmith | where is that? | 19:46 |
| jokke | that's from stable/train policy.json we shipped | 19:46 |
| dansmith | changed in stable? but aren't we talking about (re)activate ? | 19:47 |
| jokke | all the defaults | 19:47 |
| dansmith | https://github.com/openstack/glance/blob/stable/train/etc/policy.json#L36 | 19:48 |
| dansmith | are you talking about downstream? | 19:48 |
| jokke | dansmith: all the defaults not that specific one ... | 19:48 |
| jokke | I was pointing out that I managed to find 3 from the whole bloody thing that were not wide open :P | 19:49 |
| dansmith | oh,, I see.. I meant s/all/both/ above, in "all the places" | 19:49 |
| jokke | 'though I'm not sure if that file has all possible policies mapped ... the default was admin, so anyhing not listed should be pointing to admin | 19:50 |
| *** whoami-rajat has quit IRC | 19:50 | |
| jokke | but yeah, I don't think this is the right place to lock the specific one down lbragstad go with #3, sorry for confusion and derailing the convo for past 2 hours or so | 19:51 |
| jokke | my comment was based on how it was supposed to be, not how it has been shipping | 19:52 |
| lbragstad | ok - but we're good with exposing this at the project-level is what i'm hearing | 19:52 |
| jokke | well t least we know it's policy controlled. Not that it gets blocked somewhere else due to being hardcoded | 19:54 |
| *** lifeless_ is now known as lifeless | 19:56 | |
| lbragstad | well - kind of | 19:56 |
| lbragstad | https://opendev.org/openstack/glance/src/branch/master/glance/api/authorization.py#L369-L371 | 19:57 |
| lbragstad | ^ i have no idea how or where that gets invoked | 19:57 |
| lbragstad | it doesn't appear to be in the code paths i'm testing | 19:57 |
| lbragstad | but that's the code equivalent of "!" deny all | 19:58 |
| jokke | uuh, that's immutable proxy | 19:58 |
| lbragstad | i'm not sure if you have to do something special to hit that? | 19:58 |
| jokke | yeah, that's literally read-only image object | 19:58 |
| lbragstad | ah | 19:58 |
| jokke | I think that might be the mecanism which prevets lots of things happening if you're not the owner | 19:59 |
| lbragstad | so - that could be something like "False:%(protected)s" if we were to convert that | 19:59 |
| dansmith | it's very effective, and also very frustrating from a testing perspective :) | 20:00 |
| dansmith | it's a very brute-force way to make sure you don't accidentally mutate anything in the image, like metadata or the many linked structures you get back with an image | 20:01 |
| dansmith | there's magic that happens when you save an image, so it seems to aim to generate an exception when you even touch your "image" object before you get a chance to save() it and maybe have some unintended things get changed | 20:02 |
| jokke | dansmith: yeah | 20:02 |
| jokke | like you said it's crude but effective | 20:02 |
| lbragstad | ok - sounds like the publicize image policy check and how that's invoked before add_image | 20:03 |
| dansmith | yeah, I can see places where it would allow a leak, thinking that it was airtight, and also where it would prevent us from allowing certain operations we want to grant, without granting control over the image itself | 20:03 |
| jokke | it's also very frustrating combo with the domain model and all possible proxies | 20:04 |
| jokke | where you really never know which object you actually got back. it might look right but only way to be sure is to check the type | 20:05 |
| jokke | specially when there is stuff like properties which might be some proxy object depending where you got the image object from, and which layer happened to swap it between proxy object that behaves bit like a list or dict vs actual list or dict | 20:06 |
| jokke | which is what I assume was happening on that locations list you had to remove. It should be just list of dicts, but some layers gives you proxy | 20:07 |
| jokke | but it's past 8PM. Dinner, movie or something and sleep. | 20:12 |
| *** happyhemant has quit IRC | 20:13 | |
| lbragstad | ok - i'm following up on the whole 204/actual deactivation thing | 20:20 |
| lbragstad | the status when a project-member deactivates an image is 'deactivate' | 20:21 |
| dansmith | lbragstad: don't leave us hanging bro | 20:46 |
| lbragstad | that's it - that's the news | 20:46 |
| *** rcernin has joined #openstack-glance | 20:46 | |
| dansmith | I thought you were going to try reactivate as a user and see if it actually recovers | 20:46 |
| lbragstad | oh - yeah, i think we have a test for that | 20:48 |
| *** k_mouza has quit IRC | 20:49 | |
| *** openstackgerrit has joined #openstack-glance | 20:49 | |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: Add tests for image membership, deactivation, and reactivation https://review.opendev.org/c/openstack/glance-tempest-plugin/+/775742 | 20:49 |
| lbragstad | ok - i loaded ^ with a bunch of comments | 20:50 |
| lbragstad | to hopefully make the tests a little more clear | 20:50 |
| *** gyee has joined #openstack-glance | 21:01 | |
| *** rcernin has quit IRC | 21:17 | |
| dansmith | lbragstad: definitely helpful, and seems like your comment density went up as you got further in the file :) | 21:17 |
| dansmith | still have some questions, which I just left in there | 21:17 |
| dansmith | they're probably more like "glance will let you do whaa?" than actually policy-related, but.. since you're asserting that things are this way, you get to shoulder that burden I guess :) | 21:18 |
| dansmith | actually, nevermind, I should have just looked it up myself first | 21:27 |
| *** irclogbot_0 has quit IRC | 21:33 | |
| *** ianw has quit IRC | 21:33 | |
| *** lyr has quit IRC | 21:33 | |
| *** freerunner has quit IRC | 21:33 | |
| *** irclogbot_0 has joined #openstack-glance | 21:34 | |
| *** ianw has joined #openstack-glance | 21:34 | |
| *** lyr has joined #openstack-glance | 21:34 | |
| *** freerunner has joined #openstack-glance | 21:34 | |
| *** zzzeek has quit IRC | 21:44 | |
| *** zzzeek has joined #openstack-glance | 21:45 | |
| *** rcernin has joined #openstack-glance | 22:05 | |
| *** rcernin has quit IRC | 22:11 | |
| *** rcernin has joined #openstack-glance | 22:11 | |
| *** ajitha has quit IRC | 22:23 | |
| openstackgerrit | Dan Smith proposed openstack/glance master: Add housekeeping module and staging cleaner https://review.opendev.org/c/openstack/glance/+/777012 | 22:43 |
| openstackgerrit | Dan Smith proposed openstack/glance master: Add a check on startup for staging directory https://review.opendev.org/c/openstack/glance/+/778493 | 22:43 |
| *** k_mouza has joined #openstack-glance | 22:49 | |
| *** k_mouza has quit IRC | 22:54 | |
| *** rcernin has quit IRC | 22:56 | |
| *** rcernin has joined #openstack-glance | 23:01 | |
| *** k_mouza has joined #openstack-glance | 23:17 | |
| openstackgerrit | Dan Smith proposed openstack/glance master: Enable second glance worker for import testing https://review.opendev.org/c/openstack/glance/+/770629 | 23:18 |
| *** k_mouza has quit IRC | 23:22 | |
| *** rcernin has quit IRC | 23:26 | |
| *** k_mouza has joined #openstack-glance | 23:27 | |
| *** k_mouza has quit IRC | 23:31 | |
| *** k_mouza has joined #openstack-glance | 23:36 | |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: Add tests for image membership, deactivation, and reactivation https://review.opendev.org/c/openstack/glance-tempest-plugin/+/775742 | 23:39 |
| lbragstad | dansmith ok - i pulled some of your subtext comments into ps13 | 23:40 |
| lbragstad | but i wasn't sure if you meant for some of those to be added or not? | 23:40 |
| *** zzzeek has quit IRC | 23:40 | |
| lbragstad | if i misinterpreted that - let me know and i can re-add them | 23:40 |
| lbragstad | er - add them* | 23:40 |
| *** k_mouza has quit IRC | 23:41 | |
| *** zzzeek has joined #openstack-glance | 23:42 | |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Add glance functional protection tests to check and gate https://review.opendev.org/c/openstack/glance/+/778079 | 23:42 |
| dansmith | lbragstad: yeah, happy to have them in there | 23:46 |
| dansmith | lbragstad: ninja'd the test job patch | 23:47 |
| lbragstad | sweet | 23:47 |
| openstackgerrit | Merged openstack/glance master: Distributed image import https://review.opendev.org/c/openstack/glance/+/769976 | 23:59 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!