Skip to content

Commit ed8bd00

Browse files
authored
Merge pull request #3952 from zac-nixon/znixon/sg-revoke-flip
bug fix: try SG permission add prior to revoke
2 parents ba4152c + 31ffe10 commit ed8bd00

File tree

3 files changed

+411
-10
lines changed

3 files changed

+411
-10
lines changed

pkg/networking/security_group_manager.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ func (opts *FetchSGInfoOptions) ApplyOptions(options ...FetchSGInfoOption) {
3333

3434
type FetchSGInfoOption func(opts *FetchSGInfoOptions)
3535

36-
// WithReloadIgnoringCache is a option that sets the ReloadIgnoringCache to true.
36+
// WithReloadIgnoringCache is an option that sets the ReloadIgnoringCache to true.
3737
func WithReloadIgnoringCache() FetchSGInfoOption {
3838
return func(opts *FetchSGInfoOptions) {
3939
opts.ReloadIgnoringCache = true

pkg/networking/security_group_reconciler.go

Lines changed: 32 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -77,23 +77,24 @@ func (r *defaultSecurityGroupReconciler) ReconcileIngress(ctx context.Context, s
7777
return err
7878
}
7979
sgInfo := sgInfoByID[sgID]
80-
if err := r.reconcileIngressWithSGInfo(ctx, sgInfo, desiredPermissions, reconcileOpts); err != nil {
80+
81+
if err := r.reconcileIngressWithSGInfo(ctx, sgInfo, desiredPermissions, false, reconcileOpts); err != nil {
8182
if !r.shouldRetryWithoutCache(err) {
8283
return err
8384
}
85+
revokeFirst := r.shouldRemoveSGRulesFirst(err)
86+
r.logger.Info("Retrying ReconcileIngress without using cache", "revokeFirst", revokeFirst)
8487
sgInfoByID, err := r.sgManager.FetchSGInfosByID(ctx, []string{sgID}, WithReloadIgnoringCache())
8588
if err != nil {
8689
return err
8790
}
8891
sgInfo := sgInfoByID[sgID]
89-
if err := r.reconcileIngressWithSGInfo(ctx, sgInfo, desiredPermissions, reconcileOpts); err != nil {
90-
return err
91-
}
92+
return r.reconcileIngressWithSGInfo(ctx, sgInfo, desiredPermissions, revokeFirst, reconcileOpts)
9293
}
9394
return nil
9495
}
9596

96-
func (r *defaultSecurityGroupReconciler) reconcileIngressWithSGInfo(ctx context.Context, sgInfo SecurityGroupInfo, desiredPermissions []IPPermissionInfo, reconcileOpts SecurityGroupReconcileOptions) error {
97+
func (r *defaultSecurityGroupReconciler) reconcileIngressWithSGInfo(ctx context.Context, sgInfo SecurityGroupInfo, desiredPermissions []IPPermissionInfo, revokeFirst bool, reconcileOpts SecurityGroupReconcileOptions) error {
9798
extraPermissions := diffIPPermissionInfos(sgInfo.Ingress, desiredPermissions)
9899
permissionsToRevoke := make([]IPPermissionInfo, 0, len(extraPermissions))
99100
for _, permission := range extraPermissions {
@@ -102,24 +103,46 @@ func (r *defaultSecurityGroupReconciler) reconcileIngressWithSGInfo(ctx context.
102103
}
103104
}
104105
permissionsToGrant := diffIPPermissionInfos(desiredPermissions, sgInfo.Ingress)
105-
if len(permissionsToRevoke) > 0 && !reconcileOpts.AuthorizeOnly {
106-
if err := r.sgManager.RevokeSGIngress(ctx, sgInfo.SecurityGroupID, permissionsToRevoke); err != nil {
107-
return err
106+
107+
if revokeFirst {
108+
if len(permissionsToRevoke) > 0 && !reconcileOpts.AuthorizeOnly {
109+
if err := r.sgManager.RevokeSGIngress(ctx, sgInfo.SecurityGroupID, permissionsToRevoke); err != nil {
110+
return err
111+
}
108112
}
109113
}
114+
110115
if len(permissionsToGrant) > 0 {
111116
if err := r.sgManager.AuthorizeSGIngress(ctx, sgInfo.SecurityGroupID, permissionsToGrant); err != nil {
112117
return err
113118
}
114119
}
120+
121+
if !revokeFirst {
122+
if len(permissionsToRevoke) > 0 && !reconcileOpts.AuthorizeOnly {
123+
if err := r.sgManager.RevokeSGIngress(ctx, sgInfo.SecurityGroupID, permissionsToRevoke); err != nil {
124+
return err
125+
}
126+
}
127+
}
128+
115129
return nil
116130
}
117131

118132
// shouldRetryWithoutCache tests whether we should retry SecurityGroup rules reconcile without cache.
119133
func (r *defaultSecurityGroupReconciler) shouldRetryWithoutCache(err error) bool {
120134
var apiErr smithy.APIError
121135
if errors.As(err, &apiErr) {
122-
return apiErr.ErrorCode() == "InvalidPermission.Duplicate" || apiErr.ErrorCode() == "InvalidPermission.NotFound"
136+
return apiErr.ErrorCode() == "InvalidPermission.Duplicate" || apiErr.ErrorCode() == "InvalidPermission.NotFound" || apiErr.ErrorCode() == "RulesPerSecurityGroupLimitExceeded"
137+
}
138+
return false
139+
}
140+
141+
// shouldRemoveSGRulesFirst tests whether we should retry SecurityGroup rules reconcile but revoking rules prior to adding new rules.
142+
func (r *defaultSecurityGroupReconciler) shouldRemoveSGRulesFirst(err error) bool {
143+
var apiErr smithy.APIError
144+
if errors.As(err, &apiErr) {
145+
return apiErr.ErrorCode() == "RulesPerSecurityGroupLimitExceeded"
123146
}
124147
return false
125148
}

0 commit comments

Comments
 (0)