HYPERFLEET-1237 - feat: add JWT auth support for HyperFleet API client#220
Open
rh-amarin wants to merge 1 commit into
Open
HYPERFLEET-1237 - feat: add JWT auth support for HyperFleet API client#220rh-amarin wants to merge 1 commit into
rh-amarin wants to merge 1 commit 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 (17)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (13)
📝 WalkthroughSummary by CodeRabbit
WalkthroughOptional JWT auth was added for the HyperFleet API client. Helm values, templates, and examples now include Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Sentinel as runServe
participant Client as NewHyperFleetClient
participant Token as fileTokenSource
participant Disk as token file
participant API as HyperFleetAPI
Sentinel->>Client: tokenPath, tokenCacheTTL
Client->>Token: newFileTokenSource() when tokenPath is set
Client->>Token: get()
alt cache hit
Token-->>Client: cached token
else cache miss or disabled
Token->>Disk: read token file
Disk-->>Token: trimmed token
Token-->>Client: token
end
Client->>API: Authorization: Bearer token
CWE-306: auth stays optional and disabled by default. CWE-522: bearer material is loaded from a projected service-account token file. CWE-209: token read failures return errors without exposing token contents. 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 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: 7
🧹 Nitpick comments (2)
internal/client/token_test.go (1)
141-178: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test for token-retrieval failure propagating through
FetchResources.
client.go's request editor wraps token errors as"bearer token unavailable: %w", but no test exercises this — e.g. a missing/unreadable token file at request time. As per path instructions, "Error paths SHOULD be tested, not just happy paths."✅ Proposed additional test
func TestNewHyperFleetClient_BearerTokenError_WhenTokenFileMissing(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { t.Fatal("server should not be called when token retrieval fails") })) defer server.Close() c, err := NewHyperFleetClient(server.URL, 10*time.Second, "test-sentinel", "test", "/nonexistent/path/token", 0) if err != nil { t.Fatalf("NewHyperFleetClient: %v", err) } if _, err := c.FetchResources(context.Background(), ResourceTypeClusters, nil); err == nil { t.Fatal("expected error when token file is missing") } }🤖 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 `@internal/client/token_test.go` around lines 141 - 178, Add a negative test for token retrieval failure in `internal/client/token_test.go` to cover the `requestEditor`/`FetchResources` path when the bearer token cannot be read. Create a test like `TestNewHyperFleetClient_BearerTokenError_WhenTokenFileMissing` that uses `NewHyperFleetClient` with a nonexistent token file, calls `FetchResources`, and asserts an error is returned. Also verify the test server is never hit so the failure is clearly coming from token loading rather than the request itself.Source: Path instructions
internal/client/token.go (1)
28-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winExclusive lock taken even when caching is disabled.
When
cacheTTL == 0, no shared field is ever written, yet every call still takes the exclusives.mu.Lock()at line 41 to read the file. This serializes all concurrent per-request token reads under the "no-cache" mode — the opposite of what you want for a hot path hit on every outbound API call.♻️ Proposed fix: bypass locking when caching is disabled
func (s *fileTokenSource) get() (string, error) { + if s.cacheTTL == 0 { + return s.readToken() + } + now := time.Now().UnixNano() - - if s.cacheTTL > 0 { - s.mu.RLock() - if now < s.expiresAt { - tok := s.cached - s.mu.RUnlock() - return tok, nil - } - s.mu.RUnlock() - } + s.mu.RLock() + if now < s.expiresAt { + tok := s.cached + s.mu.RUnlock() + return tok, nil + } + s.mu.RUnlock() s.mu.Lock() defer s.mu.Unlock() - // Re-check under write lock — another goroutine may have refreshed. - if s.cacheTTL > 0 && now < s.expiresAt { + // Re-check under write lock — another goroutine may have refreshed. + if now < s.expiresAt { return s.cached, nil } - raw, err := os.ReadFile(s.path) - if err != nil { - return "", fmt.Errorf("reading token file %s: %w", s.path, err) - } - tok := strings.TrimSpace(string(raw)) - if tok == "" { - return "", fmt.Errorf("token file %s is empty", s.path) - } - - if s.cacheTTL > 0 { - s.cached = tok - s.expiresAt = time.Now().Add(s.cacheTTL).UnixNano() - } + tok, err := s.readToken() + if err != nil { + return "", err + } + s.cached = tok + s.expiresAt = time.Now().Add(s.cacheTTL).UnixNano() return tok, nil } + +func (s *fileTokenSource) readToken() (string, error) { + raw, err := os.ReadFile(s.path) + if err != nil { + return "", fmt.Errorf("reading token file %s: %w", s.path, err) + } + tok := strings.TrimSpace(string(raw)) + if tok == "" { + return "", fmt.Errorf("token file %s is empty", s.path) + } + return tok, nil +}🤖 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 `@internal/client/token.go` around lines 28 - 62, The fileTokenSource.get method still takes s.mu.Lock even when cacheTTL is disabled, which unnecessarily serializes concurrent token reads on the hot path. Update get to bypass both the read and write lock paths entirely when cacheTTL == 0, while still preserving the existing cache validation and refresh logic for the cached case. Keep the changes localized to fileTokenSource.get and the s.mu/s.cached/s.expiresAt handling so the no-cache mode reads the token file without locking.
🤖 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/templates/deployment.yaml`:
- Around line 125-133: The projected service account token in the
hyperfleet-api-token volume is using Kubernetes’ default world-readable
permissions, so tighten it by setting an explicit defaultMode on the projected
volume in the deployment template. Update the hyperfleetApi auth block in the
deployment.yaml template near the serviceAccountToken source so the token file
is mounted with restricted permissions instead of inheriting 0644, keeping the
existing audience, expirationSeconds, and tokenPath settings intact.
- Around line 111-115: The Hyperfleet API token volume mount uses
`.Values.config.clients.hyperfleetApi.auth.tokenPath | dir`, which can turn a
bare filename into an invalid mount path and reject the Pod spec. Update the
validation in `internal/config/config.go` to require an absolute `tokenPath`
when `config.clients.hyperfleetApi.auth.enabled` is set, and make sure the
`charts/templates/deployment.yaml` mountPath logic assumes that validated
absolute path instead of deriving a directory from a relative file name.
In `@internal/client/client.go`:
- Around line 52-57: The doc comment on NewHyperFleetClient contradicts the
actual zero-TTL behavior in fileTokenSource.get: tokenCacheTTL of 0 does not use
a 30s default, it disables caching and re-reads the token file on every request.
Update the comment on NewHyperFleetClient and the tokenCacheTTL description to
match the implementation in internal/client/token.go, so operators get the
correct expectation for cache behavior.
In `@internal/config/config.go`:
- Around line 90-99: Add table-driven tests for HyperFleetAPIAuthConfig.Validate
to cover the new auth validation path used by SentinelConfig.Validate. Include
cases for a missing token_path, a negative token_cache_ttl, and a valid auth
configuration, and assert the expected errors or success so the CWE-20 boundary
checks remain covered.
In `@internal/sentinel/sentinel_test.go`:
- Around line 165-166: `client.NewHyperFleetClient(...)` is being called with
its error discarded at several `sentinel_test.go` call sites, which can hide
setup failures and later cause nil-pointer panics in `NewSentinel` or `trigger`.
Update each affected test to check the returned error explicitly (using the same
pattern already used in `TestTrigger_CreatesRequiredSpans`) and fail the test
immediately if client construction fails, so the failure is reported at the call
site instead of downstream.
In `@test/integration/integration_test.go`:
- Line 171: The constructor error from client.NewHyperFleetClient is being
ignored at multiple call sites in this test file, which can hide client setup
failures and lead to nil-pointer panics. Update each affected call site to
handle the returned error the same way as TestIntegration_EndToEndSpanHierarchy
does: capture the error, fail the test immediately if construction fails, and
keep the existing client variable usage only after successful initialization.
---
Nitpick comments:
In `@internal/client/token_test.go`:
- Around line 141-178: Add a negative test for token retrieval failure in
`internal/client/token_test.go` to cover the `requestEditor`/`FetchResources`
path when the bearer token cannot be read. Create a test like
`TestNewHyperFleetClient_BearerTokenError_WhenTokenFileMissing` that uses
`NewHyperFleetClient` with a nonexistent token file, calls `FetchResources`, and
asserts an error is returned. Also verify the test server is never hit so the
failure is clearly coming from token loading rather than the request itself.
In `@internal/client/token.go`:
- Around line 28-62: The fileTokenSource.get method still takes s.mu.Lock even
when cacheTTL is disabled, which unnecessarily serializes concurrent token reads
on the hot path. Update get to bypass both the read and write lock paths
entirely when cacheTTL == 0, while still preserving the existing cache
validation and refresh logic for the cached case. Keep the changes localized to
fileTokenSource.get and the s.mu/s.cached/s.expiresAt handling so the no-cache
mode reads the token file without locking.
🪄 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: 2a8c06cb-a8e1-4b9f-b428-3d53b3daf585
📒 Files selected for processing (15)
charts/README.mdcharts/templates/configmap.yamlcharts/templates/deployment.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamlinternal/client/client.gointernal/client/client_test.gointernal/client/token.gointernal/client/token_test.gointernal/config/config.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.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)
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/client/client.go (1)
52-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDoc comment still misrepresents zero-TTL caching behavior.
This exact text was flagged in a prior review round and remains unfixed.
NewHyperFleetClientdoc says "0 uses the default (30s)", butfileTokenSource.get()ininternal/client/token.gotreatscacheTTL == 0as "no caching — read the file on every request" — there's no 30s default anywhere in the implementation. An operator relying on this comment will misconfiguretoken_cache_ttl: 0expecting a 30s cache and instead get a disk read every poll cycle.📝 Proposed fix
// tokenPath is optional; when non-empty the client reads a bearer token from that file and // injects it as an Authorization header on every request. tokenCacheTTL controls how long -// the token is cached before the file is re-read; 0 uses the default (30s). +// the token is cached before the file is re-read; 0 disables caching, causing the file to +// be re-read on every request.🤖 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 `@internal/client/client.go` around lines 52 - 54, Update the documentation for NewHyperFleetClient so it matches the actual zero-TTL behavior in fileTokenSource.get(): tokenCacheTTL == 0 means the token is never cached and the file is re-read on every request, not that a 30s default is used. Adjust the comment near the tokenPath/tokenCacheTTL description to remove the incorrect default wording and clearly describe the real caching semantics.
🧹 Nitpick comments (1)
internal/client/client.go (1)
300-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAuth-header injection logic duplicated verbatim in two call sites.
VerifyConnectivity(Lines 317-323) andfetchResourcesPage(Lines 451-457) both re-implement "check tokenSource, fetch token, set header, wrap error" identically except for the error type returned. Any future change to auth logic (e.g. adding retry-on-token-error, token refresh, or scoping headers) has to be kept in sync in two places.♻️ Proposed fix: extract shared helper
func (c *HyperFleetClient) setAuthHeader(req *http.Request) error { if c.tokenSource == nil { return nil } tok, err := c.tokenSource.get() if err != nil { return fmt.Errorf("bearer token unavailable: %w", err) } req.Header.Set("Authorization", "Bearer "+tok) return nil }req.Header.Set("User-Agent", c.userAgent) - if c.tokenSource != nil { - tok, err := c.tokenSource.get() - if err != nil { - return fmt.Errorf("bearer token unavailable: %w", err) - } - req.Header.Set("Authorization", "Bearer "+tok) - } + if err := c.setAuthHeader(req); err != nil { + return fmt.Errorf("could not verify connectivity: %w", err) + }req.Header.Set("User-Agent", c.userAgent) - if c.tokenSource != nil { - tok, err := c.tokenSource.get() - if err != nil { - return nil, 0, &APIError{StatusCode: 0, Message: fmt.Sprintf("bearer token unavailable: %v", err), Retriable: false} - } - req.Header.Set("Authorization", "Bearer "+tok) - } + if err := c.setAuthHeader(req); err != nil { + return nil, 0, &APIError{StatusCode: 0, Message: err.Error(), Retriable: false} + }Also applies to: 437-457
🤖 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 `@internal/client/client.go` around lines 300 - 323, The auth-header injection logic is duplicated in VerifyConnectivity and fetchResourcesPage, so extract it into a shared helper on HyperFleetClient such as setAuthHeader and use it from both call sites. Move the tokenSource nil check, token retrieval, Authorization header assignment, and token error wrapping into that helper, then keep the existing request-specific error handling in VerifyConnectivity and fetchResourcesPage by calling the helper before the request is sent.
🤖 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/README.md`:
- Line 58: The inline config default in the README table is being parsed as
extra columns because the JSON contains the messageDecision expression with ||.
Update the config cell in the values table to escape the pipe characters or
otherwise move the full default object out of the table so the row stays intact.
Use the existing config, messageDecision, and auth-enabled default entry as the
location to fix.
In `@internal/client/client.go`:
- Around line 399-435: fetchPaginated currently relies only on total and empty
pages to stop, so if the API misreports counts or ignores page it can loop
indefinitely and keep growing allResources. Add an independent hard limit on
pagination in fetchPaginated (using the existing page loop and c.pageSize), and
fail closed or return an error when that ceiling is reached or when the page
sequence looks inconsistent. Keep the fix localized to fetchPaginated and
preserve the existing fetchPage, convert, and resourceLabel flow.
---
Duplicate comments:
In `@internal/client/client.go`:
- Around line 52-54: Update the documentation for NewHyperFleetClient so it
matches the actual zero-TTL behavior in fileTokenSource.get(): tokenCacheTTL ==
0 means the token is never cached and the file is re-read on every request, not
that a 30s default is used. Adjust the comment near the tokenPath/tokenCacheTTL
description to remove the incorrect default wording and clearly describe the
real caching semantics.
---
Nitpick comments:
In `@internal/client/client.go`:
- Around line 300-323: The auth-header injection logic is duplicated in
VerifyConnectivity and fetchResourcesPage, so extract it into a shared helper on
HyperFleetClient such as setAuthHeader and use it from both call sites. Move the
tokenSource nil check, token retrieval, Authorization header assignment, and
token error wrapping into that helper, then keep the existing request-specific
error handling in VerifyConnectivity and fetchResourcesPage by calling the
helper before the request is sent.
🪄 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: b685e0a2-f16c-44bf-9569-887d66c4daf1
📒 Files selected for processing (15)
charts/README.mdcharts/templates/configmap.yamlcharts/templates/deployment.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamlinternal/client/client.gointernal/client/client_test.gointernal/client/token.gointernal/client/token_test.gointernal/config/config.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.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 skipped from review due to trivial changes (3)
- configs/gcp-pubsub-example.yaml
- configs/rabbitmq-example.yaml
- configs/dev-example.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- charts/templates/deployment.yaml
- cmd/sentinel/main.go
- charts/templates/configmap.yaml
- internal/config/config.go
- internal/client/token_test.go
- internal/client/client_test.go
- internal/sentinel/sentinel_test.go
- internal/client/token.go
- charts/values.yaml
Risk Score: 5 —
|
| Signal | Detail | Points |
|---|---|---|
| PR size | 552 lines (>500) | +2 |
| Sensitive paths | cmd/ | +2 |
| Test coverage | Missing tests for: cmd/sentinel | +1 |
Computed by hyperfleet-risk-scorer
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/client/client.go (1)
52-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDoc comment still misrepresents zero-TTL caching behavior.
This exact text was flagged in a prior review round and remains unfixed.
NewHyperFleetClientdoc says "0 uses the default (30s)", butfileTokenSource.get()ininternal/client/token.gotreatscacheTTL == 0as "no caching — read the file on every request" — there's no 30s default anywhere in the implementation. An operator relying on this comment will misconfiguretoken_cache_ttl: 0expecting a 30s cache and instead get a disk read every poll cycle.📝 Proposed fix
// tokenPath is optional; when non-empty the client reads a bearer token from that file and // injects it as an Authorization header on every request. tokenCacheTTL controls how long -// the token is cached before the file is re-read; 0 uses the default (30s). +// the token is cached before the file is re-read; 0 disables caching, causing the file to +// be re-read on every request.🤖 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 `@internal/client/client.go` around lines 52 - 54, Update the documentation for NewHyperFleetClient so it matches the actual zero-TTL behavior in fileTokenSource.get(): tokenCacheTTL == 0 means the token is never cached and the file is re-read on every request, not that a 30s default is used. Adjust the comment near the tokenPath/tokenCacheTTL description to remove the incorrect default wording and clearly describe the real caching semantics.
🧹 Nitpick comments (1)
internal/client/client.go (1)
300-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAuth-header injection logic duplicated verbatim in two call sites.
VerifyConnectivity(Lines 317-323) andfetchResourcesPage(Lines 451-457) both re-implement "check tokenSource, fetch token, set header, wrap error" identically except for the error type returned. Any future change to auth logic (e.g. adding retry-on-token-error, token refresh, or scoping headers) has to be kept in sync in two places.♻️ Proposed fix: extract shared helper
func (c *HyperFleetClient) setAuthHeader(req *http.Request) error { if c.tokenSource == nil { return nil } tok, err := c.tokenSource.get() if err != nil { return fmt.Errorf("bearer token unavailable: %w", err) } req.Header.Set("Authorization", "Bearer "+tok) return nil }req.Header.Set("User-Agent", c.userAgent) - if c.tokenSource != nil { - tok, err := c.tokenSource.get() - if err != nil { - return fmt.Errorf("bearer token unavailable: %w", err) - } - req.Header.Set("Authorization", "Bearer "+tok) - } + if err := c.setAuthHeader(req); err != nil { + return fmt.Errorf("could not verify connectivity: %w", err) + }req.Header.Set("User-Agent", c.userAgent) - if c.tokenSource != nil { - tok, err := c.tokenSource.get() - if err != nil { - return nil, 0, &APIError{StatusCode: 0, Message: fmt.Sprintf("bearer token unavailable: %v", err), Retriable: false} - } - req.Header.Set("Authorization", "Bearer "+tok) - } + if err := c.setAuthHeader(req); err != nil { + return nil, 0, &APIError{StatusCode: 0, Message: err.Error(), Retriable: false} + }Also applies to: 437-457
🤖 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 `@internal/client/client.go` around lines 300 - 323, The auth-header injection logic is duplicated in VerifyConnectivity and fetchResourcesPage, so extract it into a shared helper on HyperFleetClient such as setAuthHeader and use it from both call sites. Move the tokenSource nil check, token retrieval, Authorization header assignment, and token error wrapping into that helper, then keep the existing request-specific error handling in VerifyConnectivity and fetchResourcesPage by calling the helper before the request is sent.
🤖 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/README.md`:
- Line 58: The inline config default in the README table is being parsed as
extra columns because the JSON contains the messageDecision expression with ||.
Update the config cell in the values table to escape the pipe characters or
otherwise move the full default object out of the table so the row stays intact.
Use the existing config, messageDecision, and auth-enabled default entry as the
location to fix.
In `@internal/client/client.go`:
- Around line 399-435: fetchPaginated currently relies only on total and empty
pages to stop, so if the API misreports counts or ignores page it can loop
indefinitely and keep growing allResources. Add an independent hard limit on
pagination in fetchPaginated (using the existing page loop and c.pageSize), and
fail closed or return an error when that ceiling is reached or when the page
sequence looks inconsistent. Keep the fix localized to fetchPaginated and
preserve the existing fetchPage, convert, and resourceLabel flow.
---
Duplicate comments:
In `@internal/client/client.go`:
- Around line 52-54: Update the documentation for NewHyperFleetClient so it
matches the actual zero-TTL behavior in fileTokenSource.get(): tokenCacheTTL ==
0 means the token is never cached and the file is re-read on every request, not
that a 30s default is used. Adjust the comment near the tokenPath/tokenCacheTTL
description to remove the incorrect default wording and clearly describe the
real caching semantics.
---
Nitpick comments:
In `@internal/client/client.go`:
- Around line 300-323: The auth-header injection logic is duplicated in
VerifyConnectivity and fetchResourcesPage, so extract it into a shared helper on
HyperFleetClient such as setAuthHeader and use it from both call sites. Move the
tokenSource nil check, token retrieval, Authorization header assignment, and
token error wrapping into that helper, then keep the existing request-specific
error handling in VerifyConnectivity and fetchResourcesPage by calling the
helper before the request is sent.
🪄 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: b685e0a2-f16c-44bf-9569-887d66c4daf1
📒 Files selected for processing (15)
charts/README.mdcharts/templates/configmap.yamlcharts/templates/deployment.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamlinternal/client/client.gointernal/client/client_test.gointernal/client/token.gointernal/client/token_test.gointernal/config/config.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.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 skipped from review due to trivial changes (3)
- configs/gcp-pubsub-example.yaml
- configs/rabbitmq-example.yaml
- configs/dev-example.yaml
🚧 Files skipped from review as they are similar to previous changes (9)
- charts/templates/deployment.yaml
- cmd/sentinel/main.go
- charts/templates/configmap.yaml
- internal/config/config.go
- internal/client/token_test.go
- internal/client/client_test.go
- internal/sentinel/sentinel_test.go
- internal/client/token.go
- charts/values.yaml
🛑 Comments failed to post (2)
charts/README.md (1)
58-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Escape the pipes in the default JSON.
The inline
configdefault contains||, which Markdown parses as table separators. That breaks the generated values table at Line 58 (MD056), so the auth-enabled default is effectively unreadable. Escape the pipes (\|\|) or move this object out of the table. CWE-116.🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 58-58: Table column count
Expected: 4; Actual: 10; Too many cells, extra data will be missing(MD056, table-column-count)
🤖 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 `@charts/README.md` at line 58, The inline config default in the README table is being parsed as extra columns because the JSON contains the messageDecision expression with ||. Update the config cell in the values table to escape the pipe characters or otherwise move the full default object out of the table so the row stays intact. Use the existing config, messageDecision, and auth-enabled default entry as the location to fix.Source: Linters/SAST tools
internal/client/client.go (1)
399-435: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant file and nearby symbols first. ast-grep outline internal/client/client.go --view expanded || true printf '\n--- relevant lines ---\n' sed -n '340,470p' internal/client/client.go printf '\n--- page size / pagination usages ---\n' rg -n "fetchPaginated|pageSize|total|Pagination|List.*Page" internal/client -nRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 10365
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- fetchResourcesPage response shape ---' sed -n '438,560p' internal/client/client.go printf '\n%s\n' '--- pagination tests ---' sed -n '1120,1295p' internal/client/client_test.go printf '\n%s\n' '--- any other pagination safeguards ---' rg -n "len\\(items\\) == 0|pageSize|totalResources|max page|maxPage|pagination" internal/client docs configs openapi -nRepository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 10681
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '%s\n' '--- FetchResources call sites ---' rg -n "FetchResources\\(" . printf '\n%s\n' '--- context lifetime around resource polling ---' rg -n "context.WithTimeout|context.WithDeadline|WithCancel|poll|fetchResourcesOnce|fetchResources\\(" internal cmd pkg .Repository: openshift-hyperfleet/hyperfleet-sentinel
Length of output: 39559
Bound pagination with an independent hard cap —
fetchPaginatedonly stops onlen(allResources) >= totalor an empty page. If the API misreportstotalor keeps returning non-empty pages while ignoringpage, this can pin the poll cycle and growallResourceswithout limit (CWE-400). Add an independent page ceiling or fail closed on inconsistent pagination.🤖 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 `@internal/client/client.go` around lines 399 - 435, fetchPaginated currently relies only on total and empty pages to stop, so if the API misreports counts or ignores page it can loop indefinitely and keep growing allResources. Add an independent hard limit on pagination in fetchPaginated (using the existing page loop and c.pageSize), and fail closed or return an error when that ceiling is reached or when the page sequence looks inconsistent. Keep the fix localized to fetchPaginated and preserve the existing fetchPage, convert, and resourceLabel flow.
rh-amarin
force-pushed
the
1237
branch
2 times, most recently
from
61e5602 to
7c2dc6a
Compare
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 `@charts/README.md`:
- Line 58: The generated values table row for the `config` entry contains an
unescaped CEL `||` in `messageDecision.result`, which breaks the Markdown table
layout. Update the `charts/README.md` source that produces this row so the `||`
characters are escaped or the default value is rewritten to avoid pipes, keeping
the table columns intact. Focus on the `config` documentation row and the
`messageDecision.result` default string so the rendered README stays valid.
🪄 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: f4ab46fd-e30c-4397-9de0-a3bcfaf58479
📒 Files selected for processing (17)
charts/README.mdcharts/templates/_helpers.tplcharts/templates/configmap.yamlcharts/templates/deployment.yamlcharts/values.yamlcmd/sentinel/main.goconfigs/dev-example.yamlconfigs/gcp-pubsub-example.yamlconfigs/rabbitmq-example.yamlinternal/client/client.gointernal/client/client_test.gointernal/client/token.gointernal/client/token_test.gointernal/config/config.gointernal/config/config_test.gointernal/sentinel/sentinel_test.gotest/integration/integration_test.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 skipped from review due to trivial changes (3)
- configs/rabbitmq-example.yaml
- configs/gcp-pubsub-example.yaml
- configs/dev-example.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- charts/templates/deployment.yaml
- cmd/sentinel/main.go
- internal/sentinel/sentinel_test.go
- internal/client/token.go
- charts/templates/configmap.yaml
- internal/config/config.go
- internal/client/token_test.go
- test/integration/integration_test.go
- charts/values.yaml
- internal/client/client.go
- internal/client/client_test.go
Adds optional Kubernetes service account token authentication to the HyperFleet API HTTP client. When configured, a bearer token is read from a file and injected as an Authorization header on every request. Token reading is backed by a configurable in-memory cache (TTL > 0) or file-per-request mode (TTL == 0) using a sync.RWMutex with double-checked locking for safe concurrent access. The Helm chart gains an auth block under config.clients.hyperfleetApi that mounts a projected ServiceAccountToken volume with a configurable audience, token path, expiration, and cache TTL. Auth is disabled by default; existing deployments are unaffected. Co-Authored-By: Claude <noreply@anthropic.com>
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
Bearertoken is read from a file and injected into every API requesttoken_cache_ttl > 0) or file-per-request mode (token_cache_ttl: 0), usingsync.RWMutexwith double-checked locking for safe concurrent accessauthblock underconfig.clients.hyperfleetApithat mounts a projectedServiceAccountTokenvolume with configurableaudience,tokenPath,expirationSeconds, andtokenCacheTtl; disabled by default so existing deployments are unaffectedConfiguration
The env vars
HYPERFLEET_API_AUTH_TOKEN_PATHandHYPERFLEET_API_AUTH_TOKEN_CACHE_TTLcan also override the config file.Test plan
make verify && make test-unit— all packages passhelm lint charts/— no errorshelm templatewithauth.enabled=truerenders projected volume, volume mount, andauth.token_pathin configmaphelm templatewithauth.enabled=false(default) renders no auth-related resourcesAuthorization: Bearer <token>header reaches the API🤖 Generated with Claude Code