Skip to content
This repository was archived by the owner on Jan 25, 2023. It is now read-only.

Addresses an issue with logging configuration with improper for_each iterator #242

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Addresses an issue with logging configuration with improper for_each iterator #242

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3ee2f5d
[n/a] revert the once
dchernivetsky Apr 12, 2021
58427ef
[n/a] use can for proper bool
dchernivetsky Apr 12, 2021
3fdd8d8
[n/a] missing paren
dchernivetsky Apr 12, 2021
281d73b
[n/a] remove use of can
dchernivetsky May 24, 2021
a73e7bd
Update modules/vault-elb/main.tf
dchernivetsky May 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions modules/vault-elb/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ resource "aws_elb" "vault" {

# optional access_logs creation
dynamic "access_logs" {
for_each = var.access_logs == null ? [] : ["once"]
for_each = var.access_logs == null ? [] : [ var.access_logs ]

content {
enabled = lookup(access_logs.value, "enabled", lookup(access_logs.value, "bucket", null))
enabled = can(lookup(access_logs.value, "enabled")) ? lookup(access_logs.value, "enabled") : can(lookup(access_logs.value, "bucket"))
Copy link
Collaborator

@brikis98 brikis98 Apr 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link

@annawoodall annawoodall May 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@brikis98 brikis98 May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dchernivetsky dchernivetsky May 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

bucket = lookup(access_logs.value, "bucket", null)
bucket_prefix = lookup(access_logs.value, "bucket_prefix", null)
interval = lookup(access_logs.value, "interval", null)
Expand Down