resource/aws_waf_web_acl: Add logging configuration

[WhileLoop]

Fixes #5760

Changes proposed in this pull request:

• Add support for logging configuration for WAF Web ACL.

Potential Terraform Configuration

resource "aws_waf_web_acl" "waf_acl" {
  name        = "test"
  metric_name = "test"

  logging {
    log_destination = "${aws_kinesis_firehose_delivery_stream.waf_stream.arn}"

    redacted_fields {

      field_to_match {
        type = "QUERY_STRING"
      }

      field_to_match {
        type = "METHOD"
      }

      field_to_match {
        type = "URI"
      }

      field_to_match {
        type = "HEADER"
        data = "referer"
      }

    }
  }

  default_action {
    type = "ALLOW"
  }

  rules {
    action {
      type = "BLOCK"
    }

    priority = 1
    rule_id  = "${aws_waf_rule.wafrule.id}"
    type     = "REGULAR"
  }
}

[WhileLoop]

@bflad WIP, I have some questions about this before I keep working on it.

• When does it make sense to use TypeSet vs. TypeList, can you take a look at the schema?
• How clean up expandLoggingConfiguration?
• Should the field types use the all upper case AWS values and expandFieldToMatch or keep the current way?
• Can you show me an example of how to implement flattenLoggingConfiguration in this case?

Thanks

[bflad]

When does it make sense to use TypeSet vs. TypeList, can you take a look at the schema?

The great debate! We have some information about this here: https://www.terraform.io/docs/extend/schemas/schema-types.html#aggregate-types

But essentially it boils down to:

• Is it a "container" attribute (e.g. so you can make a HCL configuration block containing attributes of various Types inside it) -- if so it should be Type: schema.TypeList and MaxItems: 1
• Does ordering matter? If unsure, err on the side that it should, choose Type: schema.TypeList (we cannot automatically migrate TypeSet -> TypeList should there be issues). If yes, choose Type: schema.TypeList. If no, maybe choose Type: schema.TypeSet.

Basically, just prefer to use TypeList unless necessary (I didn't give other exhaustive examples for TypeSet on purpose). 😅

How clean up expandLoggingConfiguration?

Sorry I don't have time to provide an extensive review right now, but I would take a look at other existing expand functions. If the SDK object contains other SDK objects (e.g. waf.Thing1 has an attribute with waf.Thing2), that's generally a good sign that another expand function is needed. 👍

Should the field types use the all upper case AWS values and expandFieldToMatch or keep the current way?

Without deeply looking into this, we generally prefer to follow the API/SDK unless it really enhances user experience. It can become a maintenance burden to create our own translations of concepts, especially if additional API fields are added in the future. I would recommend making the expandWAFFieldToMatch method if the implementation is switched to something like a field_to_match list attribute.

Can you show me an example of how to implement flattenLoggingConfiguration in this case?

It will depend on the implementation of the schema. I would recommend looking at other existing flatten functions for inspiration.

[bflad]

@WhileLoop did you have other questions or will you have time to keep working on this?

[WhileLoop]

@bflad Forgot all about this tbh. Almost done. On line 204 I use resp2, is there a better way to do that?

[WhileLoop]

@bflad I'm stuck. Can't get past error setting logging configuration: Invalid address to set: []string{"logging_configuration", "0", "redacted_fields"}. Any pointers?

[bflad]

@WhileLoop it looks like the redacted_fields attribute is currently Type: schema.TypeSet -- in order to correctly set it in the state you would need to use schema.NewSet()

My recommendation would be to change that attribute to Type: schema.TypeList and MaxItems: 1 if it is only used to define a single configuration block.

[WhileLoop]

@bflad redacted_fields can have multiple field_to_match blocks.

[anilkasu]

Any update on this enhancement, when can we expect to release this? Any chances including this feature in v0.12?

[WhileLoop]

@bflad Can you take another look at this? I changed logging_config and redacted_fields to TypeList. When I apply the WAF gets created with right config and the statefile has lines like "logging_configuration.0.redacted_fields.0.field_to_match.2316364334.type": "QUERY_STRING", but when reading state I get the error error setting logging configuration: Invalid address to set: []string{"logging_configuration", "0", "redacted_fields", "0", "field_to_match"}

[bflad]

Hi @WhileLoop 👋 Thanks for submitting this and getting this going. I know you only asked about the error during d.Set(), but I wanted to provide a more comprehensive review since a lot of folks are waiting on this functionality. Please let us know if you have questions or do not have time to implement these items.

Sorry if I wasn't clear before about my recommendation to use schema.NewSet(). Admittedly, its a complicated provider development subject, especially since its only required for nested TypeSet attributes. The implementation I used below with schema.HashResource() is a shortcut for us to skip creating a custom schema.TypeSet hash function that would be used as the Set: schema field you may see in other resources.

Besides that, with the below changes, I was able to get a basic run of the acceptance testing passing with some updates to your submitted test configuration (replacing rInt with rName, using the required firehose prefix, etc.).

To get this functionality merged, we will request adding the following to the acceptance testing to verify successful creation with logging configuration, import, update of logging configuration, and removal of logging configuration:

func TestAccAWSWafWebAcl_LoggingConfiguration(t *testing.T) {
	var webACL waf.WebACL
	rName := fmt.Sprintf("wafacl%s", acctest.RandString(5))
	resourceName := "aws_waf_web_acl.test"

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckAWSWafWebAclDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccAWSWafWebAclConfig_Logging(rName),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckAWSWafWebAclExists(resourceName, &webACL),
					resource.TestCheckResourceAttr(resourceName, "logging_configuration.#", "1"),
					resource.TestCheckResourceAttr(resourceName, "logging_configuration.0.redacted_fields.#", "1"),
					resource.TestCheckResourceAttr(resourceName, "logging_configuration.0.redacted_fields.0.field_to_match.#", "4"),
				),
			},
			// Test resource import
			{
				ResourceName:      resourceName,
				ImportState:       true,
				ImportStateVerify: true,
			},
			// Test logging configuration update
			{
				Config: testAccAWSWafWebAclConfig_LoggingUpdate(rName),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckAWSWafWebAclExists(resourceName, &webACL),
					resource.TestCheckResourceAttr(resourceName, "logging_configuration.#", "1"),
					resource.TestCheckResourceAttr(resourceName, "logging_configuration.0.redacted_fields.#", "0"),
				),
			},
			// Test logging configuration removal
			{
				Config: testAccAWSWafWebAclConfig_Required(rName),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckAWSWafWebAclExists(resourceName, &webACL),
					resource.TestCheckResourceAttr(resourceName, "logging_configuration.#", "0"),
				),
			},
		},
	})
}

func testAccAWSWafWebAclConfig_LoggingUpdate(rName string) string {
	return fmt.Sprintf(`
resource "aws_waf_web_acl" "test" {
  metric_name = %q
  name        = %q

  default_action {
    type = "ALLOW"
  }

  logging_configuration {
    log_destination = "${aws_kinesis_firehose_delivery_stream.test_stream.arn}"
  }
}
resource "aws_s3_bucket" "bucket" {
  bucket = %q
  acl    = "private"
}

resource "aws_iam_role" "firehose_role" {
  name = %q

  assume_role_policy = <<EOF
{
  "Version": "2012-10-17",
  "Statement": [
    {
      "Action": "sts:AssumeRole",
      "Principal": {
        "Service": "firehose.amazonaws.com"
      },
      "Effect": "Allow",
      "Sid": ""
    }
  ]
}
EOF
}

resource "aws_kinesis_firehose_delivery_stream" "test_stream" {
  # the name must begin with aws-waf-logs-
  name        = "aws-waf-logs-%s"
  destination = "s3"

  s3_configuration {
    role_arn   = "${aws_iam_role.firehose_role.arn}"
    bucket_arn = "${aws_s3_bucket.bucket.arn}"
  }
}

`, rName, rName, rName, rName, rName)
}

Hope this helps and please let us know if you need anything else.

[bflad]

Prior to returning, this logic should ensure an empty logging_configuration is set into the Terraform state, otherwise when removed from AWS, Terraform will always report a difference.

Suggested change

	return nil
	if err := d.Set("logging_configuration", []interface{}{}); err != nil {
	return fmt.Errorf("error setting logging_configuration: %s", err)
	}
	return nil

[bflad]

I believe the AWS Go SDK will automatically call Validate() so this is extraneous.

[bflad]

The redacted_fields argument is optional, so if it is not configured this will panic. We should add a length and nil check here, e.g.

Suggested change

	rfm := rf[0].(map[string]interface{})
	if len(rf) == 0 || rf[0] == nil {
	return lc
	}
	rfm := rf[0].(map[string]interface{})

[bflad]

To prevent potential panics, we should perform a length check here and use the AWS Go SDK provided helper for dereferencing the value:

Suggested change

	m["log_destination"] = *n.LogDestinationConfigs[0]
	if len(n.LogDestinationConfigs) > 0 {
	m["log_destination"] = aws.StringValue(n.LogDestinationConfigs[0])
	}

[bflad]

Here is a working example of this function for configurations with and without redacted_fields:

func flattenRedactedFields(fieldToMatches []*waf.FieldToMatch) []interface{} {
	if len(fieldToMatches) == 0 {
		return []interface{}{}
	}

	fieldToMatchResource := &schema.Resource{
		Schema: map[string]*schema.Schema{
			"data": {
				Type:     schema.TypeString,
				Optional: true,
			},
			"type": {
				Type:     schema.TypeString,
				Required: true,
			},
		},
	}
	l := make([]interface{}, len(fieldToMatches))

	for i, fieldToMatch := range fieldToMatches {
		l[i] = flattenFieldToMatch(fieldToMatch)[0]
	}

	m := map[string]interface{}{
		"field_to_match": schema.NewSet(schema.HashResource(fieldToMatchResource), l),
	}

	return []interface{}{m}
}

[bflad]

The name for WAF logging Kinesis Firehose Delivery Streams must begin with aws-waf-logs-

Suggested change

	name = "terraform-kinesis-firehose-waftest-%d"
	name = "aws-waf-logs-%s"

[bflad]

If you need further examples of anything, you might find the WAF Regional equivalent of this pull request (#7480) helpful. 👍

[WhileLoop]

Thanks @bflad. The expand/flatten functions for loggingConfiguration and redactedFields can probably be shared by regional and global WAF resource.

[bflad]

The expand/flatten functions for loggingConfiguration and redactedFields can probably be shared by regional and global WAF resource.

@WhileLoop most likely, but we can handle that after getting the support in. 😄 The WAF service code has quite a few issues with code structure and naming due to a large portion of it being migrated from hashicorp/terraform pull requests right after Terraform providers were split from the single repository.

---

Let us know when you think this is ready for another review and we'll take a look. 👍

[WhileLoop]

@bflad this is ready to go.

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -parallel 20 -run=TestAccAWSWafWebAcl_LoggingConfiguration -timeout 120m
=== RUN   TestAccAWSWafWebAcl_LoggingConfiguration
=== PAUSE TestAccAWSWafWebAcl_LoggingConfiguration
=== CONT  TestAccAWSWafWebAcl_LoggingConfiguration
--- PASS: TestAccAWSWafWebAcl_LoggingConfiguration (100.49s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	101.497s

[bflad]

Thanks for all the updates, @WhileLoop! FYI, I copied over the logging_configuration argument documentation from #7480 to finish this up. 👍

Output from acceptance testing:

--- PASS: TestAccAWSWafWebAcl_basic (6.55s)
--- PASS: TestAccAWSWafWebAcl_LoggingConfiguration (63.50s)
--- PASS: TestAccAWSWafWebAcl_disappears (7.44s)
--- PASS: TestAccAWSWafWebAcl_Rules (25.95s)
--- PASS: TestAccAWSWafWebAcl_changeNameForceNew (12.07s)
--- PASS: TestAccAWSWafWebAcl_DefaultAction (12.88s)

[bflad]

This has been released in version 1.59.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

[ghost]

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!