mirror of
https://github.com/mulgadc/spinifex.git
synced 2026-10-15 09:01:31 +00:00
fix(iam): revision-guard DeleteInstanceProfile and skip dangling roles in ListInstanceProfiles
DeleteInstanceProfile now deletes via kvutil.DeleteIf at the revision it checked, so a role attached after the RoleName check sends the delete back through the DeleteConflict guard instead of removing the profile. ListInstanceProfiles logs and skips a profile whose attached role no longer exists rather than failing the whole account listing.
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
// ============================================================================
|
||||
|
||||
Reference in New Issue
Block a user