| opendevreview | Hervé Beraud proposed openstack/oslo.rootwrap master: DNM test https://review.opendev.org/c/openstack/oslo.rootwrap/+/896354 | 08:37 |
|---|---|---|
| JayF | Working on heat test failures for the oslo.messaging bump. | 16:48 |
| JayF | reverting my security fix does cause them to pass, so that's the root cause, going to see if I can shake out why | 16:48 |
| JayF | if not, how would you all feel about a rewrite of that fix in the way I originally did: explicitly blank sensitive values -- in an attempt to get us something backportable | 16:49 |
| stephenfin | JayF: If it's not gross looking and won't cause us hassle down the line, I have no objections to anything that makes your life easier | 16:58 |
| JayF | Honestly, the thing that may make my life easier would be someone being willing to rubber duck this with me if I get stuck | 16:58 |
| JayF | I've confirmed already it's my sanitization change that breaks it | 16:58 |
| JayF | I am not sure but I think it's got to do with osprofiler; osprofiler hooks in via notifications | 16:59 |
| JayF | and given the way the tests are hanging, I suspect something around osprofiler freaking out when it gets a serialized copy of a different context | 16:59 |
| JayF | but I am not sure and am mainly troubleshooting thru changing oslo messaging code and seeing how the unit tests change (if they do) | 16:59 |
| JayF | if I replace _sanitize_context with > return ctxt.__class__.from_dict(ctxt.__class__.to_dict(ctxt)) | 17:09 |
| JayF | it passes | 17:09 |
| JayF | so it's not even angry about it being a different context object; it's doing something with the context being passed from the notification and hanging afterwards | 17:09 |
| JayF | oh wow I got it, this is crazy | 17:20 |
| JayF | https://github.com/openstack/heat/blob/master/heat/common/context.py#L120 | 17:20 |
| JayF | we filter is_admin | 17:20 |
| JayF | and as a result it does a roundtrip to policy, using a context that's already been stripped of anything that makes sense of policy | 17:21 |
| JayF | that's how we end up doing work with a notification-cleaned context | 17:21 |
| JayF | I believe I shuold be able to work around this by adding is_admin to the safe list | 17:21 |
| * JayF crosses fingers | 17:22 | |
| JayF | bingo | 17:24 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging master: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896451 | 17:30 |
| JayF | stephenfin: ^ | 17:30 |
| JayF | stephenfin: we need to land that, release it as 14.4.1, then bump https://review.opendev.org/c/openstack/requirements/+/892919 | 17:30 |
| JayF | https://bugs.launchpad.net/oslo.messaging/+bug/2037312 | 17:32 |
| JayF | hberaud: frickler: Please take notice of the oslo.messaging fix above ^ I believe that should be the last step in getting an oslo.messaging that's happy with all 2023.2 projects and able to be released for a security fix | 17:33 |
| JayF | hberaud: frickler: https://review.opendev.org/c/openstack/oslo.messaging/+/896451 all details, including a link to the bug and the cause, are indicated in there | 17:33 |
| opendevreview | Jay Faulkner proposed openstack/oslo.context master: Add is_admin to default version of redacted_context https://review.opendev.org/c/openstack/oslo.context/+/896453 | 17:37 |
| JayF | (that will prevent a similar bug when transitioning to the better way to do this in caracal) | 17:38 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging stable/2023.2: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896422 | 17:39 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging stable/2023.1: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896423 | 17:40 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging stable/zed: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896424 | 17:40 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging stable/yoga: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896425 | 17:40 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging stable/xena: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896426 | 17:40 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging master: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896451 | 17:50 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging master: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896451 | 17:51 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging stable/2023.2: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896422 | 17:51 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging stable/2023.1: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896423 | 17:52 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging stable/zed: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896424 | 17:52 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging stable/yoga: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896425 | 17:52 |
| opendevreview | Jay Faulkner proposed openstack/oslo.messaging stable/xena: Add is_admin to safe fields list for notifications https://review.opendev.org/c/openstack/oslo.messaging/+/896426 | 17:53 |
| JayF | frickler: hberaud: Updated as per code review feedback and re-backported thru. (I'm early-cherry-picking just to try and reduce latency/back and forth since this fix has taken a while) | 17:53 |
| frickler | JayF: thx, I'm really glad that the fix is so simple, looking at the test failures I had feared something much worse happening | 18:01 |
| JayF | I was mostly terrified the entire time | 18:01 |
| JayF | LOL | 18:01 |
| JayF | My biggest fear was that it was just straight-up broken with osprofiler, but nope, just some cleverness in __init__ | 18:02 |
| frickler | also, if you have the fix in oslo.context in master, would it be enough to do the messaging fix as stable-only? | 18:03 |
| JayF | Here's sorta how I am thinking about it, logically? | 18:03 |
| JayF | the state of messaging master is "messaging redacts the context" | 18:03 |
| JayF | that is broken; so fixing it is a unit of work | 18:04 |
| JayF | a separate parallel unit of work is taking out that piece and replacing it with calls to oslo.context (which, at this point, needs a merge and re-release to prevent this bug too when we switch to it, see https://review.opendev.org/c/openstack/oslo.context/+/896453 ) | 18:04 |
| JayF | So I would rather treat it as two separate efforts because there is less between us and "fixed" if we just fix master+backport, then replace the implementation in master | 18:05 |
| JayF | I can do it the other way (just pull master forward to using cxt.redacted_copy()) but that's going to take a bit longer to land, and I don't like messaging master being broken that long | 18:05 |
| JayF | just my $.02 | 18:05 |
| JayF | TBH I'm not super familiar with like ... how you all approach this kind of stuff in oslo; I will likely be working more in these common areas as time moves on so if that approach is like ... incompatible with how you usually handle that stuff, I'd love to learn | 18:06 |
| opendevreview | Jay Faulkner proposed openstack/oslo.context master: Add is_admin to default version of redacted_context https://review.opendev.org/c/openstack/oslo.context/+/896453 | 18:09 |
| frickler | note that I'm only an outsider myself, I just mention this because this seems to be similar to what happened for oslo.db. the fixes were merged into stable/2023.2 and a new release was cut there to fix bobcat, while master stayed untouched, ready to make further progress towards sqla2 | 18:09 |
| JayF | frickler: yeah, in that case oslo.db master was significantly different than 2023.2; in this case they are literally identical except for the perfunctory patches landed when a branch is cut | 18:09 |
| JayF | (and it's not a "move backwards" in the same way a revert is; it's just a straight moving-forward fix) | 18:10 |
| frickler | I think the fwd vs. reverse thing is a good point, ack, so I rest my case on that ;) | 18:12 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!