fix(eks): Self managed nodes cannot be added to LoadBalancers created via the LoadBalancer service type

[iliapolo]

Following this PR, self managed nodes are now attached with the cluster security group. This causes the self managed nodes to have multiple security groups with the "owned" tag. This in turn causes load balancers to reject these instances since its unable to determine which security groups should be added with ingress rules to allow the load balancer to connect to the instances.

The fix is to exclude tagging the dedicated ASG security group with this tag, it is no longer necessary since the cluster security group has that tag by default.

Fixes #12166

This breaksge is unfortunate, but I can't see a way out of it. And it does actually fix a bug.

BREAKING CHANGE: Existing self managed nodes may loose the ability to host additional services of type LoadBalancer . See #12269 (comment) for possible mitigations.

---

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

[gitpod-io]

[iliapolo]

After applying this fix, the dedicated security group of self managed nodes will no longer be tagged with the "owned" tag.
This will prevent these nodes from being added to load balancers because they will not have any owned security groups attached to them, effectively causing the same phenomena this PR is meant to solve.

Note: This only applies to self managed nodes launched with a version prior to 1.79.0. Nodes provisioned following that version will have the cluster security group attached to them, which has this tag by default.

While this will not cause any immediate disruption, it does pose an operational risk for future service deployments. The proper setup for nodes should be that they are attached with exactly one owned security group.

We suggest one of the following approaches to achieve that:

• Deploy the fix and replace the existing nodes (either manually or via an instance refresh) (recommended)

• Temporarily configure the updatePolicy of the auto-scaling group to either UpdatePolicy.replacingUpdate or UpdatePolicy.rollingUpdate and deploy the fix. Wait for the instances to be refreshed, and revert the policy back.

• Manually attach the cluster security group to the existing instances.

[iliapolo]

@eladb I failed miserably in trying to get the integ test to deterministically cover this scenario.

To make sure its tested properly, we need to force the load balancer to only connect to self-managed nodes, since this is what fails and will cause the service not to be deployed.

Supposedly this is done by attaching labels to the nodes (i.e self-managed=true) and adding an annotation to the service:

'service.beta.kubernetes.io/aws-load-balancer-target-node-labels': 'self-managed=true'

However, this doesn't seem to work. I'm probably missing something, but I figured its not worth delaying this PR for. I verified the expected behavior manually and we also have a unit test and the current integ test to make sure we don't attach the unnecessary tag.

I feel we can merge this.

Let me know if you think differently.

[eladb]

Yeah sounds reasonable

[mergify]

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

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
• Commit ID: c88d42b
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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