Clean the env vars of all TF_* flags #1850
Conversation
openshift-ci-robot
added
the
size/S
|
Would it be possible to whitelist TF_LOG? |
| for _, env := range environ { | ||
| splits := strings.Split(env, "=") | ||
| key := splits[0] | ||
| if strings.HasPrefix(key, "TF_") { |
There was a problem hiding this comment.
Doesn't this also unset TF_VAR_libvirt_master_memory and TF_VAR_libvirt_master_vcpu? We need a way to do that.
There was a problem hiding this comment.
If the memory requirements are inadequate you need to fix the template. Terraform is not for users.
There was a problem hiding this comment.
Yeah, we can have the right amount for memory in the template i guess. What about the VCPU? I guess it's time we implement suggestion of @cgwalters to autodetect that.
cmd/openshift-install/main.go
Outdated
| splits := strings.Split(env, "=") | ||
| key := splits[0] | ||
| if strings.HasPrefix(key, "TF_") { | ||
| os.Unsetenv(key) |
There was a problem hiding this comment.
Is this called early enough? Environment mutation is unsafe in the presence of threads.
|
The installer use aws-sdk, which allow certains envs that can be used to turn on debugging etc.. So i'm not sure why this is being forced? This only allows vars to changes at global level, which is already very selective... so i'm not sure how preventive this is. and we we do want to do that, can we move this to TF_LOG needs to be allowed. |
|
Looking at https://www.terraform.io/docs/configuration/variables.html#assigning-values-to-root-module-variables the env can only be used to set the variable of the root module like variables-aws.tf, which are already exposed through install-config to the user. So i don't see the use-case for removing the envs. /close |
|
@abhinavdahiya: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
|
/test all |
Such variables would allow a crafty user to directly manipulate terraform. We do not want users doing that. Terraform is NOT an API. It is a hidden completely internal implementation detail. So clear those env vars.
|
We are using TF_VAR_* on ovirt-e2e job, so this will always fail on our CI. [1] #3399 |
|
@eparis: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/close |
|
@abhinavdahiya: Closed this PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Such variables would allow a crafty user to directly manipulate terraform.
We do not want users doing that. So clear those env vars.