Friday, 2021-03-05

« Thursday, 2021-03-04 Index Saturday, 2021-03-06 »
openstackgerritDan Smith proposed openstack/glance master: WIP: Pass a target to task policy enforce  https://review.opendev.org/c/openstack/glance/+/77879900:08
dansmithlbragstad: total guess for when you get back ^00:08
*** tosky has quit IRC00:11
dansmithseems to work for me locally00:12
*** yoctozepto has quit IRC00:13
*** yoctozepto has joined #openstack-glance00:13
dansmithoh yeah, I think that's the ticket :)00:39
*** ajitha has joined #openstack-glance00:51
*** felixhuettner[m] has quit IRC01:49
*** k_mouza has joined #openstack-glance02:21
*** k_mouza has quit IRC02:26
*** felixhuettner[m] has joined #openstack-glance02:31
lbragstaddansmith dang - i suppose... everything was essentially "" before... so having the target be {} didn't matter02:42
lbragstadbut now that the check is reasonable and more opinionated, the target needs to be, too02:43
lbragstadjust 3 failures it looks like https://zuul.opendev.org/t/openstack/build/875a97024c6a4075b35e7e93fb2aa3ac02:45
*** irclogbot_0 has quit IRC02:52
*** irclogbot_2 has joined #openstack-glance02:55
*** gyee has quit IRC03:08
dansmithlbragstad: yeah, about the target03:37
dansmithlbragstad: so I guess that policy rule also probably is not usable now anyway03:37
dansmithmeaning you can't really write sane rules? or maybe you could if it's just a role check or somethin03:38
dansmithlbragstad: but yeah, still several class of problem to resolve in the tests, but muuuuuch closer03:38
*** k_mouza has joined #openstack-glance03:59
*** k_mouza has quit IRC04:04
*** ratailor has joined #openstack-glance04:37
*** ratailor_ has joined #openstack-glance04:59
*** ratailor__ has joined #openstack-glance05:02
*** ratailor has quit IRC05:03
*** ratailor_ has quit IRC05:06
*** eandersson has left #openstack-glance05:42
*** udesale has joined #openstack-glance05:55
*** rcernin has quit IRC06:00
openstackgerritAbhishek Kekane proposed openstack/glance master: WIP: download_image policy check  https://review.opendev.org/c/openstack/glance/+/77884506:33
*** ralonsoh has joined #openstack-glance07:03
*** zzzeek has quit IRC07:52
*** zzzeek has joined #openstack-glance07:53
*** lpetrut has joined #openstack-glance08:00
*** zzzeek has quit IRC08:08
*** zzzeek has joined #openstack-glance08:09
*** zzzeek has quit IRC08:40
*** zzzeek has joined #openstack-glance08:41
*** zzzeek has quit IRC08:42
*** zzzeek has joined #openstack-glance08:43
*** tosky has joined #openstack-glance09:23
*** k_mouza has joined #openstack-glance09:42
*** Underknowledge has quit IRC09:48
*** Underknowledge1 has joined #openstack-glance09:49
*** Underknowledge1 is now known as Underknowledge09:49
*** udesale has quit IRC10:59
*** udesale has joined #openstack-glance10:59
*** k_mouza has quit IRC11:10
*** jv_ has quit IRC11:15
*** k_mouza has joined #openstack-glance11:22
*** k_mouza has quit IRC12:16
*** tkajinam has quit IRC12:35
*** tkajinam has joined #openstack-glance12:35
*** ratailor__ has quit IRC12:36
*** belmoreira has joined #openstack-glance12:40
lbragstaddansmith yeah - i'll roll that patch into the main one for tasks here in a bit12:41
lbragstadgood call12:41
*** k_mouza has joined #openstack-glance12:52
*** k_mouza has quit IRC12:52
*** k_mouza has joined #openstack-glance12:53
*** whoami-rajat has joined #openstack-glance12:58
lbragstadso - now i'm wondering...13:22
lbragstadafaict - we have policies in the policy file that, when changed to be inconsistent with other policies in glance (copy_image/set_image_location/get_image_location) or other policies in OpenStack, it'll cause issues like this13:25
lbragstadthe external task APIs remains admin-only and is protected by the task_api_access policy13:27
lbragstad(and it's deprecated)13:27
lbragstadi'm just digging through old branched13:30
lbragstadbranches*13:30
lbragstadstable/ocata for example wires the policy for add_task up to the controller13:31
lbragstadbut yeah, still several class of problem to res13:31
lbragstadbad paste from above*13:31
lbragstadhttps://github.com/openstack/glance/blob/stable/ocata/glance/api/v2/tasks.py#L7713:31
lbragstadhttps://github.com/openstack/glance/blob/stable/ocata/glance/api/policy.py#L340-L34213:31
lbragstadhttps://github.com/openstack/glance/blob/stable/ocata/glance/api/policy.py#L344-L34613:32
lbragstadi don't think that flow is possible anymore now that the API is deprecated and moved to admin-only13:32
lbragstadshould we consider removing the add_task, get_tasks, get_task, and modify_task policies?13:33
openstackgerritLance Bragstad proposed openstack/glance master: Update default policies for task API  https://review.opendev.org/c/openstack/glance/+/76320813:38
openstackgerritLance Bragstad proposed openstack/glance master: Make secure RBAC protection job voting  https://review.opendev.org/c/openstack/glance/+/77825813:38
openstackgerritLance Bragstad proposed openstack/glance master: DNM: Check some jobs with new policy defaults  https://review.opendev.org/c/openstack/glance/+/77873613:38
abhishekklbragstad, at this time of cycle it is not good idea to remove those policies, may be in next cycle we can consider it13:59
abhishekksmcginnis, if you are around and have little time, could you please have a look at below patches?14:01
abhishekkhttps://review.opendev.org/c/openstack/glance/+/77586014:01
abhishekk https://review.opendev.org/c/openstack/python-glanceclient/+/77640314:02
abhishekkhttps://review.opendev.org/c/openstack/glance/+/77701214:02
abhishekkhttps://review.opendev.org/c/openstack/glance/+/77849314:02
lbragstadabhishekk ok - i updated the most recent tasks policy patch to only include a role check... since i'm not sure building a valid target is possible14:21
lbragstadbut, i could be wrong there, too14:21
abhishekkso for valid target at the moment we just need project_id right?14:22
dansmithlbragstad: modify_task should have a task dict as the target, but we don't have one immediately where we need it I think, maybe that's what you meant14:24
abhishekktask_dict like similar what we passing as ImageTarget?14:25
dansmithyeah, I mean that's the *right* thing to do, but I'm not sure it really matters,14:27
dansmithbecause having this policy check here just allows operators to break import in weird ways they wouldn't understand14:27
dansmithif there's no way for import to work without these "inner" policies being configured the same as import, then they're pretty pointless14:28
dansmithbut I agree I'd be nervous about removing them at this point in the cycle14:28
abhishekkSo how about we not touching task policies at all?14:29
abhishekkor just the one which we are ideally depends like add_tasks?14:29
dansmithwill it work with just that one? the task stuff uses the context to modify the tasks during the process, I didn't test14:31
dansmithwe could just leave them blank (like they are today) or just do what I did in my WIP patch, and then remove them in X14:31
abhishekkFormer option sounds good to me14:32
dansmithalso,14:32
dansmithdoes your image/tasks api need get_tasks permission?14:32
abhishekkno14:32
dansmithit bypasses these layers? I can't remember14:32
abhishekkIt bypasses these layers14:33
dansmithsmart guy :)14:33
abhishekkI thought there was no point if we want this open for all14:34
dansmithyup14:35
abhishekkLooks like we have plenty of work to do in next cycle14:35
lbragstadok - so we're thinking about just leaving those policies open because 1.) they're only internal policies 2.) we can't really build a valid target for it anyway14:37
lbragstadand we'll deprecate them for removal next cycle?14:37
abhishekkand task API's are already marked as deprecated14:38
dansmithlbragstad: and you can't really make them more restrictive than the high-level operations that use them or things break14:38
lbragstadyeah - exactly14:38
dansmithlbragstad: we're considering whether to do that, or just do what I did in my WIP14:38
dansmitheither works for me.. it'd debt either way, one feels more consistent with what we're doing now (passing a target) and the other feels more like "we're ignoring this because we're going to remove it anyway"14:39
dansmiths/it'd/it's/14:39
lbragstadyeah...14:39
abhishekkdansmith, with your WIP patch as well I saw couple of 403 for add_task14:39
lbragstadwould anyone be opposed to deprecating them this cycle as is?14:41
abhishekkI doubt, I remember either rosmaita or jokke saying even task APIs are deprecated we need to keep it forever14:43
openstackgerritFelix Huettner proposed openstack/glance master: Fix users being able to delete disabled images  https://review.opendev.org/c/openstack/glance/+/77895114:44
dansmithabhishekk: yep, I commented that I'd look at those this morning14:44
abhishekkack14:44
dansmithlbragstad: so what's your suggestion? leave them as they are today but mark them as deprecated in W?14:45
dansmithbasically drop that patch?14:46
openstackgerritGhanshyam proposed openstack/glance master: Move setting of enforce_scope to devstack side  https://review.opendev.org/c/openstack/glance/+/77895214:48
lbragstaddansmith yeah - just mark them as deprecated14:48
lbragstadthey would still work as is, but it would be a stronger signal to operators?14:48
lbragstadwith a warning that says modifying these policies can result in weird issues with other services and call out the copy-image thing we're hitting14:49
* lbragstad shrugs14:49
lbragstadi have the least experience of anyone here :)14:50
openstackgerritGhanshyam proposed openstack/glance-tempest-plugin master: Consume enforce_scope config from Tempest  https://review.opendev.org/c/openstack/glance-tempest-plugin/+/77895414:51
gmannlbragstad: ^^ consuming the enforce_scope setting from tempest/devstack side14:53
gmannthis and its depends-on patches14:53
dansmithlbragstad: yeah, maybe it's just because it's friday but I don't have a strong opinion.. I defer to abhishekk :)14:53
lbragstadgmann excellent - thank you14:53
gmannlbragstad: I will start adding devstack setting for other services too so that you can consume those in yoru tests14:54
abhishekkI am just thinking that we can mark these policies as deprecated now, and work towards either improving them with context to rbac in next cycle or else remove them as per deprecation policy in following cycle which will be Y or Z14:55
gmannjust for context, in nova we still added the new policy for deprecated API also (as they might be used) for consistency of token and policy enforcement. and policy can go away along with APIs.14:57
gmanneven policy were already deprecated with API sometime in old releases.14:58
abhishekkfor glance I think we have only marked API as deprecated and restrict its use for Admin only (at least that was the case earlier)14:59
*** lpetrut has quit IRC15:03
lbragstadok - i can update that patch again - i just rebased things to see if a role-only check string would help15:07
abhishekkack15:07
lbragstadi have to run to an appt, but feel free to tinker with things in the mean time if y'all want to try anything, i'll catch up on the differences when i get back15:08
abhishekkack15:13
dansmithso, if we just leave them as-is and deprecate,15:33
dansmithdoes that mean that a scoped token will be unusable for any path that runs across those rules?15:33
dansmithor are those unlikely to really be usable for anything in W anyway?15:34
abhishekknot sure15:34
*** udesale has quit IRC15:43
openstackgerritGhanshyam proposed openstack/glance-tempest-plugin master: Consume enforce_scope config from Tempest  https://review.opendev.org/c/openstack/glance-tempest-plugin/+/77895415:49
dansmithlbragstad: okay so we're still failing the download_image policy check for some things.. I see you've got a specific rule for that, but it's hard for me to tell what is actually failing about that check15:56
*** jv_ has joined #openstack-glance15:57
abhishekkdansmith, even after changing it to admin_or_project_member it keeps failing16:06
abhishekkhttps://review.opendev.org/c/openstack/glance/+/77884516:06
dansmithabhishekk: okay that failed like everything though.. are you sure something else didn't break?16:07
dansmiththe weird thing is that it only fails on a couple of situations, like server rescue16:08
dansmithand that rescue image is created in the test itself16:08
abhishekkno, didn't checked thorough16:09
dansmithhttps://52be629c92393d57c88f-4c748ecb2936007225f2a0c5ad22f670.ssl.cf1.rackcdn.com/778799/1/check/tempest-integrated-storage-import/8c3b7eb/testr_results.html16:09
dansmithwe failed a couple of base image tests with that same download_image thing16:09
dansmithwhich seems weird16:09
dansmithoh, interesting16:10
dansmithhang on let me confirm something16:12
abhishekkok16:12
dansmithokay nevermind, I thought it seemed like the tests might be calling deactivate on the cirros image or something,16:18
dansmithbut I see the deactivate test tries to download the image, which is what looked like it was the other test at the same time trying to get the image we just deactivated, but it's not16:18
dansmithalthough.. that's the thing that's failing in the deactivate test.. the show_image16:19
dansmithoh,16:20
dansmiththat one is failing because we don't reactivate the image apparently16:20
dansmithhttps://github.com/openstack/tempest/blob/4de12b1113a2b9a1b1991dba87572706302cd414/tempest/api/image/v2/test_images.py#L337-L33916:21
dansmithfailing with download_image there16:21
dansmithso we can show the image after reactivation as expected, but we can't download it16:21
dansmithalthough we didn't try downloading it before deactivation, so ... maybe it isn't related to the deactivation, I dunno16:22
dansmithahh, these are the only two tests that try to download the image16:23
dansmithI guess I'm pretty slow16:24
abhishekkyes16:24
dansmithbut I guess that means that the cirros public image is fine, and nova can use that for almost everything,16:24
dansmithbut rescue downloads and re-creates the image, and it's these images that we create in tempest that are not downloadable because of this policy?16:24
abhishekkwhat is the visibility of that image?16:25
dansmithhttps://github.com/openstack/tempest/blob/4de12b1113a2b9a1b1991dba87572706302cd414/tempest/api/image/v2/test_images.py#L23016:25
dansmithprivate16:25
dansmithin that case at least16:26
abhishekkthat could be a problem?16:26
dansmiththat _should_ be okay right? private means "private to me"16:27
dansmithabhishekk: maybe member_id isn't in the target?16:29
dansmithoh, there's also no allowance for visibility==private?16:30
abhishekkyeah that was I thought16:30
dansmithabhishekk: that's probably it16:30
abhishekkbut it is failing for admin_or_project_member as well16:30
dansmithalthough16:31
dansmiththose are OR'd in and we should have passed project_id==$us16:31
abhishekkhmm16:32
* lbragstad catches up 16:37
lbragstadstill having issues with the download_image policy?16:39
dansmithyup, that's what we're trying to figure out16:39
lbragstadso the image that gets created from the server is technically considered private, yeah?16:49
dansmith"from the server" ?16:49
lbragstadi worded that wrong - i'm looking at one of the failed tests https://github.com/openstack/tempest/blob/bf66abce089a2e2169b9945a18d1e6508dfcdb44/tempest/api/compute/servers/test_server_rescue.py#L16516:50
lbragstadthat creates a server and then updates the image16:51
dansmithoh sorry, you're right it does actually create a snapshot and use that as the rescue image16:51
dansmithI thought it was copying the cirros image to make that, but clearly didn't read16:51
dansmithhowever,16:51
dansmiththe other base image test doesn't use a server as the intermediate, so it's a little more straightforward16:51
dansmithhttps://github.com/openstack/tempest/blob/4de12b1113a2b9a1b1991dba87572706302cd414/tempest/api/image/v2/test_images.py#L21116:51
lbragstadok - looking at that quick16:52
lbragstadhttps://52be629c92393d57c88f-4c748ecb2936007225f2a0c5ad22f670.ssl.cf1.rackcdn.com/778799/1/check/tempest-integrated-storage-import/8c3b7eb/testr_results.html didn't come from https://review.opendev.org/c/openstack/glance/+/778799/1 i don't think?16:57
dansmithlbragstad: it did, look at the url16:58
dansmith778799/1/check/16:58
lbragstadaha - ok, nevermind16:59
lbragstadi was looking at the nova-ceph rbac job16:59
dansmitheasier to look at the base job(s) now just because there is less going on17:01
dansmithI went straight for the most complicated job first, but realized we should get a very basic job run and focus on that, since it doesn't require the complex multistore copy stuff to do *anything*17:02
abhishekkyeah, results of nova-ceph-rbac looks scary17:03
abhishekkso from logs its not possible to know which policy check is invoked, right?17:03
dansmithyeah, you can17:04
dansmithif oslo_policy debug is enabled17:04
dansmiththe nova-ceph-multistore job has that, which is another reason I went for that first17:04
dansmithwe could add that debug to the others though17:04
abhishekkSo could we have one patch wehere only two jobs (remove all other jobs) with debug flag on?17:04
dansmithhttps://github.com/openstack/nova/blob/master/.zuul.yaml#L40017:04
lbragstadhttp://paste.openstack.org/raw/803288/17:05
dansmithit sucks how you have to do it though17:05
lbragstadthat's the oslo.policy debug logging from the nova-ceph-rbac job for this test17:05
lbragstadhttps://github.com/openstack/tempest/blob/4de12b1113a2b9a1b1991dba87572706302cd414/tempest/api/image/v2/test_images.py#L21117:05
dansmiththe gate queues are filling up quick, good luck getting any results for about 10h at this point :/17:06
lbragstadbased on that output - i don't see how that's failing the download_image check17:06
lbragstadthe project_id from the context and the owner from the target match...17:07
lbragstadso - we should be short-circuiting the policy check at the first condition of the project-member check string for downloading the image17:07
abhishekktarget does not have project id17:08
*** belmoreira has quit IRC17:10
lbragstaddamn...17:10
lbragstad "owner": "494c741f16504a8d96264a65cdd3a2de" is right though17:10
dansmithabhishekk: target does not have a project_id where?17:10
abhishekkin the above paste17:10
abhishekkhttp://paste.openstack.org/raw/80328817:11
lbragstadyep - i see it17:11
lbragstadthat should be running through this though? https://review.opendev.org/c/openstack/glance/+/764754/27/glance/api/policy.py@26917:11
dansmithokay I see but I don't know where in the code that is,17:11
dansmithlbragstad: right exactly17:11
dansmithI'm saying target _should_ have a project_id based on what I see17:12
lbragstadwhat are these doing? https://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L16017:13
abhishekkhttps://review.opendev.org/c/openstack/glance/+/764754/27/glance/api/policy.py@21117:13
lbragstadhttps://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L29217:14
dansmithlbragstad: huzzah, I bet that's it17:14
abhishekk+117:14
lbragstadit download_image is getting invoked from the image controller, 'project_id' should be set, even if it's None17:15
lbragstads/it/if/17:15
abhishekkright, this means it is visiting cache for downloading image from the cache17:16
* abhishekk brb17:17
openstackgerritLance Bragstad proposed openstack/glance master: Implement project personas for image actions  https://review.opendev.org/c/openstack/glance/+/76475417:19
openstackgerritLance Bragstad proposed openstack/glance master: Update default policies for task API  https://review.opendev.org/c/openstack/glance/+/76320817:19
openstackgerritLance Bragstad proposed openstack/glance master: Make secure RBAC protection job voting  https://review.opendev.org/c/openstack/glance/+/77825817:19
openstackgerritLance Bragstad proposed openstack/glance master: DNM: Check some jobs with new policy defaults  https://review.opendev.org/c/openstack/glance/+/77873617:19
dansmithlbragstad: surely that is a bug too,17:19
lbragstadhttps://review.opendev.org/c/openstack/glance/+/764754/28/glance/api/middleware/cache.py trying this17:19
dansmithbecause it's not passing the full image as the target,17:19
dansmithso you could write a rule that would work against the real one, but fail in the cache one?17:19
lbragstadpotentially?17:19
dansmithI'm not even sure what the image_metadata thing is that it's passing17:20
lbragstadhttps://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L11617:20
lbragstadit's an image reference - from what i can tell17:20
lbragstadbut - it also looks contingent on the API version17:21
lbragstadso - i'm not sure if that changes the image reference depending on the API version (e.g., you might have a policy that works for v1 but not for v2)17:21
lbragstaddepending on the image?17:21
dansmithoh I see and it does wrap it in a target, okay17:21
lbragstadi guess now i'm wondering why this is in middleware?17:22
dansmithsomething something caching17:22
lbragstadspecifically, if the caching bits are in middleware, do they need to have their own enforcement calls?17:23
*** k_mouza has quit IRC17:24
lbragstadif we get past https://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L149 we know the image *isn't* cached, right?17:24
dansmithlbragstad: looks like they have them right?17:25
lbragstadat which point wouldn't https://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L169 just invoke the normal API (that has policy enforcement in 1.) the controller or 2.) the glance/api/policy.py method)17:25
dansmithbut not for forbidden?17:26
dansmithI'm not sure what you're saying17:26
lbragstadi'm wondering if this code is redundant https://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L16017:26
dansmithah, because that happens before the process_v2_request on?17:26
dansmith*one17:26
lbragstadyes17:26
lbragstadand if the request is GET /v2/image/{image_id}/data17:27
lbragstadwe're going to enforce download_image policy in the controller anyway, right?17:27
dansmithhrm17:27
dansmithnot if we return this without hitting the controller right?17:27
lbragstadif the request is GET /v2/image/{image_id} - why are we calling enforcement for download_image?17:28
dansmithbut yeah I'm not sure why this wasn't deferring to the controller with the return None17:28
dansmithI thought this thing was for caching the image data17:28
abhishekkAFAIK if image is cached it will not visit the controller17:29
dansmithbased on the PATTERNS above17:29
dansmithabhishekk: right, that's my assumption17:29
lbragstadhmm17:29
abhishekkI guess I am the one who added that check in cache 3-4 years before17:29
lbragstadso - our get_image policy is ADMIN_OR_PROJECT_READER17:30
dansmithabhishekk: which one, L160 or the later one?17:30
abhishekkthe one pointed out by lbragstad17:30
lbragstadwhich is less restrictive than the policy for download_image17:30
dansmithlbragstad: I'm getting lost,17:30
lbragstadsorry - i'll let y'all finish17:31
dansmithwe're clearly hitting this extra check and failing here and not just because of the stricter download policy right?17:31
dansmiththe looser get_image policy would fail here too without the target['project_id'] addition ?17:31
abhishekkprobably17:32
lbragstadcorrect17:32
lbragstadthe targets don't have project_id = image.get('owner')17:32
dansmithack, okay, thought you were saying the stricter policy was the problem17:32
dansmithright17:32
lbragstadbut - now i'm wondering about something else, but related,17:32
dansmithI'm also not sure if/why the policies should be different (by default) between get_image and download_image17:32
lbragstad^ that's what i'm about to get at17:32
dansmiththe operator may want to configure them differently, but by default they should be the same I would think17:33
lbragstadhttps://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L44-L49 means this cache middleware is invoked for GET /v2/images/{image_id} and GET /v2/images/{image_id}/file17:33
dansmithI don't think so17:33
dansmithonly /data for v2 right?17:33
dansmither, /file17:34
lbragstadoh - right17:34
abhishekkyeah /file for v217:34
lbragstadhttps://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L45-L46 is only v117:34
dansmithright17:34
lbragstadok - i was worried by default download_image was going to get enforced before get_image17:34
lbragstadwhich download image having a more restrictive default (breaking project-readers getting images)17:35
dansmithnot here I don't think17:35
* lbragstad stays in his lane17:35
lbragstadok - good deal17:35
dansmithlol17:35
lbragstadso - this middleware doesn't seem to have image get methods for v1 - is it always expected to be run with v2?17:37
lbragstadi don't see a v1 equivalent for https://github.com/openstack/glance/blob/master/glance/api/middleware/cache.py#L10817:37
dansmithI expect v1 and v2 differ in how the user gets the data (i.e. with what url_17:38
dansmithbut I think this is _purely_ about caching the data and not the details17:38
abhishekkmay be removed when v1 related code is removed17:38
lbragstadok - so getting back to dansmith's concern17:41
lbragstadshould download_image and get_image have different policy check strings?17:41
dansmith*defaults* you mean right?17:41
lbragstadyes - defaults17:41
abhishekklbragstad, so related to your recent change at line #160, it will fail again at line #292 because line #169 will call that17:41
abhishekkmay be we can revisit after current discussion17:42
abhishekkI think those should be different17:43
dansmithdifferent defaults?17:43
abhishekkget will have only read access, but download means you can access the data, right?17:43
dansmithyeah, I agree they should be different policy names, but I would think the default values for each would be the same, unless the operator wants to change them17:44
lbragstadget_image would mean in can view data about the image, download_image would mean i can download an actual copy of the image data17:44
abhishekkyou can view the image properties17:45
lbragstadyeah - as a project-reader, i can get the image properties, but i can't actually download the image (at least that's how the defaults are written now)17:46
abhishekkright17:46
dansmithwhat's the use-case there?17:46
lbragstadcan images contain sensitive information that shouldn't be exposed to project-readers?17:46
dansmithbut so can details17:46
dansmithI guess "downloading the image" seems like "reading" to me17:47
dansmithI could put my software license keys in my image details, or my root password, etc17:47
dansmithmaybe I don't know what project reader really is supposed to be17:47
abhishekkright17:47
lbragstadwith the role implications built into keystone, reader is the role with the least privilege within a scope17:48
lbragstadso the person you would trust the least17:48
dansmithsure, but I just don't really know why you'd trust someone with your software keys and root password but not the actual image itself17:49
dansmithand if that's what project_reader is, it seems poorly named :)17:49
abhishekk:D17:49
lbragstadthe least privilege definition applies to the reader role in general - since it the role at the bottom of the hierarchy17:49
lbragstadbut in this case - it's applied to the project scope17:49
dansmithyeah, I get that, I just don't understand why you17:50
abhishekkand that's where property protection comes in the picture, if you put your sensitive information in the image details you can restrict it using property protection17:50
dansmithwould expose one and not the other17:50
*** jawad_axd has joined #openstack-glance17:50
dansmithabhishekk: ack that makes sense17:51
abhishekkand that's the reason why download_image policy is invoked in cache as well17:51
dansmithI guess my problem is just with the name. should be called "project_look_but_no_touch" or something :D17:51
abhishekk:D17:51
dansmithor project_browser17:52
lbragstadyeah - i can see that line of thinking17:53
lbragstadwe attempted to clarify similar concerns in keystone's documentation directly https://docs.openstack.org/keystone/latest/admin/service-api-protection.html#primer17:53
dansmithso project_reader is -----x--x, project_member is ------rwx, and project_admin is ---rwxrwx ?17:53
abhishekkso whats the suggestion, should we keep same defaults for get and download or keep those different in defaults as well?17:53
dansmiththat was backwards17:53
abhishekkI am more inclined towards later one17:54
lbragstaddansmith switch the project_reader with project_member and i think you're right17:54
dansmithlbragstad: I reversed the ordering of the bits , and forgot x for group for project_member17:55
dansmithanyway17:55
lbragstadso - i guess what dansmith is saying, if you name a file 'my_password_is_foo.txt' then put a password (which is foo) in that file, the only thing the current proposal is doing is prevent the project_reader from confirm foo is your password in the file17:55
dansmithright, and abhishekk is saying "but we can selectively hide parts of filenames" (property protection)17:55
dansmithwhich makes sense17:55
dansmithit just seems weird to offer images to users they can't use17:56
dansmithback to my use-case, but it sounds like ya'll are on the same page, which is totally cool with me :)17:56
dansmithnova will do a lot of work on your behalf, allocate networks, volumes, etc, only to find out pretty late that ... sad trombone, I can't download the image17:57
abhishekk:)17:58
lbragstadi originally proposed https://review.opendev.org/c/openstack/glance/+/764754/8/glance/policies/image.py as a project-reader action17:58
abhishekkyes17:59
lbragstadbut i bumped it to project-member and i guess i never really understood the whole story (but this is helping)17:59
dansmithproject reader isn't really a thing today right?17:59
lbragstadcorrect - not unless you're implementing it via custom policy18:00
dansmithit's just a default role option people have after some point?18:00
lbragstadyes - once services are on the same page about default policies and implementing reader support, it will be a thing people can do18:00
dansmithyeah, so then whatever, it's just an option (poorly named IMHO) but it's not going to break anything if everyone is currently a member18:00
dansmithso we need not spend much time on the details18:00
* lbragstad nods18:01
abhishekkagree18:01
lbragstadthat's fair18:01
dansmithabhi says it should default to no download for readers, that's all that matters :)18:01
abhishekk:d18:01
abhishekkso coming back to policy enforcement in cache18:02
abhishekkcan we add project id here, https://review.opendev.org/c/openstack/glance/+/764754/28/glance/api/middleware/cache.py@10218:02
lbragstadoh - i see what you mean18:03
dansmithyeah18:03
*** irclogbot_2 has quit IRC18:03
lbragstad_enforce is only ever dealing with image references in there, it looks like18:04
abhishekkyes18:04
*** irclogbot_3 has joined #openstack-glance18:04
lbragstadok - fixed18:06
openstackgerritLance Bragstad proposed openstack/glance master: Implement project personas for image actions  https://review.opendev.org/c/openstack/glance/+/76475418:06
openstackgerritLance Bragstad proposed openstack/glance master: Update default policies for task API  https://review.opendev.org/c/openstack/glance/+/76320818:06
openstackgerritLance Bragstad proposed openstack/glance master: Make secure RBAC protection job voting  https://review.opendev.org/c/openstack/glance/+/77825818:06
openstackgerritLance Bragstad proposed openstack/glance master: DNM: Check some jobs with new policy defaults  https://review.opendev.org/c/openstack/glance/+/77873618:06
abhishekkSo now this only leaves us with tasks to worry about18:06
dansmithoh I asked about the scope thing18:07
abhishekk?18:07
dansmithlbragstad: if we just ignore the tasks policies, and don't add the scope= thing, will that cause people not to be able to experiment with scopes?18:07
dansmithor should we still add those new scoped rules, but default them to "" like they are today for the "internal" task policies18:08
lbragstadif we don't set scope_types on the rule, i don't think it with break anything18:09
lbragstadeven if someone sets enforce_scope=True18:09
lbragstadthat check is only done in oslo.policy if the rule has scope_types set18:10
lbragstadhttps://opendev.org/openstack/oslo.policy/src/branch/master/oslo_policy/policy.py#L103418:10
*** k_mouza has joined #openstack-glance18:10
dansmithokay, so does that mean we could just drop the tasks patch and replace it with deprecation notices for those policies? I think that's what abhishekk was leaning towards18:10
dansmiths/drop/effectively drop/18:10
lbragstadyes - we could - i can propose the deprecations18:10
abhishekkyeah18:11
dansmithare we out of problems to solve now? :P18:11
abhishekkyour DNM tempest patch will tell us that now18:11
* lbragstad doesn't want to jinx anything18:11
dansmithabhishekk: yep18:12
abhishekkthat was a smart move18:12
dansmith:)18:12
abhishekkNow coming back to other challenges18:14
*** k_mouza has quit IRC18:15
abhishekkAs we are having shortage on reviewers due to busy schedule, I am going to ninja approve some low priority patches to reduce the load in the next week18:15
abhishekkI will keep eyes on them during weekend where most probably will be less traffic in the zuul18:16
*** gyee has joined #openstack-glance18:17
*** irclogbot_3 has quit IRC18:24
*** irclogbot_3 has joined #openstack-glance18:28
abhishekkdansmith, need to follow up with qa guys for this one as well https://review.opendev.org/c/openstack/tempest/+/77430318:34
dansmithabhishekk: what about ninja'ing your tasks api bump and client patch? are you thinking we'll get review on those at some point?18:34
abhishekkno harm18:35
dansmithabhishekk: yep18:35
abhishekkyou could ninja approve those as well18:35
dansmithabhishekk: ack18:35
*** ralonsoh has quit IRC18:35
* abhishekk signing now, have a good weekend all o/~18:42
abhishekkdansmith, just to remind, please rebase your DNM tempest job on top of lbragstad work once deprecation for tasks patch is up18:43
dansmithI thought he did?18:44
lbragstadi'm working on the deprecation patch now18:44
*** jawad_axd has quit IRC18:44
abhishekkyep, he did, sorry I overlooked18:44
dansmithcool18:44
* abhishekk leaving18:44
lbragstadthanks abhishekk18:45
*** jawad_axd has joined #openstack-glance19:05
*** takamatsu has quit IRC19:13
*** k_mouza has joined #openstack-glance19:25
*** takamatsu has joined #openstack-glance19:27
*** k_mouza has quit IRC19:30
*** jv_ has quit IRC19:46
*** jv_ has joined #openstack-glance19:46
*** jawad_axd has quit IRC19:52
*** jawad_axd has joined #openstack-glance19:57
openstackgerritLance Bragstad proposed openstack/glance master: DNM: Test is fake method work  https://review.opendev.org/c/openstack/glance/+/77904220:10
lbragstaddansmith i'm still working through unit test failures on the policy patch - i'm not sure the fakes in the cache middleware unit tests are doing what they should ^20:10
openstackgerritLance Bragstad proposed openstack/glance master: DNM: Test if fake method works in cache middleware tests  https://review.opendev.org/c/openstack/glance/+/77904220:11
dansmithI would find that utterly shocking20:11
dansmithlbragstad: if you want to push up what you have I can try iterating on unit tests while you go do something more important20:11
lbragstadthe code that casts the ImageTarget to a dictionary is failing - and i though it was because the ImageStubs are different20:12
lbragstadhttps://github.com/openstack/glance/blob/master/glance/tests/unit/test_policy.py#L58-L8820:12
lbragstadhttps://github.com/openstack/glance/blob/master/glance/tests/unit/test_cache_middleware.py#L32-L4220:12
lbragstadso i updated this test to fix the ImageStub https://github.com/openstack/glance/blob/master/glance/tests/unit/test_cache_middleware.py#L40720:13
lbragstadand it failed with the same issue20:13
lbragstadlet me push what i have20:14
openstackgerritLance Bragstad proposed openstack/glance master: Implement project personas for image actions  https://review.opendev.org/c/openstack/glance/+/76475420:15
openstackgerritLance Bragstad proposed openstack/glance master: Deprecate the external task API policies  https://review.opendev.org/c/openstack/glance/+/76320820:15
openstackgerritLance Bragstad proposed openstack/glance master: Make secure RBAC protection job voting  https://review.opendev.org/c/openstack/glance/+/77825820:15
openstackgerritLance Bragstad proposed openstack/glance master: DNM: Check some jobs with new policy defaults  https://review.opendev.org/c/openstack/glance/+/77873620:15
openstackgerritMerged openstack/python-glanceclient master: Get tasks associated with image  https://review.opendev.org/c/openstack/python-glanceclient/+/77640320:15
lbragstadthis https://review.opendev.org/c/openstack/glance/+/764754/30/glance/api/middleware/cache.py is what breaks the tests20:15
dansmithpulled it down, will poke20:16
lbragstadack - thank you20:16
lbragstadi have to hop into a meeting, but i'll circle back20:16
dansmithroger20:16
dansmithoh yeah,20:21
dansmiththis is just because there are some properties we expect have to be there,20:21
dansmithbut things treat this like it's a dict of optional stuff, which it's not20:22
lbragstadthis probably isn't here or there, but there are at least two different implementations of ImageStub20:23
lbragstad(one implements container_format, the other doesn't)20:23
lbragstadand the failure for that test which my change complains about not having the container_format key in the Image20:23
lbragstadi think20:24
dansmithlbragstad: I'm sure there are more than tat20:28
dansmithlbragstad: I have a patch still in WIP from last summer to fix one of them that isn't this20:28
dansmithRan 1 test in 0.158s20:28
dansmithOK20:28
lbragstadthis is the failure i get20:29
lbragstadhttp://paste.openstack.org/raw/803291/20:29
dansmithI pasted that to show I fixed on20:30
dansmith*one20:30
openstackgerritDan Smith proposed openstack/glance master: Deprecate the external task API policies  https://review.opendev.org/c/openstack/glance/+/76320820:34
openstackgerritDan Smith proposed openstack/glance master: Make secure RBAC protection job voting  https://review.opendev.org/c/openstack/glance/+/77825820:34
openstackgerritDan Smith proposed openstack/glance master: DNM: Check some jobs with new policy defaults  https://review.opendev.org/c/openstack/glance/+/77873620:34
dansmithlbragstad: ^ how's that?20:34
dansmithoh crap20:36
lbragstadthat passes for you locally?20:36
dansmithI pushed those into the wrong patch20:36
dansmithyeah that whole test_cache_middleware module works for me with that20:37
lbragstadhrm20:37
lbragstadok - weird20:37
lbragstadi tried reusing an image stub i assumed to work20:38
lbragstadhttps://review.opendev.org/c/openstack/glance/+/764754/30/glance/tests/unit/test_cache_middleware.py20:38
lbragstadthe ImageStub in test_policy has the container_format attribute20:38
openstackgerritDan Smith proposed openstack/glance master: Implement project personas for image actions  https://review.opendev.org/c/openstack/glance/+/76475420:40
openstackgerritDan Smith proposed openstack/glance master: Deprecate the external task API policies  https://review.opendev.org/c/openstack/glance/+/76320820:40
openstackgerritDan Smith proposed openstack/glance master: Make secure RBAC protection job voting  https://review.opendev.org/c/openstack/glance/+/77825820:40
openstackgerritDan Smith proposed openstack/glance master: DNM: Check some jobs with new policy defaults  https://review.opendev.org/c/openstack/glance/+/77873620:40
dansmithokay that should be right20:40
dansmithlbragstad: were you looking at this? https://review.opendev.org/c/openstack/glance/+/764754/30..31/glance/tests/unit/test_cache_middleware.py20:41
lbragstadi started copying attributes into ImageStub like you did here https://review.opendev.org/c/openstack/glance/+/764754/31/glance/tests/unit/test_cache_middleware.py#46 - then i noticed we have an ImageStub in glance.tests.unit.test_policy, so I tried reusing it and things still didn't seem to work20:45
lbragstadso that's what made me question if the fake was doing what it was supposed to20:45
lbragstadbut - maybe i overlooked something if they pass for you20:45
dansmithyou'd have to have all of them20:46
dansmithbut I apparently didn't fully pull those changes back a patch, just a sec20:46
*** jv_ has quit IRC20:46
lbragstadah20:48
dansmithah no I also missed a couple cases, hang on20:49
dansmithyeah I think we'll be okay with rbac20:50
dansmithguh20:50
dansmithRan 19 tests in 0.464s20:50
dansmithOK20:50
openstackgerritDan Smith proposed openstack/glance master: Implement project personas for image actions  https://review.opendev.org/c/openstack/glance/+/76475420:52
openstackgerritDan Smith proposed openstack/glance master: Deprecate the external task API policies  https://review.opendev.org/c/openstack/glance/+/76320820:52
openstackgerritDan Smith proposed openstack/glance master: Make secure RBAC protection job voting  https://review.opendev.org/c/openstack/glance/+/77825820:52
openstackgerritDan Smith proposed openstack/glance master: DNM: Check some jobs with new policy defaults  https://review.opendev.org/c/openstack/glance/+/77873620:52
dansmithalso fixed a pep8 error in there20:52
dansmithbut yeah, that passes all the tests in that module for me now20:52
*** jawad_axd has quit IRC20:55
*** hoonetorg has quit IRC20:57
*** jv_ has joined #openstack-glance21:01
*** hoonetorg has joined #openstack-glance21:01
*** k_mouza has joined #openstack-glance21:10
*** k_mouza has quit IRC21:15
*** jawad_axd has joined #openstack-glance21:17
*** ajitha has quit IRC21:18
*** jawad_axd has quit IRC21:31
openstackgerritLance Bragstad proposed openstack/glance-tempest-plugin master: Reuse project credentials from tempest for tenancy checks  https://review.opendev.org/c/openstack/glance-tempest-plugin/+/77905922:32
lbragstaddansmith abhishekk that might help make things more readable and consistent eventually - i know keeping all the users and projects in some of those tests is tough22:33
lbragstadbut - something we can choose to pursue next release if so22:34
openstackgerritLance Bragstad proposed openstack/glance-tempest-plugin master: Reuse project credentials from tempest for tenancy checks  https://review.opendev.org/c/openstack/glance-tempest-plugin/+/77905922:38
openstackgerritLance Bragstad proposed openstack/glance-tempest-plugin master: Reuse project credentials from tempest for tenancy checks  https://review.opendev.org/c/openstack/glance-tempest-plugin/+/77905923:04
dansmithah yeah that's good23:06
dansmithman, 9 things in gate for glance for five hours, none have even been allocated nodes23:13
lbragstad=/ i was looking at that about an hour ago23:13
lbragstadi haven't bothered to check since23:13
*** k_mouza has joined #openstack-glance23:52
*** k_mouza has quit IRC23:57
« Thursday, 2021-03-04 Index Saturday, 2021-03-06 »

Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!