[Draft] feat(nvcf-compute-plane): cert-manager integration, nvca support - #483
[Draft] feat(nvcf-compute-plane): cert-manager integration, nvca support#483estroz wants to merge 1 commit into
Conversation
Signed-off-by: Eric Stroczynski <estroczynski@nvidia.com>
📝 WalkthroughWalkthroughThe compute-plane stack adds shared cert-manager PKI, configures Grove, Dynamo, and NVCA webhook dependencies, extends NVCA configuration and RBAC, and implements cert-manager-backed NVCA certificate creation, secret mounting, CA injection, rotation handling, and certificate watching. ChangesCompute-plane webhook PKI
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Helmfile
participant CertManager as cert-manager
participant PKI as compute-plane-webhook-pki
participant NVCA as NVCA operator
participant Webhook as NVCA webhook server
Helmfile->>CertManager: Install cert-manager dependency
Helmfile->>PKI: Install shared PKI chart
PKI->>CertManager: Create CA-backed issuer
Helmfile->>NVCA: Enable cert-manager webhook configuration
NVCA->>CertManager: Create webhook Certificate
CertManager->>NVCA: Populate TLS Secret
NVCA->>Webhook: Mount and watch TLS certificate
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 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="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 🔧 Trivy (0.72.0)Trivy execution failed: 2026-07-27T22:50:24Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: terraformplan-json scan error: fs filter error: fs filter error: walk error range error: stat smartylint.json: no such file or directory: range error: stat smartylint.json: no such file or directory 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: 10
🧹 Nitpick comments (1)
src/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile.go (1)
549-550: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWrap the TLS-secret wait failure with context.
Returning the raw error loses the failed operation in reconciliation logs and status propagation.
Proposed fix
if err := bc.waitForWebhookTLSSecret(ctx, nb); err != nil { - return err + return fmt.Errorf("wait for webhook TLS secret: %w", err) }As per path instructions, check Go error wrapping (
%w); coding guidelines require context-rich errors when useful.🤖 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 549 - 550, Update the error return in the reconciliation flow around bc.waitForWebhookTLSSecret to wrap the failure with descriptive context using Go’s %w error wrapping, while preserving the original error for unwrapping and status propagation.Sources: Coding guidelines, Path instructions
🤖 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 `@deploy/helm/compute-plane-webhook-pki/templates/grove-certificate.yaml`:
- Around line 5-10: The PKI chart must not define or own the Grove namespace.
Remove the Namespace manifest using .Values.groveOperator.namespace from the
chart templates, leaving the Certificate resource as the chart’s
namespace-related output and relying on the Grove release or stack bootstrap to
create it.
In `@deploy/helm/nvca-operator/nvca-operator/templates/role.yaml`:
- Around line 71-73: Update the Certificate permissions rule in the
nvca-operator Role template so it is rendered only when cert-manager is enabled,
matching the conditional structure in the referenced nvca operator Role
template. First align the values hierarchy used by this chart, then wrap the
existing cert-manager.io certificates CRUD rule with that enablement condition
while leaving other permissions unchanged.
In `@deploy/helm/nvca-operator/nvca-operator/values.yaml`:
- Around line 241-252: Align cert-manager configuration across all three sites:
in deploy/helm/nvca-operator/nvca-operator/values.yaml lines 241-252, use the
webhook.certManager hierarchy consumed by the templates; in
deploy/helm/nvca-operator/nvca-operator/templates/self-managed-nvcfbackend-cm.yaml
lines 94-104 and
src/compute-plane-services/nvca/deployments/nvca-operator/templates/self-managed-nvcfbackend-cm.yaml
lines 94-104, read that hierarchy and render the settings under webhookConfig:
so clusterDTO.WebhookConfig deserializes them correctly.
In `@deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl`:
- Line 40: Remove the duplicate top-level releases key in the Helmfile template,
keeping the original releases mapping and its entries unchanged so strict YAML
decoding succeeds.
In `@src/compute-plane-services/nvca/pkg/apis/nvcf/v1/nvcfbackend_types.go`:
- Around line 160-172: Add Godoc comments for WebhookCertManagerConfig and
WebhookConfig, describing their webhook certificate-manager and webhook
settings. Then regenerate the OpenAPI artifacts so WebhookConfig includes the
CertManager field under spec.webhookConfig.certManager in the CRD schema.
In
`@src/compute-plane-services/nvca/pkg/operator/reconcile/webhooks_certmanager_test.go`:
- Around line 33-43: Refactor TestWebhookCertManagerEnabled into a table-driven
test with named cases covering nil CertManager and an enabled CertManager
configuration. Iterate over the cases and invoke webhookCertManagerEnabled for
each expected result, preserving the existing assertions while making additional
states easy to add.
In
`@src/compute-plane-services/nvca/pkg/operator/reconcile/webhooks_certmanager.go`:
- Line 35: Add a Godoc comment immediately before the exported constant
NVCAWebhookCertificateName, beginning with the constant’s exact name and briefly
describing its purpose.
- Around line 109-112: Update the reconciliation flow around the Certificate
update and existing object comparison to call client.Update only when the
desired spec or annotations differ from existing. Preserve the resourceVersion
assignment for actual updates, and return the existing object unchanged when
both are equal.
In `@src/compute-plane-services/nvca/pkg/webhook/cmd.go`:
- Around line 388-412: The runWithReload flow must bypass the Secret-informer
reload wait when Webhook.TLSSecretName is empty, invoking startWebhooks directly
so the certwatcher HTTPS server starts immediately. Preserve the existing
informer/reload behavior for configured TLS Secrets, and add a regression test
covering direct startup without a TLS Secret.
- Around line 414-420: The webhook listener setup should preserve bind context
and keep the TLS serve condition within the line-length limit. In the listener
startup flow, wrap the `net.Listen` error with a descriptive message using `%w`;
inside the goroutine, assign `server.ServeTLS` to a local `err` first, then
separately check that error and ignore only `http.ErrServerClosed`.
---
Nitpick comments:
In
`@src/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile.go`:
- Around line 549-550: Update the error return in the reconciliation flow around
bc.waitForWebhookTLSSecret to wrap the failure with descriptive context using
Go’s %w error wrapping, while preserving the original error for unwrapping and
status propagation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6fc4b3fa-e634-4b82-88fb-fd32be141f8f
⛔ Files ignored due to path filters (1)
src/compute-plane-services/nvca/pkg/apis/nvcf/v1/zz_generated.deepcopy.gois excluded by!**/zz_generated.*
📒 Files selected for processing (27)
deploy/helm/compute-plane-webhook-pki/Chart.yamldeploy/helm/compute-plane-webhook-pki/README.mddeploy/helm/compute-plane-webhook-pki/templates/_helpers.tpldeploy/helm/compute-plane-webhook-pki/templates/cluster-issuer.yamldeploy/helm/compute-plane-webhook-pki/templates/grove-certificate.yamldeploy/helm/compute-plane-webhook-pki/values.yamldeploy/helm/nvca-operator/nvca-operator/templates/role.yamldeploy/helm/nvca-operator/nvca-operator/templates/self-managed-nvcfbackend-cm.yamldeploy/helm/nvca-operator/nvca-operator/values.yamldeploy/stacks/nvcf-compute-plane/Makefile.distdeploy/stacks/nvcf-compute-plane/README.mddeploy/stacks/nvcf-compute-plane/environments/base.yamldeploy/stacks/nvcf-compute-plane/global.yaml.gotmpldeploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpldeploy/stacks/nvcf-compute-plane/helmfile.d/02-nvca.yaml.gotmplsrc/compute-plane-services/nvca/deployments/nvca-operator/templates/role.yamlsrc/compute-plane-services/nvca/deployments/nvca-operator/templates/self-managed-nvcfbackend-cm.yamlsrc/compute-plane-services/nvca/deployments/nvca-operator/values.yamlsrc/compute-plane-services/nvca/pkg/apis/nvcf/v1/nvcfbackend_types.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/clustermgmt/ngcclient.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/clustermgmt/types.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/nvcaagent_reconcile.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/webhooks.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/webhooks_certmanager.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/webhooks_certmanager_test.gosrc/compute-plane-services/nvca/pkg/operator/reconcile/webhooks_certrefresh.gosrc/compute-plane-services/nvca/pkg/webhook/cmd.go
| apiVersion: v1 | ||
| kind: Namespace | ||
| metadata: | ||
| name: {{ .Values.groveOperator.namespace }} | ||
| labels: | ||
| {{- include "compute-plane-webhook-pki.labels" . | nindent 4 }} |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not make the PKI release own the Grove namespace.
Helm deletes manifest resources on uninstall; deleting this Namespace cascades to every Grove resource in it. Ensure the namespace is created and owned by the Grove release or stack bootstrap instead, while this chart only creates the Certificate.
🤖 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 `@deploy/helm/compute-plane-webhook-pki/templates/grove-certificate.yaml`
around lines 5 - 10, The PKI chart must not define or own the Grove namespace.
Remove the Namespace manifest using .Values.groveOperator.namespace from the
chart templates, leaving the Certificate resource as the chart’s
namespace-related output and relying on the Grove release or stack bootstrap to
create it.
| - apiGroups: ["cert-manager.io"] | ||
| resources: ["certificates"] | ||
| verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Gate Certificate permissions on cert-manager enablement.
This chart grants cluster-wide Certificate CRUD even when the feature is disabled. Match the conditional rule used by src/compute-plane-services/nvca/deployments/nvca-operator/templates/role.yaml after aligning the values hierarchy.
🤖 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 `@deploy/helm/nvca-operator/nvca-operator/templates/role.yaml` around lines 71
- 73, Update the Certificate permissions rule in the nvca-operator Role template
so it is rendered only when cert-manager is enabled, matching the conditional
structure in the referenced nvca operator Role template. First align the values
hierarchy used by this chart, then wrap the existing cert-manager.io
certificates CRUD rule with that enablement condition while leaving other
permissions unchanged.
| ## @section Webhook TLS (cert-manager) | ||
| ## @param webhookConfig.imageConfig.pullPolicy Pull policy for the webhook container image | ||
| ## @param webhookConfig.certManager.enabled Use cert-manager for webhook TLS (requires cert-manager in cluster) | ||
| ## @param webhookConfig.certManager.issuerName ClusterIssuer or Issuer name for the webhook Certificate | ||
| ## @param webhookConfig.certManager.issuerKind Issuer kind (ClusterIssuer or Issuer) | ||
| webhookConfig: | ||
| imageConfig: | ||
| pullPolicy: IfNotPresent | ||
| certManager: | ||
| enabled: false | ||
| issuerName: compute-plane-ca-issuer | ||
| issuerKind: ClusterIssuer |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Align the cert-manager configuration path end-to-end.
The chart uses three incompatible paths: external values define webhookConfig, templates read/render webhook, and clusterDTO unmarshals webhookConfig. Consequently, enabling cert-manager does not populate NVCFBackend.Spec.WebhookConfig.CertManager.
deploy/helm/nvca-operator/nvca-operator/values.yaml#L241-L252: use the samewebhook.certManagerhierarchy as the consuming templates, or update every consumer consistently.deploy/helm/nvca-operator/nvca-operator/templates/self-managed-nvcfbackend-cm.yaml#L94-L104: read the selected values hierarchy and renderwebhookConfig:incluster-dto.yaml.src/compute-plane-services/nvca/deployments/nvca-operator/templates/self-managed-nvcfbackend-cm.yaml#L94-L104: renderwebhookConfig:soclusterDTO.WebhookConfigcan deserialize the settings.
📍 Affects 3 files
deploy/helm/nvca-operator/nvca-operator/values.yaml#L241-L252(this comment)deploy/helm/nvca-operator/nvca-operator/templates/self-managed-nvcfbackend-cm.yaml#L94-L104src/compute-plane-services/nvca/deployments/nvca-operator/templates/self-managed-nvcfbackend-cm.yaml#L94-L104
🤖 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 `@deploy/helm/nvca-operator/nvca-operator/values.yaml` around lines 241 - 252,
Align cert-manager configuration across all three sites: in
deploy/helm/nvca-operator/nvca-operator/values.yaml lines 241-252, use the
webhook.certManager hierarchy consumed by the templates; in
deploy/helm/nvca-operator/nvca-operator/templates/self-managed-nvcfbackend-cm.yaml
lines 94-104 and
src/compute-plane-services/nvca/deployments/nvca-operator/templates/self-managed-nvcfbackend-cm.yaml
lines 94-104, read that hierarchy and render the settings under webhookConfig:
so clusterDTO.WebhookConfig deserializes them correctly.
|
|
||
| releases: | ||
|
|
||
| releases: |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Remove the duplicate top-level releases key.
Line 40 repeats the key already declared on line 38. Strict YAML decoding rejects duplicate mapping keys, preventing Helmfile from rendering.
Proposed fix
releases:
-
-releases:
- name: cert-manager📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| releases: | |
| releases: | |
| - name: cert-manager |
🤖 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 `@deploy/stacks/nvcf-compute-plane/helmfile.d/01-dependencies.yaml.gotmpl` at
line 40, Remove the duplicate top-level releases key in the Helmfile template,
keeping the original releases mapping and its entries unchanged so strict YAML
decoding succeeds.
| // +k8s:openapi-gen=true | ||
| type WebhookCertManagerConfig struct { | ||
| Enabled bool `json:"enabled,omitempty"` | ||
| IssuerName string `json:"issuerName,omitempty"` | ||
| IssuerKind string `json:"issuerKind,omitempty"` | ||
| } | ||
|
|
||
| // +k8s:openapi-gen=true | ||
| type WebhookConfig struct { | ||
| ListenPort int32 `json:"listenPort,omitempty"` | ||
| ServicePort int32 `json:"servicePort,omitempty"` | ||
| ImageConfig ImageConfig `json:"imageConfig,omitempty"` | ||
| ListenPort int32 `json:"listenPort,omitempty"` | ||
| ServicePort int32 `json:"servicePort,omitempty"` | ||
| ImageConfig ImageConfig `json:"imageConfig,omitempty"` | ||
| CertManager *WebhookCertManagerConfig `json:"certManager,omitempty"` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## AGENTS files\n'
git ls-files '**/AGENTS.md'
printf '\n## Relevant file outline\n'
ast-grep outline src/compute-plane-services/nvca/pkg/apis/nvcf/v1/nvcfbackend_types.go --view expanded || true
printf '\n## Relevant file excerpt\n'
sed -n '1,260p' src/compute-plane-services/nvca/pkg/apis/nvcf/v1/nvcfbackend_types.go
printf '\n## Search for generated-artifact mentions\n'
rg -n "WebhookCertManagerConfig|WebhookConfig|codegen-update|openapigen-update|openapi-gen" src/compute-plane-services/nvca/pkg/apis -SRepository: NVIDIA/nvcf
Length of output: 31956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "AGENTS:"
fd -a '^AGENTS\.md$' . || true
echo
echo "File lines:"
nl -ba src/compute-plane-services/nvca/pkg/apis/nvcf/v1/nvcfbackend_types.go | sed -n '145,210p'
echo
echo "Search for generated API files referencing the new types:"
rg -n "WebhookCertManagerConfig|WebhookConfig" src/compute-plane-services/nvca -g '!**/vendor/**' -S
echo
echo "Any commit-time generated artifacts adjacent to pkg/apis/nvcf/v1:"
git ls-files src/compute-plane-services/nvca/pkg/apis/nvcf/v1Repository: NVIDIA/nvcf
Length of output: 2243
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '## generated.openapi.go excerpt\n'
sed -n '1060,1125p' src/compute-plane-services/nvca/pkg/apis/nvcf/v1/generated.openapi.go
printf '\n## zz_generated.deepcopy.go excerpt\n'
sed -n '715,760p' src/compute-plane-services/nvca/pkg/apis/nvcf/v1/zz_generated.deepcopy.goRepository: NVIDIA/nvcf
Length of output: 3919
Document the webhook config types and refresh the OpenAPI schema. Add Godoc for WebhookCertManagerConfig and WebhookConfig; regenerate the OpenAPI artifacts so spec.webhookConfig.certManager is included in the CRD schema.
🤖 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/apis/nvcf/v1/nvcfbackend_types.go` around
lines 160 - 172, Add Godoc comments for WebhookCertManagerConfig and
WebhookConfig, describing their webhook certificate-manager and webhook
settings. Then regenerate the OpenAPI artifacts so WebhookConfig includes the
CertManager field under spec.webhookConfig.certManager in the CRD schema.
Source: Coding guidelines
| func TestWebhookCertManagerEnabled(t *testing.T) { | ||
| t.Parallel() | ||
| nb := &nvidiaiov1.NVCFBackend{} | ||
| if webhookCertManagerEnabled(nb) { | ||
| t.Fatal("expected disabled when CertManager nil") | ||
| } | ||
| nb.Spec.WebhookConfig.CertManager = &nvidiaiov1.WebhookCertManagerConfig{Enabled: true} | ||
| if !webhookCertManagerEnabled(nb) { | ||
| t.Fatal("expected enabled") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a table-driven test for the enabled and disabled cases.
This test covers two scenarios but encodes them sequentially. Use named cases so additional CertManager states remain easy to add.
As per coding guidelines, “use table-driven tests for multiple scenarios.”
🤖 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/webhooks_certmanager_test.go`
around lines 33 - 43, Refactor TestWebhookCertManagerEnabled into a table-driven
test with named cases covering nil CertManager and an enabled CertManager
configuration. Iterate over the cases and invoke webhookCertManagerEnabled for
each expected result, preserving the existing assertions while making additional
states easy to add.
Source: Coding guidelines
| ) | ||
|
|
||
| const ( | ||
| NVCAWebhookCertificateName = "nvca-webhook-cert" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add Godoc for the exported constant.
Line 35 exports NVCAWebhookCertificateName without a matching Godoc comment.
Proposed fix
- NVCAWebhookCertificateName = "nvca-webhook-cert"
+ // NVCAWebhookCertificateName is the cert-manager Certificate used by the NVCA webhook.
+ NVCAWebhookCertificateName = "nvca-webhook-cert"As per coding guidelines, “Add Godoc comments to all exported Go symbols.”
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| NVCAWebhookCertificateName = "nvca-webhook-cert" | |
| // NVCAWebhookCertificateName is the cert-manager Certificate used by the NVCA webhook. | |
| NVCAWebhookCertificateName = "nvca-webhook-cert" |
🤖 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/webhooks_certmanager.go`
at line 35, Add a Godoc comment immediately before the exported constant
NVCAWebhookCertificateName, beginning with the constant’s exact name and briefly
describing its purpose.
Source: Coding guidelines
| cert.SetResourceVersion(existing.GetResourceVersion()) | ||
| if _, err := client.Update(ctx, cert, metav1.UpdateOptions{}); err != nil { | ||
| return fmt.Errorf("update webhook Certificate: %w", err) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C4 'setupWebhookCertificate|certificateGVR|\.Update\(ctx, cert' \
src/compute-plane-services/nvca/pkg/operator/reconcileRepository: NVIDIA/nvcf
Length of output: 7824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' src/compute-plane-services/nvca/pkg/operator/reconcile/webhooks_certmanager.go
printf '\n--- TESTS ---\n'
sed -n '1,220p' src/compute-plane-services/nvca/pkg/operator/reconcile/webhooks_certmanager_test.goRepository: NVIDIA/nvcf
Length of output: 8407
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C3 'DeepEqual|reflect\.DeepEqual|equality\.Semantic|managedFields|Compare|annotations' \
src/compute-plane-services/nvca/pkg/operator/reconcileRepository: NVIDIA/nvcf
Length of output: 23653
🏁 Script executed:
#!/bin/bash
set -euo pipefail
nl -ba src/compute-plane-services/nvca/pkg/operator/reconcile/webhooks_certmanager.go | sed -n '70,120p'Repository: NVIDIA/nvcf
Length of output: 189
Skip the update when the Certificate is unchanged. Compare the desired spec and annotations with the existing object before calling Update; otherwise each reconcile does a no-op write and bumps resourceVersion.
🤖 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/webhooks_certmanager.go`
around lines 109 - 112, Update the reconciliation flow around the Certificate
update and existing object comparison to call client.Update only when the
desired spec or annotations differ from existing. Preserve the resourceVersion
assignment for actual updates, and return the existing object unchanged when
both are equal.
Source: Coding guidelines
| if m.cfg.Webhook.TLSSecretName == "" { | ||
| certWatcher, err := certwatcher.New(m.cfg.Webhook.TLSCertFile, m.cfg.Webhook.TLSKeyFile) | ||
| if err != nil { | ||
| return fmt.Errorf("create certificate watcher: %w", err) | ||
| } | ||
| go func() { | ||
| if err := certWatcher.Start(ctx); err != nil { | ||
| log.WithError(err).Error("certificate watcher stopped with error") | ||
| } | ||
| }() | ||
| tlsCfg := &tls.Config{ | ||
| GetCertificate: certWatcher.GetCertificate, | ||
| NextProtos: []string{"h2"}, | ||
| } | ||
| listener, err := tls.Listen("tcp", m.cfg.Webhook.SvcAddress, tlsCfg) | ||
| if err != nil { | ||
| return fmt.Errorf("listen for tls webhooks: %w", err) | ||
| } | ||
|
|
||
| go func() { | ||
| logErr := func(err error) { | ||
| if err != nil && !errors.Is(err, http.ErrServerClosed) { | ||
| go func() { | ||
| log.Infof("Serving HTTPS at: %v", listener.Addr()) | ||
| if err := server.Serve(listener); err != nil && !errors.Is(err, http.ErrServerClosed) { | ||
| log.Error(err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Start directly when no TLS Secret is configured.
This branch is selected when TLSSecretName is empty, but runWithReload still waits for the initial Secret-informer reloadSignal before it invokes startWebhooks. With no named Secret, that signal is never sent, so the certwatcher server never starts. Bypass the informer/reload loop for this mode and add a regression test.
Proposed fix
func (m *webhookManager) runWithReload(parentCtx context.Context) error {
+ if m.cfg.Webhook.TLSSecretName == "" {
+ shutdownCompleted := make(chan struct{})
+ if err := m.startWebhooks(parentCtx, shutdownCompleted); err != nil {
+ return err
+ }
+ <-parentCtx.Done()
+ <-shutdownCompleted
+ return nil
+ }
+
reloadSignal := make(chan struct{})As per coding guidelines, “Go code changes must include/extend tests.”
🧰 Tools
🪛 ast-grep (0.44.1)
[warning] 397-400: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
GetCertificate: certWatcher.GetCertificate,
NextProtos: []string{"h2"},
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(missing-ssl-minversion-go)
🤖 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/webhook/cmd.go` around lines 388 - 412,
The runWithReload flow must bypass the Secret-informer reload wait when
Webhook.TLSSecretName is empty, invoking startWebhooks directly so the
certwatcher HTTPS server starts immediately. Preserve the existing
informer/reload behavior for configured TLS Secrets, and add a regression test
covering direct startup without a TLS Secret.
Source: Coding guidelines
| listener, err := net.Listen("tcp", m.cfg.Webhook.SvcAddress) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if m.cfg.Webhook.TLSCertFile != "" || m.cfg.Webhook.TLSKeyFile != "" { | ||
| go func() { | ||
| log.Infof("Serving HTTPS at: %v", listener.Addr()) | ||
| logErr(server.ServeTLS(listener, m.cfg.Webhook.TLSCertFile, m.cfg.Webhook.TLSKeyFile)) | ||
| } else { | ||
| log.Infof("Serving HTTP at: %v", listener.Addr()) | ||
| logErr(server.Serve(listener)) | ||
| } | ||
| }() | ||
| if err := server.ServeTLS(listener, m.cfg.Webhook.TLSCertFile, m.cfg.Webhook.TLSKeyFile); err != nil && !errors.Is(err, http.ErrServerClosed) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/compute-plane-services/nvca/pkg/webhook/cmd.go'
echo '--- line numbers around 414-420 ---'
sed -n '404,426p' "$file" | cat -n
echo
echo '--- exact line lengths for 414-420 ---'
nl -ba "$file" | sed -n '414,420p' | python3 - <<'PY'
import sys
for line in sys.stdin:
if not line.strip():
continue
num, text = line.split('\t', 1)
text = text.rstrip('\n')
print(f"{num.strip()}: {len(text)} | {text}")
PYRepository: NVIDIA/nvcf
Length of output: 1068
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/compute-plane-services/nvca/pkg/webhook/cmd.go'
echo '--- around the target block with line numbers ---'
nl -ba "$file" | sed -n '406,424p'
echo
echo '--- nearby function signature / context ---'
sed -n '380,430p' "$file"
echo
echo '--- count total lines in file ---'
wc -l "$file"Repository: NVIDIA/nvcf
Length of output: 239
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='src/compute-plane-services/nvca/pkg/webhook/cmd.go'
python3 - <<'PY'
from pathlib import Path
path = Path('src/compute-plane-services/nvca/pkg/webhook/cmd.go')
lines = path.read_text().splitlines()
for i in range(410, 422):
if i <= len(lines):
text = lines[i-1]
print(f"{i}: {len(text)} | {text}")
PYRepository: NVIDIA/nvcf
Length of output: 639
Wrap the bind error and split the ServeTLS check. net.Listen drops the bind context on failure, and the ServeTLS condition exceeds the 120-char limit. Return a %w-wrapped listener error and assign the ServeTLS call to a local err before checking it.
🤖 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/webhook/cmd.go` around lines 414 - 420,
The webhook listener setup should preserve bind context and keep the TLS serve
condition within the line-length limit. In the listener startup flow, wrap the
`net.Listen` error with a descriptive message using `%w`; inside the goroutine,
assign `server.ServeTLS` to a local `err` first, then separately check that
error and ignore only `http.ErrServerClosed`.
Sources: Coding guidelines, Path instructions
1 participant
TL;DR
See #427 for details
Additional Details (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
For the Reviewer
For QA (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
Issues
Closes #427
Checklist
Summary by CodeRabbit
New Features
Documentation
Chores