Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Terraform 0.12 Update #10

Merged
merged 6 commits into from Nov 8, 2019

Conversation

@pjdufour-truss
Copy link
Contributor

pjdufour-truss commented Oct 30, 2019

This PR updates the module to terraform 0.12 and adds support for blocking a dynamic list of ip sets.

@pjdufour-truss pjdufour-truss requested a review from chrisgilmerproj Oct 30, 2019
Copy link
Contributor

dynamike left a comment

I played with Terratest to test this out and ran into one issue around the associate_alb variable. Looking at it, I don't really see why we have the ALB association be conditional. Maybe we should just take it out? Below is the example I used to test.

#
# WAF
#

module "waf" {
  source = "../../"

  alb_arn       = aws_lb.simple.arn
  associate_alb = true

  regex_host_allow_pattern_strings    = []
  regex_path_disallow_pattern_strings = []

  web_acl_name        = "app-hello-world"
  web_acl_metric_name = "wafAppHelloWorld"
}

#
# ALB
#

resource "aws_lb" "simple" {
  name_prefix        = "simple"
  internal           = true
  load_balancer_type = "application"
  subnets = [
    aws_subnet.simple1.id,
    aws_subnet.simple2.id,
  ]

  tags = {
    Automation = "Terraform"
  }
}

#
# VPC
#

resource "aws_vpc" "simple" {
  cidr_block = "10.0.0.0/16"

  tags = {
    Automation = "Terraform"
  }
}


resource "aws_subnet" "simple1" {
  vpc_id     = "${aws_vpc.simple.id}"
  cidr_block = "10.0.1.0/24"

  tags = {
    Automation = "Terraform"
  }
}

resource "aws_subnet" "simple2" {
  vpc_id     = "${aws_vpc.simple.id}"
  cidr_block = "10.0.2.0/24"

  tags = {
    Automation = "Terraform"
  }
}
main.tf Outdated
}

resource "aws_wafregional_web_acl_association" "main" {
count = "${var.associate_alb}"
count = var.associate_alb > 0 ? 1 : 0

This comment has been minimized.

Copy link
@dynamike

dynamike Nov 1, 2019

Contributor

TestTerraformAwsWaf 2019-11-01T14:52:48-07:00 command.go:158: on ../../main.tf line 197, in resource "aws_wafregional_web_acl_association" "main":
TestTerraformAwsWaf 2019-11-01T14:52:48-07:00 command.go:158: 197: count = var.associate_alb > 0 ? 1 : 0
TestTerraformAwsWaf 2019-11-01T14:52:48-07:00 command.go:158: |----------------
TestTerraformAwsWaf 2019-11-01T14:52:48-07:00 command.go:158: | var.associate_alb is "true"
TestTerraformAwsWaf 2019-11-01T14:52:48-07:00 command.go:158:
TestTerraformAwsWaf 2019-11-01T14:52:48-07:00 command.go:158: Unsuitable value for left operand: a number is required.

This comment has been minimized.

Copy link
@dynamike

dynamike Nov 1, 2019

Contributor
Suggested change
count = var.associate_alb > 0 ? 1 : 0
count = var.associate_alb ? 1 : 0
@pjdufour-truss pjdufour-truss requested a review from dynamike Nov 4, 2019
Copy link
Contributor

dynamike left a comment

Left two more non-blocking comments. 👍

README.md Outdated Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
Copy link
Contributor

chrisgilmerproj left a comment

🚀 - I believe this is good to go from testing on my infra.

README.md Outdated
ip_rate_limit = 2000
ip_set = "${aws_wafregional_ipset.global.id}"
wafregional_rule_f5_id = "${var.wafregional_rule_id}"
ip_sets = list(aws_wafregional_ipset.global.id)

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Nov 4, 2019

Contributor
Suggested change
ip_sets = list(aws_wafregional_ipset.global.id)
ip_sets = [aws_wafregional_ipset.global.id]
main.tf Outdated
rule_id = "${aws_wafregional_rule.ips.id}"
priority = 3
dynamic "rule" {
for_each = length(var.wafregional_rule_f5_id) > 0 ? list(var.wafregional_rule_f5_id) : []

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Nov 4, 2019

Contributor
Suggested change
for_each = length(var.wafregional_rule_f5_id) > 0 ? list(var.wafregional_rule_f5_id) : []
for_each = length(var.wafregional_rule_f5_id) > 0 ? [var.wafregional_rule_f5_id] : []
README.md Outdated

Version `2.0.0` removes the `environment` variable and adds `web_acl_metric_name` and `web_acl_name` variables to provide more control over naming. AWS WAF rules will be prefixed by the `web_acl_name` of their associated Web ACL to provide for easy visual sorting.

Version `2.0.0` replaces the `ip_set` variable with a `ip_sets` list variable, which accepts a list of `aws_wafregional_ipset` ids. This variable allows the Web ACL to pull from multiple lists of blocked ip addresses, such that you can combine a global blocked list, and application-specific lists. For example: `ip_sets = list(resource.aws_wafregional_ipset.global.id, resource.aws_wafregional_ipset.helloworld.id)`.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Nov 4, 2019

Contributor
Suggested change
Version `2.0.0` replaces the `ip_set` variable with a `ip_sets` list variable, which accepts a list of `aws_wafregional_ipset` ids. This variable allows the Web ACL to pull from multiple lists of blocked ip addresses, such that you can combine a global blocked list, and application-specific lists. For example: `ip_sets = list(resource.aws_wafregional_ipset.global.id, resource.aws_wafregional_ipset.helloworld.id)`.
Version `2.0.0` replaces the `ip_set` variable with a `ip_sets` list variable, which accepts a list of `aws_wafregional_ipset` ids. This variable allows the Web ACL to pull from multiple lists of blocked ip addresses, such that you can combine a global blocked list, and application-specific lists. For example: `ip_sets = [resource.aws_wafregional_ipset.global.id, resource.aws_wafregional_ipset.helloworld.id]`.
README.md Outdated

Version `2.0.0` replaces the `ip_set` variable with a `ip_sets` list variable, which accepts a list of `aws_wafregional_ipset` ids. This variable allows the Web ACL to pull from multiple lists of blocked ip addresses, such that you can combine a global blocked list, and application-specific lists. For example: `ip_sets = list(resource.aws_wafregional_ipset.global.id, resource.aws_wafregional_ipset.helloworld.id)`.

During the initial upgrade to `2.0.0`, and if you add additional dynamic rules, you'll need to delete your web ACLs, as terraform cannot properly handle peer dependencies between Rules and Web ACLs. For convenience, you can use the `delete-web-acl` script in the scripts folder. For example: `scripts/delete-web-acl WEB_ACL_ID`. Once the Web ACL is deleted use terraform apply to recreate the Web ACL and associate with your resources as you had before. Deleting a Web ACL does not delete any associated resources, such as Application Load Balancers; however, it will lead the resources temporarily unprotected.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Nov 4, 2019

Contributor
Suggested change
During the initial upgrade to `2.0.0`, and if you add additional dynamic rules, you'll need to delete your web ACLs, as terraform cannot properly handle peer dependencies between Rules and Web ACLs. For convenience, you can use the `delete-web-acl` script in the scripts folder. For example: `scripts/delete-web-acl WEB_ACL_ID`. Once the Web ACL is deleted use terraform apply to recreate the Web ACL and associate with your resources as you had before. Deleting a Web ACL does not delete any associated resources, such as Application Load Balancers; however, it will lead the resources temporarily unprotected.
During the initial upgrade to `2.0.0`, and if you add additional dynamic rules, you'll need to delete your web ACLs, as terraform cannot properly handle peer dependencies between Rules and Web ACLs. For convenience, you can use the `delete-web-acl` script in the scripts folder. For example: `scripts/delete-web-acl WEB_ACL_ID`. Once the Web ACL is deleted use terraform apply to recreate the Web ACL and associate with your resources as you had before. Deleting a Web ACL does not delete any associated resources, such as Application Load Balancers; however, it will leave the resources temporarily unprotected.

This comment has been minimized.

Copy link
@chrisgilmerproj

chrisgilmerproj Nov 4, 2019

Contributor

s/lead/leave/g

@pjdufour-truss pjdufour-truss changed the title tf 0.12 update Terraform 0.12 update Nov 8, 2019
@pjdufour-truss pjdufour-truss changed the title Terraform 0.12 update Terraform 0.12 Update Nov 8, 2019
@pjdufour-truss pjdufour-truss merged commit 7eb06ce into master Nov 8, 2019
1 check passed
1 check passed
ci/circleci: validate Your tests passed on CircleCI!
Details
@pjdufour-truss pjdufour-truss deleted the tf12 branch Nov 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
3 participants
You can’t perform that action at this time.