HYPERFLEET-1147 - feat: add caller identity support for audit attribution#120
Open
kuudori wants to merge 1 commit into
Open
HYPERFLEET-1147 - feat: add caller identity support for audit attribution#120kuudori wants to merge 1 commit into
kuudori wants to merge 1 commit into
Conversation
|
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:
📝 WalkthroughWalkthroughAdds JWT caller-identity configuration for E2E tests, acquires tokens through Kubernetes TokenRequest, and injects bearer tokens into HyperFleet API requests. It adds configuration validation and defaults, documents setup, and introduces conditional audit assertions for cluster creation and deletion using a new Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant E2EHelper
participant KubernetesAPI
participant HyperFleetAPI
participant ClusterTests
E2EHelper->>KubernetesAPI: Request service-account JWT
KubernetesAPI-->>E2EHelper: Return token
E2EHelper->>HyperFleetAPI: Send API request with Bearer token
HyperFleetAPI-->>ClusterTests: Return cluster audit fields
ClusterTests->>ClusterTests: Assert created_by or deleted_by identity
🚥 Pre-merge checks | ✅ 9 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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
🤖 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 `@cmd/hyperfleet-e2e/main.go`:
- Around line 33-34: Add a new CLI flag binding for the identity token so
token-only usage is supported: declare or reuse the variable identityToken and
register it with pfs.StringVar(&identityToken, "identity-token", "", "Caller
identity token (alternative to identity-header/identity-value)"), and add the
same binding in the other flagset where identityHeader/identityValue are
registered (the other pfs.StringVar calls referenced in the comment). Update any
flag parsing logic that constructs request identity to prefer identityToken when
set (instead of header/value).
In `@cmd/hyperfleet-e2e/test/cmd.go`:
- Around line 60-67: The viper.BindPFlag calls for binding parent flags are
ignoring returned errors; update the block that calls viper.BindPFlag (for
config.API.URL, config.Log.Level, config.Log.Format, config.Log.Output,
config.Identity.Header, config.Identity.Value using parentFlags.Lookup("..."))
to check each error and handle it (e.g., return the error from the surrounding
function or log/fatal with context) instead of discarding; ensure you capture
the returned error from each viper.BindPFlag call and include a clear message
referencing the flag/key when reporting the failure.
In `@e2e/cluster/delete.go`:
- Around line 56-58: When h.ExpectedIdentity() returns a non-empty expected
identity, don't just assert deletedCluster.DeletedBy is non-nil — assert it
equals the expected identity for end-to-end attribution. Update the test in
delete.go to (1) fetch expected := h.ExpectedIdentity(), (2) ensure
deletedCluster.DeletedBy is present, and then (3) compare the DeletedBy value
against expected (use the appropriate field/representation on
deletedCluster.DeletedBy) so the assertion fails if the API attributes deletion
to the wrong caller.
In `@pkg/config/config.go`:
- Around line 497-499: The log currently emits c.Identity.Value as
"identity_value" in plaintext using valueOrNotSet; change this to a redacted
form (reuse or add a helper like redactValue similar to redactToken) so
identity_value does not reveal emails/identifiers in logs; update the call in
the block that builds the map (replace valueOrNotSet(c.Identity.Value) with a
redaction helper) and ensure redactToken(c.Identity.Token) remains unchanged.
- Around line 462-465: In Validate(), after the existing header/value presence
check (the block referencing c.Identity.Header and c.Identity.Value), add a
format validation for c.Identity.Value so malformed audit identities fail fast;
call or implement a small validator (e.g., parseAuditIdentity(...) or a tailored
regexp) and return a descriptive error from Validate() if it doesn't match the
expected audit-identity format (include the invalid value in the fmt.Errorf
message) so config loading fails early rather than at API runtime.
🪄 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: f6bf6e8a-4652-469d-9a0d-4495a6cd0f70
📒 Files selected for processing (14)
AGENTS.mdcmd/hyperfleet-e2e/main.gocmd/hyperfleet-e2e/test/cmd.goconfigs/config.yamldeploy-scripts/lib/api.shdocs/getting-started.mddocs/local-kind-setup.mde2e/cluster/creation.goe2e/cluster/delete.gopkg/client/client.gopkg/config/config.gopkg/helper/helper.gopkg/helper/matchers.gopkg/helper/suite.go
kuudori
force-pushed
the
HYPERFLEET-1147-caller-identity
branch
from
e286b25 to
5d8c6b8
Compare
kuudori
force-pushed
the
HYPERFLEET-1147-caller-identity
branch
from
5d8c6b8 to
9fe3db5
Compare
|
[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 |
kuudori
force-pushed
the
HYPERFLEET-1147-caller-identity
branch
from
9fe3db5 to
17ae4a9
Compare
Contributor
Author
|
/test e2e-deployment-validation |
kuudori
force-pushed
the
HYPERFLEET-1147-caller-identity
branch
from
17ae4a9 to
671d18f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/config/config.go (1)
492-501: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInline defaults
3600/"hyperfleet-api"duplicateconfigs/config.yamland bypass theDefault*constant convention.These values are declared in two places (here and
configs/config.yamllines 149-150). A change in one silently diverges from the other. Every other default in this file references aDefault*constant, and the project treatspkg/config/defaults.goas the single source of truth for config defaults.As per coding guidelines: "Config defaults | `pkg/config/defaults.go`" and QUAL-02 (magic numbers/strings should be named constants).♻️ Hoist to named constants in
pkg/config/defaults.go// pkg/config/defaults.go const ( DefaultTokenRequestExpirationSeconds int64 = 3600 DefaultTokenRequestAudience string = "hyperfleet-api" )if c.Identity.TokenRequest.IsEnabled() { if c.Identity.TokenRequest.ExpirationSeconds == 0 { - c.Identity.TokenRequest.ExpirationSeconds = 3600 + c.Identity.TokenRequest.ExpirationSeconds = DefaultTokenRequestExpirationSeconds } if c.Identity.TokenRequest.Audience == "" { - c.Identity.TokenRequest.Audience = "hyperfleet-api" + c.Identity.TokenRequest.Audience = DefaultTokenRequestAudience } }🤖 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/config.go` around lines 492 - 501, Replace the inline token request defaults in the config defaulting logic with named constants. Add DefaultTokenRequestExpirationSeconds and DefaultTokenRequestAudience to the existing constants in defaults.go, then update the Identity.TokenRequest default assignments in the relevant configuration function to reference them instead of 3600 and "hyperfleet-api".Source: Coding guidelines
🤖 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 `@docs/getting-started.md`:
- Around line 41-49: The JWT configuration in the getting-started documentation
should be conditional on identity enforcement being enabled, not presented as
universally required. Update the JWT setup section to state the Kubernetes
prerequisites: the test runner needs cluster credentials and permission to
create a TokenRequest for the configured service account; clarify that these
exports are unnecessary when enforcement is disabled. Also document whether the
e2e-deployment-validation path enables identity enforcement before requiring
this configuration.
In `@pkg/helper/suite.go`:
- Around line 85-92: Prevent bearer-token injection for non-TLS endpoints in the
client-option setup near cfg.Identity.Token and openapi.WithRequestEditorFn:
validate cfg.API.URL uses HTTPS before attaching the Authorization header, and
reject the configuration or skip token injection for HTTP URLs.
---
Nitpick comments:
In `@pkg/config/config.go`:
- Around line 492-501: Replace the inline token request defaults in the config
defaulting logic with named constants. Add DefaultTokenRequestExpirationSeconds
and DefaultTokenRequestAudience to the existing constants in defaults.go, then
update the Identity.TokenRequest default assignments in the relevant
configuration function to reference them instead of 3600 and "hyperfleet-api".
🪄 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: 25b9d127-b733-484a-8c38-5fc91ba4a118
📒 Files selected for processing (12)
AGENTS.mdcmd/hyperfleet-e2e/main.goconfigs/config.yamldocs/getting-started.mde2e/cluster/creation.goe2e/cluster/delete.gopkg/client/client.gopkg/client/kubernetes/client.gopkg/config/config.gopkg/helper/helper.gopkg/helper/matchers.gopkg/helper/suite.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift-hyperfleet/architecture(manual)openshift-hyperfleet/hyperfleet-api(manual)openshift-hyperfleet/hyperfleet-sentinel(manual)openshift-hyperfleet/hyperfleet-adapter(manual)openshift-hyperfleet/hyperfleet-broker(manual)
💤 Files with no reviewable changes (1)
- cmd/hyperfleet-e2e/main.go
✅ Files skipped from review due to trivial changes (1)
- AGENTS.md
🚧 Files skipped from review as they are similar to previous changes (5)
- pkg/helper/helper.go
- e2e/cluster/creation.go
- e2e/cluster/delete.go
- pkg/client/client.go
- pkg/helper/matchers.go
Comment on lines
+85
to
+92
| var opts []openapi.ClientOption | ||
| if token := cfg.Identity.Token(); token != "" { | ||
| opts = append(opts, openapi.WithRequestEditorFn( | ||
| func(_ context.Context, req *http.Request) error { | ||
| req.Header.Set("Authorization", "Bearer "+token) | ||
| return nil | ||
| })) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'API\.URL' pkg/ cmd/
rg -nP 'https?://|url\.Parse|Scheme' pkg/config/config.go pkg/client/client.go pkg/helper/suite.goRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 743
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## pkg/config/config.go (around validation + URL helpers)\n'
sed -n '480,640p' pkg/config/config.go
printf '\n## pkg/helper/suite.go (around client construction)\n'
sed -n '1,130p' pkg/helper/suite.go
printf '\n## command wiring for api-url\n'
sed -n '1,120p' cmd/hyperfleet-e2e/test/cmd.goRepository: openshift-hyperfleet/hyperfleet-e2e
Length of output: 12264
Refuse bearer tokens on non-TLS API URLs (CWE-319)
pkg/helper/suite.go:85-92 unconditionally adds Authorization: Bearer ... for every request. If cfg.API.URL is http://, the JWT is sent in cleartext. Reject non-HTTPS API URLs before attaching the header, or skip token injection unless TLS is in use.
🤖 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/helper/suite.go` around lines 85 - 92, Prevent bearer-token injection for
non-TLS endpoints in the client-option setup near cfg.Identity.Token and
openapi.WithRequestEditorFn: validate cfg.API.URL uses HTTPS before attaching
the Authorization header, and reject the configuration or skip token injection
for HTTP URLs.
- Add TokenRequest-based JWT acquisition using K8s ServiceAccount tokens - Add CreateToken() method on K8s client via TokenRequest API - Add IdentityConfig with tokenRequest config (SA name, namespace, audience, expiration) - Add expectedIdentity field for audit assertion (created_by, deleted_by) - Add HaveAuditIdentity matcher and ExpectedIdentity() helper - Add audit assertions in cluster creation and deletion tests - Inject Bearer token via openapi.WithRequestEditorFn on all API requests - Accept ClientOption variadic in NewHyperFleetClient for request editors - Update config.yaml, AGENTS.md, and getting-started docs
kuudori
force-pushed
the
HYPERFLEET-1147-caller-identity
branch
from
671d18f to
76c422d
Compare
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.
Inject caller identity headers into all E2E API requests so tests work against an API with identity enforcement enabled (production default).
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)