feat(check): add control-plane validator, stale namespace detection, and registry credential checks - #782
feat(check): add control-plane validator, stale namespace detection, and registry credential checks#782rohithb-hub wants to merge 4 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 (2)
📝 WalkthroughWalkthroughThe self-hosted check command now supports registry credential validation, stale-namespace detection, and role-aware cluster validation. It resolves control-plane and compute-plane targets across Kubernetes modes and passes role-specific configuration to preflight checks and validator Jobs. ChangesSelf-hosted validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI as self-hosted check command
participant Dispatch as runPreflightByRole
participant Preflight as preflight
participant Registry as RegistryCredentialChecker
participant Namespace as StaleNamespaceProber
participant Validator as cluster-validator Job
CLI->>Dispatch: resolve mode and targeted roles
Dispatch->>Preflight: pass role configuration
Preflight->>Registry: check configured registries
Preflight->>Namespace: inspect role namespaces
Preflight->>Validator: run role-specific validation
Validator-->>Preflight: return cluster validation result
Preflight-->>CLI: emit category and check results
Possibly related PRs
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/clis/nvcf-cli/cmd/self_hosted_check.go (1)
145-162: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ModeSinglenow runs the validator twice, but the outer budget still assumes one run.
cpRCandgpuRCboth setClusterValidatorin theModeSinglebranch, and the twoRunPreflightForRolecalls at Line 482 and Line 483 are sequential. EachrunClusterValidatorinvocation owns a 5-minuteclusterValidatorTimeout, so the worst case is 10 minutes plus RBAC bootstrap and log fetch.outerTimeoutis 6 minutes. The compute-plane validator then derivesvctxfrom the remaining ceiling and its wait is truncated, which is the exact failure the comment at Line 155 sets out to prevent.Two related effects in the same path: the second run calls
sweepPriorClusterValidatorJobs, which deletes the control-plane Job, so--no-cleanupcannot preserve it for debugging.Size the budget for the number of validator runs.
🐛 Proposed fix
outerTimeout := 2 * time.Minute - if clusterValidatorWillRun { - outerTimeout = 6 * time.Minute + if clusterValidatorWillRun { + // ModeSingle runs the control-plane and compute-plane validators + // sequentially against the same cluster; budget both. + runs := 1 + if mode == kubectx.ModeSingle && + controlPlaneIsTargeted(mode) && computePlaneIsTargeted(mode) { + runs = 2 + } + outerTimeout = time.Duration(runs) * 6 * time.Minute }🤖 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/clis/nvcf-cli/cmd/self_hosted_check.go` around lines 145 - 162, Update the outerTimeout calculation near clusterValidatorWillRun to account for both sequential validator executions in ModeSingle, using a 10-minute validator budget plus existing headroom while retaining the shorter timeout for a single run. Ensure the resulting context preserves the full wait for both RunPreflightForRole calls and does not alter unrelated cleanup behavior.
🧹 Nitpick comments (7)
src/clis/nvcf-cli/cmd/self_hosted_check.go (1)
287-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
resolveStackValuesFiledepends on the operator's working directory and a fixed environment name.The function walks up from
os.Getwd()fordeploy/stacks/self-managed/environments/local.yaml. Two limits follow:
- An installed CLI run outside the source tree never finds the file, so
global.image.registrynever contributes a registry entry.- The path pins the
localenvironment. An operator running a staging or production environment file gets no registry from this source.Add a flag or Viper key for the values file, and use this walk only as the fallback.
🤖 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/clis/nvcf-cli/cmd/self_hosted_check.go` around lines 287 - 310, Update resolveStackValuesFile to first use a configurable values-file flag or Viper key when provided, allowing any environment path and installed CLI usage; retain the existing working-directory walk for deploy/stacks/self-managed/environments/local.yaml only as the fallback when no override is configured.src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go (2)
494-495: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an assertion that
VALIDATOR_ROLEreaches the container env.Every
buildClusterValidatorJobtest passes""for the newroleargument. The Job env var is the only carrier of the role frompreflight.goto the validator binary, and a dropped or misplacedroleargument would still pass this suite. Add a case that builds withclusterValidatorControlPlaneRoleand assertsenv["VALIDATOR_ROLE"].As per coding guidelines: "Code changes must include tests, or the Pull Request must explain why tests are not applicable".
💚 Proposed test
+func TestBuildClusterValidatorJobShape_RolePropagated(t *testing.T) { + job := buildClusterValidatorJob("test-job", "img:1", "", clusterValidatorControlPlaneRole, false) + env := map[string]string{} + for _, e := range job.Spec.Template.Spec.Containers[0].Env { + env[e.Name] = e.Value + } + assert.Equal(t, clusterValidatorControlPlaneRole, env["VALIDATOR_ROLE"], + "VALIDATOR_ROLE selects the validator check set and must reach the container env") +}Also applies to: 532-544
🤖 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/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go` around lines 494 - 495, Add a test case in TestBuildClusterValidatorJobShape that calls buildClusterValidatorJob with clusterValidatorControlPlaneRole and asserts the generated container environment contains that value under VALIDATOR_ROLE. Keep the existing shape assertions and ensure the test covers role propagation through the Job env.Source: Coding guidelines
345-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
slices.Containsinstead of a local helper.
strSliceContainsreimplementsslices.Containsfrom the standard library.🤖 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/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go` around lines 345 - 352, Remove the local strSliceContains helper and replace its call sites with the standard-library slices.Contains function, adding the required slices import while preserving the existing membership-check behavior.src/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go (1)
42-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer an injected transport over mutating
http.DefaultTransport.Three tests swap the process-wide
http.DefaultTransport. The restore is correct today because no test in this package callst.Parallel. If any test inpackage selfhostedlater becomes parallel, these swaps race with every other HTTP-using test. Consider givingprobeRegistryCredentialan injectable*http.Client(or transport) seam instead.🤖 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/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go` around lines 42 - 46, Update probeRegistryCredential to accept an injected *http.Client or transport, and use that dependency for requests instead of the process-wide http.DefaultTransport. Revise the affected tests to pass srv.Client() (or its transport) directly and remove the DefaultTransport replacement and cleanup.src/clis/nvcf-cli/cmd/self_hosted_check_test.go (1)
347-385: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the matching table for
controlPlaneIsTargeted.
controlPlaneIsTargetedis new and gatescpClusterValidatorinrunPreflightByRole. OnlycomputePlaneIsTargetedhas a table test. The two predicates differ in which flag they read, so a copy-paste error between them would not be caught.As per coding guidelines: "Code changes must include tests, or the Pull Request must explain why tests are not applicable".
🤖 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/clis/nvcf-cli/cmd/self_hosted_check_test.go` around lines 347 - 385, Add a table-driven TestControlPlaneIsTargeted alongside TestComputePlaneIsTargeted, covering control-plane targeting across ModeSingle and ModeSplit, including --pre, --compute-plane, --all, and no relevant flags. Assert each case against controlPlaneIsTargeted and reset the shared checkPre, checkComputePlane, and checkAll state after the test.Source: Coding guidelines
src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go (2)
360-384: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild the ConfigMap YAML from a struct instead of string surgery.
buildControlPlaneValidatorConfiginterpolates registry hostnames into a raw YAML string and then relies onstrings.Replacefinding the literal"enforcement:"token. Two consequences:
- A hostname containing YAML-significant characters produces a malformed document that the validator cannot parse.
- Any future edit to the template that changes or reorders
enforcement:silently breaks the insertion point.
sigs.k8s.io/yamlis already a dependency in this package. Define the config as Go structs and marshal 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/clis/nvcf-cli/internal/selfhosted/clustervalidator.go` around lines 360 - 384, Replace string-based YAML interpolation in buildControlPlaneValidatorConfig with typed config structs and sigs.k8s.io/yaml marshaling, including the baseline endpoints and enforcement settings currently represented by controlPlaneValidatorConfigTemplate. Parse and append valid extra registries as non-critical tcp+tls endpoints, allowing YAML escaping to handle hostnames safely, and remove the strings.Replace insertion logic.
386-408: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReplace the hand-rolled host:port parser with
net.SplitHostPort.The current parser has two defects:
- IPv6 literals break.
[::1]:5000splits at the last colon and returns host[::1]only by accident;::1returns host:and port 1."nvcr.io:"returns host"nvcr.io:"with the trailing colon, which then becomes a malformedhost:value in the ConfigMap.
net.SplitHostPortplusstrconv.Atoicovers both cases and is the idiomatic choice.♻️ Proposed refactor
func parseRegistryHostPort(s string) (host string, port int) { s = strings.TrimSpace(s) if s == "" { return "", 0 } - if idx := strings.LastIndex(s, ":"); idx > 0 { - h := s[:idx] - p := s[idx+1:] - n := 0 - for _, c := range p { - if c < '0' || c > '9' { - return s, 443 - } - n = n*10 + int(c-'0') - } - if n > 0 && n <= 65535 { - return h, n - } - } - return s, 443 + h, p, err := net.SplitHostPort(s) + if err != nil { + return s, 443 + } + n, err := strconv.Atoi(p) + if err != nil || n <= 0 || n > 65535 { + return h, 443 + } + return h, n }🤖 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/clis/nvcf-cli/internal/selfhosted/clustervalidator.go` around lines 386 - 408, Replace the hand-rolled parsing in parseRegistryHostPort with net.SplitHostPort and strconv.Atoi. Support bracketed and unbracketed IPv6 correctly, remove trailing colons from empty-port inputs such as "nvcr.io:", and preserve the existing fallback host/port behavior for missing or invalid ports.
🤖 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/clis/nvcf-cli/cmd/self_hosted_check_test.go`:
- Around line 390-448: Update TestCheck_ComputePlaneFlagRunsChecks and
TestCheck_ControlPlaneFlagRunsChecks to run with --skip-cluster-validation, and
set NVCF_CLI_SELFHOSTED_SKIP_INOTIFY via t.Setenv in each test. Preserve the
existing JSONL parsing and category assertions.
In `@src/clis/nvcf-cli/cmd/self_hosted_check.go`:
- Around line 200-207: Update the registry credential setup block to run
whenever !localOnly, removing the clusterValidatorImage non-empty condition.
Continue obtaining extraRegistries and stackValuesFile, and pass the possibly
empty clusterValidatorImage to selfhosted.EnumerateRegistries so
global.image.registry and configured extras are checked independently of the
validator image.
In `@src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go`:
- Around line 210-225: Confirm the validator’s namespace-wide write requirements
by tracing the operations used by the validator binary, especially namespace,
pod, service, and network-policy checks. If writes only target the probe
namespace, replace the cluster-wide permissions with namespace-scoped
Role/RoleBinding access while retaining required cluster-wide read permissions;
otherwise, add cleanup in the --cleanup flow to delete the validator ClusterRole
and ClusterRoleBinding after the run.
- Around line 145-150: Preserve the error from ensureClusterValidatorConfig in
the control-plane path instead of assigning it to _. Store a non-fatal config
note and append it to cleaned before every ClusterValidatorResult return, or
otherwise expose it through the result transcript, while retaining the wrapped
error context and continuing validation.
In `@src/clis/nvcf-cli/internal/selfhosted/preflight.go`:
- Around line 348-353: Ensure the registry-credentials category is constructed
and executed only once per command invocation, rather than once for each role
passed to RunPreflightForRole. Update buildCategories or the cmd-layer
orchestration around RunPreflightForRole to gate registry handling to a single
role/invocation while preserving all other role-specific categories and result
emission.
- Around line 687-690: Update the stale-namespace message construction around
r.Message to emit remediation hints per stale reason rather than one blanket
kubectl delete command. For “stuck Terminating,” direct operators to remove
namespace finalizers; for “no Helm release,” provide a cautious
inspection/removal hint that does not imply force-deleting the namespace.
Preserve the stale namespace names and counts in the output.
In `@src/clis/nvcf-cli/internal/selfhosted/registry_cred.go`:
- Around line 172-178: The registry endpoint parsing must preserve non-default
ports and correctly handle IPv6 and trailing-colon inputs. In
src/clis/nvcf-cli/internal/selfhosted/registry_cred.go lines 172-178, update the
extras handling around parseRegistryHostPort so RegistryEntry.Registry retains
the parsed port when it is not 443, allowing probeRegistryCredential to use the
correct URL. In src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go lines
386-408, replace the manual parsing in parseRegistryHostPort with
net.SplitHostPort and strconv.Atoi, and add table cases covering [::1]:5000 and
nvcr.io:.
- Around line 95-108: The probeRegistryCredential flow must require configured
credentials for critical registry entries before accepting a successful
exchangeBearerToken result. Check credentialsForRegistry and the entry’s
critical status before returning success, while preserving the existing
rejected-credentials error for configured credentials and the anonymous-token
behavior for non-critical entries.
In `@src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go`:
- Around line 103-111: Update the Secret List call in the stale namespace check
to set ListOptions.Limit to 1, since only existence is required. Add a concise
comment documenting that this check assumes Helm’s default secret storage driver
and may report namespaces using configmap or SQL storage as having no Helm
release.
In `@src/clis/nvcf-cli/internal/selfhosted/validatortag.go`:
- Around line 334-343: Trim whitespace from unquoted parameter values in the
parsing branch of the validator, before assigning or using val. Preserve the
existing comma splitting and empty-params behavior, while ensuring values such
as service after a comma are passed without leading spaces.
- Around line 218-229: Validate the realm URL before applying credentials in the
request flow around credentialsForRegistry: parse the realm and reject it unless
it uses HTTPS and has an acceptable host for the registry authentication
endpoint. Ensure this validation occurs before req.SetBasicAuth, so credentials
are never sent to HTTP or unrelated hosts.
---
Outside diff comments:
In `@src/clis/nvcf-cli/cmd/self_hosted_check.go`:
- Around line 145-162: Update the outerTimeout calculation near
clusterValidatorWillRun to account for both sequential validator executions in
ModeSingle, using a 10-minute validator budget plus existing headroom while
retaining the shorter timeout for a single run. Ensure the resulting context
preserves the full wait for both RunPreflightForRole calls and does not alter
unrelated cleanup behavior.
---
Nitpick comments:
In `@src/clis/nvcf-cli/cmd/self_hosted_check_test.go`:
- Around line 347-385: Add a table-driven TestControlPlaneIsTargeted alongside
TestComputePlaneIsTargeted, covering control-plane targeting across ModeSingle
and ModeSplit, including --pre, --compute-plane, --all, and no relevant flags.
Assert each case against controlPlaneIsTargeted and reset the shared checkPre,
checkComputePlane, and checkAll state after the test.
In `@src/clis/nvcf-cli/cmd/self_hosted_check.go`:
- Around line 287-310: Update resolveStackValuesFile to first use a configurable
values-file flag or Viper key when provided, allowing any environment path and
installed CLI usage; retain the existing working-directory walk for
deploy/stacks/self-managed/environments/local.yaml only as the fallback when no
override is configured.
In `@src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go`:
- Around line 494-495: Add a test case in TestBuildClusterValidatorJobShape that
calls buildClusterValidatorJob with clusterValidatorControlPlaneRole and asserts
the generated container environment contains that value under VALIDATOR_ROLE.
Keep the existing shape assertions and ensure the test covers role propagation
through the Job env.
- Around line 345-352: Remove the local strSliceContains helper and replace its
call sites with the standard-library slices.Contains function, adding the
required slices import while preserving the existing membership-check behavior.
In `@src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go`:
- Around line 360-384: Replace string-based YAML interpolation in
buildControlPlaneValidatorConfig with typed config structs and sigs.k8s.io/yaml
marshaling, including the baseline endpoints and enforcement settings currently
represented by controlPlaneValidatorConfigTemplate. Parse and append valid extra
registries as non-critical tcp+tls endpoints, allowing YAML escaping to handle
hostnames safely, and remove the strings.Replace insertion logic.
- Around line 386-408: Replace the hand-rolled parsing in parseRegistryHostPort
with net.SplitHostPort and strconv.Atoi. Support bracketed and unbracketed IPv6
correctly, remove trailing colons from empty-port inputs such as "nvcr.io:", and
preserve the existing fallback host/port behavior for missing or invalid ports.
In `@src/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go`:
- Around line 42-46: Update probeRegistryCredential to accept an injected
*http.Client or transport, and use that dependency for requests instead of the
process-wide http.DefaultTransport. Revise the affected tests to pass
srv.Client() (or its transport) directly and remove the DefaultTransport
replacement and cleanup.
🪄 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: 5787046b-7875-46b3-b291-3db6a64094de
📒 Files selected for processing (12)
src/clis/nvcf-cli/cmd/self_hosted_check.gosrc/clis/nvcf-cli/cmd/self_hosted_check_test.gosrc/clis/nvcf-cli/internal/selfhosted/clustervalidator.gosrc/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.gosrc/clis/nvcf-cli/internal/selfhosted/preflight.gosrc/clis/nvcf-cli/internal/selfhosted/preflight_test.gosrc/clis/nvcf-cli/internal/selfhosted/registry_cred.gosrc/clis/nvcf-cli/internal/selfhosted/registry_cred_test.gosrc/clis/nvcf-cli/internal/selfhosted/stale_namespace.gosrc/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.gosrc/clis/nvcf-cli/internal/selfhosted/validatortag.gosrc/clis/nvcf-cli/internal/selfhosted/validatortag_test.go
| func TestCheck_ComputePlaneFlagRunsChecks(t *testing.T) { | ||
| t.Cleanup(func() { | ||
| selfHostedJSON = false | ||
| selfHostedOutput = "text" | ||
| checkComputePlane = false | ||
| }) | ||
|
|
||
| var stderr bytes.Buffer | ||
| rootCmd.SetErr(&stderr) | ||
| rootCmd.SetOut(&bytes.Buffer{}) | ||
|
|
||
| rootCmd.SetArgs([]string{"self-hosted", "check", "--compute-plane", "--json"}) | ||
| _ = rootCmd.Execute() | ||
|
|
||
| lines := parseJSONLLines(t, stderr.String()) | ||
| require.NotEmpty(t, lines, "expected at least one JSONL line") | ||
|
|
||
| var categories []string | ||
| for _, l := range lines[1:] { | ||
| if l["event"] == "category_completed" { | ||
| if cat, ok := l["category"].(string); ok { | ||
| categories = append(categories, cat) | ||
| } | ||
| } | ||
| } | ||
| assert.Contains(t, categories, "compute-plane-cluster", | ||
| "--compute-plane must produce compute-plane-cluster events") | ||
| } | ||
|
|
||
| // TestCheck_ControlPlaneFlagRunsChecks verifies that --control-plane alone | ||
| // produces control-plane-cluster category events. | ||
| func TestCheck_ControlPlaneFlagRunsChecks(t *testing.T) { | ||
| t.Cleanup(func() { | ||
| selfHostedJSON = false | ||
| selfHostedOutput = "text" | ||
| checkControlPlane = false | ||
| }) | ||
|
|
||
| var stderr bytes.Buffer | ||
| rootCmd.SetErr(&stderr) | ||
| rootCmd.SetOut(&bytes.Buffer{}) | ||
|
|
||
| rootCmd.SetArgs([]string{"self-hosted", "check", "--control-plane", "--json"}) | ||
| _ = rootCmd.Execute() | ||
|
|
||
| lines := parseJSONLLines(t, stderr.String()) | ||
| require.NotEmpty(t, lines, "expected at least one JSONL line") | ||
|
|
||
| var categories []string | ||
| for _, l := range lines[1:] { | ||
| if l["event"] == "category_completed" { | ||
| if cat, ok := l["category"].(string); ok { | ||
| categories = append(categories, cat) | ||
| } | ||
| } | ||
| } | ||
| assert.Contains(t, categories, "control-plane-cluster", | ||
| "--control-plane must produce control-plane-cluster events") | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
These two tests run the real cluster and registry paths.
TestCheck_ComputePlaneFlagRunsChecks and TestCheck_ControlPlaneFlagRunsChecks execute the command without --local-only and without --skip-cluster-validation. On a machine with a live kubeconfig, the run contacts the API server through the stale-namespace prober, resolves the validator image, and performs outbound HTTPS registry probes. Runtime and outcome then depend on the developer's environment, and CI results differ from local results.
The two skip-note tests below already pass --skip-cluster-validation. Apply the same seam here, and set NVCF_CLI_SELFHOSTED_SKIP_INOTIFY with t.Setenv. Both assertions only need the category event, which the skip path still emits.
🤖 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/clis/nvcf-cli/cmd/self_hosted_check_test.go` around lines 390 - 448,
Update TestCheck_ComputePlaneFlagRunsChecks and
TestCheck_ControlPlaneFlagRunsChecks to run with --skip-cluster-validation, and
set NVCF_CLI_SELFHOSTED_SKIP_INOTIFY via t.Setenv in each test. Preserve the
existing JSONL parsing and category assertions.
| if !localOnly && clusterValidatorImage != "" { | ||
| extraRegistries := viper.GetStringSlice("cluster_validator_registries") | ||
| stackValuesFile := resolveStackValuesFile() | ||
| credEntries = selfhosted.EnumerateRegistries( | ||
| clusterValidatorImage, stackValuesFile, extraRegistries, | ||
| ) | ||
| registryChecker = newRegistryCredentialCheckerForSelfHosted() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The registry credential check is gated on the validator image.
credEntries and registryChecker are populated only when clusterValidatorImage != "". An operator who does not configure cluster_validator_image gets no credential check at all, even though global.image.registry from the stack values file and the --cluster-validator-registries extras are still the registries the install pulls from. The two features are otherwise independent.
Gate the credential check on !localOnly alone, and let EnumerateRegistries handle an empty image ref.
🤖 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/clis/nvcf-cli/cmd/self_hosted_check.go` around lines 200 - 207, Update
the registry credential setup block to run whenever !localOnly, removing the
clusterValidatorImage non-empty condition. Continue obtaining extraRegistries
and stackValuesFile, and pass the possibly empty clusterValidatorImage to
selfhosted.EnumerateRegistries so global.image.registry and configured extras
are checked independently of the validator image.
| if role == clusterValidatorControlPlaneRole { | ||
| if err := ensureClusterValidatorConfig(vctx, client, registries); err != nil { | ||
| // Non-fatal: reachability checks will be skipped, not the whole run. | ||
| _ = err | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not discard the ConfigMap error.
_ = err drops the only signal that the reachability configuration was not applied. The run continues and the validator silently skips every configurable check, so the operator sees a green validator row that proves less than it appears to. ensureClusterValidatorConfig already wraps its errors with %w; surface that context instead of dropping it.
Attach the message to the result transcript, or return it in ClusterValidatorResult as a non-fatal note.
♻️ Proposed change
if role == clusterValidatorControlPlaneRole {
if err := ensureClusterValidatorConfig(vctx, client, registries); err != nil {
- // Non-fatal: reachability checks will be skipped, not the whole run.
- _ = err
+ // Non-fatal: reachability checks are skipped, not the whole run.
+ // Keep the reason so the transcript explains the missing rows.
+ configNote = fmt.Sprintf("validator config not applied: %v", err)
}
}configNote then needs to be appended to cleaned before each ClusterValidatorResult return.
As per path instructions: "Check Go error wrapping (%w), structured logging with required context fields".
🤖 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/clis/nvcf-cli/internal/selfhosted/clustervalidator.go` around lines 145 -
150, Preserve the error from ensureClusterValidatorConfig in the control-plane
path instead of assigning it to _. Store a non-fatal config note and append it
to cleaned before every ClusterValidatorResult return, or otherwise expose it
through the result transcript, while retaining the wrapped error context and
continuing validation.
Source: Path instructions
| // Read-only: cluster inventory and configuration. | ||
| {APIGroups: []string{""}, Resources: []string{"nodes", "configmaps"}, Verbs: []string{"get", "list", "watch"}}, | ||
| // Read + write: enforcement checks create and delete probe namespaces | ||
| // and pods; the active-LB check creates and deletes a probe service. | ||
| {APIGroups: []string{""}, Resources: []string{"namespaces", "pods", "services"}, Verbs: []string{"get", "list", "watch", "create", "delete"}}, | ||
| // Pod log subresource: read probe output without exec. | ||
| {APIGroups: []string{""}, Resources: []string{"pods/log"}, Verbs: []string{"get"}}, | ||
| {APIGroups: []string{"storage.k8s.io"}, Resources: []string{"csidrivers", "storageclasses"}, Verbs: []string{"get", "list"}}, | ||
| {APIGroups: []string{"networking.k8s.io"}, Resources: []string{"networkpolicies"}, Verbs: []string{"get", "list"}}, | ||
| // NetworkPolicies: read for CNI detection; write for enforcement | ||
| // check which creates/updates/deletes policies in the temp namespace. | ||
| {APIGroups: []string{"networking.k8s.io"}, Resources: []string{"networkpolicies"}, Verbs: []string{"get", "list", "create", "update", "delete"}}, | ||
| {APIGroups: []string{"admissionregistration.k8s.io"}, Resources: []string{"mutatingwebhookconfigurations", "validatingwebhookconfigurations"}, Verbs: []string{"get", "list"}}, | ||
| {APIGroups: []string{"apps"}, Resources: []string{"deployments", "daemonsets", "statefulsets"}, Verbs: []string{"get", "list"}}, | ||
| // Gateway API: control-plane gateway and route health checks. | ||
| {APIGroups: []string{"gateway.networking.k8s.io"}, Resources: []string{"gatewayclasses", "gateways", "httproutes", "grpcroutes"}, Verbs: []string{"get", "list"}}, | ||
| {NonResourceURLs: []string{"/readyz", "/version", "/healthz"}, Verbs: []string{"get"}}, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Confirm that cluster-wide write verbs are required.
The ClusterRole grants create and delete on namespaces, pods, and services across every namespace, bound cluster-wide through the ClusterRoleBinding. A ServiceAccount that can create pods in any namespace can mount any ServiceAccount token in that namespace, which is a standard privilege-escalation path. The validator is short-lived, but the ClusterRole and binding persist after the Job because only Jobs are swept.
Confirm the validator binary needs namespace-wide write access rather than write access inside its own probe namespace. If it does, consider deleting the ClusterRole and binding at the end of a run that used --cleanup.
#!/bin/bash
# Locate the validator's own RBAC expectations and any existing cleanup of cluster-scoped RBAC.
rg -nP -C3 'clusterValidatorName|ClusterRoleBindings\(\)|ClusterRoles\(\)' --type=go
rg -nP -C3 'namespaces|networkpolicies' --type=yaml -g '**/cluster-validator/**'🤖 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/clis/nvcf-cli/internal/selfhosted/clustervalidator.go` around lines 210 -
225, Confirm the validator’s namespace-wide write requirements by tracing the
operations used by the validator binary, especially namespace, pod, service, and
network-policy checks. If writes only target the probe namespace, replace the
cluster-wide permissions with namespace-scoped Role/RoleBinding access while
retaining required cluster-wide read permissions; otherwise, add cleanup in the
--cleanup flow to delete the validator ClusterRole and ClusterRoleBinding after
the run.
| // Registry credential check runs from the operator's machine — no cluster | ||
| // contact needed. Placed after local-host-tools but before cluster probes | ||
| // so credential failures surface early. | ||
| if len(cfg.Registries) > 0 && cfg.RegistryChecker != nil { | ||
| out = append(out, buildRegistryCredentialCategory(cfg)) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
The registry category is built per role, so every probe runs twice.
buildCategories adds registry-credentials before the role gate. The cmd layer calls RunPreflightForRole once per role: sequentially in ModeSingle and concurrently in ModeSplit. Each registry then receives two live HTTPS round trips, and the event stream carries two registry-cred-<host> results with identical IDs. JSON consumers that key results by ID see a collision.
Run the registry category once per invocation, for example by building it in the cmd layer or by gating it on a single role.
#!/bin/bash
# Confirm how many times RunPreflightForRole is invoked per command run.
rg -nP -C4 '\bRunPreflightForRole\s*\(' --type=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/clis/nvcf-cli/internal/selfhosted/preflight.go` around lines 348 - 353,
Ensure the registry-credentials category is constructed and executed only once
per command invocation, rather than once for each role passed to
RunPreflightForRole. Update buildCategories or the cmd-layer orchestration
around RunPreflightForRole to gate registry handling to a single role/invocation
while preserving all other role-specific categories and result emission.
| _, err = exchangeBearerToken(pctx, client, registry, repoHint, wwwAuth) | ||
| if err != nil { | ||
| // Use credentialsForRegistry (not ngcCredentials) so NGC_API_KEY does | ||
| // not masquerade as credentials for quay.io, GHCR, or Harbor — those | ||
| // registries reject NGC tokens, which would wrongly produce "credentials | ||
| // rejected" when the real diagnosis is "no credentials configured." | ||
| _, _, hasCreds := credentialsForRegistry(registry) | ||
| if !hasCreds { | ||
| return fmt.Errorf("no credentials configured for %s "+ | ||
| "(add to ~/.docker/config.json or set NGC_API_KEY for NGC registries)", registry) | ||
| } | ||
| return fmt.Errorf("credentials rejected by %s: %w", registry, err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The probe can report valid credentials when no credentials exist.
exchangeBearerToken sends the token request without Authorization when credentialsForRegistry finds nothing. GHCR and Docker Hub issue anonymous tokens in that case, so probeRegistryCredential returns nil and the check renders "credentials valid". The subsequent image pull still fails. For entries marked Critical, this converts a hard error into a silent pass.
Require credentials for critical entries before treating a successful token exchange as a pass.
🤖 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/clis/nvcf-cli/internal/selfhosted/registry_cred.go` around lines 95 -
108, The probeRegistryCredential flow must require configured credentials for
critical registry entries before accepting a successful exchangeBearerToken
result. Check credentialsForRegistry and the entry’s critical status before
returning success, while preserving the existing rejected-credentials error for
configured credentials and the anonymous-token behavior for non-critical
entries.
| // Source 4: operator-supplied extras (--cluster-validator-registries). | ||
| for _, e := range extras { | ||
| host, _ := parseRegistryHostPort(e) | ||
| if host != "" { | ||
| add(host, false) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Registry endpoint parsing loses the port and mishandles IPv6. Both sites depend on the parseRegistryHostPort contract: the helper returns host and port separately, the credential path drops the port, and the helper itself cannot split IPv6 literals or a trailing-colon input. A registry such as harbor.company.internal:5000 is credential-probed on port 443 and reported as failing.
src/clis/nvcf-cli/internal/selfhosted/registry_cred.go#L172-L178: keep the port inRegistryEntry.Registrywhen it is not 443, soprobeRegistryCredentialbuilds the correcthttps://host:port/v2/URL.src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go#L386-L408: replace the manual scan withnet.SplitHostPortplusstrconv.Atoi, and add table cases for[::1]:5000and"nvcr.io:".
📍 Affects 2 files
src/clis/nvcf-cli/internal/selfhosted/registry_cred.go#L172-L178(this comment)src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go#L386-L408
🤖 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/clis/nvcf-cli/internal/selfhosted/registry_cred.go` around lines 172 -
178, The registry endpoint parsing must preserve non-default ports and correctly
handle IPv6 and trailing-colon inputs. In
src/clis/nvcf-cli/internal/selfhosted/registry_cred.go lines 172-178, update the
extras handling around parseRegistryHostPort so RegistryEntry.Registry retains
the parsed port when it is not 443, allowing probeRegistryCredential to use the
correct URL. In src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go lines
386-408, replace the manual parsing in parseRegistryHostPort with
net.SplitHostPort and strconv.Atoi, and add table cases covering [::1]:5000 and
nvcr.io:.
| secrets, err := client.CoreV1().Secrets(name).List(ctx, metav1.ListOptions{ | ||
| LabelSelector: "owner=helm", | ||
| }) | ||
| if err != nil { | ||
| return stale, fmt.Errorf("list Helm secrets in %s: %w", name, err) | ||
| } | ||
| if len(secrets.Items) == 0 { | ||
| stale = append(stale, StaleNamespace{Name: name, Reason: "no Helm release"}) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Bound the Secret List and document the storage-driver assumption.
The code only needs to know whether at least one owner=helm Secret exists. The current List downloads every Helm release Secret in the namespace, including the gzipped release payload in .data, which is frequently hundreds of kilobytes per revision. Set Limit: 1.
The check also assumes the default Helm secret storage driver. With HELM_DRIVER=configmap or sql, no owner=helm Secret exists and a healthy namespace is reported as "no Helm release" at error severity.
♻️ Proposed change
secrets, err := client.CoreV1().Secrets(name).List(ctx, metav1.ListOptions{
LabelSelector: "owner=helm",
+ // Existence is all that matters; avoid pulling full release payloads.
+ Limit: 1,
})🤖 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/clis/nvcf-cli/internal/selfhosted/stale_namespace.go` around lines 103 -
111, Update the Secret List call in the stale namespace check to set
ListOptions.Limit to 1, since only existence is required. Add a concise comment
documenting that this check assumes Helm’s default secret storage driver and may
report namespaces using configmap or SQL storage as having no Helm release.
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| // Add credentials when present. Docker config covers any registry; | ||
| // NGC API key is only applicable to NGC-hosted registries. | ||
| // Critically: do NOT apply NGC_API_KEY to non-NGC registries — quay.io, | ||
| // GHCR, and Harbor will reject it, producing a misleading "credentials | ||
| // rejected" error when the real situation is "no credentials configured." | ||
| if user, pass, ok := credentialsForRegistry(registry); ok { | ||
| req.SetBasicAuth(user, pass) | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate the realm URL before you attach credentials.
realm comes from the registry's Www-Authenticate response header. The code sends Basic credentials from ~/.docker/config.json or NGC_API_KEY to that URL without checking the scheme or host. A registry that returns realm="http://..." receives the credentials in cleartext. A registry that returns a realm on an unrelated host receives credentials intended for a different service.
Reject non-HTTPS realms before the credential is applied.
🔒 Proposed fix
u, err := url.Parse(realm)
if err != nil {
return exchangeNGCBearerToken(ctx, client, registry, repo)
}
+ // Never send credentials over cleartext or to a non-absolute realm.
+ if u.Scheme != "https" || u.Host == "" {
+ return "", fmt.Errorf("refusing token exchange at insecure realm %q for %s", realm, registry)
+ }🤖 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/clis/nvcf-cli/internal/selfhosted/validatortag.go` around lines 218 -
229, Validate the realm URL before applying credentials in the request flow
around credentialsForRegistry: parse the realm and reject it unless it uses
HTTPS and has an acceptable host for the registry authentication endpoint.
Ensure this validation occurs before req.SetBasicAuth, so credentials are never
sent to HTTP or unrelated hosts.
| } else { | ||
| comma := strings.IndexByte(params, ',') | ||
| if comma < 0 { | ||
| val = params | ||
| params = "" | ||
| } else { | ||
| val = params[:comma] | ||
| params = params[comma+1:] | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Trim unquoted challenge values.
Registries may send the challenge without quotes, for example Bearer realm=https://auth.example.com/token, service=example. The unquoted branch keeps the leading space after the comma, so service becomes " example" and the token request carries a wrong parameter. The quoted branch already trims.
🐛 Proposed fix
} else {
comma := strings.IndexByte(params, ',')
if comma < 0 {
- val = params
+ val = strings.TrimSpace(params)
params = ""
} else {
- val = params[:comma]
+ val = strings.TrimSpace(params[:comma])
params = params[comma+1:]
}
}📝 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.
| } else { | |
| comma := strings.IndexByte(params, ',') | |
| if comma < 0 { | |
| val = params | |
| params = "" | |
| } else { | |
| val = params[:comma] | |
| params = params[comma+1:] | |
| } | |
| } | |
| } else { | |
| comma := strings.IndexByte(params, ',') | |
| if comma < 0 { | |
| val = strings.TrimSpace(params) | |
| params = "" | |
| } else { | |
| val = strings.TrimSpace(params[:comma]) | |
| params = params[comma+1:] | |
| } | |
| } |
🤖 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/clis/nvcf-cli/internal/selfhosted/validatortag.go` around lines 334 -
343, Trim whitespace from unquoted parameter values in the parsing branch of the
validator, before assigning or using val. Preserve the existing comma splitting
and empty-params behavior, while ensuring values such as service after a comma
are passed without leading spaces.
1 participant
TL;DR
Adds three new capabilities to nvcf self-hosted check: a control-plane cluster validator (wired to the companion nvca PR), stale namespace detection before install, and pre-install registry credential validation using the generic OCI Bearer token flow.
Additional Details
This PR contains three logical changes, each in a separate commit.
Commit 1 -- re-gate cluster-validator and widen RBAC (Req 1, Req 3)
The validator was only triggered by --pre or --all, and --compute-plane was a
complete no-op. Two predicate functions now gate the validator:
--pre in ModeSplit no longer triggers the validator because --pre does not
constitute explicit compute-plane targeting when two separate clusters are named.
mode is hoisted to runSelfHostedCheck so image resolution, timeout sizing,
and skip-note messaging all share the same answer.
The preflight-validator ClusterRole is widened to support the new checks:
The ClusterRole uses update-or-create so existing installs pick up the new
rules on the next check run without manual intervention.
Commit 2 -- stale namespace detection and control-plane validator wiring (Req 6, Req 2+4)
probeStaleNamespaces checks each known NVCF stack namespace and reports two
failure conditions:
Severity is error so anyFailed trips the non-zero exit code. The check never
auto-cleans. The error message names every stale namespace and provides a
kubectl delete command operators can copy and run.
The control-plane validator is wired into controlPlaneCheckCategory. Before
submitting the Job, nvcf-cli creates a ConfigMap cluster-validator-network-checks
in the default namespace with nvcr.io reachability and enforcement config.
VALIDATOR_ROLE=control-plane is set in the Job env. The --cluster-validator-registries
flag (or cluster_validator_registries config key) appends operator-supplied
registry endpoints to the ConfigMap alongside nvcr.io.
Commit 3 -- registry credential validation (Req 5)
exchangeBearerToken now implements the OCI Distribution Spec Bearer token flow:
parses realm, service, and scope from the WWW-Authenticate header returned in
the registry 401 response, then fetches a token from that realm. The old NGC
/proxy_auth path is kept as a named fallback for when the header is absent or
NGC-specific behavior is needed.
credentialsForRegistry separates NGC_API_KEY from generic docker config lookup.
NGC_API_KEY applies only to NGC registries (nvcr.io, nvidia.com domains).
Sending it to quay.io or GHCR would cause confusing "credentials rejected"
errors when the real situation is "no credentials configured."
EnumerateRegistries builds the check list from:
The RepoHint carries the actual repo path from the image ref so the token
exchange uses the correct org scope. NGC checks org-level access, so using a
synthetic repo name like probe/credential-check returns 403 even with valid
credentials.
For the Reviewer
cmd/self_hosted_check.go: computePlaneIsTargeted, controlPlaneIsTargeted, mode hoisting, anyValidatorIsTargeted, resolveStackValuesFile, registry credential wiringinternal/selfhosted/clustervalidator.go: RBAC rule set, VALIDATOR_ROLE in Job env, ConfigMap creation, ClusterValidatorParams.Role and .Registriesinternal/selfhosted/preflight.go: staleNamespaceCheck, buildRegistryCredentialCategory, clusterValidatorCheck role+registries params, PreflightConfig new fieldsinternal/selfhosted/stale_namespace.go: new file with prober type, check logic, and namespace listsinternal/selfhosted/registry_cred.go: new file with probeRegistryCredential, EnumerateRegistries, readGlobalImageRegistryinternal/selfhosted/validatortag.go: parseWWWAuthenticate, exchangeBearerToken (generic OCI), credentialsForRegistryFor QA
Tested on k3d-ncp-local (NVCF stack deployed) and kind-kind (bare cluster).
nvcr.io credential check:
quay.io and ghcr.io:
ECR hostname: "ECR registry detected, use aws ecr get-login-password" at warning severity
Stale namespace detection:
Control-plane validator:
Issues
NO-REF
Checklist
Summary by CodeRabbit
New Features
Bug Fixes