14:00:13 <abhishekk> #startmeeting glance
14:00:13 <opendevmeet> Meeting started Thu Jul  8 14:00:13 2021 UTC and is due to finish in 60 minutes.  The chair is abhishekk. Information about MeetBot at http://wiki.debian.org/MeetBot.
14:00:13 <opendevmeet> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
14:00:13 <opendevmeet> The meeting name has been set to 'glance'
14:00:15 <abhishekk> #topic roll call
14:00:25 <abhishekk> #link https://etherpad.openstack.org/p/glance-team-meeting-agenda
14:00:27 <abhishekk> o/
14:00:27 <dansmith> o/
14:00:36 <rosmaita> o/
14:00:37 <croelandt> o/
14:01:00 <abhishekk> lets wait couple of minutes for jokke_
14:01:04 <jokke_> o/
14:01:16 <abhishekk> cool
14:01:33 <abhishekk> #topic release/periodic jobs update
14:01:51 <abhishekk> M2 is next week, so is our spec freeze
14:02:43 <abhishekk> I am planning to Tag M2 on 13th July
14:03:13 <abhishekk> Before that I/we need to verify config refresh patch and releasenotes are inline or not
14:03:43 <abhishekk> Periodic jobs all green \o/
14:03:48 <jokke_> nice
14:03:53 <abhishekk> M2 Target progress check
14:04:08 <rosmaita> we may need to review those jobs and make sure they are still testing something :)
14:04:08 <abhishekk> Quotas, everything is done from glance side
14:04:17 <abhishekk> :P
14:04:17 <jokke_> rosmaita: :P
14:05:08 <abhishekk> I think credit goes to dansmith for figuring out timeout issue, which was harassing us mostly during milestone releases
14:05:25 <abhishekk> Thank you all for your reviews
14:05:32 <rosmaita> hooray for dansmith !
14:05:40 * dansmith blushes
14:05:41 <abhishekk> We need to followup with tempest patches
14:05:48 <abhishekk> dansmith, ^^^
14:05:57 <dansmith> quota ones you mean
14:06:01 <abhishekk> yeah
14:06:07 <dansmith> this week has been crazy busy for me with other stuff,
14:06:17 <dansmith> and I don't normally harp on the tempest people until the patches are actually merge-able
14:06:26 <dansmith> but yeah I'll start working on that again tomorrow or next week
14:06:46 <abhishekk> ack, I can followup with them as well
14:06:49 <dansmith> mergeable meaning.. the feature has landed
14:07:02 <dansmith> I know the tempest test needs at least a unit test for the limits client I added
14:07:44 <abhishekk> right
14:07:53 <abhishekk> Cache API
14:08:10 <abhishekk> I think implementation is up, waiting on additional tests and doc changes
14:08:51 <abhishekk> If those are up till Monday EOD then we will consider it for M2 else we will shift it to M3
14:08:52 <dansmith> link?
14:08:53 <jokke_> yup, working on those and the client calls
14:09:09 <abhishekk> #link https://review.opendev.org/c/openstack/glance/+/792022
14:09:26 <abhishekk> Client changes will be done in M3 timeframe, need to merge client spec before next week
14:09:56 <dansmith> abhishekk: thanks
14:10:10 <abhishekk> client spec link #https://review.opendev.org/c/openstack/glance-specs/+/794709
14:10:38 <abhishekk> moving to next topic
14:10:55 <abhishekk> #topic Policy refactoring
14:11:29 <jokke_> abhishekk: I don't care when the client patch gets merged, but makes it easier for me to work on the docs and tests when I have the workflow sorted out and manually testable ;)
14:12:04 <abhishekk> jokke_, ack and agree
14:12:22 <abhishekk> Spec freeze is approaching and we have almost settled everything, just pending thing is nested check vs backward compatibility
14:13:10 <abhishekk> AFAIK, considering our default policies I am more than sure we are not breaking any backward compatibility by avoiding nested policy checks
14:14:16 <abhishekk> also there is no need to major version bump to avoid nested policy checks
14:14:24 <dansmith> also because the default policy that comes as a result of the refactor (i.e. replacing our existing "" with an actual rule) will keep the same default behavior
14:14:53 <abhishekk> Fact is our most of the policies are broken
14:14:54 <jokke_> I'm more worried about the fundamental behavioural change than what ever our default behaviour happens to be at any given time.
14:15:59 <jokke_> something that started as "We should refactor where our policies gets enforced" is turning proper hot mess changing the API behaviour is my problem with that proposal
14:16:16 <croelandt> do we really change the API though?
14:16:24 <dansmith> please specify what is changing exactly
14:16:25 <croelandt> like, we are not removing get_foo or what get_foo means
14:16:36 <dansmith> croelandt: exactly
14:16:39 <croelandt> I was slightly annoyed with this but dansmith's comment seemed reasonable to me
14:16:41 <abhishekk> correct
14:16:52 <croelandt> the changes seem to be "implementation details" rather than "contract with the API user"
14:17:15 <jokke_> we do. If you're able to change any part of the image as of now you can always verify that your change took effect. After the proposed change that is not the case anymore
14:17:30 <dansmith> croelandt: also, the policy-writing admin knowing that our internals check get before update is definitely requiring them to understand on and rely on internal implementation details,
14:17:39 <dansmith> whereas if each policy needs to be secure, it doesn't matter
14:18:10 <abhishekk> jokke_, could you please explore more on this?
14:18:12 <dansmith> jokke_: can you explain that in more detail? because I don't understand that
14:18:38 <croelandt> Do you mean you can be allowed to modify_image but not get_image
14:18:49 <croelandt> and therefore you might change an image but not print it to check it worked?
14:18:56 <abhishekk> Because I can see no use of blocking get call and giving access to update
14:19:17 <dansmith> ah, okay, but if we return 200 then the change was made, if you have to get it later to check, we're breaking our RESTfulness :)
14:20:12 <jokke_> as of now, to be able to do a chage, say in the image metadata, you need to have ability to get that image object as the get_ is prerequisite to do any writes. As proposed that would not be the case anymore leading to situations where people can blindly change things without being able to fetch that data to confirm it. I did walk through this already in the review
14:20:25 <dansmith> that's like saying that UNIX shouldn't support dropbox directories of -wx because you can't ls the directory to make sure the file was written
14:20:50 <croelandt> dansmith: but usually you have "r" if you have "w"
14:20:57 <croelandt> so it's more of a config issue on the user side
14:21:00 <dansmith> croelandt: not in a dropbox situation
14:21:17 <croelandt> hm
14:21:17 <dansmith> croelandt: but yes, by default and in most cases an operator would give people r and w at the same time
14:21:21 <rosmaita> but the new default will allow both GET and UPDATE, right?
14:21:22 <croelandt> ok
14:21:27 <croelandt> not sure I've ever done the dropbox thingy
14:21:34 <dansmith> croelandt: I'm just saying we don't need to force them to do that, like for the bot account cases I detailed earlier in the spec
14:21:42 <dansmith> rosmaita: yes
14:21:44 <abhishekk> rosmaita, not only the new defualt but the current one as well
14:21:45 <rosmaita> so the difference is that now an operator could do a weird policy config that they couldn't do before
14:22:06 <dansmith> croelandt: maybe you went to college too recently.. in my day, we submitted assignments on the department unix system in a dropbox directory :)
14:22:25 <dansmith> rosmaita: yeah, and I explained several legit reasons they might want to do that
14:22:44 <jokke_> dansmith: but we do not follow the unix permission model in a first place and we never have
14:22:46 <croelandt> dansmith: oh yeah yeah
14:22:48 <dansmith> rosmaita: the important thing I think, is that they also won't be able to configure wildly insecure rules for update that rely in "can see the image" as a gateway,
14:22:51 <croelandt> I've had students do taht!
14:22:57 <dansmith> rosmaita: which we already have some tests that do that :)
14:23:14 <rosmaita> dansmith: i am actually agreeing with you (i think)
14:23:16 <dansmith> jokke_: of course not, it's just a classic example of "writer-only" access patterns
14:23:20 <dansmith> rosmaita: yep
14:24:17 <abhishekk> I am totally failing to understand why someone will block get call if he gives aceess to delete,download,upload,update calls
14:24:44 <rosmaita> i guess my question is, there are probably weird behaviors configurable in policy now that don't make sense, too
14:24:51 <dansmith> abhishekk: my examples in the spec discussion earlier?
14:25:05 <rosmaita> so i'm not convinced this should be considered an API change
14:25:15 <abhishekk> dansmith, yep
14:25:17 <jokke_> abhishekk: well dansmith had couple of examples of usecases where that might be handy in the previous revision comments
14:25:43 <dansmith> rosmaita: for sure, but also most of the enforcement is hard-coded in python, which means people think they can actually control things in policy, but can't.. like most of the rules are technically "open to anyone" and just rely on hard-coded python in reality
14:26:00 <abhishekk> that is what our next topic is
14:26:16 <abhishekk> #link https://review.opendev.org/c/openstack/glance/+/798266
14:26:26 <abhishekk> Policy checks Vs Read Only checks
14:26:39 <abhishekk> This is the case with almost all metadef policies as well
14:27:06 <abhishekk> It passes the policy check but gets rejected at DB layer on visibility check
14:27:19 <dansmith> rosmaita: almost all of the existing policies resolve to this: https://github.com/openstack/glance/blob/master/glance/policies/base.py#L69
14:27:31 <dansmith> rosmaita: which means anyone can do anything
14:27:52 <dansmith> but of course, they can't because we have hard-coded behaviors that you can't override, calcified in the DB layers
14:28:10 <rosmaita> yeah, but with policy-in-code, "default" is never used anymore
14:28:22 <dansmith> rosmaita: eh? it's always used
14:28:36 <rosmaita> where?
14:29:04 <abhishekk> for each policy we are using rule:default
14:29:18 <abhishekk> s/each/most
14:29:26 <dansmith> https://github.com/openstack/glance/blob/master/glance/policies/image.py#L34
14:29:39 <dansmith> every one of those is using rule:default unless you have experimental RBAC turned on
14:29:45 <dansmith> but today, everyone has rule:default for most everything
14:30:04 <dansmith> abhishekk and I know this, because our tests are wildly wrong on most things :)
14:30:31 <abhishekk> yeah
14:31:03 * dansmith wonders if rosmaita's head just exploded
14:31:10 <rosmaita> i will shut up, but oslo policy used to (and maybe still does) recognize a special rule named 'default'
14:31:16 <abhishekk> So we can have/enforce get check at API level but frankly speaking it will be of no use
14:31:41 <dansmith> rosmaita: our default rule is defined with a check string of ""
14:32:15 <rosmaita> yeah, so i don't like that on 2 levels ... anyway, i will shut up as not being hip to glance policy changes since train
14:32:40 <abhishekk> Ok
14:33:58 <dansmith> hope that wasm
14:34:08 <dansmith> wasn't a rage quit
14:34:15 <jokke_> rosmaita: IIUC the 'default' policy came from oslo_policy as long as it wasn't specified. Which was obviously noticeable, say if you did not have policy file. Since the policy in code, we always overwrite it with empty checkstring, and that might not be so obvious, like everything else on this policy in code mess
14:34:28 <abhishekk> hopefully
14:34:28 <jokke_> oh, he dropped
14:35:04 <dansmith> yeah I dunno about any behavior of what happens if you don't specify a default rule,
14:35:11 <dansmith> but we do, and I'm sure it's getting honored
14:35:29 <jokke_> yup
14:35:30 <abhishekk> I think this is the right time to set everything in correct order
14:36:02 <dansmith> yeah, because this is confusing and hairy and so getting things straight while we all have context is a good plan
14:36:13 <abhishekk> agree
14:37:38 <rosmaita> sorry about that
14:37:39 <abhishekk> he is back :D
14:37:45 <dansmith> rosmaita: gave us a scare :)
14:38:02 <jokke_> rosmaita: wb, we were wondering if you had mic drop moment
14:38:05 <rosmaita> i blame bluejeans (because i am multitasking)
14:38:17 <abhishekk> aha
14:38:21 <jokke_> rosmaita: IIUC the 'default' policy came from oslo_policy as long as it wasn't specified. Which was obviously noticeable, say if you did not have policy file. Since the policy in code, we always overwrite it with empty checkstring, and that might not be so obvious, like everything else on this policy in code mess
14:38:41 <abhishekk> and we need multitasking badly :D
14:38:43 <jokke_> rosmaita: ^^ just replay of what I typed when you dropped
14:38:48 <rosmaita> yeah, i was talikng from old knowledge
14:39:03 <rosmaita> anyway, maybe it's too late now, but i would not advise having a rule named "default"
14:39:10 <dansmith> anyway, I do not thinking tying update and get policies together at the lower layers will do operators any favors. at best they limit what they can do, and at worst they encourage them to write insecure update policies banking on the get policy being enforced
14:39:39 <dansmith> if we have to to move forward, then we can, but I think it's detrimental to actually making this understandable from the outside
14:39:43 <rosmaita> i think we probably mostly agree on that?
14:40:01 <dansmith> you and I do
14:40:13 <abhishekk> agree on not having default?
14:40:41 <dansmith> I thought he meant "agree on not tying get and update together"
14:41:03 <abhishekk> I am onboard to that from beginning
14:41:24 <jokke_> yeah, if we were designing a new API I would be way easier convinced that it's reasonable approach to move forward. Not when it's breaking behaviour that has been established and enforced for, what, 7-8 years now
14:41:55 <dansmith> again, please specify the exact behavior that is brekaing
14:42:12 <abhishekk> with context to default policies today we have
14:42:36 <jokke_> dansmith: I've done it multiple times, most recently 22min ago
14:42:39 <rosmaita> ok, i am now in this meeting 100%
14:43:02 <croelandt> rosmaita: ... and it's over!
14:43:09 <rosmaita> i wish
14:43:32 <rosmaita> so jokke_, i don't see that there is an API guarantee about being able to use every call in the API
14:43:34 <abhishekk> jokke_, according to me we are not breaking anything if we consider our default policies as of today
14:44:00 <jokke_> abhishekk: defaults have nothing to do with it.
14:44:15 <abhishekk> Then ?
14:45:06 <rosmaita> jokke_: apologies for not having this already, but can you paste the link to the review where you already mentioned this?
14:45:17 <jokke_> abhishekk: how the API behaves under _different_ config situation and what the user can expect from it. I think we can all agree that our _default_ policies are not something anyone would run in production
14:45:20 <rosmaita> i think if i read your comment in context i will grasp your point better
14:46:00 <jokke_> #link https://review.opendev.org/c/openstack/glance-specs/+/796753/3/specs/xena/approved/glance/glance-policy-refactoring.rst
14:46:50 <rosmaita> ty
14:46:51 <dansmith> our API is super broken if everyone that does an image update has to get the image right after and verify that something took effect when we reported 200...
14:47:46 <jokke_> dansmith: our api is super broken also if anyone doing such change cannot do so
14:47:54 <jokke_> like they have for past years
14:48:19 <croelandt> dansmith: I'm the kind of guy who checks both ways when crossing a one-way street, and I must say I like to check everything like that :D
14:48:40 <dansmith> well, the HTTP contract with a 200 says "we did that thing".. there's no actual contract in HTTP says that you must be able to GET things you PUT
14:48:46 <jokke_> croelandt: I do that even before entering into rounabout
14:48:54 <jokke_> roundabout
14:49:13 <abhishekk> SO even if the road is one way ??
14:49:22 <dansmith> croelandt: sure, but telling the prof you want to be able to make sure other students' homework is correct too is not justification for having r-- access :)
14:49:37 <croelandt> dansmith: yeah :)
14:49:39 <dansmith> and in the case of security policy here, "rule of least privilege" IS the safer thing :)
14:49:47 <jokke_> dansmith: so you're perfectly fine us to change our API behaviour how ever we like as long as it doesn't break HTTP protocol?
14:49:59 <croelandt> I mean, worst case scenario, users will complain and the admin will give them read permission on their own images, right?
14:50:02 <rosmaita> jokke_: i still don't see this as a change in api behavior
14:50:17 <abhishekk> exactly
14:50:17 <dansmith> jokke_: I'm perfectly fine not synthesizing behavioral promises that don't exist :)
14:50:18 <rosmaita> it's a change in what can be configured via policy
14:51:18 <dansmith> croelandt: it's very unlikely this would even be a thing for owned images, and definitely not for actual human users.. this whole argument is rather contrived in when or how it would ever play out except for specific circumstances like I described
14:51:44 <dansmith> rosmaita: right and if anything, it *appears* like we've allowed you to configure this for years, we just don't actually honor the policy in any meaningfulway. So it's really a bugfix :)
14:51:59 <abhishekk> Even if we consider update situation that we should enforce get check, what is the need for similar check on delete/download/upload calls
14:52:36 <dansmith> a garbage-collecting bot able to delete images but not see the license keys in their metadata is a clear example of why I should be able to grant only delete to a special service account
14:53:17 <dansmith> like a bot that deletes resources from an employee that has left should not need global permissions on the entire cloud to read anything, it only needs to delete things it is told to delete
14:53:30 <dansmith> otherwise that bot's account becomes a huge target because it's so powerul
14:53:32 <dansmith> *powerful
14:53:42 <abhishekk> Last 7 minutes
14:53:57 <abhishekk> I would suggest everyone should review new revision and vote accordingly
14:54:16 <jokke_> dansmith: that bot example is very much the reason why we have property protections
14:54:54 <jokke_> so you don't expose your sensitive information through the metadata
14:55:03 <dansmith> jokke_: when PP was added, was that considered a breaking API change?
14:55:15 <abhishekk> I am also considering this for FFE as I don't want to miss this opportunity to correct the things
14:55:34 <jokke_> abhishekk: :P
14:55:43 <abhishekk> As I said if it doesn't happen now, it will never happen again
14:55:51 <dansmith> yep,
14:56:01 <dansmith> and we're really already very short on time and have a LONG road ahead
14:56:18 <abhishekk> yes
14:56:20 <rosmaita> ok, i have not reviewed the latest patch set on the sepc, will do so right now
14:56:35 <abhishekk> ack, thank you
14:56:42 <abhishekk> That's it from me today
14:56:47 <abhishekk> Moving to open discussion
14:56:53 <abhishekk> #topic Open discussion
14:57:12 <abhishekk> Nothing from me
14:57:32 <rosmaita> ok, i definitely have jokke_'s concerns in mind and they are clearly stated in gerrit, so i will think carefully about them while reviewing
14:57:47 <abhishekk> ack
14:58:53 <jokke_> I have nothing for open, will keep grinding the cache api stuff and the swift driver bugfixes
14:58:58 <abhishekk> I am exploring metadef
14:59:14 <abhishekk> Sounds like our command line need more helpful/meaningful messages
14:59:39 <abhishekk> Time to wrap up
14:59:43 <abhishekk> thank you all
14:59:49 <jokke_> TY
14:59:54 <abhishekk> have a nice weekend o/~
15:00:02 <rosmaita> thanks!
15:00:02 <abhishekk> #endmeeting