SweetOps #pr-reviews for September, 2022

Pull Request Reviews for Cloud Posse Projects

2022-09-01

Brent Farand

Hello! I have a PR for the terraform-aws-transit-gateway module (https://github.com/cloudposse/terraform-aws-transit-gateway/pull/29). It allows the amazon_side_asn to be specified for the purpose of both having multiple transit gateways in the same deployment to have different ASNs (following AWS’s documented best practices), as well as to allow importing an existing transit gateway with a non-default ASN without needing to destroy and recreate it.

what

• The new variable ‘amazon_side_asn’ was added to the module with a default value of 64512 (AWS default, preserving current behaviour). • Variable ‘amazon_side_asn’ is now provided as an argument to the ‘aws_ec2_transit_gateway.default’ resource. • Documentation updated accordingly.

why

• Allows the ASN of a transit gateway to be set, allowing the best practice of having multiple transit gateways used in the same deployment to have different ASNs (https://docs.aws.amazon.com/vpc/latest/tgw/tgw-best-design-practices.html) • In cases where a user is importing their existing infrastructure so it is managed via terraform, specifying the existing ASN means that the existing transit gateway does not need to be destroyed and recreated

references

• closes #27

2022-09-02

Andre Lohmann

07:16:26 AM

Hi there, I found a Bug (or not a bug, but more a missing feature) in the cloudposse/s3-bucket/aws module (https://github.com/cloudposse/terraform-aws-s3-bucket) which I was able to trace down to the cloudposse/iam-s3-user/aws module and the cloudposse/iam-system-user/aws module (the last one introduced the max age for the expirable access token and set that to a default of 30 days, which currently can’t be influenced in the s3 module).

I created two PRs to solve the problem

https://github.com/cloudposse/terraform-aws-iam-s3-user/pull/47 https://github.com/cloudposse/terraform-aws-s3-bucket/pull/155

How can I make sure these two PRs are getting merged and/or how can I align/adjust my changes to fulfil (to me currently unknown) requirements?

Nitin

09:14:01 AM

https://github.com/cloudposse/terraform-aws-elasticache-memcached/pull/49

what

• Remove join splat on module.security_group_arn

why

• Fix conflict with using custom security group in associated_security_group_ids and argument create_security_group is false

references

• N/A

2022-09-08

Chris Dobbyn

09:14:25 PM

Hi there, simple change. It adds aws_default_route_table that gets create automatically with every aws_vpc resource. https://github.com/cloudposse/terraform-aws-vpc/pull/110

what

• Manages the default route table created alongside aws_vpc resource automatically.

why

• If not managed there are no identifying features about this resource which is confusing. • In our case we establish ownership via tags passed into this module, if no tags are present it is difficult to report on ownership

references

• Closes #109 • aws_default_route_table

2022-09-11

Nitin

06:33:39 AM

https://github.com/cloudposse/terraform-aws-ec2-instance/pull/136

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-09-12

Chris Dobbyn

02:04:44 PM

https://github.com/cloudposse/terraform-aws-vpc/pull/110

what

• Manages the default route table created alongside aws_vpc resource automatically.

why

references

• Closes #109 • aws_default_route_table

Tommy

02:09:10 PM

just added an output to acm module: https://github.com/cloudposse/terraform-aws-acm-request-certificate/pull/59

from terraform aws provider documentation: It deals with requesting certificates and managing their attributes and life-cycle. This resource does not deal with validation of a certificate but can provide inputs for other resources implementing the validation. It does not wait for a certificate to be issued. Use a aws_acm_certificate_validation resource for this.

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

2022-09-13

2022-09-14

07:06:20 PM

Please review

• https://github.com/cloudposse/terraform-aws-acm-request-certificate/pull/54/files

• https://github.com/cloudposse/terraform-aws-acm-request-certificate/pull/61/files

11:02:48 PM

Cc @Andriy Knysh (Cloud Posse) if you get a chance

2022-09-15

Amrutha Sunkara

02:23:32 PM

Can someone take a look at https://github.com/cloudposse/terraform-aws-rds-db-proxy/pull/11 ??

what

• This module creates a write endpoint by default. Creating a RDS proxy read endpoint will ensure underlying primary/master instance is excluded from the connections isolating DDLs & DMLs in addition, to providing and additional layer for just read based workload.

why

• Usecase involves a separate read proxy endpoint that isolates reads to the replicas & separate write proxy endpoint to the primary instance.

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/db_proxy_endpoint • closes #10

Amrutha Sunkara

01:07:56 PM

Could someone take a look at this PR?

what

why

• Usecase involves a separate read proxy endpoint that isolates reads to the replicas & separate write proxy endpoint to the primary instance.

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/db_proxy_endpoint • closes #10

praneeth

02:24:58 PM

@praneeth has joined the channel

2022-09-19

07:05:57 PM

https://github.com/cloudposse/terraform-aws-sns-topic/pull/57

2022-09-20

Joe Niland

01:52:31 AM

https://github.com/cloudposse/terraform-aws-components/pull/495

what

• Adds a flag var.deny_users which, when disabled, will allow IAM users to assume team roles • Specific users can still be denied via var.denied_principal_arns

why

• The existing functionality denies all IAM users from assuming team roles • In legacy/specific situations it would be good to allow this

references

• None

(PR submitted on behalf of @Gowiem)

2022-09-29

Tyrone Meijn

08:51:21 AM

I don’t know if this is necessarily the right channel, but I’d like to revert the behavior introduced in PR #142. I created an issue to explain why, but maybe I’m missing something.

Tyrone Meijn

06:55:27 AM

Hey @RB I added a comment in response to yours. WDYT?

@nitrocode If I understand the previous behavior correctly:

db_subnet_group_name = local.db_subnet_group_name_provided ? var.db_subnet_group_name : (
    local.subnet_ids_provided ? join("", aws_db_subnet_group.default.*.name) : null
  )

if local.subnet_ids_provided is false, and no external db_subnet is provided, then it returns null

subnet_ids_provided = var.subnet_ids != null && length(var.subnet_ids) > 0

var.subnet_ids is default []

So not providing subnet_ids to the module should evaluate to the same behavior PR #142 added, while also allowing the case that is described in this issue.

Tyrone Meijn

12:30:10 PM

Hey @Erik Osterman (Cloud Posse) sorry to tag you directly, but this issue has been stuck for more than two months and this “bug” prevents me from upgrading the module version. Do you know someone that could move this forward?

Erik Osterman (Cloud Posse)

03:16:47 PM

@Tyrone Meijn we’re happy to look at any PRs, (promote them in #pr-reviews). Otherwise, I have to echo what @RB said, that we cannot prioritize looking into a fix/pr at this time unless the issue is raised by a customer. I hate to push back, but we don’t have anyone who can take a look right now.

roth.andy

03:17:15 PM

Can I get some help figuring out why the pipelines are failing? Looks like the error is happening in the step that updates the Status of the run in the PR

https://github.com/cloudposse/terraform-aws-ec2-instance/pull/141

what

• Add new variable tenancy that allows user to configure instance tenancy (default (default), dedicated, or host)

why

• An organization that I work with has a security policy that requires use of dedicated tenancy. This PR adds the capability to do that when using this module.

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance#tenancy • Closes #139

Erik Osterman (Cloud Posse)

01:23:07 PM

@Jeremy G (Cloud Posse)

what

• Add new variable tenancy that allows user to configure instance tenancy (default (default), dedicated, or host)

why

• An organization that I work with has a security policy that requires use of dedicated tenancy. This PR adds the capability to do that when using this module.

references

• https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance#tenancy • Closes #139

Jeremy G (Cloud Posse)

06:57:00 PM

Yes, our test framework was broken. GitHub overwrote an environment variable we were using (which was fair, since our variable started with GITHUB_). This has been fixed, now, and this particular PR is waiting for input validation to be added.

Erik Osterman (Cloud Posse)

05:53:02 PM

Btw, saw this morning that github is deprecating all set-output commands soon

Erik Osterman (Cloud Posse)

05:53:09 PM

so we need to address that now too