Store terraform launchtemplate userdata in plaintext rather than b64 #9340
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rifelpet The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
|
|
When is the plan to drop terraform 0.11? |
|
I'd love for us to drop terraform 0.11 support asap, but we should probably add deprecation warnings and announce it according to our deprecation policy. Perhaps another office hours topic? |
k8s-ci-robot
added
the
needs-rebase
| if b64d != "" { | ||
| b64UserDataResource := fi.WrapResource(fi.NewStringResource(b64d)) | ||
| if d != nil { | ||
| userDataResource := fi.WrapResource(fi.NewBytesResource(d)) |
There was a problem hiding this comment.
I suppose i could check the value of the terraform feature flag here and conditionally encode the userdata contents, but that feels ugly. any other ideas?
There was a problem hiding this comment.
or add a new target.AddBase64File function that wraps target.AddFile
There was a problem hiding this comment.
Seems like it's in the nature of the feature flag to be ugly. A nice reminder that has to go away soon. 😄
I would go with the flag.
There was a problem hiding this comment.
done 👍 hack/verify-terraform.sh passes too. I'm not sure why there isnt a prow job for it, i'll look into creating one.
There was a problem hiding this comment.
LGTM, but still WIP and also the new job will need some work to pass.
This makes it easier to grok the userdata contents
k8s-ci-robot
removed
the
needs-rebase
|
/test pull-kops-verify-terraform |
|
/test pull-kops-verify-terraform |
| @@ -36,7 +36,7 @@ while IFS= read -r -d '' -u 3 test_dir; do | |||
| cluster=$(basename "${test_dir}") | |||
| kube::util::array_contains "${cluster}" "${CLUSTERS_0_11[@]}" && tag=$TAG_0_11 || tag=$TAG_0_12 | |||
|
|
|||
| docker run --rm -it -v "${test_dir}":"${test_dir}" -w "${test_dir}" --entrypoint=sh hashicorp/terraform:$tag -c '/bin/terraform init >/dev/null && /bin/terraform validate' || RC=$? | |||
| docker run --rm -v "${test_dir}":"${test_dir}" -w "${test_dir}" --entrypoint=sh hashicorp/terraform:$tag -c '/bin/terraform init >/dev/null && /bin/terraform validate' || RC=$? | |||
There was a problem hiding this comment.
Any reason the tag also contains patch version?
For 0.11 I not much will change and for 0.12 I think we want to know if something breaks.
There was a problem hiding this comment.
Hashicorp doesn't have major.minor tags: https://hub.docker.com/r/hashicorp/terraform/tags
I suppose we could keep the 0.11.14 tag and use latest for 0.12+ ? I may address that in a followup PR though.
rifelpet
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/lgtm |
k8s-ci-robot
added
the
lgtm
AWS launch templates require their userdata be submitted in base64. terraform has a filebase64 function for this purpose, in fact their aws_launch_template example uses it. Using it allows us to store the userdata contents in plaintext and terraform performs the encoding, rather than storing it encoded. This makes it easier to grok the userdata contents and any changes.
I'm not super happy with changing the AddFile function signature and updating every reference to it in cloudup.
WIP while I think of better alternatives (feedback welcome)