#pr-reviews - September 2020

https://github.com/cloudposse/terraform-aws-nlb/pull/6 hey - updated a module to work with TF 0.13 x-post #terraform-aws-modules

Added ignore_members and delete_default_resources to Opsgenie module.https://github.com/cloudposse/terraform-opsgenie-incident-management/pull/8/files
@Dan Meyers

@Jeff Wozniak have you had a chance to review @Jeremy G (Cloud Posse)’s comments? https://github.com/cloudposse/terraform-aws-eks-node-group/pull/30

@Jeff Wozniak has joined the channel

@Jeremy G (Cloud Posse) has joined the channel

i have. i'm working on modifying it currently.

my new plan is to just provide the ability for users to specify any additional sg ids they want to associate with the launch template.

i think it addresses most of the issues raised and allows for customizing rules without dramatically increasing the complexity of the module.

My worry is correctly duplicating the default behavior of adding the security groups needed for the node group to function properly as part of the cluster (i.e. communicate with the control plane and other node groups in the cluster).

has the current module-generated launch template functionality been tested to see if it actually creates a functional cluster? when i attempted to use it unmodified, it resulted in apply errors (remote_access and launch_template are mutually exclusive on the aws_eks_node_group resource) and i don't see that it is currently doing anything at all with security groups.

my updated PR adds the ability for the user to supply a list of security groups to apply to the generated launch template. this puts the responsibility on the user to ensure that the proper rules are in place rather than forcing the module to attempt to duplicate the default aws_eks_node_group remote_access security group behavior.

@Jeremy G (Cloud Posse) ☝️

@Jeff Wozniak The current template works when NOT setting SSH keys at all, both using the generated launch template and not. It is expected to FAIL WITH ERRORS when setting SSH keys and generating a launch template, because I considered that better than succeeding but not enabling SSH access.

the updated PR now sets the ssh key for the generated launch template if the ssh key var is set. it's no longer doing any active security group management.

it's up to you which method you prefer: leaving it to the user to specify an sg with the appropriate rules or i can try to generate and attach one, but that opens back up all the previous questions surrounding the various use cases

#1 priority is backward compatibility. We do not want to break anything that worked in v0.8.0 and preferably also not break anything that works in v0.11.1

i think my current changes meet that goal

Can you do both: Automatically add the default SG if the user only supplies remote access SGs and give the user the option to override the default SG (with a list of SGs) in a separate variable?

item 1: definitely. do you also want me to emulate the default aws behavior where if you specify ssh key but not source sgs it opens 22 to the world? i've got no strong preference and am happy to do either

item 2: i'd propose supporting both. i.e. if remote access SGs are specified, always create the default group. and also allow the user to specify a list of additional SGs to attach

so default SG + additional SGs, rather than either/or

the reasoning is that as a user, i think it's cleaner to provide your own SG if you want to add custom rules, rather than getting the default SG id from the outputs and attaching rules after the fact.

happy to do either, but wanted to suggest that alternative

The thing I am concerned about is that in many cases we are creating a launch template just to enable some other feature and the user really does not care or need to know that we are creating a launch template. I do not want them to enable "propagate tags" and then all of a sudden find they have to supply a security group.

definitely. it would be solely for the purpose of providing custom rules.

and i'm thinking along the same lines. i don't want someone who needs custom rules to suddenly need to also provide the necessary ssh remote access rules as well. so i think we're on the same page. i'll push some changes later today

My thinking is a bit clouded by the fact that I do not have a solid grasp on how the default EKS security groups work. If we can reliably get the default groups when needed (and not get them when not needed) then we do not need to give the user the ability to add additional security groups beyond the remote access groups. At the point they need that feature, they can create and supply their own launch template, which is the big escape hatch.

the default is: if you just supply ssh key, it creates an sg with 22 open to the world. if you provide a key and remote access SG ids, it creates an sg with 22 open to those ids only

i'll mess with it a bit this afternoon. if it becomes too much of a pain, i'll just scrap the entire thing, use 0.11.1 as is and supply my own template. the module is getting extremely complex as is.

it might be worth considering breaking all of the launch template logic into something like a terraform-aws-eks-launch-template module to abstract all of the launch template conditionals and use that within the node-group module for generating the default template.

You can break the launch template out into a launch-template.tf file like we did with ami.tf

Also, @Jeff Wozniak, if you do not mind, please change the name of the variable from userdata_override to userdata_override_base64. Thanks.

@Jeremy G (Cloud Posse) how would you feel if i abandon the ability to specify additional SGs? if the default isn't good enough, then users can just specify their own template

@Jeff Wozniak BTW, can you tell me what US GovCloud AZ IDs look like? For example, us-west-2 they look like usw2-az1, usw2-az2, etc. Are the GovCloud ones like usgw1-az1?

As to your question, Users need to be able to specify remote access security groups to go with SSH keys. They do not need to be able to specify any additional security groups.

cool. i'm testing my changes right now reproducing the default eks node group behavior. i'm going to rip out the additional sg stuff then i think i'll be ready for you to review

Cool!

I'm really tired so maybe have this wrong, but I'm pretty sure that you have defined the security groups incorrectly. The rules need to have the from port set to any. Where did you come up with the name "eks-${var.cluster_name}-${module.label.id}-remote-access"? I would think ${module.label.id}-remote-access would be enough.

@Jeff Wozniak ☝️

thanks i didn't see that this morning. not picky on the name, happy to make that change. let me doublecheck on that SG comment

you're talking about here? https://github.com/cloudposse/terraform-aws-eks-node-group/pull/30/files#diff-f6fad48a4c11319ef2d8fd46ebccf233R13

this is emulating the default non-lt node group remote_access behavior which just opens up ssh.

here, i'm assigning the cluster sg to the node group launch template, which enables communication between workers/control plane and workers/workers: https://github.com/cloudposse/terraform-aws-eks-node-group/pull/30/files#diff-7a370d8342e7203b805911c92454f0f4R51

here's the docs on that default behavior: https://docs.aws.amazon.com/eks/latest/APIReference/API_RemoteAccessConfig.html

Like I said, I was tired. When I'm tired, I think from_port means source_port and to_port means destination_port, but of course they mean min_port and max_port of a port range.

@Jeff Wozniak I think what you have is good, but I found other bugs. Is it OK with you if I make some changes directly on your PR?

go for it

OK, I pushed up changes. LMK if you have any comments/concerns/suggestions.

sure. looking now. the only comment i'd make so far is in the naming of the output eks_node_group_remote_access_security_group_id

it's kind of a tricky scenario because that's only valid if launch template is used. otherwise, for non lt node groups, it gets returned deep in the node group resource outputs

which is to say, i've got no issue with the rename, but it may be confusing to users if they're expecting that to always be populated.

That's what documentation is for 😁

works for me. changes look good.

I could only test it to the point of verifying the security groups looked good. I don't have any nodes with public IPs, so I could not actually test SSH access. If you would like to do that, please go ahead.

sure. i've got the changes running in an env already

ssh is working

since you're here, can i bug you to review two more fast ones? https://github.com/cloudposse/terraform-aws-route53-alias/pull/25

and once that's merged and has a new version: https://github.com/cloudposse/terraform-aws-cloudfront-s3-cdn/pull/100

i've gotta run. have a great weekend and thanks for the help

Expect the new route53-alias version to be 0.8.2

route53-alias changes approved and release. cloudfront-s3-cdn module is failing tests, likely a breaking change between AWS provider v2.x and v3.x. I do not have time to get into it.

is there a plan on what to do about the breaking changes in 3.0 for the s3 bucket module?

https://github.com/cloudposse/terraform-aws-s3-bucket/pull/51 is ready for review

Make allow all egress ECS rule optional: https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/75

@Jeff Wozniak FYI, terraform-aws-eks-node-group v0.12.0 has been released . LMK if you see any issues or have improvements for it.

Any chance to get a review/reply on my open PR's?
• https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/37
• https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/38
• https://github.com/cloudposse/terraform-aws-ec2-bastion-server/pull/34
Thanks!

Not my PR but could anyone get eyes on this? Cheers
https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/76

Hi, could somebody review my PR? ❤️ It should be a relatively small one, fixing the outputs of the aws-terraform-ec2-bastion-server module: https://github.com/cloudposse/terraform-aws-ec2-bastion-server/pull/36

Can these PR's get some love too?
• https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/37
• https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/38
Would really like to get these ones off our board 🙂 Thanks!

hey @Frank - looks like the terratest is failling on #37