21:02:04 #startmeeting swift 21:02:04 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 Useful Commands: #action #agreed #help #info #idea #link #topic #startvote. 21:02:04 The meeting name has been set to 'swift' 21:02:12 who's here for the swift meeting? 21:02:18 o/ 21:04:17 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 first up 21:04:25 o/ 21:04:29 #topic https://bugs.launchpad.net/swift/+bug/2091124 21:04:30 Bug #2091124 - Use of eval() on untrusted data (In Progress) 21:04:51 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 fortunately, the fix was simple 21:05:04 #link https://review.opendev.org/c/openstack/swift/+/937493 21:05:05 patch 937493 - swift - xprofile: Stop using eval() (MERGED) - 1 patch set 21:05:16 and backports have been proposed (and merged) for all stable branches 21:05:26 additionally, i proposed doc changes to make it clear that xprofile is for testing only, and not intended for production 21:05:31 #link https://review.opendev.org/c/openstack/swift/+/937494 21:05:31 patch 937494 - swift - docs: Call out that xprofile is not intended for p... (MERGED) - 1 patch set 21:05:46 and another change to flip our bandit config from "run these tests" to "skip these tests" 21:05:51 #link https://review.opendev.org/c/openstack/swift/+/937495 21:05:51 patch 937495 - swift - CI: Configure bandit better - 1 patch set 21:06:29 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 thanks for all the reviews mattoliver! 21:07:18 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 nps, good find! 21:07:30 tim 21:08:10 in https://review.opendev.org/c/openstack/swift/+/937495 is the diskfile assert change related to the bandit change? 21:08:10 patch 937495 - swift - CI: Configure bandit better - 1 patch set 21:09:10 oh, yes! bandit will flag asserts, since they can become passes if running with python -O 21:09:41 i suppose an alternative would be to break that part out as a separate follow-on 21:10:35 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 yup :-( 21:11:02 speaking of things i discovered in this week... 21:11:06 #topic eventlet and underscores in headers 21:11:11 I might prefer it to be a follow-up? IDK 21:11:17 can do 21:11:22 recent versions of eventlet ignore headers with underscores in their names when building the request environment 21:11:32 #link https://github.com/eventlet/eventlet/commit/ed743d75 21:11:43 this caused breakage with some tempest tests that were sending headers like x-container-meta-foo_bar: baz 21:11:51 tempest should be fixed with p 937416, but heads-up: other clients may be affected 21:11:52 https://review.opendev.org/c/openstack/tempest/+/937416 - tempest - Stop sending underscores in header names to Swift - 3 patch sets 21:12:14 now i've got a few questions: 21:12:22 (1) should we have a similar sort of a change in python-swiftclient, to .replace('_', '-') on user-provided headers/meta? 21:12:31 (2) should we write up a bug eventlet about the change impacting behaviors that have nothing to do with message framing? 21:12:42 and (3) should we have a fix server-side to do a similar translation? 21:12:47 since we've still got access to environ['headers_raw'] which has *all* the headers 21:13:44 Oh geesh, big questions 21:14:33 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 quick search suggests underscores in headers are allowed, so what motivated eventlet? 21:15:33 something something security, i think? 21:16:00 there's a little more conversaion in 21:16:02 #link https://github.com/eventlet/eventlet/pull/959 21:17:26 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 i think we could probably scope any potential server fix to *just* look at a/c/o user meta 21:19:54 ...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 I guess headers are mapped to '_' delimited keys in the env so there's an ambiguity ? 🤷 21:20:26 yup 21:20:29 i.e. it's a wsgi thing 21:21:05 in the wsgi env there's no difference between content-length, Content-Length, and CONTENT_LENGTH on the wire 21:21:52 ("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 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 Merged openstack/swift master: Add unit test for object-updater recon dump https://review.opendev.org/c/openstack/swift/+/937252 21:23:03 someone round here often extos us to be "tolerant in what we accept, strict in what we send" 21:23:10 extols* 21:23:55 but there's a limit once we start worrying about CVE surface area 21:24:05 but does a server-side fix mean us overriding something in eventlet wsgi? 21:24:35 yes, not so tolerant as to accept executable scripts 21:24:37 depends on where/when we do it 21:26:19 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 ok, no idea where that extra 'w' came from :P 21:27:04 I think this is something I need to think more about 21:28:27 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 yeah, we can think more about it -- a reasonable stance might be just "wait and see who all notices" 21:29:25 #topic constraining deps in the gate 21:29:29 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 yup 21:29:44 which is different to "case insensitive" 21:29:58 to follow-on from eventlet breaking our CORS tests (which i mentioned last week) 21:29:59 p 936872 has now merged 21:29:59 https://review.opendev.org/c/openstack/swift/+/936872 - swift - CI: drop pip --upgrade flag in tox.ini (MERGED) - 1 patch set 21:30:08 but p 937045 sent me down a bit of a rabbit hole (more in a bit) 21:30:09 https://review.opendev.org/c/openstack/swift/+/937045 - swift - CI: Use constraints for api-ref builds - 1 patch set 21:30:25 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 https://review.opendev.org/c/openstack/swift/+/937072 - swift - Bring py3-constraints.txt more in line with global... - 2 patch sets 21:30:26 https://review.opendev.org/c/openstack/swift/+/937073 - swift - Bring py3-constraints.txt more in line with global... - 2 patch sets 21:30:50 timburke: has been doing CI house keeping 🙏 21:31:06 there were some up-revs (-72) and down-revs (-73) required, which was why i split up the changes 21:32:18 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 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 (though i'm not at all sure which version of python that ought to run under...) 21:33:52 speaking of 21:33:59 hmmm, maybe the oldest?? 21:34:06 #topic "supported" python versions 21:34:12 what do we (intend to) mean when we want to say "we support python version X.Y"? 21:34:16 and what kind of testing matrix do we want/need to keep us honest? 21:34:36 a few different things prompt my question: 21:34:44 we intend to mean that X!=2 :D 21:34:44 acoles noticed recently that `tox -e pep8` no longer works on py38 (his default) 21:35:00 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 (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 (this is that rabbit hole i fell down) 21:36:17 and acoles also noticed that a sizeable portion of our s3api func tests are skipped in the gate because 21:36:22 (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 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 (2) boto does not work on py312 (raising an import error), 21:36:35 and (3) all of our voting func test jobs now run on py312 21:37:01 so: how much of our tox envs should still be expected to be able to run under a given python version? 21:37:35 i use py39 everywhere, for some reasons my system python is 312 21:37:55 sorry on my phone (dealing with kids) so typing is hard :P 21:38:09 FYI i just kicked off tox -e docs on py38 and it fails because upper constraints isn't compatible with py38 21:38:28 I tend to just run the sphinx build command to build docs 21:38:39 that seems about right :-/ 21:39:10 does it help any to set TOX_CONSTRIANTS_FILE=py3-constraints.txt ? 21:40:12 i don't think the docs-only deps are anywhere in there, so it'd be "unconstrained" for those 21:42:16 timburke: no and nor does TOX_CONSTRAINTS_FILE=py3-constraints.txt 🤔 21:42:40 boo! 21:43:57 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 oh! maybe try UPPER_CONSTRAINTS_FILE -- i merged something recently to standardize on the other 21:46:36 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 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 i think the trick is that tox uses it, but it's not available to pip/pytest/etc 21:48:36 or is passenv for vars passed to the venv, not tox 21:48:45 bingo 21:49:03 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 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 "unit, func, and probe tests to all pass on pyXY" yes, absolutely 21:49:41 but i'm not clear on where that leaves doc builds, flake8, or even s3 cross-compat tests 21:50:37 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 👍 21:51:15 cross-compat tests I'd like to include under "func tests" 21:52:16 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 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 but I can live with the env var trick, especially now it is in my command history 21:53:22 after all, what version of python does AWS run? ;-P 21:54:03 all right, we're running a little low on time 21:54:08 #topic open discussion 21:54:16 anything else we ought to bring up this week? 21:54:25 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 thanks for the data point mattoliver! 21:54:52 "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 https://www.irccloud.com/pastebin/e8UeXpe8/ 21:56:03 oops, not sure how I posted that as a snippet 21:56:37 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 Umm I have reworked the sharder rsync_then_merge patch. https://review.opendev.org/c/openstack/swift/+/901095 21:57:19 patch 901095 - swift - db_replication: big containers that haven't sunk s... - 2 patch sets 21:57:57 "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 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 yeah! i need to take a look at that, mattoliver! 21:58:43 thanks timburke for figuring out all the tox/zuul stuff! 21:59:39 yeah +1 21:59:54 anyway I think we're at time. Sorry was a bit distracted with kids this meeting. 22:00:00 no worries 22:00:17 thank you all for coming, and thank you for working on swift! 22:00:21 #endmeeting