From 4aa753159a5c850c23120693aedf532d39b4132c Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 19:24:57 +0100 Subject: [PATCH 1/6] fix(eks): keep a failed create's attempt identifier for a late cluster A create interrupted before AWS reports the cluster now keeps the attempt's creation identifier as a local, untrusted hint keyed to the account, name and region. A later lifecycle command records ownership only when the live cluster carries exactly that creation tag; any other same-named cluster is still refused. The hint is removed once used, and when eksctl or the EKS API confirms the cluster does not exist. Part of #8324 Co-Authored-By: Claude Opus 5.5 --- pkg/cli/cmd/cluster/eks_creation_hint.go | 150 +++++++++++++++++ pkg/cli/cmd/cluster/eks_creation_hint_test.go | 151 +++++++++++++++++ pkg/cli/cmd/cluster/eks_failed_create.go | 83 +++++++-- pkg/cli/cmd/cluster/unmanaged_guard.go | 26 ++- pkg/svc/eksidentity/identity.go | 7 + pkg/svc/eksidentity/identity_test.go | 35 ++++ pkg/svc/state/eks_component_state.go | 18 +- pkg/svc/state/eks_creation_hint_state.go | 159 ++++++++++++++++++ pkg/svc/state/eks_creation_hint_state_test.go | 122 ++++++++++++++ 9 files changed, 732 insertions(+), 19 deletions(-) create mode 100644 pkg/cli/cmd/cluster/eks_creation_hint.go create mode 100644 pkg/cli/cmd/cluster/eks_creation_hint_test.go create mode 100644 pkg/svc/state/eks_creation_hint_state.go create mode 100644 pkg/svc/state/eks_creation_hint_state_test.go diff --git a/pkg/cli/cmd/cluster/eks_creation_hint.go b/pkg/cli/cmd/cluster/eks_creation_hint.go new file mode 100644 index 0000000000..53eb916dca --- /dev/null +++ b/pkg/cli/cmd/cluster/eks_creation_hint.go @@ -0,0 +1,150 @@ +package cluster + +import ( + "context" + "errors" + "fmt" + "strings" + + "github.com/devantler-tech/ksail/v7/pkg/cli/lifecycle" + eksclient "github.com/devantler-tech/ksail/v7/pkg/client/eks" + "github.com/devantler-tech/ksail/v7/pkg/svc/credentials" + "github.com/devantler-tech/ksail/v7/pkg/svc/eksidentity" + eksprovisioner "github.com/devantler-tech/ksail/v7/pkg/svc/provisioner/cluster/eks" + "github.com/devantler-tech/ksail/v7/pkg/svc/state" +) + +// adoptEKSCreationHint records ownership of a cluster that a failed `ksail cluster create` began +// building but that AWS reported only after KSail exited (ksail#8324). It runs only when no ownership +// record exists for the target and a failed-create hint does. The hint is untrusted: ownership is +// recorded only when the caller's account is the one the hint was kept for and the live cluster +// carries exactly the hint's creation tag. Any other same-named cluster leaves the hint in place and +// the record missing, so the normal ownership check still refuses it. When AWS confirms that no such +// cluster exists, the hint is removed. Persisting the record also removes the hint. +func adoptEKSCreationHint( + ctx context.Context, + resolved *lifecycle.ResolvedClusterInfo, + client eksidentity.Client, +) error { + clusterName := resolved.ClusterName + region := strings.TrimSpace(resolved.AWSRegion) + + _, err := state.LoadEKSOwnershipState(clusterName, region) + if !errors.Is(err, state.ErrEKSOwnershipStateNotFound) { + // A record exists, or one is present but unusable: the ownership verifier owns that outcome. + return nil + } + + hint, matches, err := loadCallerEKSCreationHint(ctx, client, clusterName, region) + if err != nil || !matches { + return err + } + + ownership, err := eksidentity.ObserveCreation( + ctx, + client, + clusterName, + region, + eksprovisioner.ClusterCreationTag, + hint.CreationID, + ) + + switch { + case errors.Is(err, eksclient.ErrClusterNotFound): + return discardEKSCreationHint(clusterName, region) + case errors.Is(err, eksidentity.ErrCreationUnattributed): + return nil + case err != nil: + return fmt.Errorf("check the failed create's attempt identifier against AWS: %w", err) + } + + ownership.AWSOptions = credentials.AWSOptionsWithDefaults(resolved.AWSOpts) + + err = eksidentity.Persist(clusterName, ownership.Region, ownership) + if err != nil { + return fmt.Errorf("record ownership of the failed create's EKS cluster: %w", err) + } + + return nil +} + +// discardStaleEKSCreationHint removes the failed-create hint once the exact eksctl query has reported +// that the cluster does not exist and the EKS API confirms it in the account the hint was kept for. +func discardStaleEKSCreationHint( + ctx context.Context, + resolved *lifecycle.ResolvedClusterInfo, + auth credentials.AWSResolution, +) error { + clusterName := resolved.ClusterName + region := strings.TrimSpace(resolved.AWSRegion) + + // Only a present hint is worth an AWS client; a damaged one fails in the caller-scoped load below. + _, err := state.LoadEKSCreationHint(clusterName, region) + if errors.Is(err, state.ErrEKSCreationHintNotFound) { + return nil + } + + eksIdentityClientFactoryState.RLock() + factory := eksIdentityClientFactoryState.factory + eksIdentityClientFactoryState.RUnlock() + + client, err := factory(ctx, region, auth) + if err != nil { + return fmt.Errorf( + "create AWS client to check the failed create's attempt identifier: %w", + err, + ) + } + + _, matches, err := loadCallerEKSCreationHint(ctx, client, clusterName, region) + if err != nil || !matches { + return err + } + + _, err = client.DescribeCluster(ctx, clusterName) + if errors.Is(err, eksclient.ErrClusterNotFound) { + return discardEKSCreationHint(clusterName, region) + } + + if err != nil { + return fmt.Errorf("check the failed create's attempt identifier against AWS: %w", err) + } + + return nil +} + +// loadCallerEKSCreationHint loads the failed-create hint for the target and reports whether it was +// kept for the caller's AWS account. A missing hint is not an error. +func loadCallerEKSCreationHint( + ctx context.Context, + client eksidentity.Client, + clusterName, region string, +) (*state.EKSCreationHint, bool, error) { + hint, err := state.LoadEKSCreationHint(clusterName, region) + if errors.Is(err, state.ErrEKSCreationHintNotFound) { + return nil, false, nil + } + + if err != nil { + return nil, false, fmt.Errorf("load the failed create's attempt identifier: %w", err) + } + + accountID, err := client.CallerAccountID(ctx) + if err != nil { + return nil, false, fmt.Errorf( + "resolve current AWS account for the failed create's attempt identifier: %w", + err, + ) + } + + return hint, strings.TrimSpace(accountID) == strings.TrimSpace(hint.AccountID), nil +} + +func discardEKSCreationHint(clusterName, region string) error { + err := state.DeleteEKSCreationHint(clusterName, region) + if err != nil { + return fmt.Errorf("remove the failed create's attempt identifier: %w", err) + } + + return nil +} diff --git a/pkg/cli/cmd/cluster/eks_creation_hint_test.go b/pkg/cli/cmd/cluster/eks_creation_hint_test.go new file mode 100644 index 0000000000..b525f51652 --- /dev/null +++ b/pkg/cli/cmd/cluster/eks_creation_hint_test.go @@ -0,0 +1,151 @@ +package cluster_test + +import ( + "fmt" + "testing" + + ekstypes "github.com/aws/aws-sdk-go-v2/service/eks/types" + eksclient "github.com/devantler-tech/ksail/v7/pkg/client/eks" + "github.com/devantler-tech/ksail/v7/pkg/svc/state" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const testEKSAccountID = "123456789012" + +// runInterruptedEKSCreate runs a failing create while AWS does not report the cluster yet, the way an +// interrupted create looks before the control plane is visible, and returns the shared identity +// client so the caller can make the cluster appear afterwards. +func runInterruptedEKSCreate(t *testing.T) (*fakeEKSIdentityClient, *[]string) { + t.Helper() + + client := &fakeEKSIdentityClient{ + accountID: testEKSAccountID, + describeErr: fmt.Errorf("%w: st-eks", eksclient.ErrClusterNotFound), + } + + output, deleted, err := runFailingEKSCreate(t, client) + + require.ErrorIs(t, err, errNodegroupCreateQuota) + assert.Contains(t, output, "kept this create attempt's identifier") + assert.NotContains(t, output, "recorded ownership") + + hint, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") + require.NoError(t, hintErr, "the failed create must keep its attempt identifier") + assert.Equal(t, testEKSCreationID, hint.CreationID) + assert.Equal(t, testEKSAccountID, hint.AccountID) + + _, loadErr := state.LoadEKSOwnershipState("st-eks", "eu-west-1") + require.ErrorIs(t, loadErr, state.ErrEKSOwnershipStateNotFound, + "the hint alone must not become an ownership record") + + return client, deleted +} + +// appear makes the cluster visible to the identity client, as AWS reports it once creation went on +// after KSail exited. +func (f *fakeEKSIdentityClient) appear(cluster *ekstypes.Cluster) { + f.describeErr = nil + f.cluster = cluster +} + +// TestCreate_EKSInterruptedCreateLetsDeleteRemoveLateCluster pins ksail#8324: a create interrupted +// before AWS reports the cluster keeps the attempt's identifier, and a later delete records ownership +// and removes the cluster once it appears carrying that identifier's tag. The hint is then spent. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreate_EKSInterruptedCreateLetsDeleteRemoveLateCluster(t *testing.T) { + client, deleted := runInterruptedEKSCreate(t) + + client.appear(taggedEKSCluster(testEKSCreationID)) + + calls, deleteErr := runEKSDeleteAfterFailedCreate(t) + require.NoError(t, deleteErr) + assert.Equal( + t, + []string{"get cluster --name st-eks --output json --region eu-west-1"}, + calls, + "delete must still pass the eksctl provenance query", + ) + assert.Equal(t, []string{"st-eks"}, *deleted) + + _, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") + require.ErrorIs(t, hintErr, state.ErrEKSCreationHintNotFound, "a used hint must be removed") +} + +// TestCreate_EKSCreationHintNeverAuthorizesAnotherCluster keeps every other same-named cluster +// refused after an interrupted create: one without this attempt's tag, one carrying another +// attempt's tag, and one reached under another AWS account. The hint stays for the real cluster. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreate_EKSCreationHintNeverAuthorizesAnotherCluster(t *testing.T) { + testCases := []struct { + name string + cluster *ekstypes.Cluster + accountID string + }{ + {name: "late cluster without a creation tag", cluster: taggedEKSCluster("")}, + { + name: "late cluster created by another attempt", + cluster: taggedEKSCluster("another-attempt"), + }, + { + name: "caller in another account", + cluster: taggedEKSCluster(testEKSCreationID), + accountID: "210987654321", + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + client, deleted := runInterruptedEKSCreate(t) + + client.appear(testCase.cluster) + + if testCase.accountID != "" { + client.accountID = testCase.accountID + } + + _, deleteErr := runEKSDeleteAfterFailedCreate(t) + require.Error(t, deleteErr, "delete must refuse a cluster this attempt did not create") + assert.Empty(t, *deleted) + + _, loadErr := state.LoadEKSOwnershipState("st-eks", "eu-west-1") + require.ErrorIs(t, loadErr, state.ErrEKSOwnershipStateNotFound) + + _, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") + require.NoError(t, hintErr, "an unmatched cluster must not spend the hint") + }) + } +} + +// TestCreate_EKSStaleCreationHintIsRemoved removes the hint once delete confirms that no such +// cluster exists, whether the exact eksctl query or the EKS API reports the absence. Delete still +// refuses, as it did before the hint existed. +func TestCreate_EKSStaleCreationHintIsRemoved(t *testing.T) { + testCases := []struct { + name string + eksctlAbsent bool + }{ + {name: "eksctl reports no cluster", eksctlAbsent: true}, + {name: "EKS API reports no cluster"}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + _, deleted := runInterruptedEKSCreate(t) + + if testCase.eksctlAbsent { + t.Setenv("KSAIL_EKSCTL_CLUSTER_ABSENT", "1") + } + + _, deleteErr := runEKSDeleteAfterFailedCreate(t) + require.Error(t, deleteErr) + assert.Empty(t, *deleted) + + _, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") + require.ErrorIs(t, hintErr, state.ErrEKSCreationHintNotFound, + "a hint for a cluster that does not exist must be removed") + }) + } +} diff --git a/pkg/cli/cmd/cluster/eks_failed_create.go b/pkg/cli/cmd/cluster/eks_failed_create.go index 4fa4e90a5d..c3c2714765 100644 --- a/pkg/cli/cmd/cluster/eks_failed_create.go +++ b/pkg/cli/cmd/cluster/eks_failed_create.go @@ -64,12 +64,25 @@ func retainFailedEKSCreateOwnership( ) defer cancel() - ownership, err := observeFailedEKSCreate(ctx, clusterCtx, clusterName, region) - if errors.Is(err, eksclient.ErrClusterNotFound) || - errors.Is(err, eksidentity.ErrCreationUnattributed) { - // Either the create failed before AWS accepted a control plane, or the cluster with this name - // is not the one this attempt began building (for example it already existed). Neither is - // this attempt's to record. + client, err := newFailedCreateIdentityClient(ctx, clusterCtx, region) + if err != nil { + return errors.Join( + createErr, + fmt.Errorf("record ownership of partially created EKS cluster: %w", err), + ) + } + + ownership, err := observeFailedEKSCreate(ctx, client, clusterCtx, clusterName, region) + if errors.Is(err, eksclient.ErrClusterNotFound) { + // AWS does not report the cluster yet. An interrupted create can still bring it into + // existence after KSail exits, so this attempt's identifier is kept as a local hint that a + // later lifecycle command checks against the live cluster's creation tag (ksail#8324). + return keepFailedEKSCreateHint(ctx, cmd, client, clusterCtx, clusterName, region, createErr) + } + + if errors.Is(err, eksidentity.ErrCreationUnattributed) { + // The cluster with this name is not the one this attempt began building (for example it + // already existed), so it is not this attempt's to record. return createErr } @@ -105,13 +118,52 @@ func hasEKSCreationAttempt(clusterCtx *localregistry.Context) bool { strings.TrimSpace(clusterCtx.EKSConfig.CreationID) != "" } -// observeFailedEKSCreate reads the cluster back under the create's frozen credentials and returns its -// identity only when it carries this attempt's creation tag. -func observeFailedEKSCreate( +// keepFailedEKSCreateHint stores this attempt's creation identifier for a cluster AWS did not report +// yet, keyed to the caller's account and the cluster's name and region. The hint never authorizes +// anything by itself. The original create error is always returned; a failure to keep the hint is +// joined to it. +func keepFailedEKSCreateHint( ctx context.Context, + cmd *cobra.Command, + client eksidentity.Client, clusterCtx *localregistry.Context, clusterName, region string, -) (*state.EKSOwnershipState, error) { + createErr error, +) error { + accountID, err := client.CallerAccountID(ctx) + if err == nil { + err = state.SaveEKSCreationHint(clusterName, region, &state.EKSCreationHint{ + Version: state.EKSCreationHintVersion, + ClusterName: clusterName, + Region: region, + AccountID: strings.TrimSpace(accountID), + CreationID: clusterCtx.EKSConfig.CreationID, + }) + } + + if err != nil { + return errors.Join( + createErr, + fmt.Errorf("keep the failed EKS create's attempt identifier: %w", err), + ) + } + + notify.Infof( + cmd.ErrOrStderr(), + "kept this create attempt's identifier for EKS cluster %q; if AWS finishes creating it, "+ + "`ksail cluster delete` can remove it", + clusterName, + ) + + return createErr +} + +// newFailedCreateIdentityClient builds the read-only AWS client for the create's frozen credentials. +func newFailedCreateIdentityClient( + ctx context.Context, + clusterCtx *localregistry.Context, + region string, +) (eksidentity.Client, error) { eksIdentityClientFactoryState.RLock() factory := eksIdentityClientFactoryState.factory eksIdentityClientFactoryState.RUnlock() @@ -121,6 +173,17 @@ func observeFailedEKSCreate( return nil, fmt.Errorf("create AWS client: %w", err) } + return client, nil +} + +// observeFailedEKSCreate reads the cluster back and returns its identity only when it carries this +// attempt's creation tag. +func observeFailedEKSCreate( + ctx context.Context, + client eksidentity.Client, + clusterCtx *localregistry.Context, + clusterName, region string, +) (*state.EKSOwnershipState, error) { ownership, err := eksidentity.ObserveCreation( ctx, client, diff --git a/pkg/cli/cmd/cluster/unmanaged_guard.go b/pkg/cli/cmd/cluster/unmanaged_guard.go index d2616ef8bc..76dca344e7 100644 --- a/pkg/cli/cmd/cluster/unmanaged_guard.go +++ b/pkg/cli/cmd/cluster/unmanaged_guard.go @@ -258,6 +258,11 @@ func ensureAWSClusterManaged( return err } + err = adoptEKSCreationHint(ctx, resolved, identityClient) + if err != nil { + return err + } + clusterName := resolved.ClusterName region := resolved.AWSRegion @@ -556,7 +561,7 @@ func queryFrozenAWSOwnership( ) if err != nil { if errors.Is(err, eksctlclient.ErrClusterNotFound) { - return credentials.AWSResolution{}, awsOwnershipMismatchError(resolved, err) + return credentials.AWSResolution{}, eksTargetAbsentError(ctx, resolved, auth, err) } return credentials.AWSResolution{}, awsOwnershipQueryError(resolved, err) @@ -821,3 +826,22 @@ func resolveUpdateTarget( return resolved } + +// eksTargetAbsentError reports an exact eksctl query that found no cluster. A failed create may have +// kept an attempt identifier for a cluster that never came to exist (ksail#8324): the lookup still +// fails, and that stale hint is removed alongside. +func eksTargetAbsentError( + ctx context.Context, + resolved *lifecycle.ResolvedClusterInfo, + auth credentials.AWSResolution, + cause error, +) error { + mismatchErr := awsOwnershipMismatchError(resolved, cause) + + discardErr := discardStaleEKSCreationHint(ctx, resolved, auth) + if discardErr != nil { + return errors.Join(mismatchErr, discardErr) + } + + return mismatchErr +} diff --git a/pkg/svc/eksidentity/identity.go b/pkg/svc/eksidentity/identity.go index d0b2f9d04a..6e132e01f5 100644 --- a/pkg/svc/eksidentity/identity.go +++ b/pkg/svc/eksidentity/identity.go @@ -99,6 +99,13 @@ func Persist(clusterName, region string, ownership *state.EKSOwnershipState) err return fmt.Errorf("persist immutable EKS ownership identity: %w", err) } + // A failed-create hint only ever stands in for a missing ownership record. Once the record + // exists, the hint is spent (ksail#8324). + err = state.DeleteEKSCreationHint(clusterName, region) + if err != nil { + return fmt.Errorf("clear spent EKS creation hint after ownership capture: %w", err) + } + return nil } diff --git a/pkg/svc/eksidentity/identity_test.go b/pkg/svc/eksidentity/identity_test.go index 8d8a314285..4065244151 100644 --- a/pkg/svc/eksidentity/identity_test.go +++ b/pkg/svc/eksidentity/identity_test.go @@ -110,6 +110,41 @@ func TestCaptureClearsStaleNodegroupCapacityState(t *testing.T) { require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) } +// TestCaptureSpendsFailedCreateHint pins ksail#8324: once an ownership record exists, a failed +// create's attempt identifier for the same name and region is spent and removed. +func TestCaptureSpendsFailedCreateHint(t *testing.T) { + t.Parallel() + + const clusterName = "capture-spends-creation-hint" + + t.Cleanup(func() { _ = state.DeleteClusterState(clusterName) }) + + require.NoError(t, state.SaveEKSCreationHint(clusterName, "eu-north-1", &state.EKSCreationHint{ + Version: state.EKSCreationHintVersion, + ClusterName: clusterName, + Region: "eu-north-1", + AccountID: "123456789012", + CreationID: "attempt-8324", + })) + + client := &fakeClient{ + accountID: "123456789012", + cluster: eksCluster( + clusterName, + "arn:aws:eks:eu-north-1:123456789012:cluster/"+clusterName, + identityTime(), + ), + } + + _, err := eksidentity.Capture( + t.Context(), client, clusterName, "eu-north-1", captureAWSOptions(), + ) + require.NoError(t, err) + + _, err = state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.ErrorIs(t, err, state.ErrEKSCreationHintNotFound) +} + func TestCaptureFailsClosedWhenNodegroupCapacityStateCannotBeCleared(t *testing.T) { t.Parallel() diff --git a/pkg/svc/state/eks_component_state.go b/pkg/svc/state/eks_component_state.go index 961977eeb6..4f33a4034e 100644 --- a/pkg/svc/state/eks_component_state.go +++ b/pkg/svc/state/eks_component_state.go @@ -362,14 +362,17 @@ func eksRegionStatePaths(clusterName, region string, accountIDs []string) ([]str paths = append(paths, unboundPaths...) } - nodegroupPath, err := eksNodegroupStatePath(clusterName, region) - if err != nil { - return nil, err - } + for _, regionScopedPath := range []func(string, string) (string, error){ + eksNodegroupStatePath, + eksOwnershipStatePath, + eksCreationHintPath, + } { + statePath, pathErr := regionScopedPath(clusterName, region) + if pathErr != nil { + return nil, pathErr + } - ownershipPath, err := eksOwnershipStatePath(clusterName, region) - if err != nil { - return nil, err + paths = append(paths, statePath) } ttlPaths, err := eksRegionTTLPaths(clusterName, region) @@ -382,7 +385,6 @@ func eksRegionStatePaths(clusterName, region string, accountIDs []string) ([]str return nil, err } - paths = append(paths, nodegroupPath, ownershipPath) paths = append(paths, ttlPaths...) return append(paths, specPath), nil diff --git a/pkg/svc/state/eks_creation_hint_state.go b/pkg/svc/state/eks_creation_hint_state.go new file mode 100644 index 0000000000..c7a2bba20a --- /dev/null +++ b/pkg/svc/state/eks_creation_hint_state.go @@ -0,0 +1,159 @@ +package state + +import ( + "encoding/json" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/devantler-tech/ksail/v7/pkg/fsutil" +) + +const ( + // EKSCreationHintVersion is the on-disk schema version for failed-create hints. + EKSCreationHintVersion = 1 + // eksCreationHintFileNameFormat keeps the hint region-scoped like the ownership record. Its name + // deliberately does not match the ownership record pattern, so a hint is never listed or loaded + // as ownership. + eksCreationHintFileNameFormat = "eks-creation-hint-%s.json" +) + +var ( + // ErrEKSCreationHintNotFound reports that no failed-create hint exists for the target. + ErrEKSCreationHintNotFound = errors.New("EKS creation hint not found") + // ErrInvalidEKSCreationHint reports a malformed or incomplete failed-create hint. + ErrInvalidEKSCreationHint = errors.New("invalid EKS creation hint") +) + +// EKSCreationHint keeps the creation identifier of a failed `ksail cluster create` on EKS whose +// cluster AWS did not report yet, so the cluster can still be claimed if it appears after KSail +// exited (ksail#8324). It is local and untrusted: it never authorizes a lifecycle operation on its +// own. A caller may only use it to look for a live cluster in the same account and region that +// carries exactly this identifier as its creation tag. +type EKSCreationHint struct { + Version int `json:"version"` + ClusterName string `json:"clusterName"` + Region string `json:"region"` + AccountID string `json:"accountId"` + CreationID string `json:"creationId"` +} + +// SaveEKSCreationHint atomically persists one validated failed-create hint. +func SaveEKSCreationHint(clusterName, region string, hint *EKSCreationHint) error { + statePath, err := eksCreationHintPath(clusterName, region) + if err != nil { + return err + } + + err = validateEKSCreationHint(clusterName, region, hint) + if err != nil { + return err + } + + err = os.MkdirAll(filepath.Dir(statePath), dirPermissions) + if err != nil { + return fmt.Errorf("failed to create EKS creation hint directory: %w", err) + } + + data, err := json.MarshalIndent(hint, "", " ") + if err != nil { + return fmt.Errorf("failed to marshal EKS creation hint: %w", err) + } + + err = fsutil.AtomicWriteFile(statePath, data, filePermissions) + if err != nil { + return fmt.Errorf("failed to write EKS creation hint: %w", err) + } + + return nil +} + +// LoadEKSCreationHint reads and validates the failed-create hint for one cluster name and region. +func LoadEKSCreationHint(clusterName, region string) (*EKSCreationHint, error) { + statePath, err := eksCreationHintPath(clusterName, region) + if err != nil { + return nil, err + } + + //nolint:gosec // clusterStateDir and eksRegionScopedStatePath constrain both path components. + data, err := os.ReadFile(statePath) + if err != nil { + if os.IsNotExist(err) { + return nil, fmt.Errorf( + "%w: %s in %s", + ErrEKSCreationHintNotFound, + clusterName, + region, + ) + } + + return nil, fmt.Errorf("failed to read EKS creation hint: %w", err) + } + + var hint EKSCreationHint + + err = json.Unmarshal(data, &hint) + if err != nil { + return nil, fmt.Errorf( + "unmarshal EKS creation hint: %w: %w", + err, + ErrInvalidEKSCreationHint, + ) + } + + err = validateEKSCreationHint(clusterName, region, &hint) + if err != nil { + return nil, err + } + + return &hint, nil +} + +// DeleteEKSCreationHint removes the failed-create hint for one cluster name and region. A missing +// hint is not an error. +func DeleteEKSCreationHint(clusterName, region string) error { + statePath, err := eksCreationHintPath(clusterName, region) + if err != nil { + return err + } + + err = os.Remove(statePath) + if err != nil && !os.IsNotExist(err) { + return fmt.Errorf("failed to remove EKS creation hint: %w", err) + } + + return nil +} + +func validateEKSCreationHint(clusterName, region string, hint *EKSCreationHint) error { + if hint == nil { + return fmt.Errorf("%w: hint is nil", ErrInvalidEKSCreationHint) + } + + if hint.Version != EKSCreationHintVersion { + return fmt.Errorf( + "%w: unsupported version %d (want %d)", + ErrInvalidEKSCreationHint, + hint.Version, + EKSCreationHintVersion, + ) + } + + if strings.TrimSpace(hint.ClusterName) != strings.TrimSpace(clusterName) || + strings.TrimSpace(hint.Region) != strings.TrimSpace(region) || + !awsAccountIDPattern.MatchString(strings.TrimSpace(hint.AccountID)) || + strings.TrimSpace(hint.CreationID) == "" { + return fmt.Errorf( + "%w: required fields are missing or do not match the state key", + ErrInvalidEKSCreationHint, + ) + } + + return nil +} + +func eksCreationHintPath(clusterName, region string) (string, error) { + return eksRegionScopedStatePath(clusterName, region, eksCreationHintFileNameFormat) +} diff --git a/pkg/svc/state/eks_creation_hint_state_test.go b/pkg/svc/state/eks_creation_hint_state_test.go new file mode 100644 index 0000000000..f35552b133 --- /dev/null +++ b/pkg/svc/state/eks_creation_hint_state_test.go @@ -0,0 +1,122 @@ +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 validEKSCreationHint(clusterName, region string) *state.EKSCreationHint { + return &state.EKSCreationHint{ + Version: state.EKSCreationHintVersion, + ClusterName: clusterName, + Region: region, + AccountID: "123456789012", + CreationID: "attempt-8324", + } +} + +func TestSaveLoadAndDeleteEKSCreationHint(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + const clusterName = "eks-hint-round-trip" + + want := validEKSCreationHint(clusterName, "eu-north-1") + require.NoError(t, state.SaveEKSCreationHint(clusterName, "eu-north-1", want)) + + got, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.NoError(t, err) + assert.Equal(t, want, got) + + statePath := filepath.Join( + home, ".ksail", "clusters", clusterName, "eks-creation-hint-eu-north-1.json", + ) + info, err := os.Stat(statePath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + + _, err = state.LoadEKSCreationHint(clusterName, "eu-west-1") + require.ErrorIs(t, err, state.ErrEKSCreationHintNotFound, "a hint is scoped to its region") + + _, err = state.ListEKSOwnershipStates(clusterName) + require.ErrorIs(t, err, state.ErrEKSOwnershipStateNotFound, "a hint is never ownership") + + require.NoError(t, state.DeleteEKSCreationHint(clusterName, "eu-north-1")) + require.NoError(t, state.DeleteEKSCreationHint(clusterName, "eu-north-1"), "absence is fine") + + _, err = state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.ErrorIs(t, err, state.ErrEKSCreationHintNotFound) +} + +func TestEKSCreationHintRejectsIncompleteOrMismatchedFields(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + mutate func(*state.EKSCreationHint) + }{ + {name: "nil hint"}, + {name: "unknown version", mutate: func(h *state.EKSCreationHint) { h.Version = 2 }}, + {name: "other cluster", mutate: func(h *state.EKSCreationHint) { h.ClusterName = "other" }}, + {name: "other region", mutate: func(h *state.EKSCreationHint) { h.Region = "eu-west-1" }}, + {name: "malformed account", mutate: func(h *state.EKSCreationHint) { h.AccountID = "12" }}, + {name: "empty creation id", mutate: func(h *state.EKSCreationHint) { h.CreationID = " " }}, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + t.Parallel() + + var hint *state.EKSCreationHint + if testCase.mutate != nil { + hint = validEKSCreationHint("eks-hint", "eu-north-1") + testCase.mutate(hint) + } + + err := state.SaveEKSCreationHint("eks-hint", "eu-north-1", hint) + require.ErrorIs(t, err, state.ErrInvalidEKSCreationHint) + }) + } +} + +func TestLoadEKSCreationHintRejectsInvalidJSON(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + stateDir := filepath.Join(home, ".ksail", "clusters", "invalid-hint") + require.NoError(t, os.MkdirAll(stateDir, 0o700)) + require.NoError(t, os.WriteFile( + filepath.Join(stateDir, "eks-creation-hint-eu-north-1.json"), + []byte("{"), + 0o600, + )) + + _, err := state.LoadEKSCreationHint("invalid-hint", "eu-north-1") + require.ErrorIs(t, err, state.ErrInvalidEKSCreationHint) +} + +func TestDeleteEKSRegionStateRemovesCreationHint(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "eks-hint-region-delete" + + require.NoError(t, state.SaveEKSCreationHint( + clusterName, "eu-north-1", validEKSCreationHint(clusterName, "eu-north-1"), + )) + require.NoError(t, state.SaveEKSCreationHint( + clusterName, "eu-west-1", validEKSCreationHint(clusterName, "eu-west-1"), + )) + + require.NoError(t, state.DeleteEKSRegionState(clusterName, "eu-north-1")) + + _, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.ErrorIs(t, err, state.ErrEKSCreationHintNotFound) + + _, err = state.LoadEKSCreationHint(clusterName, "eu-west-1") + require.NoError(t, err, "another region's hint must survive") +} From 87c65a4155ec395092836acde297f8506482dafc Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 21:16:12 +0100 Subject: [PATCH 2/6] fix(eks): keep every unresolved create attempt and reach it from anywhere Address the review of the failed-create hint: - Read the caller account once and reuse it for the hint, so a second failing account lookup cannot cost the attempt its hint. - Keep every unresolved attempt identifier (bounded) instead of replacing an earlier one with a retry's, and let a failed retry record a cluster an earlier kept attempt started. - Accept a hint as candidate local evidence, so a delete by name outside the project still reaches the eksctl, account and tag checks. - Read the hint through the confined reader. Part of #8324 Co-Authored-By: Claude Opus 5.5 --- pkg/cli/cmd/cluster/eks_creation_hint.go | 7 +- pkg/cli/cmd/cluster/eks_creation_hint_test.go | 94 +++++++++++++- pkg/cli/cmd/cluster/eks_failed_create.go | 75 ++++++----- pkg/cli/cmd/cluster/eks_failed_create_test.go | 24 +++- .../cluster/eks_immutable_identity_test.go | 10 ++ pkg/cli/cmd/cluster/unmanaged_guard.go | 12 +- pkg/svc/eksidentity/identity.go | 39 +++++- pkg/svc/eksidentity/identity_test.go | 2 +- pkg/svc/state/eks_creation_hint_state.go | 105 +++++++++++++--- pkg/svc/state/eks_creation_hint_state_test.go | 118 +++++++++++++++++- 10 files changed, 422 insertions(+), 64 deletions(-) diff --git a/pkg/cli/cmd/cluster/eks_creation_hint.go b/pkg/cli/cmd/cluster/eks_creation_hint.go index 53eb916dca..d0f4dbfa89 100644 --- a/pkg/cli/cmd/cluster/eks_creation_hint.go +++ b/pkg/cli/cmd/cluster/eks_creation_hint.go @@ -18,7 +18,7 @@ import ( // building but that AWS reported only after KSail exited (ksail#8324). It runs only when no ownership // record exists for the target and a failed-create hint does. The hint is untrusted: ownership is // recorded only when the caller's account is the one the hint was kept for and the live cluster -// carries exactly the hint's creation tag. Any other same-named cluster leaves the hint in place and +// carries exactly one of the hint's creation tags. Any other same-named cluster leaves the hint in place and // the record missing, so the normal ownership check still refuses it. When AWS confirms that no such // cluster exists, the hint is removed. Persisting the record also removes the hint. func adoptEKSCreationHint( @@ -40,13 +40,14 @@ func adoptEKSCreationHint( return err } - ownership, err := eksidentity.ObserveCreation( + ownership, err := eksidentity.ObserveCreationInAccount( ctx, client, + hint.AccountID, clusterName, region, eksprovisioner.ClusterCreationTag, - hint.CreationID, + hint.CreationIDs, ) switch { diff --git a/pkg/cli/cmd/cluster/eks_creation_hint_test.go b/pkg/cli/cmd/cluster/eks_creation_hint_test.go index b525f51652..e8fb7b5edc 100644 --- a/pkg/cli/cmd/cluster/eks_creation_hint_test.go +++ b/pkg/cli/cmd/cluster/eks_creation_hint_test.go @@ -32,7 +32,7 @@ func runInterruptedEKSCreate(t *testing.T) (*fakeEKSIdentityClient, *[]string) { hint, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") require.NoError(t, hintErr, "the failed create must keep its attempt identifier") - assert.Equal(t, testEKSCreationID, hint.CreationID) + assert.Equal(t, []string{testEKSCreationID}, hint.CreationIDs) assert.Equal(t, testEKSAccountID, hint.AccountID) _, loadErr := state.LoadEKSOwnershipState("st-eks", "eu-west-1") @@ -149,3 +149,95 @@ func TestCreate_EKSStaleCreationHintIsRemoved(t *testing.T) { }) } } + +// notFoundEKSCluster is the identity client's answer while AWS does not report st-eks. +func notFoundEKSCluster() error { + return fmt.Errorf("%w: st-eks", eksclient.ErrClusterNotFound) +} + +// TestCreate_EKSInterruptedCreateKeepsHintWhenLaterAccountLookupFails pins a review finding on +// ksail#8335: the account read before the cluster lookup is reused for the hint, so a second account +// lookup that would fail cannot cost the attempt its hint. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreate_EKSInterruptedCreateKeepsHintWhenLaterAccountLookupFails(t *testing.T) { + client := &fakeEKSIdentityClient{ + accountID: testEKSAccountID, + describeErr: notFoundEKSCluster(), + accountErrAfter: 1, + } + + output, _, err := runFailingEKSCreate(t, client) + + require.ErrorIs(t, err, errNodegroupCreateQuota) + require.NotErrorIs(t, err, errEKSIdentityQuery) + assert.Contains(t, output, "kept this create attempt's identifier") + + hint, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") + require.NoError(t, hintErr) + assert.Equal(t, []string{testEKSCreationID}, hint.CreationIDs) +} + +// TestCreate_EKSRetriedCreateKeepsEarlierAttempt pins a review finding on ksail#8335: a retry that +// also fails before AWS reports the cluster keeps the earlier attempt, so the cluster the earlier +// attempt started is still deletable when it appears. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreate_EKSRetriedCreateKeepsEarlierAttempt(t *testing.T) { + client, _ := runInterruptedEKSCreate(t) + + _, deleted, err := runFailingEKSCreateAttempt(t, client, "attempt-retry") + require.ErrorIs(t, err, errNodegroupCreateQuota) + + hint, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") + require.NoError(t, hintErr) + assert.Equal(t, []string{testEKSCreationID, "attempt-retry"}, hint.CreationIDs) + + client.appear(taggedEKSCluster(testEKSCreationID)) + + _, deleteErr := runEKSDeleteAfterFailedCreate(t) + require.NoError(t, deleteErr) + assert.Equal(t, []string{"st-eks"}, *deleted) +} + +// TestCreate_EKSRetryRecordsEarlierAttemptsLateCluster pins a review finding on ksail#8335: when the +// cluster an earlier interrupted attempt started has appeared, a retry fails because the name is +// taken. That failure records ownership of the cluster, because its live creation tag matches the +// kept earlier attempt, and spends the hint. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreate_EKSRetryRecordsEarlierAttemptsLateCluster(t *testing.T) { + client, _ := runInterruptedEKSCreate(t) + + client.appear(taggedEKSCluster(testEKSCreationID)) + + output, _, err := runFailingEKSCreateAttempt(t, client, "attempt-retry") + require.ErrorIs(t, err, errNodegroupCreateQuota) + assert.Contains(t, output, "recorded ownership of partially created EKS cluster") + + ownership, loadErr := state.LoadEKSOwnershipState("st-eks", "eu-west-1") + require.NoError(t, loadErr) + assert.Equal(t, testEKSAccountID, ownership.AccountID) + + _, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") + require.ErrorIs(t, hintErr, state.ErrEKSCreationHintNotFound, "the used hint is spent") +} + +// TestCreate_EKSHintAloneLetsDeleteReachVerificationOutsideTheProject pins a review finding on +// ksail#8335: when the hint is the only local record of the cluster, a delete by name from another +// directory still reaches the eksctl, account and creation-tag checks, and removes the cluster that +// carries the kept tag. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreate_EKSHintAloneLetsDeleteReachVerificationOutsideTheProject(t *testing.T) { + client, deleted := runInterruptedEKSCreate(t) + + client.appear(taggedEKSCluster(testEKSCreationID)) + t.Chdir(t.TempDir()) + t.Setenv("AWS_REGION", "eu-west-1") + + calls, deleteErr := runEKSDeleteAfterFailedCreate(t, "--name", "st-eks", "--provider", "AWS") + require.NoError(t, deleteErr) + assert.Contains(t, calls, "get cluster --name st-eks --output json --region eu-west-1") + assert.Equal(t, []string{"st-eks"}, *deleted) +} diff --git a/pkg/cli/cmd/cluster/eks_failed_create.go b/pkg/cli/cmd/cluster/eks_failed_create.go index c3c2714765..e67a729cd3 100644 --- a/pkg/cli/cmd/cluster/eks_failed_create.go +++ b/pkg/cli/cmd/cluster/eks_failed_create.go @@ -65,6 +65,14 @@ func retainFailedEKSCreateOwnership( defer cancel() client, err := newFailedCreateIdentityClient(ctx, clusterCtx, region) + + var accountID string + if err == nil { + // The account is read once and reused, so a later failing account lookup can never cost + // the attempt its hint. + accountID, err = client.CallerAccountID(ctx) + } + if err != nil { return errors.Join( createErr, @@ -72,17 +80,23 @@ func retainFailedEKSCreateOwnership( ) } - ownership, err := observeFailedEKSCreate(ctx, client, clusterCtx, clusterName, region) + accountID = strings.TrimSpace(accountID) + creationID := clusterCtx.EKSConfig.CreationID + + ownership, err := observeFailedEKSCreate( + ctx, client, accountID, clusterName, region, + failedCreateCandidateIDs(clusterName, region, accountID, creationID), + ) if errors.Is(err, eksclient.ErrClusterNotFound) { // AWS does not report the cluster yet. An interrupted create can still bring it into // existence after KSail exits, so this attempt's identifier is kept as a local hint that a // later lifecycle command checks against the live cluster's creation tag (ksail#8324). - return keepFailedEKSCreateHint(ctx, cmd, client, clusterCtx, clusterName, region, createErr) + return keepFailedEKSCreateHint(cmd, clusterName, region, accountID, creationID, createErr) } if errors.Is(err, eksidentity.ErrCreationUnattributed) { - // The cluster with this name is not the one this attempt began building (for example it - // already existed), so it is not this attempt's to record. + // The cluster with this name is not one this attempt or an earlier kept attempt began + // building (for example it already existed), so it is not this attempt's to record. return createErr } @@ -118,29 +132,31 @@ func hasEKSCreationAttempt(clusterCtx *localregistry.Context) bool { strings.TrimSpace(clusterCtx.EKSConfig.CreationID) != "" } -// keepFailedEKSCreateHint stores this attempt's creation identifier for a cluster AWS did not report -// yet, keyed to the caller's account and the cluster's name and region. The hint never authorizes -// anything by itself. The original create error is always returned; a failure to keep the hint is -// joined to it. +// failedCreateCandidateIDs returns this attempt's identifier followed by every earlier unresolved +// attempt kept for the same account. A retry whose create fails because an earlier attempt's cluster +// appeared in the meantime can then still record that cluster, but only when its live creation tag +// matches one of these identifiers (ksail#8324). +func failedCreateCandidateIDs(clusterName, region, accountID, creationID string) []string { + candidates := []string{creationID} + + hint, err := state.LoadEKSCreationHint(clusterName, region) + if err != nil || hint.AccountID != accountID { + return candidates + } + + return append(candidates, hint.CreationIDs...) +} + +// keepFailedEKSCreateHint adds this attempt's creation identifier to the hint for a cluster AWS did +// not report yet, keyed to the caller's account and the cluster's name and region. Earlier unresolved +// attempts stay in the hint. The hint never authorizes anything by itself. The original create error +// is always returned; a failure to keep the hint is joined to it. func keepFailedEKSCreateHint( - ctx context.Context, cmd *cobra.Command, - client eksidentity.Client, - clusterCtx *localregistry.Context, - clusterName, region string, + clusterName, region, accountID, creationID string, createErr error, ) error { - accountID, err := client.CallerAccountID(ctx) - if err == nil { - err = state.SaveEKSCreationHint(clusterName, region, &state.EKSCreationHint{ - Version: state.EKSCreationHintVersion, - ClusterName: clusterName, - Region: region, - AccountID: strings.TrimSpace(accountID), - CreationID: clusterCtx.EKSConfig.CreationID, - }) - } - + err := state.RecordEKSCreationAttempt(clusterName, region, accountID, creationID) if err != nil { return errors.Join( createErr, @@ -176,21 +192,22 @@ func newFailedCreateIdentityClient( return client, nil } -// observeFailedEKSCreate reads the cluster back and returns its identity only when it carries this -// attempt's creation tag. +// observeFailedEKSCreate reads the cluster back in the caller's account and returns its identity only +// when it carries one of the candidate creation tags. func observeFailedEKSCreate( ctx context.Context, client eksidentity.Client, - clusterCtx *localregistry.Context, - clusterName, region string, + accountID, clusterName, region string, + creationIDs []string, ) (*state.EKSOwnershipState, error) { - ownership, err := eksidentity.ObserveCreation( + ownership, err := eksidentity.ObserveCreationInAccount( ctx, client, + accountID, clusterName, region, eksprovisioner.ClusterCreationTag, - clusterCtx.EKSConfig.CreationID, + creationIDs, ) if err != nil { return nil, fmt.Errorf("observe partially created EKS cluster: %w", err) diff --git a/pkg/cli/cmd/cluster/eks_failed_create_test.go b/pkg/cli/cmd/cluster/eks_failed_create_test.go index 6a1392bb7e..e85a0e6c89 100644 --- a/pkg/cli/cmd/cluster/eks_failed_create_test.go +++ b/pkg/cli/cmd/cluster/eks_failed_create_test.go @@ -81,6 +81,21 @@ func taggedEKSCluster(creationID string) *ekstypes.Cluster { func runFailingEKSCreate(t *testing.T, client eksidentity.Client) (string, *[]string, error) { t.Helper() + clearFailedCreateOwnership(t) + t.Cleanup(func() { clearFailedCreateOwnership(t) }) + + return runFailingEKSCreateAttempt(t, client, testEKSCreationID) +} + +// runFailingEKSCreateAttempt is runFailingEKSCreate for one attempt identifier, keeping any st-eks state +// an earlier attempt in the same test left behind. +func runFailingEKSCreateAttempt( + t *testing.T, + client eksidentity.Client, + creationID string, +) (string, *[]string, error) { + t.Helper() + workingDir := t.TempDir() t.Chdir(workingDir) setupMockRegistryBackend(t) @@ -89,12 +104,9 @@ func runFailingEKSCreate(t *testing.T, client eksidentity.Client) (string, *[]st deleted := &[]string{} t.Cleanup(cluster.SetProvisionerFactoryForTests(failingCreateFactory{deleted: deleted})) - t.Cleanup(cluster.ExportSetEKSCreationIDGenerator(func() string { return testEKSCreationID })) + t.Cleanup(cluster.ExportSetEKSCreationIDGenerator(func() string { return creationID })) setEKSIdentityClient(t, client) - clearFailedCreateOwnership(t) - t.Cleanup(func() { clearFailedCreateOwnership(t) }) - cmd := cluster.NewCreateCmd() var output bytes.Buffer @@ -112,7 +124,7 @@ func runFailingEKSCreate(t *testing.T, client eksidentity.Client) (string, *[]st // runEKSDeleteAfterFailedCreate runs `ksail cluster delete --force` in the failed create's working // directory, with an eksctl stand-in that reports the cluster as eksctl-created and records each // ownership query. It returns the recorded eksctl calls and the command error. -func runEKSDeleteAfterFailedCreate(t *testing.T) ([]string, error) { +func runEKSDeleteAfterFailedCreate(t *testing.T, extraArgs ...string) ([]string, error) { t.Helper() binDir := t.TempDir() @@ -127,7 +139,7 @@ func runEKSDeleteAfterFailedCreate(t *testing.T) ([]string, error) { cmd.SetContext(t.Context()) cmd.SetOut(io.Discard) cmd.SetErr(io.Discard) - cmd.SetArgs([]string{"--force"}) + cmd.SetArgs(append([]string{"--force"}, extraArgs...)) err := cmd.Execute() diff --git a/pkg/cli/cmd/cluster/eks_immutable_identity_test.go b/pkg/cli/cmd/cluster/eks_immutable_identity_test.go index e726f55396..6f9c007763 100644 --- a/pkg/cli/cmd/cluster/eks_immutable_identity_test.go +++ b/pkg/cli/cmd/cluster/eks_immutable_identity_test.go @@ -33,9 +33,19 @@ type fakeEKSIdentityClient struct { describeErr error describeCalls int beforeDescribe func(int) + // accountCalls counts CallerAccountID calls; accountErrAfter, when positive, makes every call after + // that many fail with errEKSIdentityQuery. + accountCalls int + accountErrAfter int } func (f *fakeEKSIdentityClient) CallerAccountID(_ context.Context) (string, error) { + f.accountCalls++ + + if f.accountErrAfter > 0 && f.accountCalls > f.accountErrAfter { + return "", errEKSIdentityQuery + } + return f.accountID, f.accountErr } diff --git a/pkg/cli/cmd/cluster/unmanaged_guard.go b/pkg/cli/cmd/cluster/unmanaged_guard.go index 76dca344e7..db61b95be7 100644 --- a/pkg/cli/cmd/cluster/unmanaged_guard.go +++ b/pkg/cli/cmd/cluster/unmanaged_guard.go @@ -600,13 +600,21 @@ func hasLocalKSailEKSTargetEvidence(resolved *lifecycle.ResolvedClusterInfo) boo // A region-qualified command can validate its exact record even when an unrelated // sibling is unreadable. A name-only command still needs a complete listing. _, ownershipErr := state.LoadEKSOwnershipState(resolved.ClusterName, region) + if ownershipErr == nil { + return true + } + + // A failed create that kept its attempt identifier (ksail#8324) is candidate evidence + // only: the eksctl provenance query, the account check and the live creation-tag match + // still have to pass before anything is recorded or deleted. + _, hintErr := state.LoadEKSCreationHint(resolved.ClusterName, region) - return ownershipErr == nil + return hintErr == nil } _, ownershipErr := state.ListEKSOwnershipStates(resolved.ClusterName) - return ownershipErr == nil + return ownershipErr == nil || state.HasEKSCreationHint(resolved.ClusterName) } return spec.Distribution == v1alpha1.DistributionEKS && spec.Provider == v1alpha1.ProviderAWS diff --git a/pkg/svc/eksidentity/identity.go b/pkg/svc/eksidentity/identity.go index 6e132e01f5..a36a6a4092 100644 --- a/pkg/svc/eksidentity/identity.go +++ b/pkg/svc/eksidentity/identity.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "slices" "strings" "github.com/aws/aws-sdk-go-v2/aws" @@ -280,21 +281,49 @@ func ObserveCreation( return nil, fmt.Errorf("%w: no creation identifier to match", ErrCreationUnattributed) } - accountID, cluster, err := describeForCapture(ctx, client, clusterName) + accountID, err := client.CallerAccountID(ctx) if err != nil { - return nil, err + return nil, fmt.Errorf("resolve current AWS account for EKS ownership capture: %w", err) + } + + return ObserveCreationInAccount( + ctx, client, accountID, clusterName, region, tagKey, []string{creationID}, + ) +} + +// ObserveCreationInAccount is ObserveCreation for a caller whose account the caller already resolved, +// matching any one of creationIDs. Every identifier is one a KSail create attempt wrote and kept, so a +// cluster carrying one of them as its tagKey tag is a cluster one of those attempts began building. The +// cluster must still sit in accountID. It performs no AWS mutation and persists nothing. +func ObserveCreationInAccount( + ctx context.Context, + client Client, + accountID, clusterName, region, tagKey string, + creationIDs []string, +) (*state.EKSOwnershipState, error) { + creationIDs = slices.DeleteFunc(slices.Clone(creationIDs), func(id string) bool { + return strings.TrimSpace(id) == "" + }) + + if strings.TrimSpace(tagKey) == "" || len(creationIDs) == 0 { + return nil, fmt.Errorf("%w: no creation identifier to match", ErrCreationUnattributed) } - if cluster == nil || cluster.Tags[tagKey] != creationID { + cluster, err := client.DescribeCluster(ctx, clusterName) + if err != nil { + return nil, fmt.Errorf("describe EKS cluster for ownership capture: %w", err) + } + + if cluster == nil || !slices.Contains(creationIDs, cluster.Tags[tagKey]) { return nil, fmt.Errorf( - "%w: cluster %q does not carry this attempt's %s tag", + "%w: cluster %q does not carry a creation tag this attempt kept (%s)", ErrCreationUnattributed, clusterName, tagKey, ) } - return identityFromCluster(clusterName, region, accountID, cluster) + return identityFromCluster(clusterName, region, strings.TrimSpace(accountID), cluster) } func identityFromCluster( //nolint:cyclop // one validation boundary intentionally checks every AWS identity field. diff --git a/pkg/svc/eksidentity/identity_test.go b/pkg/svc/eksidentity/identity_test.go index 4065244151..150c447017 100644 --- a/pkg/svc/eksidentity/identity_test.go +++ b/pkg/svc/eksidentity/identity_test.go @@ -124,7 +124,7 @@ func TestCaptureSpendsFailedCreateHint(t *testing.T) { ClusterName: clusterName, Region: "eu-north-1", AccountID: "123456789012", - CreationID: "attempt-8324", + CreationIDs: []string{"attempt-8324"}, })) client := &fakeClient{ diff --git a/pkg/svc/state/eks_creation_hint_state.go b/pkg/svc/state/eks_creation_hint_state.go index c7a2bba20a..6cebb91f1a 100644 --- a/pkg/svc/state/eks_creation_hint_state.go +++ b/pkg/svc/state/eks_creation_hint_state.go @@ -4,8 +4,10 @@ import ( "encoding/json" "errors" "fmt" + "io/fs" "os" "path/filepath" + "slices" "strings" "github.com/devantler-tech/ksail/v7/pkg/fsutil" @@ -14,6 +16,9 @@ import ( const ( // EKSCreationHintVersion is the on-disk schema version for failed-create hints. EKSCreationHintVersion = 1 + // MaxEKSCreationHintAttempts bounds how many unresolved create attempts one hint remembers. The + // oldest attempt is dropped first. + MaxEKSCreationHintAttempts = 16 // eksCreationHintFileNameFormat keeps the hint region-scoped like the ownership record. Its name // deliberately does not match the ownership record pattern, so a hint is never listed or loaded // as ownership. @@ -27,17 +32,45 @@ var ( ErrInvalidEKSCreationHint = errors.New("invalid EKS creation hint") ) -// EKSCreationHint keeps the creation identifier of a failed `ksail cluster create` on EKS whose -// cluster AWS did not report yet, so the cluster can still be claimed if it appears after KSail -// exited (ksail#8324). It is local and untrusted: it never authorizes a lifecycle operation on its -// own. A caller may only use it to look for a live cluster in the same account and region that -// carries exactly this identifier as its creation tag. +// EKSCreationHint keeps the creation identifiers of failed `ksail cluster create` attempts on EKS whose +// cluster AWS did not report yet, so the cluster can still be claimed if it appears after KSail exited +// (ksail#8324). Every unresolved attempt is kept, because a retry that also fails must not hide an +// earlier attempt whose cluster may still appear. It is local and untrusted: it never authorizes a +// lifecycle operation on its own. A caller may only use it to look for a live cluster in the same +// account and region that carries exactly one of these identifiers as its creation tag. type EKSCreationHint struct { - Version int `json:"version"` - ClusterName string `json:"clusterName"` - Region string `json:"region"` - AccountID string `json:"accountId"` - CreationID string `json:"creationId"` + Version int `json:"version"` + ClusterName string `json:"clusterName"` + Region string `json:"region"` + AccountID string `json:"accountId"` + CreationIDs []string `json:"creationIds"` +} + +// RecordEKSCreationAttempt adds one unresolved create attempt to the target's hint. Attempts kept +// earlier for the same account are retained, newest last, up to MaxEKSCreationHintAttempts. A hint kept +// for another account, or one that cannot be read, is replaced: its attempts could never be matched +// from this account. +func RecordEKSCreationAttempt(clusterName, region, accountID, creationID string) error { + hint := &EKSCreationHint{ + Version: EKSCreationHintVersion, + ClusterName: strings.TrimSpace(clusterName), + Region: strings.TrimSpace(region), + AccountID: strings.TrimSpace(accountID), + } + + existing, err := LoadEKSCreationHint(clusterName, region) + if err == nil && existing.AccountID == hint.AccountID { + hint.CreationIDs = slices.DeleteFunc(existing.CreationIDs, func(id string) bool { + return id == creationID + }) + } + + hint.CreationIDs = append(hint.CreationIDs, creationID) + if overflow := len(hint.CreationIDs) - MaxEKSCreationHintAttempts; overflow > 0 { + hint.CreationIDs = hint.CreationIDs[overflow:] + } + + return SaveEKSCreationHint(clusterName, region, hint) } // SaveEKSCreationHint atomically persists one validated failed-create hint. @@ -70,17 +103,17 @@ func SaveEKSCreationHint(clusterName, region string, hint *EKSCreationHint) erro return nil } -// LoadEKSCreationHint reads and validates the failed-create hint for one cluster name and region. +// LoadEKSCreationHint reads and validates the failed-create hint for one cluster name and region. The +// read is confined to the hint's own directory, so a hint file that resolves elsewhere is refused. func LoadEKSCreationHint(clusterName, region string) (*EKSCreationHint, error) { statePath, err := eksCreationHintPath(clusterName, region) if err != nil { return nil, err } - //nolint:gosec // clusterStateDir and eksRegionScopedStatePath constrain both path components. - data, err := os.ReadFile(statePath) + data, err := fsutil.ReadFileSafe(filepath.Dir(statePath), statePath) if err != nil { - if os.IsNotExist(err) { + if isMissingStateFile(statePath, err) { return nil, fmt.Errorf( "%w: %s in %s", ErrEKSCreationHintNotFound, @@ -111,6 +144,29 @@ func LoadEKSCreationHint(clusterName, region string) (*EKSCreationHint, error) { return &hint, nil } +// HasEKSCreationHint reports whether a failed-create hint file exists for the cluster in any region. +// It says nothing about whether the hint is valid or matches anything; callers use it only as local +// evidence that a KSail create was attempted for this name. +func HasEKSCreationHint(clusterName string) bool { + dir, err := clusterStateDir(clusterName) + if err != nil { + return false + } + + entries, err := os.ReadDir(dir) + if err != nil { + return false + } + + pattern := fmt.Sprintf(eksCreationHintFileNameFormat, "*") + + return slices.ContainsFunc(entries, func(entry fs.DirEntry) bool { + matched, matchErr := filepath.Match(pattern, entry.Name()) + + return matchErr == nil && matched && entry.Type().IsRegular() + }) +} + // DeleteEKSCreationHint removes the failed-create hint for one cluster name and region. A missing // hint is not an error. func DeleteEKSCreationHint(clusterName, region string) error { @@ -127,6 +183,22 @@ func DeleteEKSCreationHint(clusterName, region string) error { return nil } +// isMissingStateFile reports whether a confined read failed because the file does not exist. The +// confined reader reports a missing file as outside its base, so that case is checked directly. +func isMissingStateFile(statePath string, readErr error) bool { + if errors.Is(readErr, os.ErrNotExist) { + return true + } + + if !errors.Is(readErr, fsutil.ErrPathOutsideBase) { + return false + } + + _, statErr := os.Lstat(statePath) + + return errors.Is(statErr, os.ErrNotExist) +} + func validateEKSCreationHint(clusterName, region string, hint *EKSCreationHint) error { if hint == nil { return fmt.Errorf("%w: hint is nil", ErrInvalidEKSCreationHint) @@ -144,7 +216,10 @@ func validateEKSCreationHint(clusterName, region string, hint *EKSCreationHint) if strings.TrimSpace(hint.ClusterName) != strings.TrimSpace(clusterName) || strings.TrimSpace(hint.Region) != strings.TrimSpace(region) || !awsAccountIDPattern.MatchString(strings.TrimSpace(hint.AccountID)) || - strings.TrimSpace(hint.CreationID) == "" { + len(hint.CreationIDs) == 0 || len(hint.CreationIDs) > MaxEKSCreationHintAttempts || + slices.ContainsFunc(hint.CreationIDs, func(id string) bool { + return strings.TrimSpace(id) == "" + }) { return fmt.Errorf( "%w: required fields are missing or do not match the state key", ErrInvalidEKSCreationHint, diff --git a/pkg/svc/state/eks_creation_hint_state_test.go b/pkg/svc/state/eks_creation_hint_state_test.go index f35552b133..a54ba13077 100644 --- a/pkg/svc/state/eks_creation_hint_state_test.go +++ b/pkg/svc/state/eks_creation_hint_state_test.go @@ -1,6 +1,7 @@ package state_test import ( + "fmt" "os" "path/filepath" "testing" @@ -16,7 +17,7 @@ func validEKSCreationHint(clusterName, region string) *state.EKSCreationHint { ClusterName: clusterName, Region: region, AccountID: "123456789012", - CreationID: "attempt-8324", + CreationIDs: []string{"attempt-8324"}, } } @@ -65,7 +66,20 @@ func TestEKSCreationHintRejectsIncompleteOrMismatchedFields(t *testing.T) { {name: "other cluster", mutate: func(h *state.EKSCreationHint) { h.ClusterName = "other" }}, {name: "other region", mutate: func(h *state.EKSCreationHint) { h.Region = "eu-west-1" }}, {name: "malformed account", mutate: func(h *state.EKSCreationHint) { h.AccountID = "12" }}, - {name: "empty creation id", mutate: func(h *state.EKSCreationHint) { h.CreationID = " " }}, + { + name: "blank creation id", + mutate: func(h *state.EKSCreationHint) { h.CreationIDs = []string{" "} }, + }, + {name: "no creation ids", mutate: func(h *state.EKSCreationHint) { h.CreationIDs = nil }}, + { + name: "too many creation ids", + mutate: func(h *state.EKSCreationHint) { + h.CreationIDs = make([]string, state.MaxEKSCreationHintAttempts+1) + for i := range h.CreationIDs { + h.CreationIDs[i] = fmt.Sprintf("attempt-%d", i) + } + }, + }, } for _, testCase := range testCases { @@ -120,3 +134,103 @@ func TestDeleteEKSRegionStateRemovesCreationHint(t *testing.T) { _, err = state.LoadEKSCreationHint(clusterName, "eu-west-1") require.NoError(t, err, "another region's hint must survive") } + +// TestRecordEKSCreationAttemptKeepsEarlierUnresolvedAttempts pins the review finding on ksail#8335: a +// retry that also fails before AWS reports the cluster must not drop an earlier attempt whose cluster +// can still appear. +func TestRecordEKSCreationAttemptKeepsEarlierUnresolvedAttempts(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "eks-hint-retries" + + require.NoError( + t, + state.RecordEKSCreationAttempt(clusterName, "eu-north-1", "123456789012", "a"), + ) + require.NoError( + t, + state.RecordEKSCreationAttempt(clusterName, "eu-north-1", "123456789012", "b"), + ) + require.NoError( + t, + state.RecordEKSCreationAttempt(clusterName, "eu-north-1", "123456789012", "a"), + ) + + hint, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.NoError(t, err) + assert.Equal(t, []string{"b", "a"}, hint.CreationIDs, "earlier attempts stay, newest last") + + require.NoError( + t, + state.RecordEKSCreationAttempt(clusterName, "eu-north-1", "210987654321", "c"), + ) + + hint, err = state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.NoError(t, err) + assert.Equal(t, "210987654321", hint.AccountID) + assert.Equal(t, []string{"c"}, hint.CreationIDs, "another account's attempts cannot match here") +} + +func TestRecordEKSCreationAttemptDropsOldestBeyondTheBound(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "eks-hint-bound" + + for i := range state.MaxEKSCreationHintAttempts + 2 { + require.NoError(t, state.RecordEKSCreationAttempt( + clusterName, "eu-north-1", "123456789012", fmt.Sprintf("attempt-%d", i), + )) + } + + hint, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.NoError(t, err) + require.Len(t, hint.CreationIDs, state.MaxEKSCreationHintAttempts) + assert.Equal(t, "attempt-2", hint.CreationIDs[0]) + assert.Equal( + t, + fmt.Sprintf("attempt-%d", state.MaxEKSCreationHintAttempts+1), + hint.CreationIDs[len(hint.CreationIDs)-1], + ) +} + +// TestLoadEKSCreationHintRefusesFileOutsideItsDirectory keeps the hint read confined to the cluster's +// state directory: a hint file that is a symbolic link to elsewhere is refused, not followed. +func TestLoadEKSCreationHintRefusesFileOutsideItsDirectory(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + const clusterName = "eks-hint-link" + + outside := filepath.Join(t.TempDir(), "hint.json") + data := `{"version":1,"clusterName":"eks-hint-link","region":"eu-north-1",` + + `"accountId":"123456789012","creationIds":["attempt-8324"]}` + require.NoError(t, os.WriteFile(outside, []byte(data), 0o600)) + + stateDir := filepath.Join(home, ".ksail", "clusters", clusterName) + require.NoError(t, os.MkdirAll(stateDir, 0o700)) + require.NoError( + t, + os.Symlink(outside, filepath.Join(stateDir, "eks-creation-hint-eu-north-1.json")), + ) + + _, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.Error(t, err) + require.NotErrorIs(t, err, state.ErrEKSCreationHintNotFound) +} + +func TestHasEKSCreationHint(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "eks-hint-presence" + + assert.False(t, state.HasEKSCreationHint(clusterName)) + + require.NoError( + t, + state.RecordEKSCreationAttempt(clusterName, "eu-north-1", "123456789012", "a"), + ) + assert.True(t, state.HasEKSCreationHint(clusterName)) + + require.NoError(t, state.DeleteEKSCreationHint(clusterName, "eu-north-1")) + assert.False(t, state.HasEKSCreationHint(clusterName)) +} From 7b1647342be6483163b352df294aecc2992c1e1d Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 21:30:11 +0100 Subject: [PATCH 3/6] refactor(eks): read the failed create's caller account in one helper Part of #8324 Co-Authored-By: Claude Opus 5.5 --- pkg/cli/cmd/cluster/eks_creation_hint_test.go | 2 - pkg/cli/cmd/cluster/eks_failed_create.go | 38 ++++++++++++------- 2 files changed, 24 insertions(+), 16 deletions(-) diff --git a/pkg/cli/cmd/cluster/eks_creation_hint_test.go b/pkg/cli/cmd/cluster/eks_creation_hint_test.go index e8fb7b5edc..7f9781cce8 100644 --- a/pkg/cli/cmd/cluster/eks_creation_hint_test.go +++ b/pkg/cli/cmd/cluster/eks_creation_hint_test.go @@ -227,8 +227,6 @@ func TestCreate_EKSRetryRecordsEarlierAttemptsLateCluster(t *testing.T) { // ksail#8335: when the hint is the only local record of the cluster, a delete by name from another // directory still reaches the eksctl, account and creation-tag checks, and removes the cluster that // carries the kept tag. -// -//nolint:paralleltest // uses t.Chdir and mutates shared test hooks func TestCreate_EKSHintAloneLetsDeleteReachVerificationOutsideTheProject(t *testing.T) { client, deleted := runInterruptedEKSCreate(t) diff --git a/pkg/cli/cmd/cluster/eks_failed_create.go b/pkg/cli/cmd/cluster/eks_failed_create.go index e67a729cd3..752bba98fa 100644 --- a/pkg/cli/cmd/cluster/eks_failed_create.go +++ b/pkg/cli/cmd/cluster/eks_failed_create.go @@ -64,15 +64,7 @@ func retainFailedEKSCreateOwnership( ) defer cancel() - client, err := newFailedCreateIdentityClient(ctx, clusterCtx, region) - - var accountID string - if err == nil { - // The account is read once and reused, so a later failing account lookup can never cost - // the attempt its hint. - accountID, err = client.CallerAccountID(ctx) - } - + client, accountID, err := failedCreateCaller(ctx, clusterCtx, region) if err != nil { return errors.Join( createErr, @@ -80,7 +72,6 @@ func retainFailedEKSCreateOwnership( ) } - accountID = strings.TrimSpace(accountID) creationID := clusterCtx.EKSConfig.CreationID ownership, err := observeFailedEKSCreate( @@ -137,14 +128,12 @@ func hasEKSCreationAttempt(clusterCtx *localregistry.Context) bool { // appeared in the meantime can then still record that cluster, but only when its live creation tag // matches one of these identifiers (ksail#8324). func failedCreateCandidateIDs(clusterName, region, accountID, creationID string) []string { - candidates := []string{creationID} - hint, err := state.LoadEKSCreationHint(clusterName, region) if err != nil || hint.AccountID != accountID { - return candidates + return []string{creationID} } - return append(candidates, hint.CreationIDs...) + return append([]string{creationID}, hint.CreationIDs...) } // keepFailedEKSCreateHint adds this attempt's creation identifier to the hint for a cluster AWS did @@ -215,3 +204,24 @@ func observeFailedEKSCreate( return ownership, nil } + +// failedCreateCaller builds the read-only AWS client for the create's frozen credentials and reads +// the caller's account once. The account is reused for the observation and the hint, so a later +// failing account lookup can never cost the attempt its hint. +func failedCreateCaller( + ctx context.Context, + clusterCtx *localregistry.Context, + region string, +) (eksidentity.Client, string, error) { + client, err := newFailedCreateIdentityClient(ctx, clusterCtx, region) + if err != nil { + return nil, "", err + } + + accountID, err := client.CallerAccountID(ctx) + if err != nil { + return nil, "", fmt.Errorf("resolve current AWS account: %w", err) + } + + return client, strings.TrimSpace(accountID), nil +} From 3f960393f933d4cf8e7b87240cabd4122bf2fb44 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 22:23:56 +0100 Subject: [PATCH 4/6] fix(eks): keep failed create attempts per account without a bound A hint now groups unresolved attempt identifiers by the AWS account they ran in and drops none of them: neither a long run of retries nor an attempt made after switching accounts can hide an earlier attempt whose cluster may still appear. Recording ownership, or AWS confirming the cluster does not exist, forgets only the caller account's attempts. A region state cleanup no longer removes the hint. Part of #8324 Co-Authored-By: Claude Opus 5.5 --- pkg/cli/cmd/cluster/eks_creation_hint.go | 54 ++++--- pkg/cli/cmd/cluster/eks_creation_hint_test.go | 35 ++++- pkg/cli/cmd/cluster/eks_failed_create.go | 4 +- pkg/svc/eksidentity/identity.go | 4 +- pkg/svc/eksidentity/identity_test.go | 21 +-- pkg/svc/state/eks_component_state.go | 18 +-- pkg/svc/state/eks_creation_hint_state.go | 128 ++++++++++++---- pkg/svc/state/eks_creation_hint_state_test.go | 142 +++++++++--------- 8 files changed, 252 insertions(+), 154 deletions(-) diff --git a/pkg/cli/cmd/cluster/eks_creation_hint.go b/pkg/cli/cmd/cluster/eks_creation_hint.go index d0f4dbfa89..602585c0fd 100644 --- a/pkg/cli/cmd/cluster/eks_creation_hint.go +++ b/pkg/cli/cmd/cluster/eks_creation_hint.go @@ -17,10 +17,10 @@ import ( // adoptEKSCreationHint records ownership of a cluster that a failed `ksail cluster create` began // building but that AWS reported only after KSail exited (ksail#8324). It runs only when no ownership // record exists for the target and a failed-create hint does. The hint is untrusted: ownership is -// recorded only when the caller's account is the one the hint was kept for and the live cluster -// carries exactly one of the hint's creation tags. Any other same-named cluster leaves the hint in place and -// the record missing, so the normal ownership check still refuses it. When AWS confirms that no such -// cluster exists, the hint is removed. Persisting the record also removes the hint. +// recorded only when the live cluster, read in the caller's account, carries exactly one of the +// creation tags kept for that account. Any other same-named cluster leaves the hint in place and the +// record missing, so the normal ownership check still refuses it. When AWS confirms that no such +// cluster exists, the caller account's attempts are forgotten. Persisting the record also forgets them. func adoptEKSCreationHint( ctx context.Context, resolved *lifecycle.ResolvedClusterInfo, @@ -35,24 +35,24 @@ func adoptEKSCreationHint( return nil } - hint, matches, err := loadCallerEKSCreationHint(ctx, client, clusterName, region) - if err != nil || !matches { + accountID, creationIDs, err := loadCallerEKSCreationAttempts(ctx, client, clusterName, region) + if err != nil || len(creationIDs) == 0 { return err } ownership, err := eksidentity.ObserveCreationInAccount( ctx, client, - hint.AccountID, + accountID, clusterName, region, eksprovisioner.ClusterCreationTag, - hint.CreationIDs, + creationIDs, ) switch { case errors.Is(err, eksclient.ErrClusterNotFound): - return discardEKSCreationHint(clusterName, region) + return forgetEKSCreationAttempts(clusterName, region, accountID) case errors.Is(err, eksidentity.ErrCreationUnattributed): return nil case err != nil: @@ -69,8 +69,9 @@ func adoptEKSCreationHint( return nil } -// discardStaleEKSCreationHint removes the failed-create hint once the exact eksctl query has reported -// that the cluster does not exist and the EKS API confirms it in the account the hint was kept for. +// discardStaleEKSCreationHint forgets the caller account's kept attempts once the exact eksctl query +// has reported that the cluster does not exist and the EKS API confirms it in that account. Attempts +// kept for other accounts stay. func discardStaleEKSCreationHint( ctx context.Context, resolved *lifecycle.ResolvedClusterInfo, @@ -97,14 +98,14 @@ func discardStaleEKSCreationHint( ) } - _, matches, err := loadCallerEKSCreationHint(ctx, client, clusterName, region) - if err != nil || !matches { + accountID, creationIDs, err := loadCallerEKSCreationAttempts(ctx, client, clusterName, region) + if err != nil || len(creationIDs) == 0 { return err } _, err = client.DescribeCluster(ctx, clusterName) if errors.Is(err, eksclient.ErrClusterNotFound) { - return discardEKSCreationHint(clusterName, region) + return forgetEKSCreationAttempts(clusterName, region, accountID) } if err != nil { @@ -114,37 +115,40 @@ func discardStaleEKSCreationHint( return nil } -// loadCallerEKSCreationHint loads the failed-create hint for the target and reports whether it was -// kept for the caller's AWS account. A missing hint is not an error. -func loadCallerEKSCreationHint( +// loadCallerEKSCreationAttempts resolves the caller's AWS account and returns the attempt identifiers +// the target's failed-create hint kept for that account. A missing hint, or one with no attempts for +// the caller's account, returns no identifiers and no error. +func loadCallerEKSCreationAttempts( ctx context.Context, client eksidentity.Client, clusterName, region string, -) (*state.EKSCreationHint, bool, error) { +) (string, []string, error) { hint, err := state.LoadEKSCreationHint(clusterName, region) if errors.Is(err, state.ErrEKSCreationHintNotFound) { - return nil, false, nil + return "", nil, nil } if err != nil { - return nil, false, fmt.Errorf("load the failed create's attempt identifier: %w", err) + return "", nil, fmt.Errorf("load the failed create's attempt identifier: %w", err) } accountID, err := client.CallerAccountID(ctx) if err != nil { - return nil, false, fmt.Errorf( + return "", nil, fmt.Errorf( "resolve current AWS account for the failed create's attempt identifier: %w", err, ) } - return hint, strings.TrimSpace(accountID) == strings.TrimSpace(hint.AccountID), nil + accountID = strings.TrimSpace(accountID) + + return accountID, hint.CreationIDsFor(accountID), nil } -func discardEKSCreationHint(clusterName, region string) error { - err := state.DeleteEKSCreationHint(clusterName, region) +func forgetEKSCreationAttempts(clusterName, region, accountID string) error { + err := state.ForgetEKSCreationAttempts(clusterName, region, accountID) if err != nil { - return fmt.Errorf("remove the failed create's attempt identifier: %w", err) + return fmt.Errorf("forget the failed create's attempt identifiers: %w", err) } return nil diff --git a/pkg/cli/cmd/cluster/eks_creation_hint_test.go b/pkg/cli/cmd/cluster/eks_creation_hint_test.go index 7f9781cce8..97face6356 100644 --- a/pkg/cli/cmd/cluster/eks_creation_hint_test.go +++ b/pkg/cli/cmd/cluster/eks_creation_hint_test.go @@ -32,8 +32,7 @@ func runInterruptedEKSCreate(t *testing.T) (*fakeEKSIdentityClient, *[]string) { hint, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") require.NoError(t, hintErr, "the failed create must keep its attempt identifier") - assert.Equal(t, []string{testEKSCreationID}, hint.CreationIDs) - assert.Equal(t, testEKSAccountID, hint.AccountID) + assert.Equal(t, []string{testEKSCreationID}, hint.CreationIDsFor(testEKSAccountID)) _, loadErr := state.LoadEKSOwnershipState("st-eks", "eu-west-1") require.ErrorIs(t, loadErr, state.ErrEKSOwnershipStateNotFound, @@ -175,7 +174,7 @@ func TestCreate_EKSInterruptedCreateKeepsHintWhenLaterAccountLookupFails(t *test hint, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") require.NoError(t, hintErr) - assert.Equal(t, []string{testEKSCreationID}, hint.CreationIDs) + assert.Equal(t, []string{testEKSCreationID}, hint.CreationIDsFor(testEKSAccountID)) } // TestCreate_EKSRetriedCreateKeepsEarlierAttempt pins a review finding on ksail#8335: a retry that @@ -191,7 +190,11 @@ func TestCreate_EKSRetriedCreateKeepsEarlierAttempt(t *testing.T) { hint, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") require.NoError(t, hintErr) - assert.Equal(t, []string{testEKSCreationID, "attempt-retry"}, hint.CreationIDs) + assert.Equal( + t, + []string{testEKSCreationID, "attempt-retry"}, + hint.CreationIDsFor(testEKSAccountID), + ) client.appear(taggedEKSCluster(testEKSCreationID)) @@ -239,3 +242,27 @@ func TestCreate_EKSHintAloneLetsDeleteReachVerificationOutsideTheProject(t *test assert.Contains(t, calls, "get cluster --name st-eks --output json --region eu-west-1") assert.Equal(t, []string{"st-eks"}, *deleted) } + +// TestCreate_EKSAttemptInAnotherAccountKeepsEarlierAttempt pins a review finding on ksail#8335: a +// failed attempt made after switching AWS accounts keeps the first account's attempt, so its cluster +// is still deletable once the caller switches back and the cluster appears. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreate_EKSAttemptInAnotherAccountKeepsEarlierAttempt(t *testing.T) { + client, _ := runInterruptedEKSCreate(t) + + client.accountID = "210987654321" + _, deleted, err := runFailingEKSCreateAttempt(t, client, "attempt-other-account") + require.ErrorIs(t, err, errNodegroupCreateQuota) + + client.accountID = testEKSAccountID + client.appear(taggedEKSCluster(testEKSCreationID)) + + _, deleteErr := runEKSDeleteAfterFailedCreate(t) + require.NoError(t, deleteErr) + assert.Equal(t, []string{"st-eks"}, *deleted) + + hint, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") + require.NoError(t, hintErr, "the other account's attempt is not spent") + assert.Equal(t, []string{"attempt-other-account"}, hint.CreationIDsFor("210987654321")) +} diff --git a/pkg/cli/cmd/cluster/eks_failed_create.go b/pkg/cli/cmd/cluster/eks_failed_create.go index 752bba98fa..780bc8a670 100644 --- a/pkg/cli/cmd/cluster/eks_failed_create.go +++ b/pkg/cli/cmd/cluster/eks_failed_create.go @@ -129,11 +129,11 @@ func hasEKSCreationAttempt(clusterCtx *localregistry.Context) bool { // matches one of these identifiers (ksail#8324). func failedCreateCandidateIDs(clusterName, region, accountID, creationID string) []string { hint, err := state.LoadEKSCreationHint(clusterName, region) - if err != nil || hint.AccountID != accountID { + if err != nil { return []string{creationID} } - return append([]string{creationID}, hint.CreationIDs...) + return append([]string{creationID}, hint.CreationIDsFor(accountID)...) } // keepFailedEKSCreateHint adds this attempt's creation identifier to the hint for a cluster AWS did diff --git a/pkg/svc/eksidentity/identity.go b/pkg/svc/eksidentity/identity.go index a36a6a4092..a0d8237920 100644 --- a/pkg/svc/eksidentity/identity.go +++ b/pkg/svc/eksidentity/identity.go @@ -101,8 +101,8 @@ func Persist(clusterName, region string, ownership *state.EKSOwnershipState) err } // A failed-create hint only ever stands in for a missing ownership record. Once the record - // exists, the hint is spent (ksail#8324). - err = state.DeleteEKSCreationHint(clusterName, region) + // exists, the attempts kept for its account are spent (ksail#8324). + err = state.ForgetEKSCreationAttempts(clusterName, region, ownership.AccountID) if err != nil { return fmt.Errorf("clear spent EKS creation hint after ownership capture: %w", err) } diff --git a/pkg/svc/eksidentity/identity_test.go b/pkg/svc/eksidentity/identity_test.go index 150c447017..f025c892cd 100644 --- a/pkg/svc/eksidentity/identity_test.go +++ b/pkg/svc/eksidentity/identity_test.go @@ -111,7 +111,7 @@ func TestCaptureClearsStaleNodegroupCapacityState(t *testing.T) { } // TestCaptureSpendsFailedCreateHint pins ksail#8324: once an ownership record exists, a failed -// create's attempt identifier for the same name and region is spent and removed. +// create's attempts kept for that account are spent; another account's attempts stay. func TestCaptureSpendsFailedCreateHint(t *testing.T) { t.Parallel() @@ -119,13 +119,12 @@ func TestCaptureSpendsFailedCreateHint(t *testing.T) { t.Cleanup(func() { _ = state.DeleteClusterState(clusterName) }) - require.NoError(t, state.SaveEKSCreationHint(clusterName, "eu-north-1", &state.EKSCreationHint{ - Version: state.EKSCreationHintVersion, - ClusterName: clusterName, - Region: "eu-north-1", - AccountID: "123456789012", - CreationIDs: []string{"attempt-8324"}, - })) + require.NoError(t, state.RecordEKSCreationAttempt( + clusterName, "eu-north-1", "123456789012", "attempt-8324", + )) + require.NoError(t, state.RecordEKSCreationAttempt( + clusterName, "eu-north-1", "210987654321", "attempt-other-account", + )) client := &fakeClient{ accountID: "123456789012", @@ -141,8 +140,10 @@ func TestCaptureSpendsFailedCreateHint(t *testing.T) { ) require.NoError(t, err) - _, err = state.LoadEKSCreationHint(clusterName, "eu-north-1") - require.ErrorIs(t, err, state.ErrEKSCreationHintNotFound) + hint, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.NoError(t, err, "another account's attempts are not spent") + assert.Empty(t, hint.CreationIDsFor("123456789012")) + assert.Equal(t, []string{"attempt-other-account"}, hint.CreationIDsFor("210987654321")) } func TestCaptureFailsClosedWhenNodegroupCapacityStateCannotBeCleared(t *testing.T) { diff --git a/pkg/svc/state/eks_component_state.go b/pkg/svc/state/eks_component_state.go index 4f33a4034e..961977eeb6 100644 --- a/pkg/svc/state/eks_component_state.go +++ b/pkg/svc/state/eks_component_state.go @@ -362,17 +362,14 @@ func eksRegionStatePaths(clusterName, region string, accountIDs []string) ([]str paths = append(paths, unboundPaths...) } - for _, regionScopedPath := range []func(string, string) (string, error){ - eksNodegroupStatePath, - eksOwnershipStatePath, - eksCreationHintPath, - } { - statePath, pathErr := regionScopedPath(clusterName, region) - if pathErr != nil { - return nil, pathErr - } + nodegroupPath, err := eksNodegroupStatePath(clusterName, region) + if err != nil { + return nil, err + } - paths = append(paths, statePath) + ownershipPath, err := eksOwnershipStatePath(clusterName, region) + if err != nil { + return nil, err } ttlPaths, err := eksRegionTTLPaths(clusterName, region) @@ -385,6 +382,7 @@ func eksRegionStatePaths(clusterName, region string, accountIDs []string) ([]str return nil, err } + paths = append(paths, nodegroupPath, ownershipPath) paths = append(paths, ttlPaths...) return append(paths, specPath), nil diff --git a/pkg/svc/state/eks_creation_hint_state.go b/pkg/svc/state/eks_creation_hint_state.go index 6cebb91f1a..1898ef68fa 100644 --- a/pkg/svc/state/eks_creation_hint_state.go +++ b/pkg/svc/state/eks_creation_hint_state.go @@ -16,9 +16,6 @@ import ( const ( // EKSCreationHintVersion is the on-disk schema version for failed-create hints. EKSCreationHintVersion = 1 - // MaxEKSCreationHintAttempts bounds how many unresolved create attempts one hint remembers. The - // oldest attempt is dropped first. - MaxEKSCreationHintAttempts = 16 // eksCreationHintFileNameFormat keeps the hint region-scoped like the ownership record. Its name // deliberately does not match the ownership record pattern, so a hint is never listed or loaded // as ownership. @@ -34,40 +31,91 @@ var ( // EKSCreationHint keeps the creation identifiers of failed `ksail cluster create` attempts on EKS whose // cluster AWS did not report yet, so the cluster can still be claimed if it appears after KSail exited -// (ksail#8324). Every unresolved attempt is kept, because a retry that also fails must not hide an -// earlier attempt whose cluster may still appear. It is local and untrusted: it never authorizes a -// lifecycle operation on its own. A caller may only use it to look for a live cluster in the same -// account and region that carries exactly one of these identifiers as its creation tag. +// (ksail#8324). Attempts are grouped by the AWS account they ran in, and an attempt is kept until a +// lifecycle command resolves it for that account: a retry, or an attempt in another account, never +// hides an earlier attempt whose cluster may still appear. It is local and untrusted: it never +// authorizes a lifecycle operation on its own. A caller may only use it to look for a live cluster in +// its own account and this region that carries exactly one of that account's identifiers as its +// creation tag. type EKSCreationHint struct { - Version int `json:"version"` - ClusterName string `json:"clusterName"` - Region string `json:"region"` + Version int `json:"version"` + ClusterName string `json:"clusterName"` + Region string `json:"region"` + Accounts []EKSCreationAttempts `json:"accounts"` +} + +// EKSCreationAttempts lists the unresolved create attempts made in one AWS account, oldest first. +type EKSCreationAttempts struct { AccountID string `json:"accountId"` CreationIDs []string `json:"creationIds"` } -// RecordEKSCreationAttempt adds one unresolved create attempt to the target's hint. Attempts kept -// earlier for the same account are retained, newest last, up to MaxEKSCreationHintAttempts. A hint kept -// for another account, or one that cannot be read, is replaced: its attempts could never be matched -// from this account. +// CreationIDsFor returns the unresolved attempt identifiers kept for accountID, oldest first. +func (h *EKSCreationHint) CreationIDsFor(accountID string) []string { + accountID = strings.TrimSpace(accountID) + + for _, attempts := range h.Accounts { + if attempts.AccountID == accountID { + return slices.Clone(attempts.CreationIDs) + } + } + + return nil +} + +// RecordEKSCreationAttempt adds one unresolved create attempt made in accountID to the target's hint. +// Every attempt kept earlier, for this or any other account, is retained. A hint that cannot be read +// is replaced, since nothing in it can be trusted to match. func RecordEKSCreationAttempt(clusterName, region, accountID, creationID string) error { - hint := &EKSCreationHint{ - Version: EKSCreationHintVersion, - ClusterName: strings.TrimSpace(clusterName), - Region: strings.TrimSpace(region), - AccountID: strings.TrimSpace(accountID), + accountID = strings.TrimSpace(accountID) + + hint, err := LoadEKSCreationHint(clusterName, region) + if err != nil { + hint = &EKSCreationHint{ + Version: EKSCreationHintVersion, + ClusterName: strings.TrimSpace(clusterName), + Region: strings.TrimSpace(region), + } } - existing, err := LoadEKSCreationHint(clusterName, region) - if err == nil && existing.AccountID == hint.AccountID { - hint.CreationIDs = slices.DeleteFunc(existing.CreationIDs, func(id string) bool { - return id == creationID - }) + index := slices.IndexFunc(hint.Accounts, func(attempts EKSCreationAttempts) bool { + return attempts.AccountID == accountID + }) + if index < 0 { + hint.Accounts = append(hint.Accounts, EKSCreationAttempts{AccountID: accountID}) + index = len(hint.Accounts) - 1 + } + + kept := &hint.Accounts[index] + kept.CreationIDs = slices.DeleteFunc(kept.CreationIDs, func(id string) bool { + return id == creationID + }) + kept.CreationIDs = append(kept.CreationIDs, creationID) + + return SaveEKSCreationHint(clusterName, region, hint) +} + +// ForgetEKSCreationAttempts removes the attempts kept for accountID once a lifecycle command resolved +// them in that account: their cluster was recorded, or AWS confirmed it does not exist. Attempts kept +// for other accounts stay. The hint file is removed when no attempts remain. A missing hint is not an +// error. +func ForgetEKSCreationAttempts(clusterName, region, accountID string) error { + hint, err := LoadEKSCreationHint(clusterName, region) + if errors.Is(err, ErrEKSCreationHintNotFound) { + return nil + } + + if err != nil { + return err } - hint.CreationIDs = append(hint.CreationIDs, creationID) - if overflow := len(hint.CreationIDs) - MaxEKSCreationHintAttempts; overflow > 0 { - hint.CreationIDs = hint.CreationIDs[overflow:] + accountID = strings.TrimSpace(accountID) + hint.Accounts = slices.DeleteFunc(hint.Accounts, func(attempts EKSCreationAttempts) bool { + return attempts.AccountID == accountID + }) + + if len(hint.Accounts) == 0 { + return DeleteEKSCreationHint(clusterName, region) } return SaveEKSCreationHint(clusterName, region, hint) @@ -215,20 +263,36 @@ func validateEKSCreationHint(clusterName, region string, hint *EKSCreationHint) if strings.TrimSpace(hint.ClusterName) != strings.TrimSpace(clusterName) || strings.TrimSpace(hint.Region) != strings.TrimSpace(region) || - !awsAccountIDPattern.MatchString(strings.TrimSpace(hint.AccountID)) || - len(hint.CreationIDs) == 0 || len(hint.CreationIDs) > MaxEKSCreationHintAttempts || - slices.ContainsFunc(hint.CreationIDs, func(id string) bool { - return strings.TrimSpace(id) == "" - }) { + len(hint.Accounts) == 0 { return fmt.Errorf( "%w: required fields are missing or do not match the state key", ErrInvalidEKSCreationHint, ) } + seen := make(map[string]bool, len(hint.Accounts)) + + for _, attempts := range hint.Accounts { + if !validEKSCreationAttempts(attempts) || seen[attempts.AccountID] { + return fmt.Errorf( + "%w: each account must be a distinct AWS account with at least one attempt", + ErrInvalidEKSCreationHint, + ) + } + + seen[attempts.AccountID] = true + } + return nil } +func validEKSCreationAttempts(attempts EKSCreationAttempts) bool { + return awsAccountIDPattern.MatchString(attempts.AccountID) && len(attempts.CreationIDs) > 0 && + !slices.ContainsFunc(attempts.CreationIDs, func(id string) bool { + return strings.TrimSpace(id) == "" + }) +} + func eksCreationHintPath(clusterName, region string) (string, error) { return eksRegionScopedStatePath(clusterName, region, eksCreationHintFileNameFormat) } diff --git a/pkg/svc/state/eks_creation_hint_state_test.go b/pkg/svc/state/eks_creation_hint_state_test.go index a54ba13077..ef01af5a5c 100644 --- a/pkg/svc/state/eks_creation_hint_state_test.go +++ b/pkg/svc/state/eks_creation_hint_state_test.go @@ -11,13 +11,19 @@ import ( "github.com/stretchr/testify/require" ) +const ( + hintAccount = "123456789012" + hintOtherAccount = "210987654321" +) + func validEKSCreationHint(clusterName, region string) *state.EKSCreationHint { return &state.EKSCreationHint{ Version: state.EKSCreationHintVersion, ClusterName: clusterName, Region: region, - AccountID: "123456789012", - CreationIDs: []string{"attempt-8324"}, + Accounts: []state.EKSCreationAttempts{ + {AccountID: hintAccount, CreationIDs: []string{"attempt-8324"}}, + }, } } @@ -33,6 +39,8 @@ func TestSaveLoadAndDeleteEKSCreationHint(t *testing.T) { got, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") require.NoError(t, err) assert.Equal(t, want, got) + assert.Equal(t, []string{"attempt-8324"}, got.CreationIDsFor(hintAccount)) + assert.Empty(t, got.CreationIDsFor(hintOtherAccount)) statePath := filepath.Join( home, ".ksail", "clusters", clusterName, "eks-creation-hint-eu-north-1.json", @@ -65,19 +73,23 @@ func TestEKSCreationHintRejectsIncompleteOrMismatchedFields(t *testing.T) { {name: "unknown version", mutate: func(h *state.EKSCreationHint) { h.Version = 2 }}, {name: "other cluster", mutate: func(h *state.EKSCreationHint) { h.ClusterName = "other" }}, {name: "other region", mutate: func(h *state.EKSCreationHint) { h.Region = "eu-west-1" }}, - {name: "malformed account", mutate: func(h *state.EKSCreationHint) { h.AccountID = "12" }}, + {name: "no accounts", mutate: func(h *state.EKSCreationHint) { h.Accounts = nil }}, + { + name: "malformed account", + mutate: func(h *state.EKSCreationHint) { h.Accounts[0].AccountID = "12" }, + }, { name: "blank creation id", - mutate: func(h *state.EKSCreationHint) { h.CreationIDs = []string{" "} }, + mutate: func(h *state.EKSCreationHint) { h.Accounts[0].CreationIDs = []string{" "} }, + }, + { + name: "no creation ids", + mutate: func(h *state.EKSCreationHint) { h.Accounts[0].CreationIDs = nil }, }, - {name: "no creation ids", mutate: func(h *state.EKSCreationHint) { h.CreationIDs = nil }}, { - name: "too many creation ids", + name: "duplicate account", mutate: func(h *state.EKSCreationHint) { - h.CreationIDs = make([]string, state.MaxEKSCreationHintAttempts+1) - for i := range h.CreationIDs { - h.CreationIDs[i] = fmt.Sprintf("attempt-%d", i) - } + h.Accounts = append(h.Accounts, h.Accounts[0]) }, }, } @@ -114,83 +126,78 @@ func TestLoadEKSCreationHintRejectsInvalidJSON(t *testing.T) { require.ErrorIs(t, err, state.ErrInvalidEKSCreationHint) } -func TestDeleteEKSRegionStateRemovesCreationHint(t *testing.T) { +// TestRecordEKSCreationAttemptKeepsEveryUnresolvedAttempt pins the review findings on ksail#8335: a +// retry that also fails before AWS reports the cluster, many retries, or an attempt made in another +// account must never drop an earlier attempt whose cluster can still appear. +func TestRecordEKSCreationAttemptKeepsEveryUnresolvedAttempt(t *testing.T) { t.Setenv("HOME", t.TempDir()) - const clusterName = "eks-hint-region-delete" + const ( + clusterName = "eks-hint-retries" + attempts = 40 + ) - require.NoError(t, state.SaveEKSCreationHint( - clusterName, "eu-north-1", validEKSCreationHint(clusterName, "eu-north-1"), - )) - require.NoError(t, state.SaveEKSCreationHint( - clusterName, "eu-west-1", validEKSCreationHint(clusterName, "eu-west-1"), - )) + want := make([]string, 0, attempts) - require.NoError(t, state.DeleteEKSRegionState(clusterName, "eu-north-1")) + for attempt := range attempts { + id := fmt.Sprintf("attempt-%d", attempt) + want = append(want, id) - _, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") - require.ErrorIs(t, err, state.ErrEKSCreationHintNotFound) + require.NoError( + t, + state.RecordEKSCreationAttempt(clusterName, "eu-north-1", hintAccount, id), + ) - _, err = state.LoadEKSCreationHint(clusterName, "eu-west-1") - require.NoError(t, err, "another region's hint must survive") -} - -// TestRecordEKSCreationAttemptKeepsEarlierUnresolvedAttempts pins the review finding on ksail#8335: a -// retry that also fails before AWS reports the cluster must not drop an earlier attempt whose cluster -// can still appear. -func TestRecordEKSCreationAttemptKeepsEarlierUnresolvedAttempts(t *testing.T) { - t.Setenv("HOME", t.TempDir()) - - const clusterName = "eks-hint-retries" + if attempt == attempts/2 { + require.NoError(t, state.RecordEKSCreationAttempt( + clusterName, "eu-north-1", hintOtherAccount, "other-account-attempt", + )) + } + } require.NoError( t, - state.RecordEKSCreationAttempt(clusterName, "eu-north-1", "123456789012", "a"), - ) - require.NoError( - t, - state.RecordEKSCreationAttempt(clusterName, "eu-north-1", "123456789012", "b"), - ) - require.NoError( - t, - state.RecordEKSCreationAttempt(clusterName, "eu-north-1", "123456789012", "a"), + state.RecordEKSCreationAttempt(clusterName, "eu-north-1", hintAccount, "attempt-0"), ) + want = append(want[1:], "attempt-0") + hint, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") require.NoError(t, err) - assert.Equal(t, []string{"b", "a"}, hint.CreationIDs, "earlier attempts stay, newest last") - - require.NoError( + assert.Equal(t, want, hint.CreationIDsFor(hintAccount), "every attempt stays, newest last") + assert.Equal( t, - state.RecordEKSCreationAttempt(clusterName, "eu-north-1", "210987654321", "c"), + []string{"other-account-attempt"}, + hint.CreationIDsFor(hintOtherAccount), + "another account's attempt stays beside them", ) - - hint, err = state.LoadEKSCreationHint(clusterName, "eu-north-1") - require.NoError(t, err) - assert.Equal(t, "210987654321", hint.AccountID) - assert.Equal(t, []string{"c"}, hint.CreationIDs, "another account's attempts cannot match here") } -func TestRecordEKSCreationAttemptDropsOldestBeyondTheBound(t *testing.T) { +func TestForgetEKSCreationAttemptsRemovesOnlyThatAccount(t *testing.T) { t.Setenv("HOME", t.TempDir()) - const clusterName = "eks-hint-bound" + const clusterName = "eks-hint-forget" - for i := range state.MaxEKSCreationHintAttempts + 2 { - require.NoError(t, state.RecordEKSCreationAttempt( - clusterName, "eu-north-1", "123456789012", fmt.Sprintf("attempt-%d", i), - )) - } + require.NoError(t, state.ForgetEKSCreationAttempts(clusterName, "eu-north-1", hintAccount), + "a missing hint is fine") - hint, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") - require.NoError(t, err) - require.Len(t, hint.CreationIDs, state.MaxEKSCreationHintAttempts) - assert.Equal(t, "attempt-2", hint.CreationIDs[0]) - assert.Equal( + require.NoError(t, state.RecordEKSCreationAttempt(clusterName, "eu-north-1", hintAccount, "a")) + require.NoError( t, - fmt.Sprintf("attempt-%d", state.MaxEKSCreationHintAttempts+1), - hint.CreationIDs[len(hint.CreationIDs)-1], + state.RecordEKSCreationAttempt(clusterName, "eu-north-1", hintOtherAccount, "b"), ) + + require.NoError(t, state.ForgetEKSCreationAttempts(clusterName, "eu-north-1", hintAccount)) + + hint, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.NoError(t, err) + assert.Empty(t, hint.CreationIDsFor(hintAccount)) + assert.Equal(t, []string{"b"}, hint.CreationIDsFor(hintOtherAccount)) + + require.NoError(t, state.ForgetEKSCreationAttempts(clusterName, "eu-north-1", hintOtherAccount)) + + _, err = state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.ErrorIs(t, err, state.ErrEKSCreationHintNotFound, "the last account removes the file") } // TestLoadEKSCreationHintRefusesFileOutsideItsDirectory keeps the hint read confined to the cluster's @@ -203,7 +210,7 @@ func TestLoadEKSCreationHintRefusesFileOutsideItsDirectory(t *testing.T) { outside := filepath.Join(t.TempDir(), "hint.json") data := `{"version":1,"clusterName":"eks-hint-link","region":"eu-north-1",` + - `"accountId":"123456789012","creationIds":["attempt-8324"]}` + `"accounts":[{"accountId":"123456789012","creationIds":["attempt-8324"]}]}` require.NoError(t, os.WriteFile(outside, []byte(data), 0o600)) stateDir := filepath.Join(home, ".ksail", "clusters", clusterName) @@ -225,10 +232,7 @@ func TestHasEKSCreationHint(t *testing.T) { assert.False(t, state.HasEKSCreationHint(clusterName)) - require.NoError( - t, - state.RecordEKSCreationAttempt(clusterName, "eu-north-1", "123456789012", "a"), - ) + require.NoError(t, state.RecordEKSCreationAttempt(clusterName, "eu-north-1", hintAccount, "a")) assert.True(t, state.HasEKSCreationHint(clusterName)) require.NoError(t, state.DeleteEKSCreationHint(clusterName, "eu-north-1")) From e1acde34b2be0b6884677533618ef49fa3562051 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 22:47:53 +0100 Subject: [PATCH 5/6] fix(eks): keep the failed create hint under the lifecycle lock - Keep the attempt when the lookup after a failed create is inconclusive, and still report the lookup error. - Observe and update the hint while holding the cross-process EKS lifecycle lock, so concurrent KSail processes cannot lose an attempt. - Start a new hint only when none exists or its content is not a valid hint; return any other read failure without replacing the file. - Remove an unusable hint when forgetting attempts, so ownership capture does not fail on it. - Refuse hint reads, writes and removals when the cluster state directory resolves outside the clusters root. Part of #8324 Co-Authored-By: Claude Opus 5.5 --- pkg/cli/cmd/cluster/eks_creation_hint_test.go | 69 +++++++++++++++++ pkg/cli/cmd/cluster/eks_failed_create.go | 74 +++++++++++------- pkg/svc/eksidentity/identity_test.go | 39 ++++++++++ pkg/svc/state/eks_creation_hint_state.go | 39 ++++++++-- pkg/svc/state/eks_creation_hint_state_test.go | 76 +++++++++++++++++++ 5 files changed, 263 insertions(+), 34 deletions(-) diff --git a/pkg/cli/cmd/cluster/eks_creation_hint_test.go b/pkg/cli/cmd/cluster/eks_creation_hint_test.go index 97face6356..11c63bc54e 100644 --- a/pkg/cli/cmd/cluster/eks_creation_hint_test.go +++ b/pkg/cli/cmd/cluster/eks_creation_hint_test.go @@ -1,8 +1,11 @@ package cluster_test import ( + "context" + "errors" "fmt" "testing" + "time" ekstypes "github.com/aws/aws-sdk-go-v2/service/eks/types" eksclient "github.com/devantler-tech/ksail/v7/pkg/client/eks" @@ -266,3 +269,69 @@ func TestCreate_EKSAttemptInAnotherAccountKeepsEarlierAttempt(t *testing.T) { require.NoError(t, hintErr, "the other account's attempt is not spent") assert.Equal(t, []string{"attempt-other-account"}, hint.CreationIDsFor("210987654321")) } + +// TestCreate_EKSInconclusiveLookupKeepsAttempt pins a review finding on ksail#8335: when the cluster +// lookup after a failed create errors without saying whether the cluster exists, the attempt is kept +// so a cluster that appears later stays recoverable, and the lookup failure is still reported. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreate_EKSInconclusiveLookupKeepsAttempt(t *testing.T) { + client := &fakeEKSIdentityClient{ + accountID: testEKSAccountID, + describeErr: errEKSIdentityQuery, + } + + output, _, err := runFailingEKSCreate(t, client) + + require.ErrorIs(t, err, errNodegroupCreateQuota) + require.ErrorIs(t, err, errEKSIdentityQuery, "the inconclusive lookup is still reported") + assert.Contains(t, output, "kept this create attempt's identifier") + + hint, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") + require.NoError(t, hintErr) + assert.Equal(t, []string{testEKSCreationID}, hint.CreationIDsFor(testEKSAccountID)) +} + +// TestCreate_EKSFailedCreateWaitsForLifecycleLock pins a review finding on ksail#8335: the failed +// create updates its hint only while it holds the EKS lifecycle lock, so another KSail process acting +// on the same cluster cannot interleave a read-modify-write and lose an attempt. +// +//nolint:paralleltest // uses t.Chdir and mutates shared test hooks +func TestCreate_EKSFailedCreateWaitsForLifecycleLock(t *testing.T) { + client := &fakeEKSIdentityClient{accountID: testEKSAccountID, describeErr: notFoundEKSCluster()} + + locked := make(chan struct{}) + release := make(chan struct{}) + holderDone := make(chan error, 1) + + clearFailedCreateOwnership(t) + t.Cleanup(func() { clearFailedCreateOwnership(t) }) + + go func() { + holderDone <- state.WithEKSLifecycleLock(context.Background(), "st-eks", func() error { + close(locked) + <-release + + _, err := state.LoadEKSCreationHint("st-eks", "eu-west-1") + if !errors.Is(err, state.ErrEKSCreationHintNotFound) { + return fmt.Errorf("hint written while another process held the lock: %w", err) + } + + return nil + }) + }() + + <-locked + + go func() { + time.Sleep(500 * time.Millisecond) + close(release) + }() + + _, _, err := runFailingEKSCreateAttempt(t, client, testEKSCreationID) + require.ErrorIs(t, err, errNodegroupCreateQuota) + require.NoError(t, <-holderDone) + + _, hintErr := state.LoadEKSCreationHint("st-eks", "eu-west-1") + require.NoError(t, hintErr, "the hint is written once the lock is free") +} diff --git a/pkg/cli/cmd/cluster/eks_failed_create.go b/pkg/cli/cmd/cluster/eks_failed_create.go index 780bc8a670..12ea5f18fd 100644 --- a/pkg/cli/cmd/cluster/eks_failed_create.go +++ b/pkg/cli/cmd/cluster/eks_failed_create.go @@ -72,37 +72,57 @@ func retainFailedEKSCreateOwnership( ) } + // The lifecycle lock serializes the hint's read-modify-write with other KSail processes acting on + // this cluster name, so concurrent failed creates or a concurrent delete cannot lose an attempt. + err = state.WithEKSLifecycleLock(ctx, clusterName, func() error { + return settleFailedEKSCreate(ctx, cmd, client, clusterCtx, accountID, clusterName, region) + }) + if err != nil { + return errors.Join(createErr, err) + } + + return createErr +} + +// settleFailedEKSCreate records ownership when the live cluster carries this attempt's tag or that of +// an earlier kept attempt in the same account. When AWS does not report the cluster, or the lookup is +// inconclusive, this attempt's identifier is kept so a cluster that appears later stays recoverable +// (ksail#8324). A same-named cluster with any other tag is not this attempt's and is left alone. +func settleFailedEKSCreate( + ctx context.Context, + cmd *cobra.Command, + client eksidentity.Client, + clusterCtx *localregistry.Context, + accountID, clusterName, region string, +) error { creationID := clusterCtx.EKSConfig.CreationID ownership, err := observeFailedEKSCreate( ctx, client, accountID, clusterName, region, failedCreateCandidateIDs(clusterName, region, accountID, creationID), ) - if errors.Is(err, eksclient.ErrClusterNotFound) { - // AWS does not report the cluster yet. An interrupted create can still bring it into - // existence after KSail exits, so this attempt's identifier is kept as a local hint that a - // later lifecycle command checks against the live cluster's creation tag (ksail#8324). - return keepFailedEKSCreateHint(cmd, clusterName, region, accountID, creationID, createErr) - } - - if errors.Is(err, eksidentity.ErrCreationUnattributed) { - // The cluster with this name is not one this attempt or an earlier kept attempt began - // building (for example it already existed), so it is not this attempt's to record. - return createErr - } - if err == nil { - ownership.AWSOptions = credentials.AWSOptionsWithDefaults( - clusterCtx.ClusterCfg.Spec.Provider.AWS, + switch { + case errors.Is(err, eksidentity.ErrCreationUnattributed): + return nil + case errors.Is(err, eksclient.ErrClusterNotFound): + return keepFailedEKSCreateHint(cmd, clusterName, region, accountID, creationID) + case err != nil: + // The lookup could not tell whether the cluster exists. Keep the attempt, and report why + // ownership was not recorded. + return errors.Join( + fmt.Errorf("record ownership of partially created EKS cluster: %w", err), + keepFailedEKSCreateHint(cmd, clusterName, region, accountID, creationID), ) - err = eksidentity.Persist(clusterName, ownership.Region, ownership) } + ownership.AWSOptions = credentials.AWSOptionsWithDefaults( + clusterCtx.ClusterCfg.Spec.Provider.AWS, + ) + + err = eksidentity.Persist(clusterName, ownership.Region, ownership) if err != nil { - return errors.Join( - createErr, - fmt.Errorf("record ownership of partially created EKS cluster: %w", err), - ) + return fmt.Errorf("record ownership of partially created EKS cluster: %w", err) } notify.Infof( @@ -111,7 +131,7 @@ func retainFailedEKSCreateOwnership( clusterName, ) - return createErr + return nil } // hasEKSCreationAttempt reports whether clusterCtx is an EKS create carrying an attempt identifier and @@ -138,19 +158,15 @@ func failedCreateCandidateIDs(clusterName, region, accountID, creationID string) // keepFailedEKSCreateHint adds this attempt's creation identifier to the hint for a cluster AWS did // not report yet, keyed to the caller's account and the cluster's name and region. Earlier unresolved -// attempts stay in the hint. The hint never authorizes anything by itself. The original create error -// is always returned; a failure to keep the hint is joined to it. +// attempts stay in the hint. The hint never authorizes anything by itself. It returns only a failure +// to keep the hint. func keepFailedEKSCreateHint( cmd *cobra.Command, clusterName, region, accountID, creationID string, - createErr error, ) error { err := state.RecordEKSCreationAttempt(clusterName, region, accountID, creationID) if err != nil { - return errors.Join( - createErr, - fmt.Errorf("keep the failed EKS create's attempt identifier: %w", err), - ) + return fmt.Errorf("keep the failed EKS create's attempt identifier: %w", err) } notify.Infof( @@ -160,7 +176,7 @@ func keepFailedEKSCreateHint( clusterName, ) - return createErr + return nil } // newFailedCreateIdentityClient builds the read-only AWS client for the create's frozen credentials. diff --git a/pkg/svc/eksidentity/identity_test.go b/pkg/svc/eksidentity/identity_test.go index f025c892cd..5c9b45543a 100644 --- a/pkg/svc/eksidentity/identity_test.go +++ b/pkg/svc/eksidentity/identity_test.go @@ -427,3 +427,42 @@ func eksCluster(name, arn string, createdAt time.Time) *ekstypes.Cluster { func identityTime() time.Time { return time.Date(2026, 7, 18, 12, 0, 0, 0, time.UTC) } + +// TestCaptureRemovesUnusableFailedCreateHint pins a review finding on ksail#8335: a malformed hint can +// never be matched, so ownership capture removes it instead of failing after the record was written. +func TestCaptureRemovesUnusableFailedCreateHint(t *testing.T) { + t.Parallel() + + const clusterName = "capture-removes-unusable-hint" + + t.Cleanup(func() { _ = state.DeleteClusterState(clusterName) }) + + home, err := os.UserHomeDir() + require.NoError(t, err) + + hintDir := filepath.Join(home, ".ksail", "clusters", clusterName) + require.NoError(t, os.MkdirAll(hintDir, 0o700)) + require.NoError(t, os.WriteFile( + filepath.Join(hintDir, "eks-creation-hint-eu-north-1.json"), []byte("{"), 0o600, + )) + + client := &fakeClient{ + accountID: "123456789012", + cluster: eksCluster( + clusterName, + "arn:aws:eks:eu-north-1:123456789012:cluster/"+clusterName, + identityTime(), + ), + } + + _, err = eksidentity.Capture( + t.Context(), client, clusterName, "eu-north-1", captureAWSOptions(), + ) + require.NoError(t, err) + + _, err = state.LoadEKSOwnershipState(clusterName, "eu-north-1") + require.NoError(t, err) + + _, err = state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.ErrorIs(t, err, state.ErrEKSCreationHintNotFound) +} diff --git a/pkg/svc/state/eks_creation_hint_state.go b/pkg/svc/state/eks_creation_hint_state.go index 1898ef68fa..dfb4d1ec8d 100644 --- a/pkg/svc/state/eks_creation_hint_state.go +++ b/pkg/svc/state/eks_creation_hint_state.go @@ -64,12 +64,18 @@ func (h *EKSCreationHint) CreationIDsFor(accountID string) []string { } // RecordEKSCreationAttempt adds one unresolved create attempt made in accountID to the target's hint. -// Every attempt kept earlier, for this or any other account, is retained. A hint that cannot be read -// is replaced, since nothing in it can be trusted to match. +// Every attempt kept earlier, for this or any other account, is retained. Only a missing hint, or one +// whose content is not a valid hint (nothing in it can be matched), is started afresh; any other read +// failure is returned without touching the file. Callers serialize it with the EKS lifecycle lock. func RecordEKSCreationAttempt(clusterName, region, accountID, creationID string) error { accountID = strings.TrimSpace(accountID) hint, err := LoadEKSCreationHint(clusterName, region) + if err != nil && !errors.Is(err, ErrEKSCreationHintNotFound) && + !errors.Is(err, ErrInvalidEKSCreationHint) { + return err + } + if err != nil { hint = &EKSCreationHint{ Version: EKSCreationHintVersion, @@ -97,14 +103,19 @@ func RecordEKSCreationAttempt(clusterName, region, accountID, creationID string) // ForgetEKSCreationAttempts removes the attempts kept for accountID once a lifecycle command resolved // them in that account: their cluster was recorded, or AWS confirmed it does not exist. Attempts kept -// for other accounts stay. The hint file is removed when no attempts remain. A missing hint is not an -// error. +// for other accounts stay. The hint file is removed when no attempts remain, and also when its content +// is not a valid hint, since nothing in it can ever be matched. A missing hint is not an error. +// Callers serialize it with the EKS lifecycle lock. func ForgetEKSCreationAttempts(clusterName, region, accountID string) error { hint, err := LoadEKSCreationHint(clusterName, region) if errors.Is(err, ErrEKSCreationHintNotFound) { return nil } + if errors.Is(err, ErrInvalidEKSCreationHint) { + return DeleteEKSCreationHint(clusterName, region) + } + if err != nil { return err } @@ -201,6 +212,11 @@ func HasEKSCreationHint(clusterName string) bool { return false } + exists, err := clusterStateDirExists(dir) + if err != nil || !exists { + return false + } + entries, err := os.ReadDir(dir) if err != nil { return false @@ -293,6 +309,19 @@ func validEKSCreationAttempts(attempts EKSCreationAttempts) bool { }) } +// eksCreationHintPath returns the hint file for one cluster name and region, refusing a cluster state +// directory that resolves anywhere other than its own entry under the clusters root. Reads, writes and +// removals therefore never follow a relocated cluster directory out of the state store. func eksCreationHintPath(clusterName, region string) (string, error) { - return eksRegionScopedStatePath(clusterName, region, eksCreationHintFileNameFormat) + statePath, err := eksRegionScopedStatePath(clusterName, region, eksCreationHintFileNameFormat) + if err != nil { + return "", err + } + + _, err = clusterStateDirExists(filepath.Dir(statePath)) + if err != nil { + return "", err + } + + return statePath, nil } diff --git a/pkg/svc/state/eks_creation_hint_state_test.go b/pkg/svc/state/eks_creation_hint_state_test.go index ef01af5a5c..846fec1351 100644 --- a/pkg/svc/state/eks_creation_hint_state_test.go +++ b/pkg/svc/state/eks_creation_hint_state_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "testing" + "github.com/devantler-tech/ksail/v7/pkg/fsutil" "github.com/devantler-tech/ksail/v7/pkg/svc/state" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -238,3 +239,78 @@ func TestHasEKSCreationHint(t *testing.T) { require.NoError(t, state.DeleteEKSCreationHint(clusterName, "eu-north-1")) assert.False(t, state.HasEKSCreationHint(clusterName)) } + +// TestRecordEKSCreationAttemptKeepsHintItCannotRead pins a review finding on ksail#8335: a hint that +// exists but cannot be read is never replaced, so its attempts are not lost. Only a missing hint or one +// whose content is not a valid hint is started afresh. +func TestRecordEKSCreationAttemptKeepsHintItCannotRead(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + const clusterName = "eks-hint-unreadable" + + outside := filepath.Join(t.TempDir(), "hint.json") + require.NoError(t, os.WriteFile(outside, []byte("kept"), 0o600)) + + stateDir := filepath.Join(home, ".ksail", "clusters", clusterName) + require.NoError(t, os.MkdirAll(stateDir, 0o700)) + + hintPath := filepath.Join(stateDir, "eks-creation-hint-eu-north-1.json") + require.NoError(t, os.Symlink(outside, hintPath)) + + err := state.RecordEKSCreationAttempt(clusterName, "eu-north-1", hintAccount, "a") + require.Error(t, err) + + target, err := os.Readlink(hintPath) + require.NoError(t, err, "the unreadable hint must not be replaced") + assert.Equal(t, outside, target) + + require.NoError(t, os.Remove(hintPath)) + require.NoError(t, os.WriteFile(hintPath, []byte("{"), 0o600)) + + require.NoError(t, state.RecordEKSCreationAttempt(clusterName, "eu-north-1", hintAccount, "a"), + "a hint whose content is not a valid hint is started afresh") + + hint, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.NoError(t, err) + assert.Equal(t, []string{"a"}, hint.CreationIDsFor(hintAccount)) +} + +// TestEKSCreationHintRefusesRelocatedClusterDirectory pins a review finding on ksail#8335: when the +// cluster's state directory resolves outside the clusters root, the hint is neither read, written, +// nor removed there. +func TestEKSCreationHintRefusesRelocatedClusterDirectory(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + const clusterName = "eks-hint-relocated" + + outsideDir := t.TempDir() + outsideHint := filepath.Join(outsideDir, "eks-creation-hint-eu-north-1.json") + data := `{"version":1,"clusterName":"eks-hint-relocated","region":"eu-north-1",` + + `"accounts":[{"accountId":"123456789012","creationIds":["outside"]}]}` + require.NoError(t, os.WriteFile(outsideHint, []byte(data), 0o600)) + + clustersDir := filepath.Join(home, ".ksail", "clusters") + require.NoError(t, os.MkdirAll(clustersDir, 0o700)) + require.NoError(t, os.Symlink(outsideDir, filepath.Join(clustersDir, clusterName))) + + _, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.ErrorIs(t, err, fsutil.ErrPathOutsideBase) + + require.ErrorIs( + t, + state.RecordEKSCreationAttempt(clusterName, "eu-north-1", hintAccount, "a"), + fsutil.ErrPathOutsideBase, + ) + require.ErrorIs( + t, + state.DeleteEKSCreationHint(clusterName, "eu-north-1"), + fsutil.ErrPathOutsideBase, + ) + assert.False(t, state.HasEKSCreationHint(clusterName)) + + got, err := os.ReadFile(outsideHint) //nolint:gosec // test-private path + require.NoError(t, err, "the file outside the state root must survive") + assert.JSONEq(t, data, string(got)) +} From 844ae49f1eef2afc0e3328bdee5cd63bc5c13d64 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 11 Nov 2026 23:04:14 +0100 Subject: [PATCH 6/6] fix(eks): recheck the hint directory after creating it Check the cluster state directory again after creating it, before the hint is written, and release the lock in the lifecycle-lock test only once the failed create has reached the lock boundary. Part of #8324 Co-Authored-By: Claude Opus 5.5 --- pkg/cli/cmd/cluster/eks_creation_hint_test.go | 33 ++++++++++++++----- .../cluster/eks_immutable_identity_test.go | 6 ++++ pkg/svc/state/eks_creation_hint_state.go | 7 ++++ 3 files changed, 37 insertions(+), 9 deletions(-) diff --git a/pkg/cli/cmd/cluster/eks_creation_hint_test.go b/pkg/cli/cmd/cluster/eks_creation_hint_test.go index 11c63bc54e..7e8a69fe0d 100644 --- a/pkg/cli/cmd/cluster/eks_creation_hint_test.go +++ b/pkg/cli/cmd/cluster/eks_creation_hint_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "sync" "testing" "time" @@ -16,6 +17,8 @@ import ( const testEKSAccountID = "123456789012" +var errLockBoundaryNotReached = errors.New("failed create never read the caller account") + // runInterruptedEKSCreate runs a failing create while AWS does not report the cluster yet, the way an // interrupted create looks before the control plane is visible, and returns the shared identity // client so the caller can make the cluster appear afterwards. @@ -294,14 +297,23 @@ func TestCreate_EKSInconclusiveLookupKeepsAttempt(t *testing.T) { // TestCreate_EKSFailedCreateWaitsForLifecycleLock pins a review finding on ksail#8335: the failed // create updates its hint only while it holds the EKS lifecycle lock, so another KSail process acting -// on the same cluster cannot interleave a read-modify-write and lose an attempt. +// on the same cluster cannot interleave a read-modify-write and lose an attempt. The holder releases +// the lock only after the failed create has read the caller account, the last step before it asks +// for the lock, so the create is waiting at the lock boundary rather than still setting up. // //nolint:paralleltest // uses t.Chdir and mutates shared test hooks func TestCreate_EKSFailedCreateWaitsForLifecycleLock(t *testing.T) { - client := &fakeEKSIdentityClient{accountID: testEKSAccountID, describeErr: notFoundEKSCluster()} + reachedLock := make(chan struct{}) + + var signal sync.Once + + client := &fakeEKSIdentityClient{ + accountID: testEKSAccountID, + describeErr: notFoundEKSCluster(), + afterAccount: func() { signal.Do(func() { close(reachedLock) }) }, + } locked := make(chan struct{}) - release := make(chan struct{}) holderDone := make(chan error, 1) clearFailedCreateOwnership(t) @@ -310,7 +322,15 @@ func TestCreate_EKSFailedCreateWaitsForLifecycleLock(t *testing.T) { go func() { holderDone <- state.WithEKSLifecycleLock(context.Background(), "st-eks", func() error { close(locked) - <-release + + select { + case <-reachedLock: + case <-time.After(30 * time.Second): + return errLockBoundaryNotReached + } + + // Give an unguarded create ample time to write the hint while the lock is still held. + time.Sleep(250 * time.Millisecond) _, err := state.LoadEKSCreationHint("st-eks", "eu-west-1") if !errors.Is(err, state.ErrEKSCreationHintNotFound) { @@ -323,11 +343,6 @@ func TestCreate_EKSFailedCreateWaitsForLifecycleLock(t *testing.T) { <-locked - go func() { - time.Sleep(500 * time.Millisecond) - close(release) - }() - _, _, err := runFailingEKSCreateAttempt(t, client, testEKSCreationID) require.ErrorIs(t, err, errNodegroupCreateQuota) require.NoError(t, <-holderDone) diff --git a/pkg/cli/cmd/cluster/eks_immutable_identity_test.go b/pkg/cli/cmd/cluster/eks_immutable_identity_test.go index 6f9c007763..680d8420d2 100644 --- a/pkg/cli/cmd/cluster/eks_immutable_identity_test.go +++ b/pkg/cli/cmd/cluster/eks_immutable_identity_test.go @@ -37,11 +37,17 @@ type fakeEKSIdentityClient struct { // that many fail with errEKSIdentityQuery. accountCalls int accountErrAfter int + // afterAccount, when set, runs after every CallerAccountID call. + afterAccount func() } func (f *fakeEKSIdentityClient) CallerAccountID(_ context.Context) (string, error) { f.accountCalls++ + if f.afterAccount != nil { + defer f.afterAccount() + } + if f.accountErrAfter > 0 && f.accountCalls > f.accountErrAfter { return "", errEKSIdentityQuery } diff --git a/pkg/svc/state/eks_creation_hint_state.go b/pkg/svc/state/eks_creation_hint_state.go index dfb4d1ec8d..378e987157 100644 --- a/pkg/svc/state/eks_creation_hint_state.go +++ b/pkg/svc/state/eks_creation_hint_state.go @@ -149,6 +149,13 @@ func SaveEKSCreationHint(clusterName, region string, hint *EKSCreationHint) erro return fmt.Errorf("failed to create EKS creation hint directory: %w", err) } + // The confinement check before MkdirAll passes trivially for a directory that did not exist yet, + // so the created directory is checked again before anything is written into it. + _, err = clusterStateDirExists(filepath.Dir(statePath)) + if err != nil { + return err + } + data, err := json.MarshalIndent(hint, "", " ") if err != nil { return fmt.Errorf("failed to marshal EKS creation hint: %w", err)