fix(ec2): restrictDefaultSecurityGroup fails when default rules are not present

[clueleaf]

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]

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]

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.

[clueleaf]

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.

[MrArnoldPalmer]

ahhh yes, makes sense.

[clueleaf]

@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]

@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]

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

[clueleaf]

@MrArnoldPalmer
Thanks! That will be helpful.

Pull request has been modified.

[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.

[aws-cdk-automation]

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]

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).