feat: add AWS SigV4 request signing support#771
Open
ajcasagrande wants to merge 1 commit intomainfrom
Open
Conversation
Add a request_signer plugin category with SigV4 implementation using botocore. Transports sign requests via _sign_if_needed() hook, replacing Bearer token auth when --auth-type is set. Includes CLI options (--auth-type, --aws-region, --aws-service, --aws-profile), config validation, plugin registry entries, and full test coverage. Signed-off-by: Anthony Casagrande <acasagrande@nvidia.com>
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@e2151cd80488d2901f20f1fc43a489569bcdadd6Recommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@e2151cd80488d2901f20f1fc43a489569bcdadd6Last updated for commit: |
Contributor
Author
|
@dferguson992 @Lokiiiiii please take a look when you get a chance. thanks! |
WalkthroughThis PR introduces AWS Signature Version 4 (SigV4) authentication support to aiperf. It adds a request signer protocol, SigV4 implementation using botocore, new CLI options and configuration fields, plugin system integration, transport-level signing hooks, and comprehensive documentation with tutorials and tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~35 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/unit/auth/test_base_signer.py (2)
31-34: Use a behavioral runtime-checkability assertion instead of protocol internals.The assertion on Line 32-34 depends on implementation internals and can be brittle. Prefer asserting
isinstance(dummy, RequestSignerProtocol)with a minimal compliant dummy signer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth/test_base_signer.py` around lines 31 - 34, Replace the brittle internal-attribute assertion in test_protocol_is_runtime_checkable by creating a minimal dummy class implementing the protocol methods/attributes required by RequestSignerProtocol and assert isinstance(dummy, RequestSignerProtocol); specifically, implement a small DummySigner with the same public method signatures used by RequestSignerProtocol and use isinstance(dummy, RequestSignerProtocol) in the test instead of checking __protocol_attrs__ or __abstractmethods__ to verify runtime checkability.
9-32: Align test names with repository convention.Names like Line 9 (
test_headers_only) and Line 31 (test_protocol_is_runtime_checkable) should followtest_<function>_<scenario>_<expected>.As per coding guidelines
tests/**/*.py:Test naming convention: test_<function>_<scenario>_<expected>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth/test_base_signer.py` around lines 9 - 32, Rename tests to match the repository convention test_<function>_<scenario>_<expected>: update test_headers_only to something like test_SignedRequest_headers_only_url_and_body_none, test_all_fields to test_SignedRequest_all_fields_preserved, and test_slots to test_SignedRequest_slots_prevent_dynamic_attrs; likewise rename test_protocol_is_runtime_checkable (in TestRequestSignerProtocol) to test_RequestSignerProtocol_runtime_checkable; update any references/imports or test markers accordingly for SignedRequest, RequestSignerProtocol, and TestRequestSignerProtocol to keep tests discoverable.tests/unit/auth/test_sigv4_signer.py (1)
40-205: Rename test functions to the required pattern.Several test names (for example on Line 40 and Line 144) don’t follow the required
test_<function>_<scenario>_<expected>format.Proposed rename examples
- def test_stores_config(self) -> None: + def test___init___with_explicit_aws_fields_stores_values(self) -> None: - async def test_sign_adds_authorization_header(self) -> None: + async def test_sign_with_sigv4_adds_authorization_header(self) -> None:As per coding guidelines
tests/**/*.py:Test naming convention: test_<function>_<scenario>_<expected>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/auth/test_sigv4_signer.py` around lines 40 - 205, One or more test functions do not follow the required naming pattern test_<function>_<scenario>_<expected> (e.g., test_stores_config); locate offending test functions such as test_stores_config and any other methods that don’t match the pattern and rename them to the required format (for example test_init_model_endpoint_stores_config_values), updating any references where they’re invoked; ensure class names (TestSigV4RequestSignerInitCredentials, TestSigV4RequestSignerSign) remain unchanged and run the test suite to confirm names are accepted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.gitignore:
- Around line 38-40: The new .gitignore entry "docs/superpowers/*" is unrelated
to the SigV4 signing work; either remove that line from this PR or revert it and
place it in a separate, focused PR that documents why docs/superpowers should be
ignored. Locate the ".gitignore" change that adds "docs/superpowers/*", revert
that single entry from this branch (or move it into a new branch/PR) and keep
this PR limited to the AWS SigV4 request signing changes only.
In `@docs/tutorials/aws-sigv4-auth.md`:
- Around line 94-98: The fenced code block that contains the URL
"https://abc123.execute-api.us-east-1.amazonaws.com/..." is missing a language
tag which triggers markdownlint MD040; update that fence (the triple-backtick
that opens the block) to include a language specifier such as "text" (e.g.,
change ``` to ```text) so the snippet is properly labeled and lint-clean.
---
Nitpick comments:
In `@tests/unit/auth/test_base_signer.py`:
- Around line 31-34: Replace the brittle internal-attribute assertion in
test_protocol_is_runtime_checkable by creating a minimal dummy class
implementing the protocol methods/attributes required by RequestSignerProtocol
and assert isinstance(dummy, RequestSignerProtocol); specifically, implement a
small DummySigner with the same public method signatures used by
RequestSignerProtocol and use isinstance(dummy, RequestSignerProtocol) in the
test instead of checking __protocol_attrs__ or __abstractmethods__ to verify
runtime checkability.
- Around line 9-32: Rename tests to match the repository convention
test_<function>_<scenario>_<expected>: update test_headers_only to something
like test_SignedRequest_headers_only_url_and_body_none, test_all_fields to
test_SignedRequest_all_fields_preserved, and test_slots to
test_SignedRequest_slots_prevent_dynamic_attrs; likewise rename
test_protocol_is_runtime_checkable (in TestRequestSignerProtocol) to
test_RequestSignerProtocol_runtime_checkable; update any references/imports or
test markers accordingly for SignedRequest, RequestSignerProtocol, and
TestRequestSignerProtocol to keep tests discoverable.
In `@tests/unit/auth/test_sigv4_signer.py`:
- Around line 40-205: One or more test functions do not follow the required
naming pattern test_<function>_<scenario>_<expected> (e.g., test_stores_config);
locate offending test functions such as test_stores_config and any other methods
that don’t match the pattern and rename them to the required format (for example
test_init_model_endpoint_stores_config_values), updating any references where
they’re invoked; ensure class names (TestSigV4RequestSignerInitCredentials,
TestSigV4RequestSignerSign) remain unchanged and run the test suite to confirm
names are accepted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 30acc727-bb7a-422b-8950-e744e35f2787
📒 Files selected for processing (23)
.gitignoreREADME.mddocs/cli-options.mddocs/tutorials/aws-sigv4-auth.mdpyproject.tomlsrc/aiperf/auth/__init__.pysrc/aiperf/auth/base_signer.pysrc/aiperf/auth/sigv4_signer.pysrc/aiperf/common/config/endpoint_config.pysrc/aiperf/common/models/model_endpoint_info.pysrc/aiperf/endpoints/base_endpoint.pysrc/aiperf/plugin/categories.yamlsrc/aiperf/plugin/enums.pysrc/aiperf/plugin/plugins.pysrc/aiperf/plugin/plugins.yamlsrc/aiperf/plugin/schema/plugins.schema.jsonsrc/aiperf/transports/aiohttp_transport.pysrc/aiperf/transports/base_transports.pytests/unit/auth/__init__.pytests/unit/auth/test_base_signer.pytests/unit/auth/test_sigv4_signer.pytests/unit/transports/conftest.pytests/unit/transports/test_aiohttp_transport_signing.py
Comment on lines
+38
to
+40
|
|
||
| # Superpowers | ||
| docs/superpowers/* |
There was a problem hiding this comment.
Clarify the relevance of this change to the PR.
This addition ignores the docs/superpowers/* directory, but the PR objectives focus on adding AWS SigV4 request signing support. It's unclear why this unrelated gitignore entry is included in this PR. Consider moving unrelated changes to a separate PR to keep the scope focused.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.gitignore around lines 38 - 40, The new .gitignore entry
"docs/superpowers/*" is unrelated to the SigV4 signing work; either remove that
line from this PR or revert it and place it in a separate, focused PR that
documents why docs/superpowers should be ignored. Locate the ".gitignore" change
that adds "docs/superpowers/*", revert that single entry from this branch (or
move it into a new branch/PR) and keep this PR limited to the AWS SigV4 request
signing changes only.
Comment on lines
+94
to
+98
| ``` | ||
| https://abc123.execute-api.us-east-1.amazonaws.com/... | ||
| ^^^^^^^^^ | ||
| this is your --aws-region | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
The block on Line 94 is missing a language specifier (markdownlint MD040).
Proposed fix
-```
+```text
https://abc123.execute-api.us-east-1.amazonaws.com/...
^^^^^^^^^
this is your --aws-region</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 94-94: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/tutorials/aws-sigv4-auth.md` around lines 94 - 98, The fenced code block
that contains the URL "https://abc123.execute-api.us-east-1.amazonaws.com/..."
is missing a language tag which triggers markdownlint MD040; update that fence
(the triple-backtick that opens the block) to include a language specifier such
as "text" (e.g., change ``` to ```text) so the snippet is properly labeled and
lint-clean.
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
1 participant
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.
Add a request_signer plugin category with SigV4 implementation using botocore. Transports sign requests via _sign_if_needed() hook, replacing Bearer token auth when --auth-type is set. Includes CLI options (--auth-type, --aws-region, --aws-service, --aws-profile), config validation, plugin registry entries, and full test coverage.
Summary by CodeRabbit
Release Notes
New Features
--auth-type,--aws-region,--aws-service, and--aws-profilefor configuring SigV4 authentication.pip install aiperf[aws].Documentation