Skip to content

Commit bf223da

Browse files
raykruegerraykrueger
committed
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 <raykrueger@gmail.com>
1 parent 895ba50 commit bf223da

2 files changed

Lines changed: 230 additions & 25 deletions

File tree

‎pkg/cloud/services/eks/accessentry.go‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -167,13 +167,19 @@ func (s *Service) updateAccessEntry(ctx context.Context, accessEntry ekscontrolp
167167
return errors.Wrapf(err, "failed to describe access entry for principal %s", accessEntry.PrincipalARN)
168168
}
169169

170+
// Use the actual type from AWS when not specified in spec (defaults to STANDARD on EKS)
171+
desiredType := accessEntry.Type
172+
if desiredType == "" {
173+
desiredType = ekscontrolplanev1.AccessEntryTypeStandard
174+
}
175+
170176
// EKS requires recreate when changing type or removing username
171177
existingUsername := ""
172178
if describeOutput.AccessEntry.Username != nil {
173179
existingUsername = *describeOutput.AccessEntry.Username
174180
}
175181

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

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

235246
for _, policy := range accessEntry.AccessPolicies {
247+
existing, alreadyAssociated := existingPolicies[policy.PolicyARN]
248+
if alreadyAssociated && s.policyScopeMatches(policy.AccessScope, existing.AccessScope) {
249+
delete(existingPolicies, policy.PolicyARN)
250+
continue
251+
}
252+
236253
input := &eks.AssociateAccessPolicyInput{
237254
ClusterName: &clusterName,
238255
PrincipalArn: &accessEntry.PrincipalARN,
@@ -266,6 +283,19 @@ func (s *Service) reconcileAccessPolicies(ctx context.Context, accessEntry eksco
266283
return nil
267284
}
268285

286+
func (s *Service) policyScopeMatches(desired ekscontrolplanev1.AccessScope, existing *ekstypes.AccessScope) bool {
287+
if existing == nil {
288+
return false
289+
}
290+
if string(desired.Type) != string(existing.Type) {
291+
return false
292+
}
293+
if desired.Type == ekscontrolplanev1.AccessScopeTypeNamespace {
294+
return slices.Equal(desired.Namespaces, existing.Namespaces)
295+
}
296+
return true
297+
}
298+
269299
func (s *Service) getExistingAccessPolicies(ctx context.Context, principalARN string) (map[string]ekstypes.AssociatedAccessPolicy, error) {
270300
existingPolicies := map[string]ekstypes.AssociatedAccessPolicy{}
271301
var nextToken *string

‎pkg/cloud/services/eks/accessentry_test.go‎

Lines changed: 198 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -160,6 +160,7 @@ func TestReconcileAccessEntries(t *testing.T) {
160160

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

163+
// Policy already matches, should NOT re-associate
163164
m.ListAssociatedAccessPolicies(gomock.Any(), gomock.Any()).Return(&eks.ListAssociatedAccessPoliciesOutput{
164165
AssociatedAccessPolicies: []ekstypes.AssociatedAccessPolicy{
165166
{
@@ -170,15 +171,137 @@ func TestReconcileAccessEntries(t *testing.T) {
170171
},
171172
},
172173
}, nil)
174+
},
175+
expectError: false,
176+
},
177+
{
178+
name: "no drift - skip reconciliation when entry matches",
179+
accessEntries: []ekscontrolplanev1.AccessEntry{
180+
{
181+
PrincipalARN: principalARN,
182+
Type: ekscontrolplanev1.AccessEntryTypeStandard,
183+
Username: "admin",
184+
AccessPolicies: []ekscontrolplanev1.AccessPolicyReference{
185+
{
186+
PolicyARN: policyARN,
187+
AccessScope: ekscontrolplanev1.AccessScope{
188+
Type: ekscontrolplanev1.AccessScopeTypeCluster,
189+
},
190+
},
191+
},
192+
},
193+
},
194+
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
195+
m.ListAccessEntries(gomock.Any(), gomock.Any()).Return(&eks.ListAccessEntriesOutput{
196+
AccessEntries: []string{principalARN},
197+
}, nil)
173198

174-
m.AssociateAccessPolicy(gomock.Any(), &eks.AssociateAccessPolicyInput{
199+
// First DescribeAccessEntry in getManagedAccessEntries
200+
m.DescribeAccessEntry(gomock.Any(), &eks.DescribeAccessEntryInput{
175201
ClusterName: aws.String(clusterName),
176202
PrincipalArn: aws.String(principalARN),
177-
PolicyArn: aws.String(policyARN),
178-
AccessScope: &ekstypes.AccessScope{
179-
Type: ekstypes.AccessScopeTypeCluster,
203+
}).Return(&eks.DescribeAccessEntryOutput{
204+
AccessEntry: &ekstypes.AccessEntry{
205+
PrincipalArn: aws.String(principalARN),
206+
Username: aws.String("admin"),
207+
Type: ekscontrolplanev1.AccessEntryTypeStandard.APIValue(),
208+
Tags: map[string]string{
209+
"kubernetes.io/cluster/test-cluster": "owned",
210+
},
180211
},
181-
}).Return(&eks.AssociateAccessPolicyOutput{}, nil)
212+
}, nil)
213+
214+
// Second DescribeAccessEntry in updateAccessEntry
215+
m.DescribeAccessEntry(gomock.Any(), &eks.DescribeAccessEntryInput{
216+
ClusterName: aws.String(clusterName),
217+
PrincipalArn: aws.String(principalARN),
218+
}).Return(&eks.DescribeAccessEntryOutput{
219+
AccessEntry: &ekstypes.AccessEntry{
220+
PrincipalArn: aws.String(principalARN),
221+
Username: aws.String("admin"),
222+
Type: ekscontrolplanev1.AccessEntryTypeStandard.APIValue(),
223+
Tags: map[string]string{
224+
"kubernetes.io/cluster/test-cluster": "owned",
225+
},
226+
},
227+
}, nil)
228+
229+
// Policy already matches, no AssociateAccessPolicy call
230+
m.ListAssociatedAccessPolicies(gomock.Any(), gomock.Any()).Return(&eks.ListAssociatedAccessPoliciesOutput{
231+
AssociatedAccessPolicies: []ekstypes.AssociatedAccessPolicy{
232+
{
233+
PolicyArn: aws.String(policyARN),
234+
AccessScope: &ekstypes.AccessScope{
235+
Type: ekstypes.AccessScopeTypeCluster,
236+
},
237+
},
238+
},
239+
}, nil)
240+
},
241+
expectError: false,
242+
},
243+
{
244+
name: "no drift with empty type - should treat as STANDARD",
245+
accessEntries: []ekscontrolplanev1.AccessEntry{
246+
{
247+
PrincipalARN: principalARN,
248+
Username: "admin",
249+
AccessPolicies: []ekscontrolplanev1.AccessPolicyReference{
250+
{
251+
PolicyARN: policyARN,
252+
AccessScope: ekscontrolplanev1.AccessScope{
253+
Type: ekscontrolplanev1.AccessScopeTypeCluster,
254+
},
255+
},
256+
},
257+
},
258+
},
259+
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
260+
m.ListAccessEntries(gomock.Any(), gomock.Any()).Return(&eks.ListAccessEntriesOutput{
261+
AccessEntries: []string{principalARN},
262+
}, nil)
263+
264+
// First DescribeAccessEntry in getManagedAccessEntries
265+
m.DescribeAccessEntry(gomock.Any(), &eks.DescribeAccessEntryInput{
266+
ClusterName: aws.String(clusterName),
267+
PrincipalArn: aws.String(principalARN),
268+
}).Return(&eks.DescribeAccessEntryOutput{
269+
AccessEntry: &ekstypes.AccessEntry{
270+
PrincipalArn: aws.String(principalARN),
271+
Username: aws.String("admin"),
272+
Type: ekscontrolplanev1.AccessEntryTypeStandard.APIValue(),
273+
Tags: map[string]string{
274+
"kubernetes.io/cluster/test-cluster": "owned",
275+
},
276+
},
277+
}, nil)
278+
279+
// Second DescribeAccessEntry in updateAccessEntry
280+
m.DescribeAccessEntry(gomock.Any(), &eks.DescribeAccessEntryInput{
281+
ClusterName: aws.String(clusterName),
282+
PrincipalArn: aws.String(principalARN),
283+
}).Return(&eks.DescribeAccessEntryOutput{
284+
AccessEntry: &ekstypes.AccessEntry{
285+
PrincipalArn: aws.String(principalARN),
286+
Username: aws.String("admin"),
287+
Type: ekscontrolplanev1.AccessEntryTypeStandard.APIValue(),
288+
Tags: map[string]string{
289+
"kubernetes.io/cluster/test-cluster": "owned",
290+
},
291+
},
292+
}, nil)
293+
294+
// Policy already matches, no AssociateAccessPolicy call
295+
m.ListAssociatedAccessPolicies(gomock.Any(), gomock.Any()).Return(&eks.ListAssociatedAccessPoliciesOutput{
296+
AssociatedAccessPolicies: []ekstypes.AssociatedAccessPolicy{
297+
{
298+
PolicyArn: aws.String(policyARN),
299+
AccessScope: &ekstypes.AccessScope{
300+
Type: ekstypes.AccessScopeTypeCluster,
301+
},
302+
},
303+
},
304+
}, nil)
182305
},
183306
expectError: false,
184307
},
@@ -258,15 +381,7 @@ func TestReconcileAccessEntries(t *testing.T) {
258381
},
259382
}, nil)
260383

261-
m.AssociateAccessPolicy(gomock.Any(), &eks.AssociateAccessPolicyInput{
262-
ClusterName: aws.String(clusterName),
263-
PrincipalArn: aws.String(principalARN),
264-
PolicyArn: aws.String(policyARN),
265-
AccessScope: &ekstypes.AccessScope{
266-
Type: ekstypes.AccessScopeTypeCluster,
267-
},
268-
}).Return(&eks.AssociateAccessPolicyOutput{}, nil)
269-
384+
// Policy already matches, should NOT re-associate
270385
m.DeleteAccessEntry(gomock.Any(), &eks.DeleteAccessEntryInput{
271386
ClusterName: aws.String(clusterName),
272387
PrincipalArn: aws.String(secondPrincipalARN),
@@ -347,15 +462,7 @@ func TestReconcileAccessEntries(t *testing.T) {
347462
},
348463
},
349464
}, nil)
350-
351-
m.AssociateAccessPolicy(gomock.Any(), &eks.AssociateAccessPolicyInput{
352-
ClusterName: aws.String(clusterName),
353-
PrincipalArn: aws.String(principalARN),
354-
PolicyArn: aws.String(policyARN),
355-
AccessScope: &ekstypes.AccessScope{
356-
Type: ekstypes.AccessScopeTypeCluster,
357-
},
358-
}).Return(&eks.AssociateAccessPolicyOutput{}, nil)
465+
// Policy already matches, should NOT re-associate
359466
},
360467
expectError: false,
361468
},
@@ -529,6 +636,74 @@ func TestReconcileAccessPolicies(t *testing.T) {
529636
},
530637
expectError: false,
531638
},
639+
{
640+
name: "policy already matches - skip association",
641+
accessEntry: ekscontrolplanev1.AccessEntry{
642+
PrincipalARN: principalARN,
643+
Type: ekscontrolplanev1.AccessEntryTypeStandard,
644+
AccessPolicies: []ekscontrolplanev1.AccessPolicyReference{
645+
{
646+
PolicyARN: policyARN,
647+
AccessScope: ekscontrolplanev1.AccessScope{
648+
Type: ekscontrolplanev1.AccessScopeTypeCluster,
649+
},
650+
},
651+
},
652+
},
653+
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
654+
m.ListAssociatedAccessPolicies(gomock.Any(), gomock.Any()).Return(&eks.ListAssociatedAccessPoliciesOutput{
655+
AssociatedAccessPolicies: []ekstypes.AssociatedAccessPolicy{
656+
{
657+
PolicyArn: aws.String(policyARN),
658+
AccessScope: &ekstypes.AccessScope{
659+
Type: ekstypes.AccessScopeTypeCluster,
660+
},
661+
},
662+
},
663+
}, nil)
664+
// Should NOT call AssociateAccessPolicy when policy already matches
665+
},
666+
expectError: false,
667+
},
668+
{
669+
name: "policy scope changed - re-associate",
670+
accessEntry: ekscontrolplanev1.AccessEntry{
671+
PrincipalARN: principalARN,
672+
Type: ekscontrolplanev1.AccessEntryTypeStandard,
673+
AccessPolicies: []ekscontrolplanev1.AccessPolicyReference{
674+
{
675+
PolicyARN: policyARN,
676+
AccessScope: ekscontrolplanev1.AccessScope{
677+
Type: ekscontrolplanev1.AccessScopeTypeNamespace,
678+
Namespaces: []string{"kube-system"},
679+
},
680+
},
681+
},
682+
},
683+
expect: func(m *mock_eksiface.MockEKSAPIMockRecorder) {
684+
m.ListAssociatedAccessPolicies(gomock.Any(), gomock.Any()).Return(&eks.ListAssociatedAccessPoliciesOutput{
685+
AssociatedAccessPolicies: []ekstypes.AssociatedAccessPolicy{
686+
{
687+
PolicyArn: aws.String(policyARN),
688+
AccessScope: &ekstypes.AccessScope{
689+
Type: ekstypes.AccessScopeTypeCluster,
690+
},
691+
},
692+
},
693+
}, nil)
694+
695+
m.AssociateAccessPolicy(gomock.Any(), &eks.AssociateAccessPolicyInput{
696+
ClusterName: aws.String(clusterName),
697+
PrincipalArn: aws.String(principalARN),
698+
PolicyArn: aws.String(policyARN),
699+
AccessScope: &ekstypes.AccessScope{
700+
Type: ekstypes.AccessScopeTypeNamespace,
701+
Namespaces: []string{"kube-system"},
702+
},
703+
}).Return(&eks.AssociateAccessPolicyOutput{}, nil)
704+
},
705+
expectError: false,
706+
},
532707
}
533708

534709
for _, tc := range tests {

0 commit comments

Comments
 (0)