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/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. 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 +} 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")) }) }) 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`)) }) })