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