Skip to content
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

Vars overriden by VarsFrom #1096

Closed
wants to merge 1 commit into from

Conversation

kuritka
Copy link

@kuritka kuritka commented Nov 3, 2023

Fixes #634

I apologize for accidentally closing the previous PR I did git reset -hard <start commit> and git push -f on my fork which automatically closed the upstream PR. It behaved a bit differently than I expected.


The purpose of this change is to enable overriding variables mechanism. I want to define default values (specified in varsFrom via configMap or Secret) and customize them via vars.

All I do is just change the order of writing to the variables + update overriding in 090xxx test.

Example:
create a configMap named defaults-cm with the value costCenter=123456 and add it to all Terraform manifests.

apiVersion: v1
kind: ConfigMap
metadata:
  name: default-cm
data:
  costCenter: 123456

Everything that tf-runner will run will have the default variable costCenter=123456, because all Terraform Manifests have the field:

spec:
  varsFrom:
    - kind: configMap
      name: defaults-cm

A request is received that the part of the infrastructure that is specified by the Terraform manifest named override has costCenter=888888. I can do this easily, because I override the value via vars:

kind: Terraform
Metadata:
  name: override
spec:
  vars:
  - name: costCenter
    value: 88888

Thanks to particular Terraform.Spec.Vars I can override globaly defined variables specified in Terraform.Spec.VarsFrom and easily customize the entire infrastructure.

Copy link
Collaborator

@yitsushi yitsushi left a comment

Choose a reason for hiding this comment

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

Can we add a note about it here: https://github.com/weaveworks/tf-controller/blob/main/docs/use_tf_controller/to_set_variables_for_Terraform_resources.md?plain=1#L30

Now it says "the controller will use
the lattermost instance of the key passed to varsFrom" only, and I found no docs about the priority. This would be a good moment to add this info (".spec.vars takes precedent over varsFrom" or something like that)

The purpose of this change is to enable overriding variables mechanism. I want to define default values (specified in `varsFrom` via configMap or Secret) and customize them via `vars`.

All I do is just change the order of writing to the variables

Example:
create a configMap named `defaults-cm` with the value `costCenter=123456` and add it to all Terraform manifests.
```yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: default-cm
data:
  costCenter: 123456
```

Everything that tf-runner will run will have the default variable `costCenter=123456`, because all Terraform Manifests have the field:
```yaml
spec:
  varsFrom:
    - kind: configMap
      name: defaults-cm
```

A request is received that the part of the infrastructure that is specified by the Terraform  manifest named `override` has `costCenter=888888`. I can do this easily, because I override the value via vars:

```yaml
kind: Terraform
Metadata:
  name: override
spec:
  vars:
  - name: costCenter
    value: 88888
```

Thanks to particular Terraform.Spec.Vars I can override globaly defined variables specified in Terraform.Spec.VarsFrom and easily customize the entire infrastructure.

Signed-off-by: Michal Kuritka <[email protected]>
@kuritka kuritka force-pushed the change-vars-reading-order branch from eb0e7a0 to db3a897 Compare November 6, 2023 14:49
@kuritka
Copy link
Author

kuritka commented Nov 6, 2023

@yitsushi, thx for info, I amended the change. Let me know if you like to do any changes

Copy link
Collaborator

@yitsushi yitsushi left a comment

Choose a reason for hiding this comment

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

Thinking more about this, the reverse logic would make more sense to me in some cases. The question is, which one is more common.

1. Option (original logic)

  • I create a Terraform resource with default variables as vars
  • I create the variables secret and if something is not defined in there it falls back to the default vars

2. Option

  • I create the values secret with all options
  • I create one or more terraform objects using that "default value" secret and override values as vars.
  • (this can be achieved with adding a new varsFrom with new values as it will use the value from the last definition of a key).

@chanwit what's your opinion.

@k0da
Copy link
Contributor

k0da commented Nov 6, 2023

Our use case:
We manage fleet of dozens k8s clusters from this. Each Terraform object have varsFrom: global-vars (it defines environment, kubernetes version, etc). Now we need to start rolling out a new k8s upgrade. So we start with one object first, it overrides global-vars and performs upgrade. Once we confident enough we enable global (the rest) rollout by changing in global-vars reference. And remove individual overrides.

Right now. We have to temporarly remove varsFrom reference and copy every k/v pair from there into an object itself to be able to override single var.

@yitsushi
Copy link
Collaborator

yitsushi commented Nov 6, 2023

Right now. We have to temporarly remove varsFrom reference and copy every k/v pair from there into an object itself to be able to override single var.

@k0da: With the current logic. You can create an "update-vars" secret and add to the end of the list of fromVars, it will override the value from the global.

That even looks more reasonable to me in this case as you can have a global config set everywhere and an upgrade values you just add to terraform resources and you can simply remove the secret reference when you updated the global, you get the same upgrade values that way on all "test" upgraded resources.

  varsFrom:
  - kind: ConfigMap
    name: global-config
  - kind: ConfigMap
    name: upgrade-config

@k0da
Copy link
Contributor

k0da commented Nov 6, 2023

@yitsushi I see, but this way you'll have to track additional resource. controller won't kick a reconcilation on actual configmap change. And are configmap/secret lists are sorted?

@yitsushi
Copy link
Collaborator

yitsushi commented Nov 6, 2023

And are configmap/secret lists are sorted?

They are parsed in the order you define.

Note that in the case of the same variable key being passed multiple times, the controller will use
the lattermost instance of the key passed to varsFrom.

https://github.com/weaveworks/tf-controller/blob/main/docs/use_tf_controller/to_set_variables_for_Terraform_resources.md?plain=1#L30-L31

controller won't kick a reconcilation on actual configmap change

when you add it will reconcile and after that it will reconcile in the defined interval. So for example

  1. define an upgrade config map
  2. update the terrafrom object
    • add the new configmap as the last item in the list
    • change the interval to a lower value so you get faster feedback (or use tfctl reconcile)

I don't say that's how it should be done, but it is possible to achieve the same thing with the current logic, and if I would have to do that I would create one upgrade config and add to all resources I want to upgrade before changing global options.

@k0da
Copy link
Contributor

k0da commented Nov 6, 2023

@yitsushi makes sense. Lets collect more feedback

@yitsushi
Copy link
Collaborator

yitsushi commented Nov 7, 2023

After a discussion with @chanwit, the current logic is intentional.

vars are visible as plain values with an intent to be overridden by CMs or Secrets.
values can be overridden by addition varFroms. It's promoting better security.

This PR would introduce a braking change and promote the use of vars over secrets which is something we should avoid. As I described above, the same effect can be achieved using the varFroms list by defining a different value set and put it at end end (or at least further down) the list.

As per the discussion with @chanwit, I'm closing this PR.

@yitsushi yitsushi closed this Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using nested variable blocks from a configmap/secret
4 participants