From 63d930babc0d3e9dd69b66aa5c9520b2946c3831 Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Sat, 23 May 2026 11:53:19 -0500 Subject: [PATCH 1/4] fix(cluster): explicit kubeconfig precedence over in-cluster detection When an explicit kubeconfig path is provided (via --kubeconfig flag, VIRTWORK_KUBECONFIG env, or KUBECONFIG env), skip in-cluster config detection and use the explicit path directly. Previously, in-cluster config was always attempted first, silently ignoring explicit paths when running inside a pod. Resolution order: --kubeconfig > VIRTWORK_KUBECONFIG > KUBECONFIG > in-cluster service-account > ~/.kube/config. Extract ResolveKubeconfigPath for testable env-var resolution and configFromKubeconfig to reduce duplication. Signed-off-by: Melvin Hillsman --- cmd/virtwork/main.go | 4 +- internal/cluster/cluster.go | 81 +++++++++++++++++++++++++------------ 2 files changed, 58 insertions(+), 27 deletions(-) diff --git a/cmd/virtwork/main.go b/cmd/virtwork/main.go index cc00953..42e0665 100644 --- a/cmd/virtwork/main.go +++ b/cmd/virtwork/main.go @@ -197,7 +197,7 @@ func runE(cmd *cobra.Command, args []string) error { var c client.Client if !cfg.DryRun { var contextName string - c, contextName, err = cluster.Connect(cfg.KubeconfigPath) + c, contextName, err = cluster.Connect(cluster.ResolveKubeconfigPath(cfg.KubeconfigPath)) if err != nil { return fmt.Errorf("connecting to cluster: %w", err) } @@ -627,7 +627,7 @@ func cleanupE(cmd *cobra.Command, args []string) error { } // Connect to cluster early to capture context for audit - c, contextName, err := cluster.Connect(cfg.KubeconfigPath) + c, contextName, err := cluster.Connect(cluster.ResolveKubeconfigPath(cfg.KubeconfigPath)) if err != nil { return fmt.Errorf("connecting to cluster: %w", err) } diff --git a/internal/cluster/cluster.go b/internal/cluster/cluster.go index c28b447..729e699 100644 --- a/internal/cluster/cluster.go +++ b/internal/cluster/cluster.go @@ -26,40 +26,48 @@ func NewScheme() *runtime.Scheme { return scheme } -// Connect creates a controller-runtime client.Client. It first attempts -// in-cluster configuration; on failure it falls back to the kubeconfig at -// the given path (checking the KUBECONFIG env var when the path is empty). -// Both failures produce a wrapped error. +// ResolveKubeconfigPath returns the kubeconfig file path to use given +// the explicit path from Viper (--kubeconfig flag / VIRTWORK_KUBECONFIG). +// Precedence: explicitPath > KUBECONFIG env var > "" (triggers in-cluster +// or default ~/.kube/config resolution in Connect). +func ResolveKubeconfigPath(explicitPath string) string { + if explicitPath != "" { + return explicitPath + } + if v := os.Getenv("KUBECONFIG"); v != "" { + return v + } + return "" +} + +// Connect creates a controller-runtime client.Client using the following +// kubeconfig resolution order: +// +// 1. kubeconfigPath (already resolved via ResolveKubeconfigPath: --kubeconfig > VIRTWORK_KUBECONFIG > KUBECONFIG) +// 2. In-cluster service-account token (only attempted when kubeconfigPath is empty) +// 3. Default loading rules (~/.kube/config) func Connect(kubeconfigPath string) (client.Client, string, error) { scheme := NewScheme() + var restConfig *rest.Config var contextName string - if kubeconfigPath == "" { - kubeconfigPath = os.Getenv("KUBECONFIG") - } + var err error - restConfig, err := rest.InClusterConfig() - if err != nil { - // Not in-cluster, load from kubeconfig - loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() - if kubeconfigPath != "" { - loadingRules.ExplicitPath = kubeconfigPath - } - configOverrides := &clientcmd.ConfigOverrides{} - kubeConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig(loadingRules, configOverrides) - restConfig, err = kubeConfig.ClientConfig() + if kubeconfigPath != "" { + restConfig, contextName, err = configFromKubeconfig(kubeconfigPath) if err != nil { - return nil, "", fmt.Errorf("failed to build kubeconfig from %q: %w", kubeconfigPath, err) + return nil, "", err } - - // Get the current context name - rawConfig, err := kubeConfig.RawConfig() + } else { + restConfig, err = rest.InClusterConfig() if err == nil { - contextName = rawConfig.CurrentContext + contextName = "in-cluster" + } else { + restConfig, contextName, err = configFromKubeconfig("") + if err != nil { + return nil, "", err + } } - } else { - // In-cluster - no context name - contextName = "in-cluster" } c, err := client.New(restConfig, client.Options{Scheme: scheme}) @@ -69,3 +77,26 @@ func Connect(kubeconfigPath string) (client.Client, string, error) { return c, contextName, nil } + +// configFromKubeconfig loads a rest.Config and context name from a kubeconfig +// file. When path is empty, default loading rules (~/.kube/config) apply. +func configFromKubeconfig(path string) (*rest.Config, string, error) { + loadingRules := clientcmd.NewDefaultClientConfigLoadingRules() + if path != "" { + loadingRules.ExplicitPath = path + } + kubeConfig := clientcmd.NewNonInteractiveDeferredLoadingClientConfig( + loadingRules, &clientcmd.ConfigOverrides{}) + + restConfig, err := kubeConfig.ClientConfig() + if err != nil { + return nil, "", fmt.Errorf("failed to build kubeconfig from %q: %w", path, err) + } + + var contextName string + rawConfig, rawErr := kubeConfig.RawConfig() + if rawErr == nil { + contextName = rawConfig.CurrentContext + } + return restConfig, contextName, nil +} From 7f32d67a0191564dc32a4a810cb1df5068dac19c Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Sat, 23 May 2026 11:55:24 -0500 Subject: [PATCH 2/4] test(cluster): add ResolveKubeconfigPath and Connect precedence tests Cover the explicit kubeconfig precedence chain introduced in the previous commit: explicit path wins over env/in-cluster, KUBECONFIG env is the fallback, and default loading rules apply when both are absent. Signed-off-by: Melvin Hillsman --- internal/cluster/cluster_test.go | 126 ++++++++++++++++++++++++------- 1 file changed, 99 insertions(+), 27 deletions(-) diff --git a/internal/cluster/cluster_test.go b/internal/cluster/cluster_test.go index 7f7a8bc..fd151e2 100644 --- a/internal/cluster/cluster_test.go +++ b/internal/cluster/cluster_test.go @@ -45,30 +45,58 @@ var _ = Describe("NewScheme", func() { }) }) +var _ = Describe("ResolveKubeconfigPath", func() { + var origKubeconfig string + var hadKubeconfig bool + + BeforeEach(func() { + origKubeconfig, hadKubeconfig = os.LookupEnv("KUBECONFIG") + _ = os.Unsetenv("KUBECONFIG") + }) + + AfterEach(func() { + if hadKubeconfig { + _ = os.Setenv("KUBECONFIG", origKubeconfig) + } else { + _ = os.Unsetenv("KUBECONFIG") + } + }) + + It("should return explicit path when provided", func() { + _ = os.Setenv("KUBECONFIG", "/from-env") + result := cluster.ResolveKubeconfigPath("/explicit/path") + Expect(result).To(Equal("/explicit/path")) + }) + + It("should fall back to KUBECONFIG env when explicit path is empty", func() { + _ = os.Setenv("KUBECONFIG", "/from-kubeconfig-env") + result := cluster.ResolveKubeconfigPath("") + Expect(result).To(Equal("/from-kubeconfig-env")) + }) + + It("should return empty when no explicit path and no KUBECONFIG", func() { + result := cluster.ResolveKubeconfigPath("") + Expect(result).To(BeEmpty()) + }) +}) + var _ = Describe("Connect", func() { - It("should return error when both in-cluster and kubeconfig fail", func() { - // Ensure we're not running in-cluster by unsetting the env var - origHost := os.Getenv("KUBERNETES_SERVICE_HOST") + // Helper to disable in-cluster detection for tests running outside a pod + disableInCluster := func() func() { + origHost, hadHost := os.LookupEnv("KUBERNETES_SERVICE_HOST") _ = os.Unsetenv("KUBERNETES_SERVICE_HOST") - defer func() { - if origHost != "" { + return func() { + if hadHost { _ = os.Setenv("KUBERNETES_SERVICE_HOST", origHost) + } else { + _ = os.Unsetenv("KUBERNETES_SERVICE_HOST") } - }() - - _, _, err := cluster.Connect("/nonexistent/kubeconfig/path") - Expect(err).To(HaveOccurred()) - Expect(err.Error()).To(ContainSubstring("kubeconfig")) - }) + } + } - It("should use kubeconfig path when provided", func() { - // Create a minimal but invalid kubeconfig to confirm the path is used + writeFakeKubeconfig := func(contextName string) string { tmpFile, err := os.CreateTemp("", "kubeconfig-*.yaml") Expect(err).NotTo(HaveOccurred()) - defer func() { - _ = os.Remove(tmpFile.Name()) - }() - _, err = tmpFile.WriteString(`apiVersion: v1 kind: Config clusters: @@ -79,8 +107,8 @@ contexts: - context: cluster: test user: test - name: test -current-context: test + name: ` + contextName + ` +current-context: ` + contextName + ` users: - name: test user: @@ -88,20 +116,64 @@ users: `) Expect(err).NotTo(HaveOccurred()) _ = tmpFile.Close() + return tmpFile.Name() + } - // Ensure we're not running in-cluster - origHost := os.Getenv("KUBERNETES_SERVICE_HOST") - _ = os.Unsetenv("KUBERNETES_SERVICE_HOST") + It("should return error when both in-cluster and kubeconfig fail", func() { + restore := disableInCluster() + defer restore() + + _, _, err := cluster.Connect("/nonexistent/kubeconfig/path") + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("kubeconfig")) + }) + + It("should use explicit kubeconfig path when provided", func() { + restore := disableInCluster() + defer restore() + + path := writeFakeKubeconfig("explicit-ctx") + defer func() { _ = os.Remove(path) }() + + c, contextName, err := cluster.Connect(path) + Expect(err).NotTo(HaveOccurred()) + Expect(c).NotTo(BeNil()) + Expect(contextName).To(Equal("explicit-ctx")) + }) + + It("should skip in-cluster when explicit path is provided", func() { + path := writeFakeKubeconfig("explicit-over-incluster") + defer func() { _ = os.Remove(path) }() + + c, contextName, err := cluster.Connect(path) + Expect(err).NotTo(HaveOccurred()) + Expect(c).NotTo(BeNil()) + Expect(contextName).To(Equal("explicit-over-incluster")) + }) + + It("should fall back to default rules when path is empty and not in-cluster", func() { + restore := disableInCluster() + defer restore() + + origKubeconfig, had := os.LookupEnv("KUBECONFIG") defer func() { - if origHost != "" { - _ = os.Setenv("KUBERNETES_SERVICE_HOST", origHost) + if had { + _ = os.Setenv("KUBECONFIG", origKubeconfig) + } else { + _ = os.Unsetenv("KUBECONFIG") } }() - // Connect should succeed (client creation doesn't dial the server) - c, contextName, err := cluster.Connect(tmpFile.Name()) + path := writeFakeKubeconfig("default-rules-ctx") + defer func() { _ = os.Remove(path) }() + _ = os.Setenv("KUBECONFIG", path) + + // ResolveKubeconfigPath("") would pick up KUBECONFIG, but + // Connect("") should use default loading rules which also + // check KUBECONFIG. Verify it works with an empty path. + c, contextName, err := cluster.Connect("") Expect(err).NotTo(HaveOccurred()) Expect(c).NotTo(BeNil()) - Expect(contextName).NotTo(BeEmpty()) + Expect(contextName).To(Equal("default-rules-ctx")) }) }) From e1f83e4c72a35ac627f38dce89fff2e1f775c737 Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Sat, 23 May 2026 11:55:52 -0500 Subject: [PATCH 3/4] docs: document kubeconfig resolution precedence chain Add a dedicated section explaining the four-step kubeconfig resolution order and cross-reference it from the flag and environment variable tables. Signed-off-by: Melvin Hillsman --- docs/configuration.md | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index 0704533..d3c1f3e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -26,7 +26,7 @@ These apply to both `virtwork run` and `virtwork cleanup`. | Flag | Env Var | YAML Key | Default | Description | |---|---|---|---|---| | `--namespace` | `VIRTWORK_NAMESPACE` | `namespace` | `virtwork` | Kubernetes namespace for VMs and supporting resources | -| `--kubeconfig` | `VIRTWORK_KUBECONFIG` | `kubeconfig` | _empty (in-cluster, then `~/.kube/config`)_ | Path to kubeconfig file | +| `--kubeconfig` | `VIRTWORK_KUBECONFIG` | `kubeconfig` | _empty (in-cluster, then `~/.kube/config`)_ | Path to kubeconfig file (see [kubeconfig resolution](#kubeconfig-resolution)) | | `--config` | — | — | _empty_ | Path to a YAML config file to merge in | | `--verbose` | `VIRTWORK_VERBOSE` | `verbose` | `false` | Switch the logger from INFO to DEBUG | | `--audit` | `VIRTWORK_AUDIT` | `audit` | `true` | Enable SQLite audit tracking | @@ -80,7 +80,7 @@ Global flags (`--namespace`, `--kubeconfig`, `--audit`, `--no-audit`, `--audit-d | `VIRTWORK_CPU_CORES` | int | `2` | flag-bound | Default per-VM CPU cores | | `VIRTWORK_DATA_DISK_SIZE` | string | `10Gi` | flag-bound | Default data disk size | | `VIRTWORK_DRY_RUN` | bool | `false` | flag-bound | Default for `--dry-run` | -| `VIRTWORK_KUBECONFIG` | string | _empty_ | flag-bound | Kubeconfig path | +| `VIRTWORK_KUBECONFIG` | string | _empty_ | flag-bound | Kubeconfig path (takes precedence over `KUBECONFIG`; see [kubeconfig resolution](#kubeconfig-resolution)) | | `VIRTWORK_MEMORY` | string | `2Gi` | flag-bound | Default per-VM memory | | `VIRTWORK_NAMESPACE` | string | `virtwork` | flag-bound | Target namespace | | `VIRTWORK_SSH_AUTHORIZED_KEYS` | string (csv) | _empty_ | env-only | Comma-separated SSH public keys | @@ -255,6 +255,21 @@ flowchart TD DEF -->|4th| MERGE ``` +### Kubeconfig resolution + +The `kubeconfig` key has an additional resolution layer handled by `cluster.ResolveKubeconfigPath` before the connection is established: + +``` +--kubeconfig / VIRTWORK_KUBECONFIG > KUBECONFIG env var > in-cluster service-account > ~/.kube/config +``` + +1. **Explicit path** — `--kubeconfig` flag or `VIRTWORK_KUBECONFIG` env var (resolved via the standard Viper priority chain above). When set, in-cluster detection is skipped entirely. +2. **`KUBECONFIG` env var** — the standard Kubernetes variable. Checked only when no explicit path is provided. +3. **In-cluster service-account** — `rest.InClusterConfig()` is attempted only when *no* kubeconfig path is resolved from steps 1–2. +4. **Default loading rules** — `~/.kube/config` (via `clientcmd` default loading rules) is the final fallback. + +This means setting `VIRTWORK_KUBECONFIG` in a CI/CD pipeline or ConfigMap will always win over `KUBECONFIG`, and any explicit path will bypass in-cluster detection — even when running inside a pod. + How it works in code (`internal/config/config.go` → `LoadConfig`): 1. `SetDefaults(v)` seeds Viper with the built-in defaults. From 85d96ece3dd52ff4a26f4860fea1d9030a21ab88 Mon Sep 17 00:00:00 2001 From: Melvin Hillsman Date: Sat, 23 May 2026 13:06:25 -0500 Subject: [PATCH 4/4] fix(e2e): set CGO_ENABLED=1 for test binary build go-sqlite3 requires CGO; the previous CGO_ENABLED=0 produced a binary that could not initialize the audit subsystem, failing every E2E test. The registry returns "workload not found", not "unknown workload". The codebase migrated from fmt.Fprintf to structured slog logging, so cleanup output is now JSON with "vms_deleted":N rather than the old "N VMs deleted" human-readable format. Update 4 assertions across cleanup_test.go and fullcycle_test.go. Signed-off-by: Melvin Hillsman --- internal/testutil/binary.go | 6 +++--- tests/e2e/cleanup_test.go | 4 ++-- tests/e2e/dryrun_test.go | 2 +- tests/e2e/fullcycle_test.go | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/internal/testutil/binary.go b/internal/testutil/binary.go index 9d6e29f..0127e2c 100644 --- a/internal/testutil/binary.go +++ b/internal/testutil/binary.go @@ -27,8 +27,8 @@ var ( // 1. VIRTWORK_BINARY environment variable (if set) // 2. Build from source (on first call, cached thereafter) // -// The binary is built with CGO_ENABLED=0 for portability and placed in a -// temporary directory. The build finds the module root automatically by +// The binary is built with CGO_ENABLED=1 (required by go-sqlite3) and placed +// in a temporary directory. The build finds the module root automatically by // walking up from this file's location. // // Example: @@ -72,7 +72,7 @@ func buildBinary() (string, error) { //nolint:gosec cmd := exec.Command("go", "build", "-o", outputPath, "./cmd/virtwork") cmd.Dir = moduleRoot - cmd.Env = append(os.Environ(), "CGO_ENABLED=0") + cmd.Env = append(os.Environ(), "CGO_ENABLED=1") var stderr bytes.Buffer cmd.Stderr = &stderr diff --git a/tests/e2e/cleanup_test.go b/tests/e2e/cleanup_test.go index 6e56a95..417702a 100644 --- a/tests/e2e/cleanup_test.go +++ b/tests/e2e/cleanup_test.go @@ -41,7 +41,7 @@ var _ = Describe("virtwork cleanup", Label("slow"), func() { "cleanup", "--namespace", namespace) Expect(err).NotTo(HaveOccurred()) Expect(exitCode).To(Equal(0)) - Expect(stdout).To(ContainSubstring("1 VMs deleted")) + Expect(stdout).To(ContainSubstring(`"vms_deleted":1`)) }) It("should not delete the namespace by default", func() { @@ -79,6 +79,6 @@ var _ = Describe("virtwork cleanup", Label("slow"), func() { "cleanup", "--namespace", namespace) Expect(err).NotTo(HaveOccurred()) Expect(exitCode2).To(Equal(0)) - Expect(stdout).To(ContainSubstring("0 VMs deleted")) + Expect(stdout).To(ContainSubstring(`"vms_deleted":0`)) }) }) diff --git a/tests/e2e/dryrun_test.go b/tests/e2e/dryrun_test.go index 44b04c2..ff1c538 100644 --- a/tests/e2e/dryrun_test.go +++ b/tests/e2e/dryrun_test.go @@ -76,6 +76,6 @@ var _ = Describe("virtwork run --dry-run", func() { "run", "--dry-run", "--workloads", "nonexistent") Expect(err).NotTo(HaveOccurred()) Expect(exitCode).NotTo(Equal(0)) - Expect(stderr).To(ContainSubstring("unknown workload")) + Expect(stderr).To(ContainSubstring("workload not found")) }) }) diff --git a/tests/e2e/fullcycle_test.go b/tests/e2e/fullcycle_test.go index e5e0bb9..f389d28 100644 --- a/tests/e2e/fullcycle_test.go +++ b/tests/e2e/fullcycle_test.go @@ -52,7 +52,7 @@ var _ = Describe("Full deployment cycle", Label("slow"), func() { "cleanup", "--namespace", namespace) Expect(err).NotTo(HaveOccurred()) Expect(exitCode).To(Equal(0)) - Expect(stdout).To(ContainSubstring("1 VMs deleted")) + Expect(stdout).To(ContainSubstring(`"vms_deleted":1`)) // Step 4: Verify resources are gone (KubeVirt finalizers may delay removal) Eventually(func() int { @@ -90,6 +90,6 @@ var _ = Describe("Full deployment cycle", Label("slow"), func() { "cleanup", "--namespace", namespace) Expect(err).NotTo(HaveOccurred()) Expect(exitCode).To(Equal(0)) - Expect(stdout).To(ContainSubstring("2 VMs deleted")) + Expect(stdout).To(ContainSubstring(`"vms_deleted":2`)) }) })