Addresses an issue with logging configuration with improper for_each iterator

[dchernivetsky]

No description provided.

[dchernivetsky]

#238

[brikis98]

Ah, I believe the approach we actually use is to call lookup on var.access_logs instead of xxx.value. That shouldn't require the can usage...

[annawoodall]

Either var.access_logs or access_logs.value works as we are iterating over a single item. As we're using the for_each meta-argument it makes sense to use the iterator keys and values rather than referencing var.access_logs directly.

The use of can is required whether you use var.access_logs or access_logs.value to ensure successful execution when you don't specify the optional enabled argument as part of your access_logs definition.

Error: Incorrect attribute value type

  on ../vault-elb/main.tf line 57, in resource "aws_elb" "vault":
  57:       enabled       = lookup(var.access_logs, "enabled", lookup(var.access_logs, "bucket", null))
    |----------------
    | var.access_logs is object with 1 attribute "bucket"

Inappropriate value for attribute "enabled": a bool is required.

Error: Incorrect attribute value type

  on ../vault-elb/main.tf line 57, in resource "aws_elb" "vault":
  57:       enabled       = lookup(access_logs.value, "enabled", lookup(access_logs.value, "bucket", null))

Inappropriate value for attribute "enabled": a bool is required.

I've tested the solution as coded and it works in my implementation whether or not I choose to enable access logs.

[brikis98]

The 3-param version of lookup (lookup(<MAP>, <KEY>, <DEFAULT>)) is what you want instead of can.

NIT: It's true that using access_logs.value works too, but it requires you scrolling up to for_each and parsing that to understand what that .value could be. That makes sense if we're looping over multiple items, but here, it's either 0 or 1, so using var.access_logs is more understandable.

[dchernivetsky]

Updated to remove the use of can. Appears to be working. However, it looks clunky. The idea behind can was to use it as a gate. It served a purpose of determining whether the user passed access_logs.enabled in the map. It is effectively optional param.
If the user passed this param, we would use it to determine whether to enable logs. If this param was not passed we would check the bucket param and assume that the user wants the logs enabled.
As is written right now, this logic wouldn't work, whether or not the enabled param is passed the code will default to looking at the bucket param.
I.e.

  enabled = false
  bucket = "blah"

Would enable logging right now. Whereas beforehand it wouldn't enable logs (this is useful for controlling logs in lower envs)

[brikis98]

LGTM, thanks! I'll kick off tests now.

[brikis98]

Pre-commit hook failed:

[INFO] Initializing environment for https://github.com/gruntwork-io/pre-commit.
Terraform fmt............................................................Failed
- hook id: terraform-fmt
- exit code: 3

modules/vault-elb/main.tf
--- old/modules/vault-elb/main.tf
+++ new/modules/vault-elb/main.tf
@@ -26,7 +26,7 @@
 
   # optional access_logs creation  
   dynamic "access_logs" {
-    for_each = var.access_logs == null ? [] : [ var.access_logs ]
+    for_each = var.access_logs == null ? [] : [var.access_logs]
 
     content {
       # enabled flag is optional. The user can choose to specify or not. If not specified we check if bucket param is specified and assume that

Please run terraform fmt!

[hashicorp-cla]

All committers have signed the CLA.