fix: pass the endpoint name through to relation-get#2499
Conversation
james-garner-canonical
left a comment
james-garner-canonical
left a comment
There was a problem hiding this comment.
Design question: how about making the new ops-level arguments keyword only? How about using the Juju name endpoint instead of relation_name?
Also some non-blocking questions about the tests and harness update. I suspect I'm just a bit confused due to being in unfamiliar territory, but figured it's worth raising the questions anyway.
| rel_id = harness.add_relation('db1', 'remoteapp1') | ||
| harness.add_relation_unit(rel_id, 'remoteapp1/0') | ||
| with harness._event_context('foo_event'): | ||
| harness.update_relation_data(rel_id, 'remoteapp1', {'secret': 'cafedeadbeef'}) |
There was a problem hiding this comment.
Seems like an odd example given that relation data shouldn't contain secrets. The 'leak' description above feels a bit odd too -- isn't the mismatched endpoint name handling behaviour more about modeling Juju cleanly than about security? After all, a user in the charm context can just query Juju directly with a subprocess.run if it really wants the data, can't they?
There was a problem hiding this comment.
It comes from the original issue/repro. It's not leaking anything the charm as a whole couldn't get, Juju handles that. But one piece of code could get data it shouldn't have. I'll try to make it clearer.
Co-authored-by: James Garner <james.garner@canonical.com>
Use self.id instead of undefined relation_id, and pass relation_name as a keyword argument since it's keyword-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Make new relation_name argument keyword-only across backend methods (ops, harness, scenario mocking). Pass relation_name as keyword at remaining call sites. Reword test_get_relation_wrong_endpoint and the relation-with-endpoint hookcmd tests to frame the behaviour as modelling Juju rather than preventing a leak, and to clarify that only the happy path is exercised. Add an example transformation to the test/bin/relation-list comment.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 participants
This PR passes the endpoint name from the Relation down to the actual hook command, where Juju will use it to validate that it matches the provided relation ID. Without this, you can trick Ops into a mismatch between ID and name and get misleading data (although Juju enforces security, so there is no leakage of data that you couldn't get anyway).
This is mostly just plumbing the extra information through the backend and into the hookcmds call from the various places we call this command. There are also a bunch of test updates to include the relation name to match.
Scenario is updated to reflect the Juju behaviour more closely, and otherwise expect / accept the name in calls.
I've updated Harness to match, even though we are not typically doing Harness fixes, because the signature changes and it seems less likely we would break anyone's existing Harness tests this way, and it's a pretty straightforward set of changes.
Fixes #2327