feat(check): add control-plane validator, stale namespace detection, and registry credential checks #782
+12,543
−4,330
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -142,20 +142,57 @@ api_keys_owner_id: [email protected] | |
|
|
||
| # Image reference for the cluster-validator pod used by | ||
| # `nvcf-cli self-hosted check`. The CLI runs this image as a Job in the | ||
| # cluster being checked to verify prerequisites before install. One image | ||
| # serves both roles; the Job environment selects the check set. The | ||
| # control-plane checks need an image from an NVCA release that supports | ||
| # validator roles; with an older image the CLI reports a warning instead of | ||
| # running them. | ||
| # | ||
| # The Job needs a ServiceAccount, ClusterRole, and ClusterRoleBinding, which | ||
| # the CLI creates and removes per run. The kubeconfig context therefore needs | ||
| # permission to manage those objects; without it the validator cannot run and | ||
| # the check fails. Pass --skip-cluster-validation to run without it. | ||
| # | ||
| # Config key: cluster_validator_image | ||
| # Environment variable: NVCF_CLI_CLUSTER_VALIDATOR_IMAGE | ||
| # Command-line flag: --cluster-validator-image | ||
| # | ||
| # When the value has no tag, the latest tag is discovered from the | ||
| # registry (preferring stable releases over rc). If discovery fails, the | ||
| # validator is skipped with a note rather than pulled as :latest; pin a tag | ||
| # to avoid depending on discovery. If unset everywhere, the validator probe | ||
| # is skipped with a note on stderr. | ||
| # | ||
| # Staging: stg.nvcr.io/nvidia/nvcf-byoc/cluster-validator | ||
| # Prod: nvcr.io/nvidia/nvcf-byoc/cluster-validator | ||
| # cluster_validator_image: nvcr.io/nvidia/nvcf-byoc/cluster-validator | ||
|
|
||
| # Extra container registries the control-plane validator probes for | ||
| # reachability, as host:port. The registries the install pulls from are | ||
| # probed already: the validator image's registry, the stack's | ||
| # global.image.registry, and quay.io for the cert-manager ACME solver. List | ||
| # only registries beyond those. | ||
| # | ||
| # Config key: cluster_validator_registries (list) | ||
| # Environment variable: NVCF_CLI_CLUSTER_VALIDATOR_REGISTRIES (comma-separated) | ||
| # Command-line flag: --cluster-validator-registries (repeatable or comma-separated) | ||
| # | ||
| # cluster_validator_registries: | ||
| # - harbor.example.com:443 | ||
| # - ghcr.io:443 | ||
|
|
||
| # Image for the control-plane validator's node-to-node overlay probe, which | ||
| # needs sh and a busybox-style nc. Defaults to busybox:1.36 from Docker Hub. | ||
| # Set a mirror for air-gapped clusters. The probe pods get no imagePullSecrets, | ||
| # so the image must be pullable anonymously or through node-level registry | ||
| # credentials. | ||
| # | ||
| # Config key: cluster_validator_probe_image | ||
| # Environment variable: NVCF_CLI_CLUSTER_VALIDATOR_PROBE_IMAGE | ||
| # Command-line flag: --cluster-validator-probe-image | ||
| # | ||
| # cluster_validator_probe_image: harbor.example.com/library/busybox:1.36 | ||
|
|
||
| # ============================================================================== | ||
| # General Settings | ||
| # ============================================================================== | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,16 +19,132 @@ package cmd | |
|
|
||
| import ( | ||
| "context" | ||
| "net/http" | ||
| "net/http/httptest" | ||
| "os" | ||
| "testing" | ||
|
|
||
| "github.com/spf13/pflag" | ||
| "github.com/spf13/viper" | ||
|
|
||
| "nvcf-cli/internal/selfhosted" | ||
| ) | ||
|
|
||
| // Default-stub every seam that would otherwise reach the developer's cluster or | ||
| // the network. Individual tests reassign a variable when they explicitly want | ||
| // to exercise that path. | ||
| // | ||
| // These are not conveniences. Without them `go test ./cmd/` creates a hostPath | ||
| // busybox pod per node in `default` (the inotify probe), lists Secrets across | ||
| // the stack namespaces of whatever kubeconfig happens to be current, and makes | ||
| // an outbound request to nvcr.io per configured registry. That mutates a real | ||
| // cluster from a unit test, and it is why the package took minutes and failed | ||
| // on a proxied kubeconfig rather than seconds and deterministically. | ||
| func TestMain(m *testing.M) { | ||
| // Every command reads ~/.nvcf-cli.yaml and some tests save | ||
| // ~/.nvcf-cli.state, so a developer's real config would steer results and | ||
| // a test run would overwrite their saved credentials. | ||
| home, err := os.MkdirTemp("", "nvcf-cli-cmd-test-home-") | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| _ = os.Setenv("HOME", home) | ||
| resolveLatestValidatorTagForSelfHosted = func(_ context.Context, _ string) (string, bool) { | ||
| return "", false | ||
| } | ||
| // Nil prober: the inotify check is skipped rather than creating pods. | ||
| newInotifyProberForSelfHosted = func() selfhosted.NodeInotifyProber { return nil } | ||
| // No cluster contact, and a clean result so the category still renders. | ||
| newStaleNamespaceProberForSelfHosted = func() selfhosted.StaleNamespaceProber { | ||
| return func(context.Context, string, []string) ([]selfhosted.StaleNamespace, error) { | ||
| return nil, nil | ||
| } | ||
| } | ||
| // No outbound registry request. | ||
| newRegistryCredentialCheckerForSelfHosted = func() selfhosted.RegistryCredentialChecker { | ||
| return func(context.Context, string, string, bool) error { return nil } | ||
| } | ||
| // No validator Job. A developer with NVCF_CLI_CLUSTER_VALIDATOR_IMAGE | ||
| // exported, or cluster_validator_image in ~/.nvcf-cli.yaml, would | ||
| // otherwise have every check test create RBAC, Secrets and Jobs in the | ||
| // current kube context and wait up to five minutes per role. | ||
| newClusterValidatorForSelfHosted = func() selfhosted.ClusterValidator { | ||
| return func(context.Context, selfhosted.ClusterValidatorParams) selfhosted.ClusterValidatorResult { | ||
| return selfhosted.ClusterValidatorResult{Passed: true} | ||
| } | ||
| } | ||
| for _, k := range []string{ | ||
| "NVCF_CLI_CLUSTER_VALIDATOR_IMAGE", "NVCF_CLI_CLUSTER_VALIDATOR_REGISTRIES", | ||
| "NVCF_CLI_CLUSTER_VALIDATOR_PROBE_IMAGE", | ||
| } { | ||
| _ = os.Unsetenv(k) | ||
| } | ||
| // The SIS reachability check has no seam, but it resolves its URL from | ||
| // NVCF_ICMS_URL, so resetCheckFlags points check tests at this local | ||
| // server instead of the real SIS. Scoped per test: setting it for the | ||
| // whole package would change what the ICMS URL resolution tests resolve. | ||
| sis := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| w.WriteHeader(http.StatusOK) | ||
| })) | ||
| testSISURL = sis.URL | ||
| code := m.Run() | ||
| sis.Close() | ||
| _ = os.RemoveAll(home) | ||
| os.Exit(code) | ||
| } | ||
|
|
||
| // testSISURL is a local stand-in for SIS, set by TestMain. | ||
| var testSISURL string | ||
|
|
||
| // resetFlag returns f to its default value and clears its Changed marker. A | ||
| // slice flag is replaced, since Set appends once the flag has been set. | ||
| func resetFlag(f *pflag.Flag) { | ||
| if sv, ok := f.Value.(pflag.SliceValue); ok { | ||
| _ = sv.Replace(nil) | ||
| } else { | ||
| _ = f.Value.Set(f.DefValue) | ||
| } | ||
| f.Changed = false | ||
| } | ||
|
|
||
| // resetCheckFlags returns every `self-hosted check` flag to its default, | ||
| // including cobra's Changed marker, now and when the test ends. The flag | ||
| // variables are package globals that survive rootCmd.Execute, so without this | ||
| // a test that passes --pre leaks it into whichever test runs next, and a | ||
| // regression guard can pass or fail on test order alone. | ||
| func resetCheckFlags(t *testing.T) { | ||
| t.Helper() | ||
| if testSISURL != "" { | ||
| t.Setenv("NVCF_ICMS_URL", testSISURL) | ||
| } | ||
| reset := func() { | ||
| checkPre, checkControlPlane, checkComputePlane, checkAll = false, false, false, false | ||
| checkClusterName = "" | ||
|
Comment on lines
+116
to
+122
Contributor
There was a problem hiding this comment. Round-3 #19/#11 are partly fixed.
Fix:
Contributor
Author
There was a problem hiding this comment. Fixed. TestMain points HOME at a temp dir, so ~/.nvcf-cli.yaml is not read and ~/.nvcf-cli.state is not written (I checked the real state file's mtime across a full run). resetCheckFlags now restores each flag's value from DefValue, not only Changed. The probe-image test sets the flag instead of calling viper.Set. Your repro passes, and -run TestCheck_ passes for 12 of 12 shuffle seeds; with the value reset removed, it fails again (183583a). |
||
| checkLocalOnly, checkSkipInotifyCheck, checkSkipClusterValidation = false, false, false | ||
| checkClusterValidatorImage, checkClusterValidatorPullSecret = "", "" | ||
| checkClusterValidatorNoCleanup = false | ||
| checkClusterValidatorRegistries = nil | ||
| checkClusterValidatorProbeImage = "" | ||
| checkShowLogs = false | ||
| selfHostedJSON, selfHostedPlain = false, false | ||
| selfHostedOutput = "text" | ||
| selfHostedWait = "" | ||
| selfHostedControlPlaneContext, selfHostedComputePlaneContext = "", "" | ||
| // Values too, not only the Changed marker: a test that passes | ||
| // --icms-url would otherwise point every later check at its URL. | ||
| for _, fs := range []*pflag.FlagSet{selfHostedCheckCmd.Flags(), selfHostedCmd.PersistentFlags()} { | ||
| fs.VisitAll(resetFlag) | ||
| } | ||
| // Other tests call viper.Reset(), which drops the bindings made at | ||
| // init, so a flag passed to check would silently not be read. | ||
| for key, flag := range map[string]string{ | ||
| "cluster_validator_image": "cluster-validator-image", | ||
| "cluster_validator_registries": "cluster-validator-registries", | ||
| "cluster_validator_probe_image": "cluster-validator-probe-image", | ||
| } { | ||
| _ = viper.BindPFlag(key, selfHostedCheckCmd.Flags().Lookup(flag)) | ||
| } | ||
| } | ||
| reset() | ||
| t.Cleanup(reset) | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This
TestMainis the right fix, but it is incomplete:newClusterValidatorForSelfHostedand the SIS probe are still real, despite the "Default-stub every seam" comment.If a developer has
NVCF_CLI_CLUSTER_VALIDATOR_IMAGEexported, orcluster_validator_imageset in~/.nvcf-cli.yaml(which the template suggests),TestCheck_SingleClusterModeruns the real validator against the current kube context. That means Secret scans across 11 namespaces, SA/ClusterRole/CRB creation, a Secret minted fromNGC_API_KEY, and Jobs, with up to 5 minutes of waiting per role. Reproduced: it POSTs to/api/v1/namespaces/default/serviceaccounts. The--compute-planeand--alltests also GEThttps://sis.nvcf.nvidia.com/v1/health. Stub both seams, andos.Unsetenvthe image variables here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestMain now stubs newClusterValidatorForSelfHosted and unsets NVCF_CLI_CLUSTER_VALIDATOR_IMAGE, _REGISTRIES and _PROBE_IMAGE (d642bef). An image set in ~/.nvcf-cli.yaml now reaches only the stub, so no check test creates RBAC, Secrets or Jobs. SIS has no seam, so TestMain starts a local stand-in and resetCheckFlags points NVCF_ICMS_URL at it for every check test.