#pr-reviews (2022-08)

Pull Request Reviews for Cloud Posse Projects

2022-08-01

Cody Moore avatar
Cody Moore

Hey crew, a buddy and I are working on some EKS changes, would love a review on this whenever someone gets a chance: https://github.com/cloudposse/terraform-aws-eks-node-group/pull/126 (needs another reviewer besides me )

What did I do

• Add detailed monitoring flag to the launch template of EC2 nodes

Why did I do this

• Some compliance tools will flag nodes used by this module because they don’t have detailed monitoring. This also allows metrics to be reported every minute as opposed to five minute intervals

Helpful references

More AWS Documentation

1
Cody Moore avatar
Cody Moore

If I can get a team member approval from @cloudposse-team that would be appreciated

What did I do

• Add detailed monitoring flag to the launch template of EC2 nodes

Why did I do this

• Some compliance tools will flag nodes used by this module because they don’t have detailed monitoring. This also allows metrics to be reported every minute as opposed to five minute intervals

Helpful references

More AWS Documentation

Jeremy G (Cloud Posse) avatar
Jeremy G (Cloud Posse)

Approved, merged, released as v2.6.0.

1

2022-08-02

roth.andy avatar
roth.andy

Useful for deploying to GovCloud since their partition there is “aws-us-gov” instead of just “aws”

what

Support other AWS partitions by templatizing ARNs that are currently hard coded

why

So the module can be used in other AWS partitions like GovCloud

references

Closes #92

1

2022-08-04

Sam Skynner avatar
Sam Skynner

https://github.com/cloudposse/terraform-aws-ssm-tls-self-signed-cert/pull/14 If possible This breaks a number of other modules right now which are dependent on this

1
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

Cc @Andriy Knysh (Cloud Posse) @Jeremy G (Cloud Posse)

1
Jeremy G (Cloud Posse) avatar
Jeremy G (Cloud Posse)

Released as v1.0.0

Sam Skynner avatar
Sam Skynner

thank you so much for the quick reply

SergeiV avatar
SergeiV

I think vpn module will still fail because version for ssm-tls module is pinned to 0.5 : https://github.com/cloudposse/terraform-aws-ec2-client-vpn/blob/master/main.tf#L25

  version = "0.5.0"
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

this release may take about 20 min or so to be added to the registry

SergeiV avatar
SergeiV

excellent, thank you very much !

2022-08-08

Oscar Jara avatar
Oscar Jara

Hi all. I think I found a bug in the terraform-aws-ecs-web-app module. EFS volumes are not being correctly set in the resulting task definition by the the terraform-aws-ecs-alb-service-task Draft PR here. Who can I discuss this with? Thanks!

what

Creates a separated variable for EFS volumes.

why

• The way terraform-aws-ecs-web-app defines and uses the EFS volumes with the terraform-aws-ecs-alb-service-task module is not working correctly and the EFS volume is not being set in the resulting task definition. • By separating EFS volumes and Docker volumes they can be assigned to the docker_volumes and efs_volumes as expected by the module.

mihai avatar

looking forward to this

what

Creates a separated variable for EFS volumes.

why

• The way terraform-aws-ecs-web-app defines and uses the EFS volumes with the terraform-aws-ecs-alb-service-task module is not working correctly and the EFS volume is not being set in the resulting task definition. • By separating EFS volumes and Docker volumes they can be assigned to the docker_volumes and efs_volumes as expected by the module.

Oscar Jara avatar
Oscar Jara

This PR is ready for review now

Mo Omer avatar
Mo Omer

I joined this slack to mention that I also encountered this issue, and the PR would be helpful!

mihai avatar

could we get another test run on this please @jose.amengual?

1
mihai avatar

thanks for that. and for review?

2022-08-09

2022-08-10

2022-08-16

matharoo avatar
matharoo

Hi, can i please get this merged https://github.com/cloudposse/terraform-aws-rds-cluster/pull/149, the required changes have been made and tests have passed. Thanks

what

• For a multi a-z rds cluster skip creating aws_rds_cluster_instance resource when engine type is NOT aurora, aurora-mysql, aurora-postgresql • AWS provider has a bug that is causing the terraform apply to fail due to a missing rebooting state. I have a PR merged with terraform-aws-provider that fixes it and will be included in the next release 4.23.0. • hashicorp/terraform-provider-aws#25718

why

• Prevent terraform from crashing when creating a non-aurora multi a-z cluster. • aws provider update for fixing rebooting state when creating multi a-z cluster.

references

• closes #148hashicorp/terraform-provider-aws#25718

Isaac Campbell avatar
Isaac Campbell

what

• Added list of objects, which are Security Rule Ingress Definitions.

why

• Sometimes I need to add security groups to access my EKS workers, such as other EC2 instances on a variety of ports. Same with RDS instances.

references

• Link to any supporting github issues or helpful documentation to add some context (e.g. stackoverflow). • Use closes #123, if this PR closes a GitHub issue #123

2022-08-18

Tommy avatar

what

• added acm_certificate_validation.certification_arn output

why

• to avoid alb module can’t create listener because of not validated cert • use this output as certification arn in alb module

references

#58 • closes #58

Tommy avatar

Did I something wrong? I forgot to squash the PR, it’s just added an output…

what

• added acm_certificate_validation.certification_arn output

why

• to avoid alb module can’t create listener because of not validated cert • use this output as certification arn in alb module

references

#58 • closes #58

Tommy avatar

another PR needs some progress, someone can help? https://github.com/cloudposse/terraform-aws-ec2-instance/pull/117

what

• Added support for io2 and gp3 volumes

why

• io2 and gp3 are new more performant volumes therefore they should be supported

references

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance#ebs-ephemeral-and-root-block-devices • Closes #114

Jeremy (UnderGrid Network Services) avatar
Jeremy (UnderGrid Network Services)

I can say I see the iops issue with using gp3 and this module all the time in our environment… presents a constant change whenever applied because of it

what

• Added support for io2 and gp3 volumes

why

• io2 and gp3 are new more performant volumes therefore they should be supported

references

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance#ebs-ephemeral-and-root-block-devices • Closes #114

Tommy avatar

yes same here, and the suggestions for the pr are not really important, because it will work like this aswell

Tommy avatar

what

• Added support for io2 and gp3 volumes

why

• original PR had conflicts, this will work hopefully • io2 and gp3 are new more performant volumes therefore they should be supported

references

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance#ebs-ephemeral-and-root-block-devices
Closes #114

references

• Link to any supporting github issues or helpful documentation to add some context (e.g. stackoverflow). • Use closes #123, if this PR closes a GitHub issue #123

2022-08-19

Mikhail Naletov avatar
Mikhail Naletov

Could somebody take a look? Cloudflare removed two fields from healthcheck in 3.20. I already commented about bump supported version in required providers

https://github.com/cloudposse/terraform-cloudflare-zone/pull/7

what

Remove deprecated fields from resource cloudflare_healthcheck

why

Cloudflare provider just released this change

references

https://github.com/cloudflare/terraform-provider-cloudflare/pull/1789/files
https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/CHANGELOG.md

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

I added a comment, same as yours

what

Remove deprecated fields from resource cloudflare_healthcheck

why

Cloudflare provider just released this change

references

https://github.com/cloudflare/terraform-provider-cloudflare/pull/1789/files
https://github.com/cloudflare/terraform-provider-cloudflare/blob/master/CHANGELOG.md

1
Mikhail Naletov avatar
Mikhail Naletov

Nice cat, btw

1
kevcube avatar
kevcube

what

• Allow user to set runtime_platform in ECS task definition

why

• We are testing a mixed cluster of some ARM64 instances, some AMD64. We want to force certain containers to run in ARM64.

1
1
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

I don’t believe this will work

what

• Allow user to set runtime_platform in ECS task definition

why

• We are testing a mixed cluster of some ARM64 instances, some AMD64. We want to force certain containers to run in ARM64.

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

this is the example from

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ecs_task_definition#runtime_platform

  runtime_platform {
    operating_system_family = "WINDOWS_SERVER_2019_CORE"
    cpu_architecture        = "X86_64"
  }
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

but you added it as a straight input.

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

it would require a dynamic iirc

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

and I do not believe you can add multiple runtime platforms, can you ?

kevcube avatar
kevcube

@RB (Ronak) (Cloud Posse) see my comment on PR

1

2022-08-22

Tommy avatar

what

• Added support for io2 and gp3 volumes

why

• original PR had conflicts, this will work hopefully • io2 and gp3 are new more performant volumes therefore they should be supported

references

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance#ebs-ephemeral-and-root-block-devices
Closes #114

references

• Link to any supporting github issues or helpful documentation to add some context (e.g. stackoverflow). • Use closes #123, if this PR closes a GitHub issue #123

2022-08-26

kevcube avatar
kevcube

what

• Enable HTTP/3 support in CloudFront

why

• It’s faster, and shinier than http/2

references

https://aws.amazon.com/blogs/aws/new-http-3-support-for-amazon-cloudfront/https://blog.cloudflare.com/http-3-vs-http-2/

1
1
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

This does revert the < 4 pin in versions.tf from https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/71

thoughts @Jeremy G (Cloud Posse)?

kevcube avatar
kevcube

Yeah, I assumed that was because of the breaking changes in hashicorp/aws v4 that have since been undone

Jeremy G (Cloud Posse) avatar
Jeremy G (Cloud Posse)

@RB (Ronak) (Cloud Posse) @Andriy Knysh (Cloud Posse) This PR also removes support for provider version 2.x, 3.x, and < 4.27.0. I would like us to be more conservative about dropping support for older version, particularly older 4.x versions. We shall see who complains about this, but I would have allowed the old version 3.x versions and >= 4.9.0 and made a note that http3 support requires >= 4.27.0

1
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

@kevcube do you mind making the changes Jeremy is suggesting ?

Jeremy G (Cloud Posse) avatar
Jeremy G (Cloud Posse)

The PR has already been merged, let’s just wait and see what the community reaction is to it.

1
1

2022-08-30

2022-08-31

matharoo avatar
matharoo

Hello guys, i have a PR for terraform aws rds cluster @ https://github.com/cloudposse/terraform-aws-rds-cluster/pull/149 .. its approved. Can i please get a ETA when this can be merged? Its a blocker for us. Thank you :)

what

• For a multi a-z rds cluster skip creating aws_rds_cluster_instance resource when engine type is NOT aurora, aurora-mysql, aurora-postgresql • AWS provider has a bug that is causing the terraform apply to fail due to a missing rebooting state. I have a PR merged with terraform-aws-provider that fixes it and will be included in the next release 4.23.0. • hashicorp/terraform-provider-aws#25718

why

• Prevent terraform from crashing when creating a non-aurora multi a-z cluster. • aws provider update for fixing rebooting state when creating multi a-z cluster.

references

• closes #148hashicorp/terraform-provider-aws#25718

1
1
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

I can merge this in now @matharoo but this PR isn’t really changing anything other than improving the tests and raising the pin in [versions.tf](http://versions.tf)

what

• For a multi a-z rds cluster skip creating aws_rds_cluster_instance resource when engine type is NOT aurora, aurora-mysql, aurora-postgresql • AWS provider has a bug that is causing the terraform apply to fail due to a missing rebooting state. I have a PR merged with terraform-aws-provider that fixes it and will be included in the next release 4.23.0. • hashicorp/terraform-provider-aws#25718

why

• Prevent terraform from crashing when creating a non-aurora multi a-z cluster. • aws provider update for fixing rebooting state when creating multi a-z cluster.

references

• closes #148hashicorp/terraform-provider-aws#25718

RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

Are you sure the PR contains all the changes you expect it to?

matharoo avatar
matharoo

yes the main one we are concerned with is the AWS provider, because i fixed the provider to have a rebooting state

1
RB (Ronak) (Cloud Posse) avatar
RB (Ronak) (Cloud Posse)

You mean the raising of the aws provider version pin in versions.tf ?

matharoo avatar
matharoo

yes thats correct

1
matharoo avatar
matharoo

thanks man! much appreciated

1
1
Vladimir S. avatar
Vladimir S.

Could someone take a look pr pls

what

• add CloudWatch event rules • add missed CloudWatch log outputs • remove variable sns_subscriptions

why

• CloudWatch event rules are useful for invoking lambda by cron expressions • CloudWatch log outputs are useful for external usage • sns_subscriptions is unused variable

references

• Closes #27

    keyboard_arrow_up