diff --git a/spinifex/handlers/iam/instance_profiles.go b/spinifex/handlers/iam/instance_profiles.go index 6ba739625..d66300c5d 100644 --- a/spinifex/handlers/iam/instance_profiles.go +++ b/spinifex/handlers/iam/instance_profiles.go @@ -148,6 +148,12 @@ func (s *IAMServiceImpl) ListInstanceProfiles(accountID string, input *iam.ListI sdkProfile, err := s.listedProfileToSDK(ctx, accountID, &profile) if err != nil { + // One dangling role reference must not hide every other profile in the account. + if code, ok := awserrors.ResolveErrorCode(err); ok && code == awserrors.ErrorIAMNoSuchEntity { + slog.Warn("ListInstanceProfiles: skipping profile whose attached role no longer exists", + "key", key, "roleName", profile.RoleName) + continue + } return nil, err } profiles = append(profiles, sdkProfile) @@ -163,19 +169,18 @@ func (s *IAMServiceImpl) DeleteInstanceProfile(accountID string, input *iam.Dele ctx := context.Background() profileName := *input.InstanceProfileName - profile, err := s.getInstanceProfile(ctx, accountID, profileName) + // Revision-guarded, so a role attached after the check sends the delete back through it. + _, err := kvutil.DeleteIf(ctx, s.instanceProfilesBucket, accountID+"."+profileName, s.instanceProfileCASConfig(accountID, profileName), + func(p *InstanceProfile) error { + if p.RoleName != "" { + return errors.New(awserrors.ErrorIAMDeleteConflict) + } + return nil + }) if err != nil { return nil, err } - if profile.RoleName != "" { - return nil, errors.New(awserrors.ErrorIAMDeleteConflict) - } - - if err := s.instanceProfilesBucket.Delete(ctx, accountID+"."+profileName); err != nil { - return nil, fmt.Errorf("delete instance profile: %w", err) - } - slog.Info("IAM instance profile deleted", "accountID", accountID, "instanceProfileName", profileName) return &iam.DeleteInstanceProfileOutput{}, nil } @@ -373,7 +378,12 @@ func (s *IAMServiceImpl) getInstanceProfile(ctx context.Context, accountID, prof // re-reading and re-running mutate when a concurrent writer wins the race. // mutate reports whether it changed the record; a false return commits nothing. func (s *IAMServiceImpl) updateInstanceProfileCAS(ctx context.Context, accountID, profileName string, mutate func(*InstanceProfile) (bool, error)) error { - _, err := kvutil.Update(ctx, s.instanceProfilesBucket, accountID+"."+profileName, kvutil.CASConfig{ + _, err := kvutil.Update(ctx, s.instanceProfilesBucket, accountID+"."+profileName, s.instanceProfileCASConfig(accountID, profileName), mutate) + return err +} + +func (s *IAMServiceImpl) instanceProfileCASConfig(accountID, profileName string) kvutil.CASConfig { + return kvutil.CASConfig{ Attempts: instanceProfileCASMaxRetries, NotFound: errors.New(awserrors.ErrorIAMNoSuchEntity), Exhausted: func(string, int) error { @@ -381,8 +391,7 @@ func (s *IAMServiceImpl) updateInstanceProfileCAS(ctx context.Context, accountID "accountID", accountID, "instanceProfileName", profileName, "attempts", instanceProfileCASMaxRetries) return errors.New(awserrors.ErrorServerInternal) }, - }, mutate) - return err + } } // profileToSDK converts the internal InstanceProfile to the AWS SDK shape. diff --git a/spinifex/handlers/iam/instance_profiles_test.go b/spinifex/handlers/iam/instance_profiles_test.go index f93999bea..a82f36fc0 100644 --- a/spinifex/handlers/iam/instance_profiles_test.go +++ b/spinifex/handlers/iam/instance_profiles_test.go @@ -1,6 +1,7 @@ package handlers_iam import ( + "context" "fmt" "strings" "sync" @@ -9,6 +10,7 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/iam" "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/nats-io/nats.go/jetstream" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -162,6 +164,28 @@ func TestListInstanceProfiles(t *testing.T) { assert.True(t, names["profile3"]) } +// TestListInstanceProfiles_DanglingRoleSkipped pins that one profile naming a +// role that no longer exists is left out rather than failing the whole listing. +func TestListInstanceProfiles_DanglingRoleSkipped(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + createTestRole(t, svc, "gone-role") + createTestInstanceProfile(t, svc, "dangling-profile") + createTestInstanceProfile(t, svc, "healthy-profile") + _, err := svc.AddRoleToInstanceProfile(testAccountID, &iam.AddRoleToInstanceProfileInput{ + InstanceProfileName: aws.String("dangling-profile"), + RoleName: aws.String("gone-role"), + }) + require.NoError(t, err) + // DeleteRole refuses while the role is attached, so remove the record directly. + require.NoError(t, svc.rolesBucket.Delete(t.Context(), testAccountID+".gone-role")) + + out, err := svc.ListInstanceProfiles(testAccountID, &iam.ListInstanceProfilesInput{}) + require.NoError(t, err) + require.Len(t, out.InstanceProfiles, 1) + assert.Equal(t, "healthy-profile", *out.InstanceProfiles[0].InstanceProfileName) +} + func TestListInstanceProfiles_Empty(t *testing.T) { t.Parallel() svc := setupTestIAMService(t) @@ -242,6 +266,55 @@ func TestDeleteInstanceProfile_WithRoleAttached(t *testing.T) { assert.Contains(t, err.Error(), awserrors.ErrorIAMDeleteConflict) } +// attachBeforeDelete lands a role attach between DeleteInstanceProfile's read +// and its delete, the window a concurrent AddRoleToInstanceProfile can win. +type attachBeforeDelete struct { + jetstream.KeyValue + + once sync.Once + attach func() error + err error +} + +func (a *attachBeforeDelete) Delete(ctx context.Context, key string, opts ...jetstream.KVDeleteOpt) error { + a.once.Do(func() { a.err = a.attach() }) + return a.KeyValue.Delete(ctx, key, opts...) +} + +// TestDeleteInstanceProfile_RoleAttachedAfterCheck pins the DeleteConflict +// guard under concurrency: an attach that reported success must not have its +// profile deleted out from under it. +func TestDeleteInstanceProfile_RoleAttachedAfterCheck(t *testing.T) { + t.Parallel() + svc := setupTestIAMService(t) + createTestRole(t, svc, "late-role") + createTestInstanceProfile(t, svc, "late-profile") + + bucket := &attachBeforeDelete{KeyValue: svc.instanceProfilesBucket} + bucket.attach = func() error { + _, err := svc.AddRoleToInstanceProfile(testAccountID, &iam.AddRoleToInstanceProfileInput{ + InstanceProfileName: aws.String("late-profile"), + RoleName: aws.String("late-role"), + }) + return err + } + svc.instanceProfilesBucket = bucket + + _, err := svc.DeleteInstanceProfile(testAccountID, &iam.DeleteInstanceProfileInput{ + InstanceProfileName: aws.String("late-profile"), + }) + require.NoError(t, bucket.err, "attach") + require.Error(t, err) + assert.Contains(t, err.Error(), awserrors.ErrorIAMDeleteConflict) + + out, err := svc.GetInstanceProfile(testAccountID, &iam.GetInstanceProfileInput{ + InstanceProfileName: aws.String("late-profile"), + }) + require.NoError(t, err, "the profile must survive an attach that reported success") + require.Len(t, out.InstanceProfile.Roles, 1) + assert.Equal(t, "late-role", *out.InstanceProfile.Roles[0].RoleName) +} + // ============================================================================ // InstanceProfile ↔ Role Binding Tests // ============================================================================