Fix for remote access and lifecycle issues #30

woz5999 · 2020-09-03T18:18:30Z

what

Create remote access security group for launch template when necessary to enable SSH access
Resolve some lifecycle dependency issues caused by create-before-destroy

why

Solves the issue where remote_access is not valid for node groups that specify launch templates. If you specify an ssh key or source security group ids with the current state of the module, it will throw an error and prevent node group creation.
Fixes some situations where Terraform would fail to apply a plan due to a dependency cycle

woz5999 · 2020-09-03T18:20:54Z

@Nuru @danjbh This PR has fixes some module issues that make it unusable for users trying to specify an ssh key and additional remote access security groups and also fixes a regression that breaks govcloud support.

i went with a preference for maintaining existing behavior and backwards compatibility and not modifying module usage, assuming that would be preferable by the maintainers, but I'm not married to anything specific except that the module should work again.

woz5999 · 2020-09-03T18:21:53Z

main.tf

+  )
+
+  security_group_ids = concat(
+    list(data.aws_eks_cluster.eks_cluster.vpc_config[0].cluster_security_group_id),


needed to enable communication with the EKS control plane

Is this always enough? Could there be more than one or some other security group that needs to be added?

woz5999 · 2020-09-03T18:22:40Z

main.tf

@@ -25,6 +25,20 @@ locals {
    id             = coalesce(var.launch_template_id, aws_launch_template.default[0].id)
    latest_version = coalesce(var.launch_template_version, aws_launch_template.default[0].latest_version)
  }
+
+  eks_node_group_resources = merge(aws_eks_node_group.default.*.resources,


this is some formatting to maintain backward compatibility with outputs.

woz5999 · 2020-09-03T18:23:29Z

main.tf

@@ -57,6 +71,14 @@ data "aws_iam_policy_document" "assume_role" {
  }
 }

+data "aws_subnet" "default" {
+  id = var.subnet_ids[0]


this is to prevent introducing an additional required variable. happy to modify if that's a more preferred route.

Do not reference this if it is not needed.

woz5999 · 2020-09-03T18:24:08Z

main.tf

@@ -170,14 +194,6 @@ resource "aws_eks_node_group" "default" {
    version = local.launch_template.latest_version
  }

-  dynamic "remote_access" {


this block is not supported when using launch templates. currently breaks the entire module if ec2_ssh_key or source_security_group_ids is specified

woz5999 · 2020-09-03T18:25:05Z

main.tf

+  type              = "ingress"
+
+  cidr_blocks = [
+    "0.0.0.0/0"


this emulates the default behavior of non-launch template node groups that specify a remote_access ssh key but no source security group.

Nuru

Thank you for this submission.

We added a lot in v0.11.0. In particular, it should solve your tag problem and restore backward compatibility with v0.8.0. v0.9.0 and v0.10.0 should have been marked pre-releases and giving corresponding semver numbers, but we are using a new automated release system that does not support pre-releases, for which I apologize.

We did not address SSH access when creating a launch template in v0.11.0 because a launch template is not needed for v0.8.0 functionality and because it seems like there are a lot of ways for that to go wrong, plus we did not have a use case or a test for it. If you do not care about propagating tags to instances and EBS volumes, then you do not need a launch template and v0.11.0 should do what you need. Otherwise, please review 0.11 and the comments here, rebase and revise the PR, and point us to any documentation you can to assure us that the security group configuration for the launch template is correct for all the various use cases.

Nuru · 2020-09-07T02:46:05Z

main.tf

+  )
+
+  # Tagging an elastic gpu on create is not yet supported in govcloud
+  tag_spec = compact(["instance", "volume", join("", data.aws_partition.current.*.partition) == "aws-us-gov" ? "" : "elastic-gpu"])


We do not want to hard code exceptions like this. In v0.11.0 the default is no tagging and users can specify which resources to tag, so this should not longer be an issue for you.

Nuru · 2020-09-07T02:47:59Z

main.tf

@@ -25,6 +25,22 @@ locals {
    id             = coalesce(var.launch_template_id, aws_launch_template.default[0].id)
    latest_version = coalesce(var.launch_template_version, aws_launch_template.default[0].latest_version)
  }
+
+  eks_node_group_resources = [for resource in aws_eks_node_group.default.*.resources :


Rather than change the structure of the resources output, add a new output for the new info, if it is really needed, which I am not sure about.

the sg id is needed as an output for users that might want to add rules to it outside the module. i was appending to resources to match prior behavior. happy to put it as its own output.
it's important to have as an output for users that might want to add additional rules outside the module. if you're ok breaking backward compatibility, this can all be updated and users can just provide an sg id to the module to attach to the launch template and bypass most of the logic in this PR

Nuru · 2020-09-07T02:56:03Z

main.tf

+  )
+
+  security_group_ids = concat(
+    list(data.aws_eks_cluster.eks_cluster.vpc_config[0].cluster_security_group_id),


Is this always enough? Could there be more than one or some other security group that needs to be added?

Nuru · 2020-09-07T02:56:35Z

main.tf

@@ -57,6 +71,14 @@ data "aws_iam_policy_document" "assume_role" {
  }
 }

+data "aws_subnet" "default" {
+  id = var.subnet_ids[0]


Do not reference this if it is not needed.

Nuru · 2020-09-07T02:57:26Z

main.tf

+data "aws_eks_cluster" "eks_cluster" {
+  name = var.cluster_name
+}
+


This is already in v0.11.0 with appropriate conditions, although now you need to add another condition.

main.tf

Nuru · 2020-09-07T03:04:50Z

main.tf

+  key_name               = var.ec2_ssh_key
+  vpc_security_group_ids = local.security_group_ids


In case we get the security group list wrong, I do not want to specify it if we do not have to.

Suggested change

key_name = var.ec2_ssh_key

vpc_security_group_ids = local.security_group_ids

key_name = local.ec2_ssh_key

vpc_security_group_ids = local.ec2_ssh_key != null ? local.security_group_ids : null

Nuru · 2020-09-07T03:05:16Z

main.tf


  dynamic "tag_specifications" {
-    for_each = ["instance", "volume", "elastic-gpu"]
+    for_each = local.tag_spec


This is handled in v0.11.0

Co-authored-by: Nuru <Nuru@users.noreply.github.com>

Nuru · 2020-09-08T00:32:22Z

@woz5999 Note that #32 is open and likely to get merged very soon, so maybe wait until that is released to rebase your PR. Please close your PR if you are satisfied with using v0.11.1 when it comes out.

woz5999 · 2020-09-09T22:28:01Z

@Nuru modified the PR such that it only adds the ability for the users to attach additional security groups to the generated launch template. it also fixes a bug wherein remote_access and launch_template are mutually exclusive on the eks_node_group resource.

https://docs.aws.amazon.com/eks/latest/userguide/sec-group-reqs.html https://docs.aws.amazon.com/eks/latest/userguide/launch-templates.html

Nuru · 2020-09-11T23:50:10Z

/test all

Nuru · 2020-09-11T23:53:18Z

/test all

woz5999 added 2 commits September 3, 2020 11:03

fixes

178522a

add cluster security group

d9891a4

woz5999 requested review from a team as code owners September 3, 2020 18:18

woz5999 requested review from Makeshift and adamcrews and removed request for a team September 3, 2020 18:18

woz5999 commented Sep 3, 2020

View reviewed changes

woz5999 marked this pull request as draft September 3, 2020 18:30

fixes

e7d95d4

woz5999 marked this pull request as ready for review September 3, 2020 20:51

Nuru requested changes Sep 7, 2020

View reviewed changes

Update main.tf

95987bd

Co-authored-by: Nuru <Nuru@users.noreply.github.com>

woz5999 marked this pull request as draft September 8, 2020 18:11

woz5999 added 8 commits September 8, 2020 14:00

Merge remote-tracking branch 'upstream/master'

67a1c56

remove remote access sg

9a15c80

updates

8790ea8

simplify

8a8f5ea

don't specify remote_access for launch templates

d230b68

Merge branch 'fixed'

3be9203

cleanup

a52fb7e

Update variables.tf

e0fa427

woz5999 marked this pull request as ready for review September 9, 2020 19:44

woz5999 and others added 6 commits September 9, 2020 16:15

pass ssh key to launch_template

4ea1a45

var name update

9bdd14f

don't support custom sgs

d8bce3f

recreate default eks node group remote access behavior

ef60992

https://docs.aws.amazon.com/eks/latest/userguide/sec-group-reqs.html https://docs.aws.amazon.com/eks/latest/userguide/launch-templates.html

sg name update

d30e60c

Restore SSH access, fix some destroy issues with CBD

ee2aa7f

Nuru added bugfix Change that restores intended behavior terraform/0.13 Module requires Terraform 0.13 or later labels Sep 11, 2020

Nuru approved these changes Sep 12, 2020

View reviewed changes

Nuru changed the title ~~Fix for remote access and govcloud~~ Fix for remote access and lifecycle issues Sep 12, 2020

Nuru merged commit 863d4ab into cloudposse:master Sep 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for remote access and lifecycle issues #30

Fix for remote access and lifecycle issues #30

woz5999 commented Sep 3, 2020 •

edited by Nuru

Loading

woz5999 commented Sep 3, 2020 •

edited

Loading

woz5999 Sep 3, 2020

Nuru Sep 7, 2020

woz5999 Sep 3, 2020

woz5999 Sep 3, 2020

Nuru Sep 7, 2020

woz5999 Sep 3, 2020

woz5999 Sep 3, 2020

Nuru left a comment •

edited

Loading

Nuru Sep 7, 2020

Nuru Sep 7, 2020

woz5999 Sep 7, 2020

Nuru Sep 7, 2020

Nuru Sep 7, 2020

Nuru Sep 7, 2020

Nuru Sep 7, 2020

Nuru Sep 7, 2020

Nuru commented Sep 8, 2020

woz5999 commented Sep 9, 2020

Nuru commented Sep 11, 2020

Nuru commented Sep 11, 2020

		key_name = var.ec2_ssh_key
		vpc_security_group_ids = local.security_group_ids

Fix for remote access and lifecycle issues #30

Fix for remote access and lifecycle issues #30

Conversation

woz5999 commented Sep 3, 2020 • edited by Nuru Loading

what

why

woz5999 commented Sep 3, 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

Nuru left a comment • 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

Choose a reason for hiding this comment

Nuru commented Sep 8, 2020

woz5999 commented Sep 9, 2020

Nuru commented Sep 11, 2020

Nuru commented Sep 11, 2020

woz5999 commented Sep 3, 2020 •

edited by Nuru

Loading

woz5999 commented Sep 3, 2020 •

edited

Loading

Nuru left a comment •

edited

Loading