From adff1ade9d7687aba98dfe93dd6e9f25dbaa22ad Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Fri, 17 Jul 2026 06:59:05 +0200 Subject: [PATCH 01/10] fix(eks): route standalone lifecycle commands --- pkg/cli/cmd/cluster/create.go | 5 +- pkg/cli/cmd/cluster/delete.go | 31 ++- pkg/cli/cmd/cluster/eks_lifecycle_test.go | 278 ++++++++++++++++++++++ pkg/cli/cmd/cluster/export_test.go | 2 +- pkg/cli/cmd/cluster/ttl.go | 18 +- pkg/cli/lifecycle/simple.go | 75 +++++- 6 files changed, 380 insertions(+), 29 deletions(-) create mode 100644 pkg/cli/cmd/cluster/eks_lifecycle_test.go diff --git a/pkg/cli/cmd/cluster/create.go b/pkg/cli/cmd/cluster/create.go index 52dd939928..1e5c9f5abf 100644 --- a/pkg/cli/cmd/cluster/create.go +++ b/pkg/cli/cmd/cluster/create.go @@ -108,7 +108,7 @@ func handleCreateRunE( notify.Warningf(cmd.OutOrStderr(), "failed to save cluster state: %v", saveErr) } - return maybeWaitForTTL(cmd, clusterName, ctx.ClusterCfg) + return maybeWaitForTTL(cmd, clusterName, ctx.ClusterCfg, ctx.EKSConfig) } // newProvisionerFactory returns the cluster provisioner factory, using any test override if set. @@ -738,6 +738,7 @@ func maybeWaitForTTL( cmd *cobra.Command, clusterName string, clusterCfg *v1alpha1.Cluster, + eksConfig *clusterprovisioner.EKSConfig, ) error { ttlStr, _ := cmd.Flags().GetString("ttl") if ttlStr == "" { @@ -764,5 +765,5 @@ func maybeWaitForTTL( } // Block and wait for TTL, then auto-destroy. - return waitForTTLAndDelete(cmd, clusterName, clusterCfg, ttl) + return waitForTTLAndDelete(cmd, clusterName, clusterCfg, eksConfig, ttl) } diff --git a/pkg/cli/cmd/cluster/delete.go b/pkg/cli/cmd/cluster/delete.go index b149aacb7a..4c431751f9 100644 --- a/pkg/cli/cmd/cluster/delete.go +++ b/pkg/cli/cmd/cluster/delete.go @@ -116,12 +116,9 @@ func runDeleteAction( detectedInfo, isKindCluster, clusterInfo := detectDeleteClusterInfo(cmd, resolved) // Create provisioner for the provider - provisioner, err := createDeleteProvisioner( - clusterInfo, - resolved.OmniOpts, - resolved.KubernetesOpts, - flags.storage, - ) + options := minimalProvisionerOptions(resolved, flags.storage) + + provisioner, err := createDeleteProvisioner(cmd.Context(), clusterInfo, options) if err != nil { return fmt.Errorf("failed to create provisioner: %w", err) } @@ -157,6 +154,19 @@ func runDeleteAction( return nil } +func minimalProvisionerOptions( + resolved *lifecycle.ResolvedClusterInfo, + deleteStorage bool, +) lifecycle.MinimalProvisionerOptions { + return lifecycle.MinimalProvisionerOptions{ + OmniOpts: resolved.OmniOpts, + KubernetesOpts: resolved.KubernetesOpts, + AWSOpts: resolved.AWSOpts, + AWSRegion: resolved.AWSRegion, + DeleteStorage: deleteStorage, + } +} + // detectClusterDistribution detects the distribution and other cluster info. // This detection must happen before the cluster is deleted to ensure the kubeconfig // entry is still available for reading cluster information. @@ -306,10 +316,9 @@ func promptForDeletion( // createDeleteProvisioner creates the appropriate provisioner for cluster deletion. // It first checks for test overrides, then falls back to creating a minimal provisioner. func createDeleteProvisioner( + ctx context.Context, clusterInfo *clusterdetector.Info, - omniOpts v1alpha1.OptionsOmni, - kubernetesOpts v1alpha1.OptionsKubernetes, - deleteStorage bool, + options lifecycle.MinimalProvisionerOptions, ) (clusterprovisioner.Provisioner, error) { // Check for test factory override clusterProvisionerFactoryMu.RLock() @@ -319,7 +328,7 @@ func createDeleteProvisioner( clusterProvisionerFactoryMu.RUnlock() if factoryOverride != nil { - provisioner, _, err := factoryOverride.Create(context.Background(), nil) + provisioner, _, err := factoryOverride.Create(ctx, nil) if err != nil { return nil, fmt.Errorf("factory override failed: %w", err) } @@ -328,7 +337,7 @@ func createDeleteProvisioner( } provisioner, err := lifecycle.CreateMinimalProvisionerForProvider( - clusterInfo, omniOpts, kubernetesOpts, deleteStorage, + ctx, clusterInfo, options, ) if err != nil { return nil, fmt.Errorf("failed to create provisioner for provider: %w", err) diff --git a/pkg/cli/cmd/cluster/eks_lifecycle_test.go b/pkg/cli/cmd/cluster/eks_lifecycle_test.go new file mode 100644 index 0000000000..144369fc51 --- /dev/null +++ b/pkg/cli/cmd/cluster/eks_lifecycle_test.go @@ -0,0 +1,278 @@ +package cluster_test + +import ( + "fmt" + "io" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/devantler-tech/ksail/v7/pkg/cli/cmd/cluster" + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const standaloneEKSClusterFixture = `apiVersion: ksail.io/v1alpha1 +kind: Cluster +metadata: + name: config-file-name +spec: + cluster: + distribution: EKS + provider: AWS + distributionConfig: eks.yaml + connection: + kubeconfig: kubeconfig + provider: + aws: + profileEnvVar: KSAIL_PROFILE + regionEnvVar: KSAIL_REGION + accessKeyIdEnvVar: KSAIL_ACCESS + secretAccessKeyEnvVar: KSAIL_SECRET + sessionTokenEnvVar: KSAIL_SESSION +` + +const standaloneEKSEksConfigFixture = `apiVersion: eksctl.io/v1alpha5 +kind: ClusterConfig +metadata: + name: config-file-name + region: eu-west-1 +` + +const standaloneEKSEksctlFixture = `#!/bin/sh +[ "${AWS_PROFILE-}" = "selected-profile" ] || exit 41 +[ "${AWS_ACCESS_KEY_ID-}" = "fixture-access" ] || exit 42 +[ "${AWS_SECRET_ACCESS_KEY-}" = "fixture-secret" ] || exit 43 +[ "${AWS_SESSION_TOKEN-}" = "fixture-session" ] || exit 44 +[ -z "${KSAIL_PROFILE+x}" ] || exit 45 +[ -z "${KSAIL_ACCESS+x}" ] || exit 46 +[ -z "${KSAIL_SECRET+x}" ] || exit 47 +[ -z "${KSAIL_SESSION+x}" ] || exit 48 + +printf '%s\n' "$*" >> "$KSAIL_EKSCTL_MARKER" + +if [ "${KSAIL_EKSCTL_FAIL-}" = "$1" ]; then + printf 'forced %s failure\n' "$1" >&2 + exit 49 +fi + +if [ "$1 $2" = "get cluster" ]; then + printf '[{"Name":"%s","Region":"ap-southeast-2","EksctlCreated":"True"}]\n' "$KSAIL_EKS_CLUSTER" +elif [ "$1 $2" = "get nodegroup" ]; then + printf '[{"Cluster":"%s","Name":"workers","Status":"ACTIVE",' "$KSAIL_EKS_CLUSTER" + printf '"DesiredCapacity":0,"MinSize":2,"MaxSize":4,"NodeGroupType":"managed"}]\n' +fi +` + +type standaloneEKSLifecycleCase struct { + name string + newCommand func() *cobra.Command + extraArgs []string + expectedCalls func(clusterName string) []string +} + +func standaloneEKSLifecycleCases() []standaloneEKSLifecycleCase { + return []standaloneEKSLifecycleCase{ + { + name: "delete", + newCommand: cluster.NewDeleteCmd, + extraArgs: []string{"--force"}, + expectedCalls: func(clusterName string) []string { + return []string{ + "get cluster --output json --region ap-southeast-2", + fmt.Sprintf( + "delete cluster --name %s --region ap-southeast-2 --wait", + clusterName, + ), + } + }, + }, + { + name: "start", + newCommand: cluster.NewStartCmd, + expectedCalls: func(clusterName string) []string { + return []string{ + fmt.Sprintf( + "get nodegroup --cluster %s --output json --region ap-southeast-2", + clusterName, + ), + fmt.Sprintf( + "scale nodegroup --cluster %s --name workers --nodes 2 --nodes-min 2 --nodes-max 4 --region ap-southeast-2", + clusterName, + ), + } + }, + }, + { + name: "stop", + newCommand: cluster.NewStopCmd, + expectedCalls: func(clusterName string) []string { + return []string{ + fmt.Sprintf( + "get nodegroup --cluster %s --output json --region ap-southeast-2", + clusterName, + ), + fmt.Sprintf( + "scale nodegroup --cluster %s --name workers --nodes 0 --nodes-min 0 --nodes-max 4 --region ap-southeast-2", + clusterName, + ), + } + }, + }, + } +} + +// TestStandaloneEKSLifecycleCommandsRouteToEksctl proves the three standalone +// command surfaces reach the existing EKS provisioner, honor the custom AWS +// credential mapping, and prefer the configured region environment variable. +// +//nolint:paralleltest // each case changes process environment and working directory +func TestStandaloneEKSLifecycleCommandsRouteToEksctl(t *testing.T) { + for _, testCase := range standaloneEKSLifecycleCases() { + //nolint:paralleltest // setup changes process environment and working directory + t.Run(testCase.name, func(t *testing.T) { + clusterName := "ksail-eks-" + testCase.name + "-routing-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + + cmd := testCase.newCommand() + args := make([]string, 0, 4+len(testCase.extraArgs)) + args = append(args, "--name", clusterName, "--provider", "AWS") + args = append(args, testCase.extraArgs...) + cmd.SetArgs(args) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + require.NoError(t, cmd.Execute()) + + calls, err := os.ReadFile(markerPath) //nolint:gosec // test-private path + require.NoError(t, err) + assert.Equal( + t, + testCase.expectedCalls(clusterName), + strings.Split(strings.TrimSpace(string(calls)), "\n"), + ) + assertParentAWSEnvironmentUnchanged(t) + }) + } +} + +// TestStandaloneEKSLifecycleCommandsPropagateEksctlErrors covers each command's +// user-facing error path after routing has reached eksctl. +func TestStandaloneEKSLifecycleCommandsPropagateEksctlErrors(t *testing.T) { + testCases := []struct { + name string + newCommand func() *cobra.Command + extraArgs []string + failOn string + wantError string + }{ + { + name: "delete", + newCommand: cluster.NewDeleteCmd, + extraArgs: []string{"--force"}, + failOn: "delete", + wantError: "cluster deletion failed", + }, + { + name: "start", + newCommand: cluster.NewStartCmd, + failOn: "scale", + wantError: "failed to starting cluster", + }, + { + name: "stop", + newCommand: cluster.NewStopCmd, + failOn: "scale", + wantError: "failed to stopping cluster", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + clusterName := "ksail-eks-" + testCase.name + "-error-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("KSAIL_EKSCTL_FAIL", testCase.failOn) + + cmd := testCase.newCommand() + args := make([]string, 0, 4+len(testCase.extraArgs)) + args = append(args, "--name", clusterName, "--provider", "AWS") + args = append(args, testCase.extraArgs...) + cmd.SetArgs(args) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), testCase.wantError) + + calls, readErr := os.ReadFile(markerPath) //nolint:gosec // test-private path + require.NoError(t, readErr) + assert.Contains(t, string(calls), testCase.failOn+" ") + assertParentAWSEnvironmentUnchanged(t) + }) + } +} + +func setupStandaloneEKSLifecycleFixture( + t *testing.T, + clusterName string, +) string { + t.Helper() + + workingDir := t.TempDir() + t.Chdir(workingDir) + + binDir := t.TempDir() + markerPath := filepath.Join(t.TempDir(), "eksctl-calls") + eksctlPath := filepath.Join(binDir, "eksctl") + writeExecutableFixture(t, eksctlPath, standaloneEKSEksctlFixture) + + require.NoError( + t, + os.WriteFile( + filepath.Join(workingDir, "ksail.yaml"), + []byte(standaloneEKSClusterFixture), + 0o600, + ), + ) + eksConfigPath := filepath.Join(workingDir, "eks.yaml") + require.NoError(t, os.WriteFile(eksConfigPath, []byte(standaloneEKSEksConfigFixture), 0o600)) + require.NoError( + t, + os.WriteFile( + filepath.Join(workingDir, "kubeconfig"), + []byte("apiVersion: v1\nkind: Config\n"), + 0o600, + ), + ) + + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("KSAIL_EKSCTL_MARKER", markerPath) + t.Setenv("KSAIL_EKS_CLUSTER", clusterName) + t.Setenv("KSAIL_PROFILE", "selected-profile") + t.Setenv("KSAIL_REGION", "ap-southeast-2") + t.Setenv("KSAIL_ACCESS", "fixture-access") + t.Setenv("KSAIL_SECRET", "fixture-secret") + t.Setenv("KSAIL_SESSION", "fixture-session") + t.Setenv("AWS_PROFILE", "stale-profile") + t.Setenv("AWS_REGION", "stale-region") + t.Setenv("AWS_ACCESS_KEY_ID", "stale-access") + t.Setenv("AWS_SECRET_ACCESS_KEY", "stale-secret") + t.Setenv("AWS_SESSION_TOKEN", "stale-session") + + return markerPath +} + +func assertParentAWSEnvironmentUnchanged(t *testing.T) { + t.Helper() + + assert.Equal(t, "stale-profile", os.Getenv("AWS_PROFILE")) + assert.Equal(t, "stale-region", os.Getenv("AWS_REGION")) + assert.Equal(t, "stale-access", os.Getenv("AWS_ACCESS_KEY_ID")) + assert.Equal(t, "stale-secret", os.Getenv("AWS_SECRET_ACCESS_KEY")) + assert.Equal(t, "stale-session", os.Getenv("AWS_SESSION_TOKEN")) +} diff --git a/pkg/cli/cmd/cluster/export_test.go b/pkg/cli/cmd/cluster/export_test.go index a1b31ace2c..304925291b 100644 --- a/pkg/cli/cmd/cluster/export_test.go +++ b/pkg/cli/cmd/cluster/export_test.go @@ -299,7 +299,7 @@ func ExportMaybeWaitForTTL( clusterName string, clusterCfg *v1alpha1.Cluster, ) error { - return maybeWaitForTTL(cmd, clusterName, clusterCfg) + return maybeWaitForTTL(cmd, clusterName, clusterCfg, nil) } // ExportNormalizeVersionTag exposes normalizeVersionTag for testing. diff --git a/pkg/cli/cmd/cluster/ttl.go b/pkg/cli/cmd/cluster/ttl.go index 8e18e5be72..0d32a23b70 100644 --- a/pkg/cli/cmd/cluster/ttl.go +++ b/pkg/cli/cmd/cluster/ttl.go @@ -8,8 +8,10 @@ import ( "time" v1alpha1 "github.com/devantler-tech/ksail/v7/pkg/apis/cluster/v1alpha1" + "github.com/devantler-tech/ksail/v7/pkg/cli/lifecycle" "github.com/devantler-tech/ksail/v7/pkg/notify" clusterdetector "github.com/devantler-tech/ksail/v7/pkg/svc/detector/cluster" + clusterprovisioner "github.com/devantler-tech/ksail/v7/pkg/svc/provisioner/cluster" "github.com/devantler-tech/ksail/v7/pkg/svc/state" "github.com/spf13/cobra" ) @@ -25,6 +27,7 @@ func waitForTTLAndDelete( cmd *cobra.Command, clusterName string, clusterCfg *v1alpha1.Cluster, + eksConfig *clusterprovisioner.EKSConfig, ttl time.Duration, ) error { notify.Infof(cmd.OutOrStdout(), @@ -39,7 +42,7 @@ func waitForTTLAndDelete( select { case <-timer.C: - return autoDeleteCluster(cmd, clusterName, clusterCfg) + return autoDeleteCluster(cmd, clusterName, clusterCfg, eksConfig) case <-ctx.Done(): notify.Infof(cmd.OutOrStdout(), "TTL wait cancelled; cluster %q will remain running", clusterName) @@ -55,6 +58,7 @@ func autoDeleteCluster( cmd *cobra.Command, clusterName string, clusterCfg *v1alpha1.Cluster, + eksConfig *clusterprovisioner.EKSConfig, ) error { notify.Infof(cmd.OutOrStdout(), "TTL expired; auto-destroying cluster %q...", clusterName) @@ -66,7 +70,17 @@ func autoDeleteCluster( } provisioner, err := createDeleteProvisioner( - info, clusterCfg.Spec.Provider.Omni, clusterCfg.Spec.Provider.Kubernetes, false, + cmd.Context(), + info, + lifecycle.MinimalProvisionerOptions{ + OmniOpts: clusterCfg.Spec.Provider.Omni, + KubernetesOpts: clusterCfg.Spec.Provider.Kubernetes, + AWSOpts: clusterCfg.Spec.Provider.AWS, + AWSRegion: lifecycle.ResolveAWSRegion( + clusterCfg.Spec.Provider.AWS, + &clusterprovisioner.DistributionConfig{EKS: eksConfig}, + ), + }, ) if err != nil { return fmt.Errorf("TTL auto-delete: failed to create provisioner: %w", err) diff --git a/pkg/cli/lifecycle/simple.go b/pkg/cli/lifecycle/simple.go index 34f54f4f35..8ab0ebc509 100644 --- a/pkg/cli/lifecycle/simple.go +++ b/pkg/cli/lifecycle/simple.go @@ -351,10 +351,14 @@ func provisionAndAct( } provisioner, err := CreateMinimalProvisionerForProvider( + cmd.Context(), clusterInfo, - resolved.OmniOpts, - resolved.KubernetesOpts, - false, + MinimalProvisionerOptions{ + OmniOpts: resolved.OmniOpts, + KubernetesOpts: resolved.KubernetesOpts, + AWSOpts: resolved.AWSOpts, + AWSRegion: resolved.AWSRegion, + }, ) if err != nil { return fmt.Errorf("failed to create provisioner: %w", err) @@ -394,14 +398,23 @@ func CreateMinimalProvisioner( return provisioner, nil } +// MinimalProvisionerOptions contains the provider-specific context needed to +// construct a standalone lifecycle provisioner from only a resolved target. +type MinimalProvisionerOptions struct { + OmniOpts v1alpha1.OptionsOmni + KubernetesOpts v1alpha1.OptionsKubernetes + AWSOpts v1alpha1.OptionsAWS + AWSRegion string + DeleteStorage bool +} + // CreateMinimalProvisionerForProvider creates provisioners for all distributions // that support the given provider, and returns the first one that can operate on the cluster. // This is used when we only have --name and --provider flags without distribution info. func CreateMinimalProvisionerForProvider( + ctx context.Context, info *clusterdetector.Info, - omniOpts v1alpha1.OptionsOmni, - kubernetesOpts v1alpha1.OptionsKubernetes, - deleteStorage bool, + options MinimalProvisionerOptions, ) (clusterprovisioner.Provisioner, error) { switch info.Provider { case v1alpha1.ProviderDocker, "": @@ -415,7 +428,7 @@ func CreateMinimalProvisionerForProvider( // Pass info.KubeconfigPath as the nested cluster's kubeconfig for context cleanup. return newKubernetesCleanupProvisioner( info.ClusterName, - kubernetesOpts, + options.KubernetesOpts, info.KubeconfigPath, ) @@ -436,21 +449,23 @@ func CreateMinimalProvisionerForProvider( info.Provider, v1alpha1.OptionsTalos{}, v1alpha1.OptionsHetzner{}, - omniOpts, + options.OmniOpts, false, ) if err != nil { return nil, fmt.Errorf("failed to create talos provisioner: %w", err) } - provisioner.WithDeleteStorage(deleteStorage) + provisioner.WithDeleteStorage(options.DeleteStorage) return provisioner, nil - case v1alpha1.ProviderAWS, v1alpha1.ProviderGCP, v1alpha1.ProviderAzure: - // AWS, GCP, and Azure only support their managed distributions - // (EKS/GKE/AKS), which are not docker-based and cannot be produced by - // the minimal multi-provisioner path; their lifecycle goes through the factory. + case v1alpha1.ProviderAWS: + return createMinimalEKSProvisioner(ctx, info, options) + + case v1alpha1.ProviderGCP, v1alpha1.ProviderAzure: + // GCP and Azure only support their managed distributions (GKE/AKS), + // which are not yet wired into this standalone lifecycle path. return nil, fmt.Errorf( "%w: %s provider is only supported via its managed distribution", clusterprovisioner.ErrUnsupportedProvider, @@ -466,6 +481,40 @@ func CreateMinimalProvisionerForProvider( } } +// createMinimalEKSProvisioner builds the name-and-region-only EKS configuration +// needed by standalone delete/start/stop commands, while reusing the default +// factory's credential-isolated eksctl and AWS provider wiring. ConfigPath is +// deliberately empty: an explicit/resolved cluster name must never be replaced +// by a possibly unrelated local eks.yaml during a destructive delete. +func createMinimalEKSProvisioner( + ctx context.Context, + info *clusterdetector.Info, + options MinimalProvisionerOptions, +) (clusterprovisioner.Provisioner, error) { + clusterCfg := &v1alpha1.Cluster{} + clusterCfg.Name = info.ClusterName + clusterCfg.Spec.Cluster.Distribution = v1alpha1.DistributionEKS + clusterCfg.Spec.Cluster.Provider = v1alpha1.ProviderAWS + clusterCfg.Spec.Provider.AWS = options.AWSOpts + + factory := clusterprovisioner.DefaultFactory{ + DistributionConfig: &clusterprovisioner.DistributionConfig{ + EKS: &clusterprovisioner.EKSConfig{ + Name: info.ClusterName, + Region: options.AWSRegion, + KubeconfigPath: info.KubeconfigPath, + }, + }, + } + + provisioner, _, err := factory.Create(ctx, clusterCfg) + if err != nil { + return nil, fmt.Errorf("failed to create EKS provisioner: %w", err) + } + + return provisioner, nil +} + // kubernetesCleanupProvisioner deletes nested clusters on a Kubernetes provider // by removing the ksail- namespace (cascading delete removes all resources). type kubernetesCleanupProvisioner struct { From c29922c058d4b2cd2a346dd777a3dd6d47d415c9 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Fri, 17 Jul 2026 10:12:51 +0200 Subject: [PATCH 02/10] fix(eks): fail closed on standalone lifecycle targets Route EKS delete, start, and stop through exact name-region ownership checks, preserve custom AWS credentials, and bind create, update, and TTL flows to canonical config provenance. Refs #6087 --- pkg/cli/cmd/cluster/context_test.go | 19 + pkg/cli/cmd/cluster/create.go | 69 +- pkg/cli/cmd/cluster/create_test.go | 73 + pkg/cli/cmd/cluster/delete.go | 5 + pkg/cli/cmd/cluster/eks_lifecycle_test.go | 1280 ++++++++++++++++- pkg/cli/cmd/cluster/export_test.go | 18 +- pkg/cli/cmd/cluster/mutation.go | 142 +- .../cmd/cluster/post_create_context_test.go | 35 +- pkg/cli/cmd/cluster/testhelpers_test.go | 16 +- pkg/cli/cmd/cluster/ttl.go | 104 +- pkg/cli/cmd/cluster/unmanaged_guard.go | 358 ++++- pkg/cli/cmd/cluster/unmanaged_guard_test.go | 92 +- pkg/cli/cmd/cluster/update.go | 7 +- pkg/cli/lifecycle/awsregion_test.go | 16 +- pkg/cli/lifecycle/simple.go | 222 ++- pkg/cli/lifecycle/simple_additional_test.go | 41 + .../configmanager/ksail/distribution.go | 57 +- .../configmanager/ksail/distribution_test.go | 50 + .../provisioner/cluster/eks/provisioner.go | 18 +- .../cluster/eks/provisioner_test.go | 16 +- pkg/svc/provisioner/cluster/factory.go | 3 + 21 files changed, 2473 insertions(+), 168 deletions(-) diff --git a/pkg/cli/cmd/cluster/context_test.go b/pkg/cli/cmd/cluster/context_test.go index d5591fce34..c27080d8ef 100644 --- a/pkg/cli/cmd/cluster/context_test.go +++ b/pkg/cli/cmd/cluster/context_test.go @@ -6,10 +6,12 @@ import ( "github.com/devantler-tech/ksail/v7/pkg/apis/cluster/v1alpha1" "github.com/devantler-tech/ksail/v7/pkg/cli/cmd/cluster" "github.com/devantler-tech/ksail/v7/pkg/cli/setup/localregistry" + clusterprovisioner "github.com/devantler-tech/ksail/v7/pkg/svc/provisioner/cluster" "github.com/k3d-io/k3d/v5/pkg/config/types" v1alpha5 "github.com/k3d-io/k3d/v5/pkg/config/v1alpha5" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "sigs.k8s.io/kind/pkg/apis/config/v1alpha4" ) @@ -55,6 +57,23 @@ func TestResolveClusterNameFromContext_K3s(t *testing.T) { require.Equal(t, "k3s-cluster", name) } +func TestResolveClusterNameFromContext_EKS(t *testing.T) { + t.Parallel() + + ctx := &localregistry.Context{ + ClusterCfg: &v1alpha1.Cluster{ + ObjectMeta: metav1.ObjectMeta{Name: "stale-top-level-name"}, + Spec: v1alpha1.Spec{ + Cluster: v1alpha1.ClusterSpec{Distribution: v1alpha1.DistributionEKS}, + }, + }, + EKSConfig: &clusterprovisioner.EKSConfig{Name: "actual-eks-name"}, + } + + name := cluster.ExportResolveClusterNameFromContext(ctx) + require.Equal(t, "actual-eks-name", name) +} + func TestResolveClusterNameFromContext_FallbackToContext(t *testing.T) { t.Parallel() diff --git a/pkg/cli/cmd/cluster/create.go b/pkg/cli/cmd/cluster/create.go index 1e5c9f5abf..89ad0c68ab 100644 --- a/pkg/cli/cmd/cluster/create.go +++ b/pkg/cli/cmd/cluster/create.go @@ -89,6 +89,11 @@ func handleCreateRunE( return err } + err = validateEKSMutationConfigSource(ctx) + if err != nil { + return err + } + clusterflags.ApplyClusterMutationFlags(cmd, ctx.ClusterCfg) err = validatePostMutationFlags(ctx) @@ -577,15 +582,7 @@ func applyClusterNameOverride(ctx *localregistry.Context, name string) error { return nil } - // Update Kind config - if ctx.KindConfig != nil { - ctx.KindConfig.Name = name - } - - // Update K3d config - if ctx.K3dConfig != nil { - ctx.K3dConfig.Name = name - } + applyDirectClusterNameOverrides(ctx, name) // Update Talos config - must regenerate bundle for new cluster name // because cluster name is embedded in PKI and kubeconfig context @@ -598,16 +595,6 @@ func applyClusterNameOverride(ctx *localregistry.Context, name string) error { ctx.TalosConfig = newConfig } - // Update VCluster config - if ctx.VClusterConfig != nil { - ctx.VClusterConfig.Name = name - } - - // Update KWOK config - if ctx.KWOKConfig != nil { - ctx.KWOKConfig.Name = name - } - // Update the ksail.yaml context to match the pattern the created cluster uses. // Must be provider-aware: the Kubernetes (k3k) provider writes a "k3k-" // context for K3s rather than the standalone "k3d-", so post-creation CNI @@ -625,6 +612,28 @@ func applyClusterNameOverride(ctx *localregistry.Context, name string) error { return nil } +// applyDirectClusterNameOverrides updates in-memory distribution configs whose names directly drive +// creation. Talos is handled separately because renaming it must regenerate its PKI-bearing bundle. +// EKS is deliberately excluded: eksctl creates from the unchanged on-disk eks.yaml, so changing only +// EKSConfig.Name would make later state and deletion target a cluster that was never created. +func applyDirectClusterNameOverrides(ctx *localregistry.Context, name string) { + if ctx.KindConfig != nil { + ctx.KindConfig.Name = name + } + + if ctx.K3dConfig != nil { + ctx.K3dConfig.Name = name + } + + if ctx.VClusterConfig != nil { + ctx.VClusterConfig.Name = name + } + + if ctx.KWOKConfig != nil { + ctx.KWOKConfig.Name = name + } +} + // importCachedImages imports container images from a tar archive to the cluster. // This is called after cluster creation but before component installation to ensure // CNI, CSI, metrics-server, and other components can use pre-loaded images. @@ -690,16 +699,25 @@ func resolveClusterNameFromContext(ctx *localregistry.Context) string { return resolveVClusterName(ctx) case v1alpha1.DistributionKWOK: return resolveKWOKName(ctx) - case v1alpha1.DistributionEKS, v1alpha1.DistributionGKE, v1alpha1.DistributionAKS: - // EKS/GKE/AKS configs are owned by their cloud tooling (eks.yaml/ - // gke.yaml/aks.yaml) and not cached on the local registry context; - // fall back to the cluster-level name. + case v1alpha1.DistributionEKS: + return resolveEKSName(ctx) + case v1alpha1.DistributionGKE, v1alpha1.DistributionAKS: + // GKE/AKS configs are owned by their cloud tooling and not cached on the local registry + // context; fall back to the cluster-level name. return resolveFallbackName(ctx) default: return resolveFallbackName(ctx) } } +func resolveEKSName(ctx *localregistry.Context) string { + if ctx.EKSConfig != nil && strings.TrimSpace(ctx.EKSConfig.Name) != "" { + return strings.TrimSpace(ctx.EKSConfig.Name) + } + + return resolveFallbackName(ctx) +} + func resolveVClusterName(ctx *localregistry.Context) string { if ctx.VClusterConfig != nil && ctx.VClusterConfig.Name != "" { return ctx.VClusterConfig.Name @@ -757,6 +775,11 @@ func maybeWaitForTTL( return nil } + clusterName, err = ttlAutoDeleteTargetName(clusterName, clusterCfg, eksConfig) + if err != nil { + return fmt.Errorf("resolve TTL auto-delete target: %w", err) + } + // Persist TTL for informational display (ksail cluster list / info). saveErr := state.SaveClusterTTL(clusterName, ttl) if saveErr != nil { diff --git a/pkg/cli/cmd/cluster/create_test.go b/pkg/cli/cmd/cluster/create_test.go index 6ab0b616c1..aadbcc9947 100644 --- a/pkg/cli/cmd/cluster/create_test.go +++ b/pkg/cli/cmd/cluster/create_test.go @@ -515,6 +515,79 @@ func TestCreate_NoConfigFile_FlagsOnly(t *testing.T) { require.Contains(t, output, "Create cluster", "output should contain cluster lifecycle text") } +// TestCreate_NonEKSNameOverrideReachesProvisioner guards the shared mutation path: rejecting EKS +// source-name drift must not suppress explicit --name overrides for other distributions. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreate_NonEKSNameOverrideReachesProvisioner(t *testing.T) { + workingDir := t.TempDir() + t.Chdir(workingDir) + + writeFile( + t, + workingDir, + "kind.yaml", + "kind: Cluster\napiVersion: kind.x-k8s.io/v1alpha4\nname: source-kind-name\nnodes: []\n", + ) + writeFile(t, workingDir, "kubeconfig", "apiVersion: v1\nkind: Config\n") + setupMockRegistryBackend(t) + + var createdName string + + restoreFactory := cluster.SetProvisionerFactoryForTests(fakeFactory{createName: &createdName}) + defer restoreFactory() + + cmd := cluster.NewCreateCmd() + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetContext(t.Context()) + cmd.SetArgs([]string{ + "--distribution", "Vanilla", + "--name", "explicit-kind-name", + "--kubeconfig", "./kubeconfig", + }) + + require.NoError(t, cmd.Execute()) + assert.Equal(t, "explicit-kind-name", createdName) +} + +// TestCreateRejectsWhitespaceOnlyName preserves explicit-flag validation: whitespace is not an +// absent override and must not silently fall back to the distribution config target. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreateRejectsWhitespaceOnlyName(t *testing.T) { + workingDir := t.TempDir() + t.Chdir(workingDir) + + writeFile( + t, + workingDir, + "kind.yaml", + "kind: Cluster\napiVersion: kind.x-k8s.io/v1alpha4\nname: source-kind-name\nnodes: []\n", + ) + writeFile(t, workingDir, "kubeconfig", "apiVersion: v1\nkind: Config\n") + setupMockRegistryBackend(t) + + var createdName string + + restoreFactory := cluster.SetProvisionerFactoryForTests(fakeFactory{createName: &createdName}) + defer restoreFactory() + + cmd := cluster.NewCreateCmd() + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + cmd.SetContext(t.Context()) + cmd.SetArgs([]string{ + "--distribution", "Vanilla", + "--name", " ", + "--kubeconfig", "./kubeconfig", + }) + + err := cmd.Execute() + require.ErrorContains(t, err, "invalid cluster name") + assert.Empty(t, createdName) +} + // TestCreate_NoConfigFile_WithComponentFlags verifies that component flags // (e.g. --metrics-server Disabled) are respected when no ksail.yaml exists. // diff --git a/pkg/cli/cmd/cluster/delete.go b/pkg/cli/cmd/cluster/delete.go index 4c431751f9..238827e033 100644 --- a/pkg/cli/cmd/cluster/delete.go +++ b/pkg/cli/cmd/cluster/delete.go @@ -102,6 +102,11 @@ func runDeleteAction( return fmt.Errorf("failed to resolve cluster info: %w", err) } + err = lifecycle.ValidateStandaloneAWSTarget(resolved) + if err != nil { + return fmt.Errorf("validate standalone AWS target: %w", err) + } + // Refuse to destroy a cluster ksail did not provision. When the resolved context is an unmanaged // cluster (a managed cloud cluster, a kubeadm cluster, a colleague's cluster) the guard rejects // here — before any provisioner is created or the cluster is touched — so ksail never accidentally diff --git a/pkg/cli/cmd/cluster/eks_lifecycle_test.go b/pkg/cli/cmd/cluster/eks_lifecycle_test.go index 144369fc51..ef02703456 100644 --- a/pkg/cli/cmd/cluster/eks_lifecycle_test.go +++ b/pkg/cli/cmd/cluster/eks_lifecycle_test.go @@ -1,6 +1,8 @@ package cluster_test import ( + "bytes" + "context" "fmt" "io" "os" @@ -8,7 +10,12 @@ import ( "strings" "testing" + "github.com/devantler-tech/ksail/v7/pkg/apis/cluster/v1alpha1" "github.com/devantler-tech/ksail/v7/pkg/cli/cmd/cluster" + "github.com/devantler-tech/ksail/v7/pkg/cli/lifecycle" + "github.com/devantler-tech/ksail/v7/pkg/fsutil" + clusterprovisioner "github.com/devantler-tech/ksail/v7/pkg/svc/provisioner/cluster" + "github.com/devantler-tech/ksail/v7/pkg/svc/state" "github.com/spf13/cobra" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -59,7 +66,19 @@ if [ "${KSAIL_EKSCTL_FAIL-}" = "$1" ]; then fi if [ "$1 $2" = "get cluster" ]; then - printf '[{"Name":"%s","Region":"ap-southeast-2","EksctlCreated":"True"}]\n' "$KSAIL_EKS_CLUSTER" + if [ "${KSAIL_EKSCTL_CLUSTER_ABSENT-}" = "1" ]; then + printf '[]\n' + exit 0 + fi + discovered_cluster="${KSAIL_EKS_DISCOVERED_CLUSTER:-$KSAIL_EKS_CLUSTER}" + if [ "${KSAIL_EKS_DISCOVERED_REGION+x}" = "x" ]; then + discovered_region="$KSAIL_EKS_DISCOVERED_REGION" + else + discovered_region="ap-southeast-2" + fi + eksctl_created="${KSAIL_EKSCTL_CREATED-True}" + printf '[{"Name":"%s","Region":"%s",' "$discovered_cluster" "$discovered_region" + printf '"EksctlCreated":"%s"}]\n' "$eksctl_created" elif [ "$1 $2" = "get nodegroup" ]; then printf '[{"Cluster":"%s","Name":"workers","Status":"ACTIVE",' "$KSAIL_EKS_CLUSTER" printf '"DesiredCapacity":0,"MinSize":2,"MaxSize":4,"NodeGroupType":"managed"}]\n' @@ -81,6 +100,10 @@ func standaloneEKSLifecycleCases() []standaloneEKSLifecycleCase { extraArgs: []string{"--force"}, expectedCalls: func(clusterName string) []string { return []string{ + fmt.Sprintf( + "get cluster --name %s --output json --region ap-southeast-2", + clusterName, + ), "get cluster --output json --region ap-southeast-2", fmt.Sprintf( "delete cluster --name %s --region ap-southeast-2 --wait", @@ -94,6 +117,10 @@ func standaloneEKSLifecycleCases() []standaloneEKSLifecycleCase { newCommand: cluster.NewStartCmd, expectedCalls: func(clusterName string) []string { return []string{ + fmt.Sprintf( + "get cluster --name %s --output json --region ap-southeast-2", + clusterName, + ), fmt.Sprintf( "get nodegroup --cluster %s --output json --region ap-southeast-2", clusterName, @@ -110,6 +137,10 @@ func standaloneEKSLifecycleCases() []standaloneEKSLifecycleCase { newCommand: cluster.NewStopCmd, expectedCalls: func(clusterName string) []string { return []string{ + fmt.Sprintf( + "get cluster --name %s --output json --region ap-southeast-2", + clusterName, + ), fmt.Sprintf( "get nodegroup --cluster %s --output json --region ap-southeast-2", clusterName, @@ -127,15 +158,17 @@ func standaloneEKSLifecycleCases() []standaloneEKSLifecycleCase { // TestStandaloneEKSLifecycleCommandsRouteToEksctl proves the three standalone // command surfaces reach the existing EKS provisioner, honor the custom AWS // credential mapping, and prefer the configured region environment variable. -// -//nolint:paralleltest // each case changes process environment and working directory func TestStandaloneEKSLifecycleCommandsRouteToEksctl(t *testing.T) { for _, testCase := range standaloneEKSLifecycleCases() { - //nolint:paralleltest // setup changes process environment and working directory t.Run(testCase.name, func(t *testing.T) { clusterName := "ksail-eks-" + testCase.name + "-routing-test-6087" markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + if testCase.name == "start" { + // EksctlCreated is normalized explicitly: case and surrounding whitespace are benign. + t.Setenv("KSAIL_EKSCTL_CREATED", " true ") + } + cmd := testCase.newCommand() args := make([]string, 0, 4+len(testCase.extraArgs)) args = append(args, "--name", clusterName, "--provider", "AWS") @@ -217,62 +250,1233 @@ func TestStandaloneEKSLifecycleCommandsPropagateEksctlErrors(t *testing.T) { } } -func setupStandaloneEKSLifecycleFixture( - t *testing.T, - clusterName string, -) string { - t.Helper() +// TestStandaloneEKSLifecycleCommandsRejectUnmanagedEKSContext verifies a real +// eksctl kubeconfig context is not mistaken for a same-named cluster managed by +// another provider. EKS ownership must be checked with an exact query using the +// resolved AWS credentials and region, and a target absent from that query is refused +// before delete or nodegroup scaling can mutate it. +func TestStandaloneEKSLifecycleCommandsRejectUnmanagedEKSContext(t *testing.T) { + for _, testCase := range standaloneEKSLifecycleCases() { + t.Run(testCase.name, func(t *testing.T) { + clusterName := "ksail-eks-" + testCase.name + "-unmanaged-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + writeStandaloneEKSKubeconfig(t, clusterName, "ap-southeast-2") + t.Setenv("KSAIL_EKSCTL_CLUSTER_ABSENT", "1") - workingDir := t.TempDir() - t.Chdir(workingDir) + cmd := testCase.newCommand() + args := make([]string, 0, 4+len(testCase.extraArgs)) + args = append(args, "--name", clusterName, "--provider", "AWS") + args = append(args, testCase.extraArgs...) + cmd.SetArgs(args) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) - binDir := t.TempDir() - markerPath := filepath.Join(t.TempDir(), "eksctl-calls") - eksctlPath := filepath.Join(binDir, "eksctl") - writeExecutableFixture(t, eksctlPath, standaloneEKSEksctlFixture) + err := cmd.Execute() + require.ErrorIs(t, err, cluster.ErrUnmanagedCluster) + + calls := readStandaloneEKSCalls(t, markerPath) + assert.Equal( + t, + []string{fmt.Sprintf( + "get cluster --name %s --output json --region ap-southeast-2", + clusterName, + )}, + calls, + ) + + for _, call := range calls { + assert.NotContains(t, call, "delete cluster") + assert.NotContains(t, call, "scale nodegroup") + } + + assertParentAWSEnvironmentUnchanged(t) + }) + } +} + +// TestStandaloneEKSLifecycleCommandsRejectConfigRegionForDifferentExplicitName +// verifies an explicit target cannot silently inherit the region from an +// unrelated local eks.yaml. Without an explicit region, every mutating EKS +// lifecycle command must fail before invoking eksctl. +func TestStandaloneEKSLifecycleCommandsRejectConfigRegionForDifferentExplicitName(t *testing.T) { + for _, testCase := range standaloneEKSLifecycleCases() { + t.Run(testCase.name, func(t *testing.T) { + clusterName := "ksail-eks-" + testCase.name + "-region-mismatch-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + require.NoError( + t, + os.WriteFile( + "eks.yaml", + []byte(`apiVersion: eksctl.io/v1alpha5 +kind: ClusterConfig +metadata: + name: different-config-cluster-6087 + region: eu-west-1 +`), + 0o600, + ), + ) + t.Setenv("KSAIL_REGION", "") + + cmd := testCase.newCommand() + args := make([]string, 0, 4+len(testCase.extraArgs)) + args = append(args, "--name", clusterName, "--provider", "AWS") + args = append(args, testCase.extraArgs...) + cmd.SetArgs(args) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "does not match EKS config cluster") + assert.Empty(t, readStandaloneEKSCalls(t, markerPath)) + assertParentAWSEnvironmentUnchanged(t) + }) + } +} + +// TestStandaloneEKSLifecycleCommandsRequireLocalOwnershipEvidence verifies that an explicit AWS +// region and eksctl's generic creation marker are not enough to mutate a colleague's cluster. The +// target must also match a loaded KSail EKS config or have persisted KSail creation state. +func TestStandaloneEKSLifecycleCommandsRequireLocalOwnershipEvidence(t *testing.T) { + for _, testCase := range standaloneEKSLifecycleCases() { + t.Run(testCase.name, func(t *testing.T) { + clusterName := "ksail-eks-" + testCase.name + "-no-local-owner-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("HOME", t.TempDir()) + writeStandaloneEKSEksConfig(t, "unrelated-local-cluster-6087") + + cmd := testCase.newCommand() + args := make([]string, 0, 4+len(testCase.extraArgs)) + args = append(args, "--name", clusterName, "--provider", "AWS") + args = append(args, testCase.extraArgs...) + cmd.SetArgs(args) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.ErrorIs(t, err, cluster.ErrUnmanagedCluster) + assert.Contains(t, err.Error(), "no local KSail ownership evidence") + assert.Empty(t, readStandaloneEKSCalls(t, markerPath)) + assertParentAWSEnvironmentUnchanged(t) + }) + } +} + +// TestStandaloneEKSRejectsSameNamedNonEKSConfig verifies that another distribution's local config +// cannot authorize an AWS mutation merely because it carries the same cluster name. +func TestStandaloneEKSRejectsSameNamedNonEKSConfig(t *testing.T) { + const clusterName = "same-name-kind-and-eks-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("HOME", t.TempDir()) require.NoError( t, os.WriteFile( - filepath.Join(workingDir, "ksail.yaml"), - []byte(standaloneEKSClusterFixture), + "ksail.yaml", + fmt.Appendf(nil, `apiVersion: ksail.io/v1alpha1 +kind: Cluster +metadata: + name: %s +spec: + cluster: + distribution: Vanilla + provider: Docker + distributionConfig: kind.yaml + connection: + kubeconfig: kubeconfig +`, clusterName), 0o600, ), ) - eksConfigPath := filepath.Join(workingDir, "eks.yaml") - require.NoError(t, os.WriteFile(eksConfigPath, []byte(standaloneEKSEksConfigFixture), 0o600)) require.NoError( t, os.WriteFile( - filepath.Join(workingDir, "kubeconfig"), - []byte("apiVersion: v1\nkind: Config\n"), + "kind.yaml", + fmt.Appendf(nil, `kind: Cluster +apiVersion: kind.x-k8s.io/v1alpha4 +name: %s +`, clusterName), + 0o600, + ), + ) + + cmd := cluster.NewStartCmd() + cmd.SetArgs([]string{"--name", clusterName, "--provider", "AWS"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.ErrorIs(t, err, cluster.ErrUnmanagedCluster) + assert.Contains(t, err.Error(), "no local KSail ownership evidence") + assert.Empty(t, readStandaloneEKSCalls(t, markerPath)) +} + +// TestStandaloneEKSRejectsSyntheticNameFromNamelessConfig verifies ConfigManager's fallback +// `eks-default` name is not mistaken for a name explicitly authorized by an actual eks.yaml source. +func TestStandaloneEKSRejectsSyntheticNameFromNamelessConfig(t *testing.T) { + markerPath := setupStandaloneEKSLifecycleFixture(t, "eks-default") + t.Setenv("HOME", t.TempDir()) + require.NoError( + t, + os.WriteFile( + "eks.yaml", + []byte(`apiVersion: eksctl.io/v1alpha5 +kind: ClusterConfig +metadata: + region: ap-southeast-2 +`), 0o600, ), ) + cmd := cluster.NewStartCmd() + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.ErrorIs(t, err, cluster.ErrUnmanagedCluster) + assert.Contains(t, err.Error(), "no local KSail ownership evidence") + assert.Empty(t, readStandaloneEKSCalls(t, markerPath)) +} + +// TestStandaloneEKSRejectsMetadataFromWrongConfigKind verifies arbitrary YAML cannot grant local +// EKS ownership merely by carrying matching metadata. The actual distribution source must be an +// eksctl ClusterConfig before its name or region can reach a lifecycle guard. +// +//nolint:paralleltest // setup mutates process environment and the working directory +func TestStandaloneEKSRejectsMetadataFromWrongConfigKind(t *testing.T) { + testStandaloneEKSLifecycleCommandsRejectConfig( + t, + "wrong-kind-provenance", + func(clusterName string) []byte { + return fmt.Appendf(nil, `apiVersion: v1 +kind: ConfigMap +metadata: + name: %s + region: ap-southeast-2 +`, clusterName) + }, + ) +} + +// TestStandaloneEKSRejectsNonCanonicalConfigName verifies a padded raw source name cannot be +// normalized into local ownership evidence for delete, start, or stop. +// +//nolint:paralleltest // setup mutates process environment and the working directory +func TestStandaloneEKSRejectsNonCanonicalConfigName(t *testing.T) { + testStandaloneEKSLifecycleCommandsRejectConfig( + t, + "noncanonical-name", + func(clusterName string) []byte { + return fmt.Appendf(nil, `apiVersion: eksctl.io/v1alpha5 +kind: ClusterConfig +metadata: + name: "%s " + region: ap-southeast-2 +`, clusterName) + }, + ) +} + +func testStandaloneEKSLifecycleCommandsRejectConfig( + t *testing.T, + testSuffix string, + configContent func(string) []byte, +) { + t.Helper() + + for _, testCase := range standaloneEKSLifecycleCases() { + t.Run(testCase.name, func(t *testing.T) { + clusterName := "ksail-eks-" + testCase.name + "-" + testSuffix + "-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + require.NoError(t, os.WriteFile("eks.yaml", configContent(clusterName), 0o600)) + + cmd := testCase.newCommand() + args := make([]string, 0, 4+len(testCase.extraArgs)) + args = append(args, "--name", clusterName, "--provider", "AWS") + args = append(args, testCase.extraArgs...) + cmd.SetArgs(args) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid EKS config file") + assert.Empty(t, readStandaloneEKSCalls(t, markerPath)) + }) + } +} + +// TestStandaloneEKSLifecycleCommandsIgnoreRegionFromNamelessConfig verifies a local eks.yaml may +// only contribute a region when it also names the target it describes. Persisted state authorizes +// the explicit cluster, while its exact eksctl kubeconfig context supplies that cluster's region; +// borrowing the unrelated nameless file's region would redirect the mutation. +func TestStandaloneEKSLifecycleCommandsIgnoreRegionFromNamelessConfig(t *testing.T) { + for _, testCase := range standaloneEKSLifecycleCases() { + t.Run(testCase.name, func(t *testing.T) { + clusterName := "ksail-eks-" + testCase.name + "-nameless-region-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("HOME", t.TempDir()) + t.Setenv("KSAIL_REGION", "") + + require.NoError( + t, + os.WriteFile( + "eks.yaml", + []byte(`apiVersion: eksctl.io/v1alpha5 +kind: ClusterConfig +metadata: + region: eu-west-1 +`), + 0o600, + ), + ) + writeStandaloneEKSKubeconfig(t, clusterName, "ap-southeast-2") + require.NoError(t, state.SaveClusterSpec(clusterName, &v1alpha1.ClusterSpec{ + Distribution: v1alpha1.DistributionEKS, + Provider: v1alpha1.ProviderAWS, + })) + + cmd := testCase.newCommand() + args := make([]string, 0, 4+len(testCase.extraArgs)) + args = append(args, "--name", clusterName, "--provider", "AWS") + args = append(args, testCase.extraArgs...) + cmd.SetArgs(args) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + require.NoError(t, cmd.Execute()) + assert.Equal( + t, + testCase.expectedCalls(clusterName), + readStandaloneEKSCalls(t, markerPath), + ) + assertParentAWSEnvironmentUnchanged(t) + }) + } +} + +// TestStandaloneEKSStartAcceptsPersistedOwnershipWithoutConfig preserves the flag-only standalone +// workflow: when no project files exist, persisted creation state is sufficient local evidence and +// the exact cloud query still corroborates the name, region, and eksctl provenance before scaling. +func TestStandaloneEKSStartAcceptsPersistedOwnershipWithoutConfig(t *testing.T) { + const clusterName = "ksail-eks-start-state-owned-6087" + + workingDir := t.TempDir() + t.Chdir(workingDir) + t.Setenv("HOME", t.TempDir()) + + binDir := t.TempDir() + markerPath := filepath.Join(t.TempDir(), "eksctl-calls") + writeExecutableFixture(t, filepath.Join(binDir, "eksctl"), standaloneEKSEksctlFixture) + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) t.Setenv("KSAIL_EKSCTL_MARKER", markerPath) t.Setenv("KSAIL_EKS_CLUSTER", clusterName) - t.Setenv("KSAIL_PROFILE", "selected-profile") - t.Setenv("KSAIL_REGION", "ap-southeast-2") - t.Setenv("KSAIL_ACCESS", "fixture-access") - t.Setenv("KSAIL_SECRET", "fixture-secret") - t.Setenv("KSAIL_SESSION", "fixture-session") - t.Setenv("AWS_PROFILE", "stale-profile") - t.Setenv("AWS_REGION", "stale-region") - t.Setenv("AWS_ACCESS_KEY_ID", "stale-access") - t.Setenv("AWS_SECRET_ACCESS_KEY", "stale-secret") - t.Setenv("AWS_SESSION_TOKEN", "stale-session") + t.Setenv("AWS_PROFILE", "selected-profile") + t.Setenv("AWS_REGION", "ap-southeast-2") + t.Setenv("AWS_ACCESS_KEY_ID", "fixture-access") + t.Setenv("AWS_SECRET_ACCESS_KEY", "fixture-secret") + t.Setenv("AWS_SESSION_TOKEN", "fixture-session") - return markerPath + require.NoError(t, state.SaveClusterSpec(clusterName, &v1alpha1.ClusterSpec{ + Distribution: v1alpha1.DistributionEKS, + Provider: v1alpha1.ProviderAWS, + })) + + cmd := cluster.NewStartCmd() + cmd.SetArgs([]string{"--name", clusterName, "--provider", "AWS"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + require.NoError(t, cmd.Execute()) + assert.Equal( + t, + []string{ + fmt.Sprintf("get cluster --name %s --output json", clusterName), + fmt.Sprintf( + "get nodegroup --cluster %s --output json --region ap-southeast-2", + clusterName, + ), + fmt.Sprintf( + "scale nodegroup --cluster %s --name workers --nodes 2 --nodes-min 2 --nodes-max 4 --region ap-southeast-2", + clusterName, + ), + }, + readStandaloneEKSCalls(t, markerPath), + ) } -func assertParentAWSEnvironmentUnchanged(t *testing.T) { +// TestEKSMutationCommandsRejectNameOverrideMismatch proves create and update cannot claim a name +// that eksctl will ignore because the actual create/delete source is eks.yaml. +// +//nolint:paralleltest // each case changes process environment and working directory +func TestEKSMutationCommandsRejectNameOverrideMismatch(t *testing.T) { + testCases := []struct { + name string + newCommand func() *cobra.Command + }{ + {name: "create", newCommand: cluster.NewCreateCmd}, + {name: "update", newCommand: cluster.NewUpdateCmd}, + } + + //nolint:paralleltest // each case changes process environment and working directory + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + const sourceName = "eks-source-mutation-6087" + + markerPath := setupStandaloneEKSLifecycleFixture(t, sourceName) + + cmd := testCase.newCommand() + cmd.SetArgs([]string{"--name", "different-eks-mutation-6087"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot override EKS config cluster") + assert.Empty(t, readStandaloneEKSCalls(t, markerPath)) + }) + } +} + +// TestEKSMutationCommandsRequireNamedConfig verifies create and update reject a valid-shaped but +// nameless eks.yaml before any provisioner or exact ownership call. Eksctl consumes only that file, +// so KSail cannot safely bind creation, recreation, state, or TTL to a synthetic fallback name. +// +//nolint:paralleltest // setup mutates process environment and the working directory +func TestEKSMutationCommandsRequireNamedConfig(t *testing.T) { + testEKSMutationCommandsRejectConfigSource( + t, + `apiVersion: eksctl.io/v1alpha5 +kind: ClusterConfig +metadata: + region: ap-southeast-2 +`, + "EKS config metadata.name is required", + ) +} + +// TestEKSMutationCommandsRejectNonCanonicalConfigName verifies create/update cannot normalize an +// EKS source name for ownership and state while submitting a different raw name to eksctl. That +// split is especially destructive during recreation, where the normalized old target is deleted +// before eksctl consumes the unchanged source. +// +//nolint:paralleltest // setup mutates process environment and the working directory +func TestEKSMutationCommandsRejectNonCanonicalConfigName(t *testing.T) { + testEKSMutationCommandsRejectConfigSource( + t, + `apiVersion: eksctl.io/v1alpha5 +kind: ClusterConfig +metadata: + name: "config-file-name " + region: ap-southeast-2 +`, + "invalid EKS config file", + ) +} + +func testEKSMutationCommandsRejectConfigSource(t *testing.T, eksConfig, wantError string) { t.Helper() - assert.Equal(t, "stale-profile", os.Getenv("AWS_PROFILE")) - assert.Equal(t, "stale-region", os.Getenv("AWS_REGION")) - assert.Equal(t, "stale-access", os.Getenv("AWS_ACCESS_KEY_ID")) - assert.Equal(t, "stale-secret", os.Getenv("AWS_SECRET_ACCESS_KEY")) - assert.Equal(t, "stale-session", os.Getenv("AWS_SESSION_TOKEN")) + testCases := []struct { + name string + newCommand func() *cobra.Command + }{ + {name: "create", newCommand: cluster.NewCreateCmd}, + {name: "update", newCommand: cluster.NewUpdateCmd}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + markerPath := setupStandaloneEKSLifecycleFixture(t, "config-file-name") + require.NoError(t, os.WriteFile("eks.yaml", []byte(eksConfig), 0o600)) + + cmd := testCase.newCommand() + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), wantError) + assert.Empty(t, readStandaloneEKSCalls(t, markerPath)) + }) + } +} + +// TestStandaloneEKSLifecycleCommandsUseEKSConfigNameWhenMetadataDrifts verifies the actual source +// eks.yaml name wins over top-level metadata when no --name flag is given, even when region comes +// from its configured environment variable. +// +//nolint:paralleltest // each case changes process environment and working directory +func TestStandaloneEKSLifecycleCommandsUseEKSConfigNameWhenMetadataDrifts(t *testing.T) { + //nolint:paralleltest // each case changes process environment and working directory + for _, testCase := range standaloneEKSLifecycleCases() { + t.Run(testCase.name, func(t *testing.T) { + clusterName := "eks-source-name-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + require.NoError( + t, + os.WriteFile( + "eks.yaml", + []byte(`apiVersion: eksctl.io/v1alpha5 +kind: ClusterConfig +metadata: + name: eks-source-name-6087 + region: eu-west-1 +`), + 0o600, + ), + ) + + cmd := testCase.newCommand() + cmd.SetArgs(testCase.extraArgs) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + require.NoError(t, cmd.Execute()) + assert.Equal( + t, + testCase.expectedCalls(clusterName), + readStandaloneEKSCalls(t, markerPath), + ) + assertParentAWSEnvironmentUnchanged(t) + }) + } +} + +// TestStandaloneEKSLifecycleCommandsRejectMalformedPresentConfig verifies a present config load +// error cannot discard custom AWS aliases and fall back to ambient credentials for a destructive +// command. A genuinely absent config remains supported by the standalone flag-only path. +// +//nolint:paralleltest // setup changes process environment and working directory +func TestStandaloneEKSLifecycleCommandsRejectMalformedPresentConfig(t *testing.T) { + //nolint:paralleltest // each case changes process environment and working directory + for _, testCase := range standaloneEKSLifecycleCases() { + t.Run(testCase.name, func(t *testing.T) { + clusterName := "ksail-eks-" + testCase.name + "-malformed-config-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + require.NoError(t, os.WriteFile("ksail.yaml", []byte("spec: [\n"), 0o600)) + + cmd := testCase.newCommand() + args := make([]string, 0, 4+len(testCase.extraArgs)) + args = append(args, "--name", clusterName, "--provider", "AWS") + args = append(args, testCase.extraArgs...) + cmd.SetArgs(args) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "load cluster config") + assert.Empty(t, readStandaloneEKSCalls(t, markerPath)) + assertParentAWSEnvironmentUnchanged(t) + }) + } +} + +// TestEKSUpdateRoutesThroughExactAWSOwnershipContext exercises the real update command through its +// pre-mutation guard. It proves the loaded EKS config, credential aliases, and region reach the AWS +// branch instead of the legacy generic discovery path. +func TestEKSUpdateRoutesThroughExactAWSOwnershipContext(t *testing.T) { + const clusterName = "source-config-eks-update-6087" + + setupStandaloneEKSLifecycleFixture(t, "config-file-name") + t.Setenv("KSAIL_REGION", "ap-southeast-2") + require.NoError( + t, + os.WriteFile( + "eks.yaml", + []byte(`apiVersion: eksctl.io/v1alpha5 +kind: ClusterConfig +metadata: + name: source-config-eks-update-6087 + region: eu-west-1 +`), + 0o600, + ), + ) + + var captured *lifecycle.ResolvedClusterInfo + + restore := cluster.ExportSetUpdateUnmanagedGuard( + func(_ context.Context, resolved *lifecycle.ResolvedClusterInfo) error { + captured = resolved + + return cluster.ErrUnmanagedCluster + }, + ) + defer restore() + + cmd := cluster.NewUpdateCmd() + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.ErrorIs(t, err, cluster.ErrUnmanagedCluster) + require.NotNil(t, captured) + assert.Equal(t, clusterName, captured.ClusterName) + assert.Equal(t, clusterName, captured.ConfigClusterName) + assert.Equal(t, v1alpha1.ProviderAWS, captured.Provider) + assert.Equal(t, "ap-southeast-2", captured.AWSRegion) + assert.Equal(t, "KSAIL_PROFILE", captured.AWSOpts.ProfileEnvVar) + assert.Equal(t, "KSAIL_ACCESS", captured.AWSOpts.AccessKeyIDEnvVar) + assert.Equal(t, "KSAIL_SECRET", captured.AWSOpts.SecretAccessKeyEnvVar) + assert.Equal(t, "KSAIL_SESSION", captured.AWSOpts.SessionTokenEnvVar) + + expectedKubeconfig, pathErr := fsutil.EvalCanonicalPath("kubeconfig") + require.NoError(t, pathErr) + assert.Equal(t, expectedKubeconfig, captured.KubeconfigPath) +} + +// TestAutoDeleteEKSUsesCreatedTargetAndCreationRegion verifies TTL cleanup deletes the target +// actually created from eks.yaml, retains its creation-time region even if the environment changes, +// and cleans state for that exact target only. +func TestAutoDeleteEKSUsesCreatedTargetAndCreationRegion(t *testing.T) { + const ( + actualName = "ttl-actual-eks-6087" + staleAlias = "ttl-stale-alias-6087" + ) + + markerPath := setupStandaloneEKSLifecycleFixture(t, actualName) + t.Setenv("HOME", t.TempDir()) + t.Setenv("KSAIL_REGION", "us-east-1") + require.NoError(t, state.SaveClusterSpec(staleAlias, &v1alpha1.ClusterSpec{})) + + clusterCfg := standaloneEKSTTLClusterConfig() + eksConfig := &clusterprovisioner.EKSConfig{ + Name: actualName, + NameFromConfig: true, + Region: "ap-southeast-2", + ConfigPath: "eks.yaml", + KubeconfigPath: "kubeconfig", + } + cmd := &cobra.Command{Use: "ttl-delete"} + + var output bytes.Buffer + + cmd.SetContext(t.Context()) + cmd.SetOut(&output) + cmd.SetErr(&output) + + require.NoError( + t, + cluster.ExportAutoDeleteCluster(cmd, staleAlias, clusterCfg, eksConfig), + ) + assert.Equal( + t, + []string{ + fmt.Sprintf( + "get cluster --name %s --output json --region ap-southeast-2", + actualName, + ), + fmt.Sprintf( + "delete cluster --name %s --region ap-southeast-2 --wait", + actualName, + ), + }, + readStandaloneEKSCalls(t, markerPath), + ) + _, err := state.LoadClusterSpec(actualName) + require.ErrorIs(t, err, state.ErrStateNotFound) + _, err = state.LoadClusterSpec(staleAlias) + require.NoError(t, err) + assert.Contains(t, output.String(), actualName) + assert.NotContains(t, output.String(), staleAlias) + assertParentAWSEnvironmentUnchanged(t) +} + +// TestAutoDeleteEKSFailsClosedWithoutOwnership verifies TTL never constructs a destructive +// provisioner when ownership provenance is absent or the actual EKS target is unavailable. +// +//nolint:paralleltest // subtests mutate process environment and working directory +func TestAutoDeleteEKSFailsClosedWithoutOwnership(t *testing.T) { + //nolint:paralleltest // subtest mutates process environment and working directory + t.Run("unmanaged provenance", testAutoDeleteEKSRejectsUnmanagedProvenance) + //nolint:paralleltest // subtest mutates process environment and working directory + t.Run("missing EKS config", testAutoDeleteEKSRejectsMissingConfig) +} + +func testAutoDeleteEKSRejectsUnmanagedProvenance(t *testing.T) { + const actualName = "ttl-unmanaged-eks-6087" + + markerPath := setupStandaloneEKSLifecycleFixture(t, actualName) + t.Setenv("HOME", t.TempDir()) + t.Setenv("KSAIL_EKSCTL_CREATED", "False") + require.NoError(t, state.SaveClusterSpec(actualName, &v1alpha1.ClusterSpec{})) + + cmd := &cobra.Command{Use: "ttl-delete"} + + var output bytes.Buffer + + cmd.SetContext(t.Context()) + cmd.SetOut(&output) + cmd.SetErr(&output) + + err := cluster.ExportAutoDeleteCluster( + cmd, + "stale-alias", + standaloneEKSTTLClusterConfig(), + &clusterprovisioner.EKSConfig{ + Name: actualName, + NameFromConfig: true, + Region: "ap-southeast-2", + ConfigPath: "eks.yaml", + KubeconfigPath: "kubeconfig", + }, + ) + require.ErrorIs(t, err, cluster.ErrUnmanagedCluster) + assert.Equal( + t, + []string{fmt.Sprintf( + "get cluster --name %s --output json --region ap-southeast-2", + actualName, + )}, + readStandaloneEKSCalls(t, markerPath), + ) + + _, stateErr := state.LoadClusterSpec(actualName) + require.NoError(t, stateErr) + assert.NotContains(t, output.String(), "auto-destroyed") +} + +func testAutoDeleteEKSRejectsMissingConfig(t *testing.T) { + const actualName = "ttl-missing-config-eks-6087" + + markerPath := setupStandaloneEKSLifecycleFixture(t, actualName) + t.Setenv("HOME", t.TempDir()) + require.NoError(t, state.SaveClusterSpec(actualName, &v1alpha1.ClusterSpec{})) + + cmd := &cobra.Command{Use: "ttl-delete"} + + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cluster.ExportAutoDeleteCluster( + cmd, + actualName, + standaloneEKSTTLClusterConfig(), + nil, + ) + require.Error(t, err) + assert.Contains(t, err.Error(), "EKS configuration") + assert.Empty(t, readStandaloneEKSCalls(t, markerPath)) + + _, stateErr := state.LoadClusterSpec(actualName) + require.NoError(t, stateErr) +} + +func standaloneEKSTTLClusterConfig() *v1alpha1.Cluster { + clusterCfg := &v1alpha1.Cluster{} + clusterCfg.Spec.Cluster.Distribution = v1alpha1.DistributionEKS + clusterCfg.Spec.Cluster.Provider = v1alpha1.ProviderAWS + clusterCfg.Spec.Cluster.Connection.Kubeconfig = "kubeconfig" + clusterCfg.Spec.Provider.AWS = v1alpha1.OptionsAWS{ + ProfileEnvVar: "KSAIL_PROFILE", + RegionEnvVar: "KSAIL_REGION", + AccessKeyIDEnvVar: "KSAIL_ACCESS", + SecretAccessKeyEnvVar: "KSAIL_SECRET", + SessionTokenEnvVar: "KSAIL_SESSION", + } + + return clusterCfg +} + +// TestStandaloneEKSLifecycleCommandsFailClosedWhenAWSOwnershipCheckFails +// verifies a failed exact AWS ownership query cannot fall through to a mutating eksctl +// command. The ownership lookup itself is read-only and uses the same resolved +// credential aliases and region as the later lifecycle operation. +func TestStandaloneEKSLifecycleCommandsFailClosedWhenAWSOwnershipCheckFails(t *testing.T) { + for _, testCase := range standaloneEKSLifecycleCases() { + t.Run(testCase.name, func(t *testing.T) { + clusterName := "ksail-eks-" + testCase.name + "-ownership-error-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("KSAIL_EKSCTL_FAIL", "get") + + cmd := testCase.newCommand() + args := make([]string, 0, 4+len(testCase.extraArgs)) + args = append(args, "--name", clusterName, "--provider", "AWS") + args = append(args, testCase.extraArgs...) + cmd.SetArgs(args) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "verify AWS cluster ownership") + assert.Equal( + t, + []string{fmt.Sprintf( + "get cluster --name %s --output json --region ap-southeast-2", + clusterName, + )}, + readStandaloneEKSCalls(t, markerPath), + ) + assertParentAWSEnvironmentUnchanged(t) + }) + } +} + +// TestStandaloneEKSStartRejectsOwnershipRegionMismatch verifies an exact AWS +// lookup that returns the requested name in a different region is a hard stop; +// the command must not continue to nodegroup discovery or scaling. +func TestStandaloneEKSStartRejectsOwnershipRegionMismatch(t *testing.T) { + clusterName := "ksail-eks-start-region-query-mismatch-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("KSAIL_EKS_DISCOVERED_REGION", "us-east-1") + + cmd := cluster.NewStartCmd() + cmd.SetArgs([]string{"--name", clusterName, "--provider", "AWS"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains( + t, + err.Error(), + `exact AWS ownership query returned a different region: got "us-east-1", want "ap-southeast-2"`, + ) + assert.Equal( + t, + []string{fmt.Sprintf( + "get cluster --name %s --output json --region ap-southeast-2", + clusterName, + )}, + readStandaloneEKSCalls(t, markerPath), + ) + assertParentAWSEnvironmentUnchanged(t) +} + +func TestStandaloneEKSStartRejectsOwnershipNameMismatch(t *testing.T) { + clusterName := "ksail-eks-start-name-query-mismatch-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("KSAIL_EKS_DISCOVERED_CLUSTER", "different-visible-cluster") + + cmd := cluster.NewStartCmd() + cmd.SetArgs([]string{"--name", clusterName, "--provider", "AWS"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains( + t, + err.Error(), + `exact AWS ownership query returned a different cluster: got "different-visible-cluster"`, + ) + assert.Equal( + t, + []string{fmt.Sprintf( + "get cluster --name %s --output json --region ap-southeast-2", + clusterName, + )}, + readStandaloneEKSCalls(t, markerPath), + ) + assertParentAWSEnvironmentUnchanged(t) +} + +func TestStandaloneEKSStartUsesUniqueBareContextRegionFallback(t *testing.T) { + clusterName := "ksail-eks-start-context-region-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("KSAIL_REGION", "") + t.Setenv("KSAIL_EKS_DISCOVERED_REGION", "us-west-2") + writeStandaloneEKSEksConfig(t, clusterName) + writeStandaloneEKSKubeconfigContexts( + t, + clusterName, + []string{clusterName + ".us-west-2.eksctl.io"}, + ) + + cmd := cluster.NewStartCmd() + cmd.SetArgs([]string{"--name", clusterName, "--provider", "AWS"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + require.NoError(t, cmd.Execute()) + assert.Equal( + t, + []string{ + fmt.Sprintf( + "get cluster --name %s --output json --region us-west-2", + clusterName, + ), + fmt.Sprintf( + "get nodegroup --cluster %s --output json --region us-west-2", + clusterName, + ), + fmt.Sprintf( + "scale nodegroup --cluster %s --name workers --nodes 2 --nodes-min 2 --nodes-max 4 --region us-west-2", + clusterName, + ), + }, + readStandaloneEKSCalls(t, markerPath), + ) + assertParentAWSEnvironmentUnchanged(t) +} + +func TestStandaloneEKSStartRejectsAmbiguousContextRegions(t *testing.T) { + clusterName := "ksail-eks-start-ambiguous-context-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("KSAIL_REGION", "") + writeStandaloneEKSEksConfig(t, clusterName) + writeStandaloneEKSKubeconfigContexts( + t, + clusterName, + []string{ + "arn:aws:iam::123456789012:role/first@" + clusterName + ".us-east-1.eksctl.io", + clusterName + ".us-west-2.eksctl.io", + }, + ) + + cmd := cluster.NewStartCmd() + cmd.SetArgs([]string{"--name", clusterName, "--provider", "AWS"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "multiple kubeconfig regions") + assert.Empty(t, readStandaloneEKSCalls(t, markerPath)) + assertParentAWSEnvironmentUnchanged(t) +} + +func TestStandaloneEKSStartBindsRegionFromExactQueryWithoutContext(t *testing.T) { + clusterName := "ksail-eks-start-query-region-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("KSAIL_REGION", "") + t.Setenv("KSAIL_EKS_DISCOVERED_REGION", "eu-north-1") + writeStandaloneEKSEksConfig(t, clusterName) + + cmd := cluster.NewStartCmd() + cmd.SetArgs([]string{"--name", clusterName, "--provider", "AWS"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + require.NoError(t, cmd.Execute()) + assert.Equal( + t, + []string{ + fmt.Sprintf("get cluster --name %s --output json", clusterName), + fmt.Sprintf( + "get nodegroup --cluster %s --output json --region eu-north-1", + clusterName, + ), + fmt.Sprintf( + "scale nodegroup --cluster %s --name workers --nodes 2 --nodes-min 2 --nodes-max 4 --region eu-north-1", + clusterName, + ), + }, + readStandaloneEKSCalls(t, markerPath), + ) + assertParentAWSEnvironmentUnchanged(t) +} + +func TestStandaloneEKSStartRejectsEmptyRegionFromExactQuery(t *testing.T) { + clusterName := "ksail-eks-start-empty-query-region-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("KSAIL_REGION", "") + t.Setenv("KSAIL_EKS_DISCOVERED_REGION", "") + writeStandaloneEKSEksConfig(t, clusterName) + + cmd := cluster.NewStartCmd() + cmd.SetArgs([]string{"--name", clusterName, "--provider", "AWS"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.Error(t, err) + assert.Contains(t, err.Error(), "did not report a region") + assert.Equal( + t, + []string{fmt.Sprintf("get cluster --name %s --output json", clusterName)}, + readStandaloneEKSCalls(t, markerPath), + ) + assertParentAWSEnvironmentUnchanged(t) +} + +//nolint:paralleltest // setup mutates process environment and the working directory +func TestStandaloneEKSExplicitRegionWinsContextFallback(t *testing.T) { + clusterName := "ksail-eks-start-explicit-region-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + writeStandaloneEKSKubeconfigContexts( + t, + clusterName, + []string{clusterName + ".us-west-2.eksctl.io"}, + ) + + cmd := cluster.NewStartCmd() + cmd.SetArgs([]string{"--name", clusterName, "--provider", "AWS"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + require.NoError(t, cmd.Execute()) + + for _, call := range readStandaloneEKSCalls(t, markerPath) { + assert.Contains(t, call, "--region ap-southeast-2") + assert.NotContains(t, call, "us-west-2") + } + + assertParentAWSEnvironmentUnchanged(t) +} + +func TestEksctlContextTargetParsing(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + contextName string + wantName string + wantRegion string + wantOK bool + }{ + { + name: "role ARN identity", + contextName: "arn:aws:iam::123456789012:role/operator@demo.eu-west-1.eksctl.io", + wantName: "demo", + wantRegion: "eu-west-1", + wantOK: true, + }, + { + name: "identity containing at signs", + contextName: "user@example.com@demo.us-east-2.eksctl.io", + wantName: "demo", + wantRegion: "us-east-2", + wantOK: true, + }, + { + name: "bare eksctl context", + contextName: "demo.ap-southeast-1.eksctl.io", + wantName: "demo", + wantRegion: "ap-southeast-1", + wantOK: true, + }, + { + name: "similar cluster name remains exact", + contextName: "demo-extra.ap-southeast-1.eksctl.io", + wantName: "demo-extra", + wantRegion: "ap-southeast-1", + wantOK: true, + }, + {name: "missing suffix", contextName: "demo.eu-west-1", wantOK: false}, + {name: "missing region", contextName: "demo.eksctl.io", wantOK: false}, + {name: "malformed dotted region", contextName: "demo.us.east.eksctl.io", wantOK: false}, + {name: "empty target", contextName: ".eksctl.io", wantOK: false}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + name, region, ok := cluster.ExportParseEksctlContextTarget(testCase.contextName) + assert.Equal(t, testCase.wantOK, ok) + assert.Equal(t, testCase.wantName, name) + assert.Equal(t, testCase.wantRegion, region) + }) + } +} + +// TestStandaloneEKSStartRejectsMissingEksctlOwnership verifies the exact AWS +// target is still unmanaged unless eksctl explicitly reports that it created +// the cluster. False, empty, and unknown provenance all stop before scaling. +func TestStandaloneEKSStartRejectsMissingEksctlOwnership(t *testing.T) { + testCases := []struct { + name string + created string + }{ + {name: "false", created: "False"}, + {name: "empty", created: ""}, + {name: "unknown", created: "unknown"}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + clusterName := "ksail-eks-start-" + testCase.name + "-ownership-test-6087" + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("KSAIL_EKSCTL_CREATED", testCase.created) + + cmd := cluster.NewStartCmd() + cmd.SetArgs([]string{"--name", clusterName, "--provider", "AWS"}) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + err := cmd.Execute() + require.ErrorIs(t, err, cluster.ErrUnmanagedCluster) + assert.Contains(t, err.Error(), "did not report an eksctl-created cluster") + assert.Equal( + t, + []string{fmt.Sprintf( + "get cluster --name %s --output json --region ap-southeast-2", + clusterName, + )}, + readStandaloneEKSCalls(t, markerPath), + ) + assertParentAWSEnvironmentUnchanged(t) + }) + } +} + +func setupStandaloneEKSLifecycleFixture( + t *testing.T, + clusterName string, +) string { + t.Helper() + + workingDir := t.TempDir() + t.Chdir(workingDir) + + binDir := t.TempDir() + markerPath := filepath.Join(t.TempDir(), "eksctl-calls") + eksctlPath := filepath.Join(binDir, "eksctl") + writeExecutableFixture(t, eksctlPath, standaloneEKSEksctlFixture) + + require.NoError( + t, + os.WriteFile( + filepath.Join(workingDir, "ksail.yaml"), + []byte(standaloneEKSClusterFixture), + 0o600, + ), + ) + eksConfigPath := filepath.Join(workingDir, "eks.yaml") + eksConfig := strings.ReplaceAll(standaloneEKSEksConfigFixture, "config-file-name", clusterName) + require.NoError(t, os.WriteFile(eksConfigPath, []byte(eksConfig), 0o600)) + require.NoError( + t, + os.WriteFile( + filepath.Join(workingDir, "kubeconfig"), + []byte("apiVersion: v1\nkind: Config\n"), + 0o600, + ), + ) + + t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) + t.Setenv("KSAIL_EKSCTL_MARKER", markerPath) + t.Setenv("KSAIL_EKS_CLUSTER", clusterName) + t.Setenv("KSAIL_PROFILE", "selected-profile") + t.Setenv("KSAIL_REGION", "ap-southeast-2") + t.Setenv("KSAIL_ACCESS", "fixture-access") + t.Setenv("KSAIL_SECRET", "fixture-secret") + t.Setenv("KSAIL_SESSION", "fixture-session") + t.Setenv("AWS_PROFILE", "stale-profile") + t.Setenv("AWS_REGION", "stale-region") + t.Setenv("AWS_ACCESS_KEY_ID", "stale-access") + t.Setenv("AWS_SECRET_ACCESS_KEY", "stale-secret") + t.Setenv("AWS_SESSION_TOKEN", "stale-session") + + return markerPath +} + +func assertParentAWSEnvironmentUnchanged(t *testing.T) { + t.Helper() + + assert.Equal(t, "stale-profile", os.Getenv("AWS_PROFILE")) + assert.Equal(t, "stale-region", os.Getenv("AWS_REGION")) + assert.Equal(t, "stale-access", os.Getenv("AWS_ACCESS_KEY_ID")) + assert.Equal(t, "stale-secret", os.Getenv("AWS_SECRET_ACCESS_KEY")) + assert.Equal(t, "stale-session", os.Getenv("AWS_SESSION_TOKEN")) +} + +func writeStandaloneEKSKubeconfig(t *testing.T, clusterName, region string) { + t.Helper() + + contextName := fmt.Sprintf( + "arn:aws:iam::123456789012:role/ksail-test@%s.%s.eksctl.io", + clusterName, + region, + ) + writeStandaloneEKSKubeconfigContexts(t, clusterName, []string{contextName}) +} + +func writeStandaloneEKSKubeconfigContexts( + t *testing.T, + clusterName string, + contextNames []string, +) { + t.Helper() + + var contexts strings.Builder + for _, contextName := range contextNames { + fmt.Fprintf( + &contexts, + "- name: %s\n context:\n cluster: %s\n user: eksctl-user\n", + contextName, + clusterName, + ) + } + + currentContext := "" + if len(contextNames) > 0 { + currentContext = contextNames[0] + } + + kubeconfig := fmt.Sprintf(`apiVersion: v1 +kind: Config +clusters: +- name: %s + cluster: + server: https://example.invalid +contexts: +%s +current-context: %s +users: +- name: eksctl-user + user: {} +`, clusterName, contexts.String(), currentContext) + + require.NoError(t, os.WriteFile("kubeconfig", []byte(kubeconfig), 0o600)) +} + +func writeStandaloneEKSEksConfig(t *testing.T, clusterName string) { + t.Helper() + + config := fmt.Sprintf(`apiVersion: eksctl.io/v1alpha5 +kind: ClusterConfig +metadata: + name: %s +`, clusterName) + + require.NoError(t, os.WriteFile("eks.yaml", []byte(config), 0o600)) +} + +func readStandaloneEKSCalls(t *testing.T, markerPath string) []string { + t.Helper() + + calls, err := os.ReadFile(markerPath) //nolint:gosec // test-private path + if os.IsNotExist(err) { + return nil + } + + require.NoError(t, err) + + trimmed := strings.TrimSpace(string(calls)) + if trimmed == "" { + return nil + } + + return strings.Split(trimmed, "\n") } diff --git a/pkg/cli/cmd/cluster/export_test.go b/pkg/cli/cmd/cluster/export_test.go index 304925291b..47c15e4950 100644 --- a/pkg/cli/cmd/cluster/export_test.go +++ b/pkg/cli/cmd/cluster/export_test.go @@ -107,6 +107,11 @@ func ExportEnsureClusterManaged( ) } +// ExportParseEksctlContextTarget exports parseEksctlContextTarget for testing. +func ExportParseEksctlContextTarget(contextName string) (string, string, bool) { + return parseEksctlContextTarget(contextName) +} + // ExportResolveCreatedContextName exports resolveCreatedContextName for testing. func ExportResolveCreatedContextName( distribution v1alpha1.Distribution, @@ -302,6 +307,16 @@ func ExportMaybeWaitForTTL( return maybeWaitForTTL(cmd, clusterName, clusterCfg, nil) } +// ExportAutoDeleteCluster exports autoDeleteCluster for offline TTL safety tests. +func ExportAutoDeleteCluster( + cmd *cobra.Command, + clusterName string, + clusterCfg *v1alpha1.Cluster, + eksConfig *clusterprovisioner.EKSConfig, +) error { + return autoDeleteCluster(cmd, clusterName, clusterCfg, eksConfig) +} + // ExportNormalizeVersionTag exposes normalizeVersionTag for testing. func ExportNormalizeVersionTag(tag string) string { return normalizeVersionTag(tag) @@ -621,6 +636,7 @@ func ExportGuardUpdateTargetManaged( ctx context.Context, clusterCfg *v1alpha1.Cluster, clusterName string, + eksConfig *clusterprovisioner.EKSConfig, ) error { - return guardUpdateTargetManaged(ctx, clusterCfg, clusterName) + return guardUpdateTargetManaged(ctx, clusterCfg, clusterName, eksConfig) } diff --git a/pkg/cli/cmd/cluster/mutation.go b/pkg/cli/cmd/cluster/mutation.go index 7ebf8d8b07..7bf0433d40 100644 --- a/pkg/cli/cmd/cluster/mutation.go +++ b/pkg/cli/cmd/cluster/mutation.go @@ -23,10 +23,13 @@ import ( ) var ( - errEKSConfigurationUnavailable = errors.New("EKS configuration is unavailable") - errEKSClusterNameRequired = errors.New("cluster name is required") - errNoMatchingEKSContext = errors.New("no kubeconfig context matches EKS cluster") - errAmbiguousEKSContext = errors.New("multiple kubeconfig contexts match EKS cluster") + errEKSConfigurationUnavailable = errors.New("EKS configuration is unavailable") + errEKSClusterNameRequired = errors.New("cluster name is required") + errEKSMutationConfigNameNeeded = errors.New("EKS config metadata.name is required") + errEKSMutationConfigNameInvalid = errors.New("EKS config metadata.name is invalid") + errEKSNameOverrideMismatch = errors.New("cannot override EKS config cluster name") + errNoMatchingEKSContext = errors.New("no kubeconfig context matches EKS cluster") + errAmbiguousEKSContext = errors.New("multiple kubeconfig contexts match EKS cluster") ) const explicitEKSContextHint = "set spec.cluster.connection.context explicitly" @@ -134,18 +137,12 @@ func loadAndValidateClusterConfig( return nil, "", err } - // Apply cluster name override: --name flag takes priority, then metadata.name - nameOverride := cfgManager.Viper.GetString("name") - if nameOverride == "" { - nameOverride = ctx.ClusterCfg.Name + nameOverride, err := resolveMutationNameOverride(cfgManager, ctx) + if err != nil { + return nil, "", err } if nameOverride != "" { - validationErr := v1alpha1.ValidateClusterName(nameOverride) - if validationErr != nil { - return nil, "", fmt.Errorf("invalid cluster name %q: %w", nameOverride, validationErr) - } - err = applyClusterNameOverride(ctx, nameOverride) if err != nil { return nil, "", err @@ -180,6 +177,105 @@ func loadAndValidateClusterConfig( return ctx, clusterName, nil } +// resolveMutationNameOverride validates the flag/metadata name priority before any distribution +// config is mutated. EKS is the special case: eksctl consumes the name in the on-disk eks.yaml, so +// an explicit contradiction must fail rather than be silently ignored in memory. +func resolveMutationNameOverride( + cfgManager *ksailconfigmanager.ConfigManager, + ctx *localregistry.Context, +) (string, error) { + explicitName := cfgManager.Viper.GetString("name") + if explicitName != "" { + err := validateMutationClusterName(explicitName) + if err != nil { + return "", err + } + + err = validateEKSNameOverride(ctx, explicitName) + if err != nil { + return "", err + } + + return explicitName, nil + } + + metadataName := ctx.ClusterCfg.Name + if metadataName == "" { + return "", nil + } + + return metadataName, validateMutationClusterName(metadataName) +} + +func validateMutationClusterName(name string) error { + err := v1alpha1.ValidateClusterName(name) + if err != nil { + return fmt.Errorf("invalid cluster name %q: %w", name, err) + } + + return nil +} + +// validateEKSMutationConfigSource requires create/update to bind the target to the actual name in +// the eksctl source. Standalone lifecycle commands may instead use persisted KSail state, but an EKS +// create or recreation cannot safely infer the target because the provisioner passes only this file +// to eksctl and deliberately ignores the action's in-memory name argument. +func validateEKSMutationConfigSource(ctx *localregistry.Context) error { + if ctx.ClusterCfg.Spec.Cluster.Distribution != v1alpha1.DistributionEKS { + return nil + } + + if ctx.EKSConfig == nil || strings.TrimSpace(ctx.EKSConfig.ConfigPath) == "" { + return fmt.Errorf( + "%w: provide a named eksctl ClusterConfig source", + errEKSConfigurationUnavailable, + ) + } + + sourceName := ctx.EKSConfig.Name + + canonicalName := strings.TrimSpace(sourceName) + if !ctx.EKSConfig.NameFromConfig || canonicalName == "" { + return fmt.Errorf( + "%w in %s", + errEKSMutationConfigNameNeeded, + ctx.EKSConfig.ConfigPath, + ) + } + + if sourceName != canonicalName { + return fmt.Errorf( + "%w: %q in %s has leading or trailing whitespace", + errEKSMutationConfigNameInvalid, + sourceName, + ctx.EKSConfig.ConfigPath, + ) + } + + err := v1alpha1.ValidateClusterName(sourceName) + if err != nil { + return fmt.Errorf("%w: %w", errEKSMutationConfigNameInvalid, err) + } + + return nil +} + +func validateEKSNameOverride(ctx *localregistry.Context, explicitName string) error { + if ctx.ClusterCfg.Spec.Cluster.Distribution != v1alpha1.DistributionEKS || + ctx.EKSConfig == nil || strings.TrimSpace(ctx.EKSConfig.ConfigPath) == "" || + explicitName == strings.TrimSpace(ctx.EKSConfig.Name) { + return nil + } + + return fmt.Errorf( + "%w: --name %q differs from %q in %s", + errEKSNameOverrideMismatch, + explicitName, + strings.TrimSpace(ctx.EKSConfig.Name), + ctx.EKSConfig.ConfigPath, + ) +} + // runClusterCreationWorkflow performs the full cluster creation workflow. // This is the shared implementation used by both the create handler and // the update command's recreate flow. @@ -327,11 +423,12 @@ func prepareEKSOutputKubeconfigPath(kubeconfigPath string) (string, error) { // prefixes EKS contexts with the AWS identity that created the cluster. func resolvePostCreateContext(ctx *localregistry.Context) error { connection := &ctx.ClusterCfg.Spec.Cluster.Connection + distribution := ctx.ClusterCfg.Spec.Cluster.Distribution + if connection.Context != "" { return nil } - distribution := ctx.ClusterCfg.Spec.Cluster.Distribution if distribution != v1alpha1.DistributionEKS { connection.Context = resolveCreatedContextName( distribution, @@ -369,10 +466,27 @@ func resolveEKSPostCreateContext(ctx *localregistry.Context) error { } ctx.ClusterCfg.Spec.Cluster.Connection.Context = selected + pinEKSRegionFromContext(ctx, selected) return nil } +// pinEKSRegionFromContext preserves the exact region eksctl selected after a create whose region was +// delegated to the AWS profile. TTL and later lifecycle calls can then target that created region +// directly even when the kubeconfig also contains a same-named cluster elsewhere. +func pinEKSRegionFromContext(ctx *localregistry.Context, contextName string) { + if ctx.EKSConfig == nil || strings.TrimSpace(ctx.EKSConfig.Region) != "" { + return + } + + clusterName, region, ok := parseEksctlContextTarget(contextName) + if !ok || clusterName != strings.TrimSpace(ctx.EKSConfig.Name) { + return + } + + ctx.EKSConfig.Region = region +} + // loadEKSContextConfig loads the kubeconfig plus the cluster and effective // region needed to select the identity-qualified eksctl context. func loadEKSContextConfig( diff --git a/pkg/cli/cmd/cluster/post_create_context_test.go b/pkg/cli/cmd/cluster/post_create_context_test.go index d4b6f06b25..18f39ade49 100644 --- a/pkg/cli/cmd/cluster/post_create_context_test.go +++ b/pkg/cli/cmd/cluster/post_create_context_test.go @@ -153,12 +153,16 @@ func TestResolvePostCreateContext_EKSMissingConfigFailsClosed(t *testing.T) { func TestResolvePostCreateContext_EKSRegionDelegatedToProfile(t *testing.T) { t.Parallel() - const eksctlContext = "arn:aws:iam::123456789012:role/ci@st-eks.eu-west-1.eksctl.io" + const ( + eksctlContext = "arn:aws:iam::123456789012:role/ci@st-eks.eu-west-1.eksctl.io" + otherRegionContext = "arn:aws:iam::123456789012:role/ci@st-eks.us-east-1.eksctl.io" + ) kubeconfigPath := filepath.Join(t.TempDir(), "kubeconfig") config := clientcmdapi.NewConfig() config.CurrentContext = eksctlContext config.Contexts[eksctlContext] = &clientcmdapi.Context{} + config.Contexts[otherRegionContext] = &clientcmdapi.Context{} require.NoError(t, clientcmd.WriteToFile(*config, kubeconfigPath)) ctx := &localregistry.Context{ @@ -177,9 +181,34 @@ func TestResolvePostCreateContext_EKSRegionDelegatedToProfile(t *testing.T) { require.NoError(t, cluster.ExportResolvePostCreateContext(ctx)) assert.Equal(t, eksctlContext, ctx.ClusterCfg.Spec.Cluster.Connection.Context) + assert.Equal(t, "eu-west-1", ctx.EKSConfig.Region) +} + +func TestResolvePostCreateContext_EKSExplicitContextDoesNotPinUnobservedRegion(t *testing.T) { + t.Parallel() + + const explicitContext = "arn:aws:iam::123456789012:role/ci@st-eks.eu-west-1.eksctl.io" + + ctx := &localregistry.Context{ + ClusterCfg: &v1alpha1.Cluster{ + Spec: v1alpha1.Spec{ + Cluster: v1alpha1.ClusterSpec{ + Distribution: v1alpha1.DistributionEKS, + Connection: v1alpha1.Connection{ + Context: explicitContext, + }, + }, + }, + }, + EKSConfig: &clusterprovisioner.EKSConfig{Name: "st-eks"}, + } + + require.NoError(t, cluster.ExportResolvePostCreateContext(ctx)) + assert.Equal(t, explicitContext, ctx.ClusterCfg.Spec.Cluster.Connection.Context) + assert.Empty(t, ctx.EKSConfig.Region) } -func TestApplyClusterNameOverride_EKSDefersContextResolution(t *testing.T) { +func TestApplyClusterNameOverride_EKSPreservesSourceConfigAndDefersContextResolution(t *testing.T) { t.Parallel() testCases := []struct { @@ -205,9 +234,11 @@ func TestApplyClusterNameOverride_EKSDefersContextResolution(t *testing.T) { }, }, }, + EKSConfig: &clusterprovisioner.EKSConfig{Name: "old-eks-name"}, } require.NoError(t, cluster.ExportApplyClusterNameOverride(ctx, "st-eks")) + assert.Equal(t, "old-eks-name", ctx.EKSConfig.Name) assert.Equal( t, testCase.originalContext, diff --git a/pkg/cli/cmd/cluster/testhelpers_test.go b/pkg/cli/cmd/cluster/testhelpers_test.go index 290292d72c..1e411a953e 100644 --- a/pkg/cli/cmd/cluster/testhelpers_test.go +++ b/pkg/cli/cmd/cluster/testhelpers_test.go @@ -28,9 +28,15 @@ import ( // disableTraefikArg is the K3s argument to disable the built-in Traefik ingress controller. const disableTraefikArg = "--disable=traefik" -type fakeProvisioner struct{} +type fakeProvisioner struct{ createName *string } -func (*fakeProvisioner) Create(context.Context, string) error { return nil } +func (p *fakeProvisioner) Create(_ context.Context, name string) error { + if p.createName != nil { + *p.createName = name + } + + return nil +} func (*fakeProvisioner) Delete(context.Context, string) error { return nil } @@ -44,15 +50,15 @@ func (*fakeProvisioner) List(context.Context) ([]string, error) { func (*fakeProvisioner) Exists(context.Context, string) (bool, error) { return true, nil } -type fakeFactory struct{} +type fakeFactory struct{ createName *string } -func (fakeFactory) Create( +func (f fakeFactory) Create( _ context.Context, _ *v1alpha1.Cluster, ) (clusterprovisioner.Provisioner, any, error) { cfg := &v1alpha4.Cluster{Name: "test"} - return &fakeProvisioner{}, cfg, nil + return &fakeProvisioner{createName: f.createName}, cfg, nil } type fakeInstaller struct{ called bool } diff --git a/pkg/cli/cmd/cluster/ttl.go b/pkg/cli/cmd/cluster/ttl.go index 0d32a23b70..290254e818 100644 --- a/pkg/cli/cmd/cluster/ttl.go +++ b/pkg/cli/cmd/cluster/ttl.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "os/signal" + "strings" "syscall" "time" @@ -60,28 +61,22 @@ func autoDeleteCluster( clusterCfg *v1alpha1.Cluster, eksConfig *clusterprovisioner.EKSConfig, ) error { + clusterName, err := ttlAutoDeleteTargetName(clusterName, clusterCfg, eksConfig) + if err != nil { + return fmt.Errorf("TTL auto-delete: resolve target: %w", err) + } + notify.Infof(cmd.OutOrStdout(), "TTL expired; auto-destroying cluster %q...", clusterName) - info := &clusterdetector.Info{ - ClusterName: clusterName, - Distribution: clusterCfg.Spec.Cluster.Distribution, - Provider: clusterCfg.Spec.Cluster.Provider, + info, options, err := ttlDeleteProvisionerInputs( + cmd.Context(), clusterName, clusterCfg, eksConfig, + ) + if err != nil { + return err } - provisioner, err := createDeleteProvisioner( - cmd.Context(), - info, - lifecycle.MinimalProvisionerOptions{ - OmniOpts: clusterCfg.Spec.Provider.Omni, - KubernetesOpts: clusterCfg.Spec.Provider.Kubernetes, - AWSOpts: clusterCfg.Spec.Provider.AWS, - AWSRegion: lifecycle.ResolveAWSRegion( - clusterCfg.Spec.Provider.AWS, - &clusterprovisioner.DistributionConfig{EKS: eksConfig}, - ), - }, - ) + provisioner, err := createDeleteProvisioner(cmd.Context(), info, options) if err != nil { return fmt.Errorf("TTL auto-delete: failed to create provisioner: %w", err) } @@ -107,3 +102,78 @@ func autoDeleteCluster( return nil } + +func ttlDeleteProvisionerInputs( + ctx context.Context, + clusterName string, + clusterCfg *v1alpha1.Cluster, + eksConfig *clusterprovisioner.EKSConfig, +) (*clusterdetector.Info, lifecycle.MinimalProvisionerOptions, error) { + info := &clusterdetector.Info{ + ClusterName: clusterName, + Distribution: clusterCfg.Spec.Cluster.Distribution, + Provider: clusterCfg.Spec.Cluster.Provider, + KubeconfigPath: clusterCfg.Spec.Cluster.Connection.Kubeconfig, + } + options := lifecycle.MinimalProvisionerOptions{ + OmniOpts: clusterCfg.Spec.Provider.Omni, + KubernetesOpts: clusterCfg.Spec.Provider.Kubernetes, + AWSOpts: clusterCfg.Spec.Provider.AWS, + } + + if clusterCfg.Spec.Cluster.Distribution == v1alpha1.DistributionEKS { + if strings.TrimSpace(eksConfig.KubeconfigPath) != "" { + info.KubeconfigPath = strings.TrimSpace(eksConfig.KubeconfigPath) + } + + resolved := &lifecycle.ResolvedClusterInfo{ + ClusterName: clusterName, + ConfigClusterName: clusterName, + EKSConfigSource: eksConfig.NameFromConfig && + strings.TrimSpace(eksConfig.ConfigPath) != "", + Provider: v1alpha1.ProviderAWS, + KubeconfigPath: info.KubeconfigPath, + AWSOpts: clusterCfg.Spec.Provider.AWS, + // EKSConfig.Region was pinned before creation. Do not re-read a mutable region + // environment variable after the TTL wait. + AWSRegion: strings.TrimSpace(eksConfig.Region), + } + + err := ensureAWSClusterManaged(ctx, resolved) + if err != nil { + return nil, lifecycle.MinimalProvisionerOptions{}, fmt.Errorf( + "TTL auto-delete: verify EKS ownership: %w", + err, + ) + } + + eksConfig.Region = resolved.AWSRegion + options.AWSRegion = resolved.AWSRegion + } + + return info, options, nil +} + +// ttlAutoDeleteTargetName returns the exact target the provisioner created. EKS creation consumes +// EKSConfig.Name rather than the lifecycle action's name argument, so TTL cleanup must use that same +// immutable identity and fail closed when it is unavailable. +func ttlAutoDeleteTargetName( + clusterName string, + clusterCfg *v1alpha1.Cluster, + eksConfig *clusterprovisioner.EKSConfig, +) (string, error) { + if clusterCfg == nil || clusterCfg.Spec.Cluster.Distribution != v1alpha1.DistributionEKS { + return clusterName, nil + } + + if eksConfig == nil { + return "", errEKSConfigurationUnavailable + } + + actualName := strings.TrimSpace(eksConfig.Name) + if actualName == "" { + return "", errEKSClusterNameRequired + } + + return actualName, nil +} diff --git a/pkg/cli/cmd/cluster/unmanaged_guard.go b/pkg/cli/cmd/cluster/unmanaged_guard.go index 63d7f70c49..ce3a03cf22 100644 --- a/pkg/cli/cmd/cluster/unmanaged_guard.go +++ b/pkg/cli/cmd/cluster/unmanaged_guard.go @@ -4,12 +4,19 @@ import ( "context" "errors" "fmt" + "os" + "sort" + "strings" v1alpha1 "github.com/devantler-tech/ksail/v7/pkg/apis/cluster/v1alpha1" "github.com/devantler-tech/ksail/v7/pkg/cli/lifecycle" + eksctlclient "github.com/devantler-tech/ksail/v7/pkg/client/eksctl" "github.com/devantler-tech/ksail/v7/pkg/fsutil" "github.com/devantler-tech/ksail/v7/pkg/svc/clusterdiscovery" + "github.com/devantler-tech/ksail/v7/pkg/svc/credentials" clusterdetector "github.com/devantler-tech/ksail/v7/pkg/svc/detector/cluster" + clusterprovisioner "github.com/devantler-tech/ksail/v7/pkg/svc/provisioner/cluster" + "github.com/devantler-tech/ksail/v7/pkg/svc/state" ) // ErrUnmanagedCluster indicates a ksail-only lifecycle action (start/stop/…) was attempted against a @@ -18,6 +25,24 @@ import ( // lifecycle actions are refused (ksail#5885, part of the unmanaged-cluster surface epic #5654). var ErrUnmanagedCluster = errors.New("cluster is not managed by ksail") +var ( + errAWSOwnershipNameMismatch = errors.New( + "exact AWS ownership query returned a different cluster", + ) + errAWSOwnershipRegionMismatch = errors.New( + "exact AWS ownership query returned a different region", + ) + errAWSOwnershipRegionMissing = errors.New( + "exact AWS ownership query did not report a region", + ) + errAWSOwnershipRegionAmbiguous = errors.New( + "multiple kubeconfig regions match EKS cluster", + ) + errAWSOwnershipProvenanceMissing = errors.New( + "exact AWS ownership query did not report an eksctl-created cluster", + ) +) + // managedClusterLister enumerates the names of clusters ksail manages across every provider and // reports whether discovery was complete (no provider failed). The guard fails open when discovery // is incomplete so a transient provider error (e.g. a stopped Docker daemon) never wrongly refuses a @@ -84,13 +109,36 @@ func ensureClusterManaged( // context↔name mapping stays defined in exactly one place. Best-effort: a missing/unreadable // kubeconfig yields false (treated as "not an unmanaged cluster, just absent"). func kubeconfigHasClusterContext(kubeconfigPath, clusterName string) bool { - config := clusterdiscovery.LoadKubeconfig(kubeconfigPath) + return kubeconfigHasResolvedClusterContext(&lifecycle.ResolvedClusterInfo{ + ClusterName: clusterName, + KubeconfigPath: kubeconfigPath, + }) +} + +// kubeconfigHasResolvedClusterContext extends the shared prefix-based context mapping with the +// suffix-shaped eksctl context formats: @..eksctl.io and +// ..eksctl.io. +func kubeconfigHasResolvedClusterContext(resolved *lifecycle.ResolvedClusterInfo) bool { + config := clusterdiscovery.LoadKubeconfig(resolved.KubeconfigPath) if config == nil { return false } - matchesClusterName := func(name string) bool { return name == clusterName } + matchesClusterName := func(name string) bool { return name == resolved.ClusterName } + for contextName := range config.Contexts { + if resolved.Provider == v1alpha1.ProviderAWS { + if eksctlContextMatchesCluster( + contextName, + resolved.ClusterName, + resolved.AWSRegion, + ) { + return true + } + + continue + } + if clusterdiscovery.ContextIsManaged(contextName, matchesClusterName) { return true } @@ -99,9 +147,253 @@ func kubeconfigHasClusterContext(kubeconfigPath, clusterName string) bool { return false } -// unmanagedClusterGuard is the SimpleLifecycleConfig.Guard shared by the start and stop commands: it -// rejects an action on a cluster ksail did not provision, using the real cross-provider discoverer. +// parseEksctlContextTarget recognizes both identity-qualified and bare eksctl kubeconfig contexts. +// Parsing from the last @ keeps IAM identities opaque; parsing the target at its last dot avoids +// prefix matches between different cluster names. +func parseEksctlContextTarget(contextName string) (string, string, bool) { + target := contextName + if identityEnd := strings.LastIndex(target, "@"); identityEnd >= 0 { + target = target[identityEnd+1:] + } + + target, found := strings.CutSuffix(target, ".eksctl.io") + if !found { + return "", "", false + } + + regionStart := strings.LastIndex(target, ".") + if regionStart <= 0 || regionStart == len(target)-1 { + return "", "", false + } + + clusterName := target[:regionStart] + region := target[regionStart+1:] + + if strings.Contains(clusterName, ".") || strings.Contains(region, ".") { + return "", "", false + } + + return clusterName, region, true +} + +// eksctlContextMatchesCluster recognizes eksctl's real kubeconfig context shape without treating a +// mere substring as ownership proof. When the resolved region is known it must match exactly. +func eksctlContextMatchesCluster(contextName, clusterName, region string) bool { + contextCluster, contextRegion, ok := parseEksctlContextTarget(contextName) + if !ok || contextCluster != clusterName { + return false + } + + return region == "" || contextRegion == region +} + +// bindAWSRegionFromKubeconfig uses a context region only as a fallback when neither the configured +// environment variable nor eks.yaml supplied one. More than one exact-name region is ambiguous and +// must stop before even the read-only ownership query chooses an AWS profile default. +func bindAWSRegionFromKubeconfig(resolved *lifecycle.ResolvedClusterInfo) error { + if resolved.AWSRegion != "" { + return nil + } + + config := clusterdiscovery.LoadKubeconfig(resolved.KubeconfigPath) + if config == nil { + return nil + } + + regions := make(map[string]struct{}) + + for contextName := range config.Contexts { + clusterName, region, ok := parseEksctlContextTarget(contextName) + if ok && clusterName == resolved.ClusterName { + regions[region] = struct{}{} + } + } + + if len(regions) == 0 { + return nil + } + + if len(regions) == 1 { + for region := range regions { + resolved.AWSRegion = region + } + + return nil + } + + regionNames := make([]string, 0, len(regions)) + for region := range regions { + regionNames = append(regionNames, region) + } + + sort.Strings(regionNames) + + return fmt.Errorf( + "%w %q (%s); set an explicit AWS region", + errAWSOwnershipRegionAmbiguous, + resolved.ClusterName, + strings.Join(regionNames, ", "), + ) +} + +// ensureAWSClusterManaged queries the exact name and region through eksctl under the resolved AWS +// credential aliases. Unlike broad discovery (which may silently skip an unavailable provider), +// every tool, credential, command, and parse error fails closed before a destructive operation. +func ensureAWSClusterManaged( + ctx context.Context, + resolved *lifecycle.ResolvedClusterInfo, +) error { + if !hasLocalKSailEKSTargetEvidence(resolved) { + return fmt.Errorf( + "no local KSail ownership evidence for EKS target %q: %w", + resolved.ClusterName, + unmanagedClusterError(resolved.ClusterName), + ) + } + + err := bindAWSRegionFromKubeconfig(resolved) + if err != nil { + return err + } + + auth := credentials.ResolveAWS(credentials.NewAWSOptionsResolver(resolved.AWSOpts)) + eksctlOptions := credentials.OptionsForAWSChildEnvironment( + auth, + os.Environ(), + eksctlclient.WithEnvironment, + eksctlclient.RequireCredentialValues, + ) + + summary, err := eksctlclient.NewClient(eksctlOptions...).GetCluster( + ctx, + resolved.ClusterName, + resolved.AWSRegion, + ) + if err != nil { + if errors.Is(err, eksctlclient.ErrClusterNotFound) { + return awsOwnershipMismatchError(resolved, err) + } + + return awsOwnershipQueryError(resolved, err) + } + + return validateAWSOwnershipSummary(resolved, summary) +} + +// hasLocalKSailEKSTargetEvidence requires local KSail intent before the cloud-side eksctl marker is +// consulted. Matching an actual loaded eks.yaml authorizes config-backed commands; persisted EKS/AWS +// creation state preserves standalone --name/--provider operation when project files are absent. A +// kubeconfig context or EksctlCreated=True alone never qualifies. These name-based records do not +// bind an immutable AWS account/cluster instance; ksail#6202 tracks that separate hardening. +func hasLocalKSailEKSTargetEvidence(resolved *lifecycle.ResolvedClusterInfo) bool { + if resolved.EKSConfigSource && strings.TrimSpace(resolved.ConfigClusterName) != "" && + strings.TrimSpace(resolved.ConfigClusterName) == strings.TrimSpace(resolved.ClusterName) { + return true + } + + spec, err := state.LoadClusterSpec(resolved.ClusterName) + if err != nil { + return false + } + + return spec.Distribution == v1alpha1.DistributionEKS && spec.Provider == v1alpha1.ProviderAWS +} + +// validateAWSOwnershipSummary requires the exact query to corroborate name, region, and eksctl +// provenance. Every mismatch is a pre-mutation blocker. +func validateAWSOwnershipSummary( + resolved *lifecycle.ResolvedClusterInfo, + summary *eksctlclient.ClusterSummary, +) error { + if summary.Name != resolved.ClusterName { + return awsOwnershipMismatchError( + resolved, + fmt.Errorf( + "%w: got %q, want %q", + errAWSOwnershipNameMismatch, + summary.Name, + resolved.ClusterName, + ), + ) + } + + if resolved.AWSRegion != "" && summary.Region != resolved.AWSRegion { + return awsOwnershipMismatchError( + resolved, + fmt.Errorf( + "%w: got %q, want %q", + errAWSOwnershipRegionMismatch, + summary.Region, + resolved.AWSRegion, + ), + ) + } + + if resolved.AWSRegion == "" && strings.TrimSpace(summary.Region) == "" { + return awsOwnershipQueryError(resolved, errAWSOwnershipRegionMissing) + } + + if !strings.EqualFold(strings.TrimSpace(summary.EKSCTLCreated), "true") { + return fmt.Errorf( + "AWS ownership query returned an unmanaged target: %w", + errors.Join( + fmt.Errorf( + "%w: got EksctlCreated=%q", + errAWSOwnershipProvenanceMissing, + summary.EKSCTLCreated, + ), + unmanagedClusterError(resolved.ClusterName), + ), + ) + } + + if resolved.AWSRegion == "" { + resolved.AWSRegion = summary.Region + } + + return nil +} + +// awsOwnershipMismatchError classifies an absent/mismatched exact result as unmanaged only when a +// strictly parsed eksctl context corroborates the requested name and region. A context identity is +// never treated as ownership proof. +func awsOwnershipMismatchError(resolved *lifecycle.ResolvedClusterInfo, cause error) error { + if kubeconfigHasResolvedClusterContext(resolved) { + return fmt.Errorf( + "AWS ownership query did not return the kubeconfig target: %w", + errors.Join(cause, unmanagedClusterError(resolved.ClusterName)), + ) + } + + return awsOwnershipQueryError(resolved, cause) +} + +// awsOwnershipQueryError preserves tool, credential, command, and parse failures as fail-closed +// diagnostics. Those failures do not prove a kubeconfig target is unmanaged; they only prove the +// destructive command cannot establish ownership safely. +func awsOwnershipQueryError(resolved *lifecycle.ResolvedClusterInfo, cause error) error { + return fmt.Errorf( + "verify AWS cluster ownership for %q: %w", + resolved.ClusterName, + cause, + ) +} + +func unmanagedClusterError(clusterName string) error { + return fmt.Errorf( + "%q is an unmanaged cluster: %w; read-only operations (list, resource browsing, logs, exec) still work", + clusterName, + ErrUnmanagedCluster, + ) +} + +// unmanagedClusterGuard is the SimpleLifecycleConfig.Guard shared by start and stop. AWS uses the +// exact fail-closed ownership query; other providers retain the existing cross-provider guard. func unmanagedClusterGuard(ctx context.Context, resolved *lifecycle.ResolvedClusterInfo) error { + if resolved.Provider == v1alpha1.ProviderAWS { + return ensureAWSClusterManaged(ctx, resolved) + } + return ensureClusterManaged(ctx, resolved, discoverManagedClusters) } @@ -122,6 +414,7 @@ func guardUpdateTargetManaged( ctx context.Context, clusterCfg *v1alpha1.Cluster, clusterName string, + eksConfig *clusterprovisioner.EKSConfig, ) error { kubeconfigPath, err := clusterdetector.ResolveKubeconfigPath( clusterCfg.Spec.Cluster.Connection.Kubeconfig, @@ -138,8 +431,59 @@ func guardUpdateTargetManaged( return fmt.Errorf("canonicalize kubeconfig path %q: %w", kubeconfigPath, err) } - return updateUnmanagedGuardFunc(ctx, &lifecycle.ResolvedClusterInfo{ + resolved := resolveUpdateTarget(clusterCfg, clusterName, canonical, eksConfig) + + err = lifecycle.ValidateStandaloneAWSTarget(resolved) + if err != nil { + return fmt.Errorf("validate AWS update target: %w", err) + } + + err = updateUnmanagedGuardFunc(ctx, resolved) + if err != nil { + return err + } + + // A regionless eks.yaml may be safely bound by an exact kubeconfig context or the exact eksctl + // query. Preserve that verified region so the subsequent update/recreate cannot fall back to a + // different ambient/default AWS region. + if eksConfig != nil && strings.TrimSpace(resolved.AWSRegion) != "" { + eksConfig.Region = strings.TrimSpace(resolved.AWSRegion) + } + + return nil +} + +func resolveUpdateTarget( + clusterCfg *v1alpha1.Cluster, + clusterName, canonicalKubeconfig string, + eksConfig *clusterprovisioner.EKSConfig, +) *lifecycle.ResolvedClusterInfo { + resolved := &lifecycle.ResolvedClusterInfo{ ClusterName: clusterName, - KubeconfigPath: canonical, - }) + Provider: clusterCfg.Spec.Cluster.Provider, + KubeconfigPath: canonicalKubeconfig, + AWSOpts: clusterCfg.Spec.Provider.AWS, + } + if eksConfig == nil { + return resolved + } + + configuredAWSRegion := strings.TrimSpace(eksConfig.Region) + effectiveAWSRegion := lifecycle.ResolveAWSRegion( + clusterCfg.Spec.Provider.AWS, + &clusterprovisioner.DistributionConfig{EKS: eksConfig}, + ) + + // The exact verified name/region drives both in-place operations and recreation deletion. Pin it + // into the distribution config before either later path constructs its provisioner. + eksConfig.Region = effectiveAWSRegion + resolved.ConfigClusterName = strings.TrimSpace(eksConfig.Name) + resolved.EKSConfigSource = eksConfig.NameFromConfig && + strings.TrimSpace(eksConfig.ConfigPath) != "" && + resolved.ConfigClusterName != "" + resolved.AWSRegion = effectiveAWSRegion + resolved.AWSRegionFromConfig = effectiveAWSRegion != "" && + effectiveAWSRegion == configuredAWSRegion + + return resolved } diff --git a/pkg/cli/cmd/cluster/unmanaged_guard_test.go b/pkg/cli/cmd/cluster/unmanaged_guard_test.go index 30957c6416..fd099c9330 100644 --- a/pkg/cli/cmd/cluster/unmanaged_guard_test.go +++ b/pkg/cli/cmd/cluster/unmanaged_guard_test.go @@ -8,6 +8,7 @@ import ( "github.com/devantler-tech/ksail/v7/pkg/apis/cluster/v1alpha1" "github.com/devantler-tech/ksail/v7/pkg/cli/cmd/cluster" "github.com/devantler-tech/ksail/v7/pkg/cli/lifecycle" + clusterprovisioner "github.com/devantler-tech/ksail/v7/pkg/svc/provisioner/cluster" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -153,7 +154,9 @@ func TestGuardUpdateTargetManaged_UnmanagedRejected(t *testing.T) { clusterCfg := &v1alpha1.Cluster{} clusterCfg.Spec.Cluster.Connection.Kubeconfig = kubeconfigPath - err := cluster.ExportGuardUpdateTargetManaged(context.Background(), clusterCfg, "my-cluster") + err := cluster.ExportGuardUpdateTargetManaged( + context.Background(), clusterCfg, "my-cluster", nil, + ) require.ErrorIs(t, err, cluster.ErrUnmanagedCluster) require.Contains(t, err.Error(), "my-cluster") @@ -180,6 +183,91 @@ func TestGuardUpdateTargetManaged_ManagedAllows(t *testing.T) { require.NoError( t, - cluster.ExportGuardUpdateTargetManaged(context.Background(), clusterCfg, "my-cluster"), + cluster.ExportGuardUpdateTargetManaged( + context.Background(), clusterCfg, "my-cluster", nil, + ), + ) +} + +// TestGuardUpdateTargetManaged_EKSPreservesAWSOwnershipContext verifies update reaches the exact +// fail-closed AWS guard rather than the legacy cross-provider discovery path. The credential aliases +// and explicit region used by the later EKS update must be identical at the ownership boundary. +func TestGuardUpdateTargetManaged_EKSPreservesAWSOwnershipContext(t *testing.T) { + t.Setenv("KSAIL_UPDATE_REGION", "") + + clusterCfg := &v1alpha1.Cluster{} + clusterCfg.Spec.Cluster.Distribution = v1alpha1.DistributionEKS + clusterCfg.Spec.Cluster.Provider = v1alpha1.ProviderAWS + clusterCfg.Spec.Cluster.Connection.Kubeconfig = writeKubeconfigWithContext( + t, + t.TempDir(), + "update-eks.eu-north-1.eksctl.io", + ) + //nolint:gosec // these strings name test-only environment variables, not credentials + clusterCfg.Spec.Provider.AWS = v1alpha1.OptionsAWS{ + ProfileEnvVar: "KSAIL_UPDATE_PROFILE", + RegionEnvVar: "KSAIL_UPDATE_REGION", + AccessKeyIDEnvVar: "KSAIL_UPDATE_ACCESS", + SecretAccessKeyEnvVar: "KSAIL_UPDATE_SECRET", + SessionTokenEnvVar: "KSAIL_UPDATE_SESSION", + } + eksConfig := &clusterprovisioner.EKSConfig{ + Name: "update-eks", + NameFromConfig: true, + Region: "eu-north-1", + } + + var captured *lifecycle.ResolvedClusterInfo + + restore := cluster.ExportSetUpdateUnmanagedGuard( + func(_ context.Context, resolved *lifecycle.ResolvedClusterInfo) error { + captured = resolved + + return nil + }, + ) + defer restore() + + require.NoError( + t, + cluster.ExportGuardUpdateTargetManaged( + context.Background(), clusterCfg, "update-eks", eksConfig, + ), + ) + require.NotNil(t, captured) + assert.Equal(t, v1alpha1.ProviderAWS, captured.Provider) + assert.Equal(t, "eu-north-1", captured.AWSRegion) + assert.Equal(t, clusterCfg.Spec.Provider.AWS, captured.AWSOpts) +} + +// TestGuardUpdateTargetManaged_EKSPinsRegionBoundByOwnershipGuard verifies a region discovered by +// the exact guard is copied into the distribution config consumed by the later update/recreate +// provisioner. Validation must never bind a region and then discard it before mutation. +func TestGuardUpdateTargetManaged_EKSPinsRegionBoundByOwnershipGuard(t *testing.T) { + t.Setenv("KSAIL_UPDATE_REGION", "") + + clusterCfg := &v1alpha1.Cluster{} + clusterCfg.Spec.Cluster.Distribution = v1alpha1.DistributionEKS + clusterCfg.Spec.Cluster.Provider = v1alpha1.ProviderAWS + clusterCfg.Spec.Cluster.Connection.Kubeconfig = filepath.Join(t.TempDir(), "missing-kubeconfig") + clusterCfg.Spec.Provider.AWS.RegionEnvVar = "KSAIL_UPDATE_REGION" + eksConfig := &clusterprovisioner.EKSConfig{Name: "update-eks"} + + restore := cluster.ExportSetUpdateUnmanagedGuard( + func(_ context.Context, resolved *lifecycle.ResolvedClusterInfo) error { + assert.Empty(t, resolved.AWSRegion) + resolved.AWSRegion = "eu-north-1" + + return nil + }, + ) + defer restore() + + require.NoError( + t, + cluster.ExportGuardUpdateTargetManaged( + context.Background(), clusterCfg, "update-eks", eksConfig, + ), ) + assert.Equal(t, "eu-north-1", eksConfig.Region) } diff --git a/pkg/cli/cmd/cluster/update.go b/pkg/cli/cmd/cluster/update.go index 299b18b893..6a300b3d94 100644 --- a/pkg/cli/cmd/cluster/update.go +++ b/pkg/cli/cmd/cluster/update.go @@ -142,11 +142,16 @@ func handleUpdateRunE( return err } + err = validateEKSMutationConfigSource(ctx) + if err != nil { + return err + } + // Refuse to reconcile configuration to a cluster ksail did not provision. When the target is an // unmanaged cluster (a managed cloud cluster, a kubeadm cluster, a colleague's cluster) the guard // rejects here — before any change is computed or applied — so `cluster update` never mutates a // cluster ksail does not own. Read-only operations still work. (ksail#5885, epic #5654.) - err = guardUpdateTargetManaged(cmd.Context(), ctx.ClusterCfg, clusterName) + err = guardUpdateTargetManaged(cmd.Context(), ctx.ClusterCfg, clusterName, ctx.EKSConfig) if err != nil { return err } diff --git a/pkg/cli/lifecycle/awsregion_test.go b/pkg/cli/lifecycle/awsregion_test.go index b2ecd409cf..c7cae443f1 100644 --- a/pkg/cli/lifecycle/awsregion_test.go +++ b/pkg/cli/lifecycle/awsregion_test.go @@ -18,7 +18,11 @@ import ( // so it must not run in parallel. func TestResolveAWSRegion(t *testing.T) { eksDistCfg := &clusterprovisioner.DistributionConfig{ - EKS: &clusterprovisioner.EKSConfig{Region: "eu-west-1"}, + EKS: &clusterprovisioner.EKSConfig{ + Name: "configured-eks", + NameFromConfig: true, + Region: "eu-west-1", + }, } t.Run("env var overrides eks.yaml", func(t *testing.T) { @@ -50,6 +54,16 @@ func TestResolveAWSRegion(t *testing.T) { got := lifecycle.ResolveAWSRegion(v1alpha1.OptionsAWS{}, nil) assert.Empty(t, got) }) + + t.Run("ignores region from nameless eks config", func(t *testing.T) { + t.Setenv("AWS_REGION", "") + + namelessConfig := &clusterprovisioner.DistributionConfig{ + EKS: &clusterprovisioner.EKSConfig{Region: "eu-west-1"}, + } + got := lifecycle.ResolveAWSRegion(v1alpha1.OptionsAWS{}, namelessConfig) + assert.Empty(t, got) + }) } // TestResolveClusterInfoRetainsAWSCredentialMappings verifies lifecycle diff --git a/pkg/cli/lifecycle/simple.go b/pkg/cli/lifecycle/simple.go index 8ab0ebc509..b41a739154 100644 --- a/pkg/cli/lifecycle/simple.go +++ b/pkg/cli/lifecycle/simple.go @@ -32,6 +32,9 @@ var ( ErrClusterNameRequired = errors.New( "cluster name is required: use --name flag, create a ksail.yaml config, or set a kubeconfig context", ) + // ErrAWSTargetConfigMismatch indicates that a resolved EKS target would inherit the region + // from a local EKS config describing a different cluster. + ErrAWSTargetConfigMismatch = errors.New("resolved EKS target does not match local config") ) const ( @@ -111,16 +114,23 @@ func BindNameAndProviderFlags( // ResolvedClusterInfo contains the resolved cluster name, provider, and kubeconfig path. type ResolvedClusterInfo struct { - ClusterName string - Provider v1alpha1.Provider - KubeconfigPath string - OmniOpts v1alpha1.OptionsOmni - KubernetesOpts v1alpha1.OptionsKubernetes - // AWSRegion is the resolved AWS region for read-only EKS status lookups. + ClusterName string + ConfigClusterName string + // EKSConfigSource reports that ConfigClusterName came from an actual loaded eks.yaml, rather + // than another distribution config or a synthetic fallback. + EKSConfigSource bool + Provider v1alpha1.Provider + KubeconfigPath string + OmniOpts v1alpha1.OptionsOmni + KubernetesOpts v1alpha1.OptionsKubernetes + // AWSRegion is the resolved AWS region for EKS operations. // Empty defers region resolution to eksctl (AWS_REGION env / active profile). AWSRegion string - // AWSOpts retains the credential environment-variable mappings from the - // loaded cluster config for read-only EKS status lookups. + // AWSRegionFromConfig reports that AWSRegion came from the loaded EKS config rather than the + // configured region environment variable. Mutating standalone commands use this provenance to + // avoid applying one config's region to a different resolved target. + AWSRegionFromConfig bool + // AWSOpts retains the credential environment-variable mappings from the loaded cluster config. AWSOpts v1alpha1.OptionsAWS } @@ -138,20 +148,33 @@ func ResolveAWSRegion( awsOpts v1alpha1.OptionsAWS, distCfg *clusterprovisioner.DistributionConfig, ) string { + region, _ := resolveAWSRegion(awsOpts, distCfg) + + return region +} + +// resolveAWSRegion returns the resolved region together with whether it came from eks.yaml. +func resolveAWSRegion( + awsOpts v1alpha1.OptionsAWS, + distCfg *clusterprovisioner.DistributionConfig, +) (string, bool) { envVar := awsOpts.RegionEnvVar if envVar == "" { envVar = awsRegionEnvVarDefault } if region := os.Getenv(envVar); region != "" { - return region + return region, false } - if distCfg != nil && distCfg.EKS != nil { - return distCfg.EKS.Region + // A declarative region is safe to reuse only when the same actual eks.yaml also named the + // cluster it describes. ConfigManager can load a nameless file and synthesize a target name; + // treating that file's region as target-bound could redirect a standalone mutation. + if distCfg != nil && distCfg.EKS != nil && distCfg.EKS.NameFromConfig { + return distCfg.EKS.Region, distCfg.EKS.Region != "" } - return "" + return "", false } // ResolveClusterInfo resolves the cluster name, provider, and kubeconfig from flags, config, or kubeconfig. @@ -173,8 +196,12 @@ func ResolveClusterInfo( } // Always load config to fill missing fields and extract provider options. - // Even when --name is provided, provider-specific settings are still needed. - resolveFromConfig(cmd, &resolved) + // Even when --name is provided, provider-specific settings are still needed. A present config + // that cannot be loaded must fail closed rather than silently dropping credential aliases. + err := resolveFromConfig(cmd, &resolved) + if err != nil { + return nil, err + } // Fall back to kubeconfig context detection if resolved.ClusterName == "" { @@ -204,26 +231,36 @@ func ResolveClusterInfo( return &resolved, nil } -// loadConfig loads the ksail.yaml config, honoring the --config flag when cmd is non-nil. -// Returns nil if no config is found or loading fails. -func loadConfig(cmd *cobra.Command) (*v1alpha1.Cluster, *clusterprovisioner.DistributionConfig) { +// loadConfig loads the ksail.yaml config, honoring the --config flag when cmd is non-nil. A +// genuinely absent default config is a supported standalone-command case; a present, explicit, or +// discovered config that cannot be loaded is returned as an error so destructive callers fail +// closed instead of falling back to ambient provider credentials. +func loadConfig( + cmd *cobra.Command, +) (*v1alpha1.Cluster, *clusterprovisioner.DistributionConfig, error) { var configFile string if cmd != nil { cfgPath, err := flags.GetConfigPath(cmd) - if err == nil { - configFile = cfgPath + if err != nil { + return nil, nil, fmt.Errorf("resolve config path: %w", err) } + + configFile = cfgPath } cfgManager := ksailconfigmanager.NewConfigManager(nil, configFile) cfg, err := cfgManager.Load(configmanager.LoadOptions{Silent: true, SkipValidation: true}) - if err != nil || cfg == nil || !cfgManager.IsConfigFileFound() { - return nil, nil + if err != nil { + return nil, nil, fmt.Errorf("load cluster config: %w", err) + } + + if cfg == nil || !cfgManager.IsConfigFileFound() { + return nil, nil, nil } - return cfg, cfgManager.DistributionConfig + return cfg, cfgManager.DistributionConfig, nil } // resolveFromConfig fills missing cluster info and provider options from the ksail.yaml config. @@ -232,21 +269,43 @@ func loadConfig(cmd *cobra.Command) (*v1alpha1.Cluster, *clusterprovisioner.Dist func resolveFromConfig( cmd *cobra.Command, resolved *ResolvedClusterInfo, -) { - cfg, distCfg := loadConfig(cmd) - if cfg == nil { - return +) error { + cfg, distCfg, err := loadConfig(cmd) + if err != nil { + return err } - if resolved.ClusterName == "" && cfg.Name != "" { - if v1alpha1.ValidateClusterName(cfg.Name) == nil { - resolved.ClusterName = cfg.Name - } + if cfg == nil { + return nil } - if resolved.ClusterName == "" { - resolved.ClusterName = ClusterNameFromDistributionConfig(distCfg) - } + resolveClusterIdentityFromConfig(resolved, cfg, distCfg) + + resolved.OmniOpts = cfg.Spec.Provider.Omni + resolved.KubernetesOpts = cfg.Spec.Provider.Kubernetes + resolved.AWSOpts = cfg.Spec.Provider.AWS + resolved.AWSRegion, resolved.AWSRegionFromConfig = resolveAWSRegion( + cfg.Spec.Provider.AWS, + distCfg, + ) + + return nil +} + +func resolveClusterIdentityFromConfig( + resolved *ResolvedClusterInfo, + cfg *v1alpha1.Cluster, + distCfg *clusterprovisioner.DistributionConfig, +) { + resolved.ConfigClusterName = resolveConfigClusterName(cfg, distCfg) + resolved.EKSConfigSource = hasLoadedEKSConfigSource(cfg, distCfg, resolved.ConfigClusterName) + resolved.ClusterName = resolveClusterNameFromConfig( + resolved.ClusterName, + resolved.ConfigClusterName, + resolved.EKSConfigSource, + cfg, + distCfg, + ) if resolved.Provider == "" && cfg.Spec.Cluster.Provider != "" { resolved.Provider = cfg.Spec.Cluster.Provider @@ -255,11 +314,95 @@ func resolveFromConfig( if resolved.KubeconfigPath == "" && cfg.Spec.Cluster.Connection.Kubeconfig != "" { resolved.KubeconfigPath = cfg.Spec.Cluster.Connection.Kubeconfig } +} - resolved.OmniOpts = cfg.Spec.Provider.Omni - resolved.KubernetesOpts = cfg.Spec.Provider.Kubernetes - resolved.AWSOpts = cfg.Spec.Provider.AWS - resolved.AWSRegion = ResolveAWSRegion(cfg.Spec.Provider.AWS, distCfg) +func hasLoadedEKSConfigSource( + cfg *v1alpha1.Cluster, + distCfg *clusterprovisioner.DistributionConfig, + configClusterName string, +) bool { + return cfg.Spec.Cluster.Distribution == v1alpha1.DistributionEKS && + distCfg != nil && distCfg.EKS != nil && + distCfg.EKS.NameFromConfig && + strings.TrimSpace(distCfg.EKS.ConfigPath) != "" && + strings.TrimSpace(configClusterName) != "" +} + +func resolveClusterNameFromConfig( + currentName, configClusterName string, + eksConfigSource bool, + cfg *v1alpha1.Cluster, + distCfg *clusterprovisioner.DistributionConfig, +) string { + if currentName != "" { + return currentName + } + + // eksctl creates and deletes the name encoded in the actual eks.yaml source. ConfigManager may + // synthesize "eks-default" when the file is absent, so only grant this priority when ConfigPath + // proved the source exists. + if eksConfigSource { + return configClusterName + } + + if cfg.Name != "" && v1alpha1.ValidateClusterName(cfg.Name) == nil { + return cfg.Name + } + + return ClusterNameFromDistributionConfig(distCfg) +} + +// resolveConfigClusterName returns the distribution config's name when available because it is the +// source of any fallback AWS region. The top-level metadata name is the safe fallback. +func resolveConfigClusterName( + cfg *v1alpha1.Cluster, + distCfg *clusterprovisioner.DistributionConfig, +) string { + if cfg.Spec.Cluster.Distribution == v1alpha1.DistributionEKS && + distCfg != nil && distCfg.EKS != nil { + if strings.TrimSpace(distCfg.EKS.ConfigPath) == "" || !distCfg.EKS.NameFromConfig { + return "" + } + + return strings.TrimSpace(distCfg.EKS.Name) + } + + if name := ClusterNameFromDistributionConfig(distCfg); name != "" { + return name + } + + if v1alpha1.ValidateClusterName(cfg.Name) == nil { + return cfg.Name + } + + return "" +} + +// ValidateStandaloneAWSTarget rejects the ambiguous destructive case where the resolved target +// differs from the cluster described by the local EKS config while the only resolved region came +// from that config. Requiring the configured region environment variable makes the target pair +// deliberate and prevents delete/start/stop from mutating a same-named cluster in the wrong region. +func ValidateStandaloneAWSTarget(resolved *ResolvedClusterInfo) error { + if resolved == nil || resolved.Provider != v1alpha1.ProviderAWS || + !resolved.AWSRegionFromConfig || + resolved.ConfigClusterName == "" || resolved.ConfigClusterName == resolved.ClusterName { + return nil + } + + regionEnvVar := resolved.AWSOpts.RegionEnvVar + if regionEnvVar == "" { + regionEnvVar = awsRegionEnvVarDefault + } + + return fmt.Errorf( + "resolved EKS target %q does not match EKS config cluster %q: "+ + "refusing to reuse region %q from that config; set %s to select the target explicitly: %w", + resolved.ClusterName, + resolved.ConfigClusterName, + resolved.AWSRegion, + regionEnvVar, + ErrAWSTargetConfigMismatch, + ) } // commandContext returns cmd's context, falling back to context.Background() @@ -308,6 +451,11 @@ func runSimpleLifecycleAction( return err } + err = ValidateStandaloneAWSTarget(resolved) + if err != nil { + return err + } + if config.Guard != nil { err = config.Guard(cmd.Context(), resolved) if err != nil { diff --git a/pkg/cli/lifecycle/simple_additional_test.go b/pkg/cli/lifecycle/simple_additional_test.go index f93e67641c..b6b0ffb061 100644 --- a/pkg/cli/lifecycle/simple_additional_test.go +++ b/pkg/cli/lifecycle/simple_additional_test.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "errors" + "os" "testing" "github.com/devantler-tech/ksail/v7/pkg/apis/cluster/v1alpha1" @@ -67,6 +68,46 @@ func TestResolveClusterInfo(t *testing.T) { }) } +// TestResolveClusterInfoFallsBackToDistributionName verifies an intentionally empty metadata.name +// does not suppress the name carried by the distribution config. +// +//nolint:paralleltest // changes the process working directory +func TestResolveClusterInfoFallsBackToDistributionName(t *testing.T) { + workingDir := t.TempDir() + t.Chdir(workingDir) + + require.NoError( + t, + os.WriteFile( + "ksail.yaml", + []byte(`apiVersion: ksail.io/v1alpha1 +kind: Cluster +spec: + cluster: + distribution: Vanilla + provider: Docker + distributionConfig: kind.yaml +`), + 0o600, + ), + ) + require.NoError( + t, + os.WriteFile( + "kind.yaml", + []byte(`kind: Cluster +apiVersion: kind.x-k8s.io/v1alpha4 +name: distribution-only-name +`), + 0o600, + ), + ) + + resolved, err := lifecycle.ResolveClusterInfo(nil, "", "", "/non-existent-kubeconfig") + require.NoError(t, err) + assert.Equal(t, "distribution-only-name", resolved.ClusterName) +} + // TestNewSimpleLifecycleCmd tests the NewSimpleLifecycleCmd function. func TestNewSimpleLifecycleCmd(t *testing.T) { t.Parallel() diff --git a/pkg/fsutil/configmanager/ksail/distribution.go b/pkg/fsutil/configmanager/ksail/distribution.go index 2f7c29f390..82e325e5ee 100644 --- a/pkg/fsutil/configmanager/ksail/distribution.go +++ b/pkg/fsutil/configmanager/ksail/distribution.go @@ -28,6 +28,8 @@ import ( "sigs.k8s.io/yaml" ) +var errInvalidEKSConfig = errors.New("invalid EKS config file") + // loadKindConfig loads the Kind distribution configuration if it exists. // Returns ErrDistributionConfigNotFound if the file doesn't exist. // Returns error if config loading or validation fails. @@ -677,14 +679,17 @@ func (m *ConfigManager) cacheEKSConfig() error { return err } + nameFromConfig := strings.TrimSpace(name) != "" + if name == "" { name = m.resolveEKSNameFromContext() } m.DistributionConfig.EKS = &clusterprovisioner.EKSConfig{ - Name: name, - Region: region, - ConfigPath: resolvedPath, + Name: name, + NameFromConfig: nameFromConfig, + Region: region, + ConfigPath: resolvedPath, } return nil @@ -693,7 +698,9 @@ func (m *ConfigManager) cacheEKSConfig() error { // readEKSConfigMetadata reads the eksctl config file at configPath (if it // exists) and returns its canonical path plus the parsed metadata.name / // metadata.region fields. A missing file returns empty strings and no error -// so callers can fall back to context-based defaults. +// so callers can fall back to context-based defaults. A present file must be +// the supported eksctl ClusterConfig shape before its metadata can establish +// lifecycle ownership provenance. func readEKSConfigMetadata(configPath string) (string, string, string, error) { _, err := os.Stat(configPath) if err != nil { @@ -715,7 +722,9 @@ func readEKSConfigMetadata(configPath string) (string, string, string, error) { } var meta struct { - Metadata struct { + APIVersion string `json:"apiVersion"` + Kind string `json:"kind"` + Metadata struct { Name string `json:"name"` Region string `json:"region"` } `json:"metadata"` @@ -726,7 +735,43 @@ func readEKSConfigMetadata(configPath string) (string, string, string, error) { return "", "", "", fmt.Errorf("failed to parse EKS config file: %w", err) } - return canonical, meta.Metadata.Name, meta.Metadata.Region, nil + const ( + eksConfigAPIVersion = "eksctl.io/v1alpha5" + eksConfigKind = "ClusterConfig" + ) + + if meta.APIVersion != eksConfigAPIVersion || meta.Kind != eksConfigKind { + return "", "", "", fmt.Errorf( + "%w: expected apiVersion %q and kind %q, got %q and %q", + errInvalidEKSConfig, + eksConfigAPIVersion, + eksConfigKind, + meta.APIVersion, + meta.Kind, + ) + } + + name := meta.Metadata.Name + if name != "" { + if name != strings.TrimSpace(name) { + return "", "", "", fmt.Errorf( + "%w: metadata.name %q has leading or trailing whitespace", + errInvalidEKSConfig, + name, + ) + } + + err = v1alpha1.ValidateClusterName(name) + if err != nil { + return "", "", "", fmt.Errorf( + "%w: metadata.name: %w", + errInvalidEKSConfig, + err, + ) + } + } + + return canonical, name, meta.Metadata.Region, nil } // resolveEKSNameFromContext extracts the cluster name from an EKS kubeconfig diff --git a/pkg/fsutil/configmanager/ksail/distribution_test.go b/pkg/fsutil/configmanager/ksail/distribution_test.go index ab40ed9526..fce75ef572 100644 --- a/pkg/fsutil/configmanager/ksail/distribution_test.go +++ b/pkg/fsutil/configmanager/ksail/distribution_test.go @@ -294,6 +294,56 @@ func TestReadEKSConfigMetadata(t *testing.T) { assert.Empty(t, region) }) + for _, testCase := range []struct { + name string + content string + }{ + { + name: "wrong API version is rejected", + content: "apiVersion: v1\n" + + "kind: ClusterConfig\n" + + "metadata:\n name: my-eks\n region: eu-west-1\n", + }, + { + name: "wrong kind is rejected", + content: "apiVersion: eksctl.io/v1alpha5\n" + + "kind: ConfigMap\n" + + "metadata:\n name: my-eks\n region: eu-west-1\n", + }, + { + name: "missing API version is rejected", + content: "kind: ClusterConfig\nmetadata:\n name: my-eks\n region: eu-west-1\n", + }, + { + name: "missing kind is rejected", + content: "apiVersion: eksctl.io/v1alpha5\n" + + "metadata:\n name: my-eks\n region: eu-west-1\n", + }, + { + name: "padded metadata name is rejected", + content: "apiVersion: eksctl.io/v1alpha5\n" + + "kind: ClusterConfig\n" + + "metadata:\n name: 'my-eks '\n region: eu-west-1\n", + }, + { + name: "invalid metadata name is rejected", + content: "apiVersion: eksctl.io/v1alpha5\n" + + "kind: ClusterConfig\n" + + "metadata:\n name: Invalid_EKS\n region: eu-west-1\n", + }, + } { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + configPath := filepath.Join(t.TempDir(), "eks.yaml") + writeFileWithParents(t, configPath, testCase.content) + + _, _, _, err := configmanager.ReadEKSConfigMetadataForTest(configPath) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid EKS config file") + }) + } + t.Run("malformed YAML returns an error", func(t *testing.T) { t.Parallel() diff --git a/pkg/svc/provisioner/cluster/eks/provisioner.go b/pkg/svc/provisioner/cluster/eks/provisioner.go index 2a8470d117..f2654c485a 100644 --- a/pkg/svc/provisioner/cluster/eks/provisioner.go +++ b/pkg/svc/provisioner/cluster/eks/provisioner.go @@ -15,18 +15,17 @@ import ( // // All operations delegate to pkg/client/eksctl.Client, which in turn shells // out to the eksctl binary. The provisioner holds the declarative -// eksctl.yaml path so Create/Delete can be driven from the same source of -// truth scaffolded by ksail project init. +// eksctl.yaml path so Create can be driven from the source of truth +// scaffolded by ksail project init. type Provisioner struct { // name is the cluster name derived from the ksail.yaml / eksctl.yaml. name string // region is the AWS region. Cached here so operations that accept - // --region without a --config-file (e.g. GetCluster on a deleted + // --region without a --config-file (e.g. Delete or GetCluster on a deleted // cluster) still work. region string // configPath is the path to the declarative eksctl ClusterConfig. - // Required for Create; preferred over --name/--region for Delete and - // Upgrade because it keeps CloudFormation stack naming consistent. + // Required for Create. configPath string // kubeconfigPath is the exact file eksctl writes and KSail reads after create. kubeconfigPath string @@ -147,9 +146,7 @@ func (p *Provisioner) Create(ctx context.Context, name string) error { return nil } -// Delete tears down the EKS cluster. Prefers --config-file when available so -// eksctl can unwind the same CloudFormation stacks it created; falls back to -// --name/--region for clusters imported without a local config. +// Delete tears down the exact EKS cluster identified by name and region. func (p *Provisioner) Delete(ctx context.Context, name string) error { target := p.resolveName(name) @@ -158,10 +155,11 @@ func (p *Provisioner) Delete(ctx context.Context, name string) error { return fmt.Errorf("eksctl unavailable: %w", err) } - // DeleteCluster prefers configPath over name when configPath is set. + // Keep configPath empty so DeleteCluster cannot replace the validated exact + // target with a name or region from the declarative create configuration. // Wait=true because cluster deletion must complete before ksail // considers the workspace clean. - err = p.client.DeleteCluster(ctx, target, p.region, p.configPath, true) + err = p.client.DeleteCluster(ctx, target, p.region, "", true) if err != nil { return fmt.Errorf("eksctl delete cluster: %w", err) } diff --git a/pkg/svc/provisioner/cluster/eks/provisioner_test.go b/pkg/svc/provisioner/cluster/eks/provisioner_test.go index d0260ba41d..35821cbe04 100644 --- a/pkg/svc/provisioner/cluster/eks/provisioner_test.go +++ b/pkg/svc/provisioner/cluster/eks/provisioner_test.go @@ -151,7 +151,7 @@ func TestCreate_NoConfigPath_ReturnsError(t *testing.T) { require.ErrorIs(t, err, eksprovisioner.ErrConfigPathRequired) } -func TestDelete_PrefersConfigFile(t *testing.T) { +func TestDelete_UsesExactNameAndRegion(t *testing.T) { t.Parallel() prov, runner := newProvisioner(t, map[string][]response{ @@ -161,9 +161,17 @@ func TestDelete_PrefersConfigFile(t *testing.T) { require.NoError(t, prov.Delete(context.Background(), "")) require.Len(t, runner.calls, 1) - // DeleteCluster prefers --config-file over --name when configPath set. - assert.Contains(t, runner.calls[0], "--config-file") - assert.Contains(t, runner.calls[0], "--wait") + assert.Equal( + t, + []string{ + "delete", "cluster", + "--name", "ksail-test", + "--region", "us-east-1", + "--wait", + }, + runner.calls[0], + ) + assert.NotContains(t, runner.calls[0], "--config-file") } func TestStart_DelegatesToProvider(t *testing.T) { diff --git a/pkg/svc/provisioner/cluster/factory.go b/pkg/svc/provisioner/cluster/factory.go index 9278e8ea8b..7139d601be 100644 --- a/pkg/svc/provisioner/cluster/factory.go +++ b/pkg/svc/provisioner/cluster/factory.go @@ -62,6 +62,9 @@ type DistributionConfig struct { type EKSConfig struct { // Name is the cluster name (mirrors eksctl.yaml metadata.name). Name string + // NameFromConfig reports that Name was parsed from metadata.name in the actual eksctl config, + // rather than synthesized from a kubeconfig context or default. + NameFromConfig bool // Region is the AWS region. Region string // ConfigPath is the path to the declarative eksctl.yaml. From 716aa65b29b29acc861395410c0dda4be6d2d6d6 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Fri, 17 Jul 2026 10:37:23 +0200 Subject: [PATCH 03/10] fix(eks): satisfy lifecycle config lint Extract EKS metadata parsing and split invalid-file cases without changing lifecycle validation semantics. Refs #6087 --- .../configmanager/ksail/distribution.go | 37 ++++++--- .../configmanager/ksail/distribution_test.go | 80 +++++++++++-------- 2 files changed, 69 insertions(+), 48 deletions(-) diff --git a/pkg/fsutil/configmanager/ksail/distribution.go b/pkg/fsutil/configmanager/ksail/distribution.go index 82e325e5ee..2ec10141a5 100644 --- a/pkg/fsutil/configmanager/ksail/distribution.go +++ b/pkg/fsutil/configmanager/ksail/distribution.go @@ -721,18 +721,29 @@ func readEKSConfigMetadata(configPath string) (string, string, string, error) { return "", "", "", fmt.Errorf("failed to read EKS config file: %w", err) } - var meta struct { - APIVersion string `json:"apiVersion"` - Kind string `json:"kind"` - Metadata struct { - Name string `json:"name"` - Region string `json:"region"` - } `json:"metadata"` + name, region, err := parseEKSConfigMetadata(data) + if err != nil { + return "", "", "", err } - err = yaml.Unmarshal(data, &meta) + return canonical, name, region, nil +} + +type eksConfigMetadata struct { + APIVersion string `json:"apiVersion"` + Kind string `json:"kind"` + Metadata struct { + Name string `json:"name"` + Region string `json:"region"` + } `json:"metadata"` +} + +func parseEKSConfigMetadata(data []byte) (string, string, error) { + var meta eksConfigMetadata + + err := yaml.Unmarshal(data, &meta) if err != nil { - return "", "", "", fmt.Errorf("failed to parse EKS config file: %w", err) + return "", "", fmt.Errorf("failed to parse EKS config file: %w", err) } const ( @@ -741,7 +752,7 @@ func readEKSConfigMetadata(configPath string) (string, string, string, error) { ) if meta.APIVersion != eksConfigAPIVersion || meta.Kind != eksConfigKind { - return "", "", "", fmt.Errorf( + return "", "", fmt.Errorf( "%w: expected apiVersion %q and kind %q, got %q and %q", errInvalidEKSConfig, eksConfigAPIVersion, @@ -754,7 +765,7 @@ func readEKSConfigMetadata(configPath string) (string, string, string, error) { name := meta.Metadata.Name if name != "" { if name != strings.TrimSpace(name) { - return "", "", "", fmt.Errorf( + return "", "", fmt.Errorf( "%w: metadata.name %q has leading or trailing whitespace", errInvalidEKSConfig, name, @@ -763,7 +774,7 @@ func readEKSConfigMetadata(configPath string) (string, string, string, error) { err = v1alpha1.ValidateClusterName(name) if err != nil { - return "", "", "", fmt.Errorf( + return "", "", fmt.Errorf( "%w: metadata.name: %w", errInvalidEKSConfig, err, @@ -771,7 +782,7 @@ func readEKSConfigMetadata(configPath string) (string, string, string, error) { } } - return canonical, name, meta.Metadata.Region, nil + return name, meta.Metadata.Region, nil } // resolveEKSNameFromContext extracts the cluster name from an EKS kubeconfig diff --git a/pkg/fsutil/configmanager/ksail/distribution_test.go b/pkg/fsutil/configmanager/ksail/distribution_test.go index fce75ef572..9a1e122acc 100644 --- a/pkg/fsutil/configmanager/ksail/distribution_test.go +++ b/pkg/fsutil/configmanager/ksail/distribution_test.go @@ -247,8 +247,7 @@ func TestIngressFirewallPatchesSuccess(t *testing.T) { }) } -// TestReadEKSConfigMetadata verifies the eksctl config metadata reader across -// missing-file, populated, metadata-less, and malformed-YAML cases. +// TestReadEKSConfigMetadata verifies the supported absent and valid eksctl config shapes. func TestReadEKSConfigMetadata(t *testing.T) { t.Parallel() @@ -293,67 +292,78 @@ func TestReadEKSConfigMetadata(t *testing.T) { assert.Empty(t, name) assert.Empty(t, region) }) +} - for _, testCase := range []struct { - name string - content string - }{ +func TestReadEKSConfigMetadataRejectsInvalidFiles(t *testing.T) { + t.Parallel() + + for _, testCase := range invalidEKSConfigMetadataCases() { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + configPath := filepath.Join(t.TempDir(), "eks.yaml") + writeFileWithParents(t, configPath, testCase.content) + + _, _, _, err := configmanager.ReadEKSConfigMetadataForTest(configPath) + require.Error(t, err) + assert.Contains(t, err.Error(), testCase.wantError) + }) + } +} + +type invalidEKSConfigMetadataCase struct { + name string + content string + wantError string +} + +func invalidEKSConfigMetadataCases() []invalidEKSConfigMetadataCase { + return []invalidEKSConfigMetadataCase{ { - name: "wrong API version is rejected", + name: "wrong API version is rejected", + wantError: "invalid EKS config file", content: "apiVersion: v1\n" + "kind: ClusterConfig\n" + "metadata:\n name: my-eks\n region: eu-west-1\n", }, { - name: "wrong kind is rejected", + name: "wrong kind is rejected", + wantError: "invalid EKS config file", content: "apiVersion: eksctl.io/v1alpha5\n" + "kind: ConfigMap\n" + "metadata:\n name: my-eks\n region: eu-west-1\n", }, { - name: "missing API version is rejected", - content: "kind: ClusterConfig\nmetadata:\n name: my-eks\n region: eu-west-1\n", + name: "missing API version is rejected", + content: "kind: ClusterConfig\nmetadata:\n name: my-eks\n region: eu-west-1\n", + wantError: "invalid EKS config file", }, { - name: "missing kind is rejected", + name: "missing kind is rejected", + wantError: "invalid EKS config file", content: "apiVersion: eksctl.io/v1alpha5\n" + "metadata:\n name: my-eks\n region: eu-west-1\n", }, { - name: "padded metadata name is rejected", + name: "padded metadata name is rejected", + wantError: "invalid EKS config file", content: "apiVersion: eksctl.io/v1alpha5\n" + "kind: ClusterConfig\n" + "metadata:\n name: 'my-eks '\n region: eu-west-1\n", }, { - name: "invalid metadata name is rejected", + name: "invalid metadata name is rejected", + wantError: "invalid EKS config file", content: "apiVersion: eksctl.io/v1alpha5\n" + "kind: ClusterConfig\n" + "metadata:\n name: Invalid_EKS\n region: eu-west-1\n", }, - } { - t.Run(testCase.name, func(t *testing.T) { - t.Parallel() - - configPath := filepath.Join(t.TempDir(), "eks.yaml") - writeFileWithParents(t, configPath, testCase.content) - - _, _, _, err := configmanager.ReadEKSConfigMetadataForTest(configPath) - require.Error(t, err) - assert.Contains(t, err.Error(), "invalid EKS config file") - }) + { + name: "malformed YAML is rejected", + content: "metadata:\n\tname: my-eks\n", + wantError: "failed to parse EKS config file", + }, } - - t.Run("malformed YAML returns an error", func(t *testing.T) { - t.Parallel() - - configPath := filepath.Join(t.TempDir(), "eks.yaml") - // A tab in the indentation is invalid YAML. - writeFileWithParents(t, configPath, "metadata:\n\tname: my-eks\n") - - _, _, _, err := configmanager.ReadEKSConfigMetadataForTest(configPath) - require.Error(t, err) - }) } // TestResolveKWOKName verifies KWOK cluster-name resolution from the kubeconfig From 3a82ff0ffba1fbbe08d834dfebade9a9fc4e41e7 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Fri, 17 Jul 2026 11:39:28 +0200 Subject: [PATCH 04/10] fix(eks): preserve nodegroup capacity across stop Persist exact EKS nodegroup capacities before stop, restore and verify them on start, and fail closed on stale or incomplete lifecycle state. Refs #6087 --- pkg/cli/cmd/cluster/eks_lifecycle_test.go | 85 ++++- pkg/svc/provider/aws/nodegroup_state.go | 382 ++++++++++++++++++++++ pkg/svc/provider/aws/provider.go | 85 +++-- pkg/svc/provider/aws/provider_test.go | 233 ++++++++++++- pkg/svc/state/eks_nodegroup_state.go | 121 +++++++ pkg/svc/state/eks_nodegroup_state_test.go | 63 ++++ 6 files changed, 928 insertions(+), 41 deletions(-) create mode 100644 pkg/svc/provider/aws/nodegroup_state.go create mode 100644 pkg/svc/state/eks_nodegroup_state.go create mode 100644 pkg/svc/state/eks_nodegroup_state_test.go diff --git a/pkg/cli/cmd/cluster/eks_lifecycle_test.go b/pkg/cli/cmd/cluster/eks_lifecycle_test.go index ef02703456..6f5888bdb7 100644 --- a/pkg/cli/cmd/cluster/eks_lifecycle_test.go +++ b/pkg/cli/cmd/cluster/eks_lifecycle_test.go @@ -80,8 +80,15 @@ if [ "$1 $2" = "get cluster" ]; then printf '[{"Name":"%s","Region":"%s",' "$discovered_cluster" "$discovered_region" printf '"EksctlCreated":"%s"}]\n' "$eksctl_created" elif [ "$1 $2" = "get nodegroup" ]; then - printf '[{"Cluster":"%s","Name":"workers","Status":"ACTIVE",' "$KSAIL_EKS_CLUSTER" - printf '"DesiredCapacity":0,"MinSize":2,"MaxSize":4,"NodeGroupType":"managed"}]\n' + current_desired="${KSAIL_EKS_NODEGROUP_DESIRED:-0}" + current_min="${KSAIL_EKS_NODEGROUP_MIN:-2}" + if grep -q -- '--nodes 2 --nodes-min 2' "$KSAIL_EKSCTL_MARKER"; then + current_desired=2 + current_min=2 + fi + printf '[{"Cluster":"%s","Name":"workers","Status":"ACTIVE",' "$KSAIL_EKS_CLUSTER" + printf '"DesiredCapacity":%s,"MinSize":%s,"MaxSize":4,"NodeGroupType":"managed"}]\n' \ + "$current_desired" "$current_min" fi ` @@ -163,6 +170,7 @@ func TestStandaloneEKSLifecycleCommandsRouteToEksctl(t *testing.T) { t.Run(testCase.name, func(t *testing.T) { clusterName := "ksail-eks-" + testCase.name + "-routing-test-6087" markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + configureStandaloneEKSNodegroupAction(t, testCase.name) if testCase.name == "start" { // EksctlCreated is normalized explicitly: case and surrounding whitespace are benign. @@ -192,6 +200,51 @@ func TestStandaloneEKSLifecycleCommandsRouteToEksctl(t *testing.T) { } } +// TestStandaloneEKSStopStartRestoresExactCapacity exercises the user-facing commands across the +// persisted boundary and proves start verifies the restored tuple before clearing its snapshot. +func TestStandaloneEKSStopStartRestoresExactCapacity(t *testing.T) { + const clusterName = "ksail-eks-stop-start-roundtrip-6087" + + markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + t.Setenv("KSAIL_EKS_NODEGROUP_DESIRED", "2") + t.Setenv("KSAIL_EKS_NODEGROUP_MIN", "2") + runStandaloneEKSCommand(t, cluster.NewStopCmd, "--name", clusterName, "--provider", "AWS") + + t.Setenv("KSAIL_EKS_NODEGROUP_DESIRED", "0") + t.Setenv("KSAIL_EKS_NODEGROUP_MIN", "0") + runStandaloneEKSCommand(t, cluster.NewStartCmd, "--name", clusterName, "--provider", "AWS") + + assert.Equal(t, []string{ + fmt.Sprintf("get cluster --name %s --output json --region ap-southeast-2", clusterName), + fmt.Sprintf( + "get nodegroup --cluster %s --output json --region ap-southeast-2", + clusterName, + ), + fmt.Sprintf( + "scale nodegroup --cluster %s --name workers "+ + "--nodes 0 --nodes-min 0 --nodes-max 4 --region ap-southeast-2", + clusterName, + ), + fmt.Sprintf("get cluster --name %s --output json --region ap-southeast-2", clusterName), + fmt.Sprintf( + "get nodegroup --cluster %s --output json --region ap-southeast-2", + clusterName, + ), + fmt.Sprintf( + "scale nodegroup --cluster %s --name workers "+ + "--nodes 2 --nodes-min 2 --nodes-max 4 --region ap-southeast-2", + clusterName, + ), + fmt.Sprintf( + "get nodegroup --cluster %s --output json --region ap-southeast-2", + clusterName, + ), + }, readStandaloneEKSCalls(t, markerPath)) + + _, err := state.LoadEKSNodegroupState(clusterName) + require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) +} + // TestStandaloneEKSLifecycleCommandsPropagateEksctlErrors covers each command's // user-facing error path after routing has reached eksctl. func TestStandaloneEKSLifecycleCommandsPropagateEksctlErrors(t *testing.T) { @@ -227,6 +280,7 @@ func TestStandaloneEKSLifecycleCommandsPropagateEksctlErrors(t *testing.T) { t.Run(testCase.name, func(t *testing.T) { clusterName := "ksail-eks-" + testCase.name + "-error-test-6087" markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + configureStandaloneEKSNodegroupAction(t, testCase.name) t.Setenv("KSAIL_EKSCTL_FAIL", testCase.failOn) cmd := testCase.newCommand() @@ -523,6 +577,7 @@ func TestStandaloneEKSLifecycleCommandsIgnoreRegionFromNamelessConfig(t *testing t.Run(testCase.name, func(t *testing.T) { clusterName := "ksail-eks-" + testCase.name + "-nameless-region-test-6087" markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + configureStandaloneEKSNodegroupAction(t, testCase.name) t.Setenv("HOME", t.TempDir()) t.Setenv("KSAIL_REGION", "") @@ -726,6 +781,7 @@ func TestStandaloneEKSLifecycleCommandsUseEKSConfigNameWhenMetadataDrifts(t *tes t.Run(testCase.name, func(t *testing.T) { clusterName := "eks-source-name-6087" markerPath := setupStandaloneEKSLifecycleFixture(t, clusterName) + configureStandaloneEKSNodegroupAction(t, testCase.name) require.NoError( t, os.WriteFile( @@ -1377,6 +1433,7 @@ func setupStandaloneEKSLifecycleFixture( t.Setenv("PATH", binDir+string(os.PathListSeparator)+os.Getenv("PATH")) t.Setenv("KSAIL_EKSCTL_MARKER", markerPath) t.Setenv("KSAIL_EKS_CLUSTER", clusterName) + t.Setenv("HOME", t.TempDir()) t.Setenv("KSAIL_PROFILE", "selected-profile") t.Setenv("KSAIL_REGION", "ap-southeast-2") t.Setenv("KSAIL_ACCESS", "fixture-access") @@ -1391,6 +1448,30 @@ func setupStandaloneEKSLifecycleFixture( return markerPath } +func configureStandaloneEKSNodegroupAction(t *testing.T, action string) { + t.Helper() + + if action == "stop" { + t.Setenv("KSAIL_EKS_NODEGROUP_DESIRED", "2") + t.Setenv("KSAIL_EKS_NODEGROUP_MIN", "2") + } +} + +func runStandaloneEKSCommand( + t *testing.T, + newCommand func() *cobra.Command, + args ...string, +) { + t.Helper() + + cmd := newCommand() + cmd.SetArgs(args) + cmd.SetContext(t.Context()) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + require.NoError(t, cmd.Execute()) +} + func assertParentAWSEnvironmentUnchanged(t *testing.T) { t.Helper() diff --git a/pkg/svc/provider/aws/nodegroup_state.go b/pkg/svc/provider/aws/nodegroup_state.go new file mode 100644 index 0000000000..ed6c79da23 --- /dev/null +++ b/pkg/svc/provider/aws/nodegroup_state.go @@ -0,0 +1,382 @@ +package aws + +import ( + "context" + "errors" + "fmt" + "sort" + "strings" + + eksctlclient "github.com/devantler-tech/ksail/v7/pkg/client/eksctl" + "github.com/devantler-tech/ksail/v7/pkg/svc/state" +) + +var ( + errNodegroupStateVersion = errors.New("unsupported EKS nodegroup state version") + errNodegroupStateTarget = errors.New("EKS nodegroup state target mismatch") + errNodegroupStateDrift = errors.New("EKS nodegroup capacity state drift") + errNodegroupStateMissing = errors.New("EKS nodegroup capacity state is required") +) + +func newNodegroupState( + clusterName, region string, + nodegroups []eksctlclient.NodegroupSummary, +) (*state.EKSNodegroupState, error) { + region = strings.TrimSpace(region) + if region == "" { + return nil, fmt.Errorf( + "refusing to persist regionless EKS nodegroup state: %w", + errNodegroupStateTarget, + ) + } + + capacities := make([]state.EKSNodegroupCapacity, 0, len(nodegroups)) + seen := make(map[string]struct{}, len(nodegroups)) + + for _, nodegroup := range nodegroups { + err := validateLiveNodegroup(clusterName, nodegroup) + if err != nil { + return nil, err + } + + if nodegroup.DesiredCap == 0 { + return nil, fmt.Errorf( + "nodegroup %q is already stopped without a saved capacity: %w", + nodegroup.Name, + errNodegroupStateMissing, + ) + } + + if _, duplicate := seen[nodegroup.Name]; duplicate { + return nil, fmt.Errorf( + "duplicate nodegroup %q: %w", + nodegroup.Name, + errNodegroupStateDrift, + ) + } + + seen[nodegroup.Name] = struct{}{} + capacities = append(capacities, state.EKSNodegroupCapacity{ + Name: nodegroup.Name, + DesiredCapacity: nodegroup.DesiredCap, + MinSize: nodegroup.MinSize, + MaxSize: nodegroup.MaxSize, + }) + } + + _, err := validateSavedCapacities(capacities) + if err != nil { + return nil, err + } + + sort.Slice(capacities, func(i, j int) bool { return capacities[i].Name < capacities[j].Name }) + + return &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: clusterName, + Region: region, + Nodegroups: capacities, + }, nil +} + +func validateNodegroupTransition( + clusterName, region string, + snapshot *state.EKSNodegroupState, + nodegroups []eksctlclient.NodegroupSummary, +) (map[string]eksctlclient.NodegroupSummary, error) { + err := validateNodegroupStateIdentity(clusterName, region, snapshot) + if err != nil { + return nil, err + } + + savedByName, err := validateSavedCapacities(snapshot.Nodegroups) + if err != nil { + return nil, err + } + + if len(nodegroups) != len(savedByName) { + return nil, fmt.Errorf( + "live nodegroup count %d does not match saved count %d: %w", + len(nodegroups), + len(savedByName), + errNodegroupStateDrift, + ) + } + + liveByName := make(map[string]eksctlclient.NodegroupSummary, len(nodegroups)) + + for _, nodegroup := range nodegroups { + err = validateLiveNodegroup(clusterName, nodegroup) + if err != nil { + return nil, err + } + + capacity, found := savedByName[nodegroup.Name] + if !found || (!nodegroupMatchesCapacity(nodegroup, capacity) && + !nodegroupIsStopped(nodegroup, capacity)) { + return nil, fmt.Errorf( + "live nodegroup %q is neither saved nor safely stopped: %w", + nodegroup.Name, + errNodegroupStateDrift, + ) + } + + if _, duplicate := liveByName[nodegroup.Name]; duplicate { + return nil, fmt.Errorf( + "duplicate live nodegroup %q: %w", + nodegroup.Name, + errNodegroupStateDrift, + ) + } + + liveByName[nodegroup.Name] = nodegroup + } + + return liveByName, nil +} + +func validateNodegroupStateIdentity( + clusterName, region string, + snapshot *state.EKSNodegroupState, +) error { + if snapshot == nil { + return fmt.Errorf("nil EKS nodegroup state: %w", errNodegroupStateDrift) + } + + if snapshot.Version != state.EKSNodegroupStateVersion { + return fmt.Errorf( + "got version %d, want %d: %w", + snapshot.Version, + state.EKSNodegroupStateVersion, + errNodegroupStateVersion, + ) + } + + if strings.TrimSpace(clusterName) == "" || snapshot.ClusterName != clusterName || + strings.TrimSpace(region) == "" || snapshot.Region != region { + return fmt.Errorf( + "saved cluster/region %q/%q does not match target %q/%q: %w", + snapshot.ClusterName, + snapshot.Region, + clusterName, + region, + errNodegroupStateTarget, + ) + } + + return nil +} + +func validateSavedCapacities( + capacities []state.EKSNodegroupCapacity, +) (map[string]state.EKSNodegroupCapacity, error) { + if len(capacities) == 0 { + return nil, fmt.Errorf("saved nodegroup set is empty: %w", errNodegroupStateDrift) + } + + byName := make(map[string]state.EKSNodegroupCapacity, len(capacities)) + + for _, capacity := range capacities { + if strings.TrimSpace(capacity.Name) == "" || capacity.MinSize < 0 || + capacity.DesiredCapacity <= 0 || + capacity.DesiredCapacity < capacity.MinSize || capacity.MaxSize < capacity.DesiredCapacity { + return nil, fmt.Errorf( + "invalid saved nodegroup capacity for %q: %w", + capacity.Name, + errNodegroupStateDrift, + ) + } + + if _, duplicate := byName[capacity.Name]; duplicate { + return nil, fmt.Errorf( + "duplicate saved nodegroup %q: %w", + capacity.Name, + errNodegroupStateDrift, + ) + } + + byName[capacity.Name] = capacity + } + + return byName, nil +} + +func validateLiveNodegroup(clusterName string, nodegroup eksctlclient.NodegroupSummary) error { + if strings.TrimSpace(nodegroup.Name) == "" || + (nodegroup.Cluster != "" && nodegroup.Cluster != clusterName) || + nodegroup.MinSize < 0 || nodegroup.DesiredCap < 0 || nodegroup.MaxSize < nodegroup.DesiredCap { + return fmt.Errorf( + "invalid live nodegroup %q for cluster %q: %w", + nodegroup.Name, + clusterName, + errNodegroupStateDrift, + ) + } + + return nil +} + +func verifyNodegroupsRestored( + clusterName, region string, + snapshot *state.EKSNodegroupState, + nodegroups []eksctlclient.NodegroupSummary, +) error { + liveByName, err := validateNodegroupTransition(clusterName, region, snapshot, nodegroups) + if err != nil { + return fmt.Errorf("verify restored EKS nodegroups: %w", err) + } + + for _, capacity := range snapshot.Nodegroups { + if !nodegroupMatchesCapacity(liveByName[capacity.Name], capacity) { + return fmt.Errorf( + "nodegroup %q is not restored: %w", + capacity.Name, + errNodegroupStateDrift, + ) + } + } + + return nil +} + +func nodegroupMatchesCapacity( + nodegroup eksctlclient.NodegroupSummary, + capacity state.EKSNodegroupCapacity, +) bool { + return nodegroup.DesiredCap == capacity.DesiredCapacity && + nodegroup.MinSize == capacity.MinSize && + nodegroup.MaxSize == capacity.MaxSize +} + +func nodegroupIsStopped( + nodegroup eksctlclient.NodegroupSummary, + capacity state.EKSNodegroupCapacity, +) bool { + return nodegroup.DesiredCap == 0 && nodegroup.MinSize == 0 && + nodegroup.MaxSize == capacity.MaxSize +} + +func (p *Provider) loadNodegroupState( + clusterName string, +) (*state.EKSNodegroupState, bool, error) { + snapshot, err := state.LoadEKSNodegroupState(clusterName) + if errors.Is(err, state.ErrEKSNodegroupStateNotFound) { + return nil, false, nil + } + + if err != nil { + return nil, false, fmt.Errorf("load EKS nodegroup state: %w", err) + } + + return snapshot, true, nil +} + +// startNodegroupsWithoutSnapshot preserves the pre-existing lifecycle for groups stopped outside +// KSail while failing closed when both desired and minimum are zero. A KSail stop always records the +// exact desired size first, so its own round trip never reaches this compatibility path. +func (p *Provider) startNodegroupsWithoutSnapshot( + ctx context.Context, + clusterName string, + nodegroups []eksctlclient.NodegroupSummary, +) error { + seen := make(map[string]struct{}, len(nodegroups)) + + for _, nodegroup := range nodegroups { + err := validateLiveNodegroup(clusterName, nodegroup) + if err != nil { + return err + } + + if _, duplicate := seen[nodegroup.Name]; duplicate { + return fmt.Errorf( + "duplicate live nodegroup %q: %w", + nodegroup.Name, + errNodegroupStateDrift, + ) + } + + seen[nodegroup.Name] = struct{}{} + + if nodegroup.DesiredCap == 0 && nodegroup.MinSize <= 0 { + return fmt.Errorf( + "cannot restore stopped EKS nodegroup %q without the pre-stop snapshot: %w", + nodegroup.Name, + errNodegroupStateMissing, + ) + } + } + + sort.Slice(nodegroups, func(i, j int) bool { return nodegroups[i].Name < nodegroups[j].Name }) + + for _, nodegroup := range nodegroups { + if nodegroup.DesiredCap > 0 { + continue + } + + err := p.client.ScaleNodegroup( + ctx, + clusterName, + nodegroup.Name, + p.region, + nodegroup.MinSize, + nodegroup.MinSize, + nodegroup.MaxSize, + ) + if err != nil { + return fmt.Errorf("start nodes: scale nodegroup %s: %w", nodegroup.Name, err) + } + } + + return nil +} + +func (p *Provider) restoreSavedNodegroups( + ctx context.Context, + clusterName string, + snapshot *state.EKSNodegroupState, + liveByName map[string]eksctlclient.NodegroupSummary, +) error { + for _, capacity := range snapshot.Nodegroups { + if nodegroupMatchesCapacity(liveByName[capacity.Name], capacity) { + continue + } + + err := p.client.ScaleNodegroup( + ctx, + clusterName, + capacity.Name, + p.region, + capacity.DesiredCapacity, + capacity.MinSize, + capacity.MaxSize, + ) + if err != nil { + return fmt.Errorf("start nodes: scale nodegroup %s: %w", capacity.Name, err) + } + } + + return nil +} + +func (p *Provider) verifyAndClearNodegroupState( + ctx context.Context, + clusterName string, + snapshot *state.EKSNodegroupState, +) error { + restored, err := p.listNodegroupsForScale(ctx, clusterName) + if err != nil { + return fmt.Errorf("verify restored nodegroups: %w", err) + } + + err = verifyNodegroupsRestored(clusterName, p.region, snapshot, restored) + if err != nil { + return err + } + + err = state.DeleteEKSNodegroupState(clusterName) + if err != nil { + return fmt.Errorf("clear restored EKS nodegroup state: %w", err) + } + + return nil +} diff --git a/pkg/svc/provider/aws/provider.go b/pkg/svc/provider/aws/provider.go index e804b5daa3..728681114a 100644 --- a/pkg/svc/provider/aws/provider.go +++ b/pkg/svc/provider/aws/provider.go @@ -11,6 +11,7 @@ import ( eksclient "github.com/devantler-tech/ksail/v7/pkg/client/eks" eksctlclient "github.com/devantler-tech/ksail/v7/pkg/client/eksctl" "github.com/devantler-tech/ksail/v7/pkg/svc/provider" + "github.com/devantler-tech/ksail/v7/pkg/svc/state" ) // NodegroupStatusActive is the EKS nodegroup status that indicates the @@ -89,65 +90,83 @@ func NewProvider(client *eksctlclient.Client, region string, opts ...Option) (*P return prov, nil } -// StartNodes scales all managed nodegroups for the cluster back to their -// configured desired capacity if it is currently zero. Nodegroups already at -// non-zero desired capacity are left alone. -// -// When a nodegroup's desired capacity is zero, this method does not know the -// "correct" target size to scale up to, so it defers to the max(MinSize, 1) -// that eksctl returned. The provisioner — which has the ksail.yaml spec — is -// responsible for restoring the exact DesiredCapacity when needed. +// StartNodes restores every managed nodegroup to the exact desired/minimum/maximum values captured +// before StopNodes zeroed it. The snapshot survives partial failures and is removed only after a +// readback confirms that every group is restored. func (p *Provider) StartNodes(ctx context.Context, clusterName string) error { nodegroups, err := p.listNodegroupsForScale(ctx, clusterName) if err != nil { return err } - for _, nodegroup := range nodegroups { - if nodegroup.DesiredCap > 0 { - continue - } + snapshot, found, err := p.loadNodegroupState(clusterName) + if err != nil { + return err + } - target := max(nodegroup.MinSize, 1) + if !found { + return p.startNodegroupsWithoutSnapshot(ctx, clusterName, nodegroups) + } - err = p.client.ScaleNodegroup( - ctx, - clusterName, - nodegroup.Name, - p.region, - target, - nodegroup.MinSize, - nodegroup.MaxSize, - ) - if err != nil { - return fmt.Errorf("start nodes: scale nodegroup %s: %w", nodegroup.Name, err) - } + liveByName, err := validateNodegroupTransition(clusterName, p.region, snapshot, nodegroups) + if err != nil { + return err } - return nil + err = p.restoreSavedNodegroups(ctx, clusterName, snapshot, liveByName) + if err != nil { + return err + } + + return p.verifyAndClearNodegroupState(ctx, clusterName, snapshot) } -// StopNodes scales all managed nodegroups to zero desired capacity. The -// cluster control plane remains running (EKS bills $0.10/hour for it) but -// all node-hour costs stop. +// StopNodes atomically snapshots every managed nodegroup before scaling it to zero. Repeated calls +// preserve the first snapshot and skip already-stopped groups, so a partial stop remains retryable. func (p *Provider) StopNodes(ctx context.Context, clusterName string) error { nodegroups, err := p.listNodegroupsForScale(ctx, clusterName) if err != nil { return err } - for _, nodegroup := range nodegroups { + snapshot, found, err := p.loadNodegroupState(clusterName) + if err != nil { + return err + } + + if !found { + snapshot, err = newNodegroupState(clusterName, p.region, nodegroups) + if err != nil { + return err + } + + err = state.SaveEKSNodegroupState(clusterName, snapshot) + if err != nil { + return fmt.Errorf("save EKS nodegroup state before stop: %w", err) + } + } + + liveByName, err := validateNodegroupTransition(clusterName, p.region, snapshot, nodegroups) + if err != nil { + return err + } + + for _, capacity := range snapshot.Nodegroups { + if nodegroupIsStopped(liveByName[capacity.Name], capacity) { + continue + } + err = p.client.ScaleNodegroup( ctx, clusterName, - nodegroup.Name, + capacity.Name, p.region, 0, 0, - nodegroup.MaxSize, + capacity.MaxSize, ) if err != nil { - return fmt.Errorf("stop nodes: scale nodegroup %s: %w", nodegroup.Name, err) + return fmt.Errorf("stop nodes: scale nodegroup %s: %w", capacity.Name, err) } } diff --git a/pkg/svc/provider/aws/provider_test.go b/pkg/svc/provider/aws/provider_test.go index d73effd20c..f2c5ed689a 100644 --- a/pkg/svc/provider/aws/provider_test.go +++ b/pkg/svc/provider/aws/provider_test.go @@ -3,6 +3,7 @@ package aws_test import ( "context" "errors" + "fmt" "io" "strings" "testing" @@ -12,6 +13,7 @@ import ( eksctlclient "github.com/devantler-tech/ksail/v7/pkg/client/eksctl" "github.com/devantler-tech/ksail/v7/pkg/svc/provider" "github.com/devantler-tech/ksail/v7/pkg/svc/provider/aws" + "github.com/devantler-tech/ksail/v7/pkg/svc/state" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -47,6 +49,8 @@ var errScriptedRunnerEmptyArgs = errors.New("scripted runner: empty args") // GetClusterStatus surfaces a DescribeCluster failure. var errDescribeDenied = errors.New("access denied") +var errScaleDenied = errors.New("scale denied") + // scriptedRunner replays canned responses keyed by the first argument // (`create`, `delete`, `get`, `scale`, `upgrade`). It records every call for // assertions. @@ -196,7 +200,7 @@ func TestDeleteNodes_Noop(t *testing.T) { } func TestStopNodes_ScalesAllToZero(t *testing.T) { - t.Parallel() + t.Setenv("HOME", t.TempDir()) prov, runner := newProvider(t, map[string][]response{ "get nodegroup": {{stdout: []byte(`[ @@ -234,13 +238,28 @@ func TestStopNodes_NoNodegroups(t *testing.T) { } func TestStartNodes_ScalesOnlyZeroDesired(t *testing.T) { - t.Parallel() + t.Setenv("HOME", t.TempDir()) + require.NoError(t, state.SaveEKSNodegroupState("demo", &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: "demo", + Region: "us-east-1", + Nodegroups: []state.EKSNodegroupCapacity{ + {Name: "ng-1", DesiredCapacity: 2, MinSize: 2, MaxSize: 5}, + {Name: "ng-2", DesiredCapacity: 3, MinSize: 1, MaxSize: 5}, + }, + })) prov, runner := newProvider(t, map[string][]response{ - "get nodegroup": {{stdout: []byte(`[ - {"Cluster":"demo","Name":"ng-1","DesiredCapacity":0,"MinSize":2,"MaxSize":5}, - {"Cluster":"demo","Name":"ng-2","DesiredCapacity":3,"MinSize":1,"MaxSize":5} - ]`)}}, + "get nodegroup": { + {stdout: []byte(`[ + {"Cluster":"demo","Name":"ng-1","DesiredCapacity":0,"MinSize":0,"MaxSize":5}, + {"Cluster":"demo","Name":"ng-2","DesiredCapacity":3,"MinSize":1,"MaxSize":5} + ]`)}, + {stdout: []byte(`[ + {"Cluster":"demo","Name":"ng-1","DesiredCapacity":2,"MinSize":2,"MaxSize":5}, + {"Cluster":"demo","Name":"ng-2","DesiredCapacity":3,"MinSize":1,"MaxSize":5} + ]`)}, + }, "scale nodegroup": {{}}, }) @@ -262,6 +281,208 @@ func TestStartNodes_ScalesOnlyZeroDesired(t *testing.T) { assert.Equal(t, 1, scales, "should scale only nodegroups with desiredCapacity=0") } +// TestStopThenStartRestoresNodegroupCapacities proves the lifecycle round trip does not lose the +// desired/minimum sizes when stop must temporarily set both values to zero. +func TestStopThenStartRestoresNodegroupCapacities(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProvider(t, map[string][]response{ + "get nodegroup": { + {stdout: []byte(`[ + {"Cluster":"demo","Name":"ng-1","DesiredCapacity":3,"MinSize":2,"MaxSize":5} + ]`)}, + {stdout: []byte(`[ + {"Cluster":"demo","Name":"ng-1","DesiredCapacity":0,"MinSize":0,"MaxSize":5} + ]`)}, + {stdout: []byte(`[ + {"Cluster":"demo","Name":"ng-1","DesiredCapacity":3,"MinSize":2,"MaxSize":5} + ]`)}, + }, + "scale nodegroup": {{}, {}}, + }) + + require.NoError(t, prov.StopNodes(t.Context(), "demo")) + require.NoError(t, prov.StartNodes(t.Context(), "demo")) + + var scales []string + + for _, call := range runner.calls { + if len(call) >= 2 && call[0] == "scale" && call[1] == "nodegroup" { + scales = append(scales, strings.Join(call, " ")) + } + } + + require.Equal(t, []string{ + "scale nodegroup --cluster demo --name ng-1 --nodes 0 --nodes-min 0 --nodes-max 5 --region us-east-1", + "scale nodegroup --cluster demo --name ng-1 --nodes 3 --nodes-min 2 --nodes-max 5 --region us-east-1", + }, scales) + + _, err := state.LoadEKSNodegroupState("demo") + require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) +} + +func TestRepeatedStopPreservesOriginalNodegroupCapacities(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProvider(t, map[string][]response{ + "get nodegroup": { + {stdout: nodegroupJSON(3, 2)}, + {stdout: nodegroupJSON(0, 0)}, + {stdout: nodegroupJSON(0, 0)}, + {stdout: nodegroupJSON(3, 2)}, + }, + "scale nodegroup": {{}, {}}, + }) + + require.NoError(t, prov.StopNodes(t.Context(), "demo")) + require.NoError(t, prov.StopNodes(t.Context(), "demo")) + require.NoError(t, prov.StartNodes(t.Context(), "demo")) + + assert.Equal(t, []string{ + "scale nodegroup --cluster demo --name ng-1 --nodes 0 --nodes-min 0 --nodes-max 5 --region us-east-1", + "scale nodegroup --cluster demo --name ng-1 --nodes 3 --nodes-min 2 --nodes-max 5 --region us-east-1", + }, scaleCalls(runner.calls)) +} + +func TestStopNodesRejectsInvalidCapacityBeforePersisting(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProvider(t, map[string][]response{ + "get nodegroup": {{stdout: nodegroupJSON(1, 2)}}, + }) + + err := prov.StopNodes(t.Context(), "demo") + require.Error(t, err) + assert.Empty(t, scaleCalls(runner.calls)) + + _, loadErr := state.LoadEKSNodegroupState("demo") + require.ErrorIs(t, loadErr, state.ErrEKSNodegroupStateNotFound) +} + +func TestStartNodesRejectsMismatchedCapacityStateBeforeScaling(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + saveNodegroupState(t, "demo", "eu-north-1", 3, 2, 5) + + prov, runner := newProvider(t, map[string][]response{ + "get nodegroup": {{stdout: nodegroupJSON(0, 0)}}, + }) + + err := prov.StartNodes(t.Context(), "demo") + require.Error(t, err) + assert.Contains(t, err.Error(), "region") + assert.Empty(t, scaleCalls(runner.calls)) +} + +func TestStartNodesRetainsCapacityStateWhenScalingFails(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + saveNodegroupState(t, "demo", "us-east-1", 3, 2, 5) + + prov, _ := newProvider(t, map[string][]response{ + "get nodegroup": {{stdout: nodegroupJSON(0, 0)}}, + "scale nodegroup": {{err: errScaleDenied}}, + }) + + err := prov.StartNodes(t.Context(), "demo") + require.ErrorIs(t, err, errScaleDenied) + + saved, loadErr := state.LoadEKSNodegroupState("demo") + require.NoError(t, loadErr) + assert.Equal(t, 3, saved.Nodegroups[0].DesiredCapacity) + assert.Equal(t, 2, saved.Nodegroups[0].MinSize) +} + +func TestStartNodesRequiresSnapshotWhenMinimumIsZero(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProvider(t, map[string][]response{ + "get nodegroup": {{stdout: nodegroupJSON(0, 0)}}, + }) + + err := prov.StartNodes(t.Context(), "demo") + require.Error(t, err) + assert.Contains(t, err.Error(), "pre-stop snapshot") + assert.Empty(t, scaleCalls(runner.calls)) +} + +func TestStartNodesWithoutSnapshotPreflightsEveryNodegroup(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProvider(t, map[string][]response{ + "get nodegroup": {{stdout: []byte(`[ + {"Cluster":"demo","Name":"a-recoverable","DesiredCapacity":0,"MinSize":2,"MaxSize":5}, + {"Cluster":"demo","Name":"z-missing-state","DesiredCapacity":0,"MinSize":0,"MaxSize":5} + ]`)}}, + }) + + err := prov.StartNodes(t.Context(), "demo") + require.Error(t, err) + assert.Contains(t, err.Error(), "z-missing-state") + assert.Empty(t, scaleCalls(runner.calls)) +} + +func TestStartNodesRetainsCapacityStateUntilReadbackMatches(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + saveNodegroupState(t, "demo", "us-east-1", 3, 2, 5) + + prov, _ := newProvider(t, map[string][]response{ + "get nodegroup": { + {stdout: nodegroupJSON(0, 0)}, + {stdout: nodegroupJSON(0, 0)}, + }, + "scale nodegroup": {{}}, + }) + + err := prov.StartNodes(t.Context(), "demo") + require.Error(t, err) + assert.Contains(t, err.Error(), "not restored") + + _, loadErr := state.LoadEKSNodegroupState("demo") + require.NoError(t, loadErr) +} + +func nodegroupJSON(desiredCapacity, minSize int) []byte { + return fmt.Appendf( + nil, + `[{"Cluster":"demo","Name":"ng-1","DesiredCapacity":%d,"MinSize":%d,"MaxSize":5}]`, + desiredCapacity, + minSize, + ) +} + +func saveNodegroupState( + t *testing.T, + clusterName, region string, + desiredCapacity, minSize, maxSize int, +) { + t.Helper() + + require.NoError(t, state.SaveEKSNodegroupState(clusterName, &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: clusterName, + Region: region, + Nodegroups: []state.EKSNodegroupCapacity{ + { + Name: "ng-1", + DesiredCapacity: desiredCapacity, + MinSize: minSize, + MaxSize: maxSize, + }, + }, + })) +} + +func scaleCalls(calls [][]string) []string { + scales := make([]string, 0) + + for _, call := range calls { + if len(call) >= 2 && call[0] == "scale" && call[1] == "nodegroup" { + scales = append(scales, strings.Join(call, " ")) + } + } + + return scales +} + func TestStartNodes_NoNodegroups(t *testing.T) { t.Parallel() diff --git a/pkg/svc/state/eks_nodegroup_state.go b/pkg/svc/state/eks_nodegroup_state.go new file mode 100644 index 0000000000..b341d73fe8 --- /dev/null +++ b/pkg/svc/state/eks_nodegroup_state.go @@ -0,0 +1,121 @@ +package state + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + + "github.com/devantler-tech/ksail/v7/pkg/fsutil" +) + +const ( + // EKSNodegroupStateVersion is the on-disk schema version for EKS capacity snapshots. + EKSNodegroupStateVersion = 1 + // eksNodegroupStateFileName is kept separate from spec and TTL state so a successful start can + // clear only the transient capacity snapshot. + eksNodegroupStateFileName = "eks-nodegroups.json" +) + +var ( + // ErrEKSNodegroupStateNotFound reports that no stop-time EKS capacity snapshot exists. + ErrEKSNodegroupStateNotFound = errors.New("EKS nodegroup state not found") + errEKSNodegroupStateNil = errors.New("EKS nodegroup state is nil") +) + +// EKSNodegroupCapacity records the exact scaling values a stopped managed nodegroup must regain. +type EKSNodegroupCapacity struct { + Name string `json:"name"` + DesiredCapacity int `json:"desiredCapacity"` + MinSize int `json:"minSize"` + MaxSize int `json:"maxSize"` +} + +// EKSNodegroupState binds a stop-time capacity snapshot to one cluster name and AWS region. +type EKSNodegroupState struct { + Version int `json:"version"` + ClusterName string `json:"clusterName"` + Region string `json:"region"` + Nodegroups []EKSNodegroupCapacity `json:"nodegroups"` +} + +// SaveEKSNodegroupState atomically persists the capacity snapshot before stop mutates AWS. +func SaveEKSNodegroupState(clusterName string, snapshot *EKSNodegroupState) error { + statePath, err := eksNodegroupStatePath(clusterName) + if err != nil { + return err + } + + if snapshot == nil { + return errEKSNodegroupStateNil + } + + err = os.MkdirAll(filepath.Dir(statePath), dirPermissions) + if err != nil { + return fmt.Errorf("failed to create EKS nodegroup state directory: %w", err) + } + + data, err := json.MarshalIndent(snapshot, "", " ") + if err != nil { + return fmt.Errorf("failed to marshal EKS nodegroup state: %w", err) + } + + err = fsutil.AtomicWriteFile(statePath, data, filePermissions) + if err != nil { + return fmt.Errorf("failed to write EKS nodegroup state: %w", err) + } + + return nil +} + +// LoadEKSNodegroupState reads the stop-time EKS capacity snapshot. +func LoadEKSNodegroupState(clusterName string) (*EKSNodegroupState, error) { + statePath, err := eksNodegroupStatePath(clusterName) + if err != nil { + return nil, err + } + + //nolint:gosec // clusterStateDir validates the user-controlled path component. + data, err := os.ReadFile(statePath) + if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf("%w: %s", ErrEKSNodegroupStateNotFound, clusterName) + } + + return nil, fmt.Errorf("failed to read EKS nodegroup state: %w", err) + } + + var snapshot EKSNodegroupState + + err = json.Unmarshal(data, &snapshot) + if err != nil { + return nil, fmt.Errorf("failed to unmarshal EKS nodegroup state: %w", err) + } + + return &snapshot, nil +} + +// DeleteEKSNodegroupState removes only the transient EKS capacity snapshot. +func DeleteEKSNodegroupState(clusterName string) error { + statePath, err := eksNodegroupStatePath(clusterName) + if err != nil { + return err + } + + err = os.Remove(statePath) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove EKS nodegroup state: %w", err) + } + + return nil +} + +func eksNodegroupStatePath(clusterName string) (string, error) { + dir, err := clusterStateDir(clusterName) + if err != nil { + return "", err + } + + return filepath.Join(dir, eksNodegroupStateFileName), nil +} diff --git a/pkg/svc/state/eks_nodegroup_state_test.go b/pkg/svc/state/eks_nodegroup_state_test.go new file mode 100644 index 0000000000..b9dd4877c2 --- /dev/null +++ b/pkg/svc/state/eks_nodegroup_state_test.go @@ -0,0 +1,63 @@ +package state_test + +import ( + "os" + "path/filepath" + "testing" + + "github.com/devantler-tech/ksail/v7/pkg/svc/state" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSaveLoadAndDeleteEKSNodegroupState(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + const clusterName = "eks-capacity-round-trip" + + want := &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: clusterName, + Region: "eu-north-1", + Nodegroups: []state.EKSNodegroupCapacity{ + {Name: "workers", DesiredCapacity: 3, MinSize: 2, MaxSize: 5}, + }, + } + + require.NoError(t, state.SaveEKSNodegroupState(clusterName, want)) + + got, err := state.LoadEKSNodegroupState(clusterName) + require.NoError(t, err) + assert.Equal(t, want, got) + + statePath := filepath.Join(home, ".ksail", "clusters", clusterName, "eks-nodegroups.json") + info, err := os.Stat(statePath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + + neighborPath := filepath.Join(filepath.Dir(statePath), "spec.json") + require.NoError(t, os.WriteFile(neighborPath, []byte("neighbor"), 0o600)) + + require.NoError(t, state.DeleteEKSNodegroupState(clusterName)) + assert.FileExists(t, neighborPath) + + _, err = state.LoadEKSNodegroupState(clusterName) + require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) +} + +func TestLoadEKSNodegroupStateRejectsInvalidJSON(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + stateDir := filepath.Join(home, ".ksail", "clusters", "invalid-json") + require.NoError(t, os.MkdirAll(stateDir, 0o700)) + require.NoError( + t, + os.WriteFile(filepath.Join(stateDir, "eks-nodegroups.json"), []byte("{"), 0o600), + ) + + _, err := state.LoadEKSNodegroupState("invalid-json") + require.Error(t, err) + assert.Contains(t, err.Error(), "unmarshal EKS nodegroup state") +} From abfb499dec7352c31e61603456ed8559c1ec2144 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Fri, 17 Jul 2026 11:49:15 +0200 Subject: [PATCH 05/10] refactor(aws): share EKS nodegroup preflight --- pkg/svc/provider/aws/provider.go | 33 ++++++++++++++++++++------------ 1 file changed, 21 insertions(+), 12 deletions(-) diff --git a/pkg/svc/provider/aws/provider.go b/pkg/svc/provider/aws/provider.go index 728681114a..e0b9552769 100644 --- a/pkg/svc/provider/aws/provider.go +++ b/pkg/svc/provider/aws/provider.go @@ -94,12 +94,7 @@ func NewProvider(client *eksctlclient.Client, region string, opts ...Option) (*P // before StopNodes zeroed it. The snapshot survives partial failures and is removed only after a // readback confirms that every group is restored. func (p *Provider) StartNodes(ctx context.Context, clusterName string) error { - nodegroups, err := p.listNodegroupsForScale(ctx, clusterName) - if err != nil { - return err - } - - snapshot, found, err := p.loadNodegroupState(clusterName) + nodegroups, snapshot, found, err := p.loadNodegroupsAndState(ctx, clusterName) if err != nil { return err } @@ -124,12 +119,7 @@ func (p *Provider) StartNodes(ctx context.Context, clusterName string) error { // StopNodes atomically snapshots every managed nodegroup before scaling it to zero. Repeated calls // preserve the first snapshot and skip already-stopped groups, so a partial stop remains retryable. func (p *Provider) StopNodes(ctx context.Context, clusterName string) error { - nodegroups, err := p.listNodegroupsForScale(ctx, clusterName) - if err != nil { - return err - } - - snapshot, found, err := p.loadNodegroupState(clusterName) + nodegroups, snapshot, found, err := p.loadNodegroupsAndState(ctx, clusterName) if err != nil { return err } @@ -345,6 +335,25 @@ func (p *Provider) fetchNodegroups( return nodegroups, nil } +// loadNodegroupsAndState gives StartNodes and StopNodes one shared, ordered preflight: first prove +// that the target has managed nodegroups, then load any capacity snapshot before either path mutates it. +func (p *Provider) loadNodegroupsAndState( + ctx context.Context, + clusterName string, +) ([]eksctlclient.NodegroupSummary, *state.EKSNodegroupState, bool, error) { + nodegroups, err := p.listNodegroupsForScale(ctx, clusterName) + if err != nil { + return nil, nil, false, err + } + + snapshot, found, err := p.loadNodegroupState(clusterName) + if err != nil { + return nil, nil, false, err + } + + return nodegroups, snapshot, found, nil +} + // listNodegroupsForScale is a shared prelude for StartNodes and StopNodes // that guards against nil clients, fetches nodegroups, and classifies the // empty result as provider.ErrNoNodes. From d484fe0a229c6669076f8db14b67d17cab0e1c79 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Fri, 17 Jul 2026 11:57:38 +0200 Subject: [PATCH 06/10] test(aws): share nodegroup command literal --- pkg/svc/provider/aws/provider_test.go | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/pkg/svc/provider/aws/provider_test.go b/pkg/svc/provider/aws/provider_test.go index f2c5ed689a..a0e68197b1 100644 --- a/pkg/svc/provider/aws/provider_test.go +++ b/pkg/svc/provider/aws/provider_test.go @@ -21,7 +21,10 @@ import ( // testEndpoint is the control-plane endpoint the default fake describer // returns so GetClusterStatus success paths never resolve real AWS // credentials. -const testEndpoint = "https://ABCDEF0123456789.gr7.us-east-1.eks.amazonaws.com" +const ( + testEndpoint = "https://ABCDEF0123456789.gr7.us-east-1.eks.amazonaws.com" + nodegroupSubcommand = "nodegroup" +) // fakeDescriber is a credential-free stand-in for the AWS-SDK EKS // DescribeCluster seam. It records the requested name and returns the @@ -216,7 +219,7 @@ func TestStopNodes_ScalesAllToZero(t *testing.T) { var scales [][]string for _, call := range runner.calls { - if len(call) >= 2 && call[0] == "scale" && call[1] == "nodegroup" { + if len(call) >= 2 && call[0] == "scale" && call[1] == nodegroupSubcommand { scales = append(scales, call) } } @@ -269,7 +272,7 @@ func TestStartNodes_ScalesOnlyZeroDesired(t *testing.T) { scales := 0 for _, call := range runner.calls { - if len(call) >= 2 && call[0] == "scale" && call[1] == "nodegroup" { + if len(call) >= 2 && call[0] == "scale" && call[1] == nodegroupSubcommand { scales++ joined := strings.Join(call, " ") @@ -307,7 +310,7 @@ func TestStopThenStartRestoresNodegroupCapacities(t *testing.T) { var scales []string for _, call := range runner.calls { - if len(call) >= 2 && call[0] == "scale" && call[1] == "nodegroup" { + if len(call) >= 2 && call[0] == "scale" && call[1] == nodegroupSubcommand { scales = append(scales, strings.Join(call, " ")) } } @@ -475,7 +478,7 @@ func scaleCalls(calls [][]string) []string { scales := make([]string, 0) for _, call := range calls { - if len(call) >= 2 && call[0] == "scale" && call[1] == "nodegroup" { + if len(call) >= 2 && call[0] == "scale" && call[1] == nodegroupSubcommand { scales = append(scales, strings.Join(call, " ")) } } From 0eb6de39d69d10e509c1251caad9580f6a54cc9b Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Fri, 17 Jul 2026 12:08:51 +0200 Subject: [PATCH 07/10] test(aws): share scale command literal --- pkg/svc/provider/aws/provider_test.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/svc/provider/aws/provider_test.go b/pkg/svc/provider/aws/provider_test.go index a0e68197b1..21b2cbe14c 100644 --- a/pkg/svc/provider/aws/provider_test.go +++ b/pkg/svc/provider/aws/provider_test.go @@ -23,6 +23,7 @@ import ( // credentials. const ( testEndpoint = "https://ABCDEF0123456789.gr7.us-east-1.eks.amazonaws.com" + scaleSubcommand = "scale" nodegroupSubcommand = "nodegroup" ) @@ -219,7 +220,7 @@ func TestStopNodes_ScalesAllToZero(t *testing.T) { var scales [][]string for _, call := range runner.calls { - if len(call) >= 2 && call[0] == "scale" && call[1] == nodegroupSubcommand { + if len(call) >= 2 && call[0] == scaleSubcommand && call[1] == nodegroupSubcommand { scales = append(scales, call) } } @@ -272,7 +273,7 @@ func TestStartNodes_ScalesOnlyZeroDesired(t *testing.T) { scales := 0 for _, call := range runner.calls { - if len(call) >= 2 && call[0] == "scale" && call[1] == nodegroupSubcommand { + if len(call) >= 2 && call[0] == scaleSubcommand && call[1] == nodegroupSubcommand { scales++ joined := strings.Join(call, " ") @@ -310,7 +311,7 @@ func TestStopThenStartRestoresNodegroupCapacities(t *testing.T) { var scales []string for _, call := range runner.calls { - if len(call) >= 2 && call[0] == "scale" && call[1] == nodegroupSubcommand { + if len(call) >= 2 && call[0] == scaleSubcommand && call[1] == nodegroupSubcommand { scales = append(scales, strings.Join(call, " ")) } } @@ -478,7 +479,7 @@ func scaleCalls(calls [][]string) []string { scales := make([]string, 0) for _, call := range calls { - if len(call) >= 2 && call[0] == "scale" && call[1] == nodegroupSubcommand { + if len(call) >= 2 && call[0] == scaleSubcommand && call[1] == nodegroupSubcommand { scales = append(scales, strings.Join(call, " ")) } } From 56001ecc91dbe12e9d28366bc192ce5553e4b6df Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Fri, 17 Jul 2026 14:02:17 +0200 Subject: [PATCH 08/10] fix(eks): address lifecycle review findings --- pkg/cli/clusterapi/local_service.go | 30 +++ pkg/cli/clusterapi/local_service_test.go | 236 ++++++++++++++++++ pkg/cli/cmd/cluster/mutation.go | 46 ++++ .../cmd/cluster/post_create_context_test.go | 128 ++++++++-- pkg/svc/provider/aws/nodegroup_state.go | 13 +- pkg/svc/provider/aws/provider.go | 89 ++++++- pkg/svc/provider/aws/provider_test.go | 189 +++++++++++++- 7 files changed, 696 insertions(+), 35 deletions(-) diff --git a/pkg/cli/clusterapi/local_service.go b/pkg/cli/clusterapi/local_service.go index d724f5212f..fb59d7aa17 100644 --- a/pkg/cli/clusterapi/local_service.go +++ b/pkg/cli/clusterapi/local_service.go @@ -24,6 +24,7 @@ import ( "github.com/devantler-tech/ksail/v7/pkg/svc/credentials" clusterprovisioner "github.com/devantler-tech/ksail/v7/pkg/svc/provisioner/cluster" "github.com/devantler-tech/ksail/v7/pkg/svc/provisioner/cluster/clustererr" + "github.com/devantler-tech/ksail/v7/pkg/svc/state" "github.com/devantler-tech/ksail/v7/pkg/webui/api" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -516,6 +517,16 @@ func (s *Service) startJob( } s.mu.Lock() + if current, found := s.jobs[name]; found && jobIsInProgress(current.phase) { + s.mu.Unlock() + + return v1alpha1.Spec{}, fmt.Errorf( + "%w: operation already in progress for %q", + api.ErrAlreadyExists, + name, + ) + } + s.jobs[name] = &job{ distribution: distribution, provider: provider, @@ -529,6 +540,12 @@ func (s *Service) startJob( }, nil } +func jobIsInProgress(phase v1alpha1.ClusterPhase) bool { + return phase == v1alpha1.ClusterPhaseProvisioning || + phase == v1alpha1.ClusterPhaseDeleting || + phase == v1alpha1.ClusterPhaseUpdating +} + // runLifecycle marks a resolved cluster Updating, then runs the supplied provisioner action (Start or // Stop) in the background, clearing the job on success and recording the failure otherwise — the same // async pattern Create/Delete use so the web UI's list reflects progress without a UI change. @@ -588,6 +605,12 @@ func (s *Service) runCreate(ctx context.Context, name string, spec v1alpha1.Spec return p.Create(actionCtx, name) }, ) + if err == nil && spec.Cluster.Distribution == v1alpha1.DistributionEKS { + err = state.SaveClusterSpec(name, &spec.Cluster) + if err != nil { + err = fmt.Errorf("persist local EKS cluster ownership state: %w", err) + } + } s.mu.Lock() defer s.mu.Unlock() @@ -619,6 +642,13 @@ func (s *Service) runDelete(ctx context.Context, name string, spec v1alpha1.Spec return p.Delete(actionCtx, name) }, ) + if spec.Cluster.Distribution == v1alpha1.DistributionEKS && + (err == nil || errors.Is(err, clustererr.ErrClusterNotFound)) { + cleanupErr := state.DeleteClusterState(name) + if cleanupErr != nil { + err = fmt.Errorf("clean up local EKS cluster state after deletion: %w", cleanupErr) + } + } s.mu.Lock() defer s.mu.Unlock() diff --git a/pkg/cli/clusterapi/local_service_test.go b/pkg/cli/clusterapi/local_service_test.go index 9802f9b591..a4feb57a07 100644 --- a/pkg/cli/clusterapi/local_service_test.go +++ b/pkg/cli/clusterapi/local_service_test.go @@ -15,6 +15,7 @@ import ( "github.com/devantler-tech/ksail/v7/pkg/svc/clusterdiscovery" clusterprovisioner "github.com/devantler-tech/ksail/v7/pkg/svc/provisioner/cluster" "github.com/devantler-tech/ksail/v7/pkg/svc/provisioner/cluster/clustererr" + "github.com/devantler-tech/ksail/v7/pkg/svc/state" "github.com/devantler-tech/ksail/v7/pkg/webui/api" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -44,6 +45,8 @@ type fakeProvisioner struct { clusters []string createGate chan struct{} deleteGate chan struct{} + startGate chan struct{} + stopGate chan struct{} createErr error deleteErr error startErr error @@ -108,6 +111,10 @@ func (f *fakeProvisioner) List(_ context.Context) ([]string, error) { } func (f *fakeProvisioner) Start(_ context.Context, name string) error { + if f.startGate != nil { + <-f.startGate + } + f.mu.Lock() defer f.mu.Unlock() @@ -117,6 +124,10 @@ func (f *fakeProvisioner) Start(_ context.Context, name string) error { } func (f *fakeProvisioner) Stop(_ context.Context, name string) error { + if f.stopGate != nil { + <-f.stopGate + } + f.mu.Lock() defer f.mu.Unlock() @@ -410,6 +421,158 @@ func TestDeleteIsAsyncAndRemovesCluster(t *testing.T) { assert.Equal(t, []string{"old"}, provisioner.deletedNames()) } +func TestDeleteEKSClearsPersistedOwnershipAndCapacityState(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "old-eks" + + provisioner := &fakeProvisioner{} + service := newTestService(map[v1alpha1.Distribution]*fakeProvisioner{ + v1alpha1.DistributionEKS: provisioner, + }) + _, err := service.Create( + context.Background(), + clusterFor(clusterName, v1alpha1.DistributionEKS), + ) + require.NoError(t, err) + require.Eventually(t, func() bool { + list, listErr := service.List(context.Background()) + require.NoError(t, listErr) + + phase, found := phaseOf(list, clusterName) + + return found && phase == v1alpha1.ClusterPhaseReady + }, eventuallyTimeout, eventuallyTick) + + require.NoError(t, state.SaveClusterSpec(clusterName, &v1alpha1.ClusterSpec{ + Distribution: v1alpha1.DistributionEKS, + Provider: v1alpha1.ProviderAWS, + })) + require.NoError(t, state.SaveEKSNodegroupState(clusterName, &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: clusterName, + Region: "eu-north-1", + Nodegroups: []state.EKSNodegroupCapacity{ + {Name: "workers", DesiredCapacity: 2, MinSize: 1, MaxSize: 3}, + }, + })) + + require.NoError(t, service.Delete(context.Background(), "default", clusterName)) + require.Eventually(t, func() bool { + _, specErr := state.LoadClusterSpec(clusterName) + _, capacityErr := state.LoadEKSNodegroupState(clusterName) + + return errors.Is(specErr, state.ErrStateNotFound) && + errors.Is(capacityErr, state.ErrEKSNodegroupStateNotFound) + }, eventuallyTimeout, eventuallyTick) +} + +func TestDeleteEKSRejectsOverlappingCreate(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "creating-eks" + + createGate := make(chan struct{}) + provisioner := &fakeProvisioner{createGate: createGate} + service := newTestService(map[v1alpha1.Distribution]*fakeProvisioner{ + v1alpha1.DistributionEKS: provisioner, + }) + _, err := service.Create( + context.Background(), + clusterFor(clusterName, v1alpha1.DistributionEKS), + ) + require.NoError(t, err) + + err = service.Delete(context.Background(), "default", clusterName) + require.ErrorIs(t, err, api.ErrAlreadyExists) + assert.Empty(t, provisioner.deletedNames()) + + close(createGate) + require.Eventually(t, func() bool { + list, listErr := service.List(context.Background()) + require.NoError(t, listErr) + + phase, found := phaseOf(list, clusterName) + + return found && phase == v1alpha1.ClusterPhaseReady + }, eventuallyTimeout, eventuallyTick) + + persisted, loadErr := state.LoadClusterSpec(clusterName) + require.NoError(t, loadErr) + assert.Equal(t, v1alpha1.DistributionEKS, persisted.Distribution) + assert.Equal(t, v1alpha1.ProviderAWS, persisted.Provider) +} + +func TestLifecycleRejectsOverlappingOperation(t *testing.T) { + t.Parallel() + + const clusterName = "busy" + + stopGate := make(chan struct{}) + provisioner := &fakeProvisioner{clusters: []string{clusterName}, stopGate: stopGate} + service := newTestService(map[v1alpha1.Distribution]*fakeProvisioner{ + v1alpha1.DistributionVCluster: provisioner, + }) + + require.NoError(t, service.Stop(context.Background(), "default", clusterName)) + err := service.Start(context.Background(), "default", clusterName) + require.ErrorIs(t, err, api.ErrAlreadyExists) + assert.Empty(t, provisioner.startedNames()) + + close(stopGate) + require.Eventually(t, func() bool { + return slices.Equal(provisioner.stoppedNames(), []string{clusterName}) + }, eventuallyTimeout, eventuallyTick) +} + +func TestDeleteEKSFailurePreservesPersistedOwnershipAndCapacityState(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "stuck-eks" + + provisioner := &fakeProvisioner{deleteErr: errSimulatedDeleteFailure} + service := newTestService(map[v1alpha1.Distribution]*fakeProvisioner{ + v1alpha1.DistributionEKS: provisioner, + }) + _, err := service.Create( + context.Background(), + clusterFor(clusterName, v1alpha1.DistributionEKS), + ) + require.NoError(t, err) + require.Eventually(t, func() bool { + list, listErr := service.List(context.Background()) + require.NoError(t, listErr) + + phase, found := phaseOf(list, clusterName) + + return found && phase == v1alpha1.ClusterPhaseReady + }, eventuallyTimeout, eventuallyTick) + require.NoError(t, state.SaveEKSNodegroupState(clusterName, &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: clusterName, + Region: "eu-north-1", + Nodegroups: []state.EKSNodegroupCapacity{ + {Name: "workers", DesiredCapacity: 2, MinSize: 1, MaxSize: 3}, + }, + })) + + require.NoError(t, service.Delete(context.Background(), "default", clusterName)) + require.Eventually(t, func() bool { + list, listErr := service.List(context.Background()) + require.NoError(t, listErr) + + phase, found := phaseOf(list, clusterName) + + return found && phase == v1alpha1.ClusterPhaseFailed + }, eventuallyTimeout, eventuallyTick) + + _, specErr := state.LoadClusterSpec(clusterName) + require.NoError(t, specErr) + + _, capacityErr := state.LoadEKSNodegroupState(clusterName) + require.NoError(t, capacityErr) +} + // TestDeleteClearsFailedClusterWithNoUnderlyingCluster reproduces the bug where a cluster left in // the Failed phase by a failed create could never be removed from the web UI: deleting it called // the provisioner's Delete, which returned ErrClusterNotFound (there is no cluster to delete), and @@ -856,6 +1019,79 @@ func TestCreateDefaultsEKSProviderToAWS(t *testing.T) { case <-time.After(eventuallyTimeout): t.Fatal("factory was never asked to build the EKS cluster with a defaulted AWS provider") } + + var persisted *v1alpha1.ClusterSpec + + require.Eventually(t, func() bool { + var loadErr error + + persisted, loadErr = state.LoadClusterSpec("prod-eks") + + return loadErr == nil + }, eventuallyTimeout, eventuallyTick) + require.NotNil(t, persisted) + assert.Equal(t, v1alpha1.DistributionEKS, persisted.Distribution) + assert.Equal(t, v1alpha1.ProviderAWS, persisted.Provider) +} + +func TestCreateEKSFailureDoesNotPersistOwnershipState(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "failed-eks" + + provisioner := &fakeProvisioner{createErr: errSimulatedCreateFailure} + service := newTestService(map[v1alpha1.Distribution]*fakeProvisioner{ + v1alpha1.DistributionEKS: provisioner, + }) + _, err := service.Create( + context.Background(), + clusterFor(clusterName, v1alpha1.DistributionEKS), + ) + require.NoError(t, err) + require.Eventually(t, func() bool { + list, listErr := service.List(context.Background()) + require.NoError(t, listErr) + + phase, found := phaseOf(list, clusterName) + + return found && phase == v1alpha1.ClusterPhaseFailed + }, eventuallyTimeout, eventuallyTick) + + _, loadErr := state.LoadClusterSpec(clusterName) + require.ErrorIs(t, loadErr, state.ErrStateNotFound) +} + +func TestCreateEKSOwnershipPersistenceFailureMarksJobFailed(t *testing.T) { + homeFile := filepath.Join(t.TempDir(), "home-file") + require.NoError(t, os.WriteFile(homeFile, []byte("not a directory"), 0o600)) + t.Setenv("HOME", homeFile) + + const clusterName = "untracked-eks" + + provisioner := &fakeProvisioner{} + service := newTestService(map[v1alpha1.Distribution]*fakeProvisioner{ + v1alpha1.DistributionEKS: provisioner, + }) + _, err := service.Create( + context.Background(), + clusterFor(clusterName, v1alpha1.DistributionEKS), + ) + require.NoError(t, err) + require.Eventually(t, func() bool { + list, listErr := service.List(context.Background()) + require.NoError(t, listErr) + + cluster := clusterNamed(list, clusterName) + + return cluster != nil && cluster.Status.Phase == v1alpha1.ClusterPhaseFailed + }, eventuallyTimeout, eventuallyTick) + + list, listErr := service.List(context.Background()) + require.NoError(t, listErr) + + conditions := conditionsOf(list, clusterName) + require.Len(t, conditions, 1) + assert.Contains(t, conditions[0].Message, "persist local EKS cluster ownership state") } // clusterNamed returns a pointer to the cluster with the given name in the list, or nil if absent. diff --git a/pkg/cli/cmd/cluster/mutation.go b/pkg/cli/cmd/cluster/mutation.go index 7bf0433d40..fd121cab89 100644 --- a/pkg/cli/cmd/cluster/mutation.go +++ b/pkg/cli/cmd/cluster/mutation.go @@ -30,6 +30,9 @@ var ( errEKSNameOverrideMismatch = errors.New("cannot override EKS config cluster name") errNoMatchingEKSContext = errors.New("no kubeconfig context matches EKS cluster") errAmbiguousEKSContext = errors.New("multiple kubeconfig contexts match EKS cluster") + errExplicitEKSContextUnobserved = errors.New( + "explicit EKS context was not observed after creation", + ) ) const explicitEKSContextHint = "set spec.cluster.connection.context explicitly" @@ -426,6 +429,10 @@ func resolvePostCreateContext(ctx *localregistry.Context) error { distribution := ctx.ClusterCfg.Spec.Cluster.Distribution if connection.Context != "" { + if distribution == v1alpha1.DistributionEKS { + return pinObservedExplicitEKSRegion(ctx, connection.Context) + } + return nil } @@ -442,6 +449,45 @@ func resolvePostCreateContext(ctx *localregistry.Context) error { return resolveEKSPostCreateContext(ctx) } +// pinObservedExplicitEKSRegion derives a profile-selected region only from the kubeconfig output +// eksctl just made current. Parsing configured text alone could bind TTL cleanup to a stale +// same-named context in another region. +func pinObservedExplicitEKSRegion(ctx *localregistry.Context, contextName string) error { + if ctx.EKSConfig != nil && strings.TrimSpace(ctx.EKSConfig.Region) != "" { + return nil + } + + clusterName, _, config, err := loadEKSContextConfig(ctx) + if err != nil { + return err + } + + observedContext, found := config.Contexts[contextName] + + contextCluster, _, validTarget := parseEksctlContextTarget(contextName) + if !found || observedContext == nil || config.CurrentContext != contextName || + !validTarget || contextCluster != clusterName { + return fmt.Errorf( + "%w: %q for cluster %q", + errExplicitEKSContextUnobserved, + contextName, + clusterName, + ) + } + + pinEKSRegionFromContext(ctx, contextName) + + if strings.TrimSpace(ctx.EKSConfig.Region) == "" { + return fmt.Errorf( + "%w: %q did not supply a region", + errExplicitEKSContextUnobserved, + contextName, + ) + } + + return nil +} + // resolveEKSPostCreateContext selects the identity-qualified context that // eksctl wrote for the configured cluster and region. func resolveEKSPostCreateContext(ctx *localregistry.Context) error { diff --git a/pkg/cli/cmd/cluster/post_create_context_test.go b/pkg/cli/cmd/cluster/post_create_context_test.go index 18f39ade49..4debd70762 100644 --- a/pkg/cli/cmd/cluster/post_create_context_test.go +++ b/pkg/cli/cmd/cluster/post_create_context_test.go @@ -184,28 +184,128 @@ func TestResolvePostCreateContext_EKSRegionDelegatedToProfile(t *testing.T) { assert.Equal(t, "eu-west-1", ctx.EKSConfig.Region) } -func TestResolvePostCreateContext_EKSExplicitContextDoesNotPinUnobservedRegion(t *testing.T) { +func TestResolvePostCreateContext_EKSExplicitContextPinsObservedRegion(t *testing.T) { t.Parallel() const explicitContext = "arn:aws:iam::123456789012:role/ci@st-eks.eu-west-1.eksctl.io" - ctx := &localregistry.Context{ - ClusterCfg: &v1alpha1.Cluster{ - Spec: v1alpha1.Spec{ - Cluster: v1alpha1.ClusterSpec{ - Distribution: v1alpha1.DistributionEKS, - Connection: v1alpha1.Connection{ - Context: explicitContext, - }, - }, - }, + ctx := newExplicitEKSPostCreateContext( + t, + "st-eks", + explicitContext, + explicitContext, + explicitContext, + ) + + require.NoError(t, cluster.ExportResolvePostCreateContext(ctx)) + assert.Equal(t, explicitContext, ctx.ClusterCfg.Spec.Cluster.Connection.Context) + assert.Equal(t, "eu-west-1", ctx.EKSConfig.Region) +} + +func TestResolvePostCreateContext_EKSRejectsUnobservedExplicitContext(t *testing.T) { + t.Parallel() + + const ( + targetContext = "arn:aws:iam::123456789012:role/ci@st-eks.eu-west-1.eksctl.io" + otherContext = "arn:aws:iam::123456789012:role/ci@st-eks.us-east-1.eksctl.io" + wrongTarget = "arn:aws:iam::123456789012:role/ci@other.eu-west-1.eksctl.io" + ) + + testCases := []struct { + name string + explicit string + current string + contexts []string + }{ + { + name: "missing from output", + explicit: targetContext, + current: otherContext, + contexts: []string{otherContext}, }, - EKSConfig: &clusterprovisioner.EKSConfig{Name: "st-eks"}, + { + name: "stale non-current context", + explicit: targetContext, + current: otherContext, + contexts: []string{targetContext, otherContext}, + }, + { + name: "wrong cluster", + explicit: wrongTarget, + current: wrongTarget, + contexts: []string{wrongTarget}, + }, + { + name: "malformed context", + explicit: "st-eks.eu.west.eksctl.io", + current: "st-eks.eu.west.eksctl.io", + contexts: []string{"st-eks.eu.west.eksctl.io"}, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + ctx := newExplicitEKSPostCreateContext( + t, + "st-eks", + testCase.explicit, + testCase.current, + testCase.contexts..., + ) + + err := cluster.ExportResolvePostCreateContext(ctx) + require.ErrorContains(t, err, "explicit EKS context was not observed after creation") + assert.Empty(t, ctx.EKSConfig.Region) + }) + } +} + +func TestResolvePostCreateContext_EKSExplicitContextPreservesConfiguredRegion(t *testing.T) { + t.Parallel() + + const explicitContext = "arn:aws:iam::123456789012:role/ci@st-eks.eu-west-1.eksctl.io" + + ctx := &localregistry.Context{ + ClusterCfg: &v1alpha1.Cluster{Spec: v1alpha1.Spec{Cluster: v1alpha1.ClusterSpec{ + Distribution: v1alpha1.DistributionEKS, + Connection: v1alpha1.Connection{Context: explicitContext}, + }}}, + EKSConfig: &clusterprovisioner.EKSConfig{Name: "st-eks", Region: "us-east-1"}, } require.NoError(t, cluster.ExportResolvePostCreateContext(ctx)) - assert.Equal(t, explicitContext, ctx.ClusterCfg.Spec.Cluster.Connection.Context) - assert.Empty(t, ctx.EKSConfig.Region) + assert.Equal(t, "us-east-1", ctx.EKSConfig.Region) +} + +func newExplicitEKSPostCreateContext( + t *testing.T, + clusterName, explicitContext, currentContext string, + contexts ...string, +) *localregistry.Context { + t.Helper() + + kubeconfigPath := filepath.Join(t.TempDir(), "kubeconfig") + config := clientcmdapi.NewConfig() + config.CurrentContext = currentContext + + for _, contextName := range contexts { + config.Contexts[contextName] = &clientcmdapi.Context{} + } + + require.NoError(t, clientcmd.WriteToFile(*config, kubeconfigPath)) + + return &localregistry.Context{ + ClusterCfg: &v1alpha1.Cluster{Spec: v1alpha1.Spec{Cluster: v1alpha1.ClusterSpec{ + Distribution: v1alpha1.DistributionEKS, + Connection: v1alpha1.Connection{ + Context: explicitContext, + Kubeconfig: kubeconfigPath, + }, + }}}, + EKSConfig: &clusterprovisioner.EKSConfig{Name: clusterName}, + } } func TestApplyClusterNameOverride_EKSPreservesSourceConfigAndDefersContextResolution(t *testing.T) { diff --git a/pkg/svc/provider/aws/nodegroup_state.go b/pkg/svc/provider/aws/nodegroup_state.go index ed6c79da23..8e467e6d4a 100644 --- a/pkg/svc/provider/aws/nodegroup_state.go +++ b/pkg/svc/provider/aws/nodegroup_state.go @@ -39,14 +39,6 @@ func newNodegroupState( return nil, err } - if nodegroup.DesiredCap == 0 { - return nil, fmt.Errorf( - "nodegroup %q is already stopped without a saved capacity: %w", - nodegroup.Name, - errNodegroupStateMissing, - ) - } - if _, duplicate := seen[nodegroup.Name]; duplicate { return nil, fmt.Errorf( "duplicate nodegroup %q: %w", @@ -178,8 +170,9 @@ func validateSavedCapacities( for _, capacity := range capacities { if strings.TrimSpace(capacity.Name) == "" || capacity.MinSize < 0 || - capacity.DesiredCapacity <= 0 || - capacity.DesiredCapacity < capacity.MinSize || capacity.MaxSize < capacity.DesiredCapacity { + capacity.DesiredCapacity < 0 || + capacity.DesiredCapacity < capacity.MinSize || capacity.MaxSize <= 0 || + capacity.MaxSize < capacity.DesiredCapacity { return nil, fmt.Errorf( "invalid saved nodegroup capacity for %q: %w", capacity.Name, diff --git a/pkg/svc/provider/aws/provider.go b/pkg/svc/provider/aws/provider.go index e0b9552769..7b4eb7d1a9 100644 --- a/pkg/svc/provider/aws/provider.go +++ b/pkg/svc/provider/aws/provider.go @@ -94,38 +94,38 @@ func NewProvider(client *eksctlclient.Client, region string, opts ...Option) (*P // before StopNodes zeroed it. The snapshot survives partial failures and is removed only after a // readback confirms that every group is restored. func (p *Provider) StartNodes(ctx context.Context, clusterName string) error { - nodegroups, snapshot, found, err := p.loadNodegroupsAndState(ctx, clusterName) + target, nodegroups, snapshot, found, err := p.loadLifecycleNodegroupsAndState(ctx, clusterName) if err != nil { return err } if !found { - return p.startNodegroupsWithoutSnapshot(ctx, clusterName, nodegroups) + return target.startNodegroupsWithoutSnapshot(ctx, clusterName, nodegroups) } - liveByName, err := validateNodegroupTransition(clusterName, p.region, snapshot, nodegroups) + liveByName, err := validateNodegroupTransition(clusterName, target.region, snapshot, nodegroups) if err != nil { return err } - err = p.restoreSavedNodegroups(ctx, clusterName, snapshot, liveByName) + err = target.restoreSavedNodegroups(ctx, clusterName, snapshot, liveByName) if err != nil { return err } - return p.verifyAndClearNodegroupState(ctx, clusterName, snapshot) + return target.verifyAndClearNodegroupState(ctx, clusterName, snapshot) } // StopNodes atomically snapshots every managed nodegroup before scaling it to zero. Repeated calls // preserve the first snapshot and skip already-stopped groups, so a partial stop remains retryable. func (p *Provider) StopNodes(ctx context.Context, clusterName string) error { - nodegroups, snapshot, found, err := p.loadNodegroupsAndState(ctx, clusterName) + target, nodegroups, snapshot, found, err := p.loadLifecycleNodegroupsAndState(ctx, clusterName) if err != nil { return err } if !found { - snapshot, err = newNodegroupState(clusterName, p.region, nodegroups) + snapshot, err = newNodegroupState(clusterName, target.region, nodegroups) if err != nil { return err } @@ -136,7 +136,7 @@ func (p *Provider) StopNodes(ctx context.Context, clusterName string) error { } } - liveByName, err := validateNodegroupTransition(clusterName, p.region, snapshot, nodegroups) + liveByName, err := validateNodegroupTransition(clusterName, target.region, snapshot, nodegroups) if err != nil { return err } @@ -146,11 +146,11 @@ func (p *Provider) StopNodes(ctx context.Context, clusterName string) error { continue } - err = p.client.ScaleNodegroup( + err = target.client.ScaleNodegroup( ctx, clusterName, capacity.Name, - p.region, + target.region, 0, 0, capacity.MaxSize, @@ -272,6 +272,75 @@ func (p *Provider) Region() string { return p.region } +// loadLifecycleNodegroupsAndState gives both nodegroup lifecycle paths one exact-region preflight. +func (p *Provider) loadLifecycleNodegroupsAndState( + ctx context.Context, + clusterName string, +) ( + *Provider, + []eksctlclient.NodegroupSummary, + *state.EKSNodegroupState, + bool, + error, +) { + target, err := p.withResolvedLifecycleRegion(ctx, clusterName) + if err != nil { + return nil, nil, nil, false, err + } + + nodegroups, snapshot, found, err := target.loadNodegroupsAndState(ctx, clusterName) + if err != nil { + return nil, nil, nil, false, err + } + + return target, nodegroups, snapshot, found, nil +} + +// withResolvedLifecycleRegion returns a request-local provider pinned to the exact region eksctl +// observed for clusterName. When configuration delegates region selection to an AWS profile, every +// read, mutation, and persisted snapshot in this lifecycle operation must use that same resolved +// identity; mutating the shared Provider would race concurrent requests. +func (p *Provider) withResolvedLifecycleRegion( + ctx context.Context, + clusterName string, +) (*Provider, error) { + configuredRegion := strings.TrimSpace(p.region) + if configuredRegion != "" { + if configuredRegion == p.region { + return p, nil + } + + resolved := *p + resolved.region = configuredRegion + + return &resolved, nil + } + + if p.client == nil { + return nil, provider.ErrProviderUnavailable + } + + summary, err := p.client.GetCluster(ctx, clusterName, "") + if err != nil { + return nil, fmt.Errorf("resolve EKS lifecycle region: %w", translateClientErr(err)) + } + + targetName := strings.TrimSpace(clusterName) + if summary == nil || targetName == "" || strings.TrimSpace(summary.Name) != targetName || + strings.TrimSpace(summary.Region) == "" { + return nil, fmt.Errorf( + "profile-resolved EKS cluster %q did not prove its exact name and region: %w", + clusterName, + errNodegroupStateTarget, + ) + } + + resolved := *p + resolved.region = strings.TrimSpace(summary.Region) + + return &resolved, nil +} + // clusterEndpoint reads the cluster's control-plane endpoint from the AWS SDK, // which eksctl's cluster summary omits — bringing EKS to parity with the GKE // and Azure providers, which both surface the endpoint on their status. The diff --git a/pkg/svc/provider/aws/provider_test.go b/pkg/svc/provider/aws/provider_test.go index 21b2cbe14c..7e5d2ace1d 100644 --- a/pkg/svc/provider/aws/provider_test.go +++ b/pkg/svc/provider/aws/provider_test.go @@ -103,6 +103,16 @@ func (s *scriptedRunner) Run( func newProvider(t *testing.T, responses map[string][]response) (*aws.Provider, *scriptedRunner) { t.Helper() + return newProviderWithRegion(t, responses, "us-east-1") +} + +func newProviderWithRegion( + t *testing.T, + responses map[string][]response, + region string, +) (*aws.Provider, *scriptedRunner) { + t.Helper() + runner := &scriptedRunner{t: t, responses: responses} client := eksctlclient.NewClient( @@ -113,7 +123,7 @@ func newProvider(t *testing.T, responses map[string][]response) (*aws.Provider, // Inject a happy describer so GetClusterStatus success paths populate the // endpoint without resolving real AWS credentials. Tests that exercise the // endpoint itself construct the provider with their own describer inline. - prov, err := aws.NewProvider(client, "us-east-1", aws.WithClusterDescriber( + prov, err := aws.NewProvider(client, region, aws.WithClusterDescriber( &fakeDescriber{cluster: &ekstypes.Cluster{Endpoint: awssdk.String(testEndpoint)}}, )) require.NoError(t, err) @@ -230,6 +240,130 @@ func TestStopNodes_ScalesAllToZero(t *testing.T) { assert.Contains(t, strings.Join(scales[1], " "), "--nodes 0") } +func TestStopNodesPinsProfileResolvedRegion(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProviderWithRegion(t, map[string][]response{ + "get cluster": {{ + stdout: []byte(`[{"Name":"demo","Region":"eu-north-1","EksctlCreated":"True"}]`), + }}, + "get nodegroup": {{stdout: nodegroupJSON(3, 2)}}, + "scale nodegroup": {{}}, + }, "") + + require.NoError(t, prov.StopNodes(t.Context(), "demo")) + + snapshot, err := state.LoadEKSNodegroupState("demo") + require.NoError(t, err) + assert.Equal(t, "eu-north-1", snapshot.Region) + assert.Equal(t, [][]string{ + {"get", "cluster", "--name", "demo", "--output", "json"}, + {"get", "nodegroup", "--cluster", "demo", "--output", "json", "--region", "eu-north-1"}, + { + "scale", "nodegroup", "--cluster", "demo", "--name", "ng-1", "--nodes", "0", + "--nodes-min", "0", "--nodes-max", "5", "--region", "eu-north-1", + }, + }, runner.calls) +} + +func TestStartNodesPinsProfileResolvedRegion(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + require.NoError(t, state.SaveEKSNodegroupState("demo", &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: "demo", + Region: "eu-north-1", + Nodegroups: []state.EKSNodegroupCapacity{ + {Name: "ng-1", DesiredCapacity: 3, MinSize: 2, MaxSize: 5}, + }, + })) + + prov, runner := newProviderWithRegion(t, map[string][]response{ + "get cluster": {{ + stdout: []byte(`[{"Name":"demo","Region":"eu-north-1","EksctlCreated":"True"}]`), + }}, + "get nodegroup": { + {stdout: nodegroupJSON(0, 0)}, + {stdout: nodegroupJSON(3, 2)}, + }, + "scale nodegroup": {{}}, + }, "") + + require.NoError(t, prov.StartNodes(t.Context(), "demo")) + + for _, call := range runner.calls { + if call[0] == "get" && call[1] == "cluster" { + continue + } + + assert.Contains(t, call, "eu-north-1") + } + + _, err := state.LoadEKSNodegroupState("demo") + require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) +} + +func TestStopNodesRejectsProfileResolvedClusterWithoutExactIdentity(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + //nolint:paralleltest // The subtests share the process HOME set by their parent. + for name, summary := range map[string]string{ + "different name": `[{"Name":"other","Region":"eu-north-1","EksctlCreated":"True"}]`, + "missing region": `[{"Name":"demo","Region":"","EksctlCreated":"True"}]`, + } { + t.Run(name, func(t *testing.T) { + prov, runner := newProviderWithRegion(t, map[string][]response{ + "get cluster": {{stdout: []byte(summary)}}, + }, "") + + err := prov.StopNodes(t.Context(), "demo") + require.ErrorContains(t, err, "did not prove its exact name and region") + assert.Len(t, runner.calls, 1) + }) + } +} + +func TestProfileResolvedLifecycleRegionDoesNotMutateSharedProvider(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProviderWithRegion(t, map[string][]response{ + "get cluster": { + {stdout: []byte(`[{"Name":"first","Region":"eu-north-1","EksctlCreated":"True"}]`)}, + {stdout: []byte(`[{"Name":"second","Region":"us-west-2","EksctlCreated":"True"}]`)}, + }, + "get nodegroup": { + { + stdout: []byte( + `[{"Cluster":"first","Name":"workers","DesiredCapacity":2,"MinSize":1,"MaxSize":3}]`, + ), + }, + { + stdout: []byte( + `[{"Cluster":"second","Name":"workers","DesiredCapacity":2,"MinSize":1,"MaxSize":3}]`, + ), + }, + }, + "scale nodegroup": {{}, {}}, + }, "") + + require.NoError(t, prov.StopNodes(t.Context(), "first")) + require.NoError(t, prov.StopNodes(t.Context(), "second")) + assert.Empty(t, prov.Region()) + assert.Equal(t, [][]string{ + {"get", "cluster", "--name", "first", "--output", "json"}, + {"get", "nodegroup", "--cluster", "first", "--output", "json", "--region", "eu-north-1"}, + { + "scale", "nodegroup", "--cluster", "first", "--name", "workers", "--nodes", "0", + "--nodes-min", "0", "--nodes-max", "3", "--region", "eu-north-1", + }, + {"get", "cluster", "--name", "second", "--output", "json"}, + {"get", "nodegroup", "--cluster", "second", "--output", "json", "--region", "us-west-2"}, + { + "scale", "nodegroup", "--cluster", "second", "--name", "workers", "--nodes", "0", + "--nodes-min", "0", "--nodes-max", "3", "--region", "us-west-2", + }, + }, runner.calls) +} + func TestStopNodes_NoNodegroups(t *testing.T) { t.Parallel() @@ -325,6 +459,59 @@ func TestStopThenStartRestoresNodegroupCapacities(t *testing.T) { require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) } +func TestStopThenStartPreservesZeroCapacityNodegroup(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProvider(t, map[string][]response{ + "get nodegroup": { + {stdout: []byte(`[ + {"Cluster":"demo","Name":"a-idle","DesiredCapacity":0,"MinSize":0,"MaxSize":5}, + {"Cluster":"demo","Name":"b-active","DesiredCapacity":3,"MinSize":2,"MaxSize":5} + ]`)}, + {stdout: []byte(`[ + {"Cluster":"demo","Name":"a-idle","DesiredCapacity":0,"MinSize":0,"MaxSize":5}, + {"Cluster":"demo","Name":"b-active","DesiredCapacity":0,"MinSize":0,"MaxSize":5} + ]`)}, + {stdout: []byte(`[ + {"Cluster":"demo","Name":"a-idle","DesiredCapacity":0,"MinSize":0,"MaxSize":5}, + {"Cluster":"demo","Name":"b-active","DesiredCapacity":3,"MinSize":2,"MaxSize":5} + ]`)}, + }, + "scale nodegroup": {{}, {}}, + }) + + require.NoError(t, prov.StopNodes(t.Context(), "demo")) + require.NoError(t, prov.StartNodes(t.Context(), "demo")) + assert.Equal(t, []string{ + "scale nodegroup --cluster demo --name b-active --nodes 0 --nodes-min 0 --nodes-max 5 --region us-east-1", + "scale nodegroup --cluster demo --name b-active --nodes 3 --nodes-min 2 --nodes-max 5 --region us-east-1", + }, scaleCalls(runner.calls)) + + _, err := state.LoadEKSNodegroupState("demo") + require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) +} + +func TestStartNodesRejectsZeroMaximumCapacitySnapshot(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + require.NoError(t, state.SaveEKSNodegroupState("demo", &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: "demo", + Region: "us-east-1", + Nodegroups: []state.EKSNodegroupCapacity{ + {Name: "idle", DesiredCapacity: 0, MinSize: 0, MaxSize: 0}, + }, + })) + + prov, _ := newProvider(t, map[string][]response{ + "get nodegroup": {{stdout: []byte(`[ + {"Cluster":"demo","Name":"idle","DesiredCapacity":0,"MinSize":0,"MaxSize":0} + ]`)}}, + }) + + err := prov.StartNodes(t.Context(), "demo") + require.ErrorContains(t, err, "invalid saved nodegroup capacity") +} + func TestRepeatedStopPreservesOriginalNodegroupCapacities(t *testing.T) { t.Setenv("HOME", t.TempDir()) From c857211fa7bedd5c325cf2c8405381cedf1513a6 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sat, 18 Jul 2026 12:58:45 +0200 Subject: [PATCH 09/10] fix(eks): address review findings on standalone lifecycle routing Resolve four review findings from the Codex review at 56001ec. 1. Keep an upgraded cluster startable (P1). Releases before capacity snapshots stopped a cluster by scaling desired AND minimum to zero without recording anything, so every already-stopped cluster hit the new no-snapshot branch and could no longer start. Restore the historical max(MinSize, 1) fallback for that path: starting nodes is non-destructive, and the fail-closed rule is for mutations that can lose data. The unknown pre-stop size is warned about instead. 2. Scope capacity snapshots per region. A cluster name is unique only within a region, so the name-only path let same-named clusters in two regions share and clobber one snapshot. The file name now carries the resolved region, and the region is validated against path traversal. 3. Do not fail a completed deletion on local cleanup. When the EKS cluster is gone but ~/.ksail state cannot be removed, the job was pinned Failed, leaving an undismissable web UI row for a cluster that no longer exists. Report the cleanup failure as a warning and clear the job. 4. Scope strict config loading to mutating commands. Failing closed on an unreadable config in the shared resolver also blocked read-only callers (cluster info, diagnose, connect). ResolveClusterInfo is lenient again; delete and the simple lifecycle commands use the new ResolveClusterInfoStrict. Each fix is covered by a test; the region-scoping and cleanup-warning tests were confirmed to fail against the pre-fix code. --- pkg/cli/clusterapi/local_service.go | 7 +- pkg/cli/clusterapi/local_service_test.go | 84 +++++++++++++++---- pkg/cli/cmd/cluster/delete.go | 6 +- pkg/cli/cmd/cluster/eks_lifecycle_test.go | 2 +- pkg/cli/lifecycle/simple.go | 49 +++++++++-- pkg/cli/lifecycle/simple_additional_test.go | 36 ++++++++ pkg/svc/provider/aws/nodegroup_state.go | 40 +++++---- pkg/svc/provider/aws/provider.go | 6 +- pkg/svc/provider/aws/provider_test.go | 92 ++++++++++++++++----- pkg/svc/state/eks_nodegroup_state.go | 53 ++++++++---- pkg/svc/state/eks_nodegroup_state_test.go | 77 +++++++++++++++-- 11 files changed, 362 insertions(+), 90 deletions(-) diff --git a/pkg/cli/clusterapi/local_service.go b/pkg/cli/clusterapi/local_service.go index fb59d7aa17..478968285b 100644 --- a/pkg/cli/clusterapi/local_service.go +++ b/pkg/cli/clusterapi/local_service.go @@ -646,7 +646,12 @@ func (s *Service) runDelete(ctx context.Context, name string, spec v1alpha1.Spec (err == nil || errors.Is(err, clustererr.ErrClusterNotFound)) { cleanupErr := state.DeleteClusterState(name) if cleanupErr != nil { - err = fmt.Errorf("clean up local EKS cluster state after deletion: %w", cleanupErr) + // The cloud cluster is already gone, so the deletion itself succeeded. Turning a + // local-state cleanup failure (read-only home, permissions) into a job failure would + // pin an undismissable Failed row for a cluster that no longer exists — exactly the + // trap the idempotency note below describes. Warn, but still clear the job. + slog.Warn("failed to clean up local EKS cluster state after deletion", + "cluster", name, "error", cleanupErr) } } diff --git a/pkg/cli/clusterapi/local_service_test.go b/pkg/cli/clusterapi/local_service_test.go index a4feb57a07..886e0d2466 100644 --- a/pkg/cli/clusterapi/local_service_test.go +++ b/pkg/cli/clusterapi/local_service_test.go @@ -28,6 +28,9 @@ const ( // devClusterName is the discovered-cluster name shared by the List tests. devClusterName = "dev" + + // testEKSRegion is the region the EKS capacity-snapshot tests save and load under. + testEKSRegion = "eu-north-1" ) // Static sentinel errors used to drive provisioner failures in tests (err113 forbids inline @@ -448,19 +451,12 @@ func TestDeleteEKSClearsPersistedOwnershipAndCapacityState(t *testing.T) { Distribution: v1alpha1.DistributionEKS, Provider: v1alpha1.ProviderAWS, })) - require.NoError(t, state.SaveEKSNodegroupState(clusterName, &state.EKSNodegroupState{ - Version: state.EKSNodegroupStateVersion, - ClusterName: clusterName, - Region: "eu-north-1", - Nodegroups: []state.EKSNodegroupCapacity{ - {Name: "workers", DesiredCapacity: 2, MinSize: 1, MaxSize: 3}, - }, - })) + saveEKSCapacitySnapshot(t, clusterName) require.NoError(t, service.Delete(context.Background(), "default", clusterName)) require.Eventually(t, func() bool { _, specErr := state.LoadClusterSpec(clusterName) - _, capacityErr := state.LoadEKSNodegroupState(clusterName) + _, capacityErr := state.LoadEKSNodegroupState(clusterName, testEKSRegion) return errors.Is(specErr, state.ErrStateNotFound) && errors.Is(capacityErr, state.ErrEKSNodegroupStateNotFound) @@ -547,14 +543,7 @@ func TestDeleteEKSFailurePreservesPersistedOwnershipAndCapacityState(t *testing. return found && phase == v1alpha1.ClusterPhaseReady }, eventuallyTimeout, eventuallyTick) - require.NoError(t, state.SaveEKSNodegroupState(clusterName, &state.EKSNodegroupState{ - Version: state.EKSNodegroupStateVersion, - ClusterName: clusterName, - Region: "eu-north-1", - Nodegroups: []state.EKSNodegroupCapacity{ - {Name: "workers", DesiredCapacity: 2, MinSize: 1, MaxSize: 3}, - }, - })) + saveEKSCapacitySnapshot(t, clusterName) require.NoError(t, service.Delete(context.Background(), "default", clusterName)) require.Eventually(t, func() bool { @@ -569,10 +558,69 @@ func TestDeleteEKSFailurePreservesPersistedOwnershipAndCapacityState(t *testing. _, specErr := state.LoadClusterSpec(clusterName) require.NoError(t, specErr) - _, capacityErr := state.LoadEKSNodegroupState(clusterName) + _, capacityErr := state.LoadEKSNodegroupState(clusterName, testEKSRegion) require.NoError(t, capacityErr) } +// saveEKSCapacitySnapshot writes a stop-time capacity snapshot for the cluster in the region the +// EKS delete tests use, so a test can assert whether deletion cleaned it up. +func saveEKSCapacitySnapshot(t *testing.T, clusterName string) { + t.Helper() + + require.NoError(t, state.SaveEKSNodegroupState(clusterName, testEKSRegion, + &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: clusterName, + Region: testEKSRegion, + Nodegroups: []state.EKSNodegroupCapacity{ + {Name: "workers", DesiredCapacity: 2, MinSize: 1, MaxSize: 3}, + }, + })) +} + +// TestDeleteEKSClearsJobWhenOnlyLocalStateCleanupFails covers the split between the cloud +// operation and local bookkeeping: once the EKS cluster is actually gone, a failure to remove the +// local state directory (read-only home, permissions) must not pin the job Failed. That would leave +// an undismissable row in the web UI for a cluster that no longer exists — the very trap the +// idempotent-delete behaviour exists to avoid. The cleanup failure is a warning, not a job failure. +func TestDeleteEKSClearsJobWhenOnlyLocalStateCleanupFails(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "cleanup-fails" + + service := newTestService(map[v1alpha1.Distribution]*fakeProvisioner{ + v1alpha1.DistributionEKS: {}, + }) + _, err := service.Create( + context.Background(), + clusterFor(clusterName, v1alpha1.DistributionEKS), + ) + require.NoError(t, err) + require.Eventually(t, func() bool { + list, listErr := service.List(context.Background()) + require.NoError(t, listErr) + + phase, found := phaseOf(list, clusterName) + + return found && phase == v1alpha1.ClusterPhaseReady + }, eventuallyTimeout, eventuallyTick) + + // Force local state cleanup to fail deterministically: with no resolvable home directory the + // state layer cannot locate — and therefore cannot remove — the cluster's state directory. The + // provisioner still reports a clean deletion. + t.Setenv("HOME", "") + + require.NoError(t, service.Delete(context.Background(), "default", clusterName)) + require.Eventually(t, func() bool { + list, listErr := service.List(context.Background()) + require.NoError(t, listErr) + + _, found := phaseOf(list, clusterName) + + return !found + }, eventuallyTimeout, eventuallyTick) +} + // TestDeleteClearsFailedClusterWithNoUnderlyingCluster reproduces the bug where a cluster left in // the Failed phase by a failed create could never be removed from the web UI: deleting it called // the provisioner's Delete, which returned ErrClusterNotFound (there is no cluster to delete), and diff --git a/pkg/cli/cmd/cluster/delete.go b/pkg/cli/cmd/cluster/delete.go index 238827e033..44054b1d61 100644 --- a/pkg/cli/cmd/cluster/delete.go +++ b/pkg/cli/cmd/cluster/delete.go @@ -96,8 +96,10 @@ func runDeleteAction( tmr := timer.New() tmr.Start() - // Resolve cluster info from flags, config, or kubeconfig - resolved, err := lifecycle.ResolveClusterInfo(cmd, flags.Name, flags.Provider, flags.Kubeconfig) + // Strict: delete is destructive, so an unreadable config aborts before anything is removed. + resolved, err := lifecycle.ResolveClusterInfoStrict( + cmd, flags.Name, flags.Provider, flags.Kubeconfig, + ) if err != nil { return fmt.Errorf("failed to resolve cluster info: %w", err) } diff --git a/pkg/cli/cmd/cluster/eks_lifecycle_test.go b/pkg/cli/cmd/cluster/eks_lifecycle_test.go index 6f5888bdb7..88326adfa8 100644 --- a/pkg/cli/cmd/cluster/eks_lifecycle_test.go +++ b/pkg/cli/cmd/cluster/eks_lifecycle_test.go @@ -241,7 +241,7 @@ func TestStandaloneEKSStopStartRestoresExactCapacity(t *testing.T) { ), }, readStandaloneEKSCalls(t, markerPath)) - _, err := state.LoadEKSNodegroupState(clusterName) + _, err := state.LoadEKSNodegroupState(clusterName, "ap-southeast-2") require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) } diff --git a/pkg/cli/lifecycle/simple.go b/pkg/cli/lifecycle/simple.go index b41a739154..4b35412a77 100644 --- a/pkg/cli/lifecycle/simple.go +++ b/pkg/cli/lifecycle/simple.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "log/slog" "os" "strings" @@ -183,11 +184,40 @@ func resolveAWSRegion( // Priority for kubeconfig: flag > env (KUBECONFIG) > config > default (~/.kube/config). // // When cmd is non-nil, the --config persistent flag is honored for config loading. +// +// A config that is present but unreadable is tolerated: resolution falls back to flags and the +// kubeconfig context so read-only commands (cluster info, diagnose, connect) keep working while +// local project configuration is broken. Callers that are about to MUTATE a cluster must use +// ResolveClusterInfoStrict instead, so a malformed config can never silently drop credential +// aliases or region bindings ahead of a destructive operation. func ResolveClusterInfo( cmd *cobra.Command, nameFlag string, providerFlag v1alpha1.Provider, kubeconfigFlag string, +) (*ResolvedClusterInfo, error) { + return resolveClusterInfo(cmd, nameFlag, providerFlag, kubeconfigFlag, false) +} + +// ResolveClusterInfoStrict resolves cluster info like ResolveClusterInfo but fails closed when a +// present config cannot be loaded. Mutating lifecycle commands use it so a malformed ksail.yaml or +// distribution config aborts before any cluster is changed, rather than proceeding on partially +// resolved identity. +func ResolveClusterInfoStrict( + cmd *cobra.Command, + nameFlag string, + providerFlag v1alpha1.Provider, + kubeconfigFlag string, +) (*ResolvedClusterInfo, error) { + return resolveClusterInfo(cmd, nameFlag, providerFlag, kubeconfigFlag, true) +} + +func resolveClusterInfo( + cmd *cobra.Command, + nameFlag string, + providerFlag v1alpha1.Provider, + kubeconfigFlag string, + strict bool, ) (*ResolvedClusterInfo, error) { resolved := ResolvedClusterInfo{ ClusterName: nameFlag, @@ -195,12 +225,18 @@ func ResolveClusterInfo( KubeconfigPath: kubeconfigFlag, } - // Always load config to fill missing fields and extract provider options. - // Even when --name is provided, provider-specific settings are still needed. A present config - // that cannot be loaded must fail closed rather than silently dropping credential aliases. + // Always load config to fill missing fields and extract provider options: even when --name is + // provided, provider-specific settings are still needed. err := resolveFromConfig(cmd, &resolved) if err != nil { - return nil, err + if strict { + return nil, err + } + + slog.Warn( + "ignoring unreadable configuration; resolving from flags and kubeconfig instead", + "error", err, + ) } // Fall back to kubeconfig context detection @@ -444,9 +480,10 @@ func runSimpleLifecycleAction( stageWriter := notify.NewStageSeparatingWriter(cmd.OutOrStdout()) cmd.SetOut(stageWriter) - // Resolve cluster info from flags, config, or kubeconfig + // Resolve cluster info from flags, config, or kubeconfig. These commands mutate the cluster, so a + // present-but-unreadable config must abort before any change. // Empty kubeconfig flag - simple lifecycle commands don't need kubeconfig cleanup - resolved, err := ResolveClusterInfo(cmd, nameFlag, providerFlag, "") + resolved, err := ResolveClusterInfoStrict(cmd, nameFlag, providerFlag, "") if err != nil { return err } diff --git a/pkg/cli/lifecycle/simple_additional_test.go b/pkg/cli/lifecycle/simple_additional_test.go index b6b0ffb061..19aa5d7244 100644 --- a/pkg/cli/lifecycle/simple_additional_test.go +++ b/pkg/cli/lifecycle/simple_additional_test.go @@ -68,6 +68,42 @@ func TestResolveClusterInfo(t *testing.T) { }) } +// TestResolveClusterInfoToleratesUnreadableConfigForReadOnlyCallers pins the split between +// read-only and mutating resolution. A broken ksail.yaml must not lock the user out of read-only +// commands (cluster info, diagnose, connect) when they have supplied an explicit --name, but it must +// still abort a mutating command before anything is changed. +// +//nolint:paralleltest // changes the process working directory +func TestResolveClusterInfoToleratesUnreadableConfigForReadOnlyCallers(t *testing.T) { + workingDir := t.TempDir() + t.Chdir(workingDir) + + require.NoError( + t, + os.WriteFile("ksail.yaml", []byte("this: is: not: valid: yaml:\n\t- broken\n"), 0o600), + ) + + // Read-only path: resolution falls back to the explicit flags. + resolved, err := lifecycle.ResolveClusterInfo( + nil, + "explicit-name", + v1alpha1.ProviderDocker, + "/non-existent-kubeconfig", + ) + require.NoError(t, err) + require.NotNil(t, resolved) + assert.Equal(t, "explicit-name", resolved.ClusterName) + + // Mutating path: the same broken config fails closed. + _, strictErr := lifecycle.ResolveClusterInfoStrict( + nil, + "explicit-name", + v1alpha1.ProviderDocker, + "/non-existent-kubeconfig", + ) + require.Error(t, strictErr) +} + // TestResolveClusterInfoFallsBackToDistributionName verifies an intentionally empty metadata.name // does not suppress the name carried by the distribution config. // diff --git a/pkg/svc/provider/aws/nodegroup_state.go b/pkg/svc/provider/aws/nodegroup_state.go index 8e467e6d4a..df4dff96e6 100644 --- a/pkg/svc/provider/aws/nodegroup_state.go +++ b/pkg/svc/provider/aws/nodegroup_state.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "log/slog" "sort" "strings" @@ -15,7 +16,6 @@ var ( errNodegroupStateVersion = errors.New("unsupported EKS nodegroup state version") errNodegroupStateTarget = errors.New("EKS nodegroup state target mismatch") errNodegroupStateDrift = errors.New("EKS nodegroup capacity state drift") - errNodegroupStateMissing = errors.New("EKS nodegroup capacity state is required") ) func newNodegroupState( @@ -250,9 +250,9 @@ func nodegroupIsStopped( } func (p *Provider) loadNodegroupState( - clusterName string, + clusterName, region string, ) (*state.EKSNodegroupState, bool, error) { - snapshot, err := state.LoadEKSNodegroupState(clusterName) + snapshot, err := state.LoadEKSNodegroupState(clusterName, region) if errors.Is(err, state.ErrEKSNodegroupStateNotFound) { return nil, false, nil } @@ -264,9 +264,13 @@ func (p *Provider) loadNodegroupState( return snapshot, true, nil } -// startNodegroupsWithoutSnapshot preserves the pre-existing lifecycle for groups stopped outside -// KSail while failing closed when both desired and minimum are zero. A KSail stop always records the -// exact desired size first, so its own round trip never reaches this compatibility path. +// startNodegroupsWithoutSnapshot preserves the pre-existing lifecycle for groups stopped without a +// capacity snapshot. A KSail stop from this release records the exact desired size first, so its own +// round trip never reaches this compatibility path — but a cluster stopped by an earlier release (or +// scaled to zero outside KSail) has no snapshot, and earlier releases scaled both desired and minimum +// to zero. Those clusters must stay startable after an upgrade, so this path keeps the historical +// max(MinSize, 1) fallback rather than failing closed. Starting nodes is non-destructive and +// reversible; the exact pre-stop size is simply unknown, which the caller is warned about. func (p *Provider) startNodegroupsWithoutSnapshot( ctx context.Context, clusterName string, @@ -289,14 +293,6 @@ func (p *Provider) startNodegroupsWithoutSnapshot( } seen[nodegroup.Name] = struct{}{} - - if nodegroup.DesiredCap == 0 && nodegroup.MinSize <= 0 { - return fmt.Errorf( - "cannot restore stopped EKS nodegroup %q without the pre-stop snapshot: %w", - nodegroup.Name, - errNodegroupStateMissing, - ) - } } sort.Slice(nodegroups, func(i, j int) bool { return nodegroups[i].Name < nodegroups[j].Name }) @@ -306,12 +302,24 @@ func (p *Provider) startNodegroupsWithoutSnapshot( continue } + // Without a snapshot the pre-stop desired size is unknowable, so fall back to the smallest + // running capacity that honours the group's own minimum — the behaviour every release before + // capacity snapshots used. + target := max(nodegroup.MinSize, 1) + + slog.Warn( + "restoring EKS nodegroup without a capacity snapshot; pre-stop desired size is unknown", + "cluster", clusterName, + "nodegroup", nodegroup.Name, + "target", target, + ) + err := p.client.ScaleNodegroup( ctx, clusterName, nodegroup.Name, p.region, - nodegroup.MinSize, + target, nodegroup.MinSize, nodegroup.MaxSize, ) @@ -366,7 +374,7 @@ func (p *Provider) verifyAndClearNodegroupState( return err } - err = state.DeleteEKSNodegroupState(clusterName) + err = state.DeleteEKSNodegroupState(clusterName, p.region) if err != nil { return fmt.Errorf("clear restored EKS nodegroup state: %w", err) } diff --git a/pkg/svc/provider/aws/provider.go b/pkg/svc/provider/aws/provider.go index 7b4eb7d1a9..4abb939488 100644 --- a/pkg/svc/provider/aws/provider.go +++ b/pkg/svc/provider/aws/provider.go @@ -130,7 +130,7 @@ func (p *Provider) StopNodes(ctx context.Context, clusterName string) error { return err } - err = state.SaveEKSNodegroupState(clusterName, snapshot) + err = state.SaveEKSNodegroupState(clusterName, target.region, snapshot) if err != nil { return fmt.Errorf("save EKS nodegroup state before stop: %w", err) } @@ -415,7 +415,9 @@ func (p *Provider) loadNodegroupsAndState( return nil, nil, false, err } - snapshot, found, err := p.loadNodegroupState(clusterName) + // p is the request-local target pinned by withResolvedLifecycleRegion, so p.region is the exact + // region the snapshot was keyed by at stop time. + snapshot, found, err := p.loadNodegroupState(clusterName, p.region) if err != nil { return nil, nil, false, err } diff --git a/pkg/svc/provider/aws/provider_test.go b/pkg/svc/provider/aws/provider_test.go index 7e5d2ace1d..b4ae14a0a8 100644 --- a/pkg/svc/provider/aws/provider_test.go +++ b/pkg/svc/provider/aws/provider_test.go @@ -253,7 +253,7 @@ func TestStopNodesPinsProfileResolvedRegion(t *testing.T) { require.NoError(t, prov.StopNodes(t.Context(), "demo")) - snapshot, err := state.LoadEKSNodegroupState("demo") + snapshot, err := state.LoadEKSNodegroupState("demo", "eu-north-1") require.NoError(t, err) assert.Equal(t, "eu-north-1", snapshot.Region) assert.Equal(t, [][]string{ @@ -268,7 +268,7 @@ func TestStopNodesPinsProfileResolvedRegion(t *testing.T) { func TestStartNodesPinsProfileResolvedRegion(t *testing.T) { t.Setenv("HOME", t.TempDir()) - require.NoError(t, state.SaveEKSNodegroupState("demo", &state.EKSNodegroupState{ + require.NoError(t, state.SaveEKSNodegroupState("demo", "eu-north-1", &state.EKSNodegroupState{ Version: state.EKSNodegroupStateVersion, ClusterName: "demo", Region: "eu-north-1", @@ -298,7 +298,7 @@ func TestStartNodesPinsProfileResolvedRegion(t *testing.T) { assert.Contains(t, call, "eu-north-1") } - _, err := state.LoadEKSNodegroupState("demo") + _, err := state.LoadEKSNodegroupState("demo", "eu-north-1") require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) } @@ -377,7 +377,7 @@ func TestStopNodes_NoNodegroups(t *testing.T) { func TestStartNodes_ScalesOnlyZeroDesired(t *testing.T) { t.Setenv("HOME", t.TempDir()) - require.NoError(t, state.SaveEKSNodegroupState("demo", &state.EKSNodegroupState{ + require.NoError(t, state.SaveEKSNodegroupState("demo", "us-east-1", &state.EKSNodegroupState{ Version: state.EKSNodegroupStateVersion, ClusterName: "demo", Region: "us-east-1", @@ -455,7 +455,7 @@ func TestStopThenStartRestoresNodegroupCapacities(t *testing.T) { "scale nodegroup --cluster demo --name ng-1 --nodes 3 --nodes-min 2 --nodes-max 5 --region us-east-1", }, scales) - _, err := state.LoadEKSNodegroupState("demo") + _, err := state.LoadEKSNodegroupState("demo", "us-east-1") require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) } @@ -487,13 +487,13 @@ func TestStopThenStartPreservesZeroCapacityNodegroup(t *testing.T) { "scale nodegroup --cluster demo --name b-active --nodes 3 --nodes-min 2 --nodes-max 5 --region us-east-1", }, scaleCalls(runner.calls)) - _, err := state.LoadEKSNodegroupState("demo") + _, err := state.LoadEKSNodegroupState("demo", "us-east-1") require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) } func TestStartNodesRejectsZeroMaximumCapacitySnapshot(t *testing.T) { t.Setenv("HOME", t.TempDir()) - require.NoError(t, state.SaveEKSNodegroupState("demo", &state.EKSNodegroupState{ + require.NoError(t, state.SaveEKSNodegroupState("demo", "us-east-1", &state.EKSNodegroupState{ Version: state.EKSNodegroupStateVersion, ClusterName: "demo", Region: "us-east-1", @@ -546,13 +546,17 @@ func TestStopNodesRejectsInvalidCapacityBeforePersisting(t *testing.T) { require.Error(t, err) assert.Empty(t, scaleCalls(runner.calls)) - _, loadErr := state.LoadEKSNodegroupState("demo") + _, loadErr := state.LoadEKSNodegroupState("demo", "us-east-1") require.ErrorIs(t, loadErr, state.ErrEKSNodegroupStateNotFound) } +// TestStartNodesRejectsMismatchedCapacityStateBeforeScaling exercises the snapshot's own region +// identity check as defence in depth. The path is now region-scoped, so a wrong-region snapshot can +// no longer be reached by accident — this covers a corrupted or hand-edited file whose recorded +// region contradicts the target it was loaded for. func TestStartNodesRejectsMismatchedCapacityStateBeforeScaling(t *testing.T) { t.Setenv("HOME", t.TempDir()) - saveNodegroupState(t, "demo", "eu-north-1", 3, 2, 5) + saveNodegroupStateAt(t, "demo", "us-east-1", "eu-north-1", 3, 2, 5) prov, runner := newProvider(t, map[string][]response{ "get nodegroup": {{stdout: nodegroupJSON(0, 0)}}, @@ -576,38 +580,70 @@ func TestStartNodesRetainsCapacityStateWhenScalingFails(t *testing.T) { err := prov.StartNodes(t.Context(), "demo") require.ErrorIs(t, err, errScaleDenied) - saved, loadErr := state.LoadEKSNodegroupState("demo") + saved, loadErr := state.LoadEKSNodegroupState("demo", "us-east-1") require.NoError(t, loadErr) assert.Equal(t, 3, saved.Nodegroups[0].DesiredCapacity) assert.Equal(t, 2, saved.Nodegroups[0].MinSize) } -func TestStartNodesRequiresSnapshotWhenMinimumIsZero(t *testing.T) { +// TestStartNodesWithoutSnapshotFallsBackToMinimumOne pins upgrade compatibility: releases before +// capacity snapshots stopped a cluster by scaling desired AND minimum to zero without recording +// anything. Such a cluster must still start after an upgrade, using the historical max(MinSize, 1) +// fallback rather than failing closed and stranding the user outside KSail. +func TestStartNodesWithoutSnapshotFallsBackToMinimumOne(t *testing.T) { t.Setenv("HOME", t.TempDir()) prov, runner := newProvider(t, map[string][]response{ - "get nodegroup": {{stdout: nodegroupJSON(0, 0)}}, + "get nodegroup": {{stdout: nodegroupJSON(0, 0)}}, + "scale nodegroup": {{}}, }) err := prov.StartNodes(t.Context(), "demo") - require.Error(t, err) - assert.Contains(t, err.Error(), "pre-stop snapshot") - assert.Empty(t, scaleCalls(runner.calls)) + require.NoError(t, err) + assert.Equal(t, []string{ + "scale nodegroup --cluster demo --name ng-1 --nodes 1 --nodes-min 0 --nodes-max 5 " + + "--region us-east-1", + }, scaleCalls(runner.calls)) } +// TestStartNodesWithoutSnapshotHonoursEachGroupMinimum covers the mixed case: a group with its own +// non-zero minimum returns to that minimum, while a fully zeroed legacy group returns to one node. +func TestStartNodesWithoutSnapshotHonoursEachGroupMinimum(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProvider(t, map[string][]response{ + "get nodegroup": {{stdout: []byte(`[ + {"Cluster":"demo","Name":"a-recoverable","DesiredCapacity":0,"MinSize":2,"MaxSize":5}, + {"Cluster":"demo","Name":"z-legacy-zeroed","DesiredCapacity":0,"MinSize":0,"MaxSize":5} + ]`)}}, + "scale nodegroup": {{}, {}}, + }) + + err := prov.StartNodes(t.Context(), "demo") + require.NoError(t, err) + assert.Equal(t, []string{ + "scale nodegroup --cluster demo --name a-recoverable --nodes 2 --nodes-min 2 " + + "--nodes-max 5 --region us-east-1", + "scale nodegroup --cluster demo --name z-legacy-zeroed --nodes 1 --nodes-min 0 " + + "--nodes-max 5 --region us-east-1", + }, scaleCalls(runner.calls)) +} + +// TestStartNodesWithoutSnapshotPreflightsEveryNodegroup keeps the preflight guarantee: the whole +// live set is validated before any group is scaled, so a drifted listing mutates nothing. func TestStartNodesWithoutSnapshotPreflightsEveryNodegroup(t *testing.T) { t.Setenv("HOME", t.TempDir()) prov, runner := newProvider(t, map[string][]response{ "get nodegroup": {{stdout: []byte(`[ {"Cluster":"demo","Name":"a-recoverable","DesiredCapacity":0,"MinSize":2,"MaxSize":5}, - {"Cluster":"demo","Name":"z-missing-state","DesiredCapacity":0,"MinSize":0,"MaxSize":5} + {"Cluster":"demo","Name":"a-recoverable","DesiredCapacity":0,"MinSize":0,"MaxSize":5} ]`)}}, }) err := prov.StartNodes(t.Context(), "demo") require.Error(t, err) - assert.Contains(t, err.Error(), "z-missing-state") + assert.Contains(t, err.Error(), "duplicate live nodegroup") assert.Empty(t, scaleCalls(runner.calls)) } @@ -627,7 +663,7 @@ func TestStartNodesRetainsCapacityStateUntilReadbackMatches(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "not restored") - _, loadErr := state.LoadEKSNodegroupState("demo") + _, loadErr := state.LoadEKSNodegroupState("demo", "us-east-1") require.NoError(t, loadErr) } @@ -647,10 +683,22 @@ func saveNodegroupState( ) { t.Helper() - require.NoError(t, state.SaveEKSNodegroupState(clusterName, &state.EKSNodegroupState{ + saveNodegroupStateAt(t, clusterName, region, region, desiredCapacity, minSize, maxSize) +} + +// saveNodegroupStateAt writes a snapshot under pathRegion while recording bodyRegion inside it, so +// tests can construct the contradiction the in-file region identity check exists to catch. +func saveNodegroupStateAt( + t *testing.T, + clusterName, pathRegion, bodyRegion string, + desiredCapacity, minSize, maxSize int, +) { + t.Helper() + + snapshot := &state.EKSNodegroupState{ Version: state.EKSNodegroupStateVersion, ClusterName: clusterName, - Region: region, + Region: bodyRegion, Nodegroups: []state.EKSNodegroupCapacity{ { Name: "ng-1", @@ -659,7 +707,9 @@ func saveNodegroupState( MaxSize: maxSize, }, }, - })) + } + + require.NoError(t, state.SaveEKSNodegroupState(clusterName, pathRegion, snapshot)) } func scaleCalls(calls [][]string) []string { diff --git a/pkg/svc/state/eks_nodegroup_state.go b/pkg/svc/state/eks_nodegroup_state.go index b341d73fe8..0df9dfe4e0 100644 --- a/pkg/svc/state/eks_nodegroup_state.go +++ b/pkg/svc/state/eks_nodegroup_state.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "github.com/devantler-tech/ksail/v7/pkg/fsutil" ) @@ -13,15 +14,20 @@ import ( const ( // EKSNodegroupStateVersion is the on-disk schema version for EKS capacity snapshots. EKSNodegroupStateVersion = 1 - // eksNodegroupStateFileName is kept separate from spec and TTL state so a successful start can - // clear only the transient capacity snapshot. - eksNodegroupStateFileName = "eks-nodegroups.json" + // eksNodegroupStateFileNameFormat is kept separate from spec and TTL state so a successful start + // can clear only the transient capacity snapshot. The region is part of the file name because a + // cluster name is unique only within a region. + eksNodegroupStateFileNameFormat = "eks-nodegroups-%s.json" ) var ( // ErrEKSNodegroupStateNotFound reports that no stop-time EKS capacity snapshot exists. ErrEKSNodegroupStateNotFound = errors.New("EKS nodegroup state not found") - errEKSNodegroupStateNil = errors.New("EKS nodegroup state is nil") + // ErrInvalidRegion reports a missing region or one containing path separators or '..'. + ErrInvalidRegion = errors.New( + "invalid AWS region: must be non-empty and must not contain path separators or '..'", + ) + errEKSNodegroupStateNil = errors.New("EKS nodegroup state is nil") ) // EKSNodegroupCapacity records the exact scaling values a stopped managed nodegroup must regain. @@ -41,16 +47,16 @@ type EKSNodegroupState struct { } // SaveEKSNodegroupState atomically persists the capacity snapshot before stop mutates AWS. -func SaveEKSNodegroupState(clusterName string, snapshot *EKSNodegroupState) error { - statePath, err := eksNodegroupStatePath(clusterName) - if err != nil { - return err - } - +func SaveEKSNodegroupState(clusterName, region string, snapshot *EKSNodegroupState) error { if snapshot == nil { return errEKSNodegroupStateNil } + statePath, err := eksNodegroupStatePath(clusterName, region) + if err != nil { + return err + } + err = os.MkdirAll(filepath.Dir(statePath), dirPermissions) if err != nil { return fmt.Errorf("failed to create EKS nodegroup state directory: %w", err) @@ -70,8 +76,8 @@ func SaveEKSNodegroupState(clusterName string, snapshot *EKSNodegroupState) erro } // LoadEKSNodegroupState reads the stop-time EKS capacity snapshot. -func LoadEKSNodegroupState(clusterName string) (*EKSNodegroupState, error) { - statePath, err := eksNodegroupStatePath(clusterName) +func LoadEKSNodegroupState(clusterName, region string) (*EKSNodegroupState, error) { + statePath, err := eksNodegroupStatePath(clusterName, region) if err != nil { return nil, err } @@ -97,8 +103,8 @@ func LoadEKSNodegroupState(clusterName string) (*EKSNodegroupState, error) { } // DeleteEKSNodegroupState removes only the transient EKS capacity snapshot. -func DeleteEKSNodegroupState(clusterName string) error { - statePath, err := eksNodegroupStatePath(clusterName) +func DeleteEKSNodegroupState(clusterName, region string) error { + statePath, err := eksNodegroupStatePath(clusterName, region) if err != nil { return err } @@ -111,11 +117,26 @@ func DeleteEKSNodegroupState(clusterName string) error { return nil } -func eksNodegroupStatePath(clusterName string) (string, error) { +// eksNodegroupStatePath scopes the snapshot to one cluster *and* one region. Cluster names are only +// unique within a region, so keying the file by name alone lets same-named clusters in two regions +// share — and clobber — a single snapshot: a stop in one region would be restored from the other's +// capacities, or rejected outright by the snapshot's own region identity check. +func eksNodegroupStatePath(clusterName, region string) (string, error) { dir, err := clusterStateDir(clusterName) if err != nil { return "", err } - return filepath.Join(dir, eksNodegroupStateFileName), nil + region = strings.TrimSpace(region) + if region == "" { + return "", ErrInvalidRegion + } + + if strings.Contains(region, "/") || + strings.Contains(region, "\\") || + strings.Contains(region, "..") { + return "", ErrInvalidRegion + } + + return filepath.Join(dir, fmt.Sprintf(eksNodegroupStateFileNameFormat, region)), nil } diff --git a/pkg/svc/state/eks_nodegroup_state_test.go b/pkg/svc/state/eks_nodegroup_state_test.go index b9dd4877c2..a74117bf22 100644 --- a/pkg/svc/state/eks_nodegroup_state_test.go +++ b/pkg/svc/state/eks_nodegroup_state_test.go @@ -25,13 +25,15 @@ func TestSaveLoadAndDeleteEKSNodegroupState(t *testing.T) { }, } - require.NoError(t, state.SaveEKSNodegroupState(clusterName, want)) + require.NoError(t, state.SaveEKSNodegroupState(clusterName, "eu-north-1", want)) - got, err := state.LoadEKSNodegroupState(clusterName) + got, err := state.LoadEKSNodegroupState(clusterName, "eu-north-1") require.NoError(t, err) assert.Equal(t, want, got) - statePath := filepath.Join(home, ".ksail", "clusters", clusterName, "eks-nodegroups.json") + statePath := filepath.Join( + home, ".ksail", "clusters", clusterName, "eks-nodegroups-eu-north-1.json", + ) info, err := os.Stat(statePath) require.NoError(t, err) assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) @@ -39,13 +41,70 @@ func TestSaveLoadAndDeleteEKSNodegroupState(t *testing.T) { neighborPath := filepath.Join(filepath.Dir(statePath), "spec.json") require.NoError(t, os.WriteFile(neighborPath, []byte("neighbor"), 0o600)) - require.NoError(t, state.DeleteEKSNodegroupState(clusterName)) + require.NoError(t, state.DeleteEKSNodegroupState(clusterName, "eu-north-1")) assert.FileExists(t, neighborPath) - _, err = state.LoadEKSNodegroupState(clusterName) + _, err = state.LoadEKSNodegroupState(clusterName, "eu-north-1") require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) } +// TestEKSNodegroupStateIsScopedPerRegion pins the cross-region isolation the snapshot path provides: +// two same-named clusters in different regions must keep independent capacity snapshots, and +// removing one must leave the other intact. +func TestEKSNodegroupStateIsScopedPerRegion(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "same-name-two-regions" + + north := &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: clusterName, + Region: "eu-north-1", + Nodegroups: []state.EKSNodegroupCapacity{ + {Name: "workers", DesiredCapacity: 3, MinSize: 2, MaxSize: 5}, + }, + } + east := &state.EKSNodegroupState{ + Version: state.EKSNodegroupStateVersion, + ClusterName: clusterName, + Region: "us-east-1", + Nodegroups: []state.EKSNodegroupCapacity{ + {Name: "workers", DesiredCapacity: 7, MinSize: 4, MaxSize: 9}, + }, + } + + require.NoError(t, state.SaveEKSNodegroupState(clusterName, "eu-north-1", north)) + require.NoError(t, state.SaveEKSNodegroupState(clusterName, "us-east-1", east)) + + // Neither region may read the other's capacities. + gotNorth, err := state.LoadEKSNodegroupState(clusterName, "eu-north-1") + require.NoError(t, err) + assert.Equal(t, north, gotNorth) + + gotEast, err := state.LoadEKSNodegroupState(clusterName, "us-east-1") + require.NoError(t, err) + assert.Equal(t, east, gotEast) + + // Clearing one region's snapshot must not discard the other's restore data. + require.NoError(t, state.DeleteEKSNodegroupState(clusterName, "eu-north-1")) + + _, err = state.LoadEKSNodegroupState(clusterName, "eu-north-1") + require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) + + survivor, err := state.LoadEKSNodegroupState(clusterName, "us-east-1") + require.NoError(t, err) + assert.Equal(t, east, survivor) +} + +func TestEKSNodegroupStateRejectsUnusableRegion(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + for _, region := range []string{"", " ", "../escape", "eu/north", `eu\north`} { + _, err := state.LoadEKSNodegroupState("demo", region) + require.ErrorIs(t, err, state.ErrInvalidRegion, "region %q must be rejected", region) + } +} + func TestLoadEKSNodegroupStateRejectsInvalidJSON(t *testing.T) { home := t.TempDir() t.Setenv("HOME", home) @@ -54,10 +113,14 @@ func TestLoadEKSNodegroupStateRejectsInvalidJSON(t *testing.T) { require.NoError(t, os.MkdirAll(stateDir, 0o700)) require.NoError( t, - os.WriteFile(filepath.Join(stateDir, "eks-nodegroups.json"), []byte("{"), 0o600), + os.WriteFile( + filepath.Join(stateDir, "eks-nodegroups-eu-north-1.json"), + []byte("{"), + 0o600, + ), ) - _, err := state.LoadEKSNodegroupState("invalid-json") + _, err := state.LoadEKSNodegroupState("invalid-json", "eu-north-1") require.Error(t, err) assert.Contains(t, err.Error(), "unmarshal EKS nodegroup state") } From f5308a98c0ab229deb8a1c533b842d2928aa0eb4 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Sat, 18 Jul 2026 14:16:58 +0200 Subject: [PATCH 10/10] fix(eks): never persist an all-stopped capacity snapshot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An already-zero cluster — stopped by an older release, or scaled down by hand — recorded its all-zero live state as the capacity to restore. The next start then found live already matching the snapshot, skipped every scale, cleared the snapshot, and reported success with every nodegroup still at zero. It also permanently shadowed the no-snapshot fallback, which is the path that recovers exactly these clusters. Withhold the snapshot when every nodegroup is already stopped, so stop stays a no-op and the next start takes the max(MinSize, 1) fallback. A partially running cluster still snapshots normally, so recoverable capacity is never dropped. Found in review. Covered by a stop-then-start round trip that fails against the previous behaviour, plus a mixed-cluster negative control. --- pkg/svc/provider/aws/nodegroup_state.go | 17 +++++++ pkg/svc/provider/aws/provider.go | 13 ++++-- pkg/svc/provider/aws/provider_test.go | 62 +++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 3 deletions(-) diff --git a/pkg/svc/provider/aws/nodegroup_state.go b/pkg/svc/provider/aws/nodegroup_state.go index df4dff96e6..59d9a0f381 100644 --- a/pkg/svc/provider/aws/nodegroup_state.go +++ b/pkg/svc/provider/aws/nodegroup_state.go @@ -232,6 +232,23 @@ func verifyNodegroupsRestored( return nil } +// snapshotIsAllStopped reports whether every nodegroup in the snapshot is already fully scaled down. +// Such a snapshot carries no recoverable capacity — restoring to it is a no-op — so it must never be +// persisted as the target state for a later start. +func snapshotIsAllStopped(snapshot *state.EKSNodegroupState) bool { + if snapshot == nil || len(snapshot.Nodegroups) == 0 { + return false + } + + for _, capacity := range snapshot.Nodegroups { + if capacity.DesiredCapacity > 0 || capacity.MinSize > 0 { + return false + } + } + + return true +} + func nodegroupMatchesCapacity( nodegroup eksctlclient.NodegroupSummary, capacity state.EKSNodegroupCapacity, diff --git a/pkg/svc/provider/aws/provider.go b/pkg/svc/provider/aws/provider.go index 4abb939488..51417a9ce3 100644 --- a/pkg/svc/provider/aws/provider.go +++ b/pkg/svc/provider/aws/provider.go @@ -130,9 +130,16 @@ func (p *Provider) StopNodes(ctx context.Context, clusterName string) error { return err } - err = state.SaveEKSNodegroupState(clusterName, target.region, snapshot) - if err != nil { - return fmt.Errorf("save EKS nodegroup state before stop: %w", err) + // Never persist an all-stopped snapshot. A cluster already at zero (stopped by an older + // release, or scaled down manually) would otherwise record 0/0 as its "restore to" state: + // the next start finds live already matching the snapshot, skips every scale, clears the + // file, and reports success while leaving the cluster at zero. Withholding the snapshot + // keeps the no-snapshot fallback — which restores max(MinSize, 1) — reachable instead. + if !snapshotIsAllStopped(snapshot) { + err = state.SaveEKSNodegroupState(clusterName, target.region, snapshot) + if err != nil { + return fmt.Errorf("save EKS nodegroup state before stop: %w", err) + } } } diff --git a/pkg/svc/provider/aws/provider_test.go b/pkg/svc/provider/aws/provider_test.go index b4ae14a0a8..cb0d01b4c9 100644 --- a/pkg/svc/provider/aws/provider_test.go +++ b/pkg/svc/provider/aws/provider_test.go @@ -586,6 +586,68 @@ func TestStartNodesRetainsCapacityStateWhenScalingFails(t *testing.T) { assert.Equal(t, 2, saved.Nodegroups[0].MinSize) } +// TestStopNodesDoesNotPersistAnAllStoppedSnapshot guards the stop→start round trip for a cluster +// that is ALREADY at zero (stopped by an older release, or scaled down by hand). Recording its +// all-zero live state as the restore target would make the next start a silent no-op: live already +// matches the snapshot, every scale is skipped, the snapshot is cleared, and success is reported +// with every nodegroup still at zero — while permanently shadowing the no-snapshot fallback. +func TestStopNodesDoesNotPersistAnAllStoppedSnapshot(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProvider(t, map[string][]response{ + "get nodegroup": {{stdout: nodegroupJSON(0, 0)}}, + }) + + require.NoError(t, prov.StopNodes(t.Context(), "demo")) + + // Nothing to scale (already stopped) and, critically, no snapshot written. + assert.Empty(t, scaleCalls(runner.calls)) + + _, err := state.LoadEKSNodegroupState("demo", "us-east-1") + require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound, + "an all-stopped snapshot must not be persisted") +} + +// TestStopThenStartOnAlreadyStoppedClusterStillRestores is the end-to-end consequence: after a stop +// on an already-zero cluster, start must actually bring nodes back rather than report a no-op +// success. +func TestStopThenStartOnAlreadyStoppedClusterStillRestores(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, runner := newProvider(t, map[string][]response{ + "get nodegroup": {{stdout: nodegroupJSON(0, 0)}, {stdout: nodegroupJSON(0, 0)}}, + "scale nodegroup": {{}}, + }) + + require.NoError(t, prov.StopNodes(t.Context(), "demo")) + require.NoError(t, prov.StartNodes(t.Context(), "demo")) + + assert.Equal(t, []string{ + "scale nodegroup --cluster demo --name ng-1 --nodes 1 --nodes-min 0 --nodes-max 5 " + + "--region us-east-1", + }, scaleCalls(runner.calls), "start must restore capacity, not silently no-op") +} + +// TestStopNodesStillSnapshotsPartiallyRunningNodegroups is the negative half: a snapshot is only +// withheld when EVERY group is stopped, so a mixed cluster keeps its recoverable capacity. +func TestStopNodesStillSnapshotsPartiallyRunningNodegroups(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + prov, _ := newProvider(t, map[string][]response{ + "get nodegroup": {{stdout: []byte(`[ + {"Cluster":"demo","Name":"a-stopped","DesiredCapacity":0,"MinSize":0,"MaxSize":5}, + {"Cluster":"demo","Name":"b-running","DesiredCapacity":3,"MinSize":2,"MaxSize":5} + ]`)}}, + "scale nodegroup": {{}}, + }) + + require.NoError(t, prov.StopNodes(t.Context(), "demo")) + + saved, err := state.LoadEKSNodegroupState("demo", "us-east-1") + require.NoError(t, err, "a partially running cluster must keep its snapshot") + assert.Len(t, saved.Nodegroups, 2) +} + // TestStartNodesWithoutSnapshotFallsBackToMinimumOne pins upgrade compatibility: releases before // capacity snapshots stopped a cluster by scaling desired AND minimum to zero without recording // anything. Such a cluster must still start after an upgrade, using the historical max(MinSize, 1)