feat(check): add control-plane validator, stale namespace detection, and registry credential checks - #782
Open
rohithb-hub wants to merge 6 commits into
Open
feat(check): add control-plane validator, stale namespace detection, and registry credential checks#782rohithb-hub wants to merge 6 commits into
rohithb-hub wants to merge 6 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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe self-hosted check command adds 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
🚥 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
Comment on lines
+210
to
225
| // 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.
4 tasks
…decouple registry checks from validator image
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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