HYPERLFEET-1134 - feat: add configurable caller identity for audit attribution#187
Open
rh-amarin wants to merge 3 commits into
Open
HYPERLFEET-1134 - feat: add configurable caller identity for audit attribution#187rh-amarin wants to merge 3 commits into
rh-amarin wants to merge 3 commits into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR implements header-first caller identity resolution for audit attribution with JWT-claim fallback, adds configuration/flags/env bindings and docs, introduces CallerIdentityMiddleware and header-name validation, shifts routing to rely on a global ResolveCallerIdentity middleware (route registration signatures removed), updates services to use actorFromContext for CreatedBy/UpdatedBy, and adds unit and integration tests plus Helm/chart updates. Sequence Diagram(s)sequenceDiagram
participant Client
participant apiV1Router
participant ResolveCallerIdentity
participant CallerIdentityFromRequest
participant GetIdentityFromContext
participant ClusterService
participant DB
Client->>apiV1Router: HTTP request (JWT + optional identity header)
apiV1Router->>ResolveCallerIdentity: middleware invocation
ResolveCallerIdentity->>CallerIdentityFromRequest: read configured header (if enabled)
alt header present and valid
CallerIdentityFromRequest-->>ResolveCallerIdentity: header identity
else header absent
CallerIdentityFromRequest->>GetIdentityFromContext: extract configured JWT claim
GetIdentityFromContext-->>CallerIdentityFromRequest: claim value or error
end
ResolveCallerIdentity->>ClusterService: downstream handler with username in context
ClusterService->>DB: create cluster with CreatedBy = actorFromContext(ctx)
🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 943 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/hyperfleet-api/environments cmd/hyperfleet-api/server pkg/services plugins/clusters plugins/nodePools test | +1 |
Computed by hyperfleet-risk-scorer
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/server_test.go (1)
9-75: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winConvert these multi-case validations to a table-driven test.
Both
TestJWTConfig_ValidateandTestIdentityHeaderConfig_Validatenow contain multiple scenarios; converting to table-driven structure will reduce repetition and make new cases safer to add consistently.As per coding guidelines
**/*_test.go: "Table-driven tests used for multiple cases".🤖 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/config/server_test.go` around lines 9 - 75, The tests TestJWTConfig_Validate and TestIdentityHeaderConfig_Validate are written as many separate subtests; convert them into table-driven tests to reduce repetition and follow the repository guideline. Replace the multiple t.Run cases in TestJWTConfig_Validate with a single loop over a slice of structs (name, cfg JWTConfig, wantErr bool, wantMsg string) that calls cfg.Validate() and asserts accordingly (use JWTConfig.Validate as the target), and do the same for TestIdentityHeaderConfig_Validate using a slice of structs (name, cfg IdentityHeaderConfig, wantErr bool, wantMsg string) that invokes IdentityHeaderConfig.Validate; keep the existing case names and expected assertions (ContainSubstring checks) as the table entries so behavior remains identical.
🤖 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 `@charts/values.yaml`:
- Around line 57-59: The new configurable identity header (identity_header.name
/ X-HyperFleet-Identity) must be added to the default masking list so identity
values are not logged; update the default config key
config.logging.masking.headers to include "X-HyperFleet-Identity" (or reference
identity_header.name dynamically where configuration is composed) so
header-based attribution will be masked by default.
In `@pkg/auth/context_test.go`:
- Around line 11-52: Refactor TestGetIdentityFromContext into a table-driven
test: replace the four separate t.Run blocks with a single test table (slice of
structs) containing fields like name, claims (use
contextWithClaims(jwt.MapClaims{...})), identityField (the configured claim
string), want (expected identity string) and wantErr (bool); then iterate over
the table calling t.Run(tc.name, func(t *testing.T) { RegisterTestingT(t);
identity, err := GetIdentityFromContext(ctx, tc.identityField); assert expected
results depending on tc.wantErr and tc.want (use Expect/ContainSubstring for
error text when wantErr is true) }); ensure you reference and reuse
contextWithClaims and GetIdentityFromContext and preserve all existing case
expectations (including fallback/default behavior and error containing the
missing claim).
In `@pkg/auth/identity_test.go`:
- Around line 12-81: Add a test that asserts
normalizeIdentityHeaderValue/CallerIdentityFromRequest rejects header values
longer than maxCallerIdentityLen (256). In the TestCallerIdentityFromRequest
suite add a new t.Run case that constructs a request with Header
"X-HyperFleet-Identity" set to a string of length 257, uses a
CallerIdentityConfig with HeaderEnabled true and HeaderName
"X-HyperFleet-Identity", calls CallerIdentityFromRequest(r.Context(), r, cfg)
and expects an error (Expect(err).To(HaveOccurred())). This verifies the max
length boundary enforced by normalizeIdentityHeaderValue.
---
Outside diff comments:
In `@pkg/config/server_test.go`:
- Around line 9-75: The tests TestJWTConfig_Validate and
TestIdentityHeaderConfig_Validate are written as many separate subtests; convert
them into table-driven tests to reduce repetition and follow the repository
guideline. Replace the multiple t.Run cases in TestJWTConfig_Validate with a
single loop over a slice of structs (name, cfg JWTConfig, wantErr bool, wantMsg
string) that calls cfg.Validate() and asserts accordingly (use
JWTConfig.Validate as the target), and do the same for
TestIdentityHeaderConfig_Validate using a slice of structs (name, cfg
IdentityHeaderConfig, wantErr bool, wantMsg string) that invokes
IdentityHeaderConfig.Validate; keep the existing case names and expected
assertions (ContainSubstring checks) as the table entries so behavior remains
identical.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: ea0bcb02-ba3c-472a-9bea-f398472f6da5
📒 Files selected for processing (28)
charts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/server/routes.goconfigs/config.yaml.exampledocs/authentication.mddocs/config.mdpkg/auth/auth_middleware.gopkg/auth/auth_middleware_mock.gopkg/auth/context.gopkg/auth/context_test.gopkg/auth/identity.gopkg/auth/identity_test.gopkg/config/flags.gopkg/config/loader.gopkg/config/loader_test.gopkg/config/logging.gopkg/config/server.gopkg/config/server_test.gopkg/services/cluster.gopkg/services/node_pool.gopkg/services/util.goplugins/clusters/plugin.goplugins/nodePools/plugin.gotest/helper.gotest/integration/caller_identity_test.gotest/integration/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/auth/auth_middleware_mock.go
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/server_test.go (1)
9-43: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse table-driven tests for these multi-scenario validation suites.
Both validation tests encode multiple cases and should be table-driven to keep config validation coverage concise and easier to extend safely.
As per coding guidelines
**/*_test.go: "Table-driven tests used for multiple cases".Also applies to: 45-75
🤖 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/config/server_test.go` around lines 9 - 43, TestJWTConfig_Validate currently repeats multiple subtests; convert it to a single table-driven test by defining a slice of cases (struct with name, cfg JWTConfig, wantError bool, wantErrContains string) and loop over them calling t.Run(case.name, func(t *testing.T) { ... }); inside each iteration call cfg.Validate(), assert success or error using Expect based on wantError and, if expecting an error, check err.Error() contains wantErrContains; remove the repeated RegisterTestingT calls and consolidate the four existing subtests (including the cases described in TestJWTConfig_Validate) into this table so new cases can be added easily.
♻️ Duplicate comments (1)
pkg/auth/context_test.go (1)
11-52: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winConvert this multi-case test to a table-driven test.
This test has multiple scenarios and is still structured as repeated
t.Runblocks, which makes extension and maintenance harder.As per coding guidelines
**/*_test.go: "Table-driven tests used for multiple cases".🤖 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/auth/context_test.go` around lines 11 - 52, Replace the repeated t.Run blocks in TestGetIdentityFromContext with a single table-driven loop: define a slice of structs (fields: name, claims jwt.MapClaims, field string, want string, wantErr bool, wantErrSubstring string) that covers the four scenarios, call RegisterTestingT(t) once at the top, then range over cases and call t.Run(tc.name, func(t *testing.T) { ctx := contextWithClaims(tc.claims); got, err := GetIdentityFromContext(ctx, tc.field); if tc.wantErr { Expect(err).To(HaveOccurred()); if tc.wantErrSubstring != "" { Expect(err.Error()).To(ContainSubstring(tc.wantErrSubstring)) } } else { Expect(err).NotTo(HaveOccurred()); Expect(got).To(Equal(tc.want)) } }); this keeps references to GetIdentityFromContext and contextWithClaims for locating the logic and removes the repeated RegisterTestingT calls.
🤖 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 `@charts/values.yaml`:
- Around line 55-59: The Helm chart was not version-bumped and the release NOTES
were not updated to reflect the new identity header settings added in
values.yaml; update charts/Chart.yaml to increment the chart version (and
appVersion if applicable) and update charts/templates/NOTES.txt to document the
new config keys (config.server.jwt.identity_claim and
config.server.identity_header.enabled/name), state the default
(identity_header.enabled: false) and the default header name
(X-HyperFleet-Identity), and include a short note about how enabling it affects
incoming requests and how to override via values.yaml or --set.
In `@pkg/auth/auth_middleware.go`:
- Around line 21-29: NewCallerIdentityMiddleware currently lets callers set
HeaderName values that IdentityHeaderConfig.Validate() would reject (e.g.,
"Authorization"); update NewCallerIdentityMiddleware to enforce the same
validation: after setting cfg.JWTIdentityClaim default and before returning
callerIdentityMiddleware, if cfg.HeaderEnabled run the IdentityHeaderConfig
validation (or call IdentityHeaderConfig.Validate()) on the header settings and
return an error if validation fails (preserving the existing error message about
required name), so callers cannot bypass forbidden header names; ensure the
function still returns &callerIdentityMiddleware{cfg: cfg}, nil when validation
passes.
In `@pkg/auth/identity_test.go`:
- Around line 12-81: Refactor TestCallerIdentityFromRequest into a table-driven
test: replace the four separate t.Run blocks with a single slice of test cases
(each case including name, request setup values, ctx claims via
contextWithClaims / jwt.MapClaims when needed, CallerIdentityConfig fields,
expected identity or expected error) and iterate with for _, tc := range cases {
t.Run(tc.name, func(t *testing.T) { ... }) }, calling CallerIdentityFromRequest
and asserting results; remove redundant RegisterTestingT calls and keep
references to CallerIdentityFromRequest, CallerIdentityConfig,
contextWithClaims, and jwt.MapClaims to build each case (header overrides JWT,
falls back to JWT, rejects invalid header, header only when JWT disabled).
---
Outside diff comments:
In `@pkg/config/server_test.go`:
- Around line 9-43: TestJWTConfig_Validate currently repeats multiple subtests;
convert it to a single table-driven test by defining a slice of cases (struct
with name, cfg JWTConfig, wantError bool, wantErrContains string) and loop over
them calling t.Run(case.name, func(t *testing.T) { ... }); inside each iteration
call cfg.Validate(), assert success or error using Expect based on wantError
and, if expecting an error, check err.Error() contains wantErrContains; remove
the repeated RegisterTestingT calls and consolidate the four existing subtests
(including the cases described in TestJWTConfig_Validate) into this table so new
cases can be added easily.
---
Duplicate comments:
In `@pkg/auth/context_test.go`:
- Around line 11-52: Replace the repeated t.Run blocks in
TestGetIdentityFromContext with a single table-driven loop: define a slice of
structs (fields: name, claims jwt.MapClaims, field string, want string, wantErr
bool, wantErrSubstring string) that covers the four scenarios, call
RegisterTestingT(t) once at the top, then range over cases and call
t.Run(tc.name, func(t *testing.T) { ctx := contextWithClaims(tc.claims); got,
err := GetIdentityFromContext(ctx, tc.field); if tc.wantErr {
Expect(err).To(HaveOccurred()); if tc.wantErrSubstring != "" {
Expect(err.Error()).To(ContainSubstring(tc.wantErrSubstring)) } } else {
Expect(err).NotTo(HaveOccurred()); Expect(got).To(Equal(tc.want)) } }); this
keeps references to GetIdentityFromContext and contextWithClaims for locating
the logic and removes the repeated RegisterTestingT calls.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 44eae421-c975-4040-adbf-3ae93496bc77
📒 Files selected for processing (28)
charts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/server/routes.goconfigs/config.yaml.exampledocs/authentication.mddocs/config.mdpkg/auth/auth_middleware.gopkg/auth/auth_middleware_mock.gopkg/auth/context.gopkg/auth/context_test.gopkg/auth/identity.gopkg/auth/identity_test.gopkg/config/flags.gopkg/config/loader.gopkg/config/loader_test.gopkg/config/logging.gopkg/config/server.gopkg/config/server_test.gopkg/services/cluster.gopkg/services/node_pool.gopkg/services/util.goplugins/clusters/plugin.goplugins/nodePools/plugin.gotest/helper.gotest/integration/caller_identity_test.gotest/integration/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/auth/auth_middleware_mock.go
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/config/server.go`:
- Around line 8-9: The config package should not import auth; move the
IsForbiddenIdentityHeaderName function and forbiddenIdentityHeaderNames slice
out of pkg/auth into pkg/config (or a new shared pkg/validation) and update
usages accordingly so auth imports config/validation instead of the reverse;
locate the IsForbiddenIdentityHeaderName and forbiddenIdentityHeaderNames
declarations in your diff, copy them into the chosen package, remove the auth
import from the config package, and update all callers to reference the new
package name.
In `@test/integration/caller_identity_test.go`:
- Around line 24-70: Two near-duplicate tests
(TestCallerIdentityHeaderOverridesJWTOnCreate and
TestCallerIdentityFromJWTWhenHeaderAbsent) exercise the same create flow;
refactor them into a single table-driven test that iterates cases for "header
present" and "header absent". Replace the two test functions with one
TestCallerIdentityCreate table-driven test that builds the same clusterInput,
calls client.PostClusterWithResponse with test.WithAuthToken(ctx) and
conditionally adds test.WithIdentityHeader(test.IdentityHeaderName(),
headerActor) for the header-present case, then asserts resp.StatusCode() is
http.StatusCreated and resp.JSON201.CreatedBy equals either headerActor or
account.Email depending on the case; keep using helpers NewAccount,
NewAuthenticatedContext, PostClusterWithResponse, test.WithAuthToken and
test.WithIdentityHeader to locate code.
- Around line 1-2: This integration test file (package integration) needs the Go
build tag to prevent running during normal `go test`; add the line `//go:build
integration` at the very top of the file (and ensure a blank line after build
tag(s) before the `package integration` declaration); apply the same tag to
other `*_test.go` files in this integration suite if present so they are only
included when explicitly built with the integration tag.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 977ef133-5fee-4d28-86e6-5af1c40ebdab
📒 Files selected for processing (30)
charts/Chart.yamlcharts/templates/NOTES.txtcharts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/server/routes.goconfigs/config.yaml.exampledocs/authentication.mddocs/config.mdpkg/auth/auth_middleware.gopkg/auth/auth_middleware_mock.gopkg/auth/context.gopkg/auth/context_test.gopkg/auth/identity.gopkg/auth/identity_test.gopkg/config/flags.gopkg/config/loader.gopkg/config/loader_test.gopkg/config/logging.gopkg/config/server.gopkg/config/server_test.gopkg/services/cluster.gopkg/services/node_pool.gopkg/services/util.goplugins/clusters/plugin.goplugins/nodePools/plugin.gotest/helper.gotest/integration/caller_identity_test.gotest/integration/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/auth/auth_middleware_mock.go
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/config/server.go (1)
73-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject whitespace-only identity config values.
Line 80 and Line 96 only reject
""; values like" "pass validation, which can silently break audit attribution resolution at runtime. Trim before validating required fields.As per coding guidelines "Configuration changes affect all deployments. Review for: Validation rules in Validate() method".Suggested fix
import ( "fmt" "net" "strconv" + "strings" "time" "github.com/openshift-hyperfleet/hyperfleet-api/pkg/validation" ) @@ func (c *JWTConfig) Validate() error { if !c.Enabled { return nil } if c.IssuerURL == "" { return fmt.Errorf("server.jwt.issuer_url is required when jwt is enabled") } - if c.IdentityClaim == "" { + c.IdentityClaim = strings.TrimSpace(c.IdentityClaim) + if c.IdentityClaim == "" { return fmt.Errorf("server.jwt.identity_claim is required when jwt is enabled") } return nil } @@ func (c *IdentityHeaderConfig) Validate() error { if !c.Enabled { return nil } - if c.Name == "" { + c.Name = strings.TrimSpace(c.Name) + if c.Name == "" { return fmt.Errorf("server.identity_header.name is required when identity_header is enabled") } if validation.IsForbiddenIdentityHeaderName(c.Name) { return fmt.Errorf("server.identity_header.name %q is not allowed", c.Name) } return nil }Also applies to: 92-101
🤖 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/config/server.go` around lines 73 - 84, The Validate method on JWTConfig currently only checks for empty strings, allowing whitespace-only values; update JWTConfig.Validate to trim whitespace (e.g., use strings.TrimSpace) on c.IssuerURL and c.IdentityClaim before testing emptiness so values like " " are rejected, and apply the same trimming-and-empty check pattern to the related Validate block referenced (lines 92-101) for any other required JWT fields to ensure whitespace-only config values fail validation.
♻️ Duplicate comments (1)
test/integration/caller_identity_test.go (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the integration build tag to prevent default test runs from executing this suite.
Line 1 is missing
//go:build integration, so this integration test can run during normalgo test.Proposed fix
+//go:build integration + package integrationAs per coding guidelines
**/*_test.go: Integration tests tagged with //go:build integration.🤖 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 `@test/integration/caller_identity_test.go` at line 1, Add the integration build tag to the top of caller_identity_test.go so it doesn't run in default `go test`; insert `//go:build integration` as the very first line (and optionally add the legacy `// +build integration` line for older Go toolchains) before the `package integration` declaration.
🤖 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/config/server_test.go`:
- Around line 28-42: Replace the two separate t.Run validation cases for
JWTConfig with a single table-driven test: create a slice of test cases
(structs) describing name, JWTConfig fields (Enabled, IssuerURL, IdentityClaim)
and expected error presence/message, then loop over them with t.Run(case.name,
func(t *testing.T){ RegisterTestingT(t); err := case.cfg.Validate(); assert
expected outcome using Expect/ContainSubstring as needed }). Update the tests
referencing JWTConfig and Validate() (including the similar block around lines
45-73) to follow the same table-driven pattern so you have one clean loop that
covers all existing cases.
In `@pkg/validation/identity_header_test.go`:
- Around line 9-13: Replace the single-case TestIsForbiddenIdentityHeaderName
with a table-driven test: create a slice of cases each with a name, input
header, and expected bool, then iterate and call t.Run(case.name, func(t
*testing.T) { RegisterTestingT(t);
Expect(IsForbiddenIdentityHeaderName(case.header)).To(Equal(case.expected)) });
include the two scenarios ("authorization header" -> "Authorization" -> true and
"hyperfleet identity header" -> "X-HyperFleet-Identity" -> false) so failures
are reported per scenario and the test is easier to extend.
---
Outside diff comments:
In `@pkg/config/server.go`:
- Around line 73-84: The Validate method on JWTConfig currently only checks for
empty strings, allowing whitespace-only values; update JWTConfig.Validate to
trim whitespace (e.g., use strings.TrimSpace) on c.IssuerURL and c.IdentityClaim
before testing emptiness so values like " " are rejected, and apply the same
trimming-and-empty check pattern to the related Validate block referenced (lines
92-101) for any other required JWT fields to ensure whitespace-only config
values fail validation.
---
Duplicate comments:
In `@test/integration/caller_identity_test.go`:
- Line 1: Add the integration build tag to the top of caller_identity_test.go so
it doesn't run in default `go test`; insert `//go:build integration` as the very
first line (and optionally add the legacy `// +build integration` line for older
Go toolchains) before the `package integration` declaration.
🪄 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: ASSERTIVE
Plan: Enterprise
Run ID: 30750a01-9819-4a0c-ad8d-99fbfe4dd1fe
📒 Files selected for processing (32)
charts/Chart.yamlcharts/templates/NOTES.txtcharts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/server/routes.goconfigs/config.yaml.exampledocs/authentication.mddocs/config.mdpkg/auth/auth_middleware.gopkg/auth/auth_middleware_mock.gopkg/auth/context.gopkg/auth/context_test.gopkg/auth/identity.gopkg/auth/identity_test.gopkg/config/flags.gopkg/config/loader.gopkg/config/loader_test.gopkg/config/logging.gopkg/config/server.gopkg/config/server_test.gopkg/services/cluster.gopkg/services/node_pool.gopkg/services/util.gopkg/validation/identity_header.gopkg/validation/identity_header_test.goplugins/clusters/plugin.goplugins/nodePools/plugin.gotest/helper.gotest/integration/caller_identity_test.gotest/integration/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/auth/auth_middleware_mock.go
Comment on lines
+28
to
+42
| cfg := JWTConfig{ | ||
| Enabled: true, | ||
| IssuerURL: "https://sso.example.com/auth/realms/test", | ||
| IdentityClaim: "email", | ||
| } | ||
| Expect(cfg.Validate()).To(Succeed()) | ||
| }) | ||
|
|
||
| t.Run("enabled JWT without identity claim fails", func(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| cfg := JWTConfig{Enabled: true, IssuerURL: "https://sso.example.com/auth/realms/test", IdentityClaim: ""} | ||
| err := cfg.Validate() | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err.Error()).To(ContainSubstring("identity_claim")) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Convert these multi-case validation tests to table-driven form.
Both validation blocks encode case matrices with repeated setup/asserts. A single table loop per function will reduce duplication and make future case expansion safer.
As per coding guidelines **/*_test.go: "Table-driven tests used for multiple cases".
Also applies to: 45-73
🤖 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/config/server_test.go` around lines 28 - 42, Replace the two separate
t.Run validation cases for JWTConfig with a single table-driven test: create a
slice of test cases (structs) describing name, JWTConfig fields (Enabled,
IssuerURL, IdentityClaim) and expected error presence/message, then loop over
them with t.Run(case.name, func(t *testing.T){ RegisterTestingT(t); err :=
case.cfg.Validate(); assert expected outcome using Expect/ContainSubstring as
needed }). Update the tests referencing JWTConfig and Validate() (including the
similar block around lines 45-73) to follow the same table-driven pattern so you
have one clean loop that covers all existing cases.
Comment on lines
+9
to
+13
| func TestIsForbiddenIdentityHeaderName(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| Expect(IsForbiddenIdentityHeaderName("Authorization")).To(BeTrue()) | ||
| Expect(IsForbiddenIdentityHeaderName("X-HyperFleet-Identity")).To(BeFalse()) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Convert this into a table-driven, scenario-named test.
This currently packs multiple cases into one implementation-named test, which makes extension and failure diagnosis harder. Use a case table with scenario names and t.Run per case.
As per coding guidelines **/*_test.go: "Table-driven tests used for multiple cases" and "Test names describe the scenario, not the implementation".
🤖 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/validation/identity_header_test.go` around lines 9 - 13, Replace the
single-case TestIsForbiddenIdentityHeaderName with a table-driven test: create a
slice of cases each with a name, input header, and expected bool, then iterate
and call t.Run(case.name, func(t *testing.T) { RegisterTestingT(t);
Expect(IsForbiddenIdentityHeaderName(case.header)).To(Equal(case.expected)) });
include the two scenarios ("authorization header" -> "Authorization" -> true and
"hyperfleet identity header" -> "X-HyperFleet-Identity" -> false) so failures
are reported per scenario and the test is easier to extend.
….2-1779719578 Signed-off-by: red-hat-konflux-kflux-prd-rh02 <190377777+red-hat-konflux-kflux-prd-rh02[bot]@users.noreply.github.com>
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/auth/context_test.go`:
- Around line 50-53: The test helper function contextWithClaims in
pkg/auth/context_test.go is used by subtests but doesn't call t.Helper(), which
misattributes failures; update the helper signature used in the tests to call
t.Helper() at the top of the helper (inside the function body of
contextWithClaims or any local helper wrapper invoked by the subtests) so test
failures point to the caller rather than the helper.
🪄 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: 820c4701-7f71-4cd6-bb22-862347c671b4
📒 Files selected for processing (33)
Dockerfilecharts/Chart.yamlcharts/templates/NOTES.txtcharts/templates/configmap.yamlcharts/values.yamlcmd/hyperfleet-api/environments/e_integration_testing.gocmd/hyperfleet-api/environments/types.gocmd/hyperfleet-api/server/routes.goconfigs/config.yaml.exampledocs/authentication.mddocs/config.mdpkg/auth/auth_middleware.gopkg/auth/auth_middleware_mock.gopkg/auth/context.gopkg/auth/context_test.gopkg/auth/identity.gopkg/auth/identity_test.gopkg/config/flags.gopkg/config/loader.gopkg/config/loader_test.gopkg/config/logging.gopkg/config/server.gopkg/config/server_test.gopkg/services/cluster.gopkg/services/node_pool.gopkg/services/util.gopkg/validation/identity_header.gopkg/validation/identity_header_test.goplugins/clusters/plugin.goplugins/nodePools/plugin.gotest/helper.gotest/integration/caller_identity_test.gotest/integration/integration_test.go
💤 Files with no reviewable changes (1)
- pkg/auth/auth_middleware_mock.go
✅ Files skipped from review due to trivial changes (6)
- charts/Chart.yaml
- pkg/validation/identity_header_test.go
- Dockerfile
- docs/config.md
- charts/values.yaml
- pkg/config/loader_test.go
🚧 Files skipped from review as they are similar to previous changes (21)
- cmd/hyperfleet-api/environments/e_integration_testing.go
- pkg/config/flags.go
- pkg/validation/identity_header.go
- pkg/services/node_pool.go
- pkg/config/loader.go
- test/integration/integration_test.go
- docs/authentication.md
- cmd/hyperfleet-api/environments/types.go
- charts/templates/configmap.yaml
- test/helper.go
- test/integration/caller_identity_test.go
- pkg/services/cluster.go
- pkg/services/util.go
- configs/config.yaml.example
- plugins/nodePools/plugin.go
- pkg/config/server.go
- plugins/clusters/plugin.go
- pkg/auth/identity_test.go
- pkg/auth/context.go
- cmd/hyperfleet-api/server/routes.go
- pkg/auth/identity.go
Comment on lines
+50
to
+53
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| RegisterTestingT(t) | ||
| ctx := contextWithClaims(tc.claims) |
There was a problem hiding this comment.
Add t.Helper() to the test helper.
Line 69 defines a helper used by subtests, but it doesn’t mark itself as helper, which hurts failure attribution.
Suggested fix
- ctx := contextWithClaims(tc.claims)
+ ctx := contextWithClaims(t, tc.claims)
@@
-func contextWithClaims(claims jwt.MapClaims) context.Context {
+func contextWithClaims(t *testing.T, claims jwt.MapClaims) context.Context {
+ t.Helper()
token := &jwt.Token{Claims: claims}
return SetJWTTokenContext(context.Background(), token)
}As per coding guidelines **/*_test.go: "t.Helper() MUST be called in test helper functions".
Also applies to: 69-72
🤖 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/auth/context_test.go` around lines 50 - 53, The test helper function
contextWithClaims in pkg/auth/context_test.go is used by subtests but doesn't
call t.Helper(), which misattributes failures; update the helper signature used
in the tests to call t.Helper() at the top of the helper (inside the function
body of contextWithClaims or any local helper wrapper invoked by the subtests)
so test failures point to the caller rather than the helper.
|
@rh-amarin: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
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.
Summary
Adds configurable caller identity for audit attribution (created_by, updated_by, force-delete logs) while keeping JWT validation unchanged on the outer JWTHandler.
Operators can choose which JWT claim to use (server.jwt.identity_claim, default email) and optionally allow a trusted gateway to override attribution via server.identity_header (default header X-HyperFleet-Identity, header wins when non-empty).
Auth naming is clarified: JWTMiddleware / AuthenticateAccountJWT become CallerIdentityMiddleware / ResolveCallerIdentity, since that layer only resolves identity—not validates tokens.
Env: HYPERFLEET_SERVER_JWT_IDENTITY_CLAIM, HYPERFLEET_SERVER_IDENTITY_HEADER_ENABLED, HYPERFLEET_SERVER_IDENTITY_HEADER_NAME
Notable changes