feat(nvca): configure Secret-backed workload transport trust - #672
feat(nvca): configure Secret-backed workload transport trust#672mikeyrcamp wants to merge 2 commits into
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Helm configuration for workload transport TLS trust bundles and mount paths. Validates and injects certificate mounts into worker pods. Maps Secret-backed settings into agent configuration and triggers reconciliation for configuration changes. ChangesTransport TLS configuration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Helm
participant OperatorConfigMap
participant NVCAConfigMapper
participant KubernetesSecret
participant AgentConfigMap
participant WorkerPod
Helm->>OperatorConfigMap: Render transportTLS settings
NVCAConfigMapper->>OperatorConfigMap: Read and decode configuration
NVCAConfigMapper->>KubernetesSecret: Resolve trust-bundle data
NVCAConfigMapper->>AgentConfigMap: Apply validated transportTLS configuration
AgentConfigMap->>WorkerPod: Provide configured certificate mount and path
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="Running error: context loading failed: failed to load packages: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in /src/compute-plane-services/nvca:\n\tgithub.com/NVIDIA/KAI-scheduler@v0.12.6: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/NVIDIA/k8s-dra-driver-gpu@v0.0.0-20251017125642-cfe35ffd3d2c: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/NVIDIA/nvcf/src/libraries/go/lib@v0.0.0-20260722095202-f5e2792f5630: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/aws/aws-sdk-go@v1.55.5: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/bombsimon/logrusr/v4@v4.1.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt\n\tgithub.com/evanphx/json-patch/v5@v5.9.11: is explicitly required in ... [truncated 21721 characters] ... i: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/apiextensions-apiserver: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/apimachinery: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/client-go: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tk8s.io/component-base: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tsigs.k8s.io/controller-runtime: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\tgolang.org/x/crypto: is replaced in go.mod, but not marked as replaced in vendor/modules.txt\n\n\tTo ignore the vendor directory, use -mod=readonly or -mod=mod.\n\tTo sync the vendor directory, run:\n\t\tgo mod vendor\n" Comment |
30a0e8d to
3371a7c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/compute-plane-services/nvca/pkg/operator/reconcile/transport_tls_config_test.go (1)
294-312: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid asserting on the incidental
version cannot be emptyerror.Both tests prove that reconciliation ran through the finalizer assertion on the stored backend. The error text comes from unrelated backend validation deep in the reconcile path. That assertion breaks when the message changes, for a reason unrelated to ConfigMap event handling.
♻️ Proposed change
- require.ErrorContains(t, err, "version cannot be empty") + require.Error(t, err, "reconcile must run and report the incomplete test backend")🤖 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/compute-plane-services/nvca/pkg/operator/reconcile/transport_tls_config_test.go` around lines 294 - 312, Update the ConfigMap event tests around handleConfigMapAdd and handleConfigMapUpdate to stop asserting the incidental “version cannot be empty” error text. Assert only the outcome relevant to reconciliation—successful handler execution or a non-message-specific error expectation—while preserving the stored backend finalizer assertion and existing test setup.src/compute-plane-services/nvca/pkg/operator/reconcile/backendk8scache.go (1)
638-673: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeduplicate the three diff branches and fix the success log text.
The three cases repeat the same
cmp.Diffand logging block. Lines 647, 657 and 667 also log success text on the path where the diff is empty and no work happened. That text is misleading during triage.♻️ Proposed refactor sketch
+ diff := cmp.Diff(oldCM.Data, newCM.Data, cmpopts.EquateEmpty()) + log.WithField("diff", diff).Debug("configmap data diff") + if diff == "" { + log.Debug("configmap data unchanged, skipping") + return nil + } + switch { case configMapUpdateForcesNVCAReconcile(newCM.Name): - log.Debugf("found %s configmap update, syncing current NVCFBackend", newCM.Name) - diff := cmp.Diff(oldCM.Data, newCM.Data, cmpopts.EquateEmpty()) - log.WithField("diff", diff).Debugf("configmap data diff") - if diff != "" { - log.Info("configmap data has changed, forcing rollout") - return c.syncCurrentBackendForConfigMapChange(ctx, log) - } - log.Debug("successfully synced current NVCFBackend") - return nil + log.Info("configmap data has changed, forcing rollout") + return c.syncCurrentBackendForConfigMapChange(ctx, log) case newCM.Name == nvcfBackendHelmManagedConfigMapName, newCM.Name == nvcfBackendSelfManagedConfigMapName: - ... + log.Infof("configmap %s data has changed, dispatch cluster reconcile event", newCM.Name) + c.dispatchReconcileClusterFunc(ctx) + return nil }Keep the shutdown sentinel check before the diff computation so that path stays a no-op.
🤖 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/compute-plane-services/nvca/pkg/operator/reconcile/backendk8scache.go` around lines 638 - 673, Refactor the config-map update handling around configMapUpdateForcesNVCAReconcile and the nvcfBackendHelmManagedConfigMapName/nvcfBackendSelfManagedConfigMapName cases to compute and log cmp.Diff once after preserving the cleanup.ShutdownSentinelConfigMapName early return. Keep each branch’s existing action when diff is non-empty, but remove or revise the misleading success logs so empty diffs do not claim synchronization or event dispatch occurred.src/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile.go (2)
1437-1445: 🩺 Stability & Availability | 🔵 TrivialOperational note on live API reads per reconcile.
getAgentConfigToMergenow reads two ConfigMaps and one Secret through the typed client, and bothsetupAgentConfigConfigMapandnewAgentConfigChangedCheckcall it in the same reconcile. That is four uncached reads per reconcile. If reconcile rate grows, consider serving the ConfigMap reads from the existing operator-namespace informer cache and keeping only the Secret read live.🤖 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/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile.go` around lines 1437 - 1445, The reconcile path currently performs repeated live API reads for agent configuration. Update getRawAgentConfigToMerge and the related setupAgentConfigConfigMap/newAgentConfigChangedCheck flow to read the operator-namespace ConfigMaps from the existing informer cache, while retaining only the Secret lookup as a live typed-client read; preserve the current merge behavior and error handling.
1449-1452: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the ConfigMap name constant in the conflict error.
The message hardcodes
agent-config-merge.agentConfigMergeConfigMapNamealready holds that name and is used elsewhere in this package. The literal can drift from the constant.♻️ Proposed change
if mergeCfg.Workload.TransportTLS != nil { return nvcaconfig.Config{}, false, - fmt.Errorf("agent-config-merge and %s both configure workload.transportTLS", nvcaOperatorConfigMapName) + fmt.Errorf("%s and %s both configure workload.transportTLS", + agentConfigMergeConfigMapName, nvcaOperatorConfigMapName) }
TestGetAgentConfigToMerge_RejectsTransportTLSSourceConflictasserts only onboth configure workload.transportTLS, so the test stays green.🤖 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/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile.go` around lines 1449 - 1452, Update the conflict error in the agent configuration merge logic to use the existing agentConfigMergeConfigMapName constant instead of the hardcoded “agent-config-merge” string, while preserving the current error wording and behavior.src/compute-plane-services/nvca/pkg/operator/reconcile/nvca_config_mapper.go (1)
200-259: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider replacing the custom PEM scanner with
pem.Decodeiteration.
findCertificatePEMEndandisCompletePEMLinereimplement PEM block boundary detection. The stated goal is to reject leading or trailing non-PEM data thatpem.Decodeskips silently. One prefix check plus apem.Decodeloop reaches the same result with less code to maintain.♻️ Proposed simplification
func validateCertificateOnlyPEM(data []byte) error { - const ( - certificateBegin = "-----BEGIN CERTIFICATE-----" - certificateEnd = "-----END CERTIFICATE-----" - ) - + const certificateBegin = "-----BEGIN CERTIFICATE-----" remaining := bytes.TrimSpace(data) if len(remaining) == 0 { return fmt.Errorf("contains no certificates") } certificates := 0 for len(remaining) > 0 { - if !bytes.HasPrefix(remaining, []byte(certificateBegin)) || - !isCompletePEMLine(remaining, len(certificateBegin)) { + if !bytes.HasPrefix(remaining, []byte(certificateBegin)) { return fmt.Errorf("contains non-PEM data") } - end := findCertificatePEMEnd(remaining, certificateEnd) - if end == -1 || bytes.Contains(remaining[len(certificateBegin):end], []byte("\n-----BEGIN")) { - return fmt.Errorf("contains malformed certificate PEM") - } - pemBlock := remaining[:end+len("\n"+certificateEnd)] - block, rest := pem.Decode(pemBlock) - if block == nil || len(bytes.TrimSpace(rest)) != 0 { + block, rest := pem.Decode(remaining) + if block == nil { return fmt.Errorf("contains malformed certificate PEM") } if block.Type != "CERTIFICATE" { return fmt.Errorf("contains %q PEM block", block.Type) } if _, err := x509.ParseCertificate(block.Bytes); err != nil { return fmt.Errorf("parse certificate: %w", err) } certificates++ - remaining = bytes.TrimSpace(remaining[len(pemBlock):]) + remaining = bytes.TrimSpace(rest) }Keep the existing error strings, because
TestSetupAgentConfigConfigMap_SecretTrustFailurePreservesLastGoodConfigasserts oncontains non-PEM data.🤖 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/compute-plane-services/nvca/pkg/operator/reconcile/nvca_config_mapper.go` around lines 200 - 259, Replace the custom boundary scanning in validateCertificateOnlyPEM with iterative pem.Decode processing: require each remaining segment to begin with a complete CERTIFICATE PEM header, decode one block, validate its type and certificate contents, then continue with the undecoded remainder. Remove findCertificatePEMEnd and isCompletePEMLine if no longer needed, while preserving the existing error strings, including “contains non-PEM data,” and rejecting leading or trailing non-PEM content.src/compute-plane-services/nvca/pkg/operator/reconcile/BUILD.bazel (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePlace the new sources in sorted order.
Move
nvca_config_mapper.gobeforenvcaagent_reconcile.go, and movenvca_config_mapper_test.gobeforenvcaagent_reconcile_test.go. Run the repository's Bazel formatter.🤖 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/compute-plane-services/nvca/pkg/operator/reconcile/BUILD.bazel` at line 16, Update the source list in the BUILD file so nvca_config_mapper.go precedes nvcaagent_reconcile.go and nvca_config_mapper_test.go precedes nvcaagent_reconcile_test.go, then run the repository’s Bazel formatter.
🤖 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 `@src/compute-plane-services/nvca/pkg/operator/reconcile/backendk8scache.go`:
- Around line 693-697: Update the UpdateFunc callback around
cmnotel.InvokeWithSpan so the context supplied to its span callback is passed to
handleConfigMapUpdate instead of the outer ctx, matching the existing Add
handler propagation pattern.
- Around line 594-595: Update the error path in the reconciliation method
containing the “failed to sync current NVCFBackend” log to stop logging the
error before returning it. Return the originating err wrapped with contextual
text using the project’s supported error-wrapping mechanism, allowing the caller
to perform the single error log.
- Around line 584-586: Update configMapAddForcesNVCAReconcile so the ConfigMap
informer’s initial-list Add event does not force an agent rollout: return false
for nvcaOperatorConfigMapName, or add an explicit post-sync guard that prevents
forceRollout during initial-list Adds while preserving normal reconciliation.
---
Nitpick comments:
In `@src/compute-plane-services/nvca/pkg/operator/reconcile/backendk8scache.go`:
- Around line 638-673: Refactor the config-map update handling around
configMapUpdateForcesNVCAReconcile and the
nvcfBackendHelmManagedConfigMapName/nvcfBackendSelfManagedConfigMapName cases to
compute and log cmp.Diff once after preserving the
cleanup.ShutdownSentinelConfigMapName early return. Keep each branch’s existing
action when diff is non-empty, but remove or revise the misleading success logs
so empty diffs do not claim synchronization or event dispatch occurred.
In `@src/compute-plane-services/nvca/pkg/operator/reconcile/BUILD.bazel`:
- Line 16: Update the source list in the BUILD file so nvca_config_mapper.go
precedes nvcaagent_reconcile.go and nvca_config_mapper_test.go precedes
nvcaagent_reconcile_test.go, then run the repository’s Bazel formatter.
In
`@src/compute-plane-services/nvca/pkg/operator/reconcile/nvca_config_mapper.go`:
- Around line 200-259: Replace the custom boundary scanning in
validateCertificateOnlyPEM with iterative pem.Decode processing: require each
remaining segment to begin with a complete CERTIFICATE PEM header, decode one
block, validate its type and certificate contents, then continue with the
undecoded remainder. Remove findCertificatePEMEnd and isCompletePEMLine if no
longer needed, while preserving the existing error strings, including “contains
non-PEM data,” and rejecting leading or trailing non-PEM content.
In
`@src/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile.go`:
- Around line 1437-1445: The reconcile path currently performs repeated live API
reads for agent configuration. Update getRawAgentConfigToMerge and the related
setupAgentConfigConfigMap/newAgentConfigChangedCheck flow to read the
operator-namespace ConfigMaps from the existing informer cache, while retaining
only the Secret lookup as a live typed-client read; preserve the current merge
behavior and error handling.
- Around line 1449-1452: Update the conflict error in the agent configuration
merge logic to use the existing agentConfigMergeConfigMapName constant instead
of the hardcoded “agent-config-merge” string, while preserving the current error
wording and behavior.
In
`@src/compute-plane-services/nvca/pkg/operator/reconcile/transport_tls_config_test.go`:
- Around line 294-312: Update the ConfigMap event tests around
handleConfigMapAdd and handleConfigMapUpdate to stop asserting the incidental
“version cannot be empty” error text. Assert only the outcome relevant to
reconciliation—successful handler execution or a non-message-specific error
expectation—while preserving the stored backend finalizer assertion and existing
test setup.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c603c11b-8d41-4060-9aad-3e133a3c5df0
⛔ Files ignored due to path filters (1)
src/compute-plane-services/nvca/vendor/github.com/NVIDIA/nvcf/src/libraries/go/lib/pkg/types/nvca/config/types.gois excluded by!**/vendor/**
📒 Files selected for processing (22)
deploy/helm/nvca-operator/nvca-operator/README.mddeploy/helm/nvca-operator/nvca-operator/templates/operator-config-cm.yamldeploy/helm/nvca-operator/nvca-operator/values.schema.jsondeploy/helm/nvca-operator/nvca-operator/values.yamlsrc/compute-plane-services/nvca/deployments/nvca-operator/README.mdsrc/compute-plane-services/nvca/deployments/nvca-operator/templates/operator-config-cm.yamlsrc/compute-plane-services/nvca/deployments/nvca-operator/values.schema.jsonsrc/compute-plane-services/nvca/deployments/nvca-operator/values.yamlsrc/compute-plane-services/nvca/internal/miniservice/transport_tls_test.gosrc/compute-plane-services/nvca/internal/transporttls/BUILD.bazelsrc/compute-plane-services/nvca/internal/transporttls/transport_tls.gosrc/compute-plane-services/nvca/internal/transporttls/transport_tls_test.gosrc/compute-plane-services/nvca/pkg/nvca/transport_tls_test.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/BUILD.bazelsrc/compute-plane-services/nvca/pkg/operator/reconcile/backendk8scache.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/nvca_config_mapper.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/nvca_config_mapper_test.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/rollout.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/transport_tls_config_test.gosrc/compute-plane-services/nvca/scripts/lint_helm.shsrc/libraries/go/lib/pkg/types/nvca/config/types.go
|
@coderabbitai Thanks — I agree with the three actionable findings in
I am merging this PR and will address these in a focused fast-follow. The remaining suggestions are non-blocking cleanup items and are out of scope for this follow-up. |
|
Tip For best results, initiate chat on the files or code changes.
The three Would you like me to create a GitHub follow-up issue for the three items? 🐇 ✅ You are interacting with an AI system. |
3371a7c to
cc73914
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
@coderabbitai The fast-follow is tracked in #713, including the two remaining ConfigMap-handler follow-ups. The span-context item is already resolved on the rebased commit. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
You are interacting with an AI system. |
|
Superseded by #718, which rebases this transport-TLS work onto current main and adds the worker-init installer changes. |
Pull request was closed
2 participants
Customer Summary
Managed NVCA deployments can isolate router transport trust from the cluster-wide certificate path, avoiding admission-time mount collisions while preserving the existing default behavior.
TL;DR
This PR includes Secret-backed workload transport trust and adds
operatorConfig.workload.transportTLS.installedBundleMountPath. When omitted, NVCA continues using/etc/ssl/certs; the managed staging rollout uses/nvcf/transport-tlsfor the router CA.Additional Details
nvcf-trust-merged-certsat the configured path and derivesSTARGATE_TLS_CERT_PATHas<path>/ca-certificates.crt./merged-certs/ca-certificates.crtand general/proxy trust at/etc/ssl/certs.For the Reviewer
The v3.1 source PR is #708. This PR carries the equivalent main-line implementation. The mount-path change is deliberately limited to transport-TLS injection; MiniService update-path behavior is unchanged.
For QA
Passed:
operatorConfig.workload.transportTLS.installedBundleMountPath=/nvcf/transport-tls./etc/ssl/certs, Pylon setSTARGATE_TLS_CERT_PATH=/nvcf/transport-tls/ca-certificates.crt, and the reverse tunnel connected withoutUnknownIssuer.The dev validation temporarily pre-pulled the byocdev image onto both GPU nodes because API-issued workload credentials currently authorize only the staging registry. The temporary DaemonSet and credential were removed. Cold-node validation requires staging publication or an API-issued byocdev pull credential.
Tickets
Summary by CodeRabbit
New Features
Bug Fixes
Tests