feat(ec2): add allowAllSelf to SecurityGroup - #38636
Open
lemon0333 wants to merge 1 commit into
Open
Conversation
Add an `allowAllSelf` prop to `SecurityGroupProps` that creates self-referencing all-traffic ingress and egress rules, the exact configuration required by Elastic Fabric Adapter (EFA) network interfaces. The self-referencing egress rule is always emitted, even with the default `allowAllOutbound: true`. The all-outbound subsumption shortcut only covers CIDR-based egress rules and does not apply to a self-referencing security-group egress rule, so it is bypassed for this explicit rule. Closes aws#38558.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue # (if applicable)
Closes #38558.
Reason for this change
There is no single
SecurityGroupprop to allow all traffic to self. This configuration (a self-referencing ingress and egress rule for all protocols/ports) is exactly what Elastic Fabric Adapter (EFA) network interfaces require. Today users have to either drop toconnections.allowInternally(Port.allTraffic())withallowAllOutbound: false, or use an L1 escape hatch (CfnSecurityGroupEgress) to get the egress half whenallowAllOutboundis left at its default. This is a discoverability/ergonomics gap that @pahud greenlit in the issue.Description of changes
readonly allowAllSelf?: boolean(@default false) toSecurityGroupProps.SecurityGroupconstructor, whenallowAllSelfistrue, create the self-referencing all-traffic ingress and egress rules (the ergonomic equivalent ofconnections.allowInternally(Port.allTraffic())).Egress semantics (the design gotcha @pahud flagged): with the default
allowAllOutbound: true,addEgressRulenormally drops explicit egress rules because it assumes the implicit "all traffic to0.0.0.0/0" rule subsumes them. That assumption is not valid for a self-referencing security-group egress rule — which is precisely what EFA needs. So I chose the preferred option from the two discussed: always emit the self egress rule regardless ofallowAllOutbound, by bypassing the subsumption shortcut for this one explicit self-rule (aSecurityGrouppeer hascanInlineRule === false, so the normal path already delegates tosuper.addEgressRule— calling it directly produces the identicalCfnSecurityGroupEgress, same scope and id, just without the shortcut). WhenallowAllOutbound: false, the regular path is used (which also cleans up the placeholder "no traffic" egress rule).Net effect:
new ec2.SecurityGroup(this, 'EfaSg', { vpc, allowAllSelf: true })yields the exact EFA configuration in one line while keeping the defaultallowAllOutbound: truebehavior for every other rule unchanged. I rejected the stricter alternative (throw/validate thatallowAllSelfrequiresallowAllOutbound: false) because it would force users to give up all-outbound access to get a self egress rule, which isn't necessary. @pahud — happy to switch to the stricter variant if you'd prefer it.The prop doc-comment and a new README section document the exact semantics and the
allowAllOutboundinteraction.Describe any new or updated permissions being added
None.
Description of how you validated changes
aws-ec2/test/security-group.test.ts, newallowAllSelfdescribe block, 3 tests; full file 133/133 passing):allowAllOutbound: true, and the default all-outbound rule on the group is preserved;allowAllOutbound: false(placeholder no-traffic egress rule removed);allowAllSelfisfalse/omitted.integ.security-group-allow-all-self.ts) covering bothallowAllOutbounddefault andfalse, with snapshot committed. The snapshot confirms both groups emit a self-referencing ingress and egress rule (IpProtocol: -1, referencing the group itself), including the default-allowAllOutboundgroup.aws-ec2/README.mddocumentingallowAllSelfand theallowAllOutboundinteraction.Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license