feat(event-ledger): Add event ledger to NVCF - #780
Conversation
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdded the Event Ledger Go service with V1, V2, and V3 APIs, Cassandra persistence, authentication, CloudEvents publishing, observability, configuration, container packaging, and comprehensive tests. ChangesEvent Ledger service
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟡 Minor comments (30)
src/control-plane-services/event-ledger/go.mod-41-41 (1)
41-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove Markdown-like emphasis from the Go comment.
The
****markers use bold-style emphasis. Use a plain ASCII comment instead.As per path instructions, "Keep documentation and comments concise, ASCII-only, and free of bold/em-dashes/emojis."
🤖 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 `@src/control-plane-services/event-ledger/go.mod` at line 41, Update the dependency comment on github.com/uptrace/opentelemetry-go-extra/otelzap to remove the Markdown-style **** emphasis markers, while preserving the warning text and keeping the comment concise and ASCII-only.Source: Path instructions
src/control-plane-services/event-ledger/pkg/codex/codex.go-89-105 (1)
89-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn wrapped decoder errors immediately.
Both methods log the
NativeFromBinaryerror, continue processing, and then return a generic error. This discards the originating decoder error from callers.
src/control-plane-services/event-ledger/pkg/codex/codex.go#L89-L105: returnErrEnvelopewith a%w-wrapped decoder error whenNativeFromBinaryfails.src/control-plane-services/event-ledger/pkg/codex/stage_transition_event_schema.go#L77-L118: returntypes.ErrStageTransitionEventwith a%w-wrapped decoder error whenNativeFromBinaryfails.Remove the local error logs if the request boundary logs returned errors. This prevents duplicate logs.
As per coding guidelines, "When logging errors, preserve the originating error with
%wor an equivalent wrapping mechanism and do not both log and return the same error."🤖 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 `@src/control-plane-services/event-ledger/pkg/codex/codex.go` around lines 89 - 105, Update codex.go lines 89-105 in the NativeFromBinary failure path to return ErrEnvelope wrapped with the original decoder error via %w, and remove the local error log. Apply the same change to stage_transition_event_schema.go lines 77-118: return types.ErrStageTransitionEvent with the decoder error wrapped via %w and remove its local log; both sites should rely on the request boundary for logging.Sources: Coding guidelines, Path instructions
src/control-plane-services/event-ledger/README.md-46-58 (1)
46-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAdd authentication to the request examples.
Line 47 sends an unauthenticated request. Lines 108-113 state that authentication is enabled by default and POST requests require a bearer token with the
writescope. This example returns401with the documented default configuration.Add the required
Authorizationheader to POST and GET examples. Alternatively, state that the examples require a local service started with--disable-authentication.🤖 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 `@src/control-plane-services/event-ledger/README.md` around lines 46 - 58, Add authentication guidance to all POST and GET curl examples in the README, including an Authorization bearer-token header with the required write scope for POST requests. Ensure the examples work with the documented default authentication configuration, or explicitly state that they require a service started with --disable-authentication.src/control-plane-services/event-ledger/pkg/codex/codex_test.go-51-64 (1)
51-64: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the expected error result.
The
BadWrapcase passes whenWrapincorrectly returnsnil. It also callsUnwrapwith a nil payload after the expectedWrapfailure.Assert
erragainstwantErr, then return from the subtest whenWrapfails.Proposed fix
wrapped, err := codex.Wrap(context.Background(), tt.encoding, tt.msgBody) - if err != nil && !tt.wantErr { - t.Errorf("error wrapping message: %s", err.Error()) + if (err != nil) != tt.wantErr { + t.Fatalf("Wrap() error = %v, wantErr %v", err, tt.wantErr) + } + if err != nil { + return } unwrapped, err := codex.Unwrap(context.Background(), wrapped) - if err != nil && !tt.wantErr { - t.Errorf("error decoding message: %s", err.Error()) + if err != nil { + t.Fatalf("error decoding message: %v", err) }As per coding guidelines, "Code changes must include tests."
🤖 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 `@src/control-plane-services/event-ledger/pkg/codex/codex_test.go` around lines 51 - 64, Update the test case around codex.Wrap and codex.Unwrap to assert that the Wrap error presence matches tt.wantErr. When Wrap returns an error, stop the subtest after validating the expected result so Unwrap is not called with a nil payload; continue to Unwrap only for successful wrapping, preserving the existing decoded-body assertion.Source: Coding guidelines
src/control-plane-services/event-ledger/cmd/api/service/service_test.go-109-111 (1)
109-111: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNon-ASCII characters appear in Go comments. Both mock implementations annotate the
PaginationMetafields with a check-mark emoji. The subtree guidance requires comments to stay ASCII-only and free of emojis. Remove the emoji and keep the plain text.
src/control-plane-services/event-ledger/cmd/api/service/service_test.go#L109-L111: delete the emoji from thePageSizeandHasMorecomments inpassDBHandler.ListInstancesPaginated.src/control-plane-services/event-ledger/cmd/api/service/servicev2_test.go#L92-L94: delete the emoji from thePageSizeandHasMorecomments inpassDBHandlerV2.ListDeploymentInstancesPaginated.As per path instructions: "Keep documentation and comments concise, ASCII-only, and free of bold/em-dashes/emojis."
🤖 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 `@src/control-plane-services/event-ledger/cmd/api/service/service_test.go` around lines 109 - 111, Remove the check-mark emojis from the PageSize and HasMore comments in passDBHandler.ListInstancesPaginated at src/control-plane-services/event-ledger/cmd/api/service/service_test.go:109-111, and in passDBHandlerV2.ListDeploymentInstancesPaginated at src/control-plane-services/event-ledger/cmd/api/service/servicev2_test.go:92-94. Preserve the plain ASCII comment text.Source: Path instructions
src/control-plane-services/event-ledger/cmd/api/service/service_test.go-605-620 (1)
605-620: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe
wantBodyexpectation for the 500 case never runs.Line 609 sets
wantBody: "Fail this"for theUnsuccessfulPostFunctionVersionIdInstanceIdcase. Line 661 skips the body assertion wheneverwantStatusequalshttp.StatusInternalServerError. The expectation is unreachable, and the string does not match any error text this handler produces.Either remove the value or assert the real 500 body. The same guard appears at line 757 in
TestListStageTransitionEvents.Also applies to: 661-665
🤖 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 `@src/control-plane-services/event-ledger/cmd/api/service/service_test.go` around lines 605 - 620, Remove the unreachable wantBody value from UnsuccessfulPostFunctionVersionIdInstanceId, or replace it with the actual 500 response body and update the assertion guard so it runs for this case. Apply the same correction to the http.StatusInternalServerError body-assertion guard in TestListStageTransitionEvents.src/control-plane-services/event-ledger/cmd/api/service/v2.go-485-491 (1)
485-491: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe early success return skips the response log.
Lines 490-491 send HTTP 200 and return. Every other exit path in this handler calls
logging.LogHTTPResponse. This path does not. Request tracing loses the response record for the common "nothing to archive" case.Proposed fix
if strings.Contains(err.Error(), "not found") { logger.InfoContext(traceCtx, "no instances found to archive for deployment", zap.String("functionVersionId", functionVersionId.String()), zap.String("deploymentId", deploymentId.String())) - w.WriteHeader(http.StatusOK) + okStatus := http.StatusOK + w.WriteHeader(okStatus) + logging.LogHTTPResponse(traceCtx, logger, okStatus, w.Header()) return } else {As per path instructions: "Check Go error wrapping (%w), structured logging with required context fields (request/function/cluster/org id), and that request-handling changes add logs, tracing, and RED metrics per AGENTS.md."
🤖 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 `@src/control-plane-services/event-ledger/cmd/api/service/v2.go` around lines 485 - 491, Update the early “not found” success branch in the handler containing the archive flow to call logging.LogHTTPResponse before writing HTTP 200 and returning. Preserve the existing success response and ensure the response log includes the request’s established context and required fields, matching the other exit paths.Source: Path instructions
src/control-plane-services/event-ledger/cmd/api/service/v3_test.go-750-762 (1)
750-762: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace the if/else chain with a tagged switch.
staticcheck reports QF1003 at error level for this block.
Proposed fix
- if ctx.EventName == "pending" { - pendingCount++ - } else if ctx.EventName == "ready" { - readyCount++ - } + switch ctx.EventName { + case "pending": + pendingCount++ + case "ready": + readyCount++ + }🤖 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 `@src/control-plane-services/event-ledger/cmd/api/service/v3_test.go` around lines 750 - 762, Update the event-counting logic in the response.Contexts loop to replace the if/else chain on ctx.EventName with a tagged switch, preserving the existing pendingCount and readyCount increments and all assertions.Source: Linters/SAST tools
src/control-plane-services/event-ledger/internal/config/config_test.go-88-106 (1)
88-106: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
t.Setenvand check theBindEnverror.Two points:
- golangci-lint reports the unchecked
os.Unsetenvreturn at Line 89.t.Setenvrestores the previous value when the subtest ends, so the manualos.Setenv,os.Unsetenv, and the deferred cleanup are not needed.TestCloudEventsEnabledConfigurationalready usest.Setenvat Line 185.- Line 106 discards the
v.BindEnverror. Line 190 wraps the same call inrequire.NoError. Apply the same check here.Proposed change
- // Clear any existing environment variables - os.Unsetenv("INDEXER_ENABLED") - // Set test environment variables for key, value := range tt.envVars { - os.Setenv(key, value) + t.Setenv(key, value) } - defer func() { - for key := range tt.envVars { - os.Unsetenv(key) - } - }() // Create a new viper instance and cobra command for isolation v := viper.New() v.AutomaticEnv() // Enable environment variable binding - v.BindEnv("indexer.enabled", "INDEXER_ENABLED") + require.NoError(t, v.BindEnv("indexer.enabled", "INDEXER_ENABLED"))Remove the
osimport if no other reference remains. Line 182 also callsos.Unsetenv; apply the same treatment there.🤖 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 `@src/control-plane-services/event-ledger/internal/config/config_test.go` around lines 88 - 106, Update the environment setup in the affected test subtests to use t.Setenv for INDEXER_ENABLED and each test variable, removing the manual os.Setenv/os.Unsetenv cleanup and the os import if unused; also replace v.BindEnv in the relevant configuration tests with a require.NoError check, including the setup around TestCloudEventsEnabledConfiguration.Source: Linters/SAST tools
src/control-plane-services/event-ledger/internal/middleware/jwt.go-334-351 (1)
334-351: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winAn empty scope requirement fails open under
RequireAllScopes.strings.Fields("")returns an empty slice, so the loop never runs and the function returns true. A route wired with an emptyScopesvalue admits every caller.hasAnyRequiredScopereturns false for the same input, so the two matchers disagree.
src/control-plane-services/event-ledger/internal/middleware/jwt.go#L334-L351: return false whenrequiredScopesListis empty, so a wiring mistake fails closed.src/control-plane-services/event-ledger/internal/middleware/jwt_test.go#L109-L114: change the "Empty required scopes" expectation from true to false.🤖 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 `@src/control-plane-services/event-ledger/internal/middleware/jwt.go` around lines 334 - 351, The hasAllRequiredScopes matcher currently returns true for an empty required scope list; return false immediately when requiredScopesList is empty so RequireAllScopes fails closed. Update the “Empty required scopes” expectation in src/control-plane-services/event-ledger/internal/middleware/jwt_test.go lines 109-114 from true to false.src/control-plane-services/event-ledger/internal/credentials/bearer_test.go-112-112 (1)
112-112: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCheck the
Closereturn value to satisfyerrcheck.
golangci-lintreports both deferredr.Close()calls. The lint gate applies to test files too.Proposed fix
- defer r.Close() + defer func() { _ = r.Close() }()Also applies to: 136-136
🤖 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 `@src/control-plane-services/event-ledger/internal/credentials/bearer_test.go` at line 112, Update both deferred r.Close() calls in the test to explicitly handle the returned error so they satisfy errcheck, preserving the existing cleanup behavior and test flow.Source: Linters/SAST tools
src/control-plane-services/event-ledger/internal/middleware/body_limit.go-24-31 (1)
24-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMap oversized request bodies to HTTP 413.
The V1, V2, and V3 CloudEvents handlers map decoder errors to HTTP 400 without checking
*http.MaxBytesError. Only the V3 Kubernetes handler returns HTTP 413. Update the affected handlers and correct this middleware comment.🤖 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 `@src/control-plane-services/event-ledger/internal/middleware/body_limit.go` around lines 24 - 31, Update BodyLimitMiddleware’s comment to accurately state that http.MaxBytesReader limits the body and that handlers must map oversize errors to 413. In the V1, V2, and V3 CloudEvents handlers, detect *http.MaxBytesError before the generic decoder-error response and return HTTP 413; preserve HTTP 400 for other decoding errors, matching the existing V3 Kubernetes handler behavior.src/control-plane-services/event-ledger/internal/middleware/policy_test.go-1097-1117 (1)
1097-1117: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe comment contradicts the production behavior for a nil policy client.
Lines 1099 and 1100 state that a nil policy client passed to
NewPolicyMiddlewarecreates a pass-through middleware that skips authorization.policy.golines 176 to 185 do the opposite: a nil client produces a deny-all handler that returns 503.TestNewPolicyMiddlewareRejectsRequestsWithNilClientAndLoggerat line 757 confirms the deny-all behavior.Only
testPolicyMiddlewarepasses through. A maintainer who reads this comment may conclude that a nil client disables authentication in production. Correct the comment and rename the test to describe the fake middleware.🤖 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 `@src/control-plane-services/event-ledger/internal/middleware/policy_test.go` around lines 1097 - 1117, Update TestDisableAuthentication_Policy’s comments and rename the test to reflect that only testPolicyMiddleware provides pass-through behavior; a nil client passed to NewPolicyMiddleware creates a deny-all handler returning 503. Preserve the test’s existing setup and assertions while describing the fake middleware behavior accurately.src/control-plane-services/event-ledger/internal/observability/logging/http.go-63-73 (1)
63-73: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
requestURLForLoggingpreserves URL userinfo. The helper clearsRawQuery,ForceQuery, andFragment, but it keepsurl.User. A URL that carries userinfo therefore writesuser:passwordinto the log line.
src/control-plane-services/event-ledger/internal/observability/logging/http.go#L63-L73: setsanitized.User = nilbefore callingsanitized.String().src/control-plane-services/event-ledger/internal/observability/logging/http_test.go#L56-L61: add a case that parses a URL with userinfo and asserts that the returned string contains no credentials.🤖 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 `@src/control-plane-services/event-ledger/internal/observability/logging/http.go` around lines 63 - 73, Update requestURLForLogging in src/control-plane-services/event-ledger/internal/observability/logging/http.go (lines 63-73) to clear sanitized.User before returning sanitized.String(), preventing credentials from appearing in logs. Add a test case in src/control-plane-services/event-ledger/internal/observability/logging/http_test.go (lines 56-61) that parses a URL containing userinfo and asserts the logged URL contains no credentials.src/control-plane-services/event-ledger/internal/observability/logging/http_test.go-36-51 (1)
36-51: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the non-canonical header key flagged by staticcheck.
Line 37 and line 50 use
"X-ApiKey".http.Headerkeys are canonicalized bynet/http, and the canonical form isX-Apikey. staticcheck reports SA1008 for line 50. The test passes today only becausesanitizeHeadersranges over the literal map keys, so real traffic is not represented.Use the canonical key so the test matches the runtime key format.
💚 Proposed fix
- "X-ApiKey": []string{"api-secret"}, + "X-Apikey": []string{"api-secret"},- assert.Equal(t, []string{redactedHeaderValue}, sanitized["X-ApiKey"]) + assert.Equal(t, []string{redactedHeaderValue}, sanitized["X-Apikey"])🤖 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 `@src/control-plane-services/event-ledger/internal/observability/logging/http_test.go` around lines 36 - 51, Replace the non-canonical "X-ApiKey" key in the test header map and its corresponding sanitized lookup assertion with the net/http canonical form "X-Apikey", keeping the existing redaction expectations unchanged.Source: Linters/SAST tools
src/control-plane-services/event-ledger/internal/middleware/policy.go-386-413 (1)
386-413: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRecord authorization outcomes on the existing
evaluatespan.
authzHTTPClient.Evaluatealready creates the outbound span. Add authorization result attributes and record errors with error status on failure paths. Do not add a duplicate middleware span. Document thatdummyResponseWriter.WriteHeaderintentionally ignores status codes becausejwtClaims == nildetermines validation failure.🤖 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 `@src/control-plane-services/event-ledger/internal/middleware/policy.go` around lines 386 - 413, Update the existing evaluate span created by authzHTTPClient.Evaluate to attach authorization result attributes and record errors with an error status on every failure path, without creating another middleware span. Add a comment to dummyResponseWriter.WriteHeader explaining that ignored status codes are intentional because jwtClaims == nil determines validation failure.Source: Coding guidelines
src/control-plane-services/event-ledger/internal/policy/authz_client.go-230-230 (1)
230-230: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUnchecked error return values fail golangci-lint 2.12.2 errcheck. Four deferred or inline calls discard their error return. The shared root cause is the same: the
errchecklinter is enabled and these call sites do not acknowledge the returned error.
src/control-plane-services/event-ledger/internal/policy/authz_client.go#L230-L230: changedefer resp.Body.Close()todefer func() { _ = resp.Body.Close() }().src/control-plane-services/event-ledger/internal/policy/static_bearer_client.go#L83-L83: changedefer resp.Body.Close()todefer func() { _ = resp.Body.Close() }().src/control-plane-services/event-ledger/internal/policy/static_bearer_client_test.go#L43-L43: change the cleanup tot.Cleanup(func() { _ = r.Close() }).src/control-plane-services/event-ledger/internal/policy/static_bearer_client_test.go#L94-L94: assign the encode result, for example_ = json.NewEncoder(w).Encode(tc.serverResponse).If the repository prefers to exclude
errcheckforCloseon read-only bodies, add the exclusion to the golangci-lint configuration instead of changing each site.🤖 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 `@src/control-plane-services/event-ledger/internal/policy/authz_client.go` at line 230, Update the unchecked error-return call sites to satisfy errcheck: in src/control-plane-services/event-ledger/internal/policy/authz_client.go:230 and static_bearer_client.go:83, wrap response-body closes in deferred functions that explicitly discard the returned error; in static_bearer_client_test.go:43, wrap r.Close in t.Cleanup with explicit error acknowledgment; and at static_bearer_client_test.go:94, explicitly assign the JSON encoder result. Alternatively, configure golangci-lint to exclude Close errors for read-only bodies if that is the repository convention.Source: Linters/SAST tools
src/control-plane-services/event-ledger/internal/policy/static_bearer_client_test.go-107-113 (1)
107-113: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe success case does not assert the parsed response.
Line 107 discards the
*pdpv1.RuleResponse. The case named "returns parsed response on success" only checks that no error occurred. Assert the decoded value so a regression in the unmarshal path is caught.💚 Proposed fix
- _, err := client.Evaluate(context.Background(), req) + resp, err := client.Evaluate(context.Background(), req) if tc.wantErr { assert.Error(t, err) return } require.NoError(t, err) + require.NotNil(t, resp)Add a field to the table for the expected decoded response and assert it for the success 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 `@src/control-plane-services/event-ledger/internal/policy/static_bearer_client_test.go` around lines 107 - 113, The success branch of the table-driven test around client.Evaluate currently discards the decoded *pdpv1.RuleResponse. Capture the returned response, add expected decoded-response data to the relevant test cases, and assert the actual response against it when tc.wantErr is false, while preserving the existing error assertions.src/control-plane-services/event-ledger/cmd/api/startup/run_service.go-99-104 (1)
99-104: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClose the Cassandra session during shutdown. After
InitConnssucceeds, deferconns.DbHandler.Close()before registering publisher and CloudEvents cleanup so those components stop before the Cassandra session closes.🤖 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 `@src/control-plane-services/event-ledger/cmd/api/startup/run_service.go` around lines 99 - 104, After successful service.InitConns in the startup flow, defer conns.DbHandler.Close() before registering publisher and CloudEvents cleanup, ensuring those components shut down before the Cassandra session.src/control-plane-services/event-ledger/internal/db_client/cassandra/v2_resilience_test.go-150-161 (1)
150-161: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winMove the connection settings out of the test source.
Lines 151 to 161 hardcode the host, keyspace, username, and password. AGENTS.md states that allowlisted Event Ledger files are public content and must exclude credentials and deployment details. Even though
cassandra/cassandrais the well-known local default, a literal credential pair in committed source conflicts with that rule and makes the test unusable against any other cluster.Read the host, keyspace, username, and password from environment variables with local defaults.
🤖 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 `@src/control-plane-services/event-ledger/internal/db_client/cassandra/v2_resilience_test.go` around lines 150 - 161, Update the Cassandra cluster setup in the resilience test to read host, keyspace, username, and password from environment variables, using local-development defaults when variables are unset. Remove the hardcoded connection values from the cluster and gocql.PasswordAuthenticator configuration while preserving the existing consistency and timeout settings.Source: Coding guidelines
src/control-plane-services/event-ledger/internal/db_client/cassandra/v2.go-345-348 (1)
345-348: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe comment contradicts the code.
Line 346 states that the method returns an empty slice and a nil error. Line 347 returns
fmt.Errorf("not found"). Correct the comment so it describes the actual behavior.📝 Proposed fix
if len(instances) == 0 { - // Return empty slice and nil error if not found, consistent with V1 ListInstances behavior + // Return a "not found" error when no instances match, consistent with V1 ListInstances behavior return instances, fmt.Errorf("not found") }🤖 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 `@src/control-plane-services/event-ledger/internal/db_client/cassandra/v2.go` around lines 345 - 348, The comment in the instances-empty branch incorrectly says the method returns a nil error; update it to accurately describe that an empty slice is returned with a “not found” error, without changing the return behavior.src/control-plane-services/event-ledger/internal/db_client/cassandra/v2_resilience_test.go-97-99 (1)
97-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the redundant assignment in the range statement.
Line 97 uses
for _ = range iterator3. Go permitsfor range iterator3.gofmt -sand staticcheck S1005 both flag the current form, so a lint stage will fail.♻️ Proposed fix
- for _ = range iterator3 { + for range iterator3 { 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 `@src/control-plane-services/event-ledger/internal/db_client/cassandra/v2_resilience_test.go` around lines 97 - 99, Remove the redundant blank-identifier assignment from the range loop in the iterator3 counting block, using Go’s direct range form while preserving the existing count increment behavior.src/control-plane-services/event-ledger/internal/db_client/cassandra/common.go-447-470 (1)
447-470: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRecord query failures on the span.
When
info.Err != nil, callspan.RecordError(info.Err), setcodes.Error, and addattribute.Bool("error", true). Failed query spans currently have no failure status.HostInfo.String()is not nil-safe, but gocql invokes this observer only after selecting a connection, so the proposed host guard is not required.🤖 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 `@src/control-plane-services/event-ledger/internal/db_client/cassandra/common.go` around lines 447 - 470, Update OTelQueryObserver.ObserveQuery to handle info.Err != nil by recording the error on span, setting its status to codes.Error, and adding attribute.Bool("error", true); leave the existing host handling and successful-query attributes unchanged.Source: Coding guidelines
src/control-plane-services/event-ledger/internal/publisher/publisher.go-203-215 (1)
203-215: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSkip the final write when the batch is empty.
On channel close the worker calls
writeBatchwithout checking the length. Every storage client then receives aStoreBatchcall with zero events on shutdown.CloudEventsStorageClient.sendEventsreturns early, but otherBatchStorageClientimplementations may issue an empty query or an empty HTTP request.Proposed fix for the final flush
if !ok { // Channel closed, final batch processing p.logger.Warn("publisher event channel closed") - writeBatch("writing final batch") + if len(batch) > 0 { + writeBatch("writing final batch") + } return }Apply the same guard in
runV2at line 260.Also applies to: 256-268
🤖 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 `@src/control-plane-services/event-ledger/internal/publisher/publisher.go` around lines 203 - 215, Update the channel-close handling in the publisher worker and the corresponding final-flush path in runV2 to call writeBatch only when the accumulated batch is non-empty. Preserve the warning and return behavior, while preventing empty batches from reaching storage clients during shutdown.src/control-plane-services/event-ledger/internal/publisher/cloudevents/resilient_client.go-127-131 (1)
127-131: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDo not log and return the same error.
Both fallback paths log
storeErrat error level and then return an error that wrapsstoreErr. The caller logs it again, which duplicates the record. Keep the wrapped return value and remove the local log call.Proposed fix for the duplicate error record
if storeErr := c.resilienceHandler.StoreFailedCloudEvent(ctx, "v1", eventData); storeErr != nil { - c.logger.ErrorContext(ctx, "failed to store CloudEvents in Cassandra fallback", zap.Error(storeErr)) // Return the original CloudEvents error, not the Cassandra error return fmt.Errorf("CloudEvents failed: %w (also failed to store fallback: %w)", err, storeErr) }Apply the same change in
StoreBatchV2at line 164.As per coding guidelines: "When logging errors, preserve the originating error with
%wor an equivalent wrapping mechanism and do not both log and return the same error."Also applies to: 163-167
🤖 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 `@src/control-plane-services/event-ledger/internal/publisher/cloudevents/resilient_client.go` around lines 127 - 131, Remove the local logger.ErrorContext call for storeErr in both the single-event fallback and StoreBatchV2 fallback paths, while preserving the existing wrapped error returns that include storeErr. Ensure each fallback propagates the wrapped error without logging it locally.Source: Coding guidelines
src/control-plane-services/event-ledger/internal/publisher/cloudevents/client.go-79-79 (1)
79-79: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winSeparate the two parts of the CloudEvents
type.
event.Event + event.EventTypeconcatenates without a delimiter. ForEvent="ready"andEventType="sis"the type becomesreadysis. Consumers cannot parse the parts, and the value does not follow the reverse-DNS convention that the CloudEvents specification recommends.Proposed fix for the type attribute
- newEvent.SetType(event.Event + event.EventType) + newEvent.SetType(fmt.Sprintf("com.nvidia.nvcf.event-ledger.v1.%s.%s", event.EventType, event.Event))Also applies to: 97-97
🤖 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 `@src/control-plane-services/event-ledger/internal/publisher/cloudevents/client.go` at line 79, Update the CloudEvents type assignments in the client’s event-publishing flow, including both occurrences of newEvent.SetType, to join event.Event and event.EventType with a delimiter that preserves separate, parseable parts and follows the project’s established CloudEvents naming convention.src/control-plane-services/event-ledger/internal/publisher/cloudevents/resilient_client.go-74-96 (1)
74-96: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReject a nil
metricsargument in the constructor.
StoreBatchandStoreBatchV2dereferencec.metrics.EventsFallbackCounteron every fallback, andprocessFailedEventsdereferences the skipped and retried counters. A nilmetricsvalue panics on the first CloudEvents failure, which is the exact moment the fallback must work. Validate the argument in the constructor.Proposed fix for the constructor guard
) (*ResilientCloudEventsClient, error) { + if metrics == nil { + return nil, fmt.Errorf("cloudevents metrics are required") + }🤖 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 `@src/control-plane-services/event-ledger/internal/publisher/cloudevents/resilient_client.go` around lines 74 - 96, Update NewResilientCloudEventsClient to validate that the metrics argument is non-nil before constructing and returning the client; return a descriptive error when it is nil, while preserving the existing CloudEvents client creation and initialization flow for valid metrics.src/control-plane-services/event-ledger/internal/publisher/cloudevents/client_test.go-81-89 (1)
81-89: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not call
requireinside the HTTP handler goroutine.
require.Equalandrequire.NoErrorcallt.FailNow, which callsruntime.Goexit. The Go testing documentation requiresFailNowto run on the goroutine that runs the test. From the server handler the failure aborts only the handler goroutine, so the test can hang until the client timeout or report a misleading result. Record the values in the handler and assert afterStoreBatchreturns.Proposed fix for the handler assertions
+ var receivedMethod string + var decodeErr error server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { receivedContentType = r.Header.Get("Content-Type") - require.Equal(t, http.MethodPost, r.Method) - - err := json.NewDecoder(r.Body).Decode(&receivedEvents) - require.NoError(t, err) - + receivedMethod = r.Method + decodeErr = json.NewDecoder(r.Body).Decode(&receivedEvents) w.WriteHeader(http.StatusAccepted) }))Then assert
receivedMethodanddecodeErrnext to the existing assertions at line 112.🤖 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 `@src/control-plane-services/event-ledger/internal/publisher/cloudevents/client_test.go` around lines 81 - 89, Remove the require.Equal and require.NoError calls from the httptest server handler. In the handler, record the request method and JSON decode error in variables such as receivedMethod and decodeErr; after StoreBatch returns, assert those captured values alongside the existing assertions, while preserving the handler’s response behavior.src/control-plane-services/event-ledger/internal/publisher/publisher_test.go-147-174 (1)
147-174: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove the timing dependency in the call-count assertion.
BatchIntervalSecondsis 1 in this test. AfterEventuallyobserves the first call, the interval tick can still fire before line 172 and flush the fourth event. The count then becomes 3 and the assertion at line 173 fails on a loaded machine. Use a long interval, asTestBatchedPublisher_Publish_BatchFulldoes, so only the batch-size trigger and the final flush produce calls.Line 173 also reverses the testify argument order.
assert.Equaltakes the expected value first, so a failure message shows the values swapped.Proposed fix for the test
config := config2.BatchedPublisherConfig{ QueueSize: 10, BatchSize: 3, - BatchIntervalSeconds: 1, + BatchIntervalSeconds: 60, // Long interval so only batch-full and Stop trigger writes }- assert.Equal(t, errClient.GetCallCount(), 2, "Client should have been called twice, once for the batch and once for the final flush") + assert.Equal(t, 2, errClient.GetCallCount(), "Client should have been called twice, once for the batch and once for the final flush")🤖 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 `@src/control-plane-services/event-ledger/internal/publisher/publisher_test.go` around lines 147 - 174, Update the test’s BatchedPublisherConfig in the error-handling case to use a long batch interval, matching TestBatchedPublisher_Publish_BatchFull, so only the batch-size trigger and final flush invoke the client. Correct the final assert.Equal call after p.Stop so the expected call count is the first argument and the observed errClient.GetCallCount() is second.src/control-plane-services/event-ledger/internal/publisher/cloudevents/client.go-239-266 (1)
239-266: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winHonor
CredentialsRefreshIntervalindependently of token expiry.
NewCloudEventsStorageClientdoes not pass the interval toclientCredentialsTokenSource.accessTokenreloads the credentials file only when the cached token nears expiry, so rotated credentials can remain unused past the configured interval.🤖 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 `@src/control-plane-services/event-ledger/internal/publisher/cloudevents/client.go` around lines 239 - 266, Update NewCloudEventsStorageClient to pass CredentialsRefreshInterval into clientCredentialsTokenSource, then update accessToken to reload credentials when that interval elapses independently of cached token expiry. Preserve token reuse while credentials remain within the interval, and ensure rotated credentials are picked up even when the current token has not neared expiration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 21d4b4b2-f98f-4d4c-b44b-726e16b87c7a
⛔ Files ignored due to path filters (2)
src/control-plane-services/event-ledger/common/go.sumis excluded by!**/*.sumsrc/control-plane-services/event-ledger/go.sumis excluded by!**/*.sum
📒 Files selected for processing (85)
src/control-plane-services/event-ledger/.dockerignoresrc/control-plane-services/event-ledger/Dockerfilesrc/control-plane-services/event-ledger/LICENSEsrc/control-plane-services/event-ledger/NOTICEsrc/control-plane-services/event-ledger/README.mdsrc/control-plane-services/event-ledger/cmd/api/error/error.gosrc/control-plane-services/event-ledger/cmd/api/error/error_test.gosrc/control-plane-services/event-ledger/cmd/api/main.gosrc/control-plane-services/event-ledger/cmd/api/service/common.gosrc/control-plane-services/event-ledger/cmd/api/service/health.gosrc/control-plane-services/event-ledger/cmd/api/service/service.gosrc/control-plane-services/event-ledger/cmd/api/service/service_test.gosrc/control-plane-services/event-ledger/cmd/api/service/servicev2_test.gosrc/control-plane-services/event-ledger/cmd/api/service/status.gosrc/control-plane-services/event-ledger/cmd/api/service/v1.gosrc/control-plane-services/event-ledger/cmd/api/service/v1_test.gosrc/control-plane-services/event-ledger/cmd/api/service/v2.gosrc/control-plane-services/event-ledger/cmd/api/service/v3.gosrc/control-plane-services/event-ledger/cmd/api/service/v3_test.gosrc/control-plane-services/event-ledger/cmd/api/startup/root_cmd.gosrc/control-plane-services/event-ledger/cmd/api/startup/root_cmd_test.gosrc/control-plane-services/event-ledger/cmd/api/startup/run_service.gosrc/control-plane-services/event-ledger/cmd/toolbox/generate-config.gosrc/control-plane-services/event-ledger/common/core/types/types.gosrc/control-plane-services/event-ledger/common/core/utils/utils.gosrc/control-plane-services/event-ledger/common/core/utils/utils_test.gosrc/control-plane-services/event-ledger/common/go.modsrc/control-plane-services/event-ledger/go.modsrc/control-plane-services/event-ledger/internal/config/auth_config_test.gosrc/control-plane-services/event-ledger/internal/config/cliargs.gosrc/control-plane-services/event-ledger/internal/config/cloudevents.gosrc/control-plane-services/event-ledger/internal/config/cloudevents_test.gosrc/control-plane-services/event-ledger/internal/config/config.gosrc/control-plane-services/event-ledger/internal/config/config_test.gosrc/control-plane-services/event-ledger/internal/config/http.gosrc/control-plane-services/event-ledger/internal/config/publisher.gosrc/control-plane-services/event-ledger/internal/configutil/utils.gosrc/control-plane-services/event-ledger/internal/credentials/bearer.gosrc/control-plane-services/event-ledger/internal/credentials/bearer_test.gosrc/control-plane-services/event-ledger/internal/data_access/db.gosrc/control-plane-services/event-ledger/internal/db_client/cassandra/common.gosrc/control-plane-services/event-ledger/internal/db_client/cassandra/v1.gosrc/control-plane-services/event-ledger/internal/db_client/cassandra/v1_security_test.gosrc/control-plane-services/event-ledger/internal/db_client/cassandra/v2.gosrc/control-plane-services/event-ledger/internal/db_client/cassandra/v2_resilience_test.gosrc/control-plane-services/event-ledger/internal/db_client/cassandra/v2_test.gosrc/control-plane-services/event-ledger/internal/interfaces/publisher.gosrc/control-plane-services/event-ledger/internal/middleware/body_limit.gosrc/control-plane-services/event-ledger/internal/middleware/constants.gosrc/control-plane-services/event-ledger/internal/middleware/cors.gosrc/control-plane-services/event-ledger/internal/middleware/http_client.gosrc/control-plane-services/event-ledger/internal/middleware/jwt.gosrc/control-plane-services/event-ledger/internal/middleware/jwt_test.gosrc/control-plane-services/event-ledger/internal/middleware/metrics.gosrc/control-plane-services/event-ledger/internal/middleware/metrics_test.gosrc/control-plane-services/event-ledger/internal/middleware/policy.gosrc/control-plane-services/event-ledger/internal/middleware/policy_test.gosrc/control-plane-services/event-ledger/internal/observability/common/config.gosrc/control-plane-services/event-ledger/internal/observability/common/utils.gosrc/control-plane-services/event-ledger/internal/observability/logging/config.gosrc/control-plane-services/event-ledger/internal/observability/logging/http.gosrc/control-plane-services/event-ledger/internal/observability/logging/http_test.gosrc/control-plane-services/event-ledger/internal/observability/logging/logger.gosrc/control-plane-services/event-ledger/internal/observability/logging/setup.gosrc/control-plane-services/event-ledger/internal/observability/tracing/config.gosrc/control-plane-services/event-ledger/internal/observability/tracing/tracing.gosrc/control-plane-services/event-ledger/internal/policy/authz_client.gosrc/control-plane-services/event-ledger/internal/policy/authz_client_test.gosrc/control-plane-services/event-ledger/internal/policy/static_bearer_client.gosrc/control-plane-services/event-ledger/internal/policy/static_bearer_client_test.gosrc/control-plane-services/event-ledger/internal/publisher/cloudevents/client.gosrc/control-plane-services/event-ledger/internal/publisher/cloudevents/client_test.gosrc/control-plane-services/event-ledger/internal/publisher/cloudevents/resilient_client.gosrc/control-plane-services/event-ledger/internal/publisher/cloudevents/resilient_client_test.gosrc/control-plane-services/event-ledger/internal/publisher/publisher.gosrc/control-plane-services/event-ledger/internal/publisher/publisher_test.gosrc/control-plane-services/event-ledger/internal/registrations/registrations.gosrc/control-plane-services/event-ledger/pkg/codex/codex.gosrc/control-plane-services/event-ledger/pkg/codex/codex_test.gosrc/control-plane-services/event-ledger/pkg/codex/stage_transition_event_schema.gosrc/control-plane-services/event-ledger/pkg/codex/stage_transition_event_schema_test.gosrc/control-plane-services/event-ledger/pkg/constants/service.gosrc/control-plane-services/event-ledger/pkg/error/error.gosrc/control-plane-services/event-ledger/pkg/error/error_test.gosrc/control-plane-services/event-ledger/pkg/testutils/testutils.go
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
Signed-off-by: Bora Oztekin <boztekin@nvidia.com>
3 participants
TL;DR
Add the Event Ledger service as a native NVCF subproject for recording and querying distributed function lifecycle events.
This PR implements #82.
Additional Details
Event Ledger provides a shared record of NVCA and service-generated lifecycle events across distributed NVCF components.
This PR:
src/control-plane-services/event-ledger.The JWT provider is limited to the tenant-aware v3 API. Cassandra schema provisioning and deployment integration remain separate work.
The existing Event Ledger
go.modandgo.sumdependency set is included without version changes. The subtree includes its Apache 2.0LICENSEandNOTICE. Repository CI should confirm third-party dependency license compatibility.For the Reviewer
Please focus on:
internal/middleware.For QA
Tests run:
Both test suites pass.
No live Cassandra, OpenBao, or deployed-stack QA was performed. Deployment integration should be validated when the required schema and stack configuration are added.
Issues
Relates to #82
Checklist
Summary by CodeRabbit