Don't attach the control plane security group to managed nodegroups #4860

tompiscitell · 2022-02-28T14:24:20Z

Description

When creating Managed Nodegroups from an imported cluster, the control plane security group was getting attached to the worker nodes. This security group is supposed to be attached to the control plane ENIs according to the EKS docs. With this PR, only the cluster security group will get attached to Managed Nodegroups. This is consistent with the behavior of adding Managed Nodegroups to clusters created by eksctl.

Checklist

Added tests that cover your change (if possible)
Added/modified documentation as required (such as the README.md, or the userdocs directory)
Manually tested
Made sure the title of the PR is a good description that can go into the release notes
(Core team) Added labels for change area (e.g. area/nodegroup) and kind (e.g. kind/improvement)

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Backfilled missing tests for code in same general area 🎉
Refactored something and made the world a better place 🌟

When creating Managed Nodegroups from an imported cluster, the control plane security group was getting attached to the worker nodes. Now only the cluster security group will get attached, which is consistent with the behavior of adding Managed Nodegroups to clusters created by eksctl.

Himangini · 2022-03-01T11:52:23Z

@tompiscitell thanks for the nicely written PR. Core team will review it soon 👍🏻

Skarlso · 2022-03-02T08:07:30Z

Hi @tompiscitell.

That document says earlier than 1.14. What makes you think this is not required for versions above 1.14?

Oh, I see, I read the whole doc in stead of only that section. ;)

By assigning the cluster security group to the elastic network interfaces created by Amazon EKS that allow the control plane to communicate with the managed node group instances, you don't need to configure complex security group rules to allow this communication. Any instance or network interface that is assigned this security group can freely communicate with other resources with this security group.

Skarlso · 2022-03-02T08:25:56Z

pkg/vpc/importer.go

 func (si *SpecConfigImporter) SecurityGroups() gfnt.Slice {
-	return gfnt.Slice{si.ControlPlaneSecurityGroup(), si.ClusterSecurityGroup()}
+	return gfnt.Slice{si.ClusterSecurityGroup()}


This change only affects unowned clusters. For Owned clusters we are already doing this. I except this is actually not doing anything, but I have to further check the code.

I see now. It is used, it's the same as managed cluster.

This change only affects unowned clusters. For Owned clusters we are already doing this. I except this is actually not doing anything, but I have to further check the code.

why would this change only affect unowned? I'm slightly confused :/

owned cluster use the other implementation of the Importer interface: StackConfigImporter. See the if block here: https://github.com/weaveworks/eksctl/blob/main/pkg/actions/nodegroup/create.go#L169

ahh makes sense, thank you!

Skarlso · 2022-03-02T08:33:20Z

@tompiscitell Ok, now that I understand the code a bit better, let me summarise. This change is only applicable to unowned clusters. Meaning, it's a cluster that eksctl has not created. For managed clusters we already do the right thing. I'm okay with this change, but please paste in manual tests and steps and outcome so we can see that you tried this code change and an unowned cluster still works in creating managed nodegroups and communication is not disrupted.

So what you need:

create an EKS cluster but not by using eksctl
import that cluster
create a managed nodegroup
show results

Thanks! :)

Skarlso

Request Manual Test steps then LGTM.

tompiscitell · 2022-03-04T16:45:10Z

@Skarlso I made a small project for the manual test: https://github.com/tompiscitell/eks-testing in case you want to follow along. It uses the AWS CDK to create an EKS cluster and has some scripts to generate your cluster config and inspect the worker security groups.

Here is my cluster config yaml with the cluster created via the AWS CDK (note: these are all throw away resources that are already deleted, so I'm not bothering to redact):

apiVersion: eksctl.io/v1alpha5
kind: ClusterConfig

metadata:
  name: Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a
  region: us-west-2

vpc:
  id: vpc-0aee84ee8845f35e0
  securityGroup: sg-0d715dd9c05695143
  subnets:
    public:
      us-west-2a:
        id: subnet-07d14af61188fa94b
      us-west-2b:
        id: subnet-0db85e2c5476fd65c
    private:
      us-west-2b:
        id: subnet-002a374fa1fee97aa
      us-west-2a:
        id: subnet-099811bceaee69927
managedNodeGroups:
  - name: workers
    amiFamily: Bottlerocket
    privateNetworking: true
    desiredCapacity: 1

I then used eksctl to create the manage NodeGroups specified in the config:

$ ../eksctl/eksctl create nodegroup -f cluster.yml  
2022-03-04 11:32:19 [ℹ]  eksctl version 0.86.0-dev+c65b6911.2022-02-22T16:22:38Z
2022-03-04 11:32:19 [ℹ]  using region us-west-2
2022-03-04 11:32:20 [ℹ]  will use version 1.21 for new nodegroup(s) based on control plane version
2022-03-04 11:32:21 [!]  no eksctl-managed CloudFormation stacks found for "Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a", will attempt to create nodegroup(s) on non eksctl-managed cluster
2022-03-04 11:32:22 [ℹ]  nodegroup "workers" will use "" [Bottlerocket/1.21]
2022-03-04 11:32:22 [ℹ]  1 existing nodegroup(s) (ClusterNodegroupDefaultCapa-WAEdCEpBwbBD) will be excluded
2022-03-04 11:32:22 [ℹ]  1 nodegroup (workers) was included (based on the include/exclude rules)
2022-03-04 11:32:22 [ℹ]  will create a CloudFormation stack for each of 1 managed nodegroups in cluster "Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a"
2022-03-04 11:32:22 [ℹ]  1 task: { 1 task: { 1 task: { create managed nodegroup "workers" } } }
2022-03-04 11:32:22 [ℹ]  building managed nodegroup stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:32:23 [ℹ]  deploying stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:32:23 [ℹ]  waiting for CloudFormation stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:32:39 [ℹ]  waiting for CloudFormation stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:32:59 [ℹ]  waiting for CloudFormation stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:33:19 [ℹ]  waiting for CloudFormation stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:33:38 [ℹ]  waiting for CloudFormation stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:33:55 [ℹ]  waiting for CloudFormation stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:34:15 [ℹ]  waiting for CloudFormation stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:34:30 [ℹ]  waiting for CloudFormation stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:34:46 [ℹ]  waiting for CloudFormation stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:35:06 [ℹ]  waiting for CloudFormation stack "eksctl-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-nodegroup-workers"
2022-03-04 11:35:07 [ℹ]  no tasks
2022-03-04 11:35:07 [✔]  created 0 nodegroup(s) in cluster "Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a"
2022-03-04 11:35:07 [ℹ]  nodegroup "workers" has 1 node(s)
2022-03-04 11:35:07 [ℹ]  node "ip-10-0-213-133.us-west-2.compute.internal" is ready
2022-03-04 11:35:07 [ℹ]  waiting for at least 1 node(s) to become ready in "workers"
2022-03-04 11:35:07 [ℹ]  nodegroup "workers" has 1 node(s)
2022-03-04 11:35:07 [ℹ]  node "ip-10-0-213-133.us-west-2.compute.internal" is ready
2022-03-04 11:35:07 [✔]  created 1 managed nodegroup(s) in cluster "Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a"
2022-03-04 11:35:08 [ℹ]  checking security group configuration for all nodegroups
2022-03-04 11:35:08 [ℹ]  all nodegroups have up-to-date cloudformation templates

And you can see it only attaches the cluster security group (the one created by EKS):

$ aws ec2 describe-instances --filter Name=tag:alpha.eksctl.io/nodegroup-name,Values=workers | jq -r '.Reservations[0].Instances[0].SecurityGroups'
[
  {
    "GroupName": "eks-cluster-sg-Cluster9EE0221C-17854dabb51d466ebd19651517aaf92a-1038925482",
    "GroupId": "sg-080c7cf4526e395cb"
  }
]

The node (the second one in the output) still joined the cluster and is ready. The other two are ones that the CDK creates by default:

$ kubectl get nodes
NAME                                         STATUS   ROLES    AGE    VERSION
ip-10-0-171-195.us-west-2.compute.internal   Ready    <none>   2d2h   v1.21.5-eks-9017834
ip-10-0-213-133.us-west-2.compute.internal   Ready    <none>   76s    v1.21.9
ip-10-0-240-190.us-west-2.compute.internal   Ready    <none>   2d2h   v1.21.5-eks-9017834

Skarlso · 2022-03-07T11:10:44Z

@tompiscitell fantastic work! Thank you very much!

Himangini added kind/docs User documentation kind/improvement and removed kind/docs User documentation labels Mar 1, 2022

Himangini requested review from Himangini, aclevername, nikimanoledaki and Skarlso March 1, 2022 11:52

Skarlso reviewed Mar 2, 2022

View reviewed changes

Skarlso suggested changes Mar 2, 2022

View reviewed changes

Himangini requested review from a team and removed request for Himangini, nikimanoledaki and aclevername March 7, 2022 11:01

Merge branch 'main' into vpc-importer-security-groups

215ed55

Skarlso approved these changes Mar 7, 2022

View reviewed changes

Skarlso enabled auto-merge (squash) March 7, 2022 11:11

Himangini disabled auto-merge March 7, 2022 13:53

aclevername enabled auto-merge (squash) March 7, 2022 14:05

aclevername approved these changes Mar 7, 2022

View reviewed changes

aclevername merged commit 5cd40b8 into eksctl-io:main Mar 7, 2022

hspencer77 mentioned this pull request Jul 8, 2022

Upgrade eksctl brave-intl/eksctl#44

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Don't attach the control plane security group to managed nodegroups #4860

Don't attach the control plane security group to managed nodegroups #4860

tompiscitell commented Feb 28, 2022 •

edited by Himangini

Loading

Himangini commented Mar 1, 2022

Skarlso commented Mar 2, 2022 •

edited

Loading

Skarlso Mar 2, 2022

Skarlso Mar 2, 2022

aclevername Mar 7, 2022

tompiscitell Mar 7, 2022

aclevername Mar 7, 2022

Skarlso commented Mar 2, 2022

Skarlso left a comment

tompiscitell commented Mar 4, 2022

Skarlso commented Mar 7, 2022

Don't attach the control plane security group to managed nodegroups #4860

Don't attach the control plane security group to managed nodegroups #4860

Conversation

tompiscitell commented Feb 28, 2022 • edited by Himangini Loading

Description

Checklist

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

Himangini commented Mar 1, 2022

Skarlso commented Mar 2, 2022 • edited Loading

Skarlso Mar 2, 2022

Choose a reason for hiding this comment

Skarlso Mar 2, 2022

Choose a reason for hiding this comment

aclevername Mar 7, 2022

Choose a reason for hiding this comment

tompiscitell Mar 7, 2022

Choose a reason for hiding this comment

aclevername Mar 7, 2022

Choose a reason for hiding this comment

Skarlso commented Mar 2, 2022

Skarlso left a comment

Choose a reason for hiding this comment

tompiscitell commented Mar 4, 2022

Skarlso commented Mar 7, 2022

tompiscitell commented Feb 28, 2022 •

edited by Himangini

Loading

Skarlso commented Mar 2, 2022 •

edited

Loading