Resolve Vars at a directory level

[bearium]

fixes: #506

Adds functionality to resolve vars at a directory level, resolved vars will not propagate up and cause collisions, unresolved vars still propagate upwards/downwards.

Prefixes resolve at the time of var substitution and if referenced var changes at higher level already substituted var will not change.

[k8s-ci-robot]

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
• If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
• Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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.

[k8s-ci-robot]

Welcome @bearium!

It looks like this is your first PR to kubernetes-sigs/kustomize 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kustomize has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[bkuschel]

@Liujingfang1 Could you take a look at this? The current "only at the end" var substitution is very hard to work with. This pull request changes the behaviour to substitute the variable at the build level where its defined and bound (instead of at the end).

[jbrette]

Looks like braking kustomize. The value computed by kustomise is wrong. base-myServerPod does not exist anywhere, so kustomize is silently taking the wrong decision. Current behavior of kustomize is much better.

[bkuschel]

Kustomize isn’t taking any decisions. If you want the current behaviour you simply bind the variables at the top level kubernetes build as it does now “under the covers”.
That pod name doesn’t exist anywhere because isn’t subject to name reference updates as it’s not a field subject to that operation (it could be added to name references).
It’s working as intended and is clear: variable substitution happens at the time the variable is declared (with the value that exists at that time.)
I would argue the current implementation is broken and this fixes it.

[bkuschel]

Additionally, the current functionality does work with transformer resources that consume variables like secretGenerator.

@jbrette As a compromise, we could add an additional attribute level called "subsitituteImmediately" at the variable level, which uses the behaviour in this PR, otherwise default to the current "at the end" value substitution currently used.

[bearium]

Var now has a flag immediateSubstitution which if set to true, will use the new implementation, else use the old way.

[bkuschel]

@jbrette This behaviour is now behind a flag, current is default.

[bkuschel]

@Liujingfang1 @monopole Do see a problem with adopting this flag?

I feel that providing this optional behaviour change for vars can help with a host of use cases that require substitutions to occur at the level they are declared.
I understand that there is some consideration for deprecating vars in favour of #1631 but it would help tremendously as a stop gap measure until adopters have had a chance to migrate over.
Additionally, It can also help with such a transition as this new behaviour can be self contained as a transformer instead (which is what we did originally). So instead of deprecating, you could provide a transformer with this var behaviour with the caveat that it would do immediate substitution.

[pwittrock]

Thanks for your patience @bkuschel.

@Liujingfang1 is OOO and back early December. Perhaps @monopole could review, otherwise I'll make sure she reviews this when she returns.

[Liujingfang1]

@bearium Thank you for being patient with us. I took a look at the change and here is my thought.

1. When we introduced the vars, it is aimed to be used and resolved at the top level. Handling the vars at a directly level can't guarantee the correctness. In the test you added, at base1 and base2, the vars are already resolved. If the source of the vars changes in the top level kustomization, such as by adding the prefix or suffix. The vars won't be correctly resolved in the final output. The prefix or suffix adding in the top level kustomization will be missed.

2. The test case you created is basically a diamond shape kustomization overlays. I agree that we need to support them, but this is not necessary to be through vars. We recently added the ReplacementTransformer. We believe it is a better way of supporting substitutions. Take a look at the original PR. It can handle this type of use cases, please take a look at this test. Currently this transformer is an example transformer and it need some improvements to be moved to a builtin transformer. If you would like to contribute to this transformer, I can give you more details.

[ghost]

Is there any update on when this might be merged in? This seems like the best solution to a problem that I've had ever since I started using Kustomize. I'm currently just using @bearium 's branch which works well but I'm having to patch it in to lots of third party tools to replace Kustomize which is cumbersome.

[bkuschel]

@Liujingfang1 I love the approach with the replacement transformer and it would be great to retire this PR in favour of that but there is one use case that isn't addressed there that is covered by the VAR implementation; the ability to replace substrings. Here are some examples:

This is an ingress resource that needs a URL for auth-url because of an implementation details needs the full namespace qualified domain. Service also have a "prefix" which groups resources in a namespace by application (using the application name as prefix)

apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: component
  annotations:
    nginx.ingress.kubernetes.io/auth-url: http://$(PREFIX)-edge-auth.$(NAMESPACE).svc.cluster.local:8080/v1/auth

Sometimes configMaps need non-trivial replacements as well:

apiVersion: v1
kind: ConfigMap
metadata:
   name: messaging-configs
data:
   natsUri: "nats://$(PREFIX)-messaging-nats-client:4222"
   natsStreamingClusterId: "$(PREFIX)-messaging-nats-streaming-cluster"
   someConfigFIle: |-
     bunch:
       ofstuff:  "$(SOMETHING)"
     aURL: "http://$(PREFIX)-some service.$(NAMESPACE).svc.cluster.local:8080/v1"

There are also circumstances with CRDs where kubernetes (server side) variable expansion cannot be relied upon, opening up a host of circumstances where compound substitutions needs to occur within one value.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: bearium
To complete the pull request process, please assign liujingfang1
You can assign the PR to them by writing /assign @liujingfang1 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Liujingfang1]

@bkuschel We did consider the use case for replacing sub string when proposing the replacement POC but decided to not support this. Here is the reason. The replacement transformation can be applied more than once. It should have the same effect as being applied only once. Replacing a sub string by some marks like "$()" or by regexp matching doesn't guarantee this.

On the other side, users can always format the value for the whole string outside of the k8s resource files. For example in kustomization.yaml file.

replacements:
- source:
    # value should be the format of http://$(PREFIX)-edge-auth.$(NAMESPACE).svc.cluster.local:8080/v1/auth
    value: http://pre-edge-auth.ns.svc.cluster.local:8080/v1/auth
  target:
    objref:
      kind: Ingress
    fieldrefs:
    - metadata.annotations[nginx.ingress.kubernetes.io/auth-url]

[bkuschel]

The drawback of this is that potentially, especially in a microservice architecture, a replacement would need to be created for each field that would have namespace/prefix embedded in it. So changing a namespace or prefix (very typical scenarios) could require non-trivial changes to replacement values across multiple kustomization.yaml files.

I'll play back and example we are currently dealing with related to PREFIX to demonstrate the problem with current (similar) mechanisms.

---

Knowing that current VAR substitution happens at the very end, we qualify all resources with $(PREFIX) in order to create logical application namespaces within a kubernetes namespace. ("$(PREFIX)" is the prefix!). I believe this is a very common scenario.

To do this we are using the PrefixTransformer with additional nameReferences to cover additional fields not defaulted.
As this does not cover all cases, we additionally use patches to insert "$(PREFIX)" into configmap URLS, environment variables, files, substrings etc, that are not subject to simple nameReference replacement.

In the top level kustomization.yaml file, we use VAR substitution to substitute $(PREFIX) with a var reference to an entry in a ConfigMap.
So now, only 1 ConfigMap key needs to be patched and all prefixes are substituted across the board. It works, even hashes work.

To do this with replacements, we would still need to use the prefix transformer (as we would not be able to do this with replacementTransformer; partial string again). Additionally, we would need to create a replacement for all the other fields that could potentially have PREFIX embedded in them.

The problem with PREFIX (and NAMESPACE to a lesser extent) is that it is used everywhere a service needs to communicate with another service using a URL; to modify a configuration with a URL reference to another service, prepended with the prefix.

If it were possible to stipulated, by decree, that URLs to other services be specified in a consistent way as environmental variable references to configMap across the board (the simplest scenario), at a minimum we would need a value replacement for each service name for each dependent service unless we break up the URLS into constituent parts, so that the substitution could be done be a fieldref.
Ex.

ex:

replacements:
- source:
    # value should be the format of http://$(PREFIX)-edge-auth.$(NAMESPACE).svc.cluster.local:8080/v1/auth
    value: http://pre-edge-auth.ns.svc.cluster.local:8080/v1/auth
  target:
    objref:
      kind: Ingress
    fieldrefs:
    - metadata.annotations[nginx.ingress.kubernetes.io/auth-url]
- source:
    # This would require a substitution per service name
    value: http://myprefix-servicename:1234/
  target:
    objref:
      kind: ConfigMap
    fieldrefs:
    - data.servicenameUrl
- source:
    # This would not, you could get away with one for all
    value: myprefix
  target:
    objref:
      kind: ConfigMap
    fieldrefs:
    - data.servicenUrlPrefix

In this case you'd have a minimum of two, reducing this simple scenario futher to 1 would require a re-work the nginx-ingress-controller, which for us is out of the question. as it's third party component and are not interested in maintaining a hacked up version supporting a prefix field.

Again, this is the simplest scenario for the PREFIX case.

So basically, the consequence of more rigidity around replacement in yaml values, is stricter adherence to a resource schema/model, which seems to somewhat compromise one of the stronger cases for kustomize (last-mild modification).

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.