#pr-reviews (2020-09)
Pull Request Reviews for Cloud Posse Projects
2020-09-03
https://github.com/cloudposse/terraform-aws-nlb/pull/6 hey - updated a module to work with TF 0.13 x-post #terraform-aws-modules
Checking it out. As long as tests pass then we’ll get it shipped. Thanks for note!
Cheers, no worries
2020-09-04
Added ignore_members and delete_default_resources to Opsgenie module.https://github.com/cloudposse/terraform-opsgenie-incident-management/pull/8/files
@Dan Meyers
2020-09-09
@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
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.
2020-09-10
@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](http://launch-template.tf) file like we did with [ami.tf](http://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?
% AWS_PROFILE=dev-gov aws ec2 describe-availability-zones|jq -r '.AvailabilityZones[].ZoneId'
usgw1-az2
usgw1-az1
usgw1-az3
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!
2020-09-11
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
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.
2020-09-15
is there a plan on what to do about the breaking changes in 3.0 for the s3 bucket module?
Yes, I believe we’ve decide to keep 2.0 pinning and remove the region parameter.
@Jeremy G (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
awesome
let me know if you need me to make any changes to the open PR
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.
the main need right now is the s3-bucket module itself
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
https://github.com/cloudposse/terraform-aws-s3-bucket/pull/51 is ready for review
so it did not autorelease?
nah it didnt for some reason so i had to cut a release
(we haven’t yet rolled out auto-release.yml workflow everywhere - just no time yet)
ohhh I c ok, no worries
2020-09-16
Make allow all egress ECS rule optional: https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/75
2020-09-18
@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.
2020-09-21
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!
@Frank Looking into these.
- ec2-bastion-server module — approved. Good stuff, thanks!
- 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.
@Andriy Knysh (Cloud Posse)
i’m going to make fixes to the cloudfront-cdn module
first, we need to open a PR to add the latest GH actions
2020-09-24
Not my PR but could anyone get eyes on this? Cheers https://github.com/cloudposse/terraform-aws-ecs-alb-service-task/pull/76
@Alex S Released as 0.40.1 — Thanks for the bump!
Cheers!
2020-09-25
2020-09-29
Hi, could somebody review my PR? :heart: 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
@Tyrone Meijn Released as 0.8.0. Thanks for the contribution!
dang you’re fast @Matt Gowie
Haha I try to jump on new messages in #pr-reviews quickly. Originally I was combing through repos one at a time to try and get open PRs merged one at a time, but haven’t done that in a while. Figure trying to help folks who are asking for a merge is the least I can do.
2020-09-30
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
you should be able to run tests locally by running make -C test/src docker/test
assumes you have AWS_* exported
also looks like bats is failing
make -C test all
I think it should be fine now. terraform validate was not liking the examples
(only @contributors can run /test all for security reasons)
I’m running it now
Good news is bats is now passing
Bad news is the terratests are still failing
./examples_basic_test.go:45:2: undefined: assert
./examples_basic_test.go:50:2: undefined: assert
@Andriy Knysh (Cloud Posse) can you take a quick look? probably something really easy.
will do
@Frank the go test is missing some imports
we also convert it to use go modules
can you please run the following commands:
make init
make github/init
cd test/src/
go mod init github.com/cloudposse/$(cd ../../ && basename `pwd`)
go mod tidy
git rm Gopkg.*
git add go.*
cp ../../../terraform-null-label/test/src/Makefile
also please update this Makefile https://github.com/syphernl/terraform-aws-cloudfront-cdn/blob/feat/terraform_0.12/test/src/Makefile
to this:
and please rename the go file to this https://github.com/cloudposse/terraform-aws-datadog-integration/blob/master/test/src/examples_complete_test.go
if you are having problems with that, I can get your PR and update it (if I can push into your repo), let me know
@Andriy Knysh (Cloud Posse) I have made the requested changes in my branch: https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/37/commits/f64cc96371a7032c1ce52bacd404ca83739bde73
thanks, let’s run tests
@Frank please see the comments in the PR. And thanks a lot for all the work
(you’re almost across the finish line! just need to run a few commands to get the tests to pass)
Hopefully the tests are working now then, I’ve applied @Andriy Knysh (Cloud Posse)’s commands. It’s a bit tricky to get the tests to pass if you cannot execute/trigger them by yourself
@Frank thanks again for all the work
one more things
./examples_complete_test.go:27:2: testNamePrefix declared and not used
go will not compile if it has unused vars
to make it faster, you can run tests locally
go test -v -timeout 60m -run TestExamplesComplete
at least you’ll see if the terratest compiles
if you provide a default AWS profile, it will even try to provision the example on your AWS account (if you need to go all the way )
Gotcha. I removed the unused var and also saw a notice about something in the fixtures that’s unused so got rid of that too. It seems terratest is working fine now, atleast locally. I didn’t go as far as actually provisioning it though.
@Andriy Knysh (Cloud Posse) can you help get this PR over the line? https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/38
ok