21:02:04 <timburke> #startmeeting swift
21:02:04 <opendevmeet> Meeting started Wed Dec 11 21:02:04 2024 UTC and is due to finish in 60 minutes.  The chair is timburke. Information about MeetBot at http://wiki.debian.org/MeetBot.
21:02:04 <opendevmeet> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
21:02:04 <opendevmeet> The meeting name has been set to 'swift'
21:02:12 <timburke> who's here for the swift meeting?
21:02:18 <acoles> o/
21:04:17 <timburke> i had some trouble updating the agenda (wiki would let me log in, but then immediately forget who i was the next time i loaded a page) but i've got some notes for myself :-)
21:04:20 <timburke> first up
21:04:25 <mattoliver> o/
21:04:29 <timburke> #topic https://bugs.launchpad.net/swift/+bug/2091124
21:04:30 <patch-bot> Bug #2091124 - Use of eval() on untrusted data (In Progress)
21:04:51 <timburke> this was a fun bug i found last week -- as far as i can tell, the xprofile middleware has allowed remote code execution ever since it was introduced in swift 2.0.0
21:04:58 <timburke> fortunately, the fix was simple
21:05:04 <timburke> #link https://review.opendev.org/c/openstack/swift/+/937493
21:05:05 <patch-bot> patch 937493 - swift - xprofile: Stop using eval() (MERGED) - 1 patch set
21:05:16 <timburke> and backports have been proposed (and merged) for all stable branches
21:05:26 <timburke> additionally, i proposed doc changes to make it clear that xprofile is for testing only, and not intended for production
21:05:31 <timburke> #link https://review.opendev.org/c/openstack/swift/+/937494
21:05:31 <patch-bot> patch 937494 - swift - docs: Call out that xprofile is not intended for p... (MERGED) - 1 patch set
21:05:46 <timburke> and another change to flip our bandit config from "run these tests" to "skip these tests"
21:05:51 <timburke> #link https://review.opendev.org/c/openstack/swift/+/937495
21:05:51 <patch-bot> patch 937495 - swift - CI: Configure bandit better - 1 patch set
21:06:29 <timburke> i don't know that there's a lot to discuss; mostly i just wanted to raise awareness in case anyone missed the bug
21:06:44 <timburke> thanks for all the reviews mattoliver!
21:07:18 <timburke> i should also try to get a release together, but knowing my history, it seems unlikely i'll get to it before we hit the holiday lull
21:07:25 <mattoliver> nps, good find!
21:07:30 <mattoliver> tim
21:08:10 <acoles> in https://review.opendev.org/c/openstack/swift/+/937495 is the diskfile assert change related to the bandit change?
21:08:10 <patch-bot> patch 937495 - swift - CI: Configure bandit better - 1 patch set
21:09:10 <timburke> oh, yes! bandit will flag asserts, since they can become passes if running with python -O
21:09:41 <timburke> i suppose an alternative would be to break that part out as a separate follow-on
21:10:35 <acoles> OIC and I guess before we only opted in to tests and presumably the test for assert wasn't an opt in before, but now we opt out we're getting dinged
21:10:52 <timburke> yup :-(
21:11:02 <timburke> speaking of things i discovered in this week...
21:11:06 <timburke> #topic eventlet and underscores in headers
21:11:11 <acoles> I might prefer it to be a follow-up? IDK
21:11:17 <timburke> can do
21:11:22 <timburke> recent versions of eventlet ignore headers with underscores in their names when building the request environment
21:11:32 <timburke> #link https://github.com/eventlet/eventlet/commit/ed743d75
21:11:43 <timburke> this caused breakage with some tempest tests that were sending headers like x-container-meta-foo_bar: baz
21:11:51 <timburke> tempest should be fixed with p 937416, but heads-up: other clients may be affected
21:11:52 <patch-bot> https://review.opendev.org/c/openstack/tempest/+/937416 - tempest - Stop sending underscores in header names to Swift - 3 patch sets
21:12:14 <timburke> now i've got a few questions:
21:12:22 <timburke> (1) should we have a similar sort of a change in python-swiftclient, to .replace('_', '-') on user-provided headers/meta?
21:12:31 <timburke> (2) should we write up a bug eventlet about the change impacting behaviors that have nothing to do with message framing?
21:12:42 <timburke> and (3) should we have a fix server-side to do a similar translation?
21:12:47 <timburke> since we've still got access to environ['headers_raw'] which has *all* the headers
21:13:44 <mattoliver> Oh geesh, big questions
21:14:33 <timburke> i'm kinda torn -- it's definitely a breaking behavior for us (as the tempest failures demonstrate), but we've also never (to my knowledge) endorsed underscores in headers, and the translation has just been an artifact of WSGI/CGI
21:15:16 <acoles> quick search suggests underscores in headers are allowed, so what motivated eventlet?
21:15:33 <timburke> something something security, i think?
21:16:00 <timburke> there's a little more conversaion in
21:16:02 <timburke> #link https://github.com/eventlet/eventlet/pull/959
21:17:26 <timburke> my initial reaction over in #openstack-oslo was "idk why CONTENT_LENGTH vs Content-Length should be such a worry when there are so many other fun ways to break header parsing: https://github.com/python/cpython/issues/81274" :P
21:19:00 <timburke> i think we could probably scope any potential server fix to *just* look at a/c/o user meta
21:19:54 <timburke> ...but at the same time, i don't actually *want* to add this sort of a hack, and i think we can get away with it because *who sends underscores in headers!?*
21:20:10 <acoles> I guess headers are mapped to '_' delimited keys in the env so there's an ambiguity ? 🤷
21:20:26 <timburke> yup
21:20:29 <acoles> i.e. it's a wsgi thing
21:21:05 <timburke> in the wsgi env there's no difference between content-length, Content-Length, and CONTENT_LENGTH on the wire
21:21:52 <timburke> ("who does this" -- well, aside from some s3 client that prompted me to write this mess: https://opendev.org/x/swift3/commit/f3a933aa )
21:22:07 <acoles> yeah, so if you previously sent swift foo_bar we'd accept it same as foo-bar, which is an argument for us on server-side continuing t do the same with no more ambiguity than before
21:22:32 <opendevreview> Merged openstack/swift master: Add unit test for object-updater recon dump  https://review.opendev.org/c/openstack/swift/+/937252
21:23:03 <acoles> someone round here often extos us to be "tolerant in what we accept, strict in what we send"
21:23:10 <acoles> extols*
21:23:55 <timburke> but there's a limit once we start worrying about CVE surface area
21:24:05 <acoles> but does a server-side fix mean us overriding something in eventlet wsgi?
21:24:35 <acoles> yes, not so tolerant as to accept executable scripts
21:24:37 <timburke> depends on where/when we do it
21:26:19 <mattoliver> i dunno, if the http spec says w _'s are a valid char in headers, we should be supporting them.. but I don't like horrible hacks.
21:26:44 <mattoliver> ok, no idea where that extra 'w' came from :P
21:27:04 <mattoliver> I think this is something I need to think more about
21:28:27 <timburke> fwiw, we have some experience in needing to do something like this: https://github.com/openstack/swift/blob/2.34.0/swift/common/http_protocol.py#L192
21:29:07 <timburke> yeah, we can think more about it -- a reasonable stance might be just "wait and see who all notices"
21:29:25 <timburke> #topic constraining deps in the gate
21:29:29 <acoles> important to note that whilst _ may be valid in an http header, it will not be distinguished from '-' in a wsgi app (IIUC)
21:29:42 <timburke> yup
21:29:44 <acoles> which is different to "case insensitive"
21:29:58 <timburke> to follow-on from eventlet breaking our CORS tests (which i mentioned last week)
21:29:59 <timburke> p 936872 has now merged
21:29:59 <patch-bot> https://review.opendev.org/c/openstack/swift/+/936872 - swift - CI: drop pip --upgrade flag in tox.ini (MERGED) - 1 patch set
21:30:08 <timburke> but p 937045 sent me down a bit of a rabbit hole (more in a bit)
21:30:09 <patch-bot> https://review.opendev.org/c/openstack/swift/+/937045 - swift - CI: Use constraints for api-ref builds - 1 patch set
21:30:25 <timburke> and now in a separate chain, we've got p 937072 and p 937073 to bring our py3-constraints.txt more in line with the global upper-constraints.txt file
21:30:25 <patch-bot> https://review.opendev.org/c/openstack/swift/+/937072 - swift - Bring py3-constraints.txt more in line with global... - 2 patch sets
21:30:26 <patch-bot> https://review.opendev.org/c/openstack/swift/+/937073 - swift - Bring py3-constraints.txt more in line with global... - 2 patch sets
21:30:50 <acoles> timburke: has been doing CI house keeping 🙏
21:31:06 <timburke> there were some up-revs (-72) and down-revs (-73) required, which was why i split up the changes
21:32:18 <timburke> i also wrote up a tool for myself to help notice discrepancies; i figure i ought to just aim to run it periodically, since the requirements repo sees a good bit of churn
21:32:43 <timburke> but i still need to write a patch to add a (non-voting) job that runs without any constraints file at all
21:33:14 <timburke> (though i'm not at all sure which version of python that ought to run under...)
21:33:52 <timburke> speaking of
21:33:59 <acoles> hmmm, maybe the oldest??
21:34:06 <timburke> #topic "supported" python versions
21:34:12 <timburke> what do we (intend to) mean when we want to say "we support python version X.Y"?
21:34:16 <timburke> and what kind of testing matrix do we want/need to keep us honest?
21:34:36 <timburke> a few different things prompt my question:
21:34:44 <acoles> we intend to mean that X!=2 :D
21:34:44 <timburke> acoles noticed recently that `tox -e pep8` no longer works on py38 (his default)
21:35:00 <timburke> i noticed that currently, i can build docs without including https://github.com/openstack/swift/blob/2.34.0/tox.ini#L124 -- but i still need it (at least) to build docs on py2
21:35:53 <timburke> (and i have no sense for whether doc building works on any other pythons besides py310 [which is used in the gate] and py312 [which i use locally])
21:36:06 <timburke> (this is that rabbit hole i fell down)
21:36:17 <timburke> and acoles also noticed that a sizeable portion of our s3api func tests are skipped in the gate because
21:36:22 <timburke> (1) i made us skip boto tests if it can't be imported (on the assumption that it wasn't installed -- see p 918144),
21:36:22 <patch-bot> https://review.opendev.org/c/openstack/swift/+/918144 - swift - Skip boto 2.x tests if boto is not installed (MERGED) - 1 patch set
21:36:29 <timburke> (2) boto does not work on py312 (raising an import error),
21:36:35 <timburke> and (3) all of our voting func test jobs now run on py312
21:37:01 <timburke> so: how much of our tox envs should still be expected to be able to run under a given python version?
21:37:35 <mattoliver> i use py39 everywhere, for some reasons my system python is 312
21:37:55 <mattoliver> sorry on my phone (dealing with kids) so typing is hard :P
21:38:09 <acoles> FYI i just kicked off tox -e docs on py38 and it fails because upper constraints isn't compatible with py38
21:38:28 <acoles> I tend to just run the sphinx build command to build docs
21:38:39 <timburke> that seems about right :-/
21:39:10 <timburke> does it help any to set TOX_CONSTRIANTS_FILE=py3-constraints.txt ?
21:40:12 <timburke> i don't think the docs-only deps are anywhere in there, so it'd be "unconstrained" for those
21:42:16 <acoles> timburke: no and nor does TOX_CONSTRAINTS_FILE=py3-constraints.txt 🤔
21:42:40 <timburke> boo!
21:43:57 <acoles> oh but I still see 'docs: install_deps> pip install -U -r /vagrant/swift/doc/requirements.txt -r /vagrant/swift/requirements.txt -c https://releases.openstack.org/constraints/upper/master'
21:44:54 <timburke> oh! maybe try UPPER_CONSTRAINTS_FILE -- i merged something recently to standardize on the other
21:46:36 <timburke> well, are we content to say that tox -e docs,pep8 requires py39+ (which would match PTI)? but they're not necessarily expected to work on earlier pythons?
21:47:46 <acoles> yeah that's working, which is odd because I was about to say don't we need to add it to ``passenv``
21:48:33 <timburke> i think the trick is that tox uses it, but it's not available to pip/pytest/etc
21:48:36 <acoles> or is passenv for vars passed to the venv, not tox
21:48:45 <timburke> bingo
21:49:03 <timburke> i feel like what i want is to say "we expect to be able to run a prod system on python X.Y and will be responsive to reports of breakage"
21:49:05 <timburke> and a direct follow-on from there is that we should expect unit, func, and probe tests to all pass on pyXY
21:49:39 <acoles> "unit, func, and probe tests to all pass on pyXY" yes, absolutely
21:49:41 <timburke> but i'm not clear on where that leaves doc builds, flake8, or even s3 cross-compat tests
21:50:37 <acoles> I think it is unfortunate but perhaps acceptable for pep8 and docs to require >=3.9 and document the workaround for <3.9
21:51:00 <timburke> 👍
21:51:15 <acoles> cross-compat tests I'd like to include under "func tests"
21:52:16 <acoles> I mean, we could even have a different pep8-alt tox env just for me and other py3.7/3.8 freaks
21:52:51 <timburke> i'm torn -- i don't know that i'd -1 a change that wanted to start using f-strings in cross compat tests, say -- the significant thing for declaring support is the python used on the server, not the test runner
21:53:05 <acoles> but I can live with the env var trick, especially now it is in my command history
21:53:22 <timburke> after all, what version of python does AWS run? ;-P
21:54:03 <timburke> all right, we're running a little low on time
21:54:08 <timburke> #topic open discussion
21:54:16 <timburke> anything else we ought to bring up this week?
21:54:25 <mattoliver> hmm, I'm getting an openstackdocstheme.page_context error now that I try and build docs on py39.. but might be my env (its an old one).. I'll play with it in my own time :)
21:54:46 <timburke> thanks for the data point mattoliver!
21:54:52 <acoles> "the significant thing for declaring support is the python used on the server, not the test runner" - agree, I misunderstood. But can we configure a zuul job that runs a py3.7 service and test runner is py3.12?
21:55:35 <acoles> https://www.irccloud.com/pastebin/e8UeXpe8/
21:56:03 <acoles> oops, not sure how I posted that as a snippet
21:56:37 <acoles> that's how my tox -e docs just finished, I remember now that even after persuading it to build a venv under py38 it bombed out at the end :(
21:57:19 <mattoliver> Umm I have reworked the sharder rsync_then_merge patch. https://review.opendev.org/c/openstack/swift/+/901095
21:57:19 <patch-bot> patch 901095 - swift - db_replication: big containers that haven't sunk s... - 2 patch sets
21:57:57 <timburke> "can we configure a zuul job that runs a py3.7 service and test runner is py3.12?" we can, though it isn't necessarily simple. we do something like that with the rolling upgrade jobs, which are multi-node
21:58:08 <mattoliver> There is another option I'm playing with where we only ever rsync in the sharder.. but I'm worried that'll make any small handoff always fall back to rsync_then_merge and that could be quite expensive (as rsync it to tmp, and them locally merges all the old shard into the new one (with on a row or 2) and then copies it to the location.
21:58:09 <timburke> yeah! i need to take a look at that, mattoliver!
21:58:43 <acoles> thanks timburke for figuring out all the tox/zuul stuff!
21:59:39 <mattoliver> yeah +1
21:59:54 <mattoliver> anyway I think we're at time. Sorry was a bit distracted with kids this meeting.
22:00:00 <timburke> no worries
22:00:17 <timburke> thank you all for coming, and thank you for working on swift!
22:00:21 <timburke> #endmeeting