Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Don't tag self managed node security group with kubernetes.io/cluster tag #1774

Merged
merged 8 commits into from
Feb 2, 2022

Conversation

imdevin567
Copy link
Contributor

Description

This removes the forced kubernetes.io/cluster tag in self managed node security groups in favor of the user supplying it themselves.

Motivation and Context

We currently tag the created node security group with the kubernetes.io/cluster/clustername = owned tag. We are also tagging the self managed node security group with the same tag, which causes a conflict launching load balancers:

Error syncing load balancer: failed to ensure load balancer: Multiple tagged security groups found for instance i-aaaaaab0ab17adb5d; ensure only the k8s security group is tagged;

Since we don't add this tag to the EKS managed node security group, this will ensure consistency among node groups.

Breaking Changes

No breaking change

How Has This Been Tested?

  • I have tested and validated these changes using one or more of the provided examples/* projects

@bryantbiggs
Copy link
Member

I think the alternative here would be to not let the self managed node group create a security group, and instead supply your own security group

@imdevin567
Copy link
Contributor Author

That's fine, but right now the created security group for self managed nodes is unusable since it results in multiple tagged security groups. It can be worked around, but we should probably fix (or remove) the functionality that isn't working correctly.

@bryantbiggs
Copy link
Member

@daroga0002 thoughts?

@daroga0002
Copy link
Contributor

@imdevin567 could you pass here problematic module configuration which is solved by this PR?

@imdevin567
Copy link
Contributor Author

Sure thing.

module "eks" {
  source  = "terraform-aws-modules/eks/aws"
  version = "~> 18.0"

  cluster_version = "1.21"
  cluster_name    = "${var.env}-eks-cluster"
  vpc_id          = var.vpc_id
  subnet_ids      = concat(tolist(var.public_subnets), tolist(var.private_subnets))

  self_managed_node_group_defaults = {
    subnet_ids = var.private_subnets
  }

  self_managed_node_groups = {
    ci = {
      name                 = "${var.env}-ci"
      instance_types       = [var.ci_instance_type]
      min_size             = 1
      max_size             = 2
      desired_size         = 1
      bootstrap_extra_args = "--kubelet-extra-args -'-node-labels=mida.io/node_group=ci'"
    }
  }
}

This creates two problematic security groups: develop-eks-cluster-node20220112155729835300000001 which is the created node security group, and develop-ci-node-group-20220112202026375000000002 which is the created security group for the ci self managed node group. Both of these security groups contain the annotation:

kubernetes.io/cluster/develop-eks-cluster = owned

Both security groups are attached to the self managed nodes, which causes the following error when trying to create LoadBalacer resources:

Error syncing load balancer: failed to ensure load balancer: Multiple tagged security groups found for instance i-aaaaaab0ab17adb5d; ensure only the k8s security group is tagged;

@@ -459,10 +459,6 @@ resource "aws_security_group" "this" {

tags = merge(
var.tags,
{
"Name" = local.security_group_name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add back in the "Name" tag, then we should be good to go with this one

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be all set now.

Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@antonbabenko good to go 👍🏽 - we might see an issue pop up since its removing a tag, but it is the right move to remove it and let user opt in to when and where they want to use that tag

@antonbabenko antonbabenko merged commit a638e4a into terraform-aws-modules:master Feb 2, 2022
antonbabenko pushed a commit that referenced this pull request Feb 2, 2022
### [18.2.7](v18.2.6...v18.2.7) (2022-02-02)

### Bug Fixes

* Don't tag self managed node security group with kubernetes.io/cluster tag ([#1774](#1774)) ([a638e4a](a638e4a))
@antonbabenko
Copy link
Member

This PR is included in version 18.2.7 🎉

baibailiha added a commit to baibailiha/terraform-aws-eks that referenced this pull request Sep 13, 2022
### [18.2.7](terraform-aws-modules/terraform-aws-eks@v18.2.6...v18.2.7) (2022-02-02)

### Bug Fixes

* Don't tag self managed node security group with kubernetes.io/cluster tag ([#1774](terraform-aws-modules/terraform-aws-eks#1774)) ([e3cc25e](terraform-aws-modules/terraform-aws-eks@e3cc25e))
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants