Skip to content

🐛 Prevent access entries delete+recreate cycle on EKS reconciliation #6007

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
raykrueger wants to merge 1 commit into
base: main
Choose a base branch
from
+230 −25
Open

🐛 Prevent access entries delete+recreate cycle on EKS reconciliation #6007

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 32 additions & 2 deletions pkg/cloud/services/eks/accessentry.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,13 +167,19 @@ func (s *Service) updateAccessEntry(ctx context.Context, accessEntry ekscontrolp
return errors.Wrapf(err, "failed to describe access entry for principal %s", accessEntry.PrincipalARN)
}

// Use the actual type from AWS when not specified in spec (defaults to STANDARD on EKS)
desiredType := accessEntry.Type
if desiredType == "" {
desiredType = ekscontrolplanev1.AccessEntryTypeStandard
}

// EKS requires recreate when changing type or removing username
existingUsername := ""
if describeOutput.AccessEntry.Username != nil {
existingUsername = *describeOutput.AccessEntry.Username
}

if *accessEntry.Type.APIValue() != *describeOutput.AccessEntry.Type || accessEntry.Username != existingUsername {
if *desiredType.APIValue() != *describeOutput.AccessEntry.Type || accessEntry.Username != existingUsername {
if err = s.deleteAccessEntry(ctx, accessEntry.PrincipalARN); err != nil {
return errors.Wrapf(err, "failed to delete access entry for principal %s during recreation", accessEntry.PrincipalARN)
}
Expand Down Expand Up @@ -220,7 +226,12 @@ func (s *Service) deleteAccessEntry(ctx context.Context, principalArn string) er
}

func (s *Service) reconcileAccessPolicies(ctx context.Context, accessEntry ekscontrolplanev1.AccessEntry) error {
if accessEntry.Type == ekscontrolplanev1.AccessEntryTypeEC2Linux || accessEntry.Type == ekscontrolplanev1.AccessEntryTypeEC2Windows {
// Normalize type: empty means STANDARD, which needs policy reconciliation
entryType := accessEntry.Type
if entryType == "" {
entryType = ekscontrolplanev1.AccessEntryTypeStandard
}
if entryType == ekscontrolplanev1.AccessEntryTypeEC2Linux || entryType == ekscontrolplanev1.AccessEntryTypeEC2Windows {
s.scope.Info("Skipping access policy reconciliation for EC2 access type", "principalARN", accessEntry.PrincipalARN)
return nil
}
Expand All @@ -233,6 +244,12 @@ func (s *Service) reconcileAccessPolicies(ctx context.Context, accessEntry eksco
clusterName := s.scope.KubernetesClusterName()

for _, policy := range accessEntry.AccessPolicies {
existing, alreadyAssociated := existingPolicies[policy.PolicyARN]
if alreadyAssociated && s.policyScopeMatches(policy.AccessScope, existing.AccessScope) {
delete(existingPolicies, policy.PolicyARN)
continue
}

input := &eks.AssociateAccessPolicyInput{
ClusterName: &clusterName,
PrincipalArn: &accessEntry.PrincipalARN,
Expand Down Expand Up @@ -266,6 +283,19 @@ func (s *Service) reconcileAccessPolicies(ctx context.Context, accessEntry eksco
return nil
}

func (s *Service) policyScopeMatches(desired ekscontrolplanev1.AccessScope, existing *ekstypes.AccessScope) bool {
if existing == nil {
return false
}
if string(desired.Type) != string(existing.Type) {
return false
}
if desired.Type == ekscontrolplanev1.AccessScopeTypeNamespace {
return slices.Equal(desired.Namespaces, existing.Namespaces)
}
return true
}

func (s *Service) getExistingAccessPolicies(ctx context.Context, principalARN string) (map[string]ekstypes.AssociatedAccessPolicy, error) {
existingPolicies := map[string]ekstypes.AssociatedAccessPolicy{}
var nextToken *string
Expand Down
221 changes: 198 additions & 23 deletions pkg/cloud/services/eks/accessentry_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,7 @@ func TestReconcileAccessEntries(t *testing.T) {

m.UpdateAccessEntry(gomock.Any(), gomock.Any()).Return(&eks.UpdateAccessEntryOutput{}, nil)

// Policy already matches, should NOT re-associate
m.ListAssociatedAccessPolicies(gomock.Any(), gomock.Any()).Return(&eks.ListAssociatedAccessPoliciesOutput{
AssociatedAccessPolicies: []ekstypes.AssociatedAccessPolicy{
{
Expand All @@ -170,15 +171,137 @@ func TestReconcileAccessEntries(t *testing.T) {
},
},
}, nil)
},
expectError: false,
},
{
name: "no drift - skip reconciliation when entry matches",
accessEntries: []ekscontrolplanev1.AccessEntry{
{
PrincipalARN: principalARN,
Type: ekscontrolplanev1.AccessEntryTypeStandard,
Username: "admin",
AccessPolicies: []ekscontrolplanev1.AccessPolicyReference{
{
PolicyARN: policyARN,
AccessScope: ekscontrolplanev1.AccessScope{
Type: ekscontrolplanev1.AccessScopeTypeCluster,
},
},
},
},
},
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
m.ListAccessEntries(gomock.Any(), gomock.Any()).Return(&eks.ListAccessEntriesOutput{
AccessEntries: []string{principalARN},
}, nil)

m.AssociateAccessPolicy(gomock.Any(), &eks.AssociateAccessPolicyInput{
// First DescribeAccessEntry in getManagedAccessEntries
m.DescribeAccessEntry(gomock.Any(), &eks.DescribeAccessEntryInput{
ClusterName: aws.String(clusterName),
PrincipalArn: aws.String(principalARN),
PolicyArn: aws.String(policyARN),
AccessScope: &ekstypes.AccessScope{
Type: ekstypes.AccessScopeTypeCluster,
}).Return(&eks.DescribeAccessEntryOutput{
AccessEntry: &ekstypes.AccessEntry{
PrincipalArn: aws.String(principalARN),
Username: aws.String("admin"),
Type: ekscontrolplanev1.AccessEntryTypeStandard.APIValue(),
Tags: map[string]string{
"kubernetes.io/cluster/test-cluster": "owned",
},
},
}).Return(&eks.AssociateAccessPolicyOutput{}, nil)
}, nil)

// Second DescribeAccessEntry in updateAccessEntry
m.DescribeAccessEntry(gomock.Any(), &eks.DescribeAccessEntryInput{
ClusterName: aws.String(clusterName),
PrincipalArn: aws.String(principalARN),
}).Return(&eks.DescribeAccessEntryOutput{
AccessEntry: &ekstypes.AccessEntry{
PrincipalArn: aws.String(principalARN),
Username: aws.String("admin"),
Type: ekscontrolplanev1.AccessEntryTypeStandard.APIValue(),
Tags: map[string]string{
"kubernetes.io/cluster/test-cluster": "owned",
},
},
}, nil)

// Policy already matches, no AssociateAccessPolicy call
m.ListAssociatedAccessPolicies(gomock.Any(), gomock.Any()).Return(&eks.ListAssociatedAccessPoliciesOutput{
AssociatedAccessPolicies: []ekstypes.AssociatedAccessPolicy{
{
PolicyArn: aws.String(policyARN),
AccessScope: &ekstypes.AccessScope{
Type: ekstypes.AccessScopeTypeCluster,
},
},
},
}, nil)
},
expectError: false,
},
{
name: "no drift with empty type - should treat as STANDARD",
accessEntries: []ekscontrolplanev1.AccessEntry{
{
PrincipalARN: principalARN,
Username: "admin",
AccessPolicies: []ekscontrolplanev1.AccessPolicyReference{
{
PolicyARN: policyARN,
AccessScope: ekscontrolplanev1.AccessScope{
Type: ekscontrolplanev1.AccessScopeTypeCluster,
},
},
},
},
},
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
m.ListAccessEntries(gomock.Any(), gomock.Any()).Return(&eks.ListAccessEntriesOutput{
AccessEntries: []string{principalARN},
}, nil)

// First DescribeAccessEntry in getManagedAccessEntries
m.DescribeAccessEntry(gomock.Any(), &eks.DescribeAccessEntryInput{
ClusterName: aws.String(clusterName),
PrincipalArn: aws.String(principalARN),
}).Return(&eks.DescribeAccessEntryOutput{
AccessEntry: &ekstypes.AccessEntry{
PrincipalArn: aws.String(principalARN),
Username: aws.String("admin"),
Type: ekscontrolplanev1.AccessEntryTypeStandard.APIValue(),
Tags: map[string]string{
"kubernetes.io/cluster/test-cluster": "owned",
},
},
}, nil)

// Second DescribeAccessEntry in updateAccessEntry
m.DescribeAccessEntry(gomock.Any(), &eks.DescribeAccessEntryInput{
ClusterName: aws.String(clusterName),
PrincipalArn: aws.String(principalARN),
}).Return(&eks.DescribeAccessEntryOutput{
AccessEntry: &ekstypes.AccessEntry{
PrincipalArn: aws.String(principalARN),
Username: aws.String("admin"),
Type: ekscontrolplanev1.AccessEntryTypeStandard.APIValue(),
Tags: map[string]string{
"kubernetes.io/cluster/test-cluster": "owned",
},
},
}, nil)

// Policy already matches, no AssociateAccessPolicy call
m.ListAssociatedAccessPolicies(gomock.Any(), gomock.Any()).Return(&eks.ListAssociatedAccessPoliciesOutput{
AssociatedAccessPolicies: []ekstypes.AssociatedAccessPolicy{
{
PolicyArn: aws.String(policyARN),
AccessScope: &ekstypes.AccessScope{
Type: ekstypes.AccessScopeTypeCluster,
},
},
},
}, nil)
},
expectError: false,
},
Expand Down Expand Up @@ -258,15 +381,7 @@ func TestReconcileAccessEntries(t *testing.T) {
},
}, nil)

m.AssociateAccessPolicy(gomock.Any(), &eks.AssociateAccessPolicyInput{
ClusterName: aws.String(clusterName),
PrincipalArn: aws.String(principalARN),
PolicyArn: aws.String(policyARN),
AccessScope: &ekstypes.AccessScope{
Type: ekstypes.AccessScopeTypeCluster,
},
}).Return(&eks.AssociateAccessPolicyOutput{}, nil)

// Policy already matches, should NOT re-associate
m.DeleteAccessEntry(gomock.Any(), &eks.DeleteAccessEntryInput{
ClusterName: aws.String(clusterName),
PrincipalArn: aws.String(secondPrincipalARN),
Expand Down Expand Up @@ -347,15 +462,7 @@ func TestReconcileAccessEntries(t *testing.T) {
},
},
}, nil)

m.AssociateAccessPolicy(gomock.Any(), &eks.AssociateAccessPolicyInput{
ClusterName: aws.String(clusterName),
PrincipalArn: aws.String(principalARN),
PolicyArn: aws.String(policyARN),
AccessScope: &ekstypes.AccessScope{
Type: ekstypes.AccessScopeTypeCluster,
},
}).Return(&eks.AssociateAccessPolicyOutput{}, nil)
// Policy already matches, should NOT re-associate
},
expectError: false,
},
Expand Down Expand Up @@ -529,6 +636,74 @@ func TestReconcileAccessPolicies(t *testing.T) {
},
expectError: false,
},
{
name: "policy already matches - skip association",
accessEntry: ekscontrolplanev1.AccessEntry{
PrincipalARN: principalARN,
Type: ekscontrolplanev1.AccessEntryTypeStandard,
AccessPolicies: []ekscontrolplanev1.AccessPolicyReference{
{
PolicyARN: policyARN,
AccessScope: ekscontrolplanev1.AccessScope{
Type: ekscontrolplanev1.AccessScopeTypeCluster,
},
},
},
},
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
m.ListAssociatedAccessPolicies(gomock.Any(), gomock.Any()).Return(&eks.ListAssociatedAccessPoliciesOutput{
AssociatedAccessPolicies: []ekstypes.AssociatedAccessPolicy{
{
PolicyArn: aws.String(policyARN),
AccessScope: &ekstypes.AccessScope{
Type: ekstypes.AccessScopeTypeCluster,
},
},
},
}, nil)
// Should NOT call AssociateAccessPolicy when policy already matches
},
expectError: false,
},
{
name: "policy scope changed - re-associate",
accessEntry: ekscontrolplanev1.AccessEntry{
PrincipalARN: principalARN,
Type: ekscontrolplanev1.AccessEntryTypeStandard,
AccessPolicies: []ekscontrolplanev1.AccessPolicyReference{
{
PolicyARN: policyARN,
AccessScope: ekscontrolplanev1.AccessScope{
Type: ekscontrolplanev1.AccessScopeTypeNamespace,
Namespaces: []string{"kube-system"},
},
},
},
},
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
m.ListAssociatedAccessPolicies(gomock.Any(), gomock.Any()).Return(&eks.ListAssociatedAccessPoliciesOutput{
AssociatedAccessPolicies: []ekstypes.AssociatedAccessPolicy{
{
PolicyArn: aws.String(policyARN),
AccessScope: &ekstypes.AccessScope{
Type: ekstypes.AccessScopeTypeCluster,
},
},
},
}, nil)

m.AssociateAccessPolicy(gomock.Any(), &eks.AssociateAccessPolicyInput{
ClusterName: aws.String(clusterName),
PrincipalArn: aws.String(principalARN),
PolicyArn: aws.String(policyARN),
AccessScope: &ekstypes.AccessScope{
Type: ekstypes.AccessScopeTypeNamespace,
Namespaces: []string{"kube-system"},
},
}).Return(&eks.AssociateAccessPolicyOutput{}, nil)
},
expectError: false,
},
}

for _, tc := range tests {
Expand Down