feat: add SigV4 auth generator for AWS Bedrock provider#233
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: noalimoy The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (11)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughAdds AWS Bedrock SigV4 support and plumbing to propagate per-request endpoint and request-body data for signing. Introduces a new SigV4AuthGenerator that computes SigV4 headers (Authorization, X-Amz-Date, X-Amz-Content-Sha256, optional X-Amz-Security-Token) using aws-sdk-go-v2. The apikey-injection plugin gains a data-enricher mechanism and registers a Bedrock enricher that injects Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Security Observations
Prioritized action items:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
🧹 Nitpick comments (3)
go.mod (1)
19-19: ⚡ Quick win
github.com/aws/aws-sdk-go-v2should not be// indirect.
pkg/plugins/apikey-injection/auth/sigv4_auth_generator.godirectly importsgithub.com/aws/aws-sdk-go-v2/awsandgithub.com/aws/aws-sdk-go-v2/aws/signer/v4, both of which are packages within the rootgithub.com/aws/aws-sdk-go-v2module. The// indirectannotation is only correct for transitive dependencies not directly imported in any module source file. Runninggo mod tidywill strip// indirectfrom this entry.- github.com/aws/aws-sdk-go-v2 v1.41.7 // indirect + github.com/aws/aws-sdk-go-v2 v1.41.7🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@go.mod` at line 19, The go.mod entry for github.com/aws/aws-sdk-go-v2 is marked as // indirect but pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go directly imports github.com/aws/aws-sdk-go-v2/aws and github.com/aws/aws-sdk-go-v2/aws/signer/v4; remove the incorrect // indirect annotation by updating the module requirements (run `go mod tidy` or manually remove the // indirect comment) so github.com/aws/aws-sdk-go-v2 is recorded as a direct dependency, ensuring the imports in sigv4_auth_generator.go are reflected in go.mod.pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go (2)
93-96: ⚡ Quick win
strings.NewReader(body)is an unnecessary allocation —SignHTTPnever readsr.Bodywhen an explicitpayloadHashis supplied.
v4.SignHTTPuses thebodyHashargument directly for the canonical request;r.Bodyis only accessed for streaming (unsigned-payload). Passhttp.NoBodyinstead.♻️ Proposed refactor
- req, err := http.NewRequest(http.MethodPost, "https://"+endpoint+path, strings.NewReader(body)) + req, err := http.NewRequest(http.MethodPost, "https://"+endpoint+path, http.NoBody)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go` around lines 93 - 96, Replace the unnecessary allocation of strings.NewReader(body) when constructing the HTTP request for signing with http.NoBody since v4.SignHTTP uses the supplied payloadHash and does not read req.Body in this code path; update the request creation in the block that builds req (the code that currently calls http.NewRequest with strings.NewReader(body)) to pass http.NoBody, leaving the payloadHash / bodyHash usage and the subsequent call to v4.SignHTTP unchanged.
38-39: ⚡ Quick win
defaultPath = "/v1/chat/completions"is OpenAI-compatible — wrong default for native BedrockInvokeModel/Conversepaths.Native Bedrock API paths follow
/model/{modelId}/invokeor/model/{modelId}/converse. If the:pathpseudo-header is absent, every native Bedrock request silently falls back to an incorrect path and receives a 404 with no hint that the fallback was triggered. Either remove the default and return an error when_pathis empty, or document explicitly that this generator targets the Bedrock OpenAI-compatible endpoint only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go` around lines 38 - 39, The current defaultPath = "/v1/chat/completions" is OpenAI-compatible and causes native Bedrock requests to silently fall back to the wrong path; remove this hardcoded OpenAI path and make the generator fail fast when the :path pseudo-header is empty. Specifically, eliminate or set defaultPath to an empty string and update the sigv4 header generation logic that reads the :path (the code referencing defaultPath/_path) to return an explicit error when _path is blank (or document that the generator only targets the OpenAI-compatible endpoint and validate accordingly). Ensure error text clearly references the missing :path so callers can correct requests instead of receiving a silent 404.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/plugins/apikey-injection/auth/sigv4_auth_generator_test.go`:
- Around line 112-126: Add two test cases to the existing table-driven tests in
sigv4_auth_generator_test.go: one case named like "empty _request_body returns
error" with credentials including "_request_body": "" (and valid
aws-access-key-id and aws-secret-access-key) and wantErrContains set to a
substring asserting the generator rejects an empty request body; and a second
case named like "endpoint with scheme returns invalid AWS endpoint" with
"_endpoint": "https://bedrock-runtime.us-east-1.amazonaws.com" and
wantErrContains set to "invalid AWS endpoint". Place these entries alongside the
existing cases so the test harness (the same Test... function and the existing
name/wantErrContains handling) will exercise and assert the new failure paths.
In `@pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go`:
- Around line 93-96: The request builder blindly prepends "https://" to endpoint
(used in the http.NewRequest calls) which breaks when endpoint already includes
a scheme; update regionFromEndpoint to either validate and reject endpoints
containing a scheme or normalize the endpoint by stripping any "http://" or
"https://" (or use url.Parse to extract Host) so that the http.NewRequest calls
(the lines constructing "https://"+endpoint+path and the similar block at
118-123) always get a bare host; ensure you normalize once in regionFromEndpoint
and reuse the cleaned endpoint variable where req is created to avoid
duplicate-scheme URLs.
- Around line 65-68: The code is missing an empty-string check for
enrichedBodyField: in the lookup of body from credentialsData (variable body,
enrichedBodyField, credentialsData in sigv4_auth_generator.go) ensure you
validate both presence and non-empty value (i.e., !ok || body == "") and return
a formatted error like the other validators indicating the required field is
missing/empty (preserve the existing error wording pattern and hint about the
enricher).
In `@pkg/plugins/apikey-injection/plugin.go`:
- Around line 38-42: Update the credentialsEnricherFunc type to return
(map[string]string, error) instead of just map[string]string, and modify all
implementations and call sites (e.g., where credentialsEnricherFunc is
implemented and invoked in this file) to propagate and surface serialization and
missing-field errors instead of silently skipping additions; specifically,
return the json.Marshal error when marshaling request.Body fails and return an
explicit error when CycleState lacks an endpoint, then have the caller check the
error and propagate it to GenerateAuthHeaders so downstream messages reflect the
real cause.
In `@pkg/plugins/model-provider-resolver/external_model_reconciler.go`:
- Around line 67-74: The reconciler is incorrectly reading endpoint from
ExternalModel.spec (using unstructured.NestedString on obj.Object) which is
missing — update the reconcile logic that builds externalModelInfo: first
extract and validate externalProviderRefs from ExternalModel.spec (check the
boolean returns from unstructured.NestedX calls), fetch the referenced
ExternalProvider CR, and then read endpoint, provider and auth from that
ExternalProvider's spec (not from obj.Object); ensure you do not discard the
boolean ok values from unstructured.NestedString/NestedMap and return/record an
error if required fields (endpoint/provider/auth) are missing so
externalModelInfo.endpoint is never empty.
---
Nitpick comments:
In `@go.mod`:
- Line 19: The go.mod entry for github.com/aws/aws-sdk-go-v2 is marked as //
indirect but pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go directly
imports github.com/aws/aws-sdk-go-v2/aws and
github.com/aws/aws-sdk-go-v2/aws/signer/v4; remove the incorrect // indirect
annotation by updating the module requirements (run `go mod tidy` or manually
remove the // indirect comment) so github.com/aws/aws-sdk-go-v2 is recorded as a
direct dependency, ensuring the imports in sigv4_auth_generator.go are reflected
in go.mod.
In `@pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go`:
- Around line 93-96: Replace the unnecessary allocation of
strings.NewReader(body) when constructing the HTTP request for signing with
http.NoBody since v4.SignHTTP uses the supplied payloadHash and does not read
req.Body in this code path; update the request creation in the block that builds
req (the code that currently calls http.NewRequest with strings.NewReader(body))
to pass http.NoBody, leaving the payloadHash / bodyHash usage and the subsequent
call to v4.SignHTTP unchanged.
- Around line 38-39: The current defaultPath = "/v1/chat/completions" is
OpenAI-compatible and causes native Bedrock requests to silently fall back to
the wrong path; remove this hardcoded OpenAI path and make the generator fail
fast when the :path pseudo-header is empty. Specifically, eliminate or set
defaultPath to an empty string and update the sigv4 header generation logic that
reads the :path (the code referencing defaultPath/_path) to return an explicit
error when _path is blank (or document that the generator only targets the
OpenAI-compatible endpoint and validate accordingly). Ensure error text clearly
references the missing :path so callers can correct requests instead of
receiving a silent 404.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: d8f35154-b2b3-4a4e-b3e6-8d9ae8e1ab8c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (11)
go.modpkg/plugins/apikey-injection/auth/sigv4_auth_generator.gopkg/plugins/apikey-injection/auth/sigv4_auth_generator_test.gopkg/plugins/apikey-injection/plugin.gopkg/plugins/apikey-injection/plugin_test.gopkg/plugins/common/provider/provider.gopkg/plugins/common/state/state-keys.gopkg/plugins/model-provider-resolver/external_model_reconciler.gopkg/plugins/model-provider-resolver/model_store.gopkg/plugins/model-provider-resolver/plugin.gopkg/plugins/model-provider-resolver/plugin_test.go
| { | ||
| name: "empty access key returns error", | ||
| credentials: map[string]string{ | ||
| "aws-access-key-id": "", | ||
| "aws-secret-access-key": "secret", | ||
| "_request_body": "{}", | ||
| "_endpoint": "bedrock-runtime.us-east-1.amazonaws.com", | ||
| }, | ||
| wantErrContains: "aws-access-key-id", | ||
| }, | ||
| { | ||
| name: "empty credentials returns error", | ||
| credentials: map[string]string{}, | ||
| wantErrContains: "aws-access-key-id", | ||
| }, |
There was a problem hiding this comment.
Missing test case for _request_body present-but-empty, and endpoint containing a URL scheme.
Two reachable paths are untested:
_request_body: ""— currently passes validation silently (the bug flagged in the generator); once fixed, a test should assert the error._endpoint: "https://bedrock-runtime.us-east-1.amazonaws.com"— with a URL scheme — should return an"invalid AWS endpoint"error once the scheme guard is added.
➕ Suggested additional test cases
+ {
+ name: "empty request body returns error",
+ credentials: map[string]string{
+ "aws-access-key-id": "AKIA...",
+ "aws-secret-access-key": "secret",
+ "_request_body": "",
+ "_endpoint": "bedrock-runtime.us-east-1.amazonaws.com",
+ },
+ wantErrContains: "_request_body",
+ },
+ {
+ name: "endpoint with URL scheme returns error",
+ credentials: map[string]string{
+ "aws-access-key-id": "AKIA...",
+ "aws-secret-access-key": "secret",
+ "_request_body": "{}",
+ "_endpoint": "https://bedrock-runtime.us-east-1.amazonaws.com",
+ },
+ wantErrContains: "invalid AWS endpoint",
+ },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/plugins/apikey-injection/auth/sigv4_auth_generator_test.go` around lines
112 - 126, Add two test cases to the existing table-driven tests in
sigv4_auth_generator_test.go: one case named like "empty _request_body returns
error" with credentials including "_request_body": "" (and valid
aws-access-key-id and aws-secret-access-key) and wantErrContains set to a
substring asserting the generator rejects an empty request body; and a second
case named like "endpoint with scheme returns invalid AWS endpoint" with
"_endpoint": "https://bedrock-runtime.us-east-1.amazonaws.com" and
wantErrContains set to "invalid AWS endpoint". Place these entries alongside the
existing cases so the test harness (the same Test... function and the existing
name/wantErrContains handling) will exercise and assert the new failure paths.
| body, ok := credentialsData[enrichedBodyField] | ||
| if !ok { | ||
| return nil, fmt.Errorf("credentials missing required field %s (enricher not applied?)", enrichedBodyField) | ||
| } |
There was a problem hiding this comment.
_request_body empty-string check is missing — inconsistent with all other field validators.
Every other required field uses !ok || value == "", but the body only checks !ok. An empty _request_body silently proceeds, computes the SHA-256 of "" (e3b0c44...), and produces a well-formed but wrong signed request that AWS will reject with a 400, making the bug hard to trace back to this point.
🐛 Proposed fix
- body, ok := credentialsData[enrichedBodyField]
- if !ok {
+ body, ok := credentialsData[enrichedBodyField]
+ if !ok || body == "" {
return nil, fmt.Errorf("credentials missing required field %s (enricher not applied?)", enrichedBodyField)
}📝 Committable suggestion
‼️ 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.
| body, ok := credentialsData[enrichedBodyField] | |
| if !ok { | |
| return nil, fmt.Errorf("credentials missing required field %s (enricher not applied?)", enrichedBodyField) | |
| } | |
| body, ok := credentialsData[enrichedBodyField] | |
| if !ok || body == "" { | |
| return nil, fmt.Errorf("credentials missing required field %s (enricher not applied?)", enrichedBodyField) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go` around lines 65 -
68, The code is missing an empty-string check for enrichedBodyField: in the
lookup of body from credentialsData (variable body, enrichedBodyField,
credentialsData in sigv4_auth_generator.go) ensure you validate both presence
and non-empty value (i.e., !ok || body == "") and return a formatted error like
the other validators indicating the required field is missing/empty (preserve
the existing error wording pattern and hint about the enricher).
| endpoint, _, _ := unstructured.NestedString(obj.Object, "spec", "endpoint") | ||
| credsName, _, _ := unstructured.NestedString(obj.Object, "spec", "credentialRef", "name") | ||
|
|
||
| // targetModel is the model that will be used in the request body when getting inference requests. | ||
| info := &externalModelInfo{ | ||
| provider: provider, | ||
| targetModel: targetModel, | ||
| endpoint: endpoint, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether ExternalModel has a spec.endpoint field in its API type definition.
fd -i 'externalmodel_types.go' --exec cat {}Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 4391
🏁 Script executed:
#!/bin/bash
# Check the external_model_reconciler.go file around the problematic lines
fd -i 'external_model_reconciler.go' --exec head -n 90 {} | tail -n 35Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 1592
🏁 Script executed:
#!/bin/bash
# Find and examine the ExternalProvider type definition to confirm endpoint field exists
fd -i 'externalprovider_types.go' --exec cat {}Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 4098
Critical: Endpoint will always be empty — reconciler reads from non-existent ExternalModel.spec.endpoint instead of referenced ExternalProvider.
ExternalModel.spec contains only externalProviderRefs; it has no endpoint, provider, or targetModel fields. Line 67 reads from spec.endpoint which doesn't exist, and unstructured.NestedString() silently returns ("", false, nil) for missing fields. The code discards the boolean return value (_), hiding this failure completely. An empty endpoint propagates to the model store and breaks all Bedrock SigV4 signing at runtime with "credentials missing required field _endpoint" — no log-level indication at the reconciler.
The Endpoint field is defined on ExternalProvider (spec.endpoint, kubebuilder-validated required). The reconciler must:
- Extract
externalProviderRefsfromExternalModel.spec - Fetch the referenced
ExternalProviderCR - Read
endpoint,provider, andauthfrom theExternalProvider, not fromExternalModel
Additionally, discarding the boolean return value violates the GO SECURITY guideline: CR spec fields must be validated before use (CWE-754: Improper Check for Unusual Conditions).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/plugins/model-provider-resolver/external_model_reconciler.go` around
lines 67 - 74, The reconciler is incorrectly reading endpoint from
ExternalModel.spec (using unstructured.NestedString on obj.Object) which is
missing — update the reconcile logic that builds externalModelInfo: first
extract and validate externalProviderRefs from ExternalModel.spec (check the
boolean returns from unstructured.NestedX calls), fetch the referenced
ExternalProvider CR, and then read endpoint, provider and auth from that
ExternalProvider's spec (not from obj.Object); ensure you do not discard the
boolean ok values from unstructured.NestedString/NestedMap and return/record an
error if required fields (endpoint/provider/auth) are missing so
externalModelInfo.endpoint is never empty.
|
while adding E2E coverage for since or if native Bedrock endpoints ( What do you think? |
|
/group-test |
|
/cc: @yossiovadia |
yossiovadia
left a comment
yossiovadia
left a comment
There was a problem hiding this comment.
Hey Noa, a few things to address before this can merge.
1. Missing api-translation registration
aws-bedrock isn't registered in the api-translation plugin. Since api-translation runs before apikey-injection in the plugin chain, any aws-bedrock request will fail at translation before reaching the signing logic. This needs either a translator registration in this PR or a clear plan for when it lands.
2. Endpoint source will change with #183/#184
The reconciler reads spec.endpoint from the maas.opendatahub.io ExternalModel. PRs #183/#184 (currently in review) move endpoint to ExternalProvider — a separate CRD. Once those merge, the endpoint threading here will need to change (read from the provider store instead of the model reconciler).
Since this PR can't work end-to-end without the translator anyway, it might be easier to rebase after #183/#184 land rather than reworking the wiring twice.
3. Region extraction from hostname
regionFromEndpoint parses region from the hostname (bedrock-runtime.us-east-1.amazonaws.com → us-east-1). This works for standard endpoints but breaks for VPC endpoints or custom configurations. In #234 we aligned with Nir on using the config map for provider-specific settings like region — that would be more robust than hostname parsing.
4. Body-signing serialization assumption
The enricher does json.Marshal(request.Body) to get the bytes for SigV4 signing. The framework independently serializes the body when sending to the upstream. If the two serializations differ (key ordering, whitespace), AWS will reject with SignatureDoesNotMatch. Go's json.Marshal sorts map keys, so it likely matches, but this is an implicit coupling worth documenting in a comment.
5. Silent error in enricher
enrichBedrockCredentials returns map[string]string with no error. If json.Marshal fails, the _request_body field is silently missing, and the downstream error message ("enricher not applied?") is misleading. Consider returning an error or at minimum logging the marshal failure.
6. bedrockService hardcoded
Ties into Nir's "no hardcoding" feedback on #234. The service signing name could come from the provider config map rather than a constant. Not blocking for this PR but worth noting for alignment.
|
adding also Noy to review: |
|
thanks @yossiovadia 1. Missing api-translation registration 2. Endpoint source will change 3. Region extraction from hostname 4 & 5. Body-signing + silent error in enricher 6. |
noalimoy
force-pushed
the
feat/aws-bedrock-sigv4-auth
branch
from
6d32c7a to
fadd059
Compare
github-actions
Bot
added
size/XL
|
Your PR is large. Please consider breaking it into multiple PRs. The |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go`:
- Line 93: The request is created with http.NewRequest which doesn't propagate
context; change the call to http.NewRequestWithContext(ctx, http.MethodPost,
"https://"+endpoint+path, strings.NewReader(body)) so the context is
honored—ensure a context.Context named ctx is available in the scope (add a ctx
parameter to the enclosing method if needed) and keep the existing error
handling for req, err unchanged; update the instantiation that currently assigns
to req, err to use NewRequestWithContext and propagate ctx through any
downstream request lifecycle.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 193e15fd-1703-491d-a2b8-9ec4628628af
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (11)
go.modpkg/plugins/apikey-injection/auth/sigv4_auth_generator.gopkg/plugins/apikey-injection/auth/sigv4_auth_generator_test.gopkg/plugins/apikey-injection/plugin.gopkg/plugins/apikey-injection/plugin_test.gopkg/plugins/common/provider/provider.gopkg/plugins/common/state/state-keys.gopkg/plugins/model-provider-resolver/external_model_reconciler.gopkg/plugins/model-provider-resolver/model_store.gopkg/plugins/model-provider-resolver/plugin.gopkg/plugins/model-provider-resolver/plugin_test.go
✅ Files skipped from review due to trivial changes (5)
- pkg/plugins/common/state/state-keys.go
- pkg/plugins/model-provider-resolver/model_store.go
- pkg/plugins/model-provider-resolver/plugin_test.go
- go.mod
- pkg/plugins/apikey-injection/plugin_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/plugins/model-provider-resolver/plugin.go
- pkg/plugins/model-provider-resolver/external_model_reconciler.go
- pkg/plugins/common/provider/provider.go
- pkg/plugins/apikey-injection/plugin.go
- pkg/plugins/apikey-injection/auth/sigv4_auth_generator_test.go
| path = defaultPath | ||
| } | ||
|
|
||
| req, err := http.NewRequest(http.MethodPost, "https://"+endpoint+path, strings.NewReader(body)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether non-context request builders are still used in this file.
rg -n 'http\.NewRequest\(' pkg/plugins/apikey-injection/auth/sigv4_auth_generator.goRepository: opendatahub-io/ai-gateway-payload-processing
Length of output: 188
🏁 Script executed:
# Read the function containing line 93 to understand its signature and context availability
sed -n '80,115p' pkg/plugins/apikey-injection/auth/sigv4_auth_generator.goRepository: opendatahub-io/ai-gateway-payload-processing
Length of output: 1132
🏁 Script executed:
# Also check the full function signature to see if context is a parameter
rg -B 20 'http\.NewRequest\(' pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go | head -30Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 629
🏁 Script executed:
# Check imports to see what's available
head -20 pkg/plugins/apikey-injection/auth/sigv4_auth_generator.goRepository: opendatahub-io/ai-gateway-payload-processing
Length of output: 697
🏁 Script executed:
# Get the complete function signature
sed -n '50,100p' pkg/plugins/apikey-injection/auth/sigv4_auth_generator.goRepository: opendatahub-io/ai-gateway-payload-processing
Length of output: 2121
🏁 Script executed:
# Also search for the function definition
rg -B 30 'http\.NewRequest\(' pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go | grep -E '(func|GenerateAuthHeaders)'Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 70
🏁 Script executed:
# Check file structure to understand context
head -50 pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go | grep -E '(type|package|controller|kubernetes)'Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 134
🏁 Script executed:
# Check if this is used in a controller context
rg -l 'GenerateAuthHeaders' --type go | head -5Repository: opendatahub-io/ai-gateway-payload-processing
Length of output: 371
Use context-bound request construction at Line 93
http.NewRequest bypasses context propagation. Switch to http.NewRequestWithContext for proper lifecycle handling.
Proposed fix
- req, err := http.NewRequest(http.MethodPost, "https://"+endpoint+path, strings.NewReader(body))
+ req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "https://"+endpoint+path, strings.NewReader(body))📝 Committable suggestion
‼️ 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.
| req, err := http.NewRequest(http.MethodPost, "https://"+endpoint+path, strings.NewReader(body)) | |
| req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "https://"+endpoint+path, strings.NewReader(body)) |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 93-93: net/http.NewRequest must not be called. use net/http.NewRequestWithContext
(noctx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/plugins/apikey-injection/auth/sigv4_auth_generator.go` at line 93, The
request is created with http.NewRequest which doesn't propagate context; change
the call to http.NewRequestWithContext(ctx, http.MethodPost,
"https://"+endpoint+path, strings.NewReader(body)) so the context is
honored—ensure a context.Context named ctx is available in the scope (add a ctx
parameter to the enclosing method if needed) and keep the existing error
handling for req, err unchanged; update the instantiation that currently assigns
to req, err to use NewRequestWithContext and propagate ctx through any
downstream request lifecycle.
- Add SigV4AuthGenerator implementing AuthHeadersGenerator, using AWS SDK v2 signer to produce Authorization, X-Amz-Date, X-Amz-Content-Sha256, and X-Amz-Security-Token headers - Introduce dataEnrichers map on ApiKeyInjectionPlugin to enrich the credentials map with per-request data (body, endpoint) before signing, keeping the AuthHeadersGenerator interface unchanged - Thread ExternalModel.spec.endpoint through reconciler → modelInfoStore → CycleState → enricher → SigV4 signer, and extract the AWS region from the endpoint hostname - Add provider constant aws-bedrock and CycleState key endpoint - Add unit tests for SigV4 signing, region extraction, credential enrichment, session token handling, and missing-endpoint error path
noalimoy
force-pushed
the
feat/aws-bedrock-sigv4-auth
branch
from
fadd059 to
ab745b6
Compare
|
Your PR is large. Please consider breaking it into multiple PRs. The |
|
@noalimoy: The following test has Succeeded: OCI Artifact Browser URLInspecting Test Artifacts ManuallyTo inspect your test artifacts manually, follow these steps:
mkdir -p oras-artifacts
cd oras-artifacts
oras pull quay.io/opendatahub/odh-ci-artifacts:ai-gateway-group-test-gfwdz |
openshift-ci
Bot
added
the
needs-rebase
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hey @noalimoy , have not forgotten about this one. |
|
@noalimoy can we rebase this one and move the PR from draft to ready state? |
5 participants
Summary
Adds AWS SigV4 authentication support to the
apikey-injectionplugin, enabling requests to AWS Bedrock endpoints that require Signature Version 4 signing.Closes #59
What this does
SigV4AuthGenerator— implementsAuthHeadersGenerator, uses AWS SDK v2 to sign requests and produceAuthorization,X-Amz-Date,X-Amz-Content-Sha256, and (optionally)X-Amz-Security-TokenheadersdataEnrichersmap toApiKeyInjectionPluginthat injects per-request data (body, endpoint, path) into the credentials map before signing.the
AuthHeadersGeneratorinterface stays unchanged and existing providers are unaffectedExternalModel.spec.endpointvia the reconciler →modelInfoStore→CycleState, and extracts the AWS region from the hostname (e.g.bedrock-runtime.us-east-1.amazonaws.com→us-east-1)Design notes
this lives inside
apikey-injection(not a separate plugin),sources endpoint from
ExternalModel.spec.endpoint(not from:authorityheader),and will adapt naturally once
ExternalProviderreconcilers land.Changed files
auth/sigv4_auth_generator.goplugin.godataEnrichersmap +enrichBedrockCredentialsplugin_test.goauth/sigv4_auth_generator_test.gocommon/provider/provider.goAWSBedrock = "aws-bedrock"constantcommon/state/state-keys.goEndpointKeyconstantmodel-provider-resolver/*endpointfrom CR → store → CycleStateHow has this been tested
SigV4AuthGenerator: verifies header format, body hash, session token, region extraction, and error pathsplugin_test.go: fullProcessRequestflow with secret store, enricher, and SigV4 signingmodel-provider-resolverplugin test: validates endpoint is written to CycleStatego test ./...)Summary by CodeRabbit
New Features
Tests