#pr-reviews (2023-03)

Pull Request Reviews for Cloud Posse Projects

2023-03-01

Gocho avatar
Gocho
04:19:29 PM

@Gocho has joined the channel

2023-03-02

cinacio avatar
cinacio

Hi. I’ve been sent to this channel in case my PR needs a nudge, so here it is: https://github.com/cloudposse/terraform-aws-ec2-instance/pull/142

It’s almost there, but for some reason auto-format fails

#142 Do not set iops and throughput for non-supporting volume types

what

• Only set iops and throughput arguments for volume types supporting those options.

why

• According to the documentation for the aws_ebs_volume resource, the iops argument is only valid for volumes types io1, io2 & gp3. • According to the same documentation, the throughput argument is only valid for volume type gp3. • Setting the throughput of a gp2 volume to “0” triggers the following error

Error: expected throughput to be in the range (125 - 1000), got 0

with module.instance.aws_ebs_volume.default[0],
on .terraform/modules/instance/main.tf line 178, in resource “aws_ebs_volume” “default”:
178: throughput = local.ebs_throughput

references

• closes #137

1
1
RB avatar

Try running make pr/auto-format locally

#142 Do not set iops and throughput for non-supporting volume types

what

• Only set iops and throughput arguments for volume types supporting those options.

why

• According to the documentation for the aws_ebs_volume resource, the iops argument is only valid for volumes types io1, io2 & gp3. • According to the same documentation, the throughput argument is only valid for volume type gp3. • Setting the throughput of a gp2 volume to “0” triggers the following error

Error: expected throughput to be in the range (125 - 1000), got 0

with module.instance.aws_ebs_volume.default[0],
on .terraform/modules/instance/main.tf line 178, in resource “aws_ebs_volume” “default”:
178: throughput = local.ebs_throughput

references

• closes #137

2023-03-03

Nitin avatar

Terraform test is getting failed on repos because vpc limit is reached for the enviroment.

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

Thanks! Yea we will get this fixed next week. Not much happening this weekend

Nitin avatar

Thanks @Erik Osterman (Cloud Posse) for your prompt reply

2023-03-04

2023-03-07

mihai.plesa avatar
mihai.plesa
#151 Only get data.aws_ami.info if it's actually required

what

• Use data.aws_ami.info only if it’s actually required

why

• Fix Your query returned no results” error once the AMI ID is not listed within AWS marketplace

references

• closes #150 • closes #143

1
1

2023-03-08

Quentin BERTRAND avatar
Quentin BERTRAND

Hello, I have this PR to review:

Feat: add option to not create target group

I guess it will need a procedure migration of state is this PR is accepted

#33 Feat: add option to not create target group

what

Add option to not create default target group

why

Default target group can be useless.

references

#22

I guess it will need a procedure migration of state is this PR is accepted.

1

2023-03-09

Josh Baird avatar
Josh Baird
Andriy Knysh (Cloud Posse) avatar
Andriy Knysh (Cloud Posse)

@Josh Baird reviewed the PR, thanks for it, left a few comments )one of them is an annoying Tf feature, but we have to deal with it)

Josh Baird avatar
Josh Baird

Interesting, because the code (as-is) does work.

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

hmm maybe diff TF versions (and maybe TF improved it in the latest versions)

2023-03-14

kevcube avatar
kevcube

Requesting review on this

It is blocking me here because go 1.15 can’t build for darwin/arm64

1

2023-03-18

kevcube avatar
kevcube
kevcube avatar
kevcube

For the one regarding master password, maybe an optional resource shoudl be aded to store in SSM like the RDS module does.

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

Yea better to write to SSM

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

Any thoughts on this?

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

@Ben Smith (Cloud Posse) @Jeremy White (Cloud Posse) @Dan Miller (Cloud Posse)

1
Dan Miller (Cloud Posse) avatar
Dan Miller (Cloud Posse)

Yes we try to avoid writing any secrets to Terraform state. Instead write to SSM as Erik suggested and output the path to the parameter

Jeremy White (Cloud Posse) avatar
Jeremy White (Cloud Posse)

the performance insights fellow seems straightforward.

Dan Miller (Cloud Posse) avatar
Dan Miller (Cloud Posse)

agreed also. Approved and merging the performance insights PR

Jeremy White (Cloud Posse) avatar
Jeremy White (Cloud Posse)

amazon is weird though. They call the flag in aurora performance_insights_enabled

1
Jeremy White (Cloud Posse) avatar
Jeremy White (Cloud Posse)

no fault of the PR, obviously. But a reason to consider a naming convention to unite both at a future date

2023-03-21

Dan Miller (Cloud Posse) avatar
Dan Miller (Cloud Posse)
01:36:11 AM

@Dan Miller (Cloud Posse) has joined the channel

Jeremy White (Cloud Posse) avatar
Jeremy White (Cloud Posse)
01:36:11 AM

@Jeremy White (Cloud Posse) has joined the channel

Ben Smith (Cloud Posse) avatar
Ben Smith (Cloud Posse)
01:36:11 AM

@Ben Smith (Cloud Posse) has joined the channel

2023-03-22

Jeremy White (Cloud Posse) avatar
Jeremy White (Cloud Posse)

quick reminder that you can press period . while on a pull request to let you add things like terraform highlighting, or to mark directories as ‘read’…

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

@Linda Pham (Cloud Posse) can you find someone to help review https://github.com/cloudposse/terraform-aws-eks-node-group/pull/139

#139 Windows node supportattachment image

what

• Adds support for 2019/2022 (windows lts) ami-types as per the aws sdk docs • Docs to support usage and link to ref docs https://docs.aws.amazon.com/eks/latest/userguide/windows-support.html

why

• Windows EKS Managed node support aws/containers-roadmap#584

references

https://docs.aws.amazon.com/eks/latest/userguide/windows-support.htmlhttps://docs.aws.amazon.com/eks/latest/userguide/eks-ami-versions-windows.html

Tested

image
image
image
image

module "eks_windows_node_group" {
  # source  = "cloudposse/eks-node-group/aws"
  # version = "2.6.1"
  source = "github.com/ChrisMcKee/terraform-aws-eks-node-group"

  instance_types     = ["t3.large", "t3a.large", "c5.large", "c6i.large", "m6i.large", "r6i.large"]
  subnet_ids         = [data.terraform_remote_state.network.outputs.private_subnets[1]]
  min_size           = 1
  max_size           = 1
  desired_size       = 1
  cluster_name       = module.eks_cluster.eks_cluster_id
  kubernetes_version = var.kubernetes_version == null || var.kubernetes_version == "" ? [] : [var.kubernetes_version]
  kubernetes_labels  = var.labels

  ami_type = "WINDOWS_CORE_2019_x86_64"

  update_config = [{ max_unavailable = 1 }]

  capacity_type = "SPOT"

  kubernetes_taints = [{
    key    = "OS"
    value  = "Windows"
    effect = "NO_SCHEDULE"
  }]

  node_role_arn                = [aws_iam_role.worker_role_nt.arn]
  node_role_cni_policy_enabled = false #We use the Service Account as per best practice

  associated_security_group_ids = [data.terraform_remote_state.network.outputs.ops_ssh, aws_security_group.workers.id]

  # Enable the Kubernetes cluster auto-scaler to find the auto-scaling group
  cluster_autoscaler_enabled = true

  context = module.windowslabel.context

  # Ensure the cluster is fully created before trying to add the node group
  module_depends_on = [module.eks_cluster.kubernetes_config_map_id]

  # Ensure ordering of resource creation to eliminate the race conditions when applying the Kubernetes Auth ConfigMap.
  # Do not create Node Group before the EKS cluster is created and the `aws-auth` Kubernetes ConfigMap is applied.
  depends_on = [module.eks_cluster, module.eks_cluster.kubernetes_config_map_id]

  create_before_destroy = true

  node_role_policy_arns = ["arn:aws:iam::aws:policy/AmazonSSMManagedInstanceCore"]

  block_device_mappings = [
    {
      "delete_on_termination" : true,
      "device_name" : "/dev/xvda",
      "encrypted" : true,
      "volume_size" : 80,
      "volume_type" : "gp3"
    }
  ]

  node_group_terraform_timeouts = [{
    create = "40m"
    update = null
    delete = "20m"
  }]

  #Valid types are "instance", "volume", "elastic-gpu", "spot-instances-request", "network-interface".
  resources_to_tag = ["instance", "volume", "spot-instances-request", "network-interface"]
}
1
1
Linda Pham (Cloud Posse) avatar
Linda Pham (Cloud Posse)
07:06:42 PM

@Linda Pham (Cloud Posse) has joined the channel

2023-03-23

2023-03-28

Anthony B avatar
Anthony B

hello! I was looking to get this PR reviewed https://github.com/cloudposse/terraform-aws-cloudfront-cdn/pull/95 however there does seem to be an issue with permissions it looks like getting a github action to successfully run on my fork. Would I be able to get some advice? Thank you!

#95 Fix custom origin, add s3 origin for error pages

what

• custom origins can be created without error for using s3_origin_config • add the ability to add an s3 origin for use in routing traffic to error pages/custom error responses

why

• custom origins do not work with the current version of this module • allows a custom origin with s3 to be used as a secondary origin for error handling

references

#74closes #74

Joe Niland avatar
Joe Niland

Hi @Anthony B I fixed that issue. The Github workflows were out of date, as they’ve been updated recently.

#95 Fix custom origin, add s3 origin for error pages

what

• custom origins can be created without error for using s3_origin_config • add the ability to add an s3 origin for use in routing traffic to error pages/custom error responses

why

• custom origins do not work with the current version of this module • allows a custom origin with s3 to be used as a secondary origin for error handling

references

#74closes #74

Anthony B avatar
Anthony B

Hey Joe! No problem! I appreciate you checking that out!

2023-03-30

2023-03-31

kevcube avatar
kevcube
#618 s3-bucket: remove template provider

what

• remove dependency on template provider

why

• arm64 • also this provider was not pinned in versions.tf so that had to be fixed somehow

references

• closes #617

    keyboard_arrow_up