-
Notifications
You must be signed in to change notification settings - Fork 72
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
Fix Terraform secrets parsing & URLs propagations #1702
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM, but I didn't test.
# Read file `secrets.env` and return a list of tuples where first element is the key and the second is the value | ||
secrets_all_matches = fileexists(var.secrets_file) ? regexall("(.+?)=(.*)", file(var.secrets_file)) : [] | ||
# Drop all the commented lines (starting with '#') and remove quotationmarks around the values | ||
screts_valid_matches = [for tuple in local.secrets_all_matches : [tuple[0], trim(tuple[1], "\"'")] if !startswith(tuple[0], "#")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe trim also tuple[0]
because:
# SECRET=123
should be ignored as well.
# Read file `secrets.env` and return a list of tuples where first element is the key and the second is the value | ||
secrets_all_matches = fileexists(var.secrets_file) ? regexall("(.+?)=(.*)", file(var.secrets_file)) : [] | ||
# Drop all the commented lines (starting with '#') and remove quotationmarks around the values | ||
screts_valid_matches = [for tuple in local.secrets_all_matches : [tuple[0], trim(tuple[1], "\"'")] if !startswith(tuple[0], "#")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: screts_valid_matches
-> secrets_valid_matches
@@ -2,6 +2,12 @@ locals { | |||
uuid = uuid() | |||
tmp_dir = "/tmp/${var.name}#${local.uuid}" | |||
tmp_archive = "/tmp/${var.name}#${local.uuid}.zip" | |||
# Read file `secrets.env` and return a list of tuples where first element is the key and the second is the value |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spent some more time playing with regexall
on https://terraform-online-console.com/. Here was a quick playlaod if you're interested:
regexall("(.+?)=(.*)",
<<EOT
SECRET=value
SECRET=value= with=equals
abc=123
WORKS tooo = asd
a=1 b=2
MULTILINE=""
#abc=123
# abc=123
abc=123
abc=1==23
abc=
=
EOT
)
I think it would be nice to add a comment here about what we consider valid (that's the most improtant). I think it should handle at comments and ignore trailing/leading spaces.
Duplicates seem reasonable to allow (e.g. dotenv allows overrides), but you've mentioned TF doesn't handle that. That could be noted as well. We don't support multiline secrets, but that is not a common feature anyway (dotenv doesn't support it either).
Secrets are already validated by the validator, so many edge-cases won't reach the Terraform recipes. I've improved the secrets parsing on the Terraform end anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 LGTM. I spent some time thinking about (reasonable) potential edge cases but haven't come up with any...
packages/airnode-deployer/terraform/aws/modules/function/variables.tf
Outdated
Show resolved
Hide resolved
packages/airnode-deployer/terraform/aws/modules/function/variables.tf
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Airnode heartbeats returns gateway urls as <gateway_url>/uuid for GCP but as <gateway_url> (no uuid) for AWS
I like how using variables e.g. local.<gateway>_url
replaced the duplicated module.<gateway>.api_url
references and solved this missing AWS uuid bug 👍
secrets = { for tuple in local.secret_lines_matched : tuple[0] => trim(tuple[1], "\"'") } | ||
secrets_lines_matched = [for line in local.secrets_lines_uncommented : regex("^([^[:space:]]+?)=(.*)$", line) if can(regex("^([^[:space:]]+?)=(.*)$", line))] | ||
# Convert the list to a map, remove quotation marks around the values. When duplicate keys are encountered the last found value is used. | ||
secrets = merge([for tuple in local.secrets_lines_matched : { (tuple[0]) = trim(tuple[1], "\"'") }]...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit strange syntax, but I trust you it does what it should. (The dots at the end).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 code LGTM, but I didn't test.
9ef1a8c
to
87e9f4d
Compare
Close #1698
Also solves issues raised by Mertcan (https://api3workspace.slack.com/archives/C02AYRX8D89/p1679929209604659)
Needs to be backported to v0.10 as well.