Repository navigation
SDSTOR-24888: Add inject_auth_metadata helper to GrpcSyncClient - #332
Merged
d2chau merged 4 commits intoSep 8, 2026
Merged
Conversation
GrpcSyncClient held m_token_client (inherited from GrpcBaseClient) but provided no mechanism to inject it into a ClientContext. Subclasses had to access the protected member directly, duplicating the null-check and AddMetadata call. Add a protected inject_auth_metadata() that mirrors the auth injection already present in GrpcAsyncClient::AsyncStub::prepare_and_send_unary and GenericAsyncStub::prepare_and_send_unary_generic. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add two tests to the existing auth_test fixture: - sync_client_injects_auth_header: constructs EchoAndPingClient with a token client and calls inject_auth_metadata from inside call_echo (mirroring OmClient::init_ctx usage); verifies the auth-gated server accepts the call. - sync_client_no_token_is_noop: constructs EchoAndPingClient without a token client; verifies inject_auth_metadata is a no-op and the server rejects with UNAUTHENTICATED. Also adds call_echo() to EchoAndPingClient to demonstrate the intended subclass usage pattern of inject_auth_metadata. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use ASSERT_TRUE with error_message for clearer failure diagnostics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
sbinmalek
approved these changes
Sep 4, 2026
sbinmalek
left a comment
sbinmalek
left a comment
There was a problem hiding this comment.
LGTM!
Changes looks good. Added a nit comment. Approving the PR
| } | ||
|
|
||
| TEST_F(AuthEnableTest, sync_client_injects_auth_header) { | ||
| EchoAndPingClient sync_client{grpc_server_addr, m_token_client, "", ""}; |
There was a problem hiding this comment.
Is the sync_client correctly constructed? I see different args for both newly added tests.
Contributor
Author
There was a problem hiding this comment.
Yes, that's actually exactly what should be tested. For example.
- Line 341 deliberately omits the token client — the test verifies auth works when the caller injects the header manually (via context.AddMetadata)
- Line 354 passes m_token_client — the test verifies that inject_auth_metadata in call_echo() handles injection automatically
- The different signatures reflect the two distinct code paths being tested
There was a problem hiding this comment.
🟢 Approval recommended
The focused implementation matches existing async behavior and includes coverage for authenticated and tokenless clients.
Pull request overview
Adds authentication metadata support for synchronous gRPC client subclasses.
Changes:
- Adds protected
inject_auth_metadata. - Tests authenticated and tokenless sync calls.
- Bumps package version to 14.8.2.
File summaries
| File | Description |
|---|---|
include/sisl/grpc/rpc_client.hpp |
Adds sync auth metadata helper. |
src/grpc/tests/unit/auth_test.cpp |
Tests helper behavior. |
conanfile.py |
Bumps package version. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GrpcSyncClienthad no way for subclasses to inject auth metadata intosync RPC contexts, even when a
GrpcTokenClientwas present on the base classprotected inject_auth_metadata(grpc::ClientContext&) constthat readsfrom the inherited
m_token_client; no-ops silently when token client is nullGrpcAsyncClientalready had an equivalent inline auth injection; this closesthe gap for sync clients
Motivation
OmClient(block_mgr) usesGrpcSyncClientfor BM→OM registration andheartbeat RPCs. OM in cluster 117 has
grpc_auth_enable = 1/auth_mode = 1.BMs were being rejected by OM because every sync call arrived with no
Authorizationheader —m_token_clientwas populated butget_token()wasnever called. This single-method addition fixes that without any API break.