| *** tosky has quit IRC | 00:11 | |
| openstackgerrit | Merged openstack/glance master: Add administrator docs for distributed-import https://review.opendev.org/c/openstack/glance/+/778072 | 00:46 |
|---|---|---|
| *** zzzeek has quit IRC | 01:38 | |
| *** zzzeek has joined #openstack-glance | 01:39 | |
| *** rcernin has joined #openstack-glance | 01:53 | |
| openstackgerrit | Xuan Yandong proposed openstack/glance master: remove unicode in code https://review.opendev.org/c/openstack/glance/+/768984 | 03:05 |
| openstackgerrit | Xuan Yandong proposed openstack/glance master: Remove __unicode__() from Exception https://review.opendev.org/c/openstack/glance/+/769704 | 03:05 |
| *** rcernin has quit IRC | 03:06 | |
| openstackgerrit | YuehuiLei proposed openstack/glance-specs master: Add xena directory for specs https://review.opendev.org/c/openstack/glance-specs/+/778608 | 03:11 |
| *** rcernin has joined #openstack-glance | 03:24 | |
| *** rcernin has quit IRC | 03:26 | |
| *** rcernin has joined #openstack-glance | 03:27 | |
| *** lpetrut has joined #openstack-glance | 03:27 | |
| *** whoami-rajat has joined #openstack-glance | 04:20 | |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Implement project personas for image actions https://review.opendev.org/c/openstack/glance/+/764754 | 04:22 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Update default policies for task API https://review.opendev.org/c/openstack/glance/+/763208 | 04:22 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Make secure RBAC protection job voting https://review.opendev.org/c/openstack/glance/+/778258 | 04:22 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: DNM: Make sure protection tests protect against policy regressions https://review.opendev.org/c/openstack/glance/+/778609 | 04:22 |
| *** Underknowledge has quit IRC | 04:40 | |
| *** Underknowledge has joined #openstack-glance | 04:40 | |
| lbragstad | abhishekk jokke dansmith ^ those should encapsulate everything we talked about today | 04:41 |
| abhishekk | lbragstad, ack, will go through the irc communication and patches | 04:42 |
| lbragstad | sounds good - i'll catch up with you in about 7 hours | 04:42 |
| lbragstad | the first patch in the chain is failing randomly, so you may need to do some rechecking | 04:43 |
| lbragstad | everything up through 778258 passed tempest, functional, py38, and pep8 locally | 04:44 |
| abhishekk | ack | 04:44 |
| abhishekk | will keep eye on them | 04:45 |
| abhishekk | good night o/~ | 04:45 |
| lbragstad | awesome - thanks! | 04:45 |
| lbragstad | o/ | 04:45 |
| *** lpetrut has quit IRC | 04:47 | |
| *** ratailor has joined #openstack-glance | 04:49 | |
| *** udesale has joined #openstack-glance | 05:13 | |
| *** gyee has quit IRC | 05:20 | |
| *** yoctozepto has quit IRC | 06:00 | |
| openstackgerrit | Xuan Yandong proposed openstack/glance master: Remove __unicode__() from Exception https://review.opendev.org/c/openstack/glance/+/769704 | 06:01 |
| *** yoctozepto has joined #openstack-glance | 06:02 | |
| *** zzzeek has quit IRC | 06:03 | |
| *** zzzeek has joined #openstack-glance | 06:16 | |
| *** zzzeek has quit IRC | 06:25 | |
| *** zzzeek has joined #openstack-glance | 06:43 | |
| *** zzzeek has quit IRC | 06:44 | |
| *** zzzeek has joined #openstack-glance | 06:45 | |
| *** rcernin has quit IRC | 07:00 | |
| *** rcernin has joined #openstack-glance | 07:14 | |
| *** ajitha has joined #openstack-glance | 07:15 | |
| *** lpetrut has joined #openstack-glance | 07:22 | |
| *** takamatsu has joined #openstack-glance | 07:24 | |
| *** rcernin has quit IRC | 07:30 | |
| *** ralonsoh has joined #openstack-glance | 07:37 | |
| *** rcernin has joined #openstack-glance | 07:55 | |
| *** lpetrut has quit IRC | 07:56 | |
| *** rcernin has quit IRC | 08:00 | |
| *** hoonetorg has quit IRC | 08:03 | |
| *** rcernin has joined #openstack-glance | 08:12 | |
| *** rcernin has quit IRC | 08:17 | |
| *** rajinir_ has joined #openstack-glance | 08:29 | |
| *** tosky has joined #openstack-glance | 08:36 | |
| *** rajinir has quit IRC | 08:43 | |
| *** rajinir_ is now known as rajinir | 08:43 | |
| *** lpetrut has joined #openstack-glance | 08:51 | |
| *** hoonetorg has joined #openstack-glance | 08:55 | |
| *** rcernin has joined #openstack-glance | 09:13 | |
| *** rcernin has quit IRC | 09:17 | |
| *** zzzeek has quit IRC | 10:35 | |
| *** zzzeek has joined #openstack-glance | 10:35 | |
| *** k_mouza has joined #openstack-glance | 10:59 | |
| *** rcernin has joined #openstack-glance | 11:08 | |
| *** rcernin has quit IRC | 11:13 | |
| *** udesale_ has joined #openstack-glance | 11:21 | |
| *** udesale has quit IRC | 11:25 | |
| *** legochen has quit IRC | 11:53 | |
| *** tkajinam has quit IRC | 11:56 | |
| *** rcernin has joined #openstack-glance | 12:24 | |
| *** rcernin has quit IRC | 12:29 | |
| *** ratailor has quit IRC | 12:38 | |
| *** rcernin has joined #openstack-glance | 12:48 | |
| *** rcernin has quit IRC | 12:53 | |
| 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 | 13:43 |
| abhishekk | jokke, rosmaita, smcginnis, dansmith, lbragstad glance weekly meeting in 5 minutes at #openstack-meeting | 13:54 |
| abhishekk | see you there | 13:54 |
| * lbragstad nods | 13:55 | |
| *** legochen has joined #openstack-glance | 14:16 | |
| *** legochen has quit IRC | 14:21 | |
| dansmith | abhishekk: do you know about gerrit dashboards? might just be a little less work than hand-editing the etherpad (although you seem to be super fast and accurate at doing so)? | 14:45 |
| dansmith | here's a start at your wallaby one: https://bit.ly/3qmRMSY | 14:45 |
| abhishekk | looking | 14:46 |
| abhishekk | dansmith, interesting | 14:48 |
| *** rcernin has joined #openstack-glance | 14:49 | |
| abhishekk | How can we add other remaining patches, just by adding it to query parameters? | 14:49 |
| dansmith | anyway, I can flesh that out to the rest of the stuff if you want, but if you'd rather just track it in the etherpad, that's cool. just wanted to point it out in case you didn't know | 14:49 |
| dansmith | abhishekk: yeah, I can do it | 14:50 |
| abhishekk | dansmith, I will use both | 14:50 |
| abhishekk | etherpad to track todo things and dashboard for open reviews | 14:50 |
| dansmith | ack | 14:51 |
| abhishekk | thank you | 14:51 |
| *** rcernin has quit IRC | 14:54 | |
| dansmith | abhishekk: maybe you can +W this so I don't have to add it to the list :) https://review.opendev.org/c/openstack/glance/+/775299 | 14:55 |
| abhishekk | yeah | 14:56 |
| dansmith | abhishekk: I put the url at the top of the etherpad.. I *think* that's everything | 14:58 |
| abhishekk | cool | 14:58 |
| abhishekk | awesome, thank you | 15:00 |
| dansmith | np | 15:01 |
| *** jmlowe has quit IRC | 15:06 | |
| *** jmlowe has joined #openstack-glance | 15:08 | |
| *** belmoreira has joined #openstack-glance | 15:11 | |
| *** lpetrut has quit IRC | 15:36 | |
| openstackgerrit | Dan Smith proposed openstack/glance master: DNM: Check nova-ceph-glance with new policy defs https://review.opendev.org/c/openstack/glance/+/778736 | 15:49 |
| dansmith | lbragstad: abhishekk: I think ^ this will get nova to run with glance and ceph using the new defaults so we can see if anything breaks | 15:49 |
| lbragstad | awesome | 15:50 |
| lbragstad | are we expecting that to pass because the ansible playbook overrides the copy_image policy to be open? | 15:51 |
| lbragstad | because i think that's how i understood things | 15:51 |
| dansmith | I'm expecting it to fail because (unless it changed) you're defaulting set_image_location to false | 15:51 |
| dansmith | but yes, the copy_image override should still work, as expected, and override the default you're choosing | 15:51 |
| dansmith | but there are lots of nova interactions other than that which are important to validate | 15:52 |
| *** gyee has joined #openstack-glance | 15:52 | |
| lbragstad | oh - got it, that's an admin-only policy now | 15:52 |
| dansmith | copy_image is, yeah | 15:52 |
| lbragstad | and set_image_location | 15:53 |
| dansmith | set_image_location is not right? | 15:53 |
| dansmith | that's the whole point | 15:53 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/764754/24/glance/policies/image.py@188 | 15:53 |
| dansmith | going back to before defaults in code, it was "", and I think now it's rule:default right? | 15:53 |
| dansmith | lbragstad: right, you're *changing* it to admin | 15:54 |
| dansmith | but it's _currently_ not-admin | 15:54 |
| lbragstad | yes - correct | 15:54 |
| dansmith | so I expect nova to break on top of these patches, that's what I want to validate | 15:54 |
| dansmith | because I think your patches need to _not_ flip it to admin | 15:54 |
| dansmith | if we do that, we should do it separately with appropriate signaling | 15:54 |
| lbragstad | and that's because nova uses the user token to set the image location - depending on what nova has access to and what's copied into glance? | 15:55 |
| dansmith | well, yes to the first part | 15:56 |
| dansmith | when nova does a snapshot, it creates a thing in rbd and then just "tells" glance about it via set_image_location | 15:56 |
| lbragstad | ok - cool, git it | 15:56 |
| lbragstad | got it* | 15:56 |
| lbragstad | i think this is the 3rd or 4th time you've explained this to me | 15:57 |
| dansmith | it has some fallback routines for that code, so I'm also a little worried it will "work" but will have copied a ton of data around to make it seamless, even though we don't want it to | 15:57 |
| dansmith | so I will have to do some deep inspection of the logs | 15:57 |
| dansmith | lbragstad: it's okay, it's complicated, and I'm sure you've explained policy things to me at least that many times :) | 15:57 |
| dansmith | just noticed I said "setting to false" above when I meant "admin" | 16:03 |
| * dansmith is in too many parallel things | 16:03 | |
| abhishekk | dansmith, ack | 16:08 |
| abhishekk | dansmith, do we need to set glance config parameter to True as well? | 16:09 |
| dansmith | abhishekk: this isn't enforce_scope, just enforce_new_defaults, so I think we're good no? lbragstad ? | 16:10 |
| dansmith | isn't enforce_scope the one we have the extra flag for? | 16:10 |
| abhishekk | enforce_secure_rbac this one? | 16:10 |
| abhishekk | https://review.opendev.org/c/openstack/glance/+/776588/8/glance/cmd/api.py#111 | 16:11 |
| dansmith | ah dang, okay | 16:12 |
| lbragstad | nice catch abhishekk | 16:12 |
| abhishekk | :) | 16:12 |
| openstackgerrit | Dan Smith proposed openstack/glance master: DNM: Check nova-ceph-glance with new policy defs https://review.opendev.org/c/openstack/glance/+/778736 | 16:13 |
| abhishekk | perfect | 16:13 |
| *** lpetrut has joined #openstack-glance | 16:36 | |
| *** lpetrut_ has joined #openstack-glance | 16:38 | |
| *** lpetrut_ has quit IRC | 16:38 | |
| *** lpetrut has quit IRC | 16:42 | |
| openstackgerrit | Dan Smith proposed openstack/glance master: Enable second glance worker for import testing https://review.opendev.org/c/openstack/glance/+/770629 | 16:48 |
| *** rcernin has joined #openstack-glance | 16:50 | |
| *** rcernin has quit IRC | 16:55 | |
| *** belmoreira has quit IRC | 17:09 | |
| 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 | 17:18 |
| lbragstad | i'm having ^ depend on a tempest fix to get the legacy and secure rbac jobs working again | 17:19 |
| lbragstad | i also pulled the copy_image tests out of that patch | 17:19 |
| lbragstad | i'm going to add those back in a separate commit, like we did with the system-scope tests | 17:19 |
| abhishekk | ack | 17:21 |
| abhishekk | one more dependency :D | 17:22 |
| openstackgerrit | Lance Bragstad proposed openstack/glance-tempest-plugin master: Add base tests case for copy_image test https://review.opendev.org/c/openstack/glance-tempest-plugin/+/778756 | 17:23 |
| *** udesale_ has quit IRC | 17:47 | |
| * abhishekk signing out for the day, will be keeping eye on nova-ceph-rbac result if pushed again | 17:58 | |
| dansmith | lbragstad: I'm not sure what rebasing you mean, | 18:07 |
| dansmith | and I'm also not sure what got pushed that kicked that out of check | 18:07 |
| lbragstad | didn't you nova patch fail because https://review.opendev.org/c/openstack/glance/+/778609/1 failed ahead of it? | 18:08 |
| lbragstad | didn't your* | 18:09 |
| *** ralonsoh has quit IRC | 18:09 | |
| dansmith | it didn't even start running | 18:10 |
| dansmith | I guess I'm not sure what the point of that patch is, I was just trying to be above all your proposed changes | 18:10 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/778609/1 is just showing that the protection tests catch policy regressions | 18:11 |
| lbragstad | i can abandon it now that it's kind of proven it's point | 18:11 |
| dansmith | okay I thought maybe that was going to be come a revert-to-current-default patch | 18:14 |
| dansmith | I would think that my test would still run on top of it, but I will rebase | 18:14 |
| lbragstad | abandoned | 18:15 |
| openstackgerrit | Dan Smith proposed openstack/glance master: Add glance functional protection tests to check and gate https://review.opendev.org/c/openstack/glance/+/778079 | 18:15 |
| openstackgerrit | Dan Smith proposed openstack/glance master: Implement project personas for image actions https://review.opendev.org/c/openstack/glance/+/764754 | 18:15 |
| openstackgerrit | Dan Smith proposed openstack/glance master: Update default policies for task API https://review.opendev.org/c/openstack/glance/+/763208 | 18:15 |
| openstackgerrit | Dan Smith proposed openstack/glance master: DNM: Check nova-ceph-glance with new policy defs https://review.opendev.org/c/openstack/glance/+/778736 | 18:15 |
| dansmith | ah crap | 18:15 |
| dansmith | did I revert something of yours? I didn't rebase everything, I just moved it down the stack | 18:16 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/764754/24..25 looks fine | 18:16 |
| dansmith | I didn't see that you pushed anything else up on that, so I dunno why it re-pushed them all | 18:16 |
| dansmith | okay | 18:16 |
| lbragstad | ps24 contained all the important updates we talked about yesterday | 18:17 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/778258/4 didn't get rebased though? | 18:17 |
| dansmith | well, since I didn't intend to move the bottom ones, I was trying to be peer to that | 18:18 |
| lbragstad | ah - got it | 18:18 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Make secure RBAC protection job voting https://review.opendev.org/c/openstack/glance/+/778258 | 18:18 |
| * lbragstad backs away slowly | 18:19 | |
| dansmith | heh | 18:21 |
| *** rcernin has joined #openstack-glance | 18:50 | |
| *** rcernin has quit IRC | 18:56 | |
| *** mgagne has quit IRC | 18:56 | |
| *** mgagne has joined #openstack-glance | 18:57 | |
| *** whoami-rajat has quit IRC | 18:59 | |
| *** rcernin has joined #openstack-glance | 19:12 | |
| *** rcernin has quit IRC | 19:17 | |
| dansmith | lbragstad: I have good news and bad news | 19:41 |
| lbragstad | bad news first pls | 19:41 |
| dansmith | Mar 04 11:40:18 guaranine devstack@g-api.service[2967241]: DEBUG glance.api.v2.images [None req-c1ca94a6-4f24-4442-a6db-e3a0e478024c demo demo] User not permitted to update image 'a3a73bd3-3b89-4a58-8d6d-d8e209335a76' {{(pid=2967241) update /opt/stack/glance/glance/api/v2/images.py:570}} | 19:41 |
| dansmith | (actually it's all bad news) | 19:41 |
| lbragstad | oh good | 19:42 |
| lbragstad | :) | 19:42 |
| dansmith | that's what happens when nova tries to snapshot, glance refuses with the new defaults | 19:42 |
| dansmith | without enforce_new_defaults=True, the same thing works | 19:42 |
| dansmith | now for the worse (but not really for you) news, | 19:42 |
| lbragstad | ok - so it works because we're applying a logic OR in oslo.policy without enforce_new_defaults = True | 19:42 |
| dansmith | everything is pretty silent about it.. so nova shows "uploading snapshot ...." then goes back to ACTIVE, but.. no image ever shows up | 19:43 |
| dansmith | so kinda silent fail | 19:43 |
| dansmith | test should fail because it'll never find an image (assuming, hopefully, that it checks for an image) | 19:43 |
| openstackgerrit | Merged openstack/glance master: Cleanup remaining tenant terminology in glance API docs https://review.opendev.org/c/openstack/glance/+/775299 | 19:43 |
| dansmith | lbragstad: right, expected that it would work with enforce_new_defaults=False | 19:43 |
| lbragstad | hmm | 19:44 |
| lbragstad | so - iiuc glance is supposedly returning a 403 to nova somewhere, but that's not really bubbling up to the end user (should it?) | 19:45 |
| dansmith | right, so that's a UX issue, but not sure it's really solvable.. have to look and see what nova has in the way of ways to signal a failure to upload a snapshot | 19:45 |
| dansmith | however, | 19:45 |
| dansmith | the salient point for the rbac work is, I think we need to keep a default of "project_member" for set/get_image_location for the moment | 19:45 |
| lbragstad | yeah - it seems that way | 19:46 |
| dansmith | i.e. not have your patch change the default | 19:46 |
| * lbragstad pulls up the change | 19:46 | |
| lbragstad | get_image_location looks good, no? | 19:47 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/764754/25/glance/policies/image.py@172 | 19:47 |
| dansmith | ahh, nova does kinda report it: | createImage | req-c1ca94a6-4f24-4442-a6db-e3a0e478024c | Error | 2021-03-04T19:40:12.000000 | 2021-03-04T19:40:20.000000 | | 19:47 |
| dansmith | so I guess we're good on that | 19:48 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/764754/25/glance/policies/image.py@187 is the problem because the snapshotting process can't update the snapshot location? | 19:48 |
| dansmith | yeah, project_reader should be fine for that | 19:48 |
| dansmith | yes | 19:48 |
| dansmith | lemme comment on that real quick for posterity | 19:48 |
| lbragstad | ok - so my patch needs to do set_image_location: base.ADMIN_OR_PROJECT_MEMBER instead of set_image_location: role:admin | 19:49 |
| dansmith | yup | 19:51 |
| dansmith | jobs are running on that DNM patch, so I'll defer testing the copy_image thing and just wait for that to finish | 19:51 |
| dansmith | if it breaks, every nova boot will fail, and if not (expected) only the snapshot tests will fail | 19:52 |
| lbragstad | ok - cool | 19:53 |
| dansmith | so, hold off on any change to that patch a little while, so we can get that report which is being generated right now | 19:54 |
| lbragstad | ok - i have the changes prepped locally | 19:54 |
| dansmith | supposedly ~40m remaining, so shouldn't be too long | 19:55 |
| *** rcernin has joined #openstack-glance | 20:00 | |
| dansmith | okay I think the bottom one on each stack should be headed in after they finish check | 20:02 |
| openstackgerrit | Dan Smith proposed openstack/glance master: Enable second glance worker for import testing https://review.opendev.org/c/openstack/glance/+/770629 | 20:14 |
| dansmith | oh the carnage is real | 20:15 |
| lbragstad | the rbac series to ultimately waiting on a tempest fix | 20:19 |
| *** k_mouza has quit IRC | 20:19 | |
| dansmith | s/to/is/ ? | 20:19 |
| dansmith | there's a lot of fail in this nova ceph rbac job | 20:19 |
| dansmith | seems like more than just snapshots, but need to wait for the report to tell for sure | 20:19 |
| lbragstad | yeah - s/to/is/ my typing is garbage lately | 20:20 |
| lbragstad | i assume you're watching the stream? | 20:22 |
| dansmith | yup | 20:23 |
| dansmith | oh, I see, that tempest fix is blocking the bottom two going in | 20:27 |
| lbragstad | right | 20:28 |
| *** rcernin has quit IRC | 20:43 | |
| dansmith | pretty much teh failz: https://956d0fa1451be9107b7b-16c03009b76be00de1a48ecc1775c29e.ssl.cf5.rackcdn.com/778736/3/check/nova-ceph-multistore-rbac/5642b55/testr_results.html | 20:49 |
| lbragstad | oh boy | 20:50 |
| *** k_mouza has joined #openstack-glance | 20:50 | |
| dansmith | https://zuul.opendev.org/t/openstack/build/5642b55feb7f44df84a91b3a1b2e2a22/log/controller/logs/screen-n-cpu.txt#34805 | 20:51 |
| lbragstad | should i push that set_image_location change up now? i think we should be good and we're not concerned about bumping something out of the check queue, are wel? | 20:51 |
| dansmith | so yeah, copy is not working | 20:51 |
| dansmith | you can yeah | 20:51 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Implement project personas for image actions https://review.opendev.org/c/openstack/glance/+/764754 | 20:52 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Update default policies for task API https://review.opendev.org/c/openstack/glance/+/763208 | 20:52 |
| openstackgerrit | Lance Bragstad proposed openstack/glance master: Make secure RBAC protection job voting https://review.opendev.org/c/openstack/glance/+/778258 | 20:52 |
| dansmith | lbragstad: so... this is the policy file: https://956d0fa1451be9107b7b-16c03009b76be00de1a48ecc1775c29e.ssl.cf5.rackcdn.com/778736/3/check/nova-ceph-multistore-rbac/5642b55/controller/logs/etc/glance/policy.json | 20:52 |
| dansmith | should that not override the default like it did before? | 20:52 |
| lbragstad | yes - it should | 20:52 |
| dansmith | https://zuul.opendev.org/t/openstack/build/5642b55feb7f44df84a91b3a1b2e2a22/log/controller/logs/screen-g-api.txt#372 | 20:53 |
| dansmith | does enforce_new_defaults somehow break our reading of legacy json files instead of yaml? | 20:54 |
| lbragstad | i wouldn't expect it to? | 20:54 |
| dansmith | me either, but.. | 20:54 |
| dansmith | oh, you know | 20:55 |
| *** k_mouza has quit IRC | 20:55 | |
| dansmith | I think we don't have that laid down during devstack, only after before we run tempest | 20:55 |
| dansmith | loads it here: https://zuul.opendev.org/t/openstack/build/5642b55feb7f44df84a91b3a1b2e2a22/log/controller/logs/screen-g-api.txt#924 | 20:55 |
| lbragstad | glance gets bounced after that change though, right? | 20:56 |
| dansmith | yeah it's loading it, so ignore that concern | 20:57 |
| dansmith | I just forgot it happens late | 20:57 |
| dansmith | also, looks like maybe we're actually not failing on the copy permission | 20:57 |
| dansmith | https://zuul.opendev.org/t/openstack/build/5642b55feb7f44df84a91b3a1b2e2a22/log/controller/logs/screen-g-api.txt#1255 | 20:58 |
| dansmith | are there policy changes to the task creation stuff maybe? | 20:59 |
| dansmith | ah, maybe that is the copy_image failure actually | 21:00 |
| lbragstad | all of that stuff defaults to the admin api | 21:00 |
| dansmith | that's what we log when we get a forbdden during task creation | 21:00 |
| lbragstad | example copy_image debug | 21:00 |
| lbragstad | http://paste.openstack.org/raw/803239/ | 21:00 |
| lbragstad | unfortunately - oslo.policy doesn't log the check string | 21:01 |
| lbragstad | or what it's going to check all that stuff against | 21:01 |
| lbragstad | which would be useful in this situation | 21:01 |
| dansmith | nor the result right? | 21:01 |
| lbragstad | right | 21:01 |
| dansmith | so I think it's failing on add_task | 21:03 |
| dansmith | not copy_image | 21:03 |
| dansmith | "You are not authorized to complete add_task action." | 21:03 |
| lbragstad | ahhh - man | 21:04 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/763208/17/glance/policies/tasks.py | 21:04 |
| dansmith | I don't see you changing that | 21:04 |
| dansmith | oh, right next patch | 21:05 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/763208/14/glance/api/policy.py | 21:05 |
| dansmith | was default before | 21:05 |
| *** openstackgerrit has quit IRC | 21:05 | |
| lbragstad | yeah | 21:06 |
| lbragstad | and it sounds like it has to be default to make basic workflows work | 21:06 |
| lbragstad | wait... | 21:06 |
| lbragstad | so - task_api_access is admin-only | 21:07 |
| dansmith | well, without policy changes | 21:07 |
| dansmith | lbragstad: yeah, so tasks can be created or viewed via the tasks API directly, but some image actions (all of import) require creating tasks to get things done | 21:08 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/763208/14/glance/api/v2/tasks.py@222 | 21:08 |
| dansmith | so it doesn't make sense to have one allowed and not add_task that it requires | 21:08 |
| lbragstad | ok - so task_api_access is the policy that protects the actual API endpoint in glance | 21:08 |
| lbragstad | and add_task is the "internal" policy that's called within glance when various image actions happen | 21:09 |
| *** rcernin has joined #openstack-glance | 21:09 | |
| dansmith | seems like it | 21:10 |
| lbragstad | noted | 21:10 |
| dansmith | it's pretty silly to have that "internal" policy element, when really the only reason for it is to break something | 21:10 |
| dansmith | probably means it should never be anything other than "", and maybe removed in the future | 21:10 |
| dansmith | so, your changing of that add_task _should_ break image import (not just copy) for all non-admins | 21:11 |
| dansmith | do we not see jobs failing with that? | 21:11 |
| dansmith | looks like not, but I dunno why | 21:12 |
| dansmith | this makes me worry the tempest import tests are using admin or something | 21:13 |
| dansmith | everything is pretty silent about it.. so nova shows "uploading snapshot ...." then goes to | 21:14 |
| dansmith | oops | 21:14 |
| dansmith | credentials = ['primary'] | 21:14 |
| dansmith | ^ this means regular user creds in tempest right? | 21:14 |
| * lbragstad checks | 21:15 | |
| lbragstad | i think so | 21:16 |
| lbragstad | https://github.com/openstack/tempest/blob/master/tempest/lib/common/dynamic_creds.py#L413-L416 | 21:16 |
| lbragstad | https://github.com/openstack/tempest/blob/master/tempest/lib/common/dynamic_creds.py#L391-L393 | 21:16 |
| dansmith | we're definitely creating tasks in those tests normally on top of that patch, | 21:18 |
| dansmith | but no policy debug to see it actually checking each element | 21:18 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/763208/17..19/glance/policies/tasks.py#22 | 21:19 |
| dansmith | Mar 04 05:08:41.380143 ubuntu-focal-rax-dfw-0023291708 glance-api[103543]: DEBUG oslo_policy.policy [None req-6fdc7cd8-f40f-4cd6-9bcf-3456754b8f05 tempest-ImportImagesTest-1217660846 tempest-ImportImagesTest-1217660846-project] enforce: rule="add_task" creds={"domain_id": null, "is_admin_project": true, "project_domain_id": "default", "project_id": "9234e039a85444c8abc290a43a3091d2", "roles": ["member", "reader"], | 21:20 |
| dansmith | "service_project_domain_id": null, "service_project_id": null, "service_roles": [], "service_user_domain_id": null, "service_user_id": null, "system_scope": null, "user_domain_id": "default", "user_id": "aa6f0f21efb84313b13eaed44cdcdd6a"} target={} {{(pid=103543) enforce /usr/local/lib/python3.8/dist-packages/oslo_policy/policy.py:992}} | 21:20 |
| dansmith | this is where it runs the test in the ceph job which does have policy debug on | 21:20 |
| dansmith | does is_admin_project:True mean tempest is using admin creds for that test? | 21:20 |
| dansmith | must not be, because the one that fails in the DNM is the same: | 21:21 |
| dansmith | Mar 04 20:03:27.182566 ubuntu-focal-limestone-regionone-0023306141 glance-api[103126]: DEBUG oslo_policy.policy [None req-a52700a9-9335-4b69-ac0f-1f6a90483da8 tempest-DeleteServersAdminTestJSON-1328139312 tempest-DeleteServersAdminTestJSON-1328139312-project] enforce: rule="add_task" creds={"domain_id": null, "is_admin_project": true, "project_domain_id": "default", "project_id": "5aa94de242b747cf9e271ce40e42cb09", "roles": | 21:21 |
| dansmith | ["member", "reader"], "service_project_domain_id": null, "service_project_id": null, "service_roles": [], "service_user_domain_id": null, "service_user_id": null, "system_scope": null, "user_domain_id": "default", "user_id": "e38fbcdd53bf4bb1878fd2642829e3d9"} target={} {{(pid=103126) enforce /usr/local/lib/python3.8/dist-packages/oslo_policy/policy.py:992}} | 21:21 |
| dansmith | do I don't get it | 21:23 |
| dansmith | *so | 21:23 |
| dansmith | lbragstad: the place where we are failing add_task is on a context that we get with this helper: https://github.com/openstack/glance/blob/ed930ec5123a618fea3a6e6e64bf2d6bfccf7312/glance/context.py#L75-L85, but only in the case of that copy_image delegation | 21:37 |
| dansmith | does that look right to you, or missing something that might cause us to fail when the user's context would have allowed it? | 21:37 |
| dansmith | actually, I think that's not actually true | 21:39 |
| dansmith | seems like we have to somehow have more privilege when we run the tempest tests than the user token we're using through nova | 21:45 |
| dansmith | so it surely seems like changing those task crud policies is required, I just don't know why the tempest tests aren't failing on that | 21:45 |
| dansmith | which concerns me | 21:45 |
| lbragstad | sorry - i got pulled into something else | 21:46 |
| lbragstad | back now though | 21:46 |
| dansmith | anyway, if you haven't updated that by the time I get back from a break I can do it | 21:46 |
| dansmith | okay | 21:46 |
| dansmith | we're definitely getting past the copy_image check though | 21:46 |
| lbragstad | is_admin_project:True is weird | 21:47 |
| dansmith | agreed | 21:47 |
| dansmith | but it's the same across both the one that works and the one that doesn't | 21:47 |
| lbragstad | it's a policy check that evaluates if context.roles contains 'admin' | 21:47 |
| lbragstad | is_admin_project is the legacy way of denoting a system-administrator | 21:48 |
| lbragstad | so - opting into the new defaults might be changing that | 21:48 |
| lbragstad | because is_admin_project obviously doesn't care about system-scope (i'm heavily speculating) | 21:48 |
| *** rcernin has quit IRC | 21:49 | |
| dansmith | I see that on the job that isn't using new defaults though | 21:49 |
| lbragstad | ok - in the glance API, right? | 21:49 |
| dansmith | yes | 21:49 |
| dansmith | we have two nearly identical jobs you can compare, | 21:49 |
| dansmith | the nova-ceph-multistore run on your tasks patch, which has oslo_policy debug on, | 21:49 |
| dansmith | and then the -rbac version on my DNM patch on top | 21:50 |
| lbragstad | keep in mind that https://github.com/openstack/glance/blob/master/glance/context.py#L66 runs on every single request | 21:50 |
| dansmith | the latter has the new defaults, and fails lots of stuff, but former does not and does not | 21:50 |
| dansmith | lbragstad: I'll keep that in mind, but what is your point? :) | 21:51 |
| lbragstad | mm - i thought you were wondering why it was logged all the time, nevermind | 21:53 |
| dansmith | I really gotta look at not-a-computer for a while, I'll be more useful if I do that | 21:55 |
| dansmith | bbl | 21:56 |
| lbragstad | ok - i just fixed a random pep8 issue | 21:57 |
| lbragstad | in the tasks policies patch | 21:57 |
| lbragstad | and i updated the internal policies to be more like what they were before (not admin-only) | 21:57 |
| * lbragstad goes for a run | 21:58 | |
| *** ajitha has quit IRC | 22:24 | |
| *** rcernin has joined #openstack-glance | 22:29 | |
| *** takamatsu has quit IRC | 22:42 | |
| dansmith | weird, I guess I commented on an old PS, sorry about that | 22:46 |
| dansmith | okay so I should rebase that DNM patch | 22:48 |
| lbragstad | this probably isn't here or there, but | 22:51 |
| lbragstad | i was getting is_admin_project and context_is_admin confused | 22:51 |
| lbragstad | is_admin_project is in oslo.context.RequestContext and it's True by default | 22:52 |
| lbragstad | https://github.com/openstack/oslo.context/blob/master/oslo_context/context.py#L213 | 22:52 |
| lbragstad | context_is_admin is a context-level policy check that uses oslo.policy to evaluate if context.roles contains 'admin' | 22:52 |
| *** rcernin has quit IRC | 22:54 | |
| *** rcernin has joined #openstack-glance | 22:54 | |
| dansmith | that's not confusing at all | 22:54 |
| dansmith | not sure what is_admin_project being true by default is about either | 22:55 |
| * lbragstad prepares for story time | 22:55 | |
| lbragstad | so - the is_admin_project thing was the original approach to solving admin-ness back in like 2016 | 22:56 |
| lbragstad | your deployment would have an "admin" project that the operator would specify in config, and anyone with a role on that admin project would have is_admin_project:True in their context object | 22:56 |
| dansmith | wow | 22:57 |
| lbragstad | then all the policies were supposed to be rewritten to use something like "publicize_image": "role:admin and is_admin_project:True" | 22:57 |
| lbragstad | instead of something like "publicize_image": "role:admin and system_scope:all" | 22:57 |
| *** takamatsu has joined #openstack-glance | 22:57 | |
| *** tkajinam has joined #openstack-glance | 22:57 | |
| lbragstad | at the same time, we started mapping out the system-scope approach with a spec and presented both to operators at one of the forums | 22:59 |
| lbragstad | then ultimately pivoted to the system-scope approach | 22:59 |
| lbragstad | (with one of the advantages being it would be easier to implement NIST constrained RBAC using a system-model instead of a separate project tree hierarchy, where projects are overloaded for service level-access) | 23:00 |
| dansmith | yeah, I was going to say that the project approach seems like a hack, | 23:01 |
| dansmith | but I guess it's pretty much the same as unix with sudoers allowing @wheel, so... | 23:01 |
| lbragstad | but "easier" is kind of a misnomer, because both approaches require a lot of work (as we're finding out now), but conceptually, overloading projects against with another responsibility caused hesitation | 23:02 |
| lbragstad | s/against/again/ | 23:02 |
| dansmith | system scope seems like better integration | 23:03 |
| lbragstad | we also didn't know how to answer questions like "should your admin project be allowed to own resources?" "what happens if your admin project disappears?" | 23:03 |
| dansmith | yeah | 23:03 |
| lbragstad | "we should develop protections to prevent foot-gun incidents, right?" | 23:04 |
| dansmith | @wheel can (group-)own files, but.. not really a good example to follow | 23:04 |
| lbragstad | yeah | 23:05 |
| lbragstad | i guess putting resources in an admin project is kinda like that | 23:07 |
| dansmith | yeah | 23:09 |
| dansmith | okay, I'm stupid | 23:20 |
| dansmith | you're not seeing the add_task failure on any of the other jobs because you're not turning on new defaults on those tempest runs | 23:21 |
| dansmith | I was equating the nova-ceph-multistore run on your patch with the one of mine above that turns that thing on, but they're not equal, obviously | 23:21 |
| dansmith | which is why the regular import tests _do_ fail in tempest in that job, in addition to all the nova stuff | 23:22 |
| dansmith | see, I told you I needed a break :D | 23:22 |
| lbragstad | yeah - so your DNM patch failed because you were forcing the deployment into the new enforcement model | 23:22 |
| lbragstad | and that broke because i overly restricted the task policies (that are only used internally) | 23:23 |
| dansmith | right, | 23:23 |
| lbragstad | the new hypothesis is that it will work on this new run | 23:24 |
| dansmith | so the version I pushed up on top of the updated task policy patch should give us another smoke test of anything else that may still be broken | 23:24 |
| dansmith | right, or whittle down to some other thing we haven't figured out yet :) | 23:24 |
| lbragstad | yeah - ok, that makes sense | 23:24 |
| dansmith | when that is done, I should turn that flag on for more tests that cover some other bases too and we can just use that as our canary | 23:25 |
| lbragstad | ++ | 23:25 |
| dansmith | so glad to have that sorted, I was worried that maybe we haven't been testing import as a regular user this whole time or something | 23:26 |
| dansmith | there's still stuff failing in that job | 23:37 |
| dansmith | should be finishing up soo | 23:37 |
| dansmith | ssage': 'You are not authorized to complete download_image action.<br /><br />\n\n\n', 'code': '403 Forbidden', 'title': 'Forbidden'} | 23:37 |
| lbragstad | ugh | 23:45 |
| lbragstad | i kept that as project-member | 23:45 |
| lbragstad | i originally proposed it as project-reader | 23:45 |
| dansmith | so maybe nova can't boot anything because it can't read the image? | 23:45 |
| dansmith | I dunno what exactly is failing just saw that shoot by | 23:45 |
| dansmith | it takes a long time to fail every tempest test, which is why it's still going :P | 23:46 |
| lbragstad | i think the only person who can't download an image is project readers | 23:49 |
| lbragstad | https://review.opendev.org/c/openstack/glance/+/764754/8/glance/policies/image.py | 23:49 |
| lbragstad | at least in the new world | 23:49 |
| lbragstad | unless a test is relying on that policy being open (or if that's assumed somewhere?) | 23:49 |
| dansmith | I wouldn't think so, but yeah I dunno | 23:49 |
| dansmith | seems like a lot of fail, like similar to last time, so something else must be going on | 23:50 |
| lbragstad | alright - i have to step away for a bit | 23:53 |
| dansmith | ack | 23:54 |
| lbragstad | thanks for all the help with this dansmith - i feel like we've sussed out quite a bit | 23:55 |
| dansmith | yep, no prob! | 23:55 |
| *** openstackgerrit has joined #openstack-glance | 23:58 | |
| openstackgerrit | Dan Smith proposed openstack/glance master: DNM: Check some jobs with new policy defaults https://review.opendev.org/c/openstack/glance/+/778736 | 23:58 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!