From 7d424694393fb4b2612df9d0048bed3ea0f65b59 Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 12 Aug 2026 02:14:12 +0530 Subject: [PATCH 01/13] feat(check): re-gate cluster-validator per compute-plane targeting and widen RBAC --- src/clis/nvcf-cli/cmd/self_hosted_check.go | 154 ++++++++++++--- .../nvcf-cli/cmd/self_hosted_check_test.go | 160 ++++++++++++++++ .../internal/selfhosted/clustervalidator.go | 175 +++++++++++++++--- .../selfhosted/clustervalidator_test.go | 153 +++++++++++++-- 4 files changed, 584 insertions(+), 58 deletions(-) diff --git a/src/clis/nvcf-cli/cmd/self_hosted_check.go b/src/clis/nvcf-cli/cmd/self_hosted_check.go index dafb00fbf..2498e8340 100644 --- a/src/clis/nvcf-cli/cmd/self_hosted_check.go +++ b/src/clis/nvcf-cli/cmd/self_hosted_check.go @@ -22,6 +22,7 @@ import ( "fmt" "io" "os" + "path/filepath" "time" "github.com/spf13/cobra" @@ -45,6 +46,7 @@ var ( checkClusterValidatorImage string checkClusterValidatorPullSecret string checkClusterValidatorNoCleanup bool + checkClusterValidatorRegistries []string checkShowLogs bool ) @@ -58,9 +60,19 @@ var newClusterValidatorForSelfHosted = func() selfhosted.ClusterValidator { return selfhosted.NewClusterValidator() } +// Test seam. +var newStaleNamespaceProberForSelfHosted = func() selfhosted.StaleNamespaceProber { + return selfhosted.NewStaleNamespaceProber() +} + // Test seam. Tests stub this to skip the registry network call. var resolveLatestValidatorTagForSelfHosted = selfhosted.ResolveLatestValidatorTag +// Test seam. +var newRegistryCredentialCheckerForSelfHosted = func() selfhosted.RegistryCredentialChecker { + return selfhosted.NewRegistryCredentialChecker() +} + var checkWriterIsTTY = isWriterTTY var selfHostedCheckCmd = &cobra.Command{ @@ -100,6 +112,13 @@ func init() { selfHostedCheckCmd.Flags().BoolVar(&checkClusterValidatorNoCleanup, "no-cleanup", false, "Disable the validator Job's TTL so the Job persists for debugging. "+ "The next run still deletes prior Jobs via the singleton sweep.") + selfHostedCheckCmd.Flags().StringSliceVar(&checkClusterValidatorRegistries, "cluster-validator-registries", nil, + "Additional container registries to probe for reachability in the control-plane validator. "+ + "Format: host:port (e.g. harbor.company.internal:443,ghcr.io:443). "+ + "nvcr.io is always included. Env: NVCF_CLI_CLUSTER_VALIDATOR_REGISTRIES. "+ + "Can also be set in nvcf-cli config as cluster_validator_registries (list).") + _ = viper.BindPFlag("cluster_validator_registries", + selfHostedCheckCmd.Flags().Lookup("cluster-validator-registries")) selfHostedCheckCmd.Flags().BoolVar(&checkShowLogs, "show-logs", false, "Print the cleaned cluster-validator transcript to stderr after the check events. "+ "Useful when piping --json output to a script that also wants the transcript.") @@ -113,17 +132,25 @@ func runSelfHostedCheck(c *cobra.Command, _ []string) error { localOnly := checkLocalOnly || os.Getenv("NVCF_CLI_SELFHOSTED_LOCAL_ONLY") != "" skipClusterValidation := checkSkipClusterValidation || os.Getenv("NVCF_CLI_SELFHOSTED_SKIP_CLUSTER_VALIDATION") != "" + // Mode is needed before image resolution so computePlaneIsTargeted can + // gate the registry round trip. ValidateFlags in PersistentPreRunE + // guarantees mode is ModeSingle or ModeSplit here. + mode := kubectx.SelectMode(selfHostedControlPlaneContext, selfHostedComputePlaneContext) + // Resolve the validator image up-front so we can right-size the // outer timeout (only when the validator actually runs) and emit a // one-shot stderr note up-front explaining why no validator row // appears in the output. Empty == not configured anywhere. + // One image covers both roles (VALIDATOR_ROLE selects the check set). + anyValidatorIsTargeted := !localOnly && !skipClusterValidation && + (computePlaneIsTargeted(mode) || controlPlaneIsTargeted(mode)) clusterValidatorImage := "" - if !localOnly && !skipClusterValidation && (checkPre || checkAll) { + if anyValidatorIsTargeted { if img, ok := resolveClusterValidatorImage(c.Context()); ok { clusterValidatorImage = img } } - clusterValidatorWillRun := !localOnly && !skipClusterValidation && (checkPre || checkAll) && clusterValidatorImage != "" + clusterValidatorWillRun := anyValidatorIsTargeted && clusterValidatorImage != "" // The cluster-validator Job's internal budget is 5m // (selfhosted.clusterValidatorTimeout). The outer ctx must be at least @@ -154,7 +181,7 @@ func runSelfHostedCheck(c *cobra.Command, _ []string) error { // validator row" with "validator silently dropped". Print at most one // reason; --skip-cluster-validation takes precedence over missing // config since it's the explicit operator choice. - if !localOnly && (checkPre || checkAll) { + if !localOnly && (computePlaneIsTargeted(mode) || controlPlaneIsTargeted(mode)) { switch { case skipClusterValidation: fmt.Fprintln(c.ErrOrStderr(), "note: cluster-validator skipped (--skip-cluster-validation)") @@ -163,9 +190,27 @@ func runSelfHostedCheck(c *cobra.Command, _ []string) error { } } + // Enumerate registries for the local credential check. Skipped when + // local-only (no network) or when no validator image is configured. + // Uses the same extras list as the in-cluster ConfigMap reachability check. + var ( + credEntries []selfhosted.RegistryEntry + registryChecker selfhosted.RegistryCredentialChecker + ) + if !localOnly && clusterValidatorImage != "" { + extraRegistries := viper.GetStringSlice("cluster_validator_registries") + stackValuesFile := resolveStackValuesFile() + credEntries = selfhosted.EnumerateRegistries( + clusterValidatorImage, stackValuesFile, extraRegistries, + ) + registryChecker = newRegistryCredentialCheckerForSelfHosted() + } + cfg := selfhosted.PreflightConfig{ - LocalOnly: localOnly, - Tools: selfHostedPreflightTools(), + LocalOnly: localOnly, + Tools: selfHostedPreflightTools(), + Registries: credEntries, + RegistryChecker: registryChecker, } sink, err := selectCheckRenderer(c.ErrOrStderr(), selfHostedWait != "") @@ -181,8 +226,8 @@ func runSelfHostedCheck(c *cobra.Command, _ []string) error { runOnce := func() []selfhosted.CheckResult { var results []selfhosted.CheckResult - if checkPre || checkAll { - results = append(results, runPreflightByRole(ctx, cfg, sink, clusterValidatorImage)...) + if checkPre || checkAll || checkControlPlane || checkComputePlane { + results = append(results, runPreflightByRole(ctx, cfg, sink, mode, clusterValidatorImage)...) } // Inject force-fail seam for tests. if os.Getenv("NVCF_CLI_SELFHOSTED_FORCE_FAIL") != "" { @@ -194,7 +239,6 @@ func runSelfHostedCheck(c *cobra.Command, _ []string) error { Message: "forced failure (test seam)", }}, results...) } - // control-plane / compute-plane wired in M3/M4 — placeholder no-op for M2. return results } @@ -240,6 +284,44 @@ func runSelfHostedCheck(c *cobra.Command, _ []string) error { } } +// resolveStackValuesFile walks up from the current working directory to find +// the active environment values YAML. Returns "" when not found so callers +// skip the optional lookup gracefully. +func resolveStackValuesFile() string { + cwd, err := os.Getwd() + if err != nil { + return "" + } + // Walk up to 6 directory levels to find the stack environments directory. + dir := cwd + for i := 0; i < 6; i++ { + candidate := filepath.Join(dir, + "deploy", "stacks", "self-managed", "environments", "local.yaml") + if _, err := os.Stat(candidate); err == nil { + return candidate + } + parent := filepath.Dir(dir) + if parent == dir { + break + } + dir = parent + } + return "" +} + +// computePlaneIsTargeted reports whether the compute-plane validator should run: +// --compute-plane, --all, or --pre in ModeSingle. --pre in ModeSplit does not +// target it because separate clusters have no implicit compute-plane role. +func computePlaneIsTargeted(mode kubectx.Mode) bool { + return checkComputePlane || checkAll || (checkPre && mode == kubectx.ModeSingle) +} + +// controlPlaneIsTargeted mirrors computePlaneIsTargeted but for the control +// plane. Runs when: --control-plane, --all, or --pre in ModeSingle. +func controlPlaneIsTargeted(mode kubectx.Mode) bool { + return checkControlPlane || checkAll || (checkPre && mode == kubectx.ModeSingle) +} + // maybeShowClusterValidatorLogs prints the cleaned cluster-validator transcript // to the given writer when --show-logs is set, framed by markers so operators // can find it in mixed CLI output. Silent no-op when: @@ -296,10 +378,10 @@ func selectCheckRenderer(w io.Writer, wait bool) (progress.EventSink, error) { // - ModeSingle (no context flags) → RoleControlPlane + RoleComputePlane sequentially // - ModeSplit (both context flags set) → RoleControlPlane + RoleComputePlane in parallel // -// clusterValidatorImage is the already-resolved validator image (empty when -// not configured). Resolution happens in the caller so the outer-timeout -// and stderr-note logic can see the same answer this function does. -func runPreflightByRole(ctx context.Context, cfg selfhosted.PreflightConfig, sink progress.EventSink, clusterValidatorImage string) []selfhosted.CheckResult { +// mode is the already-resolved kubectx.Mode (hoisted to the caller so image +// resolution and timeout sizing share the same answer). clusterValidatorImage +// is the already-resolved validator image (empty when not configured). +func runPreflightByRole(ctx context.Context, cfg selfhosted.PreflightConfig, sink progress.EventSink, mode kubectx.Mode, clusterValidatorImage string) []selfhosted.CheckResult { // LocalOnly: skip all cluster probes. if cfg.LocalOnly { return selfhosted.RunPreflightForRole(ctx, cfg, selfhosted.RoleLocalOnly, selfhosted.RoleConfig{}, sink) @@ -309,7 +391,6 @@ func runPreflightByRole(ctx context.Context, cfg selfhosted.PreflightConfig, sin if checkAll || checkComputePlane || !checkPre { icmsURL = resolveICMSURL(selfHostedICMSURL) } - mode := kubectx.SelectMode(selfHostedControlPlaneContext, selfHostedComputePlaneContext) skipInotify := checkSkipInotifyCheck || os.Getenv("NVCF_CLI_SELFHOSTED_SKIP_INOTIFY") != "" var inotifyProber selfhosted.NodeInotifyProber @@ -328,6 +409,19 @@ func runPreflightByRole(ctx context.Context, cfg selfhosted.PreflightConfig, sin clusterValidator = newClusterValidatorForSelfHosted() } + staleNSProber := newStaleNamespaceProberForSelfHosted() + + // Additional registries to probe in the control-plane validator ConfigMap. + // Priority: flag > env > config file. + registries := viper.GetStringSlice("cluster_validator_registries") + + // The cluster-validator image is the same for both roles; VALIDATOR_ROLE + // in the Job env selects which check set runs inside the binary. + var cpClusterValidator selfhosted.ClusterValidator + if controlPlaneIsTargeted(mode) && clusterValidator != nil { + cpClusterValidator = newClusterValidatorForSelfHosted() + } + switch mode { case kubectx.ModeSplit: // Run both roles in parallel; each gets its own kubeconfig context. @@ -337,7 +431,15 @@ func runPreflightByRole(ctx context.Context, cfg selfhosted.PreflightConfig, sin ) eg, egCtx := errgroup.WithContext(ctx) eg.Go(func() error { - rc := selfhosted.RoleConfig{KubeContext: selfHostedControlPlaneContext} + rc := selfhosted.RoleConfig{ + KubeContext: selfHostedControlPlaneContext, + ClusterValidator: cpClusterValidator, + ClusterValidatorImage: clusterValidatorImage, + ClusterValidatorPullSecret: checkClusterValidatorPullSecret, + ClusterValidatorNoCleanup: checkClusterValidatorNoCleanup, + ClusterValidatorRegistries: registries, + StaleNamespaceProber: staleNSProber, + } cpResults = selfhosted.RunPreflightForRole(egCtx, cfg, selfhosted.RoleControlPlane, rc, sink) return nil }) @@ -350,6 +452,7 @@ func runPreflightByRole(ctx context.Context, cfg selfhosted.PreflightConfig, sin ClusterValidatorImage: clusterValidatorImage, ClusterValidatorPullSecret: checkClusterValidatorPullSecret, ClusterValidatorNoCleanup: checkClusterValidatorNoCleanup, + StaleNamespaceProber: staleNSProber, } gpuResults = selfhosted.RunPreflightForRole(egCtx, cfg, selfhosted.RoleComputePlane, rc, sink) return nil @@ -358,7 +461,15 @@ func runPreflightByRole(ctx context.Context, cfg selfhosted.PreflightConfig, sin return append(cpResults, gpuResults...) default: // ModeSingle — no context flags; union both role check sets sequentially. - cpRC := selfhosted.RoleConfig{SISURL: icmsURL} + cpRC := selfhosted.RoleConfig{ + SISURL: icmsURL, + ClusterValidator: cpClusterValidator, + ClusterValidatorImage: clusterValidatorImage, + ClusterValidatorPullSecret: checkClusterValidatorPullSecret, + ClusterValidatorNoCleanup: checkClusterValidatorNoCleanup, + ClusterValidatorRegistries: registries, + StaleNamespaceProber: staleNSProber, + } gpuRC := selfhosted.RoleConfig{ SISURL: icmsURL, InotifyProber: inotifyProber, @@ -366,6 +477,7 @@ func runPreflightByRole(ctx context.Context, cfg selfhosted.PreflightConfig, sin ClusterValidatorImage: clusterValidatorImage, ClusterValidatorPullSecret: checkClusterValidatorPullSecret, ClusterValidatorNoCleanup: checkClusterValidatorNoCleanup, + StaleNamespaceProber: staleNSProber, } cpResults := selfhosted.RunPreflightForRole(ctx, cfg, selfhosted.RoleControlPlane, cpRC, sink) gpuResults := selfhosted.RunPreflightForRole(ctx, cfg, selfhosted.RoleComputePlane, gpuRC, sink) @@ -373,15 +485,9 @@ func runPreflightByRole(ctx context.Context, cfg selfhosted.PreflightConfig, sin } } -// Resolves the validator image from the viper-backed config chain -// (flag > env > config-file > default). Returns ("", false) when nothing -// is configured so the caller can surface a clear "not configured" -// warning instead of pulling from a stale built-in default. -// -// When the configured value already has a tag, it is used as-is. When it -// names only a repo, the latest tag is discovered from the registry -// (preferring stable over rc, 1h cached); any discovery failure falls -// back to the configured value unchanged. +// resolveClusterValidatorImage resolves the validator image from flag > env > +// config-file. Returns ("", false) when unconfigured. When only a repo is +// given, discovers the latest stable tag (1h cached; falls back on failure). func resolveClusterValidatorImage(ctx context.Context) (string, bool) { image := viper.GetString("cluster_validator_image") if image == "" { diff --git a/src/clis/nvcf-cli/cmd/self_hosted_check_test.go b/src/clis/nvcf-cli/cmd/self_hosted_check_test.go index d3a6d94b6..218bdca80 100644 --- a/src/clis/nvcf-cli/cmd/self_hosted_check_test.go +++ b/src/clis/nvcf-cli/cmd/self_hosted_check_test.go @@ -30,6 +30,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "nvcf-cli/internal/selfhosted/kubectx" "nvcf-cli/internal/selfhosted/progress" ) @@ -343,6 +344,165 @@ func TestCheck_SplitClusterMode(t *testing.T) { assert.Contains(t, categories, "compute-plane-cluster", "expected compute-plane-cluster in split mode") } +// TestComputePlaneIsTargeted tests the predicate that gates the cluster-validator +// probe. The validator should run when the compute plane is explicitly targeted +// (--compute-plane, --all) or implicitly targeted by ModeSingle + --pre. +// It must NOT run for --pre alone in ModeSplit, where the two context flags +// identify separate clusters and --pre does not constitute targeting the compute plane. +func TestComputePlaneIsTargeted(t *testing.T) { + t.Cleanup(func() { + checkPre = false + checkComputePlane = false + checkAll = false + }) + + tests := []struct { + name string + pre bool + cp bool + all bool + mode kubectx.Mode + want bool + }{ + {"--compute-plane single", false, true, false, kubectx.ModeSingle, true}, + {"--compute-plane split", false, true, false, kubectx.ModeSplit, true}, + {"--all single", false, false, true, kubectx.ModeSingle, true}, + {"--all split", false, false, true, kubectx.ModeSplit, true}, + {"--pre single — implicit compute plane", true, false, false, kubectx.ModeSingle, true}, + {"--pre split — must not target compute plane", true, false, false, kubectx.ModeSplit, false}, + {"--control-plane only", false, false, false, kubectx.ModeSingle, false}, + {"no relevant flag", false, false, false, kubectx.ModeSingle, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + checkPre = tt.pre + checkComputePlane = tt.cp + checkAll = tt.all + assert.Equal(t, tt.want, computePlaneIsTargeted(tt.mode)) + }) + } +} + +// TestCheck_ComputePlaneFlagRunsChecks verifies that --compute-plane alone +// produces compute-plane-cluster category events. Before the gating fix this +// flag was a complete no-op and produced no check events at all. +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") +} + +// TestCheck_ValidatorSkipNoteAppearsOnComputePlane verifies that the +// "cluster-validator skipped" note appears on stderr when --compute-plane is +// used with --skip-cluster-validation (compute plane is targeted, validator is +// suppressed). +func TestCheck_ValidatorSkipNoteAppearsOnComputePlane(t *testing.T) { + t.Cleanup(func() { + selfHostedJSON = false + selfHostedOutput = "text" + checkComputePlane = false + checkSkipClusterValidation = false + }) + + var stderr bytes.Buffer + rootCmd.SetErr(&stderr) + rootCmd.SetOut(&bytes.Buffer{}) + + rootCmd.SetArgs([]string{ + "self-hosted", "check", "--compute-plane", + "--skip-cluster-validation", "--json", + }) + _ = rootCmd.Execute() + + assert.Contains(t, stderr.String(), "cluster-validator skipped", + "expected skip note when compute plane is targeted and --skip-cluster-validation is set") +} + +// TestCheck_ValidatorSkipNoteAbsentForPreInSplitMode verifies that the +// "cluster-validator skipped" note does NOT appear when --pre is used in +// split mode, because the compute plane is not explicitly targeted and +// ModeSplit + --pre does not implicitly make either context the compute plane. +func TestCheck_ValidatorSkipNoteAbsentForPreInSplitMode(t *testing.T) { + t.Cleanup(func() { + selfHostedJSON = false + selfHostedOutput = "text" + checkPre = false + checkSkipClusterValidation = false + selfHostedControlPlaneContext = "" + selfHostedComputePlaneContext = "" + }) + + var stderr bytes.Buffer + rootCmd.SetErr(&stderr) + rootCmd.SetOut(&bytes.Buffer{}) + + rootCmd.SetArgs([]string{ + "self-hosted", "check", "--pre", + "--skip-cluster-validation", "--json", + "--control-plane-context", "admin@cp", + "--compute-plane-context", "admin@gpu1", + }) + _ = rootCmd.Execute() + + assert.NotContains(t, stderr.String(), "cluster-validator skipped", + "skip note must not appear for --pre in split mode (compute plane not targeted)") +} + // parseJSONLLines splits s into non-empty lines, skips any non-JSON lines // (e.g. cobra error messages written to stderr), and unmarshals each JSON line // as an object. Returns them in order. diff --git a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go index 5df457fa7..a9e1c6832 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go +++ b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go @@ -71,6 +71,15 @@ type ClusterValidatorParams struct { Image string PullSecret string NoCleanup bool + // Role selects which check set the validator runs: "control-plane" or + // "compute-plane" (empty = compute-plane default). Passed to the Job as + // VALIDATOR_ROLE. See clustervalidator.RoleControlPlane / RoleComputePlane. + Role string + // Registries is a list of additional "host:port" registry endpoints to + // probe for reachability in the control-plane validator ConfigMap (in + // addition to nvcr.io which is always included). Ignored for the + // compute-plane role. + Registries []string } // Err is non-nil only when the run failed to execute (RBAC bootstrap, @@ -98,12 +107,12 @@ func NewClusterValidator() ClusterValidator { if err != nil { return ClusterValidatorResult{Err: fmt.Errorf("building kubernetes client: %w", err)} } - return runClusterValidator(ctx, client, p.Image, p.PullSecret, p.NoCleanup) + return runClusterValidator(ctx, client, p.Image, p.PullSecret, p.NoCleanup, p.Role, p.Registries) } } // Testable core. Pass a fake clientset to unit-test without a real cluster. -func runClusterValidator(ctx context.Context, client kubernetes.Interface, image, pullSecret string, noCleanup bool) ClusterValidatorResult { +func runClusterValidator(ctx context.Context, client kubernetes.Interface, image, pullSecret string, noCleanup bool, role string, registries []string) ClusterValidatorResult { if image == "" { // Defensive: callers gate on configured image before invoking the // validator, so this branch shouldn't fire in normal use. @@ -128,11 +137,23 @@ func runClusterValidator(ctx context.Context, client kubernetes.Interface, image return ClusterValidatorResult{Err: fmt.Errorf("bootstrapping validator RBAC: %w", err)} } + // For the control-plane role, create a ConfigMap with reachability + // endpoints and enforcement config so the validator runs its configurable + // checks. Best-effort: a failure here is logged but does not abort the + // run — the validator gracefully skips configurable checks when the + // ConfigMap is absent. + if role == clusterValidatorControlPlaneRole { + if err := ensureClusterValidatorConfig(vctx, client, registries); err != nil { + // Non-fatal: reachability checks will be skipped, not the whole run. + _ = err + } + } + sweepPriorClusterValidatorJobs(vctx, client) jobName := fmt.Sprintf("%s-%d", clusterValidatorName, time.Now().UnixNano()) if _, err := client.BatchV1().Jobs(clusterValidatorNamespace).Create( - vctx, buildClusterValidatorJob(jobName, image, pullSecret, noCleanup), metav1.CreateOptions{}, + vctx, buildClusterValidatorJob(jobName, image, pullSecret, role, noCleanup), metav1.CreateOptions{}, ); err != nil { return ClusterValidatorResult{Err: fmt.Errorf("creating validator Job: %w", err)} } @@ -167,9 +188,8 @@ func runClusterValidator(ctx context.Context, client kubernetes.Interface, image } // Creates the SA/ClusterRole/ClusterRoleBinding the validator pod runs under, -// idempotent via AlreadyExists tolerance. Permissions mirror the -// nvca-operator chart's validator rbac.yaml (read-only). Resources persist -// across runs; the sweep only deletes Jobs. +// idempotent via AlreadyExists tolerance. ClusterRole uses update-or-create so +// newer CLI versions replace stale rules without the operator needing to delete. func ensureClusterValidatorRBAC(ctx context.Context, client kubernetes.Interface) error { labels := clusterValidatorLabels() @@ -187,11 +207,21 @@ func ensureClusterValidatorRBAC(ctx context.Context, client kubernetes.Interface cr := &rbacv1.ClusterRole{ ObjectMeta: metav1.ObjectMeta{Name: clusterValidatorName, Labels: labels}, Rules: []rbacv1.PolicyRule{ - {APIGroups: []string{""}, Resources: []string{"nodes", "pods", "namespaces", "services", "configmaps"}, Verbs: []string{"get", "list", "watch"}}, + // 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"}}, }, } @@ -266,19 +296,121 @@ func sweepManagedPullSecrets(ctx context.Context, client kubernetes.Interface) { ) } -// IfNotPresent so locally-imported images (k3d image import, kind load) are -// reused. VALIDATOR_CONFIG_NAME is empty so the validator skips the -// configurable reachability/network-policy sections (ConfigMap support is a -// follow-up). -// -// VALIDATOR_PREFLIGHT=true puts the validator in preflight mode: it runs the -// readiness checks and exits, but does NOT write its summary ConfigMap. The -// ConfigMap exists for the metrics path (the NVCA agent watches it and -// republishes on /metrics), which is meaningless here because preflight runs -// before NVCA is installed and our RBAC bootstrap grants no configmaps -// create/update. Tagging the invocation keeps preflight a clean no-op rather -// than emitting a confusing "failed to create summary ConfigMap" warning. -func buildClusterValidatorJob(name, image, pullSecret string, noCleanup bool) *batchv1.Job { +// clusterValidatorControlPlaneRole is the role value passed as VALIDATOR_ROLE +// when running against the control-plane cluster. Matches nvca's RoleControlPlane +// without importing that package. +const clusterValidatorControlPlaneRole = "control-plane" + +// clusterValidatorConfigName is the default ConfigMap name the validator binary +// looks for when VALIDATOR_CONFIG_NAME is empty. Must stay in sync with +// defaultConfigMapName in nvca/cmd/cluster-validator/main.go. +const clusterValidatorConfigName = "cluster-validator-network-checks" + +// controlPlaneValidatorConfigTemplate is the baseline network-check ConfigMap for +// control-plane preflight: nvcr.io reachability (critical) and NetworkPolicy +// enforcement (non-critical). Extra registries are appended as non-critical probes. +const controlPlaneValidatorConfigTemplate = `reachability: + endpoints: + - name: nvcr.io + host: nvcr.io + port: 443 + protocol: tcp+tls + critical: true +enforcement: + enabled: true + testImage: busybox:1.36 + timeoutSeconds: 60 + critical: false +` + +// ensureClusterValidatorConfig creates or updates the network-check ConfigMap. +// extraRegistries are added as non-critical tcp+tls probes. Best-effort: the +// validator skips configurable checks when the ConfigMap is absent. +func ensureClusterValidatorConfig(ctx context.Context, client kubernetes.Interface, extraRegistries []string) error { + content := buildControlPlaneValidatorConfig(extraRegistries) + desired := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: clusterValidatorConfigName, + Namespace: clusterValidatorNamespace, + Labels: clusterValidatorLabels(), + }, + Data: map[string]string{"config.yaml": content}, + } + + existing, err := client.CoreV1().ConfigMaps(clusterValidatorNamespace).Get(ctx, clusterValidatorConfigName, metav1.GetOptions{}) + if err != nil { + if !apierrors.IsNotFound(err) { + return fmt.Errorf("get validator config ConfigMap: %w", err) + } + if _, err := client.CoreV1().ConfigMaps(clusterValidatorNamespace).Create(ctx, desired, metav1.CreateOptions{}); err != nil { + return fmt.Errorf("create validator config ConfigMap: %w", err) + } + return nil + } + + // Always update so a newer CLI version's config (or new registries) replaces stale content. + existing.Data = desired.Data + existing.Labels = desired.Labels + if _, err := client.CoreV1().ConfigMaps(clusterValidatorNamespace).Update(ctx, existing, metav1.UpdateOptions{}); err != nil { + return fmt.Errorf("update validator config ConfigMap: %w", err) + } + return nil +} + +// buildControlPlaneValidatorConfig assembles the network-check ConfigMap YAML +// from the baseline template plus any operator-supplied extra registries. +func buildControlPlaneValidatorConfig(extraRegistries []string) string { + if len(extraRegistries) == 0 { + return controlPlaneValidatorConfigTemplate + } + + // Parse host:port entries and append as non-critical tcp+tls endpoints. + var extra strings.Builder + for _, reg := range extraRegistries { + host, port := parseRegistryHostPort(reg) + if host == "" { + continue + } + // Append under the existing reachability.endpoints list. + fmt.Fprintf(&extra, " - name: %s\n host: %s\n port: %d\n protocol: tcp+tls\n critical: false\n", host, host, port) + } + if extra.Len() == 0 { + return controlPlaneValidatorConfigTemplate + } + + // Insert extra endpoints after the nvcr.io entry (before the enforcement block). + return strings.Replace(controlPlaneValidatorConfigTemplate, + "enforcement:", extra.String()+"enforcement:", 1) +} + +// parseRegistryHostPort splits a "host:port" string. Returns port 443 when +// no port is specified or when the port is not a valid number. +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 +} + +// buildClusterValidatorJob creates the validator Job. PullIfNotPresent reuses +// locally-imported images. VALIDATOR_PREFLIGHT=true skips the summary ConfigMap +// write. VALIDATOR_ROLE selects the check set (control-plane vs compute-plane). +func buildClusterValidatorJob(name, image, pullSecret, role string, noCleanup bool) *batchv1.Job { backoff := int32(0) podSpec := corev1.PodSpec{ ServiceAccountName: clusterValidatorName, @@ -291,6 +423,7 @@ func buildClusterValidatorJob(name, image, pullSecret string, noCleanup bool) *b {Name: "VALIDATOR_CONFIG_NAMESPACE", Value: clusterValidatorNamespace}, {Name: "VALIDATOR_CONFIG_NAME", Value: ""}, {Name: "VALIDATOR_PREFLIGHT", Value: "true"}, + {Name: "VALIDATOR_ROLE", Value: role}, }, }}, } diff --git a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go index 70ef4a6a9..95051c402 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go @@ -113,7 +113,7 @@ func TestRunClusterValidator_EmptyImage(t *testing.T) { // branch is defensive. Verify it returns a clear error and makes // no API calls. client := fake.NewSimpleClientset() - res := runClusterValidator(context.Background(), client, "", "", false) + res := runClusterValidator(context.Background(), client, "", "", false, "", nil) require.Error(t, res.Err) assert.Contains(t, res.Err.Error(), "image is empty") assert.False(t, res.Passed) @@ -124,7 +124,7 @@ func TestClusterValidatorCheck_OrchestratorErrorStaysWarning(t *testing.T) { cv := func(_ context.Context, _ ClusterValidatorParams) ClusterValidatorResult { return ClusterValidatorResult{Err: fmt.Errorf("transient: API server unreachable")} } - r := clusterValidatorCheck(cv, "", "", "", false).Run(context.Background()) + r := clusterValidatorCheck(cv, "", "", "", false, "", nil).Run(context.Background()) assert.False(t, r.Passed) assert.Equal(t, "warning", r.Severity, "transient orchestrator failures should not fail the overall preflight") @@ -169,7 +169,7 @@ func TestRunClusterValidator_HappyPath(t *testing.T) { }, nil }) - res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false) + res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false, "", nil) require.NoError(t, res.Err, "happy path must not surface an error") assert.True(t, res.Passed, "Succeeded>0 maps to Passed=true") assert.Equal(t, int32(0), res.ExitCode) @@ -204,7 +204,7 @@ func TestRunClusterValidator_JobFailed(t *testing.T) { }) client.PrependReactor("list", "pods", podListReactor("")) - res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false) + res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false, "", nil) require.NoError(t, res.Err, "a clean Passed=false verdict must not set Err") assert.False(t, res.Passed, "Failed>0 maps to Passed=false") assert.NotEmpty(t, res.JobName, "JobName must be populated on failure for kubectl-logs follow-up") @@ -237,7 +237,7 @@ func TestRunClusterValidator_RBACIdempotent(t *testing.T) { }) client.PrependReactor("list", "pods", podListReactor("")) - res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false) + res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false, "", nil) require.NoError(t, res.Err, "AlreadyExists on RBAC bootstrap must be treated as success") assert.True(t, res.Passed) } @@ -264,7 +264,7 @@ func TestRunClusterValidator_RBACRefreshesClusterRoleRules(t *testing.T) { }) client.PrependReactor("list", "pods", podListReactor("")) - res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false) + res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false, "", nil) require.NoError(t, res.Err) got, err := client.RbacV1().ClusterRoles().Get(context.Background(), clusterValidatorName, metav1.GetOptions{}) @@ -273,6 +273,84 @@ func TestRunClusterValidator_RBACRefreshesClusterRoleRules(t *testing.T) { "ClusterRole rules must be refreshed to the current set on each run, not kept stale") } +// TestEnsureClusterValidatorRBAC_WritableResources verifies that the +// bootstrapped ClusterRole grants the write verbs required by enforcement +// checks (namespace/pod create+delete), probe log reading (pods/log get), +// and the Gateway API checks added in Req 3/4. +func TestEnsureClusterValidatorRBAC_WritableResources(t *testing.T) { + client := fake.NewSimpleClientset() + ctx := context.Background() + + require.NoError(t, ensureClusterValidatorRBAC(ctx, client)) + + cr, err := client.RbacV1().ClusterRoles().Get(ctx, clusterValidatorName, metav1.GetOptions{}) + require.NoError(t, err) + + type check struct { + group string + resource string + verb string + } + required := []check{ + // Enforcement checks create and delete probe namespaces. + {"", "namespaces", "create"}, + {"", "namespaces", "delete"}, + // Probe pods spun up for node-to-node and inter-namespace checks. + {"", "pods", "create"}, + {"", "pods", "delete"}, + // Log reading: fetch probe output without exec. + {"", "pods/log", "get"}, + // Active LB probe service. + {"", "services", "create"}, + {"", "services", "delete"}, + // Enforcement check creates/updates/deletes NetworkPolicies in temp namespace. + {"networking.k8s.io", "networkpolicies", "create"}, + {"networking.k8s.io", "networkpolicies", "update"}, + {"networking.k8s.io", "networkpolicies", "delete"}, + // Gateway API health checks. + {"gateway.networking.k8s.io", "gatewayclasses", "get"}, + {"gateway.networking.k8s.io", "gateways", "list"}, + {"gateway.networking.k8s.io", "httproutes", "get"}, + {"gateway.networking.k8s.io", "grpcroutes", "list"}, + } + + for _, want := range required { + t.Run(fmt.Sprintf("%s/%s/%s", want.group, want.resource, want.verb), func(t *testing.T) { + assert.True(t, rbacRuleCovers(cr.Rules, want.group, want.resource, want.verb), + "ClusterRole must grant %s on %s (group %q)", want.verb, want.resource, want.group) + }) + } +} + +// rbacRuleCovers returns true when any PolicyRule in rules grants verb on +// resource within group. Wildcard verbs ("*") are treated as matching any verb. +func rbacRuleCovers(rules []rbacv1.PolicyRule, group, resource, verb string) bool { + for _, r := range rules { + if len(r.NonResourceURLs) > 0 { + continue // non-resource rules don't apply to API resources + } + if !strSliceContains(r.APIGroups, group) { + continue + } + if !strSliceContains(r.Resources, resource) { + continue + } + if strSliceContains(r.Verbs, verb) || strSliceContains(r.Verbs, "*") { + return true + } + } + return false +} + +func strSliceContains(ss []string, s string) bool { + for _, v := range ss { + if v == s { + return true + } + } + return false +} + func TestRunClusterValidator_ImagePullBackOffShortCircuits(t *testing.T) { client := fake.NewSimpleClientset() var jobName atomic.Value @@ -317,7 +395,7 @@ func TestRunClusterValidator_ImagePullBackOffShortCircuits(t *testing.T) { }) start := time.Now() - res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false) + res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false, "", nil) elapsed := time.Since(start) require.Error(t, res.Err, "ImagePullBackOff must short-circuit the wait with an error") @@ -355,7 +433,7 @@ func TestRunClusterValidator_LogFetchSurvivesValidatorTimeout(t *testing.T) { client.PrependReactor("list", "pods", podListReactor("")) // Parent ctx stays alive for the entire run; only vctx expires. - res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false) + res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false, "", nil) require.Error(t, res.Err, "wait must surface the deadline-exceeded error") assert.Contains(t, res.Err.Error(), "waiting for job", @@ -408,13 +486,13 @@ func TestRunClusterValidator_ContextCanceled(t *testing.T) { time.Sleep(50 * time.Millisecond) cancel() }() - res := runClusterValidator(ctx, client, "test-image:1.0", "", false) + res := runClusterValidator(ctx, client, "test-image:1.0", "", false, "", nil) require.Error(t, res.Err) assert.Contains(t, res.Err.Error(), "context") } func TestBuildClusterValidatorJobShape(t *testing.T) { - job := buildClusterValidatorJob("test-job", "img:1", "", false) + job := buildClusterValidatorJob("test-job", "img:1", "", "", false) assert.Equal(t, "test-job", job.Name) assert.Equal(t, clusterValidatorNamespace, job.Namespace) @@ -451,19 +529,19 @@ func TestBuildClusterValidatorJobShape(t *testing.T) { } func TestBuildClusterValidatorJobShape_WithPullSecret(t *testing.T) { - job := buildClusterValidatorJob("test-job", "img:1", "nvcr-pull-secret", false) + job := buildClusterValidatorJob("test-job", "img:1", "nvcr-pull-secret", "", false) require.Len(t, job.Spec.Template.Spec.ImagePullSecrets, 1) assert.Equal(t, "nvcr-pull-secret", job.Spec.Template.Spec.ImagePullSecrets[0].Name) } func TestBuildClusterValidatorJobShape_NoPullSecret(t *testing.T) { - job := buildClusterValidatorJob("test-job", "img:1", "", false) + job := buildClusterValidatorJob("test-job", "img:1", "", "", false) assert.Empty(t, job.Spec.Template.Spec.ImagePullSecrets, "empty pull-secret arg must not produce an empty-name ImagePullSecrets entry") } func TestBuildClusterValidatorJobShape_NoCleanup(t *testing.T) { - job := buildClusterValidatorJob("test-job", "img:1", "", true) + job := buildClusterValidatorJob("test-job", "img:1", "", "", true) assert.Nil(t, job.Spec.TTLSecondsAfterFinished, "--no-cleanup must omit TTLSecondsAfterFinished so the Job persists for debugging") } @@ -521,6 +599,55 @@ func TestKubectlLogsHint_EmptyJob(t *testing.T) { "empty jobName must produce empty hint so callers can compose detail without conditionals") } +func TestBuildControlPlaneValidatorConfig_NoExtras(t *testing.T) { + got := buildControlPlaneValidatorConfig(nil) + assert.Equal(t, controlPlaneValidatorConfigTemplate, got, + "no extra registries must return the template unchanged") + assert.Contains(t, got, "nvcr.io", "nvcr.io must always be present") + assert.Contains(t, got, "enforcement:", "enforcement block must be present") +} + +func TestBuildControlPlaneValidatorConfig_WithExtras(t *testing.T) { + got := buildControlPlaneValidatorConfig([]string{"harbor.company.internal:443", "ghcr.io:443"}) + assert.Contains(t, got, "harbor.company.internal") + assert.Contains(t, got, "ghcr.io") + assert.Contains(t, got, "nvcr.io", "nvcr.io must still be present alongside extras") + assert.Contains(t, got, "enforcement:", "enforcement block must still be present after extras") + // Extra registries must appear BEFORE enforcement. + harborIdx := strings.Index(got, "harbor.company.internal") + enforcementIdx := strings.Index(got, "enforcement:") + assert.Less(t, harborIdx, enforcementIdx, "extra registry endpoints must appear before the enforcement block") +} + +func TestBuildControlPlaneValidatorConfig_InvalidRegistrySkipped(t *testing.T) { + // A blank entry is parsed as host="" → skipped; only the valid entry appears. + got := buildControlPlaneValidatorConfig([]string{" ", "valid.registry.internal:5000"}) + assert.Contains(t, got, "valid.registry.internal", "valid registry must appear") + // The blank entry must not add an empty host: line. + assert.NotContains(t, got, "host: \n", "blank entry must not produce an empty host line") +} + +func TestParseRegistryHostPort(t *testing.T) { + tests := []struct { + in string + wantHost string + wantPort int + }{ + {"nvcr.io:443", "nvcr.io", 443}, + {"harbor.company.internal:5000", "harbor.company.internal", 5000}, + {"registry.example.com", "registry.example.com", 443}, // no port → 443 + {"", "", 0}, // empty → skip + {" ", "", 0}, // blank → skip + } + for _, tt := range tests { + t.Run(tt.in, func(t *testing.T) { + h, p := parseRegistryHostPort(tt.in) + assert.Equal(t, tt.wantHost, h) + assert.Equal(t, tt.wantPort, p) + }) + } +} + func alreadyExistsReactor(resource, name string) ktesting.ReactionFunc { gr := schema.GroupResource{Resource: resource} return func(action ktesting.Action) (bool, runtime.Object, error) { From 1ffb0c0ac498f1f8b8cd64b441ad614decc7fe6e Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 12 Aug 2026 02:14:34 +0530 Subject: [PATCH 02/13] feat(check): detect stale NVCF namespaces and add control-plane validator wiring --- .../nvcf-cli/internal/selfhosted/preflight.go | 189 ++++++++++++++--- .../internal/selfhosted/preflight_test.go | 24 ++- .../internal/selfhosted/stale_namespace.go | 114 ++++++++++ .../selfhosted/stale_namespace_test.go | 198 ++++++++++++++++++ 4 files changed, 497 insertions(+), 28 deletions(-) create mode 100644 src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go create mode 100644 src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go diff --git a/src/clis/nvcf-cli/internal/selfhosted/preflight.go b/src/clis/nvcf-cli/internal/selfhosted/preflight.go index e4dc5858a..78b061410 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/preflight.go +++ b/src/clis/nvcf-cli/internal/selfhosted/preflight.go @@ -195,6 +195,15 @@ func runVersionCmdOnce(ctx context.Context, path string, args []string, re *rege type PreflightConfig struct { LocalOnly bool Tools []BinarySpec + + // Registries is the list of container registries to credential-check. + // When empty, the registry-credentials category is omitted entirely. + // Populated by the cmd layer from EnumerateRegistries. + Registries []RegistryEntry + + // RegistryChecker validates credentials for one registry. Nil skips the + // category. Production wires NewRegistryCredentialChecker; tests pass fakes. + RegistryChecker RegistryCredentialChecker } // DefaultTools returns the kubectl/helmfile/helm specs with version floors @@ -252,10 +261,18 @@ type Role int const ( RoleLocalOnly Role = iota // shared local-host tools only; no kubectl contact - RoleControlPlane // shared + Gateway API CRDs + default StorageClass + RoleControlPlane // shared + gateway/StorageClass/LB checks via cluster-validator RoleComputePlane // shared + GPU operator + GPU node labels + (optional) SIS reachability ) +// Validator role strings passed as VALIDATOR_ROLE to the cluster-validator Job. +// These must match the constants in the nvca cluster-validator binary's +// clustervalidator package (RoleControlPlane / RoleComputePlane). +const ( + validatorRoleControlPlane = "control-plane" + validatorRoleComputePlane = "compute-plane" +) + // String returns a human-readable name for the role. func (r Role) String() string { switch r { @@ -289,6 +306,16 @@ type RoleConfig struct { ClusterValidatorImage string ClusterValidatorPullSecret string ClusterValidatorNoCleanup bool + + // StaleNamespaceProber detects NVCF stack namespaces that are stuck + // Terminating or exist as empty shells after a partial teardown. Nil skips + // the check; production wires NewStaleNamespaceProber; tests pass fakes. + StaleNamespaceProber StaleNamespaceProber + + // ClusterValidatorRegistries is an optional list of "host:port" registry + // endpoints added to the control-plane validator ConfigMap alongside the + // built-in nvcr.io probe. Ignored for the compute-plane validator. + ClusterValidatorRegistries []string } // categorySpec groups a set of checks under a named category. Categories run @@ -318,6 +345,13 @@ func buildCategories(cfg PreflightConfig, role Role, rc RoleConfig) []categorySp out = append(out, *local) } + // 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)) + } + if cfg.LocalOnly || role == RoleLocalOnly { return out } @@ -354,28 +388,42 @@ func buildLocalHostCategory(cfg PreflightConfig) *categorySpec { return &categorySpec{name: "local-host-tools", role: RoleLocalOnly, checks: checks} } -// controlPlaneCheckCategory returns the placeholder cluster-side checks for -// control plane: Gateway API CRDs, default StorageClass. Real probes land -// when M3 ships; for now they emit an "info" CheckResult so the renderer -// matrix is observable. -func controlPlaneCheckCategory(_ RoleConfig) categorySpec { - return categorySpec{ - name: "control-plane-cluster", - role: RoleControlPlane, - checks: []binaryCheckSpec{ - placeholderCheck("gateway-api-crds", "checking Gateway API CRDs…", "Gateway API CRD probe — pending M3 cluster-side preflight"), - placeholderCheck("default-storageclass", "checking default StorageClass…", "Default StorageClass probe — pending M3 cluster-side preflight"), - }, +// controlPlaneCheckCategory returns the cluster-side checks for the control +// plane. The stale-namespace check runs first so a leftover namespace from a +// prior partial teardown is surfaced before any other cluster work. +// Gateway API CRD and StorageClass probes are placeholders pending M3. +func controlPlaneCheckCategory(rc RoleConfig) categorySpec { + cat := categorySpec{ + name: "control-plane-cluster", + role: RoleControlPlane, + checks: []binaryCheckSpec{}, + } + // Stale namespace check runs first so leftover namespaces surface + // before any other cluster work. + if rc.StaleNamespaceProber != nil { + cat.checks = append(cat.checks, + staleNamespaceCheck(rc.StaleNamespaceProber, rc.KubeContext, nvcfControlPlaneNamespaces)) + } + // Containerized cluster-validator probe for control-plane checks + // (Gateway API CRDs, Envoy Gateway, StorageClass, external LB, + // node-to-node overlay, and reachability to nvcr.io + extra registries). + if rc.ClusterValidator != nil { + cat.checks = append(cat.checks, clusterValidatorCheck( + rc.ClusterValidator, + rc.KubeContext, + rc.ClusterValidatorImage, + rc.ClusterValidatorPullSecret, + rc.ClusterValidatorNoCleanup, + validatorRoleControlPlane, + rc.ClusterValidatorRegistries, + )) } + return cat } -// computePlaneCheckCategory returns the placeholder compute-plane checks. -// Conditionally adds an SIS reachability probe when RoleConfig.SISURL is set, -// a node-inotify-limits probe when RoleConfig.InotifyProber is set, and a -// containerized cluster-validator probe when RoleConfig.ClusterValidator is -// set. Each conditional probe is opt-in via its own RoleConfig field so -// callers that opted out via a --skip-* flag simply leave the corresponding -// field nil/empty and the check is omitted from the category entirely. +// computePlaneCheckCategory returns the compute-plane checks. SIS, inotify, +// and cluster-validator probes are opt-in via their RoleConfig fields; a nil +// or empty field omits the corresponding check from the category. func computePlaneCheckCategory(rc RoleConfig) categorySpec { cat := categorySpec{ name: "compute-plane-cluster", @@ -385,6 +433,13 @@ func computePlaneCheckCategory(rc RoleConfig) categorySpec { placeholderCheck("gpu-node-labels", "checking GPU node labels…", "GPU node-label probe — pending M+10 cluster-side preflight"), }, } + // Stale namespace check runs first so leftover namespaces from a prior + // partial teardown surface before any other cluster work. + if rc.StaleNamespaceProber != nil { + cat.checks = append([]binaryCheckSpec{ + staleNamespaceCheck(rc.StaleNamespaceProber, rc.KubeContext, nvcfComputePlaneNamespaces), + }, cat.checks...) + } if rc.SISURL != "" { cat.checks = append(cat.checks, sisReachabilityCheck(rc.SISURL)) } @@ -398,6 +453,8 @@ func computePlaneCheckCategory(rc RoleConfig) categorySpec { rc.ClusterValidatorImage, rc.ClusterValidatorPullSecret, rc.ClusterValidatorNoCleanup, + validatorRoleComputePlane, + nil, // registries: compute-plane doesn't use the ConfigMap reachability list )) } return cat @@ -503,10 +560,10 @@ func nodeInotifyCheck(prober NodeInotifyProber, kubeContext string) binaryCheckS } } -// Severity mapping: runner errors (RBAC/pull/timeout) -> warning, since -// they're operator-fixable infra issues; validator Passed=false -> -// error (real check failures); Passed=true -> info. -func clusterValidatorCheck(cv ClusterValidator, kubeContext, image, pullSecret string, noCleanup bool) binaryCheckSpec { +// clusterValidatorCheck runs the validator Job. Runner errors (RBAC/timeout) +// are warning severity; validator failures are error. role selects the check +// set via VALIDATOR_ROLE; registries extends the ConfigMap reachability list. +func clusterValidatorCheck(cv ClusterValidator, kubeContext, image, pullSecret string, noCleanup bool, role string, registries []string) binaryCheckSpec { const id = "cluster-validator" return binaryCheckSpec{ ID: id, @@ -521,6 +578,8 @@ func clusterValidatorCheck(cv ClusterValidator, kubeContext, image, pullSecret s Image: image, PullSecret: pullSecret, NoCleanup: noCleanup, + Role: role, + Registries: registries, }) r.Logs = result.Logs r.Detail = clusterValidatorDetail(result.JobName) @@ -552,6 +611,88 @@ func clusterValidatorDetail(jobName string) string { return "logs: " + hint } +// buildRegistryCredentialCategory returns the registry-credentials category +// that probes each configured registry for reachability and valid credentials. +func buildRegistryCredentialCategory(cfg PreflightConfig) categorySpec { + cat := categorySpec{ + name: "registry-credentials", + role: RoleLocalOnly, // runs regardless of cluster role + checks: make([]binaryCheckSpec, 0, len(cfg.Registries)), + } + for _, reg := range cfg.Registries { + reg := reg // capture loop var + cat.checks = append(cat.checks, registryCredentialCheck(cfg.RegistryChecker, reg)) + } + return cat +} + +// registryCredentialCheck returns a binaryCheckSpec that probes one registry. +// Critical registries (nvcr.io) fail at error severity; non-critical ones +// fail at warning so they don't block the operator on optional registries. +func registryCredentialCheck(checker RegistryCredentialChecker, entry RegistryEntry) binaryCheckSpec { + id := "registry-cred-" + entry.Registry + severity := "warning" + if entry.Critical { + severity = "error" + } + return binaryCheckSpec{ + ID: id, + HumanLabel: fmt.Sprintf("checking credentials for %s…", entry.Registry), + Run: func(ctx context.Context) CheckResult { + r := CheckResult{ + ID: id, + Severity: severity, + } + if err := checker(ctx, entry.Registry, entry.RepoHint); err != nil { + r.Message = entry.Registry + ": " + err.Error() + r.Err = err + return r + } + r.Passed = true + r.Severity = "info" + r.Message = entry.Registry + ": credentials valid" + return r + }, + } +} + +// staleNamespaceCheck detects NVCF namespaces stuck Terminating or left as +// empty shells after a partial teardown. Severity is error; prober errors +// degrade to warning so transient kubeconfig issues don't falsely fail. +func staleNamespaceCheck(prober StaleNamespaceProber, kubeContext string, namespaces []string) binaryCheckSpec { + const id = "stale-namespaces" + return binaryCheckSpec{ + ID: id, + HumanLabel: "checking for stale NVCF namespaces…", + Run: func(ctx context.Context) CheckResult { + r := CheckResult{ID: id, Severity: "error"} + stale, err := prober(ctx, kubeContext, namespaces) + if err != nil { + r.Severity = "warning" + r.Message = "stale namespace probe failed: " + err.Error() + r.Err = err + return r + } + if len(stale) == 0 { + r.Passed = true + r.Message = "no stale NVCF namespaces detected" + return r + } + parts := make([]string, 0, len(stale)) + names := make([]string, 0, len(stale)) + for _, ns := range stale { + parts = append(parts, ns.Name+" ("+ns.Reason+")") + names = append(names, ns.Name) + } + r.Message = fmt.Sprintf( + "%d stale namespace(s) detected: %s — remove with: kubectl delete namespace %s --force --grace-period=0", + len(stale), strings.Join(parts, ", "), strings.Join(names, " "), + ) + return r + }, + } +} + // placeholderCheck returns a binaryCheckSpec that emits a passing "info" // CheckResult with the given message. Used until M3/M+10 cluster-side probes ship. func placeholderCheck(id, label, message string) binaryCheckSpec { diff --git a/src/clis/nvcf-cli/internal/selfhosted/preflight_test.go b/src/clis/nvcf-cli/internal/selfhosted/preflight_test.go index 262075dff..93dee7b25 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/preflight_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/preflight_test.go @@ -408,7 +408,10 @@ func TestRunPreflightForRole_LocalOnly(t *testing.T) { func TestRunPreflightForRole_ControlPlaneAddsClusterCategory(t *testing.T) { sink := &captureSink{} cfg := PreflightConfig{Tools: []BinarySpec{passingToolSpec("kubectl", "1.30.0")}} - res := RunPreflightForRole(context.Background(), cfg, RoleControlPlane, RoleConfig{KubeContext: "admin@cp"}, sink) + // No StaleNamespaceProber and no ClusterValidator: the control-plane + // category is empty but the category itself still fires (CategoryCompleted + // is always emitted). + RunPreflightForRole(context.Background(), cfg, RoleControlPlane, RoleConfig{KubeContext: "admin@cp"}, sink) seen := map[string]bool{} for _, e := range sink.events { @@ -417,14 +420,27 @@ func TestRunPreflightForRole_ControlPlaneAddsClusterCategory(t *testing.T) { } } assert.True(t, seen["local-host-tools"], "expected local-host-tools category") - assert.True(t, seen["control-plane-cluster"], "expected control-plane-cluster category") + assert.True(t, seen["control-plane-cluster"], "expected control-plane-cluster category even with no checks configured") +} + +func TestRunPreflightForRole_ControlPlaneWithValidatorAddsClusterValidatorCheck(t *testing.T) { + sink := &captureSink{} + cfg := PreflightConfig{Tools: []BinarySpec{passingToolSpec("kubectl", "1.30.0")}} + cv := func(_ context.Context, p ClusterValidatorParams) ClusterValidatorResult { + return ClusterValidatorResult{Passed: true} + } + res := RunPreflightForRole(context.Background(), cfg, RoleControlPlane, RoleConfig{ + KubeContext: "admin@cp", + ClusterValidator: cv, + ClusterValidatorImage: "nvcf-validator:1.0", + }, sink) var gotCheckIDs []string for _, r := range res { gotCheckIDs = append(gotCheckIDs, r.ID) } - assert.Contains(t, gotCheckIDs, "gateway-api-crds") - assert.Contains(t, gotCheckIDs, "default-storageclass") + assert.Contains(t, gotCheckIDs, "cluster-validator", + "cluster-validator check must appear when ClusterValidator is configured for control-plane role") } func TestRunPreflightForRole_ComputePlaneWithoutSISURL(t *testing.T) { diff --git a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go new file mode 100644 index 000000000..8fe313175 --- /dev/null +++ b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go @@ -0,0 +1,114 @@ +/* +SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package selfhosted + +import ( + "context" + "fmt" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes" +) + +// nvcfControlPlaneNamespaces is the canonical set of namespaces created on the +// control-plane cluster by the NVCF self-managed stack. Any of these that +// exist without an active Helm release, or that are stuck Terminating, are +// leftover from a failed or partial teardown. +var nvcfControlPlaneNamespaces = []string{ + "cassandra-system", "nats-system", "nvcf", "api-keys", "ess", "sis", + "vault-system", "nvcf-backend", "envoy-gateway-system", "openbao-system", +} + +// nvcfComputePlaneNamespaces is the canonical set of namespaces created on the +// compute-plane cluster by the NVCF self-managed stack. +var nvcfComputePlaneNamespaces = []string{ + "nvca-operator", "nvca-system", +} + +// StaleNamespace describes a single NVCF stack namespace that appears to be a +// leftover from a failed or partial teardown. +type StaleNamespace struct { + Name string + Reason string // human-readable cause: "stuck Terminating" or "no Helm release" +} + +// StaleNamespaceProber inspects the given namespaces and returns those that +// appear stale. The probe is read-only; it never deletes or modifies anything. +// A non-nil error means the cluster could not be contacted; the returned slice +// may be a partial result. +type StaleNamespaceProber func(ctx context.Context, kubeContext string, namespaces []string) ([]StaleNamespace, error) + +// NewStaleNamespaceProber returns a StaleNamespaceProber backed by client-go. +// Chart-independent: works before any Helm release exists, and uses the +// operator's kubeconfig context to talk to the target cluster. +func NewStaleNamespaceProber() StaleNamespaceProber { + return func(ctx context.Context, kubeContext string, namespaces []string) ([]StaleNamespace, error) { + restCfg, err := loadKubeConfig(kubeContext) + if err != nil { + return nil, fmt.Errorf("building kubeconfig: %w", err) + } + client, err := kubernetes.NewForConfig(restCfg) + if err != nil { + return nil, fmt.Errorf("building kubernetes client: %w", err) + } + return probeStaleNamespaces(ctx, client, namespaces) + } +} + +// probeStaleNamespaces is the testable core that accepts a kubernetes.Interface +// so callers can inject fake.NewSimpleClientset in unit tests. +// +// A namespace is considered stale when either: +// - its DeletionTimestamp is set or its phase is Terminating (finalizer +// deadlock — it will never complete without operator intervention), or +// - it exists but holds no active Helm release (empty shell left by a partial +// helm uninstall or a failed teardown that cleaned the release but not the +// namespace). +// +// Helm 3 marks each release secret with the label owner=helm; absence of any +// such secret means no live Helm release occupies the namespace. +func probeStaleNamespaces(ctx context.Context, client kubernetes.Interface, namespaces []string) ([]StaleNamespace, error) { + var stale []StaleNamespace + for _, name := range namespaces { + ns, err := client.CoreV1().Namespaces().Get(ctx, name, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + continue // absent = healthy; the check only fires on unexpected presence + } + return stale, fmt.Errorf("get namespace %s: %w", name, err) + } + + if ns.DeletionTimestamp != nil || ns.Status.Phase == corev1.NamespaceTerminating { + stale = append(stale, StaleNamespace{Name: name, Reason: "stuck Terminating"}) + continue + } + + 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"}) + } + } + return stale, nil +} diff --git a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go new file mode 100644 index 000000000..8acd45a2f --- /dev/null +++ b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go @@ -0,0 +1,198 @@ +/* +SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package selfhosted + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" +) + +// -- probeStaleNamespaces -- + +func TestProbeStaleNamespaces_AbsentIsHealthy(t *testing.T) { + // A namespace that doesn't exist is not stale — it has simply never been + // created or was already fully deleted. + client := fake.NewSimpleClientset() + stale, err := probeStaleNamespaces(context.Background(), client, []string{"nvcf", "sis"}) + require.NoError(t, err) + assert.Empty(t, stale, "absent namespaces must not be reported as stale") +} + +func TestProbeStaleNamespaces_TerminatingIsByDeletionTimestamp(t *testing.T) { + now := metav1.Now() + client := fake.NewSimpleClientset(&corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "nvcf", DeletionTimestamp: &now}, + Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}, + }) + stale, err := probeStaleNamespaces(context.Background(), client, []string{"nvcf"}) + require.NoError(t, err) + require.Len(t, stale, 1) + assert.Equal(t, "nvcf", stale[0].Name) + assert.Contains(t, stale[0].Reason, "Terminating") +} + +func TestProbeStaleNamespaces_TerminatingIsByPhase(t *testing.T) { + client := fake.NewSimpleClientset(&corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "sis"}, + Status: corev1.NamespaceStatus{Phase: corev1.NamespaceTerminating}, + }) + stale, err := probeStaleNamespaces(context.Background(), client, []string{"sis"}) + require.NoError(t, err) + require.Len(t, stale, 1) + assert.Equal(t, "sis", stale[0].Name) + assert.Contains(t, stale[0].Reason, "Terminating") +} + +func TestProbeStaleNamespaces_EmptyShellNoHelmSecrets(t *testing.T) { + // Namespace exists and is Active but holds no Helm release secrets → + // leftover empty shell from a partial helm uninstall. + client := fake.NewSimpleClientset(&corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "nvcf"}, + Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}, + }) + stale, err := probeStaleNamespaces(context.Background(), client, []string{"nvcf"}) + require.NoError(t, err) + require.Len(t, stale, 1) + assert.Equal(t, "nvcf", stale[0].Name) + assert.Contains(t, stale[0].Reason, "Helm release") +} + +func TestProbeStaleNamespaces_HealthyReleaseNotStale(t *testing.T) { + // Namespace exists and carries an owner=helm secret → active Helm release, + // not stale. + client := fake.NewSimpleClientset( + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "nvcf"}, + Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}, + }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sh.helm.release.v1.nvcf.v1", + Namespace: "nvcf", + Labels: map[string]string{"owner": "helm", "name": "nvcf", "status": "deployed"}, + }, + }, + ) + stale, err := probeStaleNamespaces(context.Background(), client, []string{"nvcf"}) + require.NoError(t, err) + assert.Empty(t, stale, "namespace with an active Helm release must not be stale") +} + +func TestProbeStaleNamespaces_MixedNamespaces(t *testing.T) { + // One absent, one healthy, one terminating, one empty shell. + now := metav1.Now() + client := fake.NewSimpleClientset( + // "sis" — healthy with a Helm release + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "sis"}, + Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}, + }, + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "sh.helm.release.v1.sis.v1", Namespace: "sis", + Labels: map[string]string{"owner": "helm"}, + }, + }, + // "nvcf" — stuck terminating + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "nvcf", DeletionTimestamp: &now}, + Status: corev1.NamespaceStatus{Phase: corev1.NamespaceTerminating}, + }, + // "api-keys" — empty shell + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "api-keys"}, + Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}, + }, + // "cassandra-system" — absent (not present in fake) + ) + + namespaces := []string{"cassandra-system", "sis", "nvcf", "api-keys"} + stale, err := probeStaleNamespaces(context.Background(), client, namespaces) + require.NoError(t, err) + require.Len(t, stale, 2, "only nvcf (terminating) and api-keys (empty shell) should be stale") + + staleNames := make(map[string]string, 2) + for _, s := range stale { + staleNames[s.Name] = s.Reason + } + assert.Contains(t, staleNames, "nvcf") + assert.Contains(t, staleNames["nvcf"], "Terminating") + assert.Contains(t, staleNames, "api-keys") + assert.Contains(t, staleNames["api-keys"], "Helm release") +} + +// -- staleNamespaceCheck binaryCheckSpec -- + +func TestStaleNamespaceCheck_ProberErrorDegradestoWarning(t *testing.T) { + // A prober that cannot contact the cluster must not fail the overall check + // at error severity — it would produce false failures on transient network + // issues or misconfigured kubeconfigs. + prober := func(_ context.Context, _ string, _ []string) ([]StaleNamespace, error) { + return nil, fmt.Errorf("cluster unreachable") + } + r := staleNamespaceCheck(prober, "", []string{"nvcf"}).Run(context.Background()) + assert.False(t, r.Passed) + assert.Equal(t, "warning", r.Severity, + "prober errors must degrade to warning so transient failures do not block the operator") + assert.Contains(t, r.Message, "cluster unreachable") +} + +func TestStaleNamespaceCheck_StaleIsError(t *testing.T) { + // A successfully detected stale namespace must fail at error severity so + // anyFailed trips the non-zero exit code. + prober := func(_ context.Context, _ string, _ []string) ([]StaleNamespace, error) { + return []StaleNamespace{{Name: "nvcf", Reason: "stuck Terminating"}}, nil + } + r := staleNamespaceCheck(prober, "", []string{"nvcf"}).Run(context.Background()) + assert.False(t, r.Passed) + assert.Equal(t, "error", r.Severity, + "detected stale namespaces must use error severity so the exit code is non-zero") + assert.Contains(t, r.Message, "nvcf") + assert.Contains(t, r.Message, "kubectl delete namespace") +} + +func TestStaleNamespaceCheck_CleanPasses(t *testing.T) { + prober := func(_ context.Context, _ string, _ []string) ([]StaleNamespace, error) { + return nil, nil + } + r := staleNamespaceCheck(prober, "", []string{"nvcf", "sis"}).Run(context.Background()) + assert.True(t, r.Passed) +} + +func TestStaleNamespaceCheck_MessageNamesAllStaleNamespaces(t *testing.T) { + // The remediation command must name every stale namespace so the operator + // can copy-paste it without having to cross-reference the check output. + prober := func(_ context.Context, _ string, _ []string) ([]StaleNamespace, error) { + return []StaleNamespace{ + {Name: "nvcf", Reason: "stuck Terminating"}, + {Name: "api-keys", Reason: "no Helm release"}, + }, nil + } + r := staleNamespaceCheck(prober, "", []string{"nvcf", "api-keys"}).Run(context.Background()) + assert.Contains(t, r.Message, "nvcf") + assert.Contains(t, r.Message, "api-keys") + assert.Contains(t, r.Message, "kubectl delete namespace nvcf api-keys") +} From dfd490e7b2bf915e8ef825b11f30195ff118d19e Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 12 Aug 2026 02:14:51 +0530 Subject: [PATCH 03/13] feat(check): validate registry credentials before install using generic OCI auth --- .../internal/selfhosted/registry_cred.go | 202 +++++++++++++ .../internal/selfhosted/registry_cred_test.go | 286 ++++++++++++++++++ .../internal/selfhosted/validatortag.go | 200 ++++++++++-- .../internal/selfhosted/validatortag_test.go | 52 ++++ 4 files changed, 720 insertions(+), 20 deletions(-) create mode 100644 src/clis/nvcf-cli/internal/selfhosted/registry_cred.go create mode 100644 src/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go diff --git a/src/clis/nvcf-cli/internal/selfhosted/registry_cred.go b/src/clis/nvcf-cli/internal/selfhosted/registry_cred.go new file mode 100644 index 000000000..0167a9790 --- /dev/null +++ b/src/clis/nvcf-cli/internal/selfhosted/registry_cred.go @@ -0,0 +1,202 @@ +/* +SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package selfhosted + +import ( + "context" + "fmt" + "net/http" + "os" + "strings" + "time" + + "sigs.k8s.io/yaml" +) + +const ( + registryProbeTimeout = 10 * time.Second + + // certManagerRegistry is the well-known exception from the helmfile: + // cert-manager uses quay.io/jetstack images, not global.image.registry. + certManagerRegistry = "quay.io" +) + +// RegistryCredentialChecker probes whether credentials are present and valid +// for a registry. repoHint is the repository path used as the OAuth scope; +// pass "" to probe without a specific scope (works for public registries). +// Returns nil on success, a descriptive error otherwise. +type RegistryCredentialChecker func(ctx context.Context, registry, repoHint string) error + +// NewRegistryCredentialChecker returns a production RegistryCredentialChecker +// backed by real HTTP calls. +func NewRegistryCredentialChecker() RegistryCredentialChecker { + return probeRegistryCredential +} + +// probeRegistryCredential authenticates to registry using the OCI Bearer token +// flow. repoHint is the OAuth scope repository path. ECR registries return a +// clear diagnostic instead of attempting the Bearer flow. +func probeRegistryCredential(ctx context.Context, registry, repoHint string) error { + if isECRRegistry(registry) { + return fmt.Errorf("ECR registry detected — credential validation requires AWS CLI; " + + "run 'aws ecr get-login-password' to verify manually") + } + + pctx, cancel := context.WithTimeout(ctx, registryProbeTimeout) + defer cancel() + + client := &http.Client{Timeout: registryProbeTimeout} + + // Step 1: probe /v2/ unauthenticated. + probeURL := "https://" + registry + "/v2/" + req, err := http.NewRequestWithContext(pctx, http.MethodGet, probeURL, nil) + if err != nil { + return fmt.Errorf("build request: %w", err) + } + resp, err := client.Do(req) + if err != nil { + return fmt.Errorf("cannot reach %s: %w", registry, err) + } + + switch resp.StatusCode { + case http.StatusOK: + // Public registry — no credentials needed. + resp.Body.Close() + return nil + case http.StatusUnauthorized: + // Auth required — proceed with token exchange. + default: + resp.Body.Close() + return fmt.Errorf("unexpected status %s from %s", resp.Status, registry) + } + + // Step 2: exchange credentials for a Bearer token using the actual repo + // from the configured image (repoHint). Using a fake repo name causes + // org-level 403s from NGC and GHCR for non-existent orgs, which is + // indistinguishable from bad credentials. + wwwAuth := resp.Header.Get("Www-Authenticate") + resp.Body.Close() + + _, 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 +} + +// isECRRegistry returns true for AWS Elastic Container Registry hostnames, +// which use AWS SigV4 auth instead of the OCI Bearer token flow. +func isECRRegistry(registry string) bool { + return strings.Contains(registry, ".dkr.ecr.") && + strings.HasSuffix(registry, ".amazonaws.com") +} + +// RegistryEntry is one registry endpoint to credential-check. +type RegistryEntry struct { + // Registry is the hostname (and optional port) of the container registry. + Registry string + // RepoHint is the repository path used as the OAuth scope when probing + // credentials (e.g. "nvidia/nvcf-byoc/cluster-validator" for NGC). + // Empty means probe without a specific scope, which works for public + // registries (quay.io, Docker Hub public images) and GHCR anonymous access. + RepoHint string + // Critical marks registries whose credential failure should be a hard error + // rather than a warning. NGC (nvcr.io) is always critical; customer-supplied + // extras default to non-critical. + Critical bool +} + +// EnumerateRegistries builds the deduplicated list of registries to credential- +// check from the image ref, cert-manager (quay.io), the stack values file +// (global.image.registry), and operator-supplied extras. +func EnumerateRegistries(imageRef, stackValuesFile string, extras []string) []RegistryEntry { + seen := make(map[string]bool) + var out []RegistryEntry + + add := func(registry string, critical bool) { + registry = strings.TrimSpace(registry) + if registry == "" || seen[registry] { + return + } + seen[registry] = true + out = append(out, RegistryEntry{Registry: registry, Critical: critical}) + } + + // Source 1: base registry from the configured validator image. + // Carry the repo path as a scope hint so the token exchange uses the + // operator's actual org rather than a fake one — NGC returns 403 for + // orgs the API key cannot access, even if the key itself is valid. + if reg, repo, _, ok := parseImageRef(imageRef); ok && reg != "" { + seen[reg] = true + out = append(out, RegistryEntry{Registry: reg, RepoHint: repo, Critical: true}) + } + + // Source 2: read global.image.registry from the environment values file. + // This catches cases where the operator points at a custom NGC org or a + // staging environment that differs from the validator image's registry. + if stackValuesFile != "" { + if reg := readGlobalImageRegistry(stackValuesFile); reg != "" { + // If it's an NGC registry, mark critical; customer mirrors are non-critical. + add(reg, isNGCRegistry(reg)) + } + } + + // Source 3: cert-manager's well-known exception (quay.io/jetstack). + // cert-manager ignores global.image.registry and always pulls from quay.io. + add(certManagerRegistry, false) + + // Source 4: operator-supplied extras (--cluster-validator-registries). + for _, e := range extras { + host, _ := parseRegistryHostPort(e) + if host != "" { + add(host, false) + } + } + + return out +} + +// readGlobalImageRegistry reads the global.image.registry key from an +// environment values YAML file (e.g. environments/local.yaml). Returns "" +// on any error so the caller can safely ignore missing or malformed files. +func readGlobalImageRegistry(path string) string { + data, err := os.ReadFile(path) + if err != nil { + return "" + } + var vals struct { + Global struct { + Image struct { + Registry string `json:"registry" yaml:"registry"` + } `json:"image" yaml:"image"` + } `json:"global" yaml:"global"` + } + if err := yaml.Unmarshal(data, &vals); err != nil { + return "" + } + return strings.TrimSpace(vals.Global.Image.Registry) +} diff --git a/src/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go b/src/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go new file mode 100644 index 000000000..2c9bfcb66 --- /dev/null +++ b/src/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go @@ -0,0 +1,286 @@ +/* +SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. +SPDX-License-Identifier: Apache-2.0 + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package selfhosted + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "os" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// -- probeRegistryCredential -- + +func TestProbeRegistryCredential_PublicRegistry(t *testing.T) { + // Registry returns 200 on /v2/ → public, no credentials needed. + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + // Replace the default transport with the test server's transport so TLS + // validation passes against the self-signed cert. + origTransport := http.DefaultTransport + http.DefaultTransport = srv.Client().Transport + t.Cleanup(func() { http.DefaultTransport = origTransport }) + + // Use the test server's host as the registry. + host := strings.TrimPrefix(srv.URL, "https://") + err := probeRegistryCredential(context.Background(), host, "") + assert.NoError(t, err, "public registry (200 on /v2/) must not return an error") +} + +func TestProbeRegistryCredential_AuthSucceeds(t *testing.T) { + // Registry: /v2/ returns 401 with WWW-Authenticate; token endpoint returns a token. + tokenSrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Token endpoint always succeeds. + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"token":"test-token-abc"}`)) + })) + defer tokenSrv.Close() + + registrySrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Authorization") == "Bearer test-token-abc" { + w.WriteHeader(http.StatusOK) + return + } + // Point the client at our token server. + realm := tokenSrv.URL + "/token" + w.Header().Set("Www-Authenticate", + `Bearer realm="`+realm+`",service="test-registry",scope="repository:test:pull"`) + w.WriteHeader(http.StatusUnauthorized) + })) + defer registrySrv.Close() + + transport := registrySrv.Client().Transport + origTransport := http.DefaultTransport + http.DefaultTransport = transport + t.Cleanup(func() { http.DefaultTransport = origTransport }) + + host := strings.TrimPrefix(registrySrv.URL, "https://") + err := probeRegistryCredential(context.Background(), host, "") + assert.NoError(t, err, "successful token exchange must return nil") +} + +func TestProbeRegistryCredential_AuthFails(t *testing.T) { + // Registry returns 401 but the token endpoint returns 401 too → bad credentials. + tokenSrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + })) + defer tokenSrv.Close() + + registrySrv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + realm := tokenSrv.URL + "/token" + w.Header().Set("Www-Authenticate", + `Bearer realm="`+realm+`",service="test-registry"`) + w.WriteHeader(http.StatusUnauthorized) + })) + defer registrySrv.Close() + + transport := registrySrv.Client().Transport + origTransport := http.DefaultTransport + http.DefaultTransport = transport + t.Cleanup(func() { http.DefaultTransport = origTransport }) + + host := strings.TrimPrefix(registrySrv.URL, "https://") + err := probeRegistryCredential(context.Background(), host, "") + assert.Error(t, err, "failed token exchange must return an error") +} + +func TestProbeRegistryCredential_ECRSkipped(t *testing.T) { + // ECR registries must get a clear "use AWS CLI" message rather than a + // confusing Bearer token failure. + err := probeRegistryCredential(context.Background(), + "123456789.dkr.ecr.us-east-1.amazonaws.com", "") + require.Error(t, err) + assert.Contains(t, err.Error(), "ECR", "ECR registries must produce a clear diagnostic") + assert.Contains(t, err.Error(), "AWS", "message must mention AWS") +} + +// -- EnumerateRegistries -- + +func TestEnumerateRegistries_FromImageRef(t *testing.T) { + entries := EnumerateRegistries("nvcr.io/nvidia/nvcf-byoc/cluster-validator:3.1.0", "", nil) + require.NotEmpty(t, entries) + + found := false + for _, e := range entries { + if e.Registry == "nvcr.io" { + assert.True(t, e.Critical, "nvcr.io must be marked critical") + found = true + } + } + assert.True(t, found, "nvcr.io must appear in the enumerated registries") +} + +func TestEnumerateRegistries_RepoHintFromImageRef(t *testing.T) { + // The RepoHint must be the repo path from the image ref so the token + // exchange uses the operator's actual org, not a fake one. + entries := EnumerateRegistries("nvcr.io/nvidia/nvcf-byoc/cluster-validator:3.1.0", "", nil) + for _, e := range entries { + if e.Registry == "nvcr.io" { + assert.Equal(t, "nvidia/nvcf-byoc/cluster-validator", e.RepoHint, + "RepoHint must carry the actual repo path for correct NGC token scope") + return + } + } + t.Fatal("nvcr.io not found in entries") +} + +func TestEnumerateRegistries_AlwaysIncludesCertManager(t *testing.T) { + // cert-manager always uses quay.io — it must appear even when not in extras. + entries := EnumerateRegistries("nvcr.io/some/image:1.0", "", nil) + found := false + for _, e := range entries { + if e.Registry == "quay.io" { + assert.False(t, e.Critical, "quay.io must be non-critical") + found = true + } + } + assert.True(t, found, "quay.io must always be included for cert-manager") +} + +func TestEnumerateRegistries_ExtrasAppended(t *testing.T) { + entries := EnumerateRegistries("nvcr.io/some/image:1.0", "", + []string{"harbor.company.internal:443", "ghcr.io:443"}) + + registries := make(map[string]bool, len(entries)) + for _, e := range entries { + registries[e.Registry] = true + } + assert.True(t, registries["harbor.company.internal"], "extra registry must be added") + assert.True(t, registries["ghcr.io"], "extra registry must be added") +} + +func TestEnumerateRegistries_NoDuplicates(t *testing.T) { + // Pass nvcr.io both as the image registry and as an extra — must not dedup. + entries := EnumerateRegistries("nvcr.io/some/image:1.0", "", + []string{"nvcr.io"}) + + count := 0 + for _, e := range entries { + if e.Registry == "nvcr.io" { + count++ + } + } + assert.Equal(t, 1, count, "nvcr.io must appear exactly once even when listed twice") +} + +func TestEnumerateRegistries_StackValuesFile(t *testing.T) { + // Write a minimal environments/local.yaml to a temp dir. + dir := t.TempDir() + valuesPath := dir + "/local.yaml" + require.NoError(t, writeFile(valuesPath, []byte(` +global: + image: + registry: stg.nvcr.io +`))) + + entries := EnumerateRegistries("nvcr.io/some/image:1.0", valuesPath, nil) + found := false + for _, e := range entries { + if e.Registry == "stg.nvcr.io" { + assert.True(t, e.Critical, "NGC staging registry must be critical") + found = true + } + } + assert.True(t, found, "registry from values file must be included") +} + +func TestReadGlobalImageRegistry_ValidFile(t *testing.T) { + dir := t.TempDir() + path := dir + "/values.yaml" + require.NoError(t, writeFile(path, []byte(` +global: + image: + registry: nvcr.io + repository: nvidia/nvcf-byoc +`))) + got := readGlobalImageRegistry(path) + assert.Equal(t, "nvcr.io", got) +} + +func TestReadGlobalImageRegistry_MissingFile(t *testing.T) { + got := readGlobalImageRegistry("/nonexistent/path/values.yaml") + assert.Empty(t, got, "missing file must return empty string, not error") +} + +func TestReadGlobalImageRegistry_MissingKey(t *testing.T) { + dir := t.TempDir() + path := dir + "/values.yaml" + require.NoError(t, writeFile(path, []byte(`other: value`))) + got := readGlobalImageRegistry(path) + assert.Empty(t, got, "missing global.image.registry must return empty string") +} + +// -- isECRRegistry -- + +func TestIsECRRegistry(t *testing.T) { + assert.True(t, isECRRegistry("123456789012.dkr.ecr.us-east-1.amazonaws.com")) + assert.True(t, isECRRegistry("999999999999.dkr.ecr.eu-west-1.amazonaws.com")) + assert.False(t, isECRRegistry("nvcr.io")) + assert.False(t, isECRRegistry("ghcr.io")) + assert.False(t, isECRRegistry("harbor.company.internal")) + assert.False(t, isECRRegistry("s3.amazonaws.com")) // S3, not ECR +} + +// -- registryCredentialCheck binaryCheckSpec -- + +func TestRegistryCredentialCheck_PassWhenNoError(t *testing.T) { + checker := func(_ context.Context, reg, _ string) error { return nil } + spec := registryCredentialCheck(checker, RegistryEntry{Registry: "nvcr.io", Critical: true}) + r := spec.Run(context.Background()) + assert.True(t, r.Passed) + assert.Equal(t, "info", r.Severity) + assert.Contains(t, r.Message, "nvcr.io") +} + +func TestRegistryCredentialCheck_CriticalSeverityOnFailure(t *testing.T) { + checker := func(_ context.Context, reg, _ string) error { + return errorf("credentials rejected") + } + spec := registryCredentialCheck(checker, RegistryEntry{Registry: "nvcr.io", Critical: true}) + r := spec.Run(context.Background()) + assert.False(t, r.Passed) + assert.Equal(t, "error", r.Severity, "critical registry failure must be error severity") +} + +func TestRegistryCredentialCheck_WarningSeverityOnNonCriticalFailure(t *testing.T) { + checker := func(_ context.Context, reg, _ string) error { + return errorf("credentials rejected") + } + spec := registryCredentialCheck(checker, RegistryEntry{Registry: "quay.io", Critical: false}) + r := spec.Run(context.Background()) + assert.False(t, r.Passed) + assert.Equal(t, "warning", r.Severity, "non-critical registry failure must be warning severity") +} + +// helpers + +func errorf(msg string) error { return fmt.Errorf("%s", msg) } + +func writeFile(path string, data []byte) error { + return os.WriteFile(path, data, 0o644) +} diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go index f6750bbf3..4578184db 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go @@ -24,6 +24,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "path/filepath" "regexp" @@ -132,11 +133,11 @@ func fetchValidatorTags(ctx context.Context, registry, repo string) ([]string, e return doc.Tags, nil } -func fetchWithBearer(ctx context.Context, url, registry, repo string) ([]byte, error) { +func fetchWithBearer(ctx context.Context, rawURL, registry, repo string) ([]byte, error) { client := &http.Client{Timeout: validatorTagFetchTimeout} // First attempt without auth so anonymous-pullable registries work. - req, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil) if err != nil { return nil, err } @@ -152,16 +153,19 @@ func fetchWithBearer(ctx context.Context, url, registry, repo string) ([]byte, e resp.Body.Close() return nil, fmt.Errorf("registry returned %s", resp.Status) } + // Capture the auth challenge before closing. + wwwAuth := resp.Header.Get("Www-Authenticate") resp.Body.Close() - // Bearer-token exchange. Realm and scope come from the Www-Authenticate - // header; for NGC the realm is /proxy_auth and scope is repository::pull. - token, err := exchangeBearerToken(ctx, client, registry, repo) + // Generic OCI Bearer-token exchange: uses the realm/service/scope from + // the WWW-Authenticate header so any OCI-compliant registry works, not + // just NGC. Falls back to NGC's /proxy_auth when the header is absent. + token, err := exchangeBearerToken(ctx, client, registry, repo, wwwAuth) if err != nil { return nil, err } - req, err = http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + req, err = http.NewRequestWithContext(ctx, http.MethodGet, rawURL, nil) if err != nil { return nil, err } @@ -177,21 +181,100 @@ func fetchWithBearer(ctx context.Context, url, registry, repo string) ([]byte, e return io.ReadAll(resp.Body) } -// exchangeBearerToken does the NGC token exchange using credentials -// resolved from ~/.docker/config.json or NGC env vars. -// -// NGC-specific: this constructs the token URL using NGC's /proxy_auth -// realm rather than parsing the Www-Authenticate header from the 401 -// response (the generic OAuth2 distribution flow). Non-NGC registries -// (GCR, ECR, GHCR, Harbor, etc.) will return a non-200 here and the -// caller falls back to using the configured image reference as-is. -// Acceptable for v1 since the validator image only ships from NGC. -func exchangeBearerToken(ctx context.Context, client *http.Client, registry, repo string) (string, error) { +// exchangeBearerToken implements the OCI Distribution Spec Bearer token flow, +// parsing realm/service/scope from the WWW-Authenticate header. Falls back to +// the NGC /proxy_auth endpoint when the header is absent or unparseable. +func exchangeBearerToken(ctx context.Context, client *http.Client, registry, repo, wwwAuthenticate string) (string, error) { + realm, service, scope := parseWWWAuthenticate(wwwAuthenticate) + + if realm == "" { + // No parseable WWW-Authenticate — use NGC's /proxy_auth as fallback. + return exchangeNGCBearerToken(ctx, client, registry, repo) + } + + // Build the token endpoint URL with service and scope query parameters. + u, err := url.Parse(realm) + if err != nil { + return exchangeNGCBearerToken(ctx, client, registry, repo) + } + q := u.Query() + if service != "" { + q.Set("service", service) + } + // Use scope from the WWW-Authenticate header when present. + // When scope is empty and a repo is provided, synthesize the standard + // pull scope. When neither is present (credential probe, no specific + // repo needed), omit scope entirely — most registries issue a valid + // token and the absence of a resource scope avoids org-level 403s for + // non-existent repositories. + if scope == "" && repo != "" { + scope = "repository:" + repo + ":pull" + } + if scope != "" { + q.Set("scope", scope) + } + u.RawQuery = q.Encode() + + 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) + } + + resp, err := client.Do(req) + if err != nil { + return "", err + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + // WWW-Authenticate realm failed — try NGC's /proxy_auth as last resort + // for registries that implement both endpoints (e.g. staging NGC envs). + if isNGCRegistry(registry) { + return exchangeNGCBearerToken(ctx, client, registry, repo) + } + return "", fmt.Errorf("token exchange at %s returned %s", realm, resp.Status) + } + + // Both "token" (OCI spec) and "access_token" (Docker Hub variant) are valid. + var doc struct { + Token string `json:"token"` + AccessToken string `json:"access_token"` + } + if err := json.NewDecoder(resp.Body).Decode(&doc); err != nil { + return "", fmt.Errorf("decode token response: %w", err) + } + tok := doc.Token + if tok == "" { + tok = doc.AccessToken + } + if tok == "" { + return "", fmt.Errorf("empty token in response from %s", realm) + } + return tok, nil +} + +// exchangeNGCBearerToken is the NGC-specific /proxy_auth token exchange, +// kept as a named fallback for when the standard OCI flow cannot be used. +func exchangeNGCBearerToken(ctx context.Context, client *http.Client, registry, repo string) (string, error) { user, pass, ok := ngcCredentials(registry) if !ok { - return "", fmt.Errorf("no NGC credentials for %s", registry) + return "", fmt.Errorf("no credentials for %s", registry) + } + // Build scope: use actual repo when provided; omit when empty so the NGC + // /proxy_auth endpoint validates the key without org-scoped access checks. + scope := "" + if repo != "" { + scope = "repository:" + repo + ":pull" } - tokenURL := fmt.Sprintf("https://%s/proxy_auth?service=%s&scope=repository:%s:pull", registry, registry, repo) + tokenURL := fmt.Sprintf("https://%s/proxy_auth?service=%s&scope=%s", + registry, registry, scope) req, err := http.NewRequestWithContext(ctx, http.MethodGet, tokenURL, nil) if err != nil { return "", err @@ -203,7 +286,7 @@ func exchangeBearerToken(ctx context.Context, client *http.Client, registry, rep } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return "", fmt.Errorf("token exchange returned %s", resp.Status) + return "", fmt.Errorf("NGC token exchange returned %s", resp.Status) } var doc struct { Token string `json:"token"` @@ -212,11 +295,88 @@ func exchangeBearerToken(ctx context.Context, client *http.Client, registry, rep return "", err } if doc.Token == "" { - return "", fmt.Errorf("empty token in response") + return "", fmt.Errorf("empty token in NGC response") } return doc.Token, nil } +// parseWWWAuthenticate extracts realm, service, and scope from a standard +// Bearer challenge header: +// +// Bearer realm="https://auth.example.com/token",service="reg.example.com",scope="repository:lib:pull" +// +// Returns empty strings when the header is absent, not a Bearer challenge, or +// cannot be parsed. The parser handles quoted values that might contain commas. +func parseWWWAuthenticate(header string) (realm, service, scope string) { + const prefix = "Bearer " + if !strings.HasPrefix(header, prefix) { + return + } + params := strings.TrimSpace(header[len(prefix):]) + for len(params) > 0 { + // Find key= + eq := strings.IndexByte(params, '=') + if eq < 0 { + break + } + key := strings.TrimSpace(params[:eq]) + params = params[eq+1:] + + // Read value (quoted or unquoted) + var val string + if strings.HasPrefix(params, `"`) { + end := strings.IndexByte(params[1:], '"') + if end < 0 { + break + } + val = params[1 : end+1] + params = strings.TrimPrefix(strings.TrimSpace(params[end+2:]), ",") + } else { + comma := strings.IndexByte(params, ',') + if comma < 0 { + val = params + params = "" + } else { + val = params[:comma] + params = params[comma+1:] + } + } + + switch key { + case "realm": + realm = val + case "service": + service = val + case "scope": + scope = val + } + } + return +} + +// isNGCRegistry returns true when the registry is hosted on an NVIDIA / NGC +// domain, where the /proxy_auth fallback applies. +func isNGCRegistry(registry string) bool { + return strings.HasSuffix(registry, "nvcr.io") || + strings.Contains(registry, "nvidia.com") || + strings.Contains(registry, "ngc.nvidia") +} + +// credentialsForRegistry resolves (username, password) for any registry. +// Checks ~/.docker/config.json first, then falls back to NGC_API_KEY only +// for NGC-domain registries to avoid sending NGC creds to unrelated registries. +func credentialsForRegistry(registry string) (string, string, bool) { + if u, p, ok := credsFromDockerConfig(registry); ok { + return u, p, true + } + if isNGCRegistry(registry) { + if key := firstNonEmptyEnv(ngcAPIKeyEnvNames...); key != "" { + return "$oauthtoken", key, true + } + } + return "", "", false +} + // ngcCredentials resolves (username, password) for an NGC-hosted registry. // Checks ~/.docker/config.json first; falls back to NGC_API_KEY env vars // with the literal "$oauthtoken" sentinel username NGC expects. diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go index 0bd57306d..b7c58c897 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go @@ -252,3 +252,55 @@ func TestNGCCredentials_EnvFallback(t *testing.T) { assert.Equal(t, "$oauthtoken", user) assert.Equal(t, "from-env", pass) } + +// -- parseWWWAuthenticate -- + +func TestParseWWWAuthenticate_Standard(t *testing.T) { + header := `Bearer realm="https://auth.docker.io/token",service="registry-1.docker.io",scope="repository:library/ubuntu:pull"` + realm, service, scope := parseWWWAuthenticate(header) + assert.Equal(t, "https://auth.docker.io/token", realm) + assert.Equal(t, "registry-1.docker.io", service) + assert.Equal(t, "repository:library/ubuntu:pull", scope) +} + +func TestParseWWWAuthenticate_GHCR(t *testing.T) { + header := `Bearer realm="https://ghcr.io/token",service="ghcr.io",scope="repository:owner/image:pull"` + realm, service, scope := parseWWWAuthenticate(header) + assert.Equal(t, "https://ghcr.io/token", realm) + assert.Equal(t, "ghcr.io", service) + assert.Equal(t, "repository:owner/image:pull", scope) +} + +func TestParseWWWAuthenticate_NoBearer(t *testing.T) { + // Basic auth challenge — should return empty strings. + realm, service, scope := parseWWWAuthenticate(`Basic realm="My Registry"`) + assert.Empty(t, realm) + assert.Empty(t, service) + assert.Empty(t, scope) +} + +func TestParseWWWAuthenticate_Empty(t *testing.T) { + realm, service, scope := parseWWWAuthenticate("") + assert.Empty(t, realm) + assert.Empty(t, service) + assert.Empty(t, scope) +} + +func TestParseWWWAuthenticate_RealmOnly(t *testing.T) { + // Some registries omit service/scope in the initial challenge. + realm, service, scope := parseWWWAuthenticate(`Bearer realm="https://example.com/auth"`) + assert.Equal(t, "https://example.com/auth", realm) + assert.Empty(t, service) + assert.Empty(t, scope) +} + +// -- isNGCRegistry -- + +func TestIsNGCRegistry(t *testing.T) { + assert.True(t, isNGCRegistry("nvcr.io")) + assert.True(t, isNGCRegistry("stg.nvcr.io")) + assert.True(t, isNGCRegistry("registry.nvidia.com")) + assert.False(t, isNGCRegistry("ghcr.io")) + assert.False(t, isNGCRegistry("quay.io")) + assert.False(t, isNGCRegistry("harbor.company.internal")) +} From 1deb005edbc47f7c95f5d218b0752cfa8c37df00 Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 12 Aug 2026 02:43:00 +0530 Subject: [PATCH 04/13] build(check): update BUILD.bazel for new selfhosted sources and deps --- src/clis/nvcf-cli/cmd/BUILD.bazel | 1 + src/clis/nvcf-cli/internal/selfhosted/BUILD.bazel | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/clis/nvcf-cli/cmd/BUILD.bazel b/src/clis/nvcf-cli/cmd/BUILD.bazel index 22671a821..d43245fb1 100644 --- a/src/clis/nvcf-cli/cmd/BUILD.bazel +++ b/src/clis/nvcf-cli/cmd/BUILD.bazel @@ -135,6 +135,7 @@ go_test( "//src/clis/nvcf-cli/internal/selfhosted", "//src/clis/nvcf-cli/internal/selfhosted/auth", "//src/clis/nvcf-cli/internal/selfhosted/controlplaneprofile", + "//src/clis/nvcf-cli/internal/selfhosted/kubectx", "//src/clis/nvcf-cli/internal/selfhosted/progress", "//src/clis/nvcf-cli/internal/selfhosted/reachability", "//src/clis/nvcf-cli/internal/selfhosted/teardown", diff --git a/src/clis/nvcf-cli/internal/selfhosted/BUILD.bazel b/src/clis/nvcf-cli/internal/selfhosted/BUILD.bazel index 7737a9a8b..1a508d967 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/BUILD.bazel +++ b/src/clis/nvcf-cli/internal/selfhosted/BUILD.bazel @@ -26,8 +26,10 @@ go_library( "preflight.go", "pullsecret.go", "register.go", + "registry_cred.go", "render.go", "stack.go", + "stale_namespace.go", "validatortag.go", ], importpath = "nvcf-cli/internal/selfhosted", @@ -46,6 +48,7 @@ go_library( "@io_k8s_client_go//kubernetes", "@io_k8s_client_go//rest", "@io_k8s_client_go//tools/clientcmd", + "@io_k8s_sigs_yaml//:yaml", "@org_golang_x_sync//errgroup", ], ) @@ -60,8 +63,10 @@ go_test( "preflight_test.go", "pullsecret_test.go", "register_test.go", + "registry_cred_test.go", "render_test.go", "stack_test.go", + "stale_namespace_test.go", "validatortag_test.go", ], embed = [":selfhosted"], From 4d7c477f1893685169362564d5ad5af58110348f Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 12 Aug 2026 15:16:51 +0530 Subject: [PATCH 05/13] fix(check): address code-review findings in cluster-validator and preflight checks --- src/clis/nvcf-cli/cmd/self_hosted_check.go | 13 +++--- .../nvcf-cli/cmd/self_hosted_check_test.go | 40 ++++++++++++++++++- .../internal/selfhosted/clustervalidator.go | 10 ++++- .../selfhosted/clustervalidator_test.go | 10 +++++ .../nvcf-cli/internal/selfhosted/preflight.go | 35 +++++++++++----- .../internal/selfhosted/registry_cred.go | 31 ++++++++++---- .../internal/selfhosted/registry_cred_test.go | 14 +++---- .../internal/selfhosted/stale_namespace.go | 5 +++ .../selfhosted/stale_namespace_test.go | 5 ++- .../internal/selfhosted/validatortag.go | 11 ++++- 10 files changed, 137 insertions(+), 37 deletions(-) diff --git a/src/clis/nvcf-cli/cmd/self_hosted_check.go b/src/clis/nvcf-cli/cmd/self_hosted_check.go index 2498e8340..deffda153 100644 --- a/src/clis/nvcf-cli/cmd/self_hosted_check.go +++ b/src/clis/nvcf-cli/cmd/self_hosted_check.go @@ -152,13 +152,16 @@ func runSelfHostedCheck(c *cobra.Command, _ []string) error { } clusterValidatorWillRun := anyValidatorIsTargeted && clusterValidatorImage != "" - // The cluster-validator Job's internal budget is 5m - // (selfhosted.clusterValidatorTimeout). The outer ctx must be at least - // that plus headroom for RBAC bootstrap + log fetch, otherwise vctx - // derives from a shorter ceiling and silently truncates the wait. outerTimeout := 2 * time.Minute if clusterValidatorWillRun { - outerTimeout = 6 * time.Minute + // Each validator Job has a 5m internal budget. ModeSingle runs both + // validators sequentially (two 5m runs); ModeSplit runs them in + // parallel so one 6m ceiling covers both. + if mode == kubectx.ModeSingle && controlPlaneIsTargeted(mode) && computePlaneIsTargeted(mode) { + outerTimeout = 12 * time.Minute + } else { + outerTimeout = 6 * time.Minute + } } // --wait polls for the declared duration. The outer ctx has to outlive // the last iteration, so add waitDur on top of a single iteration's diff --git a/src/clis/nvcf-cli/cmd/self_hosted_check_test.go b/src/clis/nvcf-cli/cmd/self_hosted_check_test.go index 218bdca80..53bc744c3 100644 --- a/src/clis/nvcf-cli/cmd/self_hosted_check_test.go +++ b/src/clis/nvcf-cli/cmd/self_hosted_check_test.go @@ -384,6 +384,40 @@ func TestComputePlaneIsTargeted(t *testing.T) { } } +func TestControlPlaneIsTargeted(t *testing.T) { + t.Cleanup(func() { + checkPre = false + checkControlPlane = false + checkAll = false + }) + + tests := []struct { + name string + pre bool + cp bool + all bool + mode kubectx.Mode + want bool + }{ + {"--control-plane single", false, true, false, kubectx.ModeSingle, true}, + {"--control-plane split", false, true, false, kubectx.ModeSplit, true}, + {"--all single", false, false, true, kubectx.ModeSingle, true}, + {"--all split", false, false, true, kubectx.ModeSplit, true}, + {"--pre single -- implicit control plane", true, false, false, kubectx.ModeSingle, true}, + {"--pre split -- must not target control plane", true, false, false, kubectx.ModeSplit, false}, + {"--compute-plane only", false, false, false, kubectx.ModeSingle, false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + checkPre = tt.pre + checkControlPlane = tt.cp + checkAll = tt.all + assert.Equal(t, tt.want, controlPlaneIsTargeted(tt.mode)) + }) + } +} + // TestCheck_ComputePlaneFlagRunsChecks verifies that --compute-plane alone // produces compute-plane-cluster category events. Before the gating fix this // flag was a complete no-op and produced no check events at all. @@ -398,7 +432,8 @@ func TestCheck_ComputePlaneFlagRunsChecks(t *testing.T) { rootCmd.SetErr(&stderr) rootCmd.SetOut(&bytes.Buffer{}) - rootCmd.SetArgs([]string{"self-hosted", "check", "--compute-plane", "--json"}) + t.Setenv("NVCF_CLI_SELFHOSTED_SKIP_INOTIFY", "1") + rootCmd.SetArgs([]string{"self-hosted", "check", "--compute-plane", "--skip-cluster-validation", "--json"}) _ = rootCmd.Execute() lines := parseJSONLLines(t, stderr.String()) @@ -429,7 +464,8 @@ func TestCheck_ControlPlaneFlagRunsChecks(t *testing.T) { rootCmd.SetErr(&stderr) rootCmd.SetOut(&bytes.Buffer{}) - rootCmd.SetArgs([]string{"self-hosted", "check", "--control-plane", "--json"}) + t.Setenv("NVCF_CLI_SELFHOSTED_SKIP_INOTIFY", "1") + rootCmd.SetArgs([]string{"self-hosted", "check", "--control-plane", "--skip-cluster-validation", "--json"}) _ = rootCmd.Execute() lines := parseJSONLLines(t, stderr.String()) diff --git a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go index a9e1c6832..c779eac99 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go +++ b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go @@ -142,10 +142,13 @@ func runClusterValidator(ctx context.Context, client kubernetes.Interface, image // checks. Best-effort: a failure here is logged but does not abort the // run — the validator gracefully skips configurable checks when the // ConfigMap is absent. + var configNote string 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: continue without the ConfigMap; the validator skips + // configurable reachability and enforcement checks silently unless + // we surface this note in the transcript. + configNote = fmt.Sprintf("note: validator config not applied (%v); reachability checks may be skipped", err) } } @@ -166,6 +169,9 @@ func runClusterValidator(ctx context.Context, client kubernetes.Interface, image defer logCancel() rawLogs, _ := fetchClusterValidatorLogs(logCtx, client, jobName) cleaned := cleanValidatorOutput(rawLogs) + if configNote != "" { + cleaned = configNote + "\n" + cleaned + } if waitErr != nil { return ClusterValidatorResult{ diff --git a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go index 95051c402..44f487581 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go @@ -528,6 +528,16 @@ func TestBuildClusterValidatorJobShape(t *testing.T) { "preflight invocation must tag the validator so it does not attempt the metrics ConfigMap write") } +func TestBuildClusterValidatorJobShape_ValidatorRoleInEnv(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 must carry the role to the validator binary") +} + func TestBuildClusterValidatorJobShape_WithPullSecret(t *testing.T) { job := buildClusterValidatorJob("test-job", "img:1", "nvcr-pull-secret", "", false) require.Len(t, job.Spec.Template.Spec.ImagePullSecrets, 1) diff --git a/src/clis/nvcf-cli/internal/selfhosted/preflight.go b/src/clis/nvcf-cli/internal/selfhosted/preflight.go index 78b061410..89e053256 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/preflight.go +++ b/src/clis/nvcf-cli/internal/selfhosted/preflight.go @@ -346,9 +346,11 @@ func buildCategories(cfg PreflightConfig, role Role, rc RoleConfig) []categorySp } // 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 { + // contact needed. Gated on role != RoleComputePlane so the category is + // emitted at most once per invocation: in ModeSingle RunPreflightForRole + // is called for RoleControlPlane then RoleComputePlane sequentially; the + // gate ensures no duplicate check_started/check_completed events. + if role != RoleComputePlane && len(cfg.Registries) > 0 && cfg.RegistryChecker != nil { out = append(out, buildRegistryCredentialCategory(cfg)) } @@ -643,7 +645,7 @@ func registryCredentialCheck(checker RegistryCredentialChecker, entry RegistryEn ID: id, Severity: severity, } - if err := checker(ctx, entry.Registry, entry.RepoHint); err != nil { + if err := checker(ctx, entry.Registry, entry.RepoHint, entry.Critical); err != nil { r.Message = entry.Registry + ": " + err.Error() r.Err = err return r @@ -679,15 +681,28 @@ func staleNamespaceCheck(prober StaleNamespaceProber, kubeContext string, namesp return r } parts := make([]string, 0, len(stale)) - names := make([]string, 0, len(stale)) + var terminating, emptyShell []string for _, ns := range stale { parts = append(parts, ns.Name+" ("+ns.Reason+")") - names = append(names, ns.Name) + if ns.Reason == "stuck Terminating" { + terminating = append(terminating, ns.Name) + } else { + emptyShell = append(emptyShell, ns.Name) + } + } + var hints []string + if len(terminating) > 0 { + hints = append(hints, + fmt.Sprintf("remove finalizers on stuck namespaces: kubectl patch namespace %s -p '{\"spec\":{\"finalizers\":[]}}' --type=merge", + strings.Join(terminating, " "))) + } + if len(emptyShell) > 0 { + hints = append(hints, + fmt.Sprintf("inspect and delete empty namespaces: kubectl delete namespace %s", + strings.Join(emptyShell, " "))) } - r.Message = fmt.Sprintf( - "%d stale namespace(s) detected: %s — remove with: kubectl delete namespace %s --force --grace-period=0", - len(stale), strings.Join(parts, ", "), strings.Join(names, " "), - ) + r.Message = fmt.Sprintf("%d stale namespace(s) detected: %s. To resolve: %s", + len(stale), strings.Join(parts, ", "), strings.Join(hints, "; ")) return r }, } diff --git a/src/clis/nvcf-cli/internal/selfhosted/registry_cred.go b/src/clis/nvcf-cli/internal/selfhosted/registry_cred.go index 0167a9790..be51732c3 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/registry_cred.go +++ b/src/clis/nvcf-cli/internal/selfhosted/registry_cred.go @@ -39,8 +39,9 @@ const ( // RegistryCredentialChecker probes whether credentials are present and valid // for a registry. repoHint is the repository path used as the OAuth scope; // pass "" to probe without a specific scope (works for public registries). -// Returns nil on success, a descriptive error otherwise. -type RegistryCredentialChecker func(ctx context.Context, registry, repoHint string) error +// critical signals that anonymous success is insufficient; configured credentials +// must be present. Returns nil on success, a descriptive error otherwise. +type RegistryCredentialChecker func(ctx context.Context, registry, repoHint string, critical bool) error // NewRegistryCredentialChecker returns a production RegistryCredentialChecker // backed by real HTTP calls. @@ -50,8 +51,9 @@ func NewRegistryCredentialChecker() RegistryCredentialChecker { // probeRegistryCredential authenticates to registry using the OCI Bearer token // flow. repoHint is the OAuth scope repository path. ECR registries return a -// clear diagnostic instead of attempting the Bearer flow. -func probeRegistryCredential(ctx context.Context, registry, repoHint string) error { +// clear diagnostic instead of attempting the Bearer flow. When critical is true, +// anonymous token success is rejected: configured credentials must be present. +func probeRegistryCredential(ctx context.Context, registry, repoHint string, critical bool) error { if isECRRegistry(registry) { return fmt.Errorf("ECR registry detected — credential validation requires AWS CLI; " + "run 'aws ecr get-login-password' to verify manually") @@ -105,6 +107,14 @@ func probeRegistryCredential(ctx context.Context, registry, repoHint string) err } return fmt.Errorf("credentials rejected by %s: %w", registry, err) } + // For critical registries, a successful anonymous token is not enough: + // if the actual install pulls private images, anonymous access will fail. + if critical { + if _, _, hasCreds := credentialsForRegistry(registry); !hasCreds { + return fmt.Errorf("no credentials configured for %s "+ + "(add to ~/.docker/config.json or set NGC_API_KEY for NGC registries)", registry) + } + } return nil } @@ -170,11 +180,18 @@ func EnumerateRegistries(imageRef, stackValuesFile string, extras []string) []Re add(certManagerRegistry, false) // Source 4: operator-supplied extras (--cluster-validator-registries). + // Preserve non-443 ports in the registry string so probeRegistryCredential + // builds the correct https://host:port/v2/ URL. for _, e := range extras { - host, _ := parseRegistryHostPort(e) - if host != "" { - add(host, false) + host, port := parseRegistryHostPort(e) + if host == "" { + continue + } + reg := host + if port != 0 && port != 443 { + reg = fmt.Sprintf("%s:%d", host, port) } + add(reg, false) } return out diff --git a/src/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go b/src/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go index 2c9bfcb66..ca6f81104 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/registry_cred_test.go @@ -47,7 +47,7 @@ func TestProbeRegistryCredential_PublicRegistry(t *testing.T) { // Use the test server's host as the registry. host := strings.TrimPrefix(srv.URL, "https://") - err := probeRegistryCredential(context.Background(), host, "") + err := probeRegistryCredential(context.Background(), host, "", false) assert.NoError(t, err, "public registry (200 on /v2/) must not return an error") } @@ -80,7 +80,7 @@ func TestProbeRegistryCredential_AuthSucceeds(t *testing.T) { t.Cleanup(func() { http.DefaultTransport = origTransport }) host := strings.TrimPrefix(registrySrv.URL, "https://") - err := probeRegistryCredential(context.Background(), host, "") + err := probeRegistryCredential(context.Background(), host, "", false) assert.NoError(t, err, "successful token exchange must return nil") } @@ -105,7 +105,7 @@ func TestProbeRegistryCredential_AuthFails(t *testing.T) { t.Cleanup(func() { http.DefaultTransport = origTransport }) host := strings.TrimPrefix(registrySrv.URL, "https://") - err := probeRegistryCredential(context.Background(), host, "") + err := probeRegistryCredential(context.Background(), host, "", false) assert.Error(t, err, "failed token exchange must return an error") } @@ -113,7 +113,7 @@ func TestProbeRegistryCredential_ECRSkipped(t *testing.T) { // ECR registries must get a clear "use AWS CLI" message rather than a // confusing Bearer token failure. err := probeRegistryCredential(context.Background(), - "123456789.dkr.ecr.us-east-1.amazonaws.com", "") + "123456789.dkr.ecr.us-east-1.amazonaws.com", "", false) require.Error(t, err) assert.Contains(t, err.Error(), "ECR", "ECR registries must produce a clear diagnostic") assert.Contains(t, err.Error(), "AWS", "message must mention AWS") @@ -249,7 +249,7 @@ func TestIsECRRegistry(t *testing.T) { // -- registryCredentialCheck binaryCheckSpec -- func TestRegistryCredentialCheck_PassWhenNoError(t *testing.T) { - checker := func(_ context.Context, reg, _ string) error { return nil } + checker := func(_ context.Context, reg, _ string, _ bool) error { return nil } spec := registryCredentialCheck(checker, RegistryEntry{Registry: "nvcr.io", Critical: true}) r := spec.Run(context.Background()) assert.True(t, r.Passed) @@ -258,7 +258,7 @@ func TestRegistryCredentialCheck_PassWhenNoError(t *testing.T) { } func TestRegistryCredentialCheck_CriticalSeverityOnFailure(t *testing.T) { - checker := func(_ context.Context, reg, _ string) error { + checker := func(_ context.Context, reg, _ string, _ bool) error { return errorf("credentials rejected") } spec := registryCredentialCheck(checker, RegistryEntry{Registry: "nvcr.io", Critical: true}) @@ -268,7 +268,7 @@ func TestRegistryCredentialCheck_CriticalSeverityOnFailure(t *testing.T) { } func TestRegistryCredentialCheck_WarningSeverityOnNonCriticalFailure(t *testing.T) { - checker := func(_ context.Context, reg, _ string) error { + checker := func(_ context.Context, reg, _ string, _ bool) error { return errorf("credentials rejected") } spec := registryCredentialCheck(checker, RegistryEntry{Registry: "quay.io", Critical: false}) diff --git a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go index 8fe313175..2e6df32c5 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go +++ b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go @@ -100,8 +100,13 @@ func probeStaleNamespaces(ctx context.Context, client kubernetes.Interface, name continue } + // Limit to 1: only existence matters, not the full release history. + // Note: this assumes Helm's default secret storage driver. Clusters + // using HELM_DRIVER=configmap or sql will have no owner=helm secrets + // and may be incorrectly reported as empty shells. secrets, err := client.CoreV1().Secrets(name).List(ctx, metav1.ListOptions{ LabelSelector: "owner=helm", + Limit: 1, }) if err != nil { return stale, fmt.Errorf("list Helm secrets in %s: %w", name, err) diff --git a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go index 8acd45a2f..f86ba578c 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go @@ -171,7 +171,7 @@ func TestStaleNamespaceCheck_StaleIsError(t *testing.T) { assert.Equal(t, "error", r.Severity, "detected stale namespaces must use error severity so the exit code is non-zero") assert.Contains(t, r.Message, "nvcf") - assert.Contains(t, r.Message, "kubectl delete namespace") + assert.Contains(t, r.Message, "kubectl patch namespace nvcf") } func TestStaleNamespaceCheck_CleanPasses(t *testing.T) { @@ -194,5 +194,6 @@ func TestStaleNamespaceCheck_MessageNamesAllStaleNamespaces(t *testing.T) { r := staleNamespaceCheck(prober, "", []string{"nvcf", "api-keys"}).Run(context.Background()) assert.Contains(t, r.Message, "nvcf") assert.Contains(t, r.Message, "api-keys") - assert.Contains(t, r.Message, "kubectl delete namespace nvcf api-keys") + assert.Contains(t, r.Message, "kubectl patch namespace nvcf") + assert.Contains(t, r.Message, "kubectl delete namespace api-keys") } diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go index 4578184db..2c106c5d6 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go @@ -197,6 +197,13 @@ func exchangeBearerToken(ctx context.Context, client *http.Client, registry, rep if err != nil { return exchangeNGCBearerToken(ctx, client, registry, repo) } + // Reject non-HTTPS or relative realms before attaching credentials. + // The realm comes from a registry-controlled response header and must be + // an absolute HTTPS URL to prevent sending credentials over cleartext or + // to an unrelated host. + if u.Scheme != "https" || u.Host == "" { + return "", fmt.Errorf("refusing token exchange at insecure or relative realm %q for %s", realm, registry) + } q := u.Query() if service != "" { q.Set("service", service) @@ -334,10 +341,10 @@ func parseWWWAuthenticate(header string) (realm, service, scope string) { } 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:] } } From 6fa77330553bcc2723749a1672f836829ef40ab3 Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 12 Aug 2026 19:32:38 +0530 Subject: [PATCH 06/13] fix(check): replace parseRegistryHostPort with net.SplitHostPort and decouple registry checks from validator image --- src/clis/nvcf-cli/cmd/self_hosted_check.go | 10 ++++-- .../internal/selfhosted/clustervalidator.go | 35 ++++++++++--------- .../selfhosted/clustervalidator_test.go | 5 +++ 3 files changed, 32 insertions(+), 18 deletions(-) diff --git a/src/clis/nvcf-cli/cmd/self_hosted_check.go b/src/clis/nvcf-cli/cmd/self_hosted_check.go index deffda153..e9fb76a76 100644 --- a/src/clis/nvcf-cli/cmd/self_hosted_check.go +++ b/src/clis/nvcf-cli/cmd/self_hosted_check.go @@ -200,13 +200,19 @@ func runSelfHostedCheck(c *cobra.Command, _ []string) error { credEntries []selfhosted.RegistryEntry registryChecker selfhosted.RegistryCredentialChecker ) - if !localOnly && clusterValidatorImage != "" { + // Run credential checks whenever not local-only. The validator image is + // optional: EnumerateRegistries handles an empty image ref and still picks + // up global.image.registry from the stack values file and any + // --cluster-validator-registries extras independently of the image config. + if !localOnly { extraRegistries := viper.GetStringSlice("cluster_validator_registries") stackValuesFile := resolveStackValuesFile() credEntries = selfhosted.EnumerateRegistries( clusterValidatorImage, stackValuesFile, extraRegistries, ) - registryChecker = newRegistryCredentialCheckerForSelfHosted() + if len(credEntries) > 0 { + registryChecker = newRegistryCredentialCheckerForSelfHosted() + } } cfg := selfhosted.PreflightConfig{ diff --git a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go index c779eac99..ad825dfdf 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go +++ b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go @@ -21,7 +21,9 @@ import ( "context" "fmt" "io" + "net" "regexp" + "strconv" "strings" "time" @@ -389,28 +391,29 @@ func buildControlPlaneValidatorConfig(extraRegistries []string) string { "enforcement:", extra.String()+"enforcement:", 1) } -// parseRegistryHostPort splits a "host:port" string. Returns port 443 when -// no port is specified or when the port is not a valid number. +// parseRegistryHostPort splits a "host:port" string using net.SplitHostPort, +// which correctly handles IPv6 literals ([::1]:5000) and bare hostnames. +// Returns port 443 when no port is specified, the port is non-numeric, or +// the input has a trailing colon with no digit (e.g. "nvcr.io:"). 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 - } + h, p, err := net.SplitHostPort(s) + if err != nil { + // No port present (e.g. "nvcr.io") — return the input as-is. + return s, 443 + } + if p == "" { + // Trailing colon with no port digit (e.g. "nvcr.io:"). + return h, 443 + } + n, err := strconv.Atoi(p) + if err != nil || n <= 0 || n > 65535 { + return h, 443 } - return s, 443 + return h, n } // buildClusterValidatorJob creates the validator Job. PullIfNotPresent reuses diff --git a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go index 44f487581..ee70636e3 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go @@ -648,6 +648,11 @@ func TestParseRegistryHostPort(t *testing.T) { {"registry.example.com", "registry.example.com", 443}, // no port → 443 {"", "", 0}, // empty → skip {" ", "", 0}, // blank → skip + // IPv6: net.SplitHostPort handles bracketed literals correctly. + {"[::1]:5000", "::1", 5000}, + {"[2001:db8::1]:443", "2001:db8::1", 443}, + // Trailing colon with no digit → default to 443. + {"nvcr.io:", "nvcr.io", 443}, } for _, tt := range tests { t.Run(tt.in, func(t *testing.T) { From b6eaac136c92b1805b3f0110847baef3de50e0f7 Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 12 Aug 2026 20:07:23 +0530 Subject: [PATCH 07/13] fix(check): sweep ClusterRole and ClusterRoleBinding after validator Job completes --- .../internal/selfhosted/clustervalidator.go | 19 +++++++++++++++++++ .../selfhosted/clustervalidator_test.go | 3 ++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go index ad825dfdf..75c9d9a51 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go +++ b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go @@ -138,6 +138,14 @@ func runClusterValidator(ctx context.Context, client kubernetes.Interface, image if err := ensureClusterValidatorRBAC(vctx, client); err != nil { return ClusterValidatorResult{Err: fmt.Errorf("bootstrapping validator RBAC: %w", err)} } + // Remove the ClusterRole and ClusterRoleBinding after the Job finishes + // (when cleanup is enabled) to minimize the window where the elevated SA + // exists. Uses a fresh context so cleanup runs even when vctx is expired. + // When --no-cleanup is set, the operator expects to inspect the Job; we + // leave the RBAC in place so they can re-exec the pod without re-bootstrap. + if !noCleanup { + defer sweepClusterValidatorRBAC(context.Background(), client) + } // For the control-plane role, create a ConfigMap with reachability // endpoints and enforcement config so the validator runs its configurable @@ -276,6 +284,17 @@ func clusterValidatorLabels() map[string]string { } } +// sweepClusterValidatorRBAC removes the SA, ClusterRole, and ClusterRoleBinding +// created by ensureClusterValidatorRBAC. Called after Job completion (when +// --no-cleanup is not set) to close the window where the elevated ClusterRole +// exists. The next run recreates them via ensureClusterValidatorRBAC. +// Errors are swallowed: stale RBAC is preferable to failing the result. +func sweepClusterValidatorRBAC(ctx context.Context, client kubernetes.Interface) { + _ = client.RbacV1().ClusterRoleBindings().Delete(ctx, clusterValidatorName, metav1.DeleteOptions{}) + _ = client.RbacV1().ClusterRoles().Delete(ctx, clusterValidatorName, metav1.DeleteOptions{}) + _ = client.CoreV1().ServiceAccounts(clusterValidatorNamespace).Delete(ctx, clusterValidatorName, metav1.DeleteOptions{}) +} + // Errors are swallowed: a stale Job is preferable to blocking the new run. func sweepPriorClusterValidatorJobs(ctx context.Context, client kubernetes.Interface) { selector := fmt.Sprintf("app.kubernetes.io/name=%s,app.kubernetes.io/managed-by=nvcf-cli", clusterValidatorAppLabel) diff --git a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go index ee70636e3..0ecb7e602 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator_test.go @@ -264,7 +264,8 @@ func TestRunClusterValidator_RBACRefreshesClusterRoleRules(t *testing.T) { }) client.PrependReactor("list", "pods", podListReactor("")) - res := runClusterValidator(context.Background(), client, "test-image:1.0", "", false, "", nil) + // noCleanup=true so the ClusterRole is not swept before we inspect it. + res := runClusterValidator(context.Background(), client, "test-image:1.0", "", true, "", nil) require.NoError(t, res.Err) got, err := client.RbacV1().ClusterRoles().Get(context.Background(), clusterValidatorName, metav1.GetOptions{}) From 5c4d2dc5d784b68cc873d6f07f817fd330055492 Mon Sep 17 00:00:00 2001 From: rohithb Date: Mon, 17 Aug 2026 22:13:45 +0530 Subject: [PATCH 08/13] fix(nvcf-cli): add DaemonSet create/delete RBAC for node-to-node probe --- src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go index 75c9d9a51..66a409d14 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go +++ b/src/clis/nvcf-cli/internal/selfhosted/clustervalidator.go @@ -235,7 +235,10 @@ func ensureClusterValidatorRBAC(ctx context.Context, client kubernetes.Interface // 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"}}, + // Deployments/StatefulSets: list for Tier-1/Tier-2 HA readiness checks. + // DaemonSets: create/delete for the node-to-node DaemonSet probe; list to watch pod readiness. + {APIGroups: []string{"apps"}, Resources: []string{"deployments", "statefulsets"}, Verbs: []string{"get", "list"}}, + {APIGroups: []string{"apps"}, Resources: []string{"daemonsets"}, Verbs: []string{"get", "list", "create", "delete"}}, // 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"}}, From 81930891af3784168756f506a48b4a48514eb80b Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 19 Aug 2026 14:53:48 +0530 Subject: [PATCH 09/13] fix(nvcf-cli): address CodeRabbit review comments in validatortag and stale namespace isNGCRegistry: use dot-boundary host matching and strip port before comparing so evilnvcr.io and nvidia.com.invalid are rejected while nvcr.io:443 and stg.nvcr.io are correctly accepted. parseWWWAuthenticate: accept Bearer challenge schemes case-insensitively using strings.EqualFold after splitting scheme from parameters on whitespace (RFC 7235 requires case-insensitive scheme comparison). probeStaleNamespaces: fall back to listing owner=helm ConfigMaps when no owner=helm Secret exists, so clusters running HELM_DRIVER=configmap are not incorrectly reported as empty shells. stale_namespace_test.go: replace non-ASCII em dashes in comments with ASCII hyphens per repo style guidelines. --- .../internal/selfhosted/stale_namespace.go | 18 +++++-- .../selfhosted/stale_namespace_test.go | 34 ++++++++++--- .../internal/selfhosted/validatortag.go | 49 ++++++++++++++++--- .../internal/selfhosted/validatortag_test.go | 22 +++++++++ 4 files changed, 105 insertions(+), 18 deletions(-) diff --git a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go index 2e6df32c5..56a57526a 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go +++ b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go @@ -101,9 +101,9 @@ func probeStaleNamespaces(ctx context.Context, client kubernetes.Interface, name } // Limit to 1: only existence matters, not the full release history. - // Note: this assumes Helm's default secret storage driver. Clusters - // using HELM_DRIVER=configmap or sql will have no owner=helm secrets - // and may be incorrectly reported as empty shells. + // Check Secrets first (default Helm storage driver). If none exist, + // also check ConfigMaps to handle HELM_DRIVER=configmap clusters — + // both storage backends label their release objects with owner=helm. secrets, err := client.CoreV1().Secrets(name).List(ctx, metav1.ListOptions{ LabelSelector: "owner=helm", Limit: 1, @@ -111,7 +111,17 @@ func probeStaleNamespaces(ctx context.Context, client kubernetes.Interface, name if err != nil { return stale, fmt.Errorf("list Helm secrets in %s: %w", name, err) } - if len(secrets.Items) == 0 { + if len(secrets.Items) > 0 { + continue // healthy: active Helm release found via secret driver + } + cms, err := client.CoreV1().ConfigMaps(name).List(ctx, metav1.ListOptions{ + LabelSelector: "owner=helm", + Limit: 1, + }) + if err != nil { + return stale, fmt.Errorf("list Helm configmaps in %s: %w", name, err) + } + if len(cms.Items) == 0 { stale = append(stale, StaleNamespace{Name: name, Reason: "no Helm release"}) } } diff --git a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go index f86ba578c..e64700366 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace_test.go @@ -33,7 +33,7 @@ import ( // -- probeStaleNamespaces -- func TestProbeStaleNamespaces_AbsentIsHealthy(t *testing.T) { - // A namespace that doesn't exist is not stale — it has simply never been + // A namespace that doesn't exist is not stale - it has simply never been // created or was already fully deleted. client := fake.NewSimpleClientset() stale, err := probeStaleNamespaces(context.Background(), client, []string{"nvcf", "sis"}) @@ -101,11 +101,33 @@ func TestProbeStaleNamespaces_HealthyReleaseNotStale(t *testing.T) { assert.Empty(t, stale, "namespace with an active Helm release must not be stale") } +func TestProbeStaleNamespaces_HealthyReleaseConfigMapDriverNotStale(t *testing.T) { + // HELM_DRIVER=configmap stores release objects as ConfigMaps with owner=helm. + // A namespace with an owner=helm ConfigMap and no Helm Secret must not be + // reported as stale. + client := fake.NewSimpleClientset( + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{Name: "nvcf"}, + Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}, + }, + &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "nvcf.v1", + Namespace: "nvcf", + Labels: map[string]string{"owner": "helm", "name": "nvcf", "status": "deployed"}, + }, + }, + ) + stale, err := probeStaleNamespaces(context.Background(), client, []string{"nvcf"}) + require.NoError(t, err) + assert.Empty(t, stale, "namespace with an owner=helm ConfigMap (configmap driver) must not be stale") +} + func TestProbeStaleNamespaces_MixedNamespaces(t *testing.T) { // One absent, one healthy, one terminating, one empty shell. now := metav1.Now() client := fake.NewSimpleClientset( - // "sis" — healthy with a Helm release + // "sis" - healthy with a Helm release &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{Name: "sis"}, Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}, @@ -116,17 +138,17 @@ func TestProbeStaleNamespaces_MixedNamespaces(t *testing.T) { Labels: map[string]string{"owner": "helm"}, }, }, - // "nvcf" — stuck terminating + // "nvcf" - stuck terminating &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{Name: "nvcf", DeletionTimestamp: &now}, Status: corev1.NamespaceStatus{Phase: corev1.NamespaceTerminating}, }, - // "api-keys" — empty shell + // "api-keys" - empty shell &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{Name: "api-keys"}, Status: corev1.NamespaceStatus{Phase: corev1.NamespaceActive}, }, - // "cassandra-system" — absent (not present in fake) + // "cassandra-system" - absent (not present in fake) ) namespaces := []string{"cassandra-system", "sis", "nvcf", "api-keys"} @@ -148,7 +170,7 @@ func TestProbeStaleNamespaces_MixedNamespaces(t *testing.T) { func TestStaleNamespaceCheck_ProberErrorDegradestoWarning(t *testing.T) { // A prober that cannot contact the cluster must not fail the overall check - // at error severity — it would produce false failures on transient network + // at error severity - it would produce false failures on transient network // issues or misconfigured kubeconfigs. prober := func(_ context.Context, _ string, _ []string) ([]StaleNamespace, error) { return nil, fmt.Errorf("cluster unreachable") diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go index 2c106c5d6..b3eef3e0b 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go @@ -23,6 +23,7 @@ import ( "encoding/json" "fmt" "io" + "net" "net/http" "net/url" "os" @@ -315,11 +316,13 @@ func exchangeNGCBearerToken(ctx context.Context, client *http.Client, registry, // Returns empty strings when the header is absent, not a Bearer challenge, or // cannot be parsed. The parser handles quoted values that might contain commas. func parseWWWAuthenticate(header string) (realm, service, scope string) { - const prefix = "Bearer " - if !strings.HasPrefix(header, prefix) { + // Split scheme from parameters on the first whitespace. HTTP auth scheme + // names are case-insensitive (RFC 7235 s2.1), so compare with EqualFold. + idx := strings.IndexByte(header, ' ') + if idx < 0 || !strings.EqualFold(header[:idx], "Bearer") { return } - params := strings.TrimSpace(header[len(prefix):]) + params := strings.TrimSpace(header[idx+1:]) for len(params) > 0 { // Find key= eq := strings.IndexByte(params, '=') @@ -361,12 +364,42 @@ func parseWWWAuthenticate(header string) (realm, service, scope string) { return } -// isNGCRegistry returns true when the registry is hosted on an NVIDIA / NGC -// domain, where the /proxy_auth fallback applies. +// ngcApprovedHosts is the set of exact hostnames (without port) that are +// considered NGC-hosted. Dot-prefixed entries match any subdomain. +var ngcApprovedHosts = []string{ + "nvcr.io", + ".nvcr.io", + "nvidia.com", + ".nvidia.com", + "ngc.nvidia", + ".ngc.nvidia", +} + +// isNGCRegistry returns true when the registry host belongs to an NVIDIA / NGC +// domain. The check strips any port from the registry string before matching +// so that nvcr.io:443 is handled correctly, and uses dot-boundary matching to +// reject deceptive suffixes such as evilnvcr.io or nvidia.com.invalid. func isNGCRegistry(registry string) bool { - return strings.HasSuffix(registry, "nvcr.io") || - strings.Contains(registry, "nvidia.com") || - strings.Contains(registry, "ngc.nvidia") + host := registry + // Strip port if present (e.g. nvcr.io:5000 -> nvcr.io). + if h, _, err := net.SplitHostPort(registry); err == nil { + host = h + } + host = strings.ToLower(host) + for _, approved := range ngcApprovedHosts { + if strings.HasPrefix(approved, ".") { + // Subdomain match: host must end with ".suffix" or equal "suffix". + suffix := approved[1:] // strip the leading dot + if host == suffix || strings.HasSuffix(host, approved) { + return true + } + } else { + if host == approved { + return true + } + } + } + return false } // credentialsForRegistry resolves (username, password) for any registry. diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go index b7c58c897..387254f03 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go @@ -294,13 +294,35 @@ func TestParseWWWAuthenticate_RealmOnly(t *testing.T) { assert.Empty(t, scope) } +func TestParseWWWAuthenticate_CaseInsensitiveBearer(t *testing.T) { + // HTTP auth scheme names are case-insensitive (RFC 7235 s2.1). + for _, header := range []string{ + `bearer realm="https://auth.example.com/token",service="reg.example.com"`, + `BEARER realm="https://auth.example.com/token",service="reg.example.com"`, + `Bearer realm="https://auth.example.com/token",service="reg.example.com"`, + } { + realm, service, _ := parseWWWAuthenticate(header) + assert.Equal(t, "https://auth.example.com/token", realm, "header: %s", header) + assert.Equal(t, "reg.example.com", service, "header: %s", header) + } +} + // -- isNGCRegistry -- func TestIsNGCRegistry(t *testing.T) { + // Valid NGC registries. assert.True(t, isNGCRegistry("nvcr.io")) assert.True(t, isNGCRegistry("stg.nvcr.io")) assert.True(t, isNGCRegistry("registry.nvidia.com")) + assert.True(t, isNGCRegistry("nvcr.io:443"), "port must be stripped before matching") + + // Non-NGC registries must be rejected. assert.False(t, isNGCRegistry("ghcr.io")) assert.False(t, isNGCRegistry("quay.io")) assert.False(t, isNGCRegistry("harbor.company.internal")) + + // Deceptive suffixes must be rejected. + assert.False(t, isNGCRegistry("evilnvcr.io"), "suffix match without dot boundary must be rejected") + assert.False(t, isNGCRegistry("nvidia.com.invalid"), "deceptive TLD must be rejected") + assert.False(t, isNGCRegistry("fakenvidia.com"), "partial host match must be rejected") } From c3c4a41cb8a4505c262efe1c279e614afe9b8916 Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 19 Aug 2026 16:01:56 +0530 Subject: [PATCH 10/13] fix(nvcf-cli): restrict NGC token fallback to NGC registries and fix em dash --- .../nvcf-cli/internal/selfhosted/stale_namespace.go | 2 +- .../nvcf-cli/internal/selfhosted/validatortag.go | 5 +++++ .../internal/selfhosted/validatortag_test.go | 13 +++++++++++++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go index 56a57526a..8779b8e1a 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go +++ b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go @@ -102,7 +102,7 @@ func probeStaleNamespaces(ctx context.Context, client kubernetes.Interface, name // Limit to 1: only existence matters, not the full release history. // Check Secrets first (default Helm storage driver). If none exist, - // also check ConfigMaps to handle HELM_DRIVER=configmap clusters — + // also check ConfigMaps to handle HELM_DRIVER=configmap clusters; // both storage backends label their release objects with owner=helm. secrets, err := client.CoreV1().Secrets(name).List(ctx, metav1.ListOptions{ LabelSelector: "owner=helm", diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go index b3eef3e0b..5a234ef1f 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go @@ -270,7 +270,12 @@ func exchangeBearerToken(ctx context.Context, client *http.Client, registry, rep // exchangeNGCBearerToken is the NGC-specific /proxy_auth token exchange, // kept as a named fallback for when the standard OCI flow cannot be used. +// It always rejects non-NGC registries so NGC credentials are never sent +// to an unrelated /proxy_auth endpoint. func exchangeNGCBearerToken(ctx context.Context, client *http.Client, registry, repo string) (string, error) { + if !isNGCRegistry(registry) { + return "", fmt.Errorf("NGC token fallback not applicable for non-NGC registry %s", registry) + } user, pass, ok := ngcCredentials(registry) if !ok { return "", fmt.Errorf("no credentials for %s", registry) diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go index 387254f03..bcc49f8ca 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go @@ -20,6 +20,7 @@ package selfhosted import ( "context" "encoding/json" + "net/http" "os" "path/filepath" "testing" @@ -326,3 +327,15 @@ func TestIsNGCRegistry(t *testing.T) { assert.False(t, isNGCRegistry("nvidia.com.invalid"), "deceptive TLD must be rejected") assert.False(t, isNGCRegistry("fakenvidia.com"), "partial host match must be rejected") } + +// -- exchangeNGCBearerToken -- + +func TestExchangeNGCBearerToken_RejectsNonNGCRegistry(t *testing.T) { + // A non-NGC registry with an absent or malformed WWW-Authenticate header + // must not trigger the NGC /proxy_auth fallback. If it did, NGC credentials + // could be sent to an unrelated registry's /proxy_auth endpoint. + client := &http.Client{} + _, err := exchangeNGCBearerToken(context.Background(), client, "harbor.company.internal", "myrepo/image") + require.Error(t, err, "non-NGC registry must be rejected without issuing a request") + assert.Contains(t, err.Error(), "non-NGC registry") +} From de7999b9ead42bdbc2beeda926f628caf839bd44 Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 19 Aug 2026 16:47:05 +0530 Subject: [PATCH 11/13] fix(nvcf-cli): response body close, case-insensitive params, empty scope, spy transport test --- .../internal/selfhosted/validatortag.go | 24 ++++++++++------- .../internal/selfhosted/validatortag_test.go | 26 ++++++++++++++++--- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go index 5a234ef1f..f906dff16 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go @@ -240,15 +240,19 @@ func exchangeBearerToken(ctx context.Context, client *http.Client, registry, rep if err != nil { return "", err } - defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - // WWW-Authenticate realm failed — try NGC's /proxy_auth as last resort + // Close the body immediately before the fallback or the error return so + // the connection is not held open while the NGC /proxy_auth call runs. + status := resp.Status + resp.Body.Close() + // WWW-Authenticate realm failed; try NGC's /proxy_auth as last resort // for registries that implement both endpoints (e.g. staging NGC envs). if isNGCRegistry(registry) { return exchangeNGCBearerToken(ctx, client, registry, repo) } - return "", fmt.Errorf("token exchange at %s returned %s", realm, resp.Status) + return "", fmt.Errorf("token exchange at %s returned %s", realm, status) } + defer resp.Body.Close() // Both "token" (OCI spec) and "access_token" (Docker Hub variant) are valid. var doc struct { @@ -280,14 +284,14 @@ func exchangeNGCBearerToken(ctx context.Context, client *http.Client, registry, if !ok { return "", fmt.Errorf("no credentials for %s", registry) } - // Build scope: use actual repo when provided; omit when empty so the NGC - // /proxy_auth endpoint validates the key without org-scoped access checks. - scope := "" + // Build the query: use url.Values so the scope key is omitted entirely + // when repo is empty rather than sending scope= with an empty value. + // An empty scope validates the API key without org-scoped access checks. + q := url.Values{"service": {registry}} if repo != "" { - scope = "repository:" + repo + ":pull" + q.Set("scope", "repository:"+repo+":pull") } - tokenURL := fmt.Sprintf("https://%s/proxy_auth?service=%s&scope=%s", - registry, registry, scope) + tokenURL := "https://" + registry + "/proxy_auth?" + q.Encode() req, err := http.NewRequestWithContext(ctx, http.MethodGet, tokenURL, nil) if err != nil { return "", err @@ -357,7 +361,7 @@ func parseWWWAuthenticate(header string) (realm, service, scope string) { } } - switch key { + switch strings.ToLower(key) { case "realm": realm = val case "service": diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go index bcc49f8ca..e2d7809e5 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go @@ -295,6 +295,14 @@ func TestParseWWWAuthenticate_RealmOnly(t *testing.T) { assert.Empty(t, scope) } +func TestParseWWWAuthenticate_MixedCaseParams(t *testing.T) { + // Auth parameter names are parsed with mixed case in the wild. + realm, service, scope := parseWWWAuthenticate(`Bearer Realm="https://auth.example.com/token",Service="reg.example.com",Scope="repository:foo:pull"`) + assert.Equal(t, "https://auth.example.com/token", realm) + assert.Equal(t, "reg.example.com", service) + assert.Equal(t, "repository:foo:pull", scope) +} + func TestParseWWWAuthenticate_CaseInsensitiveBearer(t *testing.T) { // HTTP auth scheme names are case-insensitive (RFC 7235 s2.1). for _, header := range []string{ @@ -330,11 +338,21 @@ func TestIsNGCRegistry(t *testing.T) { // -- exchangeNGCBearerToken -- +// spyTransport is an http.RoundTripper that fails the test if called. +type spyTransport struct{ t *testing.T } + +func (s *spyTransport) RoundTrip(_ *http.Request) (*http.Response, error) { + s.t.Fatal("HTTP request must not be issued for non-NGC registry") + return nil, nil +} + func TestExchangeNGCBearerToken_RejectsNonNGCRegistry(t *testing.T) { - // A non-NGC registry with an absent or malformed WWW-Authenticate header - // must not trigger the NGC /proxy_auth fallback. If it did, NGC credentials - // could be sent to an unrelated registry's /proxy_auth endpoint. - client := &http.Client{} + // A non-NGC registry must be rejected before any HTTP request is made, + // even when NGC credentials are configured. The spy transport fails the + // test immediately if RoundTrip is called, ensuring the isNGCRegistry + // guard fires before any network activity. + t.Setenv("NGC_API_KEY", "test-key") // configure a credential so a missing guard would reach the transport + client := &http.Client{Transport: &spyTransport{t: t}} _, err := exchangeNGCBearerToken(context.Background(), client, "harbor.company.internal", "myrepo/image") require.Error(t, err, "non-NGC registry must be rejected without issuing a request") assert.Contains(t, err.Error(), "non-NGC registry") From a52685f46d7e9f3416277342b7753b95e8156c53 Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 19 Aug 2026 22:14:32 +0530 Subject: [PATCH 12/13] fix(nvcf-cli): authorize realm host before forwarding credentials and fix em dash --- .../internal/selfhosted/stale_namespace.go | 2 +- .../internal/selfhosted/validatortag.go | 22 +++++++++-- .../internal/selfhosted/validatortag_test.go | 38 +++++++++++++++++++ 3 files changed, 58 insertions(+), 4 deletions(-) diff --git a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go index 8779b8e1a..1f31403cf 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go +++ b/src/clis/nvcf-cli/internal/selfhosted/stale_namespace.go @@ -77,7 +77,7 @@ func NewStaleNamespaceProber() StaleNamespaceProber { // // A namespace is considered stale when either: // - its DeletionTimestamp is set or its phase is Terminating (finalizer -// deadlock — it will never complete without operator intervention), or +// deadlock; it will never complete without operator intervention), or // - it exists but holds no active Helm release (empty shell left by a partial // helm uninstall or a failed teardown that cleaned the release but not the // namespace). diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go index f906dff16..9064e4db0 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go @@ -199,12 +199,28 @@ func exchangeBearerToken(ctx context.Context, client *http.Client, registry, rep return exchangeNGCBearerToken(ctx, client, registry, repo) } // Reject non-HTTPS or relative realms before attaching credentials. - // The realm comes from a registry-controlled response header and must be - // an absolute HTTPS URL to prevent sending credentials over cleartext or - // to an unrelated host. if u.Scheme != "https" || u.Host == "" { return "", fmt.Errorf("refusing token exchange at insecure or relative realm %q for %s", realm, registry) } + // Authorize the realm host before forwarding credentials. The realm URL + // comes from a registry-controlled response header. Without this check, a + // malicious registry could return realm="https://attacker.com/token" and + // receive the operator's Docker credentials for the original registry. + // Allow the realm only when it matches the registry's own host, is a + // sub-domain of that host (e.g. auth.registry.example.com for registry.example.com), + // or is an NGC auth domain when the registry is NGC-hosted (NGC delegates + // token issuance to authn.nvidia.com and other nvidia.com sub-domains). + realmHost := strings.ToLower(u.Hostname()) + regHost := strings.ToLower(registry) + if h, _, err := net.SplitHostPort(registry); err == nil { + regHost = strings.ToLower(h) + } + realmOK := realmHost == regHost || + strings.HasSuffix(realmHost, "."+regHost) || + (isNGCRegistry(registry) && isNGCRegistry(realmHost)) + if !realmOK { + return "", fmt.Errorf("refusing to forward credentials to realm host %q; not authorized for registry %s", realmHost, registry) + } q := u.Query() if service != "" { q.Set("service", service) diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go index e2d7809e5..1d082ddc9 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go @@ -21,8 +21,10 @@ import ( "context" "encoding/json" "net/http" + "net/http/httptest" "os" "path/filepath" + "strings" "testing" "time" @@ -336,6 +338,37 @@ func TestIsNGCRegistry(t *testing.T) { assert.False(t, isNGCRegistry("fakenvidia.com"), "partial host match must be rejected") } +// -- exchangeBearerToken realm host authorization -- + +func TestExchangeBearerToken_RejectsAttackerRealm(t *testing.T) { + // A malicious registry returns a realm on an attacker-controlled host. + // The function must reject this without forwarding credentials. + spy := &spyTransport{t: t} + + // Set up a fake registry server that returns 401 with an attacker realm. + srv := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Www-Authenticate", `Bearer realm="https://attacker.example.com/token",service="harbor.company.internal"`) + w.WriteHeader(http.StatusUnauthorized) + })) + defer srv.Close() + + client := srv.Client() + // Replace the transport with the spy AFTER the TLS is set up; the spy + // wraps the original to preserve TLS but fails on any attacker call. + origTransport := client.Transport + client.Transport = roundTripperFunc(func(r *http.Request) (*http.Response, error) { + if r.Host == "attacker.example.com" || strings.Contains(r.URL.Host, "attacker") { + t.Fatalf("credentials must not be forwarded to attacker host: %s", r.URL) + } + return origTransport.RoundTrip(r) + }) + _ = spy + + _, err := exchangeBearerToken(context.Background(), client, "harbor.company.internal", "myrepo/image", `Bearer realm="https://attacker.example.com/token",service="harbor.company.internal"`) + require.Error(t, err) + assert.Contains(t, err.Error(), "not authorized for registry") +} + // -- exchangeNGCBearerToken -- // spyTransport is an http.RoundTripper that fails the test if called. @@ -346,6 +379,11 @@ func (s *spyTransport) RoundTrip(_ *http.Request) (*http.Response, error) { return nil, nil } +// roundTripperFunc adapts a function to the http.RoundTripper interface. +type roundTripperFunc func(*http.Request) (*http.Response, error) + +func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + func TestExchangeNGCBearerToken_RejectsNonNGCRegistry(t *testing.T) { // A non-NGC registry must be rejected before any HTTP request is made, // even when NGC credentials are configured. The spy transport fails the From 2218f34fe0cd99b7ab90d6c59361b56ac45636fe Mon Sep 17 00:00:00 2001 From: rohithb Date: Wed, 19 Aug 2026 22:39:13 +0530 Subject: [PATCH 13/13] fix(nvcf-cli): allow Docker Hub delegated auth realm in token exchange --- .../internal/selfhosted/validatortag.go | 15 ++++++++++++++- .../internal/selfhosted/validatortag_test.go | 17 +++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go index 9064e4db0..d2f9fbca9 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag.go @@ -217,7 +217,8 @@ func exchangeBearerToken(ctx context.Context, client *http.Client, registry, rep } realmOK := realmHost == regHost || strings.HasSuffix(realmHost, "."+regHost) || - (isNGCRegistry(registry) && isNGCRegistry(realmHost)) + (isNGCRegistry(registry) && isNGCRegistry(realmHost)) || + trustedRealmDelegations[regHost] == realmHost if !realmOK { return "", fmt.Errorf("refusing to forward credentials to realm host %q; not authorized for registry %s", realmHost, registry) } @@ -389,6 +390,18 @@ func parseWWWAuthenticate(header string) (realm, service, scope string) { return } +// trustedRealmDelegations maps a registry host to its authorized token host +// when the registry uses a separate host for token exchange. Only add entries +// here for registries with publicly documented auth architectures; this list +// extends the fail-closed realm validation and must not grow without a clear +// trust basis. +var trustedRealmDelegations = map[string]string{ + // Docker Hub documents this split explicitly: the pull host and the auth + // host are distinct (docs.docker.com/registry/spec/auth/token/). + "registry-1.docker.io": "auth.docker.io", + "docker.io": "auth.docker.io", +} + // ngcApprovedHosts is the set of exact hostnames (without port) that are // considered NGC-hosted. Dot-prefixed entries match any subdomain. var ngcApprovedHosts = []string{ diff --git a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go index 1d082ddc9..ce056a58b 100644 --- a/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go +++ b/src/clis/nvcf-cli/internal/selfhosted/validatortag_test.go @@ -22,6 +22,7 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "net/url" "os" "path/filepath" "strings" @@ -384,6 +385,22 @@ type roundTripperFunc func(*http.Request) (*http.Response, error) func (f roundTripperFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } +func TestExchangeBearerToken_DockerHubDelegatedRealm(t *testing.T) { + // Docker Hub uses registry-1.docker.io as the pull host and auth.docker.io + // for token exchange. The realm host check must allow this documented + // delegation rather than rejecting it as an unauthorized host. + wwwAuth := `Bearer realm="https://auth.docker.io/token",service="registry.docker.io",scope="repository:library/ubuntu:pull"` + realm, _, _ := parseWWWAuthenticate(wwwAuth) + u, err := url.Parse(realm) + require.NoError(t, err) + + realmHost := strings.ToLower(u.Hostname()) + regHost := "registry-1.docker.io" + delegated := trustedRealmDelegations[regHost] + assert.Equal(t, "auth.docker.io", delegated, "Docker Hub auth host must be in trusted delegation map") + assert.Equal(t, delegated, realmHost, "auth.docker.io realm must be authorized for registry-1.docker.io") +} + func TestExchangeNGCBearerToken_RejectsNonNGCRegistry(t *testing.T) { // A non-NGC registry must be rejected before any HTTP request is made, // even when NGC credentials are configured. The spy transport fails the