Metadata identity - #1918
Conversation
asher-pem-arm
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1918 +/- ##
========================================
+ Coverage 60.8% 61.0% +0.1%
========================================
Files 184 187 +3
Lines 15099 15226 +127
========================================
+ Hits 9185 9288 +103
- Misses 5914 5938 +24
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
|
I took the liberty to reformatted for the PR template, this should make the description more readable. |
|
During the review of this PR yesterday it came up that we are missing a good justification for separate unary RPC instead of extending the |
|
Hey @Emantor , Thanks for sharing the concern. The primary motivation for this came out of our work on the Labgrid fleeting plugin, and the more we thought about it the more sense it made. At the moment ClientStream is doing two jobs: it is both a watch/update mechanism and part of the session/identity for independent RPCs. That coupling is what we are trying to remove. A service, CI worker, web backend, or short-lived tool should be able to make a bounded RPC with identity/auth metadata on that RPC, get a response, and finish. This also matters for production/distributed deployments. If every client has to establish and maintain a ClientStream session, then coordinator endpoint changes, load balancer migration, reconnects, and horizontally scaled services all need to manage stream/session recovery. In many cases those clients gain no benefit from a long-lived session. The intent here is to decouple those concerns: clients that need ClientStream for live updates can still use it. A stream also makes auth harder as the coordinator has to continuously filter updates for each client and ensure no unauthorised state is ever sent. For unary RPCs, the auth decision is scoped to one operation and one response. If you feel this is useful context I can add this to the discussion as needed. Thanks |
8291b0e to
604b887
Compare
|
@Emantor as discussed I've updated this PR to only include the client stream decoupling |
604b887 to
04a49f6
Compare
There was a problem hiding this comment.
Looking at the test output for this PR I see that the tests seem not to use the client identity:
pytest --cov-report=term-missing --cov=labgrid tests -k coordinator -s -vv
current_level: None
============================================================================================================ test session starts =============================================================================================================
platform linux -- Python 3.13.13, pytest-9.1.1, pluggy-1.6.0 -- /home/phoenix/work/ptx/labgrid/.venv/bin/python3
cachedir: .pytest_cache
benchmark: 5.2.3 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/phoenix/work/ptx/labgrid
configfile: pytest.ini (WARNING: ignoring pytest config in pyproject.toml!)
plugins: mock-3.15.1, isort-4.0.0, dependency-0.6.1, cov-7.1.0, benchmark-5.2.3, asyncio-1.4.0, labgrid-26.1.dev27
asyncio: mode=Mode.AUTO, debug=False, asyncio_default_fixture_loop_scope=None, asyncio_default_test_loop_scope=function
collected 496 items / 478 deselected / 1 skipped / 18 selected
tests/test_coordinator.py::test_startup coordinator: INFO:root:loaded 0 place(s)
coordinator: INFO:root:Enabled channelz support
coordinator: INFO:root:Coordinator ready
coordinator: listening on [::]:20408
PASSED
------------------------------------------------------------------------------------------------------------- live log teardown --------------------------------------------------------------------------------------------------------------
INFO root: stopping Coordinator pid=27317
coordinator: INFO:root:Starting graceful shutdown...
coordinator: INFO:root:Starting graceful shutdown...
tests/test_coordinator.py::test_coordinator_add_place coordinator: INFO:root:loaded 0 place(s)
coordinator: INFO:root:Enabled channelz support
coordinator: INFO:root:Coordinator ready
coordinator: listening on [::]:20408
coordinator: INFO:root:client connected: ipv4:127.0.0.1:42882
coordinator: WARNING:root:client ipv4:127.0.0.1:42882 did not provide identity metadata; using deprecated startup identityBut since this is with the client interceptor, shouldn't these use the new identity metadata?
Edit:Scratch that, the tests use a bespoke channel stub which does not add the metadata.
Emantor
changed the title
Emantor
left a comment
Emantor
left a comment
There was a problem hiding this comment.
The code looks good to me, all thats left in my opinion is to squash the last commit.
It was nice for reviewing, but it should be cleaner to not remove and re-add StartupDone support from a history point of view.
04a49f6 to
1db182f
Compare
|
What is the meaning of ? You can't add a Reviewed-by for code you've written yourself. |
| except KeyError: | ||
| raise |
|
|
||
| # additional dev dependencies | ||
| "psutil>=5.8.0", | ||
| "pytest-asyncio==1.3.0", |
There was a problem hiding this comment.
Why do we need to pin to that exact version?
There was a problem hiding this comment.
updated to remove pin
| identity = client_identity_context.get() | ||
| if identity: | ||
| logging.debug("client identity provided in gRPC metadata") | ||
| logging.debug(identity) |
There was a problem hiding this comment.
This should be one logging.debug() line.
| logging.debug("exporter identity provided in gRPC metadata") | ||
| logging.debug(identity) |
There was a problem hiding this comment.
This should be a single logging.debug line.
| grpc.StatusCode.ALREADY_EXISTS, | ||
| f"startup failed: exporter with name '{identity.id}' is already connected from {existing.peer}", | ||
| ) | ||
| self.exporters[peer] = ExporterSession(self, peer, identity.id, command_queue, identity.user_agent) |
There was a problem hiding this comment.
Same as for ClientSession above, should be created in a single place.
There was a problem hiding this comment.
The "why" of each change should also be explained in the commit messages.
Motivation
Decouple ClientStream’s live-update role from identity and independent RPCs, allowing services and short-lived tools to perform bounded authenticated operations without maintaining stream state. This simplifies client implementations, reconnection handling, horizontal scaling, and per-operation authorization.
I'm not fully convinced by the simplification arguments.
"client implementations" might be correct for other implementations, but labgrid's own client becomes more complex through the introduction of the Interceptors.
"reconnection handling" only applies to stateless clients, right? Anything using labgrid to access resources needs a ordered and loss-less stream of resource events to keep a consistent view of a Place. Supporting reconnection would mean buffering and replaying resource events upon reconnection, and I'm not sure if the complexity required for that is worth it.
"horizontal scaling" don't seem correct to me. As long as the state is held in the coordinator process (which I don't see us changing), there can only be a single instance as this ensures consistency.
"per-operation authorization" sounds right, as we no longer need to look up the ClientSession by peer.
So, I'd prefer a motivation description which make sense (primarily) from the perspective of using labgrid with the in-tree exporter and client.
| client_identity_context: contextvars.ContextVar[Optional[ClientIdentity]] = contextvars.ContextVar( | ||
| "client_identity", default=None | ||
| ) |
There was a problem hiding this comment.
I've been trying to understand why you chose to use a ContextVar instead of using the gRPC Server Side context object passed to each API method to inspect the invocation_metadata? It feels like the ContextVar adds a mechanism similar to what is already provided by the context parameter.
Later, on the authorization side, a function decorator could also access the ServicerContext parameter of the call to decide whether to allow/deny the call.
Without a ContextVar, we don't have easy access to the ClientIdentity, but as auth checks should be done early, I'm skeptical if that access is needed at all. Without the ContextVar, the complexity of the Interceptors wouldn't be needed either.
I'm not saying we must avoid ContextVar and Interceptors, but I want to understand why they are the right choice.
There was a problem hiding this comment.
Currently the RPCs use the caller identity within RPC handlers, eg. for assigning acquired on a Place.
We expect performing authorisation in a central location (server interceptor) is beneficial, especially as more complex auth is supported.
Storing the identity (and in the future, ideally Capabilities) in a ContextVar means that the identity is accessible without needing a call the parse the metadata within each RPC.
1db182f to
7e73102
Compare
@jluebbe the original code was written by Luke and then between Luke and myself we've been making the changes for the PR comments, when either one of us makes an update the other reviews it but as part of our internal process I'm the gatekeeper that decides when it gets upstreamed. Hopefully doesn't confuse things too much |
7e73102 to
554bd1f
Compare
Hey @jluebbe , thanks for the reply. By client simplification, we primarily meant implementations such as the fleeting plugin, CI workers, and web services that need short-lived request/response operations but do not consume live place or resource updates. The in-tree client still needs There is also a practical startup-time benefit for commands such as You’re right that this PR does not implement reconnection for streaming consumers. A client could rebuild its current state from a fresh snapshot followed by ordered updates, so replay is not necessarily required. However, robust stream reconnection is a separate problem and should not be claimed as a benefit here. Likewise, horizontal scaling was intended to refer to simplifying the use of multiple caller-service instances, not multiple stateful coordinator instances. Although decoupling RPC identity from an in-process stream session could be an enabling step towards scaling coordinator instances if their state were externalised in the future. The concrete benefit for labgrid is the separation of responsibilities: I've updated the motivation and commit messages to include this |
Add client and server interceptors which attach labgrid identity metadata to gRPC calls and expose it to coordinator RPC handlers. Use the metadata identity to register client and exporter stream sessions while keeping startup-message handling as a deprecated fallback for older clients and exporters. Signed-off-by: Asher Pemberton <asher.pemberton@arm.com> Reviewed-by: Asher Pemberton <asher.pemberton@arm.com> # gatekeeper Co-authored-by: Luke Beardsmore <luke.beardsmore2@arm.com>
Allow AcquirePlace, AllowPlace and CreateReservation to identify the caller from gRPC metadata instead of requiring identity to come only from an established ClientStream session. ClientStream should remain the ordered place/resource update channel. Unary-only callers should not need to establish and synchronize that stream merely to provide identity. This reduces startup and state management overhead for short-lived tools and services. Keep the existing ClientStream session lookup as a fallback so older clients which still send startup messages on the stream continue to work. Signed-off-by: Asher Pemberton <asher.pemberton@arm.com> Reviewed-by: Asher Pemberton <asher.pemberton@arm.com> # gatekeeper Co-authored-by: Luke Beardsmore <luke.beardsmore2@arm.com>
554bd1f to
24624ba
Compare
|
note:
|
4 participants
Description
This is the first part of the coordinator unary-RPC rework. It introduces metadata-based identity for clients and exporters, reducing the coordinator’s dependency on startup messages sent through the long-lived streams.
This PR:
Motivation
ClientStream currently acts both as the ordered live-update channel and as the source of caller identity for independent RPCs. This forces short-lived tools and services to establish and synchronise a stream before making simple request/response calls, adding startup latency and state-management overhead.
Carrying identity in each RPC’s metadata allows these callers to invoke the RPC, emit the result, and exit. Stateful clients still use ClientStream for ordered place and resource updates. This separation also provides call-scoped identity for future per-operation authorization; the metadata itself is not authentication.
Compatibility note
Old clients and exporters remain compatible with the new coordinator through the startup-message fallback.
However, new clients and exporters stop sending startup identity messages and are therefore incompatible with coordinators predating this change. Those coordinators cannot establish the required stream session.
See also
Discussion on the gRPC unary rework: #1881
Follow-up RPC changes: #1919
The List RPC changes: #1933
Follow on capabilities work: #1955