Skip to content

Conversation

@florian-wagner-frequenz
Copy link
Contributor

This adds authentication and signing interceptors for stream requests. Specifically for those where the client sends a single message and receives a stream.

Copilot AI review requested due to automatic review settings May 23, 2025 08:28
@florian-wagner-frequenz florian-wagner-frequenz requested a review from a team as a code owner May 23, 2025 08:28
@github-actions github-actions bot added part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests part:code Affects the code in general labels May 23, 2025
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds HMAC signing and API key authentication interceptors for both unary-unary and unary-stream gRPC calls by extracting header logic into helper functions and registering both variants in the channel builder.

  • Factor out _add_hmac and _add_auth_header to centralize metadata injection
  • Introduce SigningInterceptorUnaryUnary/SigningInterceptorUnaryStream and AuthenticationInterceptorUnaryUnary/AuthenticationInterceptorUnaryStream
  • Update channel setup to append both interceptor types and adjust tests to call new helpers directly

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/frequenz/client/base/signing.py Add _add_hmac helper, split signing interceptors for two call types
src/frequenz/client/base/authentication.py Add _add_auth_header helper, split auth interceptors for two call types
src/frequenz/client/base/channel.py Register both UnaryUnary and UnaryStream interceptors in channel builder
tests/test_signing.py Update test to call _add_hmac directly and adjust expected sig
tests/test_authentication.py Update test to call _add_auth_header directly
RELEASE_NOTES.md Note HMAC signing support for both UnaryUnary and UnaryStream
Comments suppressed due to low confidence (3)

tests/test_signing.py:21

  • [nitpick] The test only asserts the sig field but does not verify that ts and nonce metadata entries are set correctly; consider adding assertions for those headers.
_add_hmac(b"hunter2", client_call_details, 1634567890, b"123456789")

tests/test_authentication.py:20

  • [nitpick] There are no tests covering the new UnaryStream authentication interceptor; consider adding a test to ensure _add_auth_header is applied in the stream context or via the interceptor.
_add_auth_header(key, client_call_details)

src/frequenz/client/base/signing.py:25

  • This module uses hmac.new and _logger but neither import hmac nor import logging is present; add those imports at the top to avoid NameError.
def _add_hmac(

Copy link
Contributor

@llucax llucax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. 2 very minor and optional comments:

  • Split the erros -> errors typo fix into a separate commit
  • Put the common code into a mixin class (like SigningInterceptorMixin) and then inherit from both the interceptor and the mixin. This can save a couple of extra lines (basically the __init__() method). Mostly FYI in case you are not familiar with this option in Python, the code is so simple that for me it is still perfectly fine too keep it as it is.

Signed-off-by: Florian Wagner <[email protected]>
This adds authentication and signing interceptors for stream requests.
Specifically for those where the client sends a single message and
receives a stream.

Signed-off-by: Florian Wagner <[email protected]>
@florian-wagner-frequenz
Copy link
Contributor Author

I decided not to do the mixin, but split out the error.
If we decide to do the other two classes (StreamUnary, StreamStream), I will do the mixin.

@florian-wagner-frequenz florian-wagner-frequenz added this pull request to the merge queue May 23, 2025
Merged via the queue into frequenz-floss:v0.x.x with commit 0e963b9 May 23, 2025
5 checks passed
@florian-wagner-frequenz florian-wagner-frequenz deleted the add_stream_interceptors branch May 23, 2025 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

part:code Affects the code in general part:docs Affects the documentation part:tests Affects the unit, integration and performance (benchmarks) tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants