fix: Fix cluster security group ids in outputs #792

slimm609 · 2020-03-13T23:03:03Z

PR o'clock

Description

when an eks cluster is created, the cluster security group is automatically created by eks, you can only add an additional security group, the outputs were returning the incorrect security group id because it was only returning the "additional" security group and not the cluster security group.

Please explain the changes you made here and link to any relevant issues.

Checklist

Change added to CHANGELOG.md. All changes must be added and breaking changes and highlighted
CI tests are passing
README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

dpiddockcmp · 2020-03-15T14:03:18Z

outputs.tf

-  description = "Security group ID attached to the EKS cluster."
-  value       = local.cluster_security_group_id
+  description = "Cluster security group ID attached to the EKS cluster."
+  value       = aws_eks_cluster.this[0].vpc_config[0].cluster_security_group_id


Can you test this output with a 1.13 cluster? The EKS controlled cluster security group is only added for kubenetes 1.14 and eks.3 platform. I'm wondering what Terraform sets the value to or whether it's an error.

Also, this pattern won't work with create_eks = false or possibly with a half destroyed state. See the concat(aws[*].output, [""])[0] patterned used in other outputs.

The first issue exists today. Renaming the variables in this PR didn't change anything besides the output.

I fixed the concat but haven't pushed it up yet because I am trying to work a solution for the 1.13 issue but now that I have tested, the issue exists in the current release as well as this change.

in a 1.13 cluster, the security_group_id is the primary field but still exists (either created or passed in), in a 1.14+ cluster, the security_group_id still either exists or is created but is just used as the additional field. I added a check to only use attempt to create node groups with 1.14 or greater. The eks.3 version doesn't matter because if you try to create a node group going forward, it will use the latest version (eks.8) and if you have an existing and try to modify it, it will force a recreate and pick up the latest.

dpiddockcmp · 2020-03-15T14:04:13Z

CHANGELOG.md

@@ -9,7 +9,7 @@ project adheres to [Semantic Versioning](http://semver.org/).

 ## [[v10.?.?](https://github.com/terraform-aws-modules/terraform-aws-eks/compare/v10.0.0...HEAD)] - 2020-xx-xx

- Write your awesome change here (GH-xxxx)
+- Fixed eks security group id outputs and additional security group id inputs (by @slimm609)


This is a breaking change. You've changed the value of the cluster_security_group_id output as well as renaming input values. It needs a big warning.

barryib · 2020-03-17T18:07:27Z

CHANGELOG.md

@@ -9,7 +9,7 @@ project adheres to [Semantic Versioning](http://semver.org/).

 ## [[v10.?.?](https://github.com/terraform-aws-modules/terraform-aws-eks/compare/v10.0.0...HEAD)] - 2020-xx-xx

- Write your awesome change here (GH-xxxx)
+- **Breaking:** renamed eks security group id outputs and additional security group id inputs (by @slimm609)


Please remove your change from the changelog.

barryib · 2020-03-17T18:23:08Z

local.tf

-  cluster_iam_role_name     = var.manage_cluster_iam_resources ? join("", aws_iam_role.cluster.*.name) : var.cluster_iam_role_name
-  cluster_iam_role_arn      = var.manage_cluster_iam_resources ? join("", aws_iam_role.cluster.*.arn) : join("", data.aws_iam_role.custom_cluster_iam_role.*.arn)
-  worker_security_group_id  = var.worker_create_security_group ? join("", aws_security_group.workers.*.id) : var.worker_security_group_id
+  additional_security_group_id = var.cluster_create_security_group ? join("", aws_security_group.cluster.*.id) : var.additional_security_group_id


Why rename this ?

same as below. The module is currently incorrect with the aws eks provider

barryib · 2020-03-17T18:23:44Z

variables.tf

@@ -19,8 +19,8 @@ variable "cluster_name" {
  type        = string
 }

-variable "cluster_security_group_id" {
-  description = "If provided, the EKS cluster will be attached to this security group. If not given, a security group will be created with necessary ingress/egress to work with the workers"
+variable "additional_security_group_id" {


Why rename this variable ? I don't get it.

Because they are 2 different things after 1.14 and you can not set the primary security group, only the additional security group id. The primary is created via eks automatically

https://www.terraform.io/docs/providers/aws/r/eks_cluster.html

vpc_config - Additional nested attributes:
cluster_security_group_id - The cluster security group that was created by Amazon EKS for the cluster.

You can not create this or even give an security group id for it, you can only get it from the output, so the additional_security_group_id is security_group_ids and what shows in the AWS console as additional security groups

dpiddockcmp · 2020-03-17T18:30:45Z

outputs.tf

-  description = "Security group ID attached to the EKS cluster."
-  value       = local.cluster_security_group_id
+  description = "Cluster security group ID attached to the EKS cluster."
+  value       = var.cluster_version >= 1.14 ? element(concat(aws_eks_cluster.this[*].vpc_config[0].cluster_security_group_id, list("")), 0) : local.additional_security_group_id


Please don't do this. It will make upgrade paths very scary.

this will have no effect on future upgrades. This problem only exists because node_groups are not backwards compatible. As soon at 1.16 is released, the possibility to hit this edge case no longer exists and this can be removed, because 1.13 support is dropped at the same time. This only exist because of the issue, which is also broken in the current release.

This variable has nothing to do with node groups. You are changing the value of an output based on user supplied input. It is a dangerous pattern.

Nothing is broken in the current release. It just lacks output of the new vpc_config.cluster_security_group_id value.

github-actions · 2022-11-18T02:31:15Z

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.

slimm609 mentioned this pull request Mar 15, 2020

cluster_create_security_group is mapped to Additional security groups in EKS #799

Closed

dpiddockcmp reviewed Mar 15, 2020

View reviewed changes

barryib reviewed Mar 17, 2020

View reviewed changes

dpiddockcmp reviewed Mar 17, 2020

View reviewed changes

Brian Davis added 7 commits March 18, 2020 06:41

fix cluster security group ids for output

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

GPG key ID: B5690EEEBB952194

Verified
Learn about vigilant mode

7e4476e

fix formatting

4ce4510

fix cluster.tf

39afca7

fix outputs

779c12a

fix docs and variables

23d3ea1

add min version for nodegroups

e7068ac

make outputs version deterministi

f9e5630

slimm609 force-pushed the eks_cluster_outputs branch from b46b22e to f9e5630 Compare March 18, 2020 10:49

barryib changed the title ~~fix cluster security group ids for output~~ fix: Cluster security group ids for output Mar 20, 2020

barryib changed the title ~~fix: Cluster security group ids for output~~ fix: Fix cluster security group ids in outputs Mar 20, 2020

Merge branch 'master' into eks_cluster_outputs

239ea9b

ratheo mentioned this pull request Mar 25, 2020

Get the security group of aws_eks_node_group as an output. #816

Closed

4 tasks

slimm609 closed this Apr 3, 2020

slimm609 mentioned this pull request Apr 11, 2020

fix: Add vpc_config.cluster_security_group output as primary cluster security group id #828

Merged

2 tasks

barryib mentioned this pull request Aug 26, 2020

fix: Create cluster_private_access security group rules when it should #981

Merged

1 task

github-actions bot locked as resolved and limited conversation to collaborators Nov 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix cluster security group ids in outputs #792

fix: Fix cluster security group ids in outputs #792

slimm609 commented Mar 13, 2020 •

edited

Loading

dpiddockcmp Mar 15, 2020

slimm609 Mar 16, 2020 •

edited

Loading

slimm609 Mar 16, 2020

dpiddockcmp Mar 15, 2020 •

edited

Loading

barryib Mar 17, 2020

barryib Mar 17, 2020

slimm609 Mar 17, 2020

barryib Mar 17, 2020

slimm609 Mar 17, 2020

slimm609 Mar 17, 2020

dpiddockcmp Mar 17, 2020

slimm609 Mar 17, 2020 •

edited

Loading

dpiddockcmp Mar 18, 2020

github-actions bot commented Nov 18, 2022

fix: Fix cluster security group ids in outputs #792

fix: Fix cluster security group ids in outputs #792

Conversation

slimm609 commented Mar 13, 2020 • edited Loading

PR o'clock

Description

Checklist

Choose a reason for hiding this comment

slimm609 Mar 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpiddockcmp Mar 15, 2020 • edited Loading

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

slimm609 Mar 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 18, 2022

slimm609 commented Mar 13, 2020 •

edited

Loading

slimm609 Mar 16, 2020 •

edited

Loading

dpiddockcmp Mar 15, 2020 •

edited

Loading

slimm609 Mar 17, 2020 •

edited

Loading