From d1ce815c954ac21c1fa8d524ff5c0a70a684628f Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 12:31:04 +0200 Subject: [PATCH 1/8] fix(eks): refuse an unreadable ownership record instead of treating it as absent ListEKSOwnershipStates skipped a record that could not be read or parsed, so a directory holding only a truncated or mode-000 record reported not-found. confirmConfigMatchesOwnership then let a possibly stale eks.yaml bind unopposed. Report ErrEKSOwnershipStateUnreadable, naming the files, when nothing usable survives, and refuse on it in both binding paths. A usable record beside a damaged one, and legacy records, behave as before. Fixes #6429 Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cli/clusterapi/distconfig.go | 27 +++++++- pkg/cli/clusterapi/local_service_test.go | 44 ++++++++++++ pkg/svc/state/eks_ownership_state.go | 45 ++++++++++--- pkg/svc/state/eks_ownership_state_test.go | 81 +++++++++++++++++++++++ 4 files changed, 184 insertions(+), 13 deletions(-) diff --git a/pkg/cli/clusterapi/distconfig.go b/pkg/cli/clusterapi/distconfig.go index 335554e8fe..60f7790365 100644 --- a/pkg/cli/clusterapi/distconfig.go +++ b/pkg/cli/clusterapi/distconfig.go @@ -312,10 +312,16 @@ func boundEKSRegionFromConfig(name, configPath string) (string, bool, error) { // file's region would aim a destructive action at a cluster nobody named. func confirmConfigMatchesOwnership(name, configPath, configRegion string) error { ownerships, err := state.ListEKSOwnershipStates(name) - if err != nil { + if errors.Is(err, state.ErrEKSOwnershipStateNotFound) { // No usable record: the config is the only binding evidence there is, and it is already // validated above. A cluster created before ownership records existed still works. - return nil //nolint:nilerr // absence of a record is not an error; the config binds instead. + return nil + } + + if err != nil { + // A record that exists but cannot be read is not absence. Accepting the config here would + // let a stale rendered file bind unopposed, which is the redirect this check prevents. + return unreadableOwnershipError(name, err) } if len(ownerships) > 1 { @@ -342,6 +348,19 @@ func confirmConfigMatchesOwnership(name, configPath, configRegion string) error ) } +// unreadableOwnershipError refuses a lifecycle action whose ownership record exists but cannot be +// read or parsed. err names the affected files. +func unreadableOwnershipError(name string, err error) error { + return fmt.Errorf( + "%w: cluster %q has an EKS ownership record KSail cannot read, so it cannot confirm which"+ + " region the cluster was created in; restore read access to the file or remove it and"+ + " run `ksail cluster eks-bind` to record the region again: %w", + api.ErrInvalid, + name, + err, + ) +} + // multiRegionOwnershipError reports same-named clusters recorded in several regions. func multiRegionOwnershipError(name string, ownerships []*state.EKSOwnershipState) error { regions := make([]string, 0, len(ownerships)) @@ -377,6 +396,10 @@ func multiRegionOwnershipError(name string, ownerships []*state.EKSOwnershipStat // this refuses and lists them. func bindFromOwnershipRecord(name string) (*clusterprovisioner.EKSConfig, error) { ownerships, err := state.ListEKSOwnershipStates(name) + if errors.Is(err, state.ErrEKSOwnershipStateUnreadable) { + return nil, unreadableOwnershipError(name, err) + } + if err != nil { return nil, fmt.Errorf( "%w: cluster %q has local KSail state but no eks config and no ownership record to bind"+ diff --git a/pkg/cli/clusterapi/local_service_test.go b/pkg/cli/clusterapi/local_service_test.go index 98261a4a77..2eb593828f 100644 --- a/pkg/cli/clusterapi/local_service_test.go +++ b/pkg/cli/clusterapi/local_service_test.go @@ -2543,6 +2543,50 @@ func TestBoundEKSConfigAcceptsAConfigWithNoOwnershipRecord(t *testing.T) { assert.Equal(t, "eu-north-1", region, "the config alone still binds when no record exists") } +// TestBoundEKSConfigRefusesAConfigBesideATruncatedOwnershipRecord closes the corruption route to the +// stale-config redirect. A record that exists but does not parse used to read as "no record", so the +// config-only path above accepted a rendered file the record might contradict. It must refuse and +// name the damaged file instead. +func TestBoundEKSConfigRefusesAConfigBesideATruncatedOwnershipRecord(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("AWS_REGION", "us-west-2") + + const name = "truncated-record" + + saveEKSClusterSpec(t, name) + writeStateEKSConfig(t, name, "us-west-2") + + recordPath := filepath.Join(home, ".ksail", "clusters", name, "eks-ownership-eu-north-1.json") + require.NoError(t, os.WriteFile(recordPath, []byte(`{"version":1,"region":"eu-no`), 0o600)) + + _, _, err := clusterapi.ExportEKSConfigForCreate(name) + require.ErrorIs(t, err, api.ErrInvalid) + require.ErrorIs(t, err, state.ErrEKSOwnershipStateUnreadable) + assert.Contains(t, err.Error(), recordPath, "the error must name the file that cannot be read") +} + +// TestBoundEKSConfigRefusesToBindFromAnUnreadableOwnershipRecord covers the no-config path: with no +// eks.yaml, binding comes from the record alone, so an unreadable one must say so rather than report +// that no record exists. +func TestBoundEKSConfigRefusesToBindFromAnUnreadableOwnershipRecord(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("AWS_REGION", "us-west-2") + + const name = "unreadable-record-no-config" + + saveEKSClusterSpec(t, name) + + recordPath := filepath.Join(home, ".ksail", "clusters", name, "eks-ownership-eu-north-1.json") + require.NoError(t, os.WriteFile(recordPath, []byte("{"), 0o600)) + + _, _, err := clusterapi.ExportEKSConfigForCreate(name) + require.ErrorIs(t, err, api.ErrInvalid) + require.ErrorIs(t, err, state.ErrEKSOwnershipStateUnreadable) + assert.Contains(t, err.Error(), "cannot read") +} + // TestCreateRefusesANameWhoseEKSCreateStateRemains covers the CREATE half of the region binding. // // The binding that makes delete/start/stop follow the creation region must never steer a create. diff --git a/pkg/svc/state/eks_ownership_state.go b/pkg/svc/state/eks_ownership_state.go index 8fbb3bef85..c7ce7ba5aa 100644 --- a/pkg/svc/state/eks_ownership_state.go +++ b/pkg/svc/state/eks_ownership_state.go @@ -29,7 +29,11 @@ var ( ErrEKSOwnershipStateNotFound = errors.New("EKS ownership state not found") // ErrInvalidEKSOwnershipState reports malformed, incomplete, or internally inconsistent state. ErrInvalidEKSOwnershipState = errors.New("invalid EKS ownership state") - awsAccountIDPattern = regexp.MustCompile(`^[0-9]{12}$`) + // ErrEKSOwnershipStateUnreadable reports ownership records that exist but could not be read or + // parsed, with no usable record beside them. It is deliberately distinct from + // ErrEKSOwnershipStateNotFound: corruption must refuse, never fall back to the absence path. + ErrEKSOwnershipStateUnreadable = errors.New("EKS ownership state present but unreadable") + awsAccountIDPattern = regexp.MustCompile(`^[0-9]{12}$`) ) // EKSOwnershipState binds a KSail-managed EKS target to the AWS account and exact EKS incarnation @@ -127,8 +131,11 @@ func LoadEKSOwnershipState(clusterName, region string) (*EKSOwnershipState, erro // Individually unusable records — unreadable, malformed, failing validation, or predating the // awsOptions schema — are skipped rather than failing the whole listing, so one stale record in an // unrelated region cannot strand a cluster whose target region is recorded correctly. When nothing -// usable survives, the result is indistinguishable from having no record at all, and the caller's -// absence path applies. Selecting a region from the survivors never weakens the ownership check: +// usable survives, the result is absence — unless a record was present but could not be read or +// parsed. That returns ErrEKSOwnershipStateUnreadable, naming the files, because a record that +// exists but cannot be read is evidence that something is wrong, not evidence that none was +// written: treating it as absent would let a stale rendered config bind a cluster unopposed. +// Selecting a region from the survivors never weakens the ownership check: // eksidentity.NewVerifier still loads and strictly validates the selected region's record. func ListEKSOwnershipStates(clusterName string) ([]*EKSOwnershipState, error) { dir, err := clusterStateDir(clusterName) @@ -142,15 +149,29 @@ func ListEKSOwnershipStates(clusterName string) ([]*EKSOwnershipState, error) { } ownerships := make([]*EKSOwnershipState, 0, len(paths)) + unreadable := []string{} for _, path := range paths { - ownership := loadUsableEKSOwnershipRecord(clusterName, path) + ownership, readable := loadUsableEKSOwnershipRecord(clusterName, path) + if !readable { + unreadable = append(unreadable, path) + } + if ownership != nil { ownerships = append(ownerships, ownership) } } if len(ownerships) == 0 { + if len(unreadable) > 0 { + return nil, fmt.Errorf( + "%w: %s: %s", + ErrEKSOwnershipStateUnreadable, + clusterName, + strings.Join(unreadable, ", "), + ) + } + return nil, fmt.Errorf("%w: %s", ErrEKSOwnershipStateNotFound, clusterName) } @@ -163,34 +184,36 @@ func ListEKSOwnershipStates(clusterName string) ([]*EKSOwnershipState, error) { // loadUsableEKSOwnershipRecord returns the record at path, or nil when it cannot be trusted to // contribute a credential mapping. The filename must match the region it claims, so a record cannot -// be read under another region's key. -func loadUsableEKSOwnershipRecord(clusterName, path string) *EKSOwnershipState { +// be read under another region's key. readable is false only when the file could not be read or is +// not valid JSON; a record that parses but fails validation (including one predating the awsOptions +// schema) is readable and simply unusable. +func loadUsableEKSOwnershipRecord(clusterName, path string) (*EKSOwnershipState, bool) { //nolint:gosec // glob is rooted under the validated per-cluster state directory. data, err := os.ReadFile(path) if err != nil { - return nil + return nil, false } var ownership EKSOwnershipState err = json.Unmarshal(data, &ownership) if err != nil { - return nil + return nil, false } region := strings.TrimSpace(ownership.Region) err = validateEKSOwnershipState(clusterName, region, &ownership) if err != nil { - return nil + return nil, true } expectedPath, err := eksOwnershipStatePath(clusterName, region) if err != nil || filepath.Clean(expectedPath) != filepath.Clean(path) { - return nil + return nil, true } - return &ownership + return &ownership, true } func validateEKSOwnershipState(clusterName, region string, ownership *EKSOwnershipState) error { diff --git a/pkg/svc/state/eks_ownership_state_test.go b/pkg/svc/state/eks_ownership_state_test.go index c532c03ea5..1f7a5c421f 100644 --- a/pkg/svc/state/eks_ownership_state_test.go +++ b/pkg/svc/state/eks_ownership_state_test.go @@ -334,3 +334,84 @@ func TestLoadEKSOwnershipStateRejectsInvalidJSON(t *testing.T) { require.NotErrorIs(t, err, state.ErrEKSOwnershipStateNotFound) assert.ErrorContains(t, err, "unmarshal EKS ownership state") } + +// writeRawOwnershipRecord writes arbitrary bytes where an ownership record for region belongs. +func writeRawOwnershipRecord(t *testing.T, clusterName, region string, data []byte) string { + t.Helper() + + home, err := os.UserHomeDir() + require.NoError(t, err) + + dir := filepath.Join(home, ".ksail", "clusters", clusterName) + require.NoError(t, os.MkdirAll(dir, 0o700)) + + path := filepath.Join(dir, "eks-ownership-"+region+".json") + require.NoError(t, os.WriteFile(path, data, 0o600)) + + return path +} + +// TestListEKSOwnershipStatesRefusesATruncatedRecordAsAbsence proves a record that exists but does not +// parse is reported as unreadable, not as absent. Absence licenses callers to bind from a rendered +// config alone, so a truncated record must not reach that path. +func TestListEKSOwnershipStatesRefusesATruncatedRecordAsAbsence(t *testing.T) { + t.Parallel() + + const clusterName = "ownership-list-truncated" + + path := writeRawOwnershipRecord(t, clusterName, "eu-north-1", []byte(`{"version":1,"clusterNa`)) + + _, err := state.ListEKSOwnershipStates(clusterName) + require.ErrorIs(t, err, state.ErrEKSOwnershipStateUnreadable) + require.NotErrorIs(t, err, state.ErrEKSOwnershipStateNotFound) + assert.ErrorContains(t, err, path) +} + +// TestListEKSOwnershipStatesRefusesAnUnreadableRecordAsAbsence covers a record the process cannot +// open at all (mode 000). +func TestListEKSOwnershipStatesRefusesAnUnreadableRecordAsAbsence(t *testing.T) { + t.Parallel() + + if os.Geteuid() == 0 { + t.Skip("root can read a mode-000 file, so this cannot produce a read failure") + } + + const clusterName = "ownership-list-mode-000" + + path := writeRawOwnershipRecord(t, clusterName, "eu-north-1", []byte("{}")) + require.NoError(t, os.Chmod(path, 0o000)) + + t.Cleanup(func() { _ = os.Chmod(path, 0o600) }) + + _, err := state.ListEKSOwnershipStates(clusterName) + require.ErrorIs(t, err, state.ErrEKSOwnershipStateUnreadable) + require.NotErrorIs(t, err, state.ErrEKSOwnershipStateNotFound) + assert.ErrorContains(t, err, path) +} + +// TestListEKSOwnershipStatesKeepsAUsableRecordBesideAnUnreadableOne pins that corruption only changes +// the no-usable-record case. A readable record in its own region still wins, exactly as a legacy +// record beside it is skipped, so one damaged file in an unrelated region cannot strand a cluster. +func TestListEKSOwnershipStatesKeepsAUsableRecordBesideAnUnreadableOne(t *testing.T) { + t.Parallel() + + const clusterName = "ownership-list-truncated-beside-valid" + + valid := &state.EKSOwnershipState{ + Version: state.EKSOwnershipStateVersion, + ClusterName: clusterName, + Region: "eu-north-1", + AccountID: "123456789012", + ClusterARN: "arn:aws:eks:eu-north-1:123456789012:cluster/" + clusterName, + CreatedAt: time.Date(2026, 7, 18, 12, 0, 0, 0, time.UTC), + AWSOptions: canonicalAWSOptions(), + } + require.NoError(t, state.SaveEKSOwnershipState(clusterName, valid.Region, valid)) + + writeRawOwnershipRecord(t, clusterName, "us-west-2", []byte("{")) + + ownerships, err := state.ListEKSOwnershipStates(clusterName) + require.NoError(t, err) + require.Len(t, ownerships, 1) + assert.Equal(t, "eu-north-1", ownerships[0].Region) +} From 3fcbb6cf01b0da82848a76aaee673794c6dfd5ab Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 14:04:29 +0200 Subject: [PATCH 2/8] fix(eks): refuse an unreadable ownership state directory filepath.Glob discards directory read errors, so a state directory the process could not read listed as empty and reported absence. Read it with os.ReadDir and report ErrEKSOwnershipStateUnreadable for any failure other than the directory not existing. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/svc/state/eks_ownership_state.go | 43 +++++++++++++++++++++-- pkg/svc/state/eks_ownership_state_test.go | 32 +++++++++++++++++ 2 files changed, 73 insertions(+), 2 deletions(-) diff --git a/pkg/svc/state/eks_ownership_state.go b/pkg/svc/state/eks_ownership_state.go index c7ce7ba5aa..54bf5cc206 100644 --- a/pkg/svc/state/eks_ownership_state.go +++ b/pkg/svc/state/eks_ownership_state.go @@ -4,6 +4,7 @@ import ( "encoding/json" "errors" "fmt" + "io/fs" "os" "path/filepath" "regexp" @@ -143,9 +144,9 @@ func ListEKSOwnershipStates(clusterName string) ([]*EKSOwnershipState, error) { return nil, err } - paths, err := filepath.Glob(filepath.Join(dir, "eks-ownership-*.json")) + paths, err := eksOwnershipRecordPaths(clusterName, dir) if err != nil { - return nil, fmt.Errorf("list EKS ownership state: %w", err) + return nil, err } ownerships := make([]*EKSOwnershipState, 0, len(paths)) @@ -182,6 +183,44 @@ func ListEKSOwnershipStates(clusterName string) ([]*EKSOwnershipState, error) { return ownerships, nil } +// eksOwnershipRecordPaths lists the ownership record files in a cluster's state directory. +// +// filepath.Glob is not used because it discards directory read errors, so a state directory the +// process cannot read would list as empty and report absence. Only a directory that does not exist +// means no records; any other read failure is ErrEKSOwnershipStateUnreadable. +func eksOwnershipRecordPaths(clusterName, dir string) ([]string, error) { + entries, err := os.ReadDir(dir) + if errors.Is(err, fs.ErrNotExist) { + return nil, nil + } + + if err != nil { + return nil, fmt.Errorf( + "%w: %s: read state directory %s: %w", + ErrEKSOwnershipStateUnreadable, + clusterName, + dir, + err, + ) + } + + pattern := fmt.Sprintf(eksOwnershipStateFileNameFormat, "*") + paths := make([]string, 0, len(entries)) + + for _, entry := range entries { + matched, matchErr := filepath.Match(pattern, entry.Name()) + if matchErr != nil { + return nil, fmt.Errorf("list EKS ownership state: %w", matchErr) + } + + if matched { + paths = append(paths, filepath.Join(dir, entry.Name())) + } + } + + return paths, nil +} + // loadUsableEKSOwnershipRecord returns the record at path, or nil when it cannot be trusted to // contribute a credential mapping. The filename must match the region it claims, so a record cannot // be read under another region's key. readable is false only when the file could not be read or is diff --git a/pkg/svc/state/eks_ownership_state_test.go b/pkg/svc/state/eks_ownership_state_test.go index 1f7a5c421f..94b9b4e26d 100644 --- a/pkg/svc/state/eks_ownership_state_test.go +++ b/pkg/svc/state/eks_ownership_state_test.go @@ -415,3 +415,35 @@ func TestListEKSOwnershipStatesKeepsAUsableRecordBesideAnUnreadableOne(t *testin require.Len(t, ownerships, 1) assert.Equal(t, "eu-north-1", ownerships[0].Region) } + +// TestListEKSOwnershipStatesRefusesAnUnreadableStateDirectory covers the directory itself. A listing +// that cannot read the directory must not look empty, because empty means absence and absence lets a +// rendered config bind alone. +func TestListEKSOwnershipStatesRefusesAnUnreadableStateDirectory(t *testing.T) { + t.Parallel() + + if os.Geteuid() == 0 { + t.Skip("root can read a mode-000 directory, so this cannot produce a read failure") + } + + const clusterName = "ownership-list-unreadable-dir" + + path := writeRawOwnershipRecord(t, clusterName, "eu-north-1", []byte("{}")) + dir := filepath.Dir(path) + require.NoError(t, os.Chmod(dir, 0o000)) + + t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) + + _, err := state.ListEKSOwnershipStates(clusterName) + require.ErrorIs(t, err, state.ErrEKSOwnershipStateUnreadable) + require.NotErrorIs(t, err, state.ErrEKSOwnershipStateNotFound) +} + +// TestListEKSOwnershipStatesReportsAbsenceForAMissingStateDirectory is the control: a cluster with no +// state directory at all has no records, which is genuine absence. +func TestListEKSOwnershipStatesReportsAbsenceForAMissingStateDirectory(t *testing.T) { + t.Parallel() + + _, err := state.ListEKSOwnershipStates("ownership-list-no-state-dir") + require.ErrorIs(t, err, state.ErrEKSOwnershipStateNotFound) +} From e86ffe5305aa2569de8d843f11b8283481c1ef3d Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 15:03:46 +0200 Subject: [PATCH 3/8] test(state): justify the directory-mode restore for gosec Co-Authored-By: Claude Opus 5 (1M context) --- pkg/svc/state/eks_ownership_state_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/pkg/svc/state/eks_ownership_state_test.go b/pkg/svc/state/eks_ownership_state_test.go index 94b9b4e26d..b740b2ecef 100644 --- a/pkg/svc/state/eks_ownership_state_test.go +++ b/pkg/svc/state/eks_ownership_state_test.go @@ -432,7 +432,10 @@ func TestListEKSOwnershipStatesRefusesAnUnreadableStateDirectory(t *testing.T) { dir := filepath.Dir(path) require.NoError(t, os.Chmod(dir, 0o000)) - t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) + t.Cleanup(func() { + //nolint:gosec // 0700 is a directory mode: restores traversal so TempDir cleanup can remove it. + _ = os.Chmod(dir, 0o700) + }) _, err := state.ListEKSOwnershipStates(clusterName) require.ErrorIs(t, err, state.ErrEKSOwnershipStateUnreadable) From 8ced31ee52d6962181c1bc60233698b18544d0bb Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 21:41:26 +0200 Subject: [PATCH 4/8] fix(clusterapi): report an unreadable ownership record instead of not found A cluster reachable only through its ownership record was reported as missing when that record could not be read, hiding the damaged file and its fix. The lifecycle refusal now names the record. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cli/clusterapi/local_service.go | 24 +++++++++++----- pkg/cli/clusterapi/local_service_test.go | 35 ++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/pkg/cli/clusterapi/local_service.go b/pkg/cli/clusterapi/local_service.go index e664e05626..78d8882ba3 100644 --- a/pkg/cli/clusterapi/local_service.go +++ b/pkg/cli/clusterapi/local_service.go @@ -523,14 +523,16 @@ func (s *Service) useDefaultClients() { } // resolveCluster finds the distribution and provider of an existing cluster, checking live -// providers first and then the job store (for clusters still being provisioned). +// providers first and then the job store (for clusters still being provisioned). An ownership record +// that exists but cannot be read is returned as an error rather than as "not found", so the refusal +// names the damaged file instead of claiming the cluster does not exist. func (s *Service) resolveCluster( ctx context.Context, name string, -) (v1alpha1.Distribution, v1alpha1.Provider, bool) { +) (v1alpha1.Distribution, v1alpha1.Provider, bool, error) { live := s.enumerate(ctx) if cluster, ok := live[name]; ok { - return cluster.Distribution, cluster.Provider, true + return cluster.Distribution, cluster.Provider, true, nil } s.mu.Lock() @@ -538,7 +540,7 @@ func (s *Service) resolveCluster( if current, ok := s.jobs[name]; ok { s.mu.Unlock() - return current.distribution, current.provider, true + return current.distribution, current.provider, true, nil } s.mu.Unlock() @@ -555,10 +557,14 @@ func (s *Service) resolveCluster( // account for. _, ownershipErr := state.ListEKSOwnershipStates(name) if ownershipErr == nil { - return v1alpha1.DistributionEKS, v1alpha1.ProviderAWS, true + return v1alpha1.DistributionEKS, v1alpha1.ProviderAWS, true, nil } - return "", "", false + if errors.Is(ownershipErr, state.ErrEKSOwnershipStateUnreadable) { + return "", "", false, fmt.Errorf("%w: %w", api.ErrInvalid, ownershipErr) + } + + return "", "", false, nil } // dockerFactory adapts the Service's provisioner factory to the discovery DockerFactory shape, @@ -580,7 +586,11 @@ func (s *Service) startJob( name string, phase v1alpha1.ClusterPhase, ) (v1alpha1.Spec, error) { - distribution, provider, ok := s.resolveCluster(ctx, name) + distribution, provider, ok, resolveErr := s.resolveCluster(ctx, name) + if resolveErr != nil { + return v1alpha1.Spec{}, resolveErr + } + if !ok { return v1alpha1.Spec{}, fmt.Errorf("%w: %q", api.ErrNotFound, name) } diff --git a/pkg/cli/clusterapi/local_service_test.go b/pkg/cli/clusterapi/local_service_test.go index 2eb593828f..503a5ddda9 100644 --- a/pkg/cli/clusterapi/local_service_test.go +++ b/pkg/cli/clusterapi/local_service_test.go @@ -2429,6 +2429,41 @@ func TestDeleteResolvesAPersistedEKSTargetOutsideTheSelectedRegion(t *testing.T) "a cluster KSail holds an ownership record for is not missing; the selected region is") } +// TestLifecycleReportsAnUnreadableOwnershipRecordInsteadOfNotFound covers a cluster reachable only +// through its ownership record when that record cannot be read. The mutation must still be refused, +// but as the unreadable record — naming the file — rather than as a cluster that does not exist, +// which would hide both the cause and its fix. +func TestLifecycleReportsAnUnreadableOwnershipRecordInsteadOfNotFound(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + const clusterName = "unreadable-other-region" + + service := newTestService(map[v1alpha1.Distribution]*fakeProvisioner{ + v1alpha1.DistributionEKS: {}, + }) + + recordDir := filepath.Join(home, ".ksail", "clusters", clusterName) + require.NoError(t, os.MkdirAll(recordDir, 0o750)) + + recordPath := filepath.Join(recordDir, "eks-ownership-ap-southeast-2.json") + require.NoError(t, os.WriteFile(recordPath, []byte(`{"version":1,"region":"ap-so`), 0o600)) + + ctx := context.Background() + + for action, run := range map[string]func() error{ + "delete": func() error { return service.Delete(ctx, "default", clusterName) }, + "start": func() error { return service.Start(ctx, "default", clusterName) }, + "stop": func() error { return service.Stop(ctx, "default", clusterName) }, + } { + err := run() + require.ErrorIs(t, err, state.ErrEKSOwnershipStateUnreadable, action) + require.ErrorIs(t, err, api.ErrInvalid, action) + require.NotErrorIs(t, err, api.ErrNotFound, action) + assert.Contains(t, err.Error(), recordPath, action) + } +} + // writeStateEKSConfig puts an eks.yaml under ~/.ksail/clusters/ carrying region, standing in // for a file an earlier KSail wrote from whatever region happened to be selected at the time. func writeStateEKSConfig(t *testing.T, name, region string) { From 45038b26906f129ced63c16474b507caf7f66c1a Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Wed, 16 Sep 2026 23:08:59 +0200 Subject: [PATCH 5/8] fix(eks): tell the operator how to repair an unreadable record on lifecycle actions Co-Authored-By: Claude Opus 5 (1M context) --- pkg/cli/clusterapi/local_service.go | 2 +- pkg/cli/clusterapi/local_service_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cli/clusterapi/local_service.go b/pkg/cli/clusterapi/local_service.go index 78d8882ba3..1307043f45 100644 --- a/pkg/cli/clusterapi/local_service.go +++ b/pkg/cli/clusterapi/local_service.go @@ -561,7 +561,7 @@ func (s *Service) resolveCluster( } if errors.Is(ownershipErr, state.ErrEKSOwnershipStateUnreadable) { - return "", "", false, fmt.Errorf("%w: %w", api.ErrInvalid, ownershipErr) + return "", "", false, unreadableOwnershipError(name, ownershipErr) } return "", "", false, nil diff --git a/pkg/cli/clusterapi/local_service_test.go b/pkg/cli/clusterapi/local_service_test.go index 503a5ddda9..8b4fa3406d 100644 --- a/pkg/cli/clusterapi/local_service_test.go +++ b/pkg/cli/clusterapi/local_service_test.go @@ -2461,6 +2461,7 @@ func TestLifecycleReportsAnUnreadableOwnershipRecordInsteadOfNotFound(t *testing require.ErrorIs(t, err, api.ErrInvalid, action) require.NotErrorIs(t, err, api.ErrNotFound, action) assert.Contains(t, err.Error(), recordPath, action) + assert.Contains(t, err.Error(), "ksail cluster eks-bind", action) } } From 9174c719e6fa9e153a1cd84da5f399d2d25bf972 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Thu, 17 Sep 2026 00:37:43 +0200 Subject: [PATCH 6/8] fix(eks): refuse a malformed ownership record instead of treating it as absent A record that parses but fails validation, or sits under another region's filename, is now reported as unreadable. Only a record in the schema that predated awsOptions keeps the absence behaviour. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/svc/state/eks_ownership_state.go | 53 ++++++++++++---- pkg/svc/state/eks_ownership_state_test.go | 74 +++++++++++++++++++++++ 2 files changed, 115 insertions(+), 12 deletions(-) diff --git a/pkg/svc/state/eks_ownership_state.go b/pkg/svc/state/eks_ownership_state.go index 54bf5cc206..4a10ed10db 100644 --- a/pkg/svc/state/eks_ownership_state.go +++ b/pkg/svc/state/eks_ownership_state.go @@ -23,6 +23,9 @@ const ( // eksOwnershipStateFileNameFormat keeps ownership records scoped to the AWS region. The wider // per-cluster state directory remains name-keyed until its dedicated migration (ksail#6224). eksOwnershipStateFileNameFormat = "eks-ownership-%s.json" + // legacyAWSOptionPlaceholder stands in for the awsOptions a pre-awsOptions record lacks, so the + // rest of that record can still be validated. + legacyAWSOptionPlaceholder = "LEGACY_RECORD" ) var ( @@ -30,8 +33,8 @@ var ( ErrEKSOwnershipStateNotFound = errors.New("EKS ownership state not found") // ErrInvalidEKSOwnershipState reports malformed, incomplete, or internally inconsistent state. ErrInvalidEKSOwnershipState = errors.New("invalid EKS ownership state") - // ErrEKSOwnershipStateUnreadable reports ownership records that exist but could not be read or - // parsed, with no usable record beside them. It is deliberately distinct from + // ErrEKSOwnershipStateUnreadable reports ownership records that exist but could not be read, + // parsed, or validated, with no usable record beside them. It is deliberately distinct from // ErrEKSOwnershipStateNotFound: corruption must refuse, never fall back to the absence path. ErrEKSOwnershipStateUnreadable = errors.New("EKS ownership state present but unreadable") awsAccountIDPattern = regexp.MustCompile(`^[0-9]{12}$`) @@ -132,9 +135,10 @@ func LoadEKSOwnershipState(clusterName, region string) (*EKSOwnershipState, erro // Individually unusable records — unreadable, malformed, failing validation, or predating the // awsOptions schema — are skipped rather than failing the whole listing, so one stale record in an // unrelated region cannot strand a cluster whose target region is recorded correctly. When nothing -// usable survives, the result is absence — unless a record was present but could not be read or -// parsed. That returns ErrEKSOwnershipStateUnreadable, naming the files, because a record that -// exists but cannot be read is evidence that something is wrong, not evidence that none was +// usable survives, the result is absence — unless a record was present but could not be read, +// parsed, or validated as anything other than a legacy record. That returns +// ErrEKSOwnershipStateUnreadable, naming the files, because a record that exists but cannot be +// read is evidence that something is wrong, not evidence that none was // written: treating it as absent would let a stale rendered config bind a cluster unopposed. // Selecting a region from the survivors never weakens the ownership check: // eksidentity.NewVerifier still loads and strictly validates the selected region's record. @@ -223,9 +227,9 @@ func eksOwnershipRecordPaths(clusterName, dir string) ([]string, error) { // loadUsableEKSOwnershipRecord returns the record at path, or nil when it cannot be trusted to // contribute a credential mapping. The filename must match the region it claims, so a record cannot -// be read under another region's key. readable is false only when the file could not be read or is -// not valid JSON; a record that parses but fails validation (including one predating the awsOptions -// schema) is readable and simply unusable. +// be read under another region's key. readable is false when the file could not be read, is not +// valid JSON, or parses but is not a valid ownership record. The one exception is a record in the +// schema that predated awsOptions: it is readable and simply unusable, so it keeps the absence path. func loadUsableEKSOwnershipRecord(clusterName, path string) (*EKSOwnershipState, bool) { //nolint:gosec // glob is rooted under the validated per-cluster state directory. data, err := os.ReadFile(path) @@ -242,17 +246,42 @@ func loadUsableEKSOwnershipRecord(clusterName, path string) (*EKSOwnershipState, region := strings.TrimSpace(ownership.Region) + if !isEKSOwnershipRecordAtPath(clusterName, region, path) { + return nil, false + } + err = validateEKSOwnershipState(clusterName, region, &ownership) if err != nil { - return nil, true + return nil, isLegacyEKSOwnershipRecord(clusterName, region, &ownership) } + return &ownership, true +} + +// isEKSOwnershipRecordAtPath reports whether path is the file the record's own region keys to. +func isEKSOwnershipRecordAtPath(clusterName, region, path string) bool { expectedPath, err := eksOwnershipStatePath(clusterName, region) - if err != nil || filepath.Clean(expectedPath) != filepath.Clean(path) { - return nil, true + + return err == nil && filepath.Clean(expectedPath) == filepath.Clean(path) +} + +// isLegacyEKSOwnershipRecord reports whether a record that failed validation is otherwise complete +// and fails only because it predates the awsOptions schema. +func isLegacyEKSOwnershipRecord(clusterName, region string, record *EKSOwnershipState) bool { + if hasCompleteAWSOptions(record.AWSOptions) { + return false } - return &ownership, true + ownership := *record + ownership.AWSOptions = v1alpha1.OptionsAWS{ + ProfileEnvVar: legacyAWSOptionPlaceholder, + RegionEnvVar: legacyAWSOptionPlaceholder, + AccessKeyIDEnvVar: legacyAWSOptionPlaceholder, + SecretAccessKeyEnvVar: legacyAWSOptionPlaceholder, + SessionTokenEnvVar: legacyAWSOptionPlaceholder, + } + + return validateEKSOwnershipState(clusterName, region, &ownership) == nil } func validateEKSOwnershipState(clusterName, region string, ownership *EKSOwnershipState) error { diff --git a/pkg/svc/state/eks_ownership_state_test.go b/pkg/svc/state/eks_ownership_state_test.go index b740b2ecef..aa9b748428 100644 --- a/pkg/svc/state/eks_ownership_state_test.go +++ b/pkg/svc/state/eks_ownership_state_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "os" "path/filepath" + "strings" "testing" "time" @@ -367,6 +368,79 @@ func TestListEKSOwnershipStatesRefusesATruncatedRecordAsAbsence(t *testing.T) { assert.ErrorContains(t, err, path) } +// TestListEKSOwnershipStatesRefusesAMalformedRecordAsAbsence proves a record that parses but is not +// a valid ownership record is reported as unreadable, not as absent. Only the legacy pre-awsOptions +// schema keeps the absence behaviour; any other invalid record is evidence that something is wrong. +func TestListEKSOwnershipStatesRefusesAMalformedRecordAsAbsence(t *testing.T) { + t.Parallel() + + testCases := map[string]func(clusterName string) map[string]any{ + "unsupported version": func(clusterName string) map[string]any { + record := completeOwnershipRecord(clusterName, "eu-north-1") + record["version"] = state.EKSOwnershipStateVersion + 1 + + return record + }, + "missing account id": func(clusterName string) map[string]any { + record := completeOwnershipRecord(clusterName, "eu-north-1") + delete(record, "accountId") + + return record + }, + "region does not match its filename": func(clusterName string) map[string]any { + return completeOwnershipRecord(clusterName, "us-west-2") + }, + } + + for name, build := range testCases { + t.Run(name, func(t *testing.T) { + t.Parallel() + + clusterName := "ownership-list-malformed-" + strings.ReplaceAll(name, " ", "-") + + data, err := json.Marshal(build(clusterName)) + require.NoError(t, err) + + path := writeRawOwnershipRecord(t, clusterName, "eu-north-1", data) + + _, err = state.ListEKSOwnershipStates(clusterName) + require.ErrorIs(t, err, state.ErrEKSOwnershipStateUnreadable) + require.NotErrorIs(t, err, state.ErrEKSOwnershipStateNotFound) + assert.ErrorContains(t, err, path) + }) + } +} + +// TestListEKSOwnershipStatesAcceptsARawCompleteRecord is the control for the malformed cases above: +// the unmodified record they start from lists successfully, so each refusal is caused by its edit. +func TestListEKSOwnershipStatesAcceptsARawCompleteRecord(t *testing.T) { + t.Parallel() + + const clusterName = "ownership-list-raw-complete" + + data, err := json.Marshal(completeOwnershipRecord(clusterName, "eu-north-1")) + require.NoError(t, err) + + writeRawOwnershipRecord(t, clusterName, "eu-north-1", data) + + ownerships, err := state.ListEKSOwnershipStates(clusterName) + require.NoError(t, err) + require.Len(t, ownerships, 1) +} + +// completeOwnershipRecord returns a record in the current schema that validates for region. +func completeOwnershipRecord(clusterName, region string) map[string]any { + return map[string]any{ + "version": state.EKSOwnershipStateVersion, + "clusterName": clusterName, + "region": region, + "accountId": "123456789012", + "clusterArn": "arn:aws:eks:" + region + ":123456789012:cluster/" + clusterName, + "createdAt": time.Date(2026, 7, 18, 12, 0, 0, 0, time.UTC), + "awsOptions": canonicalAWSOptions(), + } +} + // TestListEKSOwnershipStatesRefusesAnUnreadableRecordAsAbsence covers a record the process cannot // open at all (mode 000). func TestListEKSOwnershipStatesRefusesAnUnreadableRecordAsAbsence(t *testing.T) { From d2de20815792b997b4de27f35e956f6027fc9596 Mon Sep 17 00:00:00 2001 From: Nikolai Emil Damm Date: Thu, 17 Sep 2026 03:06:22 +0200 Subject: [PATCH 7/8] fix(eks): treat a present but incomplete awsOptions as an unreadable record Only a record without an awsOptions field predates that schema; an empty or partial awsOptions object is damage and must refuse instead of falling back to the absence path. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/svc/state/eks_ownership_state.go | 22 +++++++++++++++++++++- pkg/svc/state/eks_ownership_state_test.go | 14 ++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/pkg/svc/state/eks_ownership_state.go b/pkg/svc/state/eks_ownership_state.go index 4a10ed10db..bec52028b8 100644 --- a/pkg/svc/state/eks_ownership_state.go +++ b/pkg/svc/state/eks_ownership_state.go @@ -252,12 +252,32 @@ func loadUsableEKSOwnershipRecord(clusterName, path string) (*EKSOwnershipState, err = validateEKSOwnershipState(clusterName, region, &ownership) if err != nil { - return nil, isLegacyEKSOwnershipRecord(clusterName, region, &ownership) + return nil, !hasAWSOptionsField(data) && isLegacyEKSOwnershipRecord(clusterName, region, &ownership) } return &ownership, true } +// hasAWSOptionsField reports whether the raw record carries an awsOptions field at all. Only a record +// without one predates the awsOptions schema; a present but incomplete field is a damaged record. +func hasAWSOptionsField(data []byte) bool { + var fields map[string]json.RawMessage + + err := json.Unmarshal(data, &fields) + if err != nil { + return true + } + + // encoding/json matches struct fields case-insensitively, so any casing populated AWSOptions. + for name := range fields { + if strings.EqualFold(name, "awsOptions") { + return true + } + } + + return false +} + // isEKSOwnershipRecordAtPath reports whether path is the file the record's own region keys to. func isEKSOwnershipRecordAtPath(clusterName, region, path string) bool { expectedPath, err := eksOwnershipStatePath(clusterName, region) diff --git a/pkg/svc/state/eks_ownership_state_test.go b/pkg/svc/state/eks_ownership_state_test.go index aa9b748428..b49c67aac7 100644 --- a/pkg/svc/state/eks_ownership_state_test.go +++ b/pkg/svc/state/eks_ownership_state_test.go @@ -387,6 +387,20 @@ func TestListEKSOwnershipStatesRefusesAMalformedRecordAsAbsence(t *testing.T) { return record }, + "empty aws options": func(clusterName string) map[string]any { + record := completeOwnershipRecord(clusterName, "eu-north-1") + record["awsOptions"] = map[string]any{} + + return record + }, + "partial aws options": func(clusterName string) map[string]any { + record := completeOwnershipRecord(clusterName, "eu-north-1") + options := canonicalAWSOptions() + options.SessionTokenEnvVar = "" + record["awsOptions"] = options + + return record + }, "region does not match its filename": func(clusterName string) map[string]any { return completeOwnershipRecord(clusterName, "us-west-2") }, From ca9c141fc1d18e3aa0f894d09019a51f2094d7bc Mon Sep 17 00:00:00 2001 From: "ksail-bot[bot]" <262010955+ksail-bot[bot]@users.noreply.github.com> Date: Thu, 17 Sep 2026 01:20:48 +0000 Subject: [PATCH 8/8] chore: apply golangci-lint fixes --- pkg/svc/state/eks_ownership_state.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/svc/state/eks_ownership_state.go b/pkg/svc/state/eks_ownership_state.go index bec52028b8..b5e639630f 100644 --- a/pkg/svc/state/eks_ownership_state.go +++ b/pkg/svc/state/eks_ownership_state.go @@ -252,7 +252,8 @@ func loadUsableEKSOwnershipRecord(clusterName, path string) (*EKSOwnershipState, err = validateEKSOwnershipState(clusterName, region, &ownership) if err != nil { - return nil, !hasAWSOptionsField(data) && isLegacyEKSOwnershipRecord(clusterName, region, &ownership) + return nil, !hasAWSOptionsField(data) && + isLegacyEKSOwnershipRecord(clusterName, region, &ownership) } return &ownership, true