From bf223dacf383814e87262ec6a0dac125f28566ae Mon Sep 17 00:00:00 2001 From: Ray Krueger Date: Fri, 8 May 2026 17:02:08 -0500 Subject: [PATCH] fix: prevent access entries delete+recreate cycle on EKS reconciliation When accessEntries with an unspecified type field are defined in AWSManagedControlPlane, the reconciler detects drift on every cycle because an empty Type is compared as "" against "STANDARD" (the EKS default) from the AWS API. This triggers a continuous delete-and-recreate loop every ~7s, with each deletion removing the access policy association and the subsequent recreation racing against the next reconcile before the policy can be re-associated. Normalize empty Type to AccessEntryTypeStandard before comparison in updateAccessEntry. Also skip re-associating access policies that already match the desired scope to avoid unnecessary API calls. Signed-off-by: Ray Krueger --- pkg/cloud/services/eks/accessentry.go | 34 +++- pkg/cloud/services/eks/accessentry_test.go | 221 ++++++++++++++++++--- 2 files changed, 230 insertions(+), 25 deletions(-) diff --git a/pkg/cloud/services/eks/accessentry.go b/pkg/cloud/services/eks/accessentry.go index e2add8651f..8a405a1b7a 100644 --- a/pkg/cloud/services/eks/accessentry.go +++ b/pkg/cloud/services/eks/accessentry.go @@ -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) } @@ -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 } @@ -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, @@ -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 diff --git a/pkg/cloud/services/eks/accessentry_test.go b/pkg/cloud/services/eks/accessentry_test.go index 4e00f572ec..6bab5090e4 100644 --- a/pkg/cloud/services/eks/accessentry_test.go +++ b/pkg/cloud/services/eks/accessentry_test.go @@ -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{ { @@ -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, }, @@ -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), @@ -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, }, @@ -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 {