Add HMAC capabilities

[florian-wagner-frequenz]

This adds the capability for HMAC based signing into the reporting client.

[Copilot]

Pull Request Overview

Adds HMAC-based signing support to the reporting client and CLI

• Introduce SigningOptions and new --secret CLI flag, wiring through ChannelOptions
• Bump frequenz-client-base dependency and relax pytest warnings-as-errors
• Document HMAC feature in release notes and suppress specific Protobuf warnings in tests

Reviewed Changes

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

File	Description
tests/conftest.py	Show Protobuf version warnings once during tests
src/frequenz/client/reporting/cli/main.py	Import SigningOptions, add --secret flag, build ChannelOptions for HMAC
pyproject.toml	Bump frequenz-client-base to >=0.10.0,<0.11.0; adjust pytest addopts
RELEASE_NOTES.md	Add HMAC feature entry and describe new CLI flag

Comments suppressed due to low confidence (3)

src/frequenz/client/reporting/cli/main.py:129

• [nitpick] The parameter name secret is very generic; consider renaming it to hmac_secret to make its purpose (HMAC signing) clearer.

secret: str,

RELEASE_NOTES.md:14

• This line refers to a --key flag but the new flag is --secret; update it to accurately describe the --secret option.

*    * The new CLI option "key" can be used to provide the server's key.

pyproject.toml:158

• [nitpick] You removed -Werror, so deprecation warnings no longer fail the build; consider re-adding it to catch unintended deprecations in CI.

addopts = "-W=all -Wdefault::DeprecationWarning -Wdefault::PendingDeprecationWarning -vv"

[Copilot]

Always constructing SigningOptions with a potentially None secret can lead to runtime errors; add a guard to only initialize it when secret is provided or raise a clear error if missing.

Suggested change

	auth = AuthenticationOptions(key)
	auth = AuthenticationOptions(key)
	if not secret:
	raise ValueError("The 'secret' parameter is required and cannot be None.")

[llucax]

Maybe call this signing_secret for extra clarity?

[florian-wagner-frequenz]

Gladly

[llucax]

This looks pretty inconsistent, IMHO this should be done inside the client class, and the signing secret should be taken as a separate argument, like with key. The options passed here are default options. There is also an overlap, because the client is doing its own key management, which if it doesn't collide with the new auth interceptor, it will overlap.

Looking at the ReportingApiClient.__init__, I feel like the current approach we taken with the auth key and sign secret is not consistent with the default channel options :-/

All channel options were designed to be mainly set from the server_url, and the channel_defauls is supposed to just pass defaults, but the real end values are configured in the server_url if different to the default. But we don't support setting the auth key and sign secret. This maybe makes sense in a way, as it probably makes more sense to pass secrets in separate env vars in the future, instead of treating the whole server_url as a secret. But it doesn't make sense to use a default secret that is not None, as either you want to use a secret, in which case you need to specify one, or you don't, but there is no default secret one can provide.

I think we should probably change the base client to pass secrets outside the default_options to the client. Which means there are no more sign and auth option in channel options. So the base client ctor will take a auth_key and sign_secret as explicit arguments defaulting to None. So you either pass it or not, but there is no default. Sorry @florian-wagner-frequenz 🙏, I think this was your original approach and I suggested using the channel options. I think when I proposed it the idea was also to add eventually support to setting this via env vars, which is more reasonable, but I also agree reading env vars deep in some library, specially if they are secrets, is a bad idea.

A for a temporary solution I suggest changing the current ReportingApiClient.__init__ to something like:

class ReportingApiClient(BaseApiClient[ReportingStub]):
    """A client for the Reporting service."""


    def __init__(
        self,
        server_url: str,
        auth_key: str | None = None,
        sign_secret: str | None = None,
        connect: bool = True,
        channel_defaults: ChannelOptions = ChannelOptions(),  # default options
    ) -> None:
        channel_defaults = channel_defaults.replace(sign=SigningOptions(secret=sign_secret))
        channel_defaults = channel_defaults.replace(auth=AuthenticationOptions(api_key=auth_key))
        super().__init__(
            server_url,
            ReportingStub,
            connect=connect,
            channel_defaults=channel_defaults,
        )

[florian-wagner-frequenz]

Okay, I will do the temporary fix first and see how far I get with doing it properly in the base client. I agree with your arguments.
For me the core issue is:

• key and secret are not optional (as the service won't respond), so shouldn't live in the channel_defaults (as we can not conjure them from thin air
• We don't want to pull them from an environment variable deep in the library (compounding the previous issue)
• We want them to stay secret, so we don't want them in the server_url

So the targeted interface for the clients/apps stays to pass via command line argument (possibly from environment variable).
This of course pushes the burden to app maintainers, but I think that is unavoidable unless we want the "pull from environment variable in library" solution.

[llucax]

-Werror should be kept

[florian-wagner-frequenz]

okay, but I think then I might need to put the specific warning back in. This is the fix Flora applied.

[llucax]

oh, really, I didn't realized that, I guess I missed that change as there were other changes too in that line. Then something went wrong. @flora-hofmann-frequenz :-/

Removing -Werror means we don't generally treat warnings as errors anymore. I will have a look.

[llucax]

OK, I found a fix, and while at it, tried a few improvements, with these options we don't need to add the conftest.py file at all!

[tool.pytest.ini_options]
addopts = "-vv"
filterwarnings = [
  "error",
  "once::DeprecationWarning",
  "once::PendingDeprecationWarning",
  # We use a raw string (single quote) to avoid the need to escape special
  # chars as this is a regex
  'ignore:Protobuf gencode version .*exactly one major version older.*:UserWarning',
]

I think it is better to complete ignore the protobuf warnings, and also not care about the reported version (so we don't need to update the ignore rule when we bump the protobuf version), we know we are doing this, so it only adds noise to the tests to show it. We'll get a proper error if we go beyond the supported range anyways.

Also show deprecations only once, we don't need to get spammed if a deprecated symbol is used more than once.

I will add these improvements to repo-config.

[llucax]

• Improve pytest warning filtering frequenz-repo-config-python#407