fix(ec2): restrictDefaultSecurityGroup fails when default rules are not present by clueleaf · Pull Request #27039 · aws/aws-cdk

clueleaf · 2023-09-07T02:26:59Z

When using restrictDefaultSecurityGroup to remove default security group rules, an error is thrown and the deploy rolls back if the default rules are not found.
This error usually happens when developers previously removed default rules manually or by other means, and then want to switch to using restrictDefaultSecurityGroup. They have to re-add default rules and deploy again to cope with the error.
This PR fixes the custom resource to ignore the error when default rules are not found.

Closes #26390

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

clueleaf · 2023-09-07T03:19:22Z

Exemption Request
I don't think an integration test is necessary for custom resource code change.

MrArnoldPalmer

The way the linked issue was written, it seemed like this bug was an issue caused by changes in the CDK, but it looks like that's not actually the case? The handler always assumed these default rules were present whether using sdk v2 or v3 right?

An integ test would be ideal for this but I'm not actually sure how we would accomplish it. Will add the exception.

MrArnoldPalmer · 2023-09-08T16:33:05Z

+  try {
+    await ec2.revokeSecurityGroupEgress(egressRuleParams(groupId));
+  } catch (e: any) {
+    if (!(e instanceof Error) || (e instanceof Error && e.name !== 'InvalidPermission.NotFound')) {


We just rolled back using typed exceptions in sdk v3 because there are some known issues with it. We should just use e.name so we are in line with all our other custom resource code.

We should just be able to consolidate like so?

try { await ec2.revokeSecurityGroupEgress(egressRuleParams(groupId)); await ec2.revokeSecurityGroupIngress(ingressRuleParams(groupId, account)); } catch (e: any) { if (e.name === 'InvalidPermission.NotFound') { return; } throw e; }

Most typed exceptions in sdkV3 have a type for each different error.name value with that field hardcoded in. However Ec2 just has ServiceException with a bunch of different "error codes" which are also used as the error.name field. I just ran the commands against a non-existing security group to ensure that these name fields are as expected since we can't verify them in the sdk code.

Thanks for your explanation. I removed error type checks.
Just found the error code InvalidPermission.NotFound is listed in this doc.

We should just be able to consolidate like so?

I think these two API calls should be put into separate try-catch blocks, because even if the default egress rule is not found, we still want to execute revokeSecurityGroupIngress.

ahhh yes, makes sense.

clueleaf · 2023-09-09T00:07:09Z

@MrArnoldPalmer

The way the linked issue was written, it seemed like this bug was an issue caused by changes in the CDK, but it looks like that's not actually the case? The handler always assumed these default rules were present whether using sdk v2 or v3 right?

restrictDefaultSecurityGroup property and @aws-cdk/aws-ec2:restrictDefaultSecurityGroup feature flag were introduced in v2.78.0 (#25297), so setting it before v2.78.0 has no effect. The only change since the introduction of this feature was switching to use node18 and sdkv3.
I think the core cause of the issue was the actual security group situation as the error message shows.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

MrArnoldPalmer

Looks good, one snapshot needs to be updated in framework-integ. Are you able to run the integration test?

clueleaf · 2023-09-11T23:03:02Z

@MrArnoldPalmer
Thanks.
I can run the integration test, but I have trouble using my own domain.
Following this, I set up env vars:

% export HOSTED_ZONE_ID=<Hosted Zone ID that I own>
% export HOSTED_ZONE_NAME=<Hosted Zone name that I own>
% export DOMAIN_NAME=*.<Hosted Zone name that I own>

% echo $HOSTED_ZONE_ID
<Hosted Zone ID that I own>
% echo $HOSTED_ZONE_NAME
<Hosted Zone name that I own>
% echo $DOMAIN_NAME                     
*.<hosted zone name that I own>

When I try to update the snapshot,

% yarn integ test/aws-elasticloadbalancingv2/test/integ.alb.oidc.js --no-clean --update-on-failed

it requests certificate using *.example.com domain and then fails.
Do you have any guidance regarding setting up domains?

MrArnoldPalmer · 2023-09-12T17:38:25Z

ahhh you know what, let me run this for you then. I forgot this was required here.

clueleaf · 2023-09-12T23:18:27Z

@MrArnoldPalmer
Thanks! That will be helpful.

Pull request has been modified.

…ult_sg

MrArnoldPalmer

Was going to reapprove after merging in main but apparently I included some extra stuff that got generated during build. All this should be in main already or excluded. Sorry I'll clean this up tomorrow.

…ult_sg

aws-cdk-automation · 2023-09-20T16:45:55Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: d92f17d
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-09-20T16:46:20Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

clueleaf added 2 commits September 7, 2023 10:44

do not throw error if default sg rule is not found

a66f51d

add unit test

a6b199e

github-actions Bot added the valued-contributor [Pilot] contributed between 6-12 PRs to the CDK label Sep 7, 2023

aws-cdk-automation requested a review from a team September 7, 2023 02:27

github-actions Bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p2 labels Sep 7, 2023

aws-cdk-automation previously requested changes Sep 7, 2023

View reviewed changes

aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Sep 7, 2023

Merge branch 'main' into feat/restrict_default_sg

ce77941

MrArnoldPalmer suggested changes Sep 8, 2023

View reviewed changes

clueleaf added 2 commits September 9, 2023 08:36

separate test on error

ca66fb9

remove type check of exception

26b2b9c

clueleaf requested a review from MrArnoldPalmer September 9, 2023 00:07

mrgrain changed the title ~~fix(ec2): using restrictDefaultSecurityGroup should not fail when defalt rules are not present~~ fix(ec2): using restrictDefaultSecurityGroup should not fail when default rules are not present Sep 9, 2023

mrgrain changed the title ~~fix(ec2): using restrictDefaultSecurityGroup should not fail when default rules are not present~~ fix(ec2): restrictDefaultSecurityGroup fails when default rules are not present Sep 9, 2023

MrArnoldPalmer added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Sep 11, 2023

MrArnoldPalmer previously approved these changes Sep 11, 2023

View reviewed changes

update snapshots

4bf41fb

MrArnoldPalmer added 2 commits September 19, 2023 10:38

Merge branch 'main' of github.com:aws/aws-cdk into feat/restrict_defa…

5123bc9

…ult_sg

fix snapshots again

5864aa1

MrArnoldPalmer reviewed Sep 20, 2023

View reviewed changes

Merge branch 'main' of github.com:aws/aws-cdk into feat/restrict_defa…

d92f17d

…ult_sg

MrArnoldPalmer approved these changes Sep 20, 2023

View reviewed changes

mergify Bot merged commit 1e67f9d into aws:main Sep 20, 2023

clueleaf deleted the feat/restrict_default_sg branch September 21, 2023 12:06

github-actions Bot mentioned this pull request Oct 1, 2023

Monthly PRs metrics report - Sep 2023 #27368

Closed

mwebber mentioned this pull request Jun 11, 2024

(aws-ec2): restrictDefaultSecurityGroup does not remove IPv6 egress rule #29709

Open

Conversation

clueleaf commented Sep 7, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

clueleaf commented Sep 7, 2023

Uh oh!

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

Uh oh!

MrArnoldPalmer Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

clueleaf Sep 8, 2023

Choose a reason for hiding this comment

Uh oh!

MrArnoldPalmer Sep 11, 2023

Choose a reason for hiding this comment

Uh oh!

clueleaf commented Sep 9, 2023

Uh oh!

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

Uh oh!

clueleaf commented Sep 11, 2023

Uh oh!

MrArnoldPalmer commented Sep 12, 2023

Uh oh!

clueleaf commented Sep 12, 2023

Uh oh!

MrArnoldPalmer left a comment

Choose a reason for hiding this comment

Uh oh!

aws-cdk-automation commented Sep 20, 2023

AWS CodeBuild CI Report

Uh oh!

mergify Bot commented Sep 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants