#pr-reviews (2020-09)

Pull Request Reviews for Cloud Posse Projects

2020-09-21

Frank avatar
Frank
feat: Upgrade to Terraform 0.12 by syphernl · Pull Request #37 · cloudposse/terraform-aws-cloudfront-cdn

what Synced up repo config with that of other CP repos (chatops, releasing, go, actions etc) Update any version pinning Added a type to all variables Modified ordered_cache_behavior to match that …

feat: introduce enabled flag by syphernl · Pull Request #38 · cloudposse/terraform-aws-cloudfront-cdn

what renamed enabled to distribution_enabled (breaking change) added a new enabled flag why Most/all other CloudPosse modules have an enabled flag to prevent resources from being created. This m…

feat: add customizations to the bastion instance by syphernl · Pull Request #34 · cloudposse/terraform-aws-ec2-bastion-server

what Added the ability to encrypt the root block device, off by default. Added the ability to change the size of the root block device Added the ability to change the HTTP Metadata endpoint settin…

Gowiem avatar
Gowiem

@ Looking into these.

  1. ec2-bastion-server module — approved. Good stuff, thanks!
  2. I’d love to get that 0.12 PR merged. I’m trying to run the tests locally, but only have a few more minutes before my standup. I’ll try to get back to it before EOD.
feat: Upgrade to Terraform 0.12 by syphernl · Pull Request #37 · cloudposse/terraform-aws-cloudfront-cdn

what Synced up repo config with that of other CP repos (chatops, releasing, go, actions etc) Update any version pinning Added a type to all variables Modified ordered_cache_behavior to match that …

feat: introduce enabled flag by syphernl · Pull Request #38 · cloudposse/terraform-aws-cloudfront-cdn

what renamed enabled to distribution_enabled (breaking change) added a new enabled flag why Most/all other CloudPosse modules have an enabled flag to prevent resources from being created. This m…

feat: add customizations to the bastion instance by syphernl · Pull Request #34 · cloudposse/terraform-aws-ec2-bastion-server

what Added the ability to encrypt the root block device, off by default. Added the ability to change the size of the root block device Added the ability to change the HTTP Metadata endpoint settin…

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Andriy Knysh (Cloud Posse)

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

i’m going to make fixes to the cloudfront-cdn module

Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

first, we need to open a PR to add the latest GH actions

2020-09-18

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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

Release v0.12.0: Remove autoscaler permissions from worker role · cloudposse/terraform-aws-eks-node-group

Potentially breaking changes Terraform 0.13.3 or later required This release requires Terraform 0.13.3 or later because it is affected by these bugs that are fixed in 0.13.3: hashicorp/terraform#2…

:--1:1

2020-09-16

Alex Taylor avatar
Alex Taylor
Make adding all egress rule to the ECS security group optional by taylora433 · Pull Request #75 · cloudposse/terraform-aws-ecs-alb-service-task

what Allow all egress security group rule to be optional (Default to enabled for backwards compatibility) why Security and compliance rules means we cannot simply allow everything egress Need to…

2020-09-15

Jeff Wozniak avatar
Jeff Wozniak

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

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

Yes, I believe we’ve decide to keep 2.0 pinning and remove the region parameter.

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Jeremy (Cloud Posse)

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

Yes, we found that the region was not an input parameter for the S3 bucket, rather it as extracted from the provider region, which is what happens by default in 2.0 and a the only option in 3.0, so removing it should not change any behavior except make it compatible with 3.0

Jeff Wozniak avatar
Jeff Wozniak

awesome

Jeff Wozniak avatar
Jeff Wozniak

let me know if you need me to make any changes to the open PR

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

We have quite a few modules that depend on buckets (log-bucket, tfstate-backend, s3-website, etc), so if there’s any you care about and want to expedite - go ahead and open the PR for it.

Jeff Wozniak avatar
Jeff Wozniak

the main need right now is the s3-bucket module itself

Jeff Wozniak avatar
Jeff Wozniak

i’ll probably submit one for tfstate too once s3 is updated

:--1:1
Jeff Wozniak avatar
Jeff Wozniak

i guess tfstate isn’t using the s3 module; it’s just creating the resource directly. here’s a PR for that project: https://github.com/cloudposse/terraform-aws-tfstate-backend/pull/63

use minimum versions for provider pinning and support 3.0 s3 resources by woz5999 · Pull Request #63 · cloudposse/terraform-aws-tfstate-backend

what set minimum versions for providers without pinning to a specific major version why the current method makes it very difficult to maintain or upgrade consistent provider versions within a proje…

Jeff Wozniak avatar
Jeff Wozniak
use minimum versions for provider pinning by woz5999 · Pull Request #51 · cloudposse/terraform-aws-s3-bucket

what set minimum versions for providers without pinning to a specific major version why the current method makes it very difficult to maintain or upgrade consistent provider versions within a proje…

RB avatar
Enable dynamodb switch by nitrocode · Pull Request #65 · cloudposse/terraform-aws-tfstate-backend

what Adds a flag to enable/disable dynamodb why Save that money references Closes #64

:--1:1
PePe avatar

so it did not autorelease?

Enable dynamodb switch by nitrocode · Pull Request #65 · cloudposse/terraform-aws-tfstate-backend

what Adds a flag to enable/disable dynamodb why Save that money references Closes #64

RB avatar

nah it didnt for some reason so i had to cut a release

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

(we haven’t yet rolled out auto-release.yml workflow everywhere - just no time yet)

PePe avatar

ohhh I c ok, no worries

2020-09-11

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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.

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

@

Jeff Wozniak avatar
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

Jeff Wozniak avatar
Jeff Wozniak
Fix for remote access and govcloud by woz5999 · Pull Request #30 · cloudposse/terraform-aws-eks-node-group

what Create remote access security group for launch template. Don't tag elastic gpu in govcloud why Solves the issue where remote_access is not valid for node groups that specify launch templ…

Jeff Wozniak avatar
Jeff Wozniak

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

Jeff Wozniak avatar
Jeff Wozniak

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

Fix for remote access and govcloud by woz5999 · Pull Request #30 · cloudposse/terraform-aws-eks-node-group

what Create remote access security group for launch template. Don't tag elastic gpu in govcloud why Solves the issue where remote_access is not valid for node groups that specify launch templ…

Jeff Wozniak avatar
Jeff Wozniak
RemoteAccessConfig - Amazon Elastic Kubernetes Service

An object representing the remote access configuration for the managed node group.

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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.

:--1:1
Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

@ 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?

Jeff Wozniak avatar
Jeff Wozniak

go for it

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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

Jeff Wozniak avatar
Jeff Wozniak

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

Jeff Wozniak avatar
Jeff Wozniak

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

Jeff Wozniak avatar
Jeff Wozniak

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.

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

That’s what documentation is for

Jeff Wozniak avatar
Jeff Wozniak

works for me. changes look good.

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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.

Jeff Wozniak avatar
Jeff Wozniak

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

Jeff Wozniak avatar
Jeff Wozniak

ssh is working

Jeff Wozniak avatar
Jeff Wozniak

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

use minimum versions for provider pinning by woz5999 · Pull Request #25 · cloudposse/terraform-aws-route53-alias

what set minimum versions for providers without pinning to a specific major version why the current method makes it very difficult to maintain or upgrade consistent provider versions within a proje…

Jeff Wozniak avatar
Jeff Wozniak

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

use minimum versions for provider pinning by woz5999 · Pull Request #100 · cloudposse/terraform-aws-cloudfront-s3-cdn

what set minimum versions for providers without pinning to a specific major version why the current method makes it very difficult to maintain or upgrade consistent provider versions within a proje…

Jeff Wozniak avatar
Jeff Wozniak

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

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

Expect the new route53-alias version to be 0.8.2

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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.

2020-09-10

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

@Jeremy (Cloud Posse)

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

@ 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.

Jeff Wozniak avatar
Jeff Wozniak

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.

Jeff Wozniak avatar
Jeff Wozniak

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

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

#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

Jeff Wozniak avatar
Jeff Wozniak

i think my current changes meet that goal

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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?

Jeff Wozniak avatar
Jeff Wozniak

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

Jeff Wozniak avatar
Jeff Wozniak

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

Jeff Wozniak avatar
Jeff Wozniak

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

Jeff Wozniak avatar
Jeff Wozniak

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.

Jeff Wozniak avatar
Jeff Wozniak

happy to do either, but wanted to suggest that alternative

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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.

Jeff Wozniak avatar
Jeff Wozniak

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

Jeff Wozniak avatar
Jeff Wozniak

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

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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.

Jeff Wozniak avatar
Jeff Wozniak

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

Jeff Wozniak avatar
Jeff Wozniak

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.

Jeff Wozniak avatar
Jeff Wozniak

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.

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

You can break the launch template out into a [launch-template.tf> file like we did with <http://ami.tf|ami.tf](http://launch\-template\.tf)

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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

:--1:1
Jeff Wozniak avatar
Jeff Wozniak

@Jeremy (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

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

@ 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?

Jeff Wozniak avatar
Jeff Wozniak
% AWS_PROFILE=dev-gov aws ec2 describe-availability-zones|jq -r '.AvailabilityZones[].ZoneId'
usgw1-az2
usgw1-az1
usgw1-az3
1
Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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.

:--1:1
Jeff Wozniak avatar
Jeff Wozniak

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

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

Cool!

2020-09-09

Erik Osterman (Cloud Posse) avatar
Erik Osterman (Cloud Posse)

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

Fix for remote access and govcloud by woz5999 · Pull Request #30 · cloudposse/terraform-aws-eks-node-group

what Create remote access security group for launch template. Don&#39;t tag elastic gpu in govcloud why Solves the issue where remote_access is not valid for node groups that specify launch templ…

Jeff Wozniak avatar
Jeff Wozniak
06:03:48 PM

@ has joined the channel

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)
06:03:49 PM

@Jeremy (Cloud Posse) has joined the channel

Jeff Wozniak avatar
Jeff Wozniak

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

:--1:1
Jeff Wozniak avatar
Jeff Wozniak

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.

Jeff Wozniak avatar
Jeff Wozniak

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

Jeremy (Cloud Posse) avatar
Jeremy (Cloud Posse)

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).

Jeff Wozniak avatar
Jeff Wozniak

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.

Jeff Wozniak avatar
Jeff Wozniak

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.

2020-09-04

Eric Berg avatar
Eric Berg

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

Added ignore_members and delete_default_resource attributes to teams by bergbrains · Pull Request #8 · cloudposse/terraform-opsgenie-incident-management

what This PR adds the following arguments to the config module&#39;s team functionality: delete_default_resources: true ignore_members why delete_default_resources provides for wanting to suppre…

1
1

2020-09-03

Alex S avatar
Alex S

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

Update access_logs module for TF 0.13 by asmithwp · Pull Request #6 · cloudposse/terraform-aws-nlb

what Fix for TF 0.13 why The access_logs module only supports TF 0.12 references Issue #5

1
Gowiem avatar
Gowiem

Checking it out. As long as tests pass then we’ll get it shipped. Thanks for note!

Update access_logs module for TF 0.13 by asmithwp · Pull Request #6 · cloudposse/terraform-aws-nlb

what Fix for TF 0.13 why The access_logs module only supports TF 0.12 references Issue #5

Alex S avatar
Alex S

Cheers, no worries

    keyboard_arrow_up