Change interface for HMAC #145

florian-wagner-frequenz · 2025-05-23T16:24:59Z

This changes the interface for communicating the API key and the signing secret. Specifically it removes them from "authentication options" and moves them to their own parameters instead.
This allows using the default pattern for channel options in places that previously did not touch it. This is desired as soon most clients will require the API key and the signing secret to function properly, and we want to keep the changes needed to them minimal.

Copilot

Pull Request Overview

This pull request changes the interface for HMAC by removing the options dataclasses for signing and authentication and directly passing the secret and API key to the interceptors.

Removed SigningOptions and AuthenticationOptions dataclasses.
Updated SigningInterceptor and AuthenticationInterceptor constructors to accept direct string parameters.
Modified parse_grpc_uri to support the new interface by adding key and sign_secret parameters.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
src/frequenz/client/base/signing.py	Removed SigningOptions and updated constructors to use a direct secret parameter.
src/frequenz/client/base/channel.py	Updated parse_grpc_uri to introduce key and sign_secret parameters instead of using options.
src/frequenz/client/base/authentication.py	Removed AuthenticationOptions and updated constructors to use a direct API key parameter.

Comments suppressed due to low confidence (2)

src/frequenz/client/base/channel.py:91

[nitpick] Consider renaming 'key' to 'api_key' in the parse_grpc_uri function for consistency with the naming used in authentication interceptors.

key: str | None = None,

src/frequenz/client/base/channel.py:92

[nitpick] Consider renaming 'sign_secret' to 'secret' or 'signing_secret' to match the naming used in SigningInterceptor.

sign_secret: str | None = None,

Copilot · 2025-05-23T16:25:22Z

src/frequenz/client/base/signing.py

    """An Interceptor that adds HMAC authentication of the metadata fields to a gRPC call."""

-    def __init__(self, options: SigningOptions):
+    def __init__(self, secret: str):


Update the class documentation for SigningInterceptorUnaryUnary to reflect that the secret is now passed directly as a string parameter instead of via SigningOptions.

Copilot · 2025-05-23T16:25:23Z

src/frequenz/client/base/authentication.py

    """An Interceptor that adds HMAC authentication of the metadata fields to a gRPC call."""

-    def __init__(self, options: AuthenticationOptions):
+    def __init__(self, api_key: str):


Update the class documentation for AuthenticationInterceptorUnaryUnary to reflect that the API key is now passed directly as a parameter instead of via AuthenticationOptions.

llucax

We still need to make this available through the BaseClient.

src/frequenz/client/base/channel.py

llucax

Docs and tests are the most important comments for me, the rest is optional.

src/frequenz/client/base/channel.py

RELEASE_NOTES.md

llucax · 2025-06-03T09:37:37Z

src/frequenz/client/base/client.py

+            auth_key: The API key to use when connecting to the service.
+            sign_secret: The secret to use when creating message HMAC.


I would clarify in here that the old ones are kept if they are not passed or ... is passed. While at it, maybe it is worth to homogenize this and change server_url: str | EllipsisType = ..., and then we can just mention once in the docs body that any omitted value (or value passed as ...) will keep the old value. But we can change the server_url in a separate PR too.

I will clarify for now and not touch the server_url

llucax · 2025-06-03T09:42:05Z

src/frequenz/client/base/channel.py

    uri: str,
    /,
    defaults: ChannelOptions = ChannelOptions(),
+    interceptors: list[ClientInterceptor] | None = None,


Nitpick: I would move interceptors before the /, to allow to use it as a positional argument. We require the keyword for defaults because ChannelOptions are sometimes used as concrete options and sometimes as defaults that will be overridden by those concrete options, so we wanted to make it extra clear these are just defaults and might not be the options that are used by the channel at the end. But with interceptors there is no ambiguity, instances passed in there will be instances of ClientInterceptors and there is only one way to use them and there will be no overrides.

Super nitpick because at the end probably almost nobody will use parse_grpc_uri directly, so it won't make much of a difference.

tests/test_client.py

src/frequenz/client/base/channel.py

llucax

Just one tiny formatting issue with the release notes.

As for the test, I can see if you are not familiar with mocking, patching and pytest it could be a bit complicated or very time consuming, so I'm OK with merging as is and then I can try to add a few basic tests to at least make sure some basic behavior doesn't accidentally break.

llucax · 2025-06-04T09:42:02Z

RELEASE_NOTES.md


-<!-- Here goes notes on how to upgrade from previous versions, including deprecations and what they should be replaced with -->
+* Updated interface and behavior for HMAC
+    This brings with introduces a new positional argument to `parse_grpc_uri`.


This needs an extra space to render properly (or you can add some punctuation after the for HMAC if you want to keep it in one paragraph in the bullet point):

Suggested change

This brings with introduces a new positional argument to `parse_grpc_uri`.

This brings with introduces a new positional argument to `parse_grpc_uri`.

You can see how the rendered markdown is shown with the button on the top of the file:

Ah, thanks. I will add. I was hoping I could get the paragraph indented, alas, this ain't LaTeX.

I feel you couldn't decide which beginning of the sentence to use and just went with both XD

This brings with introduces a new

Damn, sometimes I focus too much on the format and forget about the contents 😆

tests/test_channel.py

tests/test_client.py

llucax

LGTM in general, there is only the grammar thing in the release notes now. Approving as we can also fix when preparing the notes for release, but feel free to fix before merging and just force-merging after it is fixed.

llucax · 2025-06-05T07:03:37Z

RELEASE_NOTES.md


-<!-- Here goes notes on how to upgrade from previous versions, including deprecations and what they should be replaced with -->
+* Updated interface and behavior for HMAC
+    This brings with introduces a new positional argument to `parse_grpc_uri`.


Damn, sometimes I focus too much on the format and forget about the contents 😆

This changes the interface for communicating the API key and the signing secret. Specifically it removes them from "authentication options" and moves them to their own parameters instead. This allows using the default pattern for channel options in places that previously did not touch it. This is desired as soon most clients will require the API key and the signing secret to function properly, and we want to keep the changes needed to them minimal. Signed-off-by: Florian Wagner <[email protected]>

florian-wagner-frequenz requested review from Copilot and llucax May 23, 2025 16:24

florian-wagner-frequenz requested a review from a team as a code owner May 23, 2025 16:25

github-actions bot added the part:code Affects the code in general label May 23, 2025

Copilot AI reviewed May 23, 2025

View reviewed changes

llucax reviewed May 26, 2025

View reviewed changes

src/frequenz/client/base/channel.py Outdated Show resolved Hide resolved

src/frequenz/client/base/channel.py Outdated Show resolved Hide resolved

florian-wagner-frequenz force-pushed the change_hmac_interface branch from e187a7c to f233d0f Compare June 2, 2025 13:00

github-actions bot added the part:tests Affects the unit, integration and performance (benchmarks) tests label Jun 2, 2025

florian-wagner-frequenz requested a review from llucax June 2, 2025 13:02

florian-wagner-frequenz force-pushed the change_hmac_interface branch from f233d0f to 8edf5b6 Compare June 2, 2025 13:06

github-actions bot added the part:docs Affects the documentation label Jun 2, 2025

llucax reviewed Jun 3, 2025

View reviewed changes

llucax added this to the v0.11.0 milestone Jun 4, 2025

florian-wagner-frequenz force-pushed the change_hmac_interface branch from 8edf5b6 to d73c6a9 Compare June 4, 2025 08:42

florian-wagner-frequenz requested a review from llucax June 4, 2025 08:43

llucax reviewed Jun 4, 2025

View reviewed changes

florian-wagner-frequenz force-pushed the change_hmac_interface branch from d73c6a9 to 34afb00 Compare June 4, 2025 15:48

florian-wagner-frequenz requested a review from llucax June 4, 2025 15:49

llucax previously approved these changes Jun 5, 2025

View reviewed changes

florian-wagner-frequenz dismissed llucax’s stale review via b24c99f June 5, 2025 07:33

florian-wagner-frequenz force-pushed the change_hmac_interface branch from 34afb00 to b24c99f Compare June 5, 2025 07:33

florian-wagner-frequenz enabled auto-merge June 5, 2025 07:51

llucax approved these changes Jun 5, 2025

View reviewed changes

florian-wagner-frequenz added this pull request to the merge queue Jun 5, 2025

llucax approved these changes Jun 5, 2025

View reviewed changes

Merged via the queue into frequenz-floss:v0.x.x with commit b225c9c Jun 5, 2025
5 checks passed

florian-wagner-frequenz deleted the change_hmac_interface branch June 5, 2025 08:07

		auth_key: The API key to use when connecting to the service.
		sign_secret: The secret to use when creating message HMAC.

	This brings with introduces a new positional argument to `parse_grpc_uri`.

	This brings with introduces a new positional argument to `parse_grpc_uri`.

Uh oh!

Change interface for HMAC #145

Change interface for HMAC #145

Uh oh!

Conversation

florian-wagner-frequenz commented May 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI May 23, 2025

Choose a reason for hiding this comment

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

llucax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants