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..602585c0fd --- /dev/null +++ b/pkg/cli/cmd/cluster/eks_creation_hint.go @@ -0,0 +1,155 @@ +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 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, + 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 + } + + accountID, creationIDs, err := loadCallerEKSCreationAttempts(ctx, client, clusterName, region) + if err != nil || len(creationIDs) == 0 { + return err + } + + ownership, err := eksidentity.ObserveCreationInAccount( + ctx, + client, + accountID, + clusterName, + region, + eksprovisioner.ClusterCreationTag, + creationIDs, + ) + + switch { + case errors.Is(err, eksclient.ErrClusterNotFound): + return forgetEKSCreationAttempts(clusterName, region, accountID) + 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 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, + 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, + ) + } + + 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 forgetEKSCreationAttempts(clusterName, region, accountID) + } + + if err != nil { + return fmt.Errorf("check the failed create's attempt identifier against AWS: %w", err) + } + + return nil +} + +// 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, +) (string, []string, error) { + hint, err := state.LoadEKSCreationHint(clusterName, region) + if errors.Is(err, state.ErrEKSCreationHintNotFound) { + return "", nil, nil + } + + if err != nil { + return "", nil, fmt.Errorf("load the failed create's attempt identifier: %w", err) + } + + accountID, err := client.CallerAccountID(ctx) + if err != nil { + return "", nil, fmt.Errorf( + "resolve current AWS account for the failed create's attempt identifier: %w", + err, + ) + } + + accountID = strings.TrimSpace(accountID) + + return accountID, hint.CreationIDsFor(accountID), nil +} + +func forgetEKSCreationAttempts(clusterName, region, accountID string) error { + err := state.ForgetEKSCreationAttempts(clusterName, region, accountID) + if err != nil { + 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 new file mode 100644 index 0000000000..7e8a69fe0d --- /dev/null +++ b/pkg/cli/cmd/cluster/eks_creation_hint_test.go @@ -0,0 +1,352 @@ +package cluster_test + +import ( + "context" + "errors" + "fmt" + "sync" + "testing" + "time" + + 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" + +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. +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, []string{testEKSCreationID}, hint.CreationIDsFor(testEKSAccountID)) + + _, 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") + }) + } +} + +// 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.CreationIDsFor(testEKSAccountID)) +} + +// 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.CreationIDsFor(testEKSAccountID), + ) + + 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. +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) +} + +// 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")) +} + +// 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. 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) { + 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{}) + 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) + + 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) { + return fmt.Errorf("hint written while another process held the lock: %w", err) + } + + return nil + }) + }() + + <-locked + + _, _, 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 4fa4e90a5d..12ea5f18fd 100644 --- a/pkg/cli/cmd/cluster/eks_failed_create.go +++ b/pkg/cli/cmd/cluster/eks_failed_create.go @@ -64,36 +64,74 @@ 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. - return createErr - } - - if err == nil { - ownership.AWSOptions = credentials.AWSOptionsWithDefaults( - clusterCtx.ClusterCfg.Spec.Provider.AWS, + client, accountID, err := failedCreateCaller(ctx, clusterCtx, region) + if err != nil { + return errors.Join( + createErr, + fmt.Errorf("record ownership of partially created EKS cluster: %w", err), ) - err = eksidentity.Persist(clusterName, ownership.Region, ownership) } + // 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), + ) + + 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( - createErr, fmt.Errorf("record ownership of partially created EKS cluster: %w", err), + keepFailedEKSCreateHint(cmd, clusterName, region, accountID, creationID), ) } + ownership.AWSOptions = credentials.AWSOptionsWithDefaults( + clusterCtx.ClusterCfg.Spec.Provider.AWS, + ) + + err = eksidentity.Persist(clusterName, ownership.Region, ownership) + if err != nil { + return fmt.Errorf("record ownership of partially created EKS cluster: %w", err) + } + notify.Infof( cmd.ErrOrStderr(), "recorded ownership of partially created EKS cluster %q; run `ksail cluster delete` to remove it", clusterName, ) - return createErr + return nil } // hasEKSCreationAttempt reports whether clusterCtx is an EKS create carrying an attempt identifier and @@ -105,13 +143,48 @@ 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( +// 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 { + hint, err := state.LoadEKSCreationHint(clusterName, region) + if err != nil { + return []string{creationID} + } + + return append([]string{creationID}, hint.CreationIDsFor(accountID)...) +} + +// 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. It returns only a failure +// to keep the hint. +func keepFailedEKSCreateHint( + cmd *cobra.Command, + clusterName, region, accountID, creationID string, +) error { + err := state.RecordEKSCreationAttempt(clusterName, region, accountID, creationID) + if err != nil { + return 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 nil +} + +// newFailedCreateIdentityClient builds the read-only AWS client for the create's frozen credentials. +func newFailedCreateIdentityClient( ctx context.Context, clusterCtx *localregistry.Context, - clusterName, region string, -) (*state.EKSOwnershipState, error) { + region string, +) (eksidentity.Client, error) { eksIdentityClientFactoryState.RLock() factory := eksIdentityClientFactoryState.factory eksIdentityClientFactoryState.RUnlock() @@ -121,13 +194,25 @@ func observeFailedEKSCreate( return nil, fmt.Errorf("create AWS client: %w", err) } - ownership, err := eksidentity.ObserveCreation( + return client, nil +} + +// 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, + accountID, clusterName, region string, + creationIDs []string, +) (*state.EKSOwnershipState, error) { + 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) @@ -135,3 +220,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 +} 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..680d8420d2 100644 --- a/pkg/cli/cmd/cluster/eks_immutable_identity_test.go +++ b/pkg/cli/cmd/cluster/eks_immutable_identity_test.go @@ -33,9 +33,25 @@ 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 + // 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 + } + return f.accountID, f.accountErr } diff --git a/pkg/cli/cmd/cluster/unmanaged_guard.go b/pkg/cli/cmd/cluster/unmanaged_guard.go index d2616ef8bc..db61b95be7 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) @@ -595,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 + } - return ownershipErr == nil + // 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 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 @@ -821,3 +834,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..a0d8237920 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" @@ -99,6 +100,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 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) + } + return nil } @@ -273,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) } - if cluster == nil || cluster.Tags[tagKey] != creationID { + 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) + } + + 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 8d8a314285..5c9b45543a 100644 --- a/pkg/svc/eksidentity/identity_test.go +++ b/pkg/svc/eksidentity/identity_test.go @@ -110,6 +110,42 @@ func TestCaptureClearsStaleNodegroupCapacityState(t *testing.T) { require.ErrorIs(t, err, state.ErrEKSNodegroupStateNotFound) } +// TestCaptureSpendsFailedCreateHint pins ksail#8324: once an ownership record exists, a failed +// create's attempts kept for that account are spent; another account's attempts stay. +func TestCaptureSpendsFailedCreateHint(t *testing.T) { + t.Parallel() + + const clusterName = "capture-spends-creation-hint" + + t.Cleanup(func() { _ = state.DeleteClusterState(clusterName) }) + + 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", + 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) + + 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) { t.Parallel() @@ -391,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 new file mode 100644 index 0000000000..378e987157 --- /dev/null +++ b/pkg/svc/state/eks_creation_hint_state.go @@ -0,0 +1,334 @@ +package state + +import ( + "encoding/json" + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "slices" + "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 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). 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"` + 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"` +} + +// 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. 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, + ClusterName: strings.TrimSpace(clusterName), + Region: strings.TrimSpace(region), + } + } + + 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, 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 + } + + 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) +} + +// 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) + } + + // 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) + } + + 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. 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 + } + + data, err := fsutil.ReadFileSafe(filepath.Dir(statePath), statePath) + if err != nil { + if isMissingStateFile(statePath, 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 +} + +// 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 + } + + exists, err := clusterStateDirExists(dir) + if err != nil || !exists { + 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 { + 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 +} + +// 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) + } + + 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) || + 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) == "" + }) +} + +// 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) { + 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 new file mode 100644 index 0000000000..846fec1351 --- /dev/null +++ b/pkg/svc/state/eks_creation_hint_state_test.go @@ -0,0 +1,316 @@ +package state_test + +import ( + "fmt" + "os" + "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" +) + +const ( + hintAccount = "123456789012" + hintOtherAccount = "210987654321" +) + +func validEKSCreationHint(clusterName, region string) *state.EKSCreationHint { + return &state.EKSCreationHint{ + Version: state.EKSCreationHintVersion, + ClusterName: clusterName, + Region: region, + Accounts: []state.EKSCreationAttempts{ + {AccountID: hintAccount, CreationIDs: []string{"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) + 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", + ) + 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: "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.Accounts[0].CreationIDs = []string{" "} }, + }, + { + name: "no creation ids", + mutate: func(h *state.EKSCreationHint) { h.Accounts[0].CreationIDs = nil }, + }, + { + name: "duplicate account", + mutate: func(h *state.EKSCreationHint) { + h.Accounts = append(h.Accounts, h.Accounts[0]) + }, + }, + } + + 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) +} + +// 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-retries" + attempts = 40 + ) + + want := make([]string, 0, attempts) + + for attempt := range attempts { + id := fmt.Sprintf("attempt-%d", attempt) + want = append(want, id) + + require.NoError( + t, + state.RecordEKSCreationAttempt(clusterName, "eu-north-1", hintAccount, id), + ) + + 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", hintAccount, "attempt-0"), + ) + + want = append(want[1:], "attempt-0") + + hint, err := state.LoadEKSCreationHint(clusterName, "eu-north-1") + require.NoError(t, err) + assert.Equal(t, want, hint.CreationIDsFor(hintAccount), "every attempt stays, newest last") + assert.Equal( + t, + []string{"other-account-attempt"}, + hint.CreationIDsFor(hintOtherAccount), + "another account's attempt stays beside them", + ) +} + +func TestForgetEKSCreationAttemptsRemovesOnlyThatAccount(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + const clusterName = "eks-hint-forget" + + require.NoError(t, state.ForgetEKSCreationAttempts(clusterName, "eu-north-1", hintAccount), + "a missing hint is fine") + + require.NoError(t, state.RecordEKSCreationAttempt(clusterName, "eu-north-1", hintAccount, "a")) + require.NoError( + t, + 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 +// 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",` + + `"accounts":[{"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", hintAccount, "a")) + assert.True(t, state.HasEKSCreationHint(clusterName)) + + 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)) +}