Add parameter for skipping default security groups #1416

flostadler · 2024-10-02T20:12:53Z

Pulumi EKS currently always creates a cluster security group and node security group.

The cluster security group gets assigned to the control plane ENIs in addition to the security group EKS creates (see AWS Docs). This security group gets an ingress rule from the node security group.
The node security group gets assigned to NodeGroup and NodeGroupV2 components that do not specify a custom security group.

Users that either manage the node security themselves or use the ManagedNodeGroup component (uses the EKS created SG) do not need those default security groups.

This change adds a flag on the cluster (skipDefaultSecurityGroups) that will skip creating those default security groups. Instead.

This introduces a small breaking change, the clusterSecurityGroup, nodeSecurityGroup and clusterIngressRule outputs are now optional. The impact of this should be minimal because users that create custom node groups usually do not use the security groups of the cluster for that. If they do, they need to add a null check.

Fixes #747

…is set to true

github-actions · 2024-10-02T20:15:37Z

Does the PR have any schema changes?

Found 4 breaking changes:

Resources

"eks:index:Cluster": required:
- 🟢 "clusterSecurityGroup" property is no longer Required
- 🟢 "eksClusterIngressRule" property is no longer Required
- 🟢 "nodeSecurityGroup" property is no longer Required

Types

🟢 "eks:index:CoreData": required: "clusterSecurityGroup" property is no longer Required
No new resources/functions.

t0yv0 · 2024-10-03T15:39:19Z

nodejs/eks/nodegroup.ts

+        .all([coreSecurityGroupId, args.nodeSecurityGroup?.id, core.nodeSecurityGroupTags])
+        .apply(([coreSecurityGroup, nodeSecurityGroup, sgTags]) => {
+            if (coreSecurityGroup && nodeSecurityGroup) {
+                if (sgTags && coreSecurityGroup !== nodeSecurityGroup) {


Always suspicious of equality but this look like it's over string right? Might be OK. Perhaps call the variables "coreSecurityGroupId" or some such.

Yeah it's over string. Noticed that it was comparing Output<string> before..

t0yv0 · 2024-10-03T15:41:17Z

nodejs/eks/cluster.ts

+    // apply the tags to the cluster security group if it was created by EKS
+    if (!eksClusterSecurityGroup) {
+        const eksCreatedSgId = eksCluster.vpcConfig.clusterSecurityGroupId;
+        eksClusterSecurityGroup = aws.ec2.SecurityGroup.get(


Does this .get work OK for you inlight of today's discussion about a possible bug in MLC R.Get ?

Yeah, this was before our call. I just pushed a new version

nodejs/eks/cluster.ts

t0yv0 · 2024-10-03T15:43:34Z

sdk/dotnet/Cluster.cs

+        /// See for more details: https://docs.aws.amazon.com/eks/latest/userguide/sec-group-reqs.html.
+        /// </summary>
+        [Input("skipDefaultSecurityGroups")]
+        public bool? SkipDefaultSecurityGroups { get; set; }


Weird three-state boolean. true, false, and unknown. Alas, not worth fixing probably.

t0yv0 · 2024-10-03T15:46:14Z

examples/examples_nodejs_test.go

+				)
+
+				// should return the EKS created security group instead of the default one the provider would create
+				assert.Equal(t, info.Outputs["eksCreatedSgMng"], info.Outputs["clusterSecurityGroupMng"])


flostadler · 2024-10-03T10:37:14Z

examples/tests/migrate-nodegroups/utils.ts

@@ -17,7 +17,7 @@ export function createNodeGroup(
    return new eks.NodeGroup(name, {
        cluster: args.cluster,
        nodeSecurityGroup: args.cluster.nodeSecurityGroup,
-        clusterIngressRule: args.cluster.eksClusterIngressRule,
+        clusterIngressRule: args.cluster.eksClusterIngressRule.apply(rule => rule!),


That's the small breaking change

flostadler · 2024-10-03T16:30:31Z

nodejs/eks/nodegroup.ts

+        .all([coreSecurityGroupId, args.nodeSecurityGroup?.id, core.nodeSecurityGroupTags])
+        .apply(([coreSecurityGroup, nodeSecurityGroup, sgTags]) => {
+            if (coreSecurityGroup && nodeSecurityGroup) {
+                if (sgTags && coreSecurityGroup !== nodeSecurityGroup) {


Yeah it's over string. Noticed that it was comparing Output<string> before..

nodejs/eks/cluster.ts

flostadler · 2024-10-03T16:37:47Z

@t0yv0 @corymhall can you please have another look? I had to change the approach due to the issues with .get in MLCs.

t0yv0 · 2024-10-03T17:34:38Z

I had to change the approach due to the issues with .get in MLCs.

Very much worth spending time on to extract a repro and link here 🙏 If you are out of capacity sent it over to me as I might have some bandwidth.

flostadler · 2024-10-03T17:35:44Z

I had to change the approach due to the issues with .get in MLCs.

Very much worth spending time on to extract a repro and link here 🙏 If you are out of capacity sent it over to me as I might have some bandwidth.

Fully agreed! I already had a chat with Will about this. I'll try to create a minimal repro tomorrow to further investigate

Pulumi EKS currently always creates a cluster security group and node security group. - The cluster security group gets assigned to the control plane ENIs in addition to the security group EKS creates (see [AWS Docs](https://docs.aws.amazon.com/eks/latest/userguide/sec-group-reqs.html)). This security group gets an ingress rule from the node security group. - The node security group gets assigned to `NodeGroup` and `NodeGroupV2` components that do not specify a custom security group. Users that either manage the node security themselves or use the `ManagedNodeGroup` component (uses the EKS created SG) do not need those default security groups. This change adds a flag on the cluster (`skipDefaultSecurityGroups`) that will skip creating those default security groups. Instead. This introduces a small breaking change, the `clusterSecurityGroup`, `nodeSecurityGroup` and `clusterIngressRule` outputs are now optional. The impact of this should be minimal because users that create custom node groups usually do not use the security groups of the cluster for that. If they do, they need to add a null check. Fixes #747

flostadler added 7 commits October 2, 2024 21:29

Add parameter for skipping defautl security groups

710cc09

Do not create default security groups when skipDefaultSecurityGroups …

d99fe3d

…is set to true

Update overlays

5e5f564

Add tests

bb1e873

Regenerate schema

d3c0236

Regenerate SDK

0b27a4a

Update migration docs

1061126

flostadler added the needs-release/major Marking a PR to compute the next major version label Oct 2, 2024

flostadler requested review from t0yv0, corymhall and a team October 2, 2024 20:12

flostadler self-assigned this Oct 2, 2024

flostadler force-pushed the flostadler/skip-default-security-groups branch from 1061126 to 7955f51 Compare October 3, 2024 10:26

corymhall approved these changes Oct 3, 2024

View reviewed changes

t0yv0 reviewed Oct 3, 2024

View reviewed changes

nodejs/eks/cluster.ts Outdated Show resolved Hide resolved

t0yv0 reviewed Oct 3, 2024

View reviewed changes

t0yv0 approved these changes Oct 3, 2024

View reviewed changes

flostadler marked this pull request as draft October 3, 2024 16:08

Allow creating default node groups without default security groups

6e9f619

flostadler force-pushed the flostadler/skip-default-security-groups branch from 7955f51 to 6e9f619 Compare October 3, 2024 16:28

flostadler added 2 commits October 3, 2024 18:32

remove unrelated changes

7fd0607

Fix tests

01c0217

flostadler commented Oct 3, 2024

View reviewed changes

flostadler marked this pull request as ready for review October 3, 2024 16:37

flostadler requested review from t0yv0 and corymhall October 3, 2024 16:37

t0yv0 approved these changes Oct 3, 2024

View reviewed changes

Fix test

29a768a

flostadler merged commit 62a8eca into release-3.x.x Oct 3, 2024
34 checks passed

flostadler deleted the flostadler/skip-default-security-groups branch October 3, 2024 20:15

flostadler mentioned this pull request Oct 4, 2024

Option to skip creating node and cluster security groups #747

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parameter for skipping default security groups #1416

Add parameter for skipping default security groups #1416

flostadler commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

t0yv0 Oct 3, 2024

flostadler Oct 3, 2024

t0yv0 Oct 3, 2024

flostadler Oct 3, 2024

t0yv0 Oct 3, 2024

t0yv0 Oct 3, 2024

flostadler Oct 3, 2024

flostadler Oct 3, 2024

flostadler commented Oct 3, 2024

t0yv0 commented Oct 3, 2024

flostadler commented Oct 3, 2024 •

edited

Loading

Add parameter for skipping default security groups #1416

Add parameter for skipping default security groups #1416

Conversation

flostadler commented Oct 2, 2024 • edited Loading

github-actions bot commented Oct 2, 2024 • edited Loading

Does the PR have any schema changes?

Resources

Types

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flostadler commented Oct 3, 2024

t0yv0 commented Oct 3, 2024

flostadler commented Oct 3, 2024 • edited Loading

flostadler commented Oct 2, 2024 •

edited

Loading

github-actions bot commented Oct 2, 2024 •

edited

Loading

flostadler commented Oct 3, 2024 •

edited

Loading