| opendevreview | Merged openstack/glance master: Suppress policy deprecation and default change warnings https://review.opendev.org/c/openstack/glance/+/805049 | 02:28 |
|---|---|---|
| opendevreview | Merged openstack/glance_store master: Xena cycle Release Notes https://review.opendev.org/c/openstack/glance_store/+/804952 | 05:35 |
| opendevreview | Merged openstack/glance master: Refactor gateway auth layer for image factory https://review.opendev.org/c/openstack/glance/+/805065 | 06:51 |
| *** redrobot1 is now known as redrobot | 07:59 | |
| abhishekk | till now, I have setup 18.04 VM with python 3.6.9 (similar to upstream) and enabled Opportunistic tests for Mysql, Ensured those are not skipped now | 09:37 |
| abhishekk | Now will keep them running for multiple times to see whether I am able to reproduce it locally or not | 09:37 |
| abhishekk | dansmith, croelandt ^^ | 09:37 |
| abhishekk | No failure on 20+ consecutive runs | 11:30 |
| opendevreview | Merged openstack/glance master: Move setting of enforce_scope to devstack side https://review.opendev.org/c/openstack/glance/+/778952 | 13:12 |
| dansmith | abhishekk: okay, really interesting that the gate run saw just one of those tests run, which seems like it is still a good indicator.. can't believe it. | 13:24 |
| dansmith | I wonder if the first one improved things and the second improves more and that's why the one was way down in terms of fail rate | 13:25 |
| dansmith | we should keep rechecking the second for a while I think | 13:25 |
| abhishekk | hmm, also, this https://review.opendev.org/c/openstack/glance/+/804898 has successful 8 run without timeout | 13:25 |
| dansmith | well, you said 80% fail right? | 13:26 |
| abhishekk | but today it has 100% ratio of passing | 13:26 |
| dansmith | oh I see, a bunch of rechecks | 13:27 |
| dansmith | I wonder if it's load-related, because those would be heavyweight tests | 13:27 |
| abhishekk | could be | 13:28 |
| dansmith | your rechecks today were all before much of the world was awake | 13:28 |
| abhishekk | yes | 13:28 |
| abhishekk | I think we should start approving patches | 13:29 |
| dansmith | I wonder if it would be responsible to make it -nv just in gate | 13:31 |
| abhishekk | I am also thinking about that | 13:32 |
| dansmith | voting in check will make it a pain for us, but once something gets in the gate queue, it won't reset for everyone else | 13:32 |
| abhishekk | I think this is good idea | 13:32 |
| dansmith | I dunno what the feeling is on that, because generally we're trying not to land things that might pass check one day, fail because of a change before gate and then land to just immediately fail check, but this is maybe a little different | 13:33 |
| dansmith | I kinda feel like we should ask one of the qa people before we do that, but if we have some things ready to go, let's approve and see how it is until they're around | 13:34 |
| abhishekk | Yes, we witness these timeouts every cycle during last milestone only | 13:34 |
| dansmith | you approved my image factory patch right? did it land? | 13:34 |
| abhishekk | yes | 13:34 |
| abhishekk | it did | 13:34 |
| dansmith | okay, so it made it through | 13:34 |
| abhishekk | also then enforce scope patch of gmanns as well | 13:34 |
| dansmith | okay | 13:35 |
| abhishekk | upload and download patches were already approved | 13:35 |
| dansmith | let's kick them before we change anything and see how it goes | 13:35 |
| abhishekk | ack | 13:35 |
| dansmith | maybe this is *all* load related or something and the last week was bad | 13:35 |
| abhishekk | Hopefully | 13:36 |
| dansmith | I think download will need to be rebased on upload or it will conflict out | 13:38 |
| abhishekk | yes | 13:38 |
| abhishekk | delete from store does not have any dependency | 13:39 |
| abhishekk | or task api repo related change as well | 13:39 |
| dansmith | okay I haven't reviewed that one yet | 13:39 |
| abhishekk | https://review.opendev.org/c/openstack/glance/+/802243/9 | 13:40 |
| abhishekk | this one | 13:40 |
| dansmith | yeah, looking at that now, | 13:41 |
| dansmith | I meant hadn't reviewed the delete-from-store one | 13:41 |
| abhishekk | yep | 13:41 |
| dansmith | abhishekk: your todo in that one could be done now right? the image_factory(auth=auth) ? | 13:42 |
| abhishekk | :D | 13:42 |
| abhishekk | good point | 13:42 |
| abhishekk | please comment | 13:42 |
| dansmith | done | 13:43 |
| abhishekk | ack, will fix after the meeting now | 13:44 |
| dansmith | the patch above is ready too, I guess I should just +W both and you can fix the image_factory thing in the actual task patch, which is where it gets used anyway | 13:45 |
| dansmith | okay? | 13:45 |
| abhishekk | ack | 13:46 |
| abhishekk | Good | 13:46 |
| abhishekk | So now from our excel sheet only thing pending is cache_manage_policy | 13:47 |
| abhishekk | everything else is covered | 13:47 |
| dansmith | nice | 13:48 |
| abhishekk | gate job cleared for py36 on upload policy patch | 13:53 |
| dansmith | well, that's good for us, but bad for understanding what the real deal is :/ | 13:54 |
| dansmith | not much point in rechecking the two revert patches if others aren't failing | 13:54 |
| abhishekk | yes | 13:54 |
| abhishekk | sounds really related to load now | 13:55 |
| abhishekk | I have also reverted policy warning patch of gmann in locally and ran it 20+ times without failure | 13:55 |
| abhishekk | Even now upstream is skipping 7 tests but my local environment is skipping only 6 :D | 13:57 |
| dansmith | hmm | 13:57 |
| abhishekk | jokke_, rosmaita, dansmith, smcginnis, croelandt upstream meeting in 3 minutes at #openstack-meeting | 13:57 |
| dansmith | abhishekk: I'm thinking the landing of gmann's warning squelch patch *has* to be related | 14:25 |
| dansmith | and speaks to the load theory | 14:25 |
| dansmith | related to why it's down today from yesterday I mean | 14:25 |
| abhishekk | could be | 14:25 |
| dansmith | not sure if it was in line ahead of the revert that timed out yesterday or not | 14:26 |
| abhishekk | Earlier also when we were facing timeout issue we reduced some deprecation logs | 14:26 |
| dansmith | well, given the behavior I think it has to be that there's still some deadlock opportunity, and we're just more likely to hit it under high load conditions or something | 14:27 |
| dansmith | logging is a classic deadlock thing where you're doing IO in critical regions where you wouldn't otherwise be | 14:27 |
| abhishekk | agree | 14:29 |
| abhishekk | Is it worth trying in local by reducing concurrency or with only one worker ? | 14:31 |
| gmann | failure rate is very low now right? https://zuul.openstack.org/builds?job_name=openstack-tox-functional-py36&project=openstack%2Fglance | 14:31 |
| abhishekk | gmann, right | 14:32 |
| abhishekk | I saw no timeout in last 12 hours | 14:33 |
| abhishekk | this means we also need to add that warning related stuff in our current patch which is in gate and all other patches | 14:35 |
| abhishekk | dansmith, ^^ | 14:35 |
| abhishekk | upload one is fine | 14:35 |
| dansmith | abhishekk: oh because we're instantiating more enforcers? yeah | 14:35 |
| dansmith | gmann: yeah failure rate is low, we're thinking maybe because of your logging patch, which lowered the incidence of it or something | 14:36 |
| abhishekk | I will look at my other metadef patches there only we have different test classes | 14:36 |
| gmann | yeah, may be | 14:37 |
| abhishekk | I have set WorkFlow to -1 to my other patches | 14:40 |
| abhishekk | dansmith, croelandt I think we should plan review strategy now | 14:45 |
| dansmith | what does that mean? sequencing? | 14:48 |
| abhishekk | yes | 14:50 |
| abhishekk | like image API is almost covered so we should give priority to all patches remaining to image API | 14:51 |
| abhishekk | then metadef and others | 14:52 |
| opendevreview | Merged openstack/glance master: Check upload_image policy in the API https://review.opendev.org/c/openstack/glance/+/804463 | 15:06 |
| abhishekk | \o/ | 15:07 |
| * abhishekk dinner break | 15:08 | |
| opendevreview | Merged openstack/glance master: Refactor gateway auth layer for task APIs https://review.opendev.org/c/openstack/glance/+/802243 | 15:09 |
| opendevreview | Merged openstack/glance master: Deprecate task specific policies https://review.opendev.org/c/openstack/glance/+/802244 | 15:32 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Check download_image policy in the API https://review.opendev.org/c/openstack/glance/+/804547 | 15:48 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Check policies for staging operation in API https://review.opendev.org/c/openstack/glance/+/804558 | 15:59 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Check policies for delete image for store in API https://review.opendev.org/c/openstack/glance/+/804585 | 16:09 |
| dansmith | abhishekk: on the add_image patch, you're saying that the DB also does its own checking on whether or not the user is admin before letting them create an image as another project? | 16:12 |
| abhishekk | yes | 16:12 |
| dansmith | okay so the functional tests are hitting that in the rbac job but just don't care about the message I guess | 16:13 |
| dansmith | why does that message even make sense from the db? "not visible" on a create? | 16:13 |
| abhishekk | because same method is called from get and update as well | 16:14 |
| dansmith | okay, in _image_get() does that mean it's actually created and they just can't see it then? | 16:14 |
| abhishekk | could be, I didn't checked db record | 16:15 |
| dansmith | I'm confused because _image_get() is called in _image_update() *if* the image_id is passed in, which shouldn't be I would hope for create | 16:16 |
| abhishekk | but we will get this error message while crating the image that it is not visible but with auth layer I guess it will be giving error you are not permitted to create image | 16:16 |
| dansmith | I don't really see how this could get hit in db.image_create() I guess I will have to try to poke it myself | 16:17 |
| dansmith | but either way, | 16:17 |
| dansmith | if you could hit it with rbac, that means our rule isn't right..right? | 16:18 |
| abhishekk | it will if RBAC is not enabled | 16:18 |
| abhishekk | yeah | 16:18 |
| abhishekk | enabaled | 16:18 |
| dansmith | rbac _not_ enabled? | 16:18 |
| abhishekk | rbac enabled | 16:18 |
| dansmith | okay then that means we're passing our add_image check but shouldn't | 16:18 |
| abhishekk | yeah, so our add image should contain role:admin or project_id:context.owner ? | 16:19 |
| dansmith | I dunno actually | 16:21 |
| dansmith | er, maybe something like that | 16:21 |
| dansmith | let me repro first | 16:21 |
| abhishekk | ack | 16:22 |
| dansmith | surely we have a functional test that tries to create an image for another owner as admin right? I'm not seeing one. | 16:30 |
| abhishekk | :D | 16:30 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Check policies for delete image for store in API https://review.opendev.org/c/openstack/glance/+/804585 | 16:30 |
| abhishekk | we don't have functional test which is using another Project (except image-member tests I guess) | 16:31 |
| dansmith | eesh | 16:32 |
| abhishekk | hit first time out of the day | 16:34 |
| abhishekk | https://review.opendev.org/c/openstack/glance/+/804558/ | 16:34 |
| abhishekk | still in the zuul | 16:35 |
| croelandt | let'sget rid of that job :) | 16:35 |
| abhishekk | :D | 16:35 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Check deactivate, reactivate policy in the API https://review.opendev.org/c/openstack/glance/+/798266 | 16:36 |
| dansmith | abhishekk: okay added this to the patch: | 16:37 |
| dansmith | testtools.matchers._impl.MismatchError: "You are not permitted to create images owned by 'someoneelse'" not in '403 Forbidden\n\nForbidding request, image 38f1227f-7857-4a1d-a322-8618f3bcddbf not visible\n\n ' | 16:37 |
| dansmith | succeeds in functional, fails as above in -rbac | 16:37 |
| abhishekk | ack | 16:38 |
| opendevreview | Pranali Deore proposed openstack/glance master: Move metadef resource type association policy checks in the API https://review.opendev.org/c/openstack/glance/+/799637 | 16:47 |
| opendevreview | Pranali Deore proposed openstack/glance master: Move metadef property policy checks in the API https://review.opendev.org/c/openstack/glance/+/799635 | 16:47 |
| opendevreview | Pranali Deore proposed openstack/glance master: Move metadef tag policy checks in the API https://review.opendev.org/c/openstack/glance/+/799636 | 16:47 |
| opendevreview | Pranali Deore proposed openstack/glance master: Implement project personas for metadef namespaces https://review.opendev.org/c/openstack/glance/+/798700 | 16:47 |
| abhishekk | 2nd time out of the day | 16:50 |
| abhishekk | looks like it is related to load only, suddenly we have multiple patches in zuul and timing out started | 16:53 |
| abhishekk | to avoid zuul traffic I will rebase my metadef related patches tomorrow | 17:08 |
| opendevreview | Lance Bragstad proposed openstack/glance master: trivial: Double quote check_string for consistency https://review.opendev.org/c/openstack/glance/+/805256 | 17:18 |
| opendevreview | Lance Bragstad proposed openstack/glance master: trivial: Double quote check_str for consistency https://review.opendev.org/c/openstack/glance/+/805256 | 17:18 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Move Tasks policy checks in the API https://review.opendev.org/c/openstack/glance/+/802245 | 17:21 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Check policies for image import operation in API https://review.opendev.org/c/openstack/glance/+/804590 | 17:21 |
| abhishekk | address ToDo in task patch | 17:22 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Add missing forbidden to not found case for GET namespace API https://review.opendev.org/c/openstack/glance/+/804358 | 17:25 |
| opendevreview | Dan Smith proposed openstack/glance master: Check add_image policy in the API https://review.opendev.org/c/openstack/glance/+/804800 | 17:35 |
| dansmith | abhishekk: your point clearly identified that create-as-another-user wasn't being checked properly in the rbac rule, and even that the way I had arranged things wouldn't allow for that once the rule was corrected | 17:36 |
| dansmith | so I think this ^ is much better now, and I added a test that repro'd it in the -rbac job before I started, and it passes in both now | 17:36 |
| dansmith | in summary, good job reviewing, I guess? :) | 17:37 |
| abhishekk | :D | 17:37 |
| abhishekk | I like the short version :D | 17:37 |
| abhishekk | looks perfect now | 17:41 |
| dansmith | well, still scrutinize it properly | 17:41 |
| abhishekk | ack | 17:43 |
| opendevreview | Brian Rosmaita proposed openstack/glance master: DNM: Add fips check jobs https://review.opendev.org/c/openstack/glance/+/790536 | 17:55 |
| abhishekk | dansmith, I think corresponding protection test also needed now (glance-tempest-plugin), not urgent but just for the note | 18:00 |
| dansmith | abhishekk: a test for create-as-other-owner in rbac tempest via the plugin you mean? | 18:01 |
| abhishekk | yes | 18:01 |
| dansmith | yeah, we probably need to be keeping a list of cases we note likely need more coverage in that scenario | 18:01 |
| dansmith | I was thinking of one the other day too | 18:01 |
| dansmith | but can't remember what it is at the moment | 18:02 |
| abhishekk | stage data with another member? | 18:02 |
| dansmith | no, but probably lots of "with another member" tests now that you mention it | 18:03 |
| abhishekk | I will make a list of those tomorrow | 18:05 |
| dansmith | timeout on my patch just now | 18:11 |
| dansmith | *eyeroll* | 18:11 |
| abhishekk | hmm, just noticed | 18:12 |
| abhishekk | we also have 1st timeout on functional-py38 as well | 18:12 |
| dansmith | where? | 18:12 |
| abhishekk | https://review.opendev.org/c/openstack/glance/+/798700 | 18:13 |
| abhishekk | {5} worker crashed after running 3 db tests | 18:15 |
| abhishekk | all three tests from, glance.tests.functional.db.migrations.test_mitaka02 | 18:16 |
| dansmith | that one is just going slow I think | 18:17 |
| dansmith | where do you see it crashed? | 18:17 |
| abhishekk | it reported timeout | 18:18 |
| dansmith | the worker or the job? | 18:18 |
| abhishekk | job | 18:19 |
| dansmith | oh sure, okay | 18:19 |
| abhishekk | it is out of zuul now | 18:19 |
| dansmith | the worker seems hung | 18:19 |
| abhishekk | yeah, that's what I wanted to say | 18:19 |
| dansmith | this is the reason I say that just killing the py36 job isn't a solution, because this is clearly a real problem, it's just manifesting (right now) in the py36 job with that mysql and those conditions | 18:20 |
| abhishekk | right | 18:20 |
| dansmith | abhishekk: did you follow some guide on setting up those tests? I did for nova aaages ago, but don't really remember much about what is required | 18:22 |
| abhishekk | I think I will enable opportunistic tests in py38 environment as well and try to run multiple functional-py38 tests at a time | 18:22 |
| dansmith | and, did you do all three or just one? | 18:22 |
| abhishekk | I did for mysql and sqlite | 18:22 |
| abhishekk | i just ran tools/test-setup.sh | 18:23 |
| abhishekk | after some corrections | 18:24 |
| abhishekk | in it | 18:24 |
| dansmith | ok | 18:24 |
| abhishekk | or you can simply create table in mysql db as stated in test-setup.sh manually as well | 18:25 |
| abhishekk | not table, but openstack-citest db in mysql | 18:28 |
| dansmith | yup | 18:28 |
| abhishekk | it runs postgre tests in gate as well, skips sqlite tests (glance.tests.functional.test_sqlite.TestSqlite.test_big_int_mapping ... SKIPPED: test requires exe: sqlite3) | 18:35 |
| dansmith | yeah, trying to get pg setup now.. it's always a pain | 18:35 |
| abhishekk | ++ | 18:36 |
| abhishekk | will do it as well | 18:36 |
| abhishekk | should I do it on 18.04 if you are doing it on 20.04 ? | 18:36 |
| dansmith | I'm doing it on fedora right now which is :((( | 18:37 |
| abhishekk | ahh, then I will do on both | 18:38 |
| dansmith | skipped 10, I don't think it did anythong | 18:39 |
| abhishekk | generally it skipps 23 in local | 18:40 |
| abhishekk | if you enable mysql then it will comes down to 7 or 6 | 18:41 |
| dansmith | okay it did run mysql and sqlite | 18:42 |
| dansmith | unable to connect to pg because OF COURSE | 18:42 |
| abhishekk | right | 18:43 |
| dansmith | okay got all three working... on 20.04 | 18:59 |
| dansmith | so I'll run that in a tight loop and see | 19:00 |
| dansmith | if nothing happens, then I'll try generating some load on that machine to slow it down | 19:00 |
| dansmith | running mysql+sqlite in a tight loop on the fedora machine in parallel | 19:00 |
| abhishekk | cool, I also got all 3 working on 18.04 | 19:04 |
| * abhishekk signing out for the day | 19:36 | |
| abhishekk | I have tried running functional-py36 on 18.04 and functional-py38 on 20.04, 5 times with just one worker - Not able to reproduce | 19:42 |
| abhishekk | will continue tomorrow | 19:42 |
| dansmith | I think one worker is less likely to hit the kind of load restriction I'm thinking of | 19:49 |
| dansmith | one worker means everything is serialized, which is less likely to race | 19:49 |
| dansmith | but yeah, I'm running mine in a tight loop.. dozens of runs so far and no hang | 19:49 |
| dansmith | will let it go the rest of the day and we'll check in tomorrow | 19:49 |
| abhishekk | ack | 19:52 |
| opendevreview | Abhishek Kekane proposed openstack/glance master: Check policies for image import operation in API https://review.opendev.org/c/openstack/glance/+/804590 | 19:53 |
| abhishekk | can we send download patch through the gate or should wait ? | 19:55 |
| dansmith | I haven't been paying attention to how often we're failing now | 19:55 |
| dansmith | but if you think there's a good chance it'll land, then yeah | 19:55 |
| abhishekk | I think it will | 19:57 |
| dansmith | aight, sent | 19:58 |
| abhishekk | cool | 19:59 |
| abhishekk | I think add_image policy is upto mark now, will give one manual test in local environment tomorrow to 100% confirm | 20:00 |
| abhishekk | download, both jobs cleared in gate | 20:22 |
| abhishekk | now counting on tempest run :D | 20:22 |
| dansmith | nice | 20:34 |
| lbragstad | so - these are the remaining policy refactor patches, yeah? | 20:35 |
| lbragstad | https://review.opendev.org/q/topic:%2522policy-refactor%2522+(status:open+OR+status:merged) | 20:35 |
| lbragstad | including the image import patch - which isn't included in that list since it's under a different topic? | 20:36 |
| dansmith | I'm going based on the dashboard that abhishekk is cultivating on the spreadsheet | 20:36 |
| dansmith | link at the top | 20:36 |
| dansmith | https://tinyurl.com/glance-xena-3 | 20:36 |
| lbragstad | oh - nice! | 20:36 |
| lbragstad | i was clearly missing some patches then | 20:37 |
| lbragstad | looks like there is more here than what i had | 20:37 |
| abhishekk | now, image import is in above dashboard as well | 20:37 |
| abhishekk | dansmith, I have added some scenarios in policy watch list sheet for tempest plugin | 20:39 |
| abhishekk | lbragstad, ^^ | 20:40 |
| dansmith | abhishekk: what you should have done was _land_ that plane | 20:40 |
| dansmith | I mean.. you should have gone to bed like you promised :) | 20:40 |
| abhishekk | yeah, going now | 20:40 |
| abhishekk | good day | 20:40 |
| * dansmith watched top gun this past weekend, so all my references are from that | 20:40 | |
| dansmith | o/ :) | 20:40 |
| abhishekk | o/~ | 20:41 |
| abhishekk | top gun second part? | 20:41 |
| dansmith | no, original | 20:41 |
| abhishekk | cool | 20:41 |
| abhishekk | nice movie | 20:41 |
| dansmith | indeed :) | 20:41 |
| lbragstad | ... wait, there's a second part to top gun? | 20:41 |
| dansmith | lbragstad: new one coming out this year or something | 20:42 |
| abhishekk | Its not released yet | 20:42 |
| dansmith | I reserve the right to hate it, but I hope it's good | 20:42 |
| abhishekk | +1 | 20:42 |
| lbragstad | i bet it's going to be at least 40% beach volleyball scenes | 20:43 |
| dansmith | we can only hope... :P | 20:43 |
| lbragstad | and super sweet high fives | 20:43 |
| dansmith | haha | 20:43 |
| dansmith | I'm tom cruise at 60 and I feel the need, the need for speed...LAX | 20:43 |
| lbragstad | lol | 20:43 |
| dansmith | release date is Nov 19 | 20:44 |
| opendevreview | Brian Rosmaita proposed openstack/glance master: DNM: Add fips check jobs https://review.opendev.org/c/openstack/glance/+/790536 | 21:12 |
| lbragstad | dansmith https://review.opendev.org/c/openstack/glance/+/804800/3/glance/policies/base.py#77 is currently done somewhere in the lower layers? | 21:23 |
| opendevreview | Merged openstack/glance master: Check download_image policy in the API https://review.opendev.org/c/openstack/glance/+/804547 | 21:23 |
| lbragstad | specifically, the project_id:%(owner)s bit? | 21:24 |
| dansmith | did you mean L72? | 21:24 |
| lbragstad | yeah - i'm trying to understand why that's getting pulled up to the check string, and i think it's because something else in glance has maintained that behavior but i don't want to make that assumption | 21:24 |
| dansmith | multiple things actually! | 21:24 |
| * lbragstad grabs popcorn | 21:25 | |
| dansmith | and once we found and removed the first thing, turns out a second thing was doing it too, just in case! for security! | 21:25 |
| lbragstad | db api? | 21:25 |
| dansmith | but yeah, that's a hard-coded-in-python check right now | 21:25 |
| lbragstad | ok | 21:25 |
| dansmith | db api was the second place | 21:25 |
| dansmith | either auth or policy layer was the first, I forget which | 21:25 |
| dansmith | lbragstad: are you asking because you think it's a bad idea there, or because you didn't notice it before? | 21:26 |
| lbragstad | i don't think it's a bad idea, i just want to understand why the check string is changing | 21:26 |
| lbragstad | i mean, everything we're doing is trying to sweep all the policy bits into a single place, right? | 21:27 |
| dansmith | you know, now that you make me look at that.. admin is project admin in the new world, and this says "let the project admin create images for other projects" | 21:27 |
| dansmith | which is probably not what we want right? | 21:27 |
| dansmith | I mean, it was already project-admin before I made this change, but I'm thinking that doesn't make sense | 21:27 |
| lbragstad | yeah - that behavior will mean different things depending on if enforce_secure_rbac=True | 21:27 |
| dansmith | well, right now, if secure_rbac=False this isn't even used | 21:27 |
| dansmith | ...right? because: https://review.opendev.org/c/openstack/glance/+/804800/3/glance/policies/image.py | 21:28 |
| lbragstad | oh - you mean "this" as in "project_id:%(owner)s" | 21:30 |
| lbragstad | and we're not using that today because it's not in the check string | 21:30 |
| dansmith | no I mean the whole rule.. with secure_rbac=false, we're using the deprecated_rule= right? | 21:31 |
| dansmith | because of the enforce_new_defaults tie? | 21:32 |
| lbragstad | https://github.com/openstack/glance/blob/master/glance/cmd/api.py#L111-L117 | 21:32 |
| lbragstad | yeah | 21:32 |
| lbragstad | well - we will be using it since it will be the new default, but it will be logically or'd with the old deprecated policy | 21:33 |
| dansmith | I didn't know that it was logically or'd, I thought it was ignored | 21:35 |
| lbragstad | but - we won't be using it exclusively unless an operator flips those two switches intentionally | 21:35 |
| dansmith | but okay | 21:35 |
| dansmith | probably the same difference I guess | 21:35 |
| lbragstad | anywho - i was trying to track down where in the code glance was doing that hard-coded ownership check | 21:35 |
| dansmith | https://github.com/openstack/glance/blob/dd3155516cec2cabf8f74963a44ab642d507384b/glance/api/authorization.py#L201-L205 | 21:37 |
| lbragstad | ok - awesome | 21:37 |
| dansmith | going back to my other point, | 21:37 |
| dansmith | this basically says "if you're an admin for project pepsi, you can create coke images" | 21:38 |
| dansmith | which is not really what we want right? | 21:38 |
| lbragstad | correct - but that's orthogonal to the specific rule you're adding your change | 21:38 |
| dansmith | well, because we're not yet to "system scope" or "system persona" or something right? | 21:39 |
| lbragstad | yes | 21:39 |
| dansmith | tbh I have a hard time keeping straight what each thing really means in terms of coverage | 21:39 |
| lbragstad | that just comes from the nature of overloading admin for everything https://review.opendev.org/c/openstack/glance/+/804800/3/glance/policies/base.py#73 | 21:39 |
| dansmith | personas, scope, system scope, etc | 21:39 |
| lbragstad | yeah - i can empathize with that | 21:39 |
| lbragstad | so - your concern is that pepsi admin can create images for coke | 21:40 |
| dansmith | I mean, that's not the correct behavior when admin means "project admin" right? | 21:40 |
| lbragstad | yes - exactly | 21:41 |
| dansmith | otherwise, why not let anyone create images for other people :) | 21:41 |
| lbragstad | we will need to update that policy when glance consumes system scoped tokens | 21:41 |
| lbragstad | or at least update it to be (role:admin and project_id:%(project_id)s and project_id:%(owner)s) | 21:41 |
| dansmith | um, update what, just the role:admin part to the above? | 21:43 |
| dansmith | so that you can create images for projects you're an admin of? in that case, why is that different from just being a member of that project | 21:44 |
| dansmith | ? | 21:44 |
| lbragstad | oh - right, it's not | 21:44 |
| lbragstad | it would need to be role:admin and system_scope:all | 21:44 |
| lbragstad | sorry | 21:44 |
| lbragstad | ok - found the second check https://github.com/openstack/glance/blob/dd3155516cec2cabf8f74963a44ab642d507384b/glance/db/sqlalchemy/api.py#L301-L303 | 21:49 |
| dansmith | actually no I don't think that comes into play on create, | 21:51 |
| dansmith | or at least it didn't in testing | 21:51 |
| dansmith | it was actually here:" | 21:51 |
| dansmith | https://github.com/openstack/glance/blob/dd3155516cec2cabf8f74963a44ab642d507384b/glance/db/sqlalchemy/api.py#L971 | 21:51 |
| dansmith | somehow we'd fail to get the image we were creating, but hadn't created | 21:51 |
| dansmith | I didn't dig too deep, because we shouldn't have gotten to the DB layer anyway, noting that policy should have stopped us first | 21:52 |
| lbragstad | oh - the visibility chekc | 21:52 |
| lbragstad | got it | 21:52 |
| lbragstad | ok - and that makes it up to https://review.opendev.org/c/openstack/glance/+/804800/3/glance/api/v2/policy.py@215 | 21:54 |
| lbragstad | nice | 21:54 |
| dansmith | yeah that's the legacy enforce-this-in-code thing, basically to behave like the auth layer does if we're not running a rule that can check it for us | 21:57 |
| dansmith | (or it's logically OR-d with a no-op :) | 21:57 |
| lbragstad | right - so *everything* is technically in the API layer, regardless if they're using the new policy enforcement or even policy at all | 21:58 |
| lbragstad | which is what i got from https://review.opendev.org/c/openstack/glance/+/804800/3/glance/tests/functional/v2/test_images_api_policy.py | 21:58 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!