kustomize icon indicating copy to clipboard operation
kustomize copied to clipboard

ConfigMapGenerator should not load values from the build environment

Open KnVerey opened this issue 2 years ago • 6 comments

It recently came to @natasha41575 and my attention that the configMapGenerator has a bug where it will load values from the environment when a line in an env file has a key but no value. This bug dates back to the origins of Kustomize, when some of the original code was copied out of kubectl, and this undesirable behaviour (for kustomize, not kubectl) went unnoticed in the adaptation.

This behaviour is a clear violation of one of Kustomize's core principles, as documented in our Eschewed Features list:

image

Very unfortunately, someone not only discovered this bug, but made a PR to document it as a feature on kubernetes.io: https://github.com/kubernetes/website/pull/30348. Unfortunately no Kustomize maintainers were tagged on the PR, and it has been live since January 2022. 😞

As stated in Kustomize documentation screenshot above, any need for environment reading should be handled as part of a pre-build workflow. We can consider adding a kustomize edit subcommand for this specifically if there is demand from those currently relying on the bug. For example, it could look like kustomize edit add configmap my-configmap --from-env-file=env/path.env --from-env=ONE,TWO.

Since this behaviour has been around unnoticed by us for so long, our plan is to to deprecate it and remove it with a major version bump rather than treating it like a bug fix. We are reverting the docs change in https://github.com/kubernetes/website/pull/35522 and emitting a warning to affected users in https://github.com/kubernetes-sigs/kustomize/pull/4730. The final step is to actually fix the behaviour in the next major release, i.e. Kustomize v5.

KnVerey avatar Jul 28 '22 22:07 KnVerey

@KnVerey: This issue is currently awaiting triage.

SIG CLI takes a lead on issue triage for this repo, but any Kubernetes member can accept issues by applying the triage/accepted label.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot avatar Jul 28 '22 22:07 k8s-ci-robot

Hi @KnVerey, thanks for writing this up!

I am the documentor. Frankly I think it's a bad feature to eschew, and leaving in the ability to fetch manifests from remote targets while pulling this out feels like just making my life as a user hard. Kustomize is currently a single, fully usable tool for manifests. If we remove this, we will have to add another one on top of it. I'm totally sick of that. Judging by the number of issues I responded to, which resulted in me documenting it, other users don't want you to eschew this feature either.

The feature has been critical several times over the last couple years. The workaround proposed requires CD to know both the env var name, the configmap name, and the folder the configmap is in in the CD pipeline definition. This means that the CD pipeline must be custom for the particular project, which feels ridiculous and exactly the problem that skaffold + kustomize solve by allowing me to declare that in yaml next to the code instead. So either we need a hook system or another system to communicate into kustomize. This one works fine, has no unexpected consequences (you can still set to empty by including the =), and is a desired feature by your users. The major benefit is that grepping for the variable name leads to something in the codebase, rather than existing solely in the CD pipe.

I'd like to hear stronger reasons for removing it than "we didn't think it was a good idea 4 years ago". I totally understand the stance "we don't want to be a templating engine" but replacing this interface with calls to the CLI that take two pieces of info the caller doesn't have is not great - nor is suggesting users write a transformer to run sed. Maybe I'm missing some key info. I just really don't see how calling kustomize edit in a CD pipeline which may be remote from the codebase leads to a better developer experience.

A reasonable compromise would perhaps be a kustomize edit command to set a var at the top level kustomization - but those are going/gone too.

My 2c as a long-time user and fan currently running kustomize+skaffold as CD for ~25 services. Here's another user's perspective, which I pretty much agree with.

p.s. apologies for not tagging any maintainers on the docs PR. Perhaps you want to add an OWNERS for that page? p.p.s. nice to meet you, however briefly, at kubecon, hope your gateway api experiments went ok.

cc @justinsb - this is the one we were talking about at kubecon and also :wave:

use case

Here is the use case I originally dug it out for - extracting the build tag to provide to an operator which spawns more pods at the same tag, but needs the tag in a config file:

      - name: Extract skaffold image tags to the environment
        # some things need to know their tag;
        # this let's kustomize or skaffold populate it from the env
        continue-on-error: true # continue if no build artifacts are found in skaffold.yaml
        run: |
          skaffold build -q --dry-run | \
            jq -r '.builds | map(.tag |= split(":"))[0] |"BUILD_SKAFFOLD_TAG="+.tag[1]' >> $GITHUB_ENV

afirth avatar Jul 29 '22 17:07 afirth

Just to be clear, I don't think the environment fallthrough is a good way - but kustomize edit feels like a much worse way. Maybe I'm building my pipes wrong. Also, sorry for saying "it's intentional" in previous issues and the docs PR - I didn't consider the possibility of copypasta nor chase the git blame as far as I could have.

afirth avatar Jul 29 '22 17:07 afirth

This change is going to be a massive issue in our organization. We've considered it a feature for a long time (no idea how we found it originally though), and rely on it for templating ingress URLs, since most of our testing environments are generated on-demand. It's been very handy, since at least for us, the namespace and the cluster host has many times been necessary during manifest rendition.

Basically, a developer requests a testing environment. A namespace is generated on kubernetes with a random namespace name. That name is then part of the URL (e.g. product-$(namespace).$(cluster-host).com). I know there are some other places where namespace and cluster-host has been necessary (for example, if an application needs to send a callback URL to an external service, then it needs to know what the address of its own ingress became)

I could image a command kustomize edit set var KEY=VALUE or something would be a possible solution... but I've frankly never really understood why the kustomize edit... commands are fine because they also move data out of version control, don't they? I don't see how having a single file which declares all env variables we rely on is a worse practise than having several kustomize edit... commands as part of our CI pipeline before calling kustomize build.

I agree pretty much with @afirth on this topic, and really hope you reconsider this

Gikkman avatar Sep 15 '22 14:09 Gikkman

In our project we use this feature to inject credentials into ConfigMaps, using kustomize edit risks exposing the credentials in left-over files (we pipe kustomize build directly into kubectl apply).

We also hope you reconsider this.

ciis0 avatar Sep 16 '22 09:09 ciis0

I can understand your choice for ConfigMapGenerator, but this warning is also for secretGenerator.

Today, our secrets are stored in Gitlab CI variables and are masked. Edit kustomize file to set these secrets variables is not a good way.

cedric-orange avatar Sep 30 '22 10:09 cedric-orange

Sad to see this feature go. Please reconsider not changin it. It's really useful, when deploying from pipelines where secrets are injected. In addition its one of the 12 factor principles: https://12factor.net/config

max-wittig avatar Oct 20 '22 11:10 max-wittig

Closed by #4957

KnVerey avatar Jan 06 '23 16:01 KnVerey

I am sad to see that.

Despite many user comments arguing this feature interest you remove it.

In addition, there no response to handle users needs.

Cquad avatar Jan 06 '23 17:01 Cquad

I can't believe this happened in an OPEN SOURCE community? No matter this is a bug or a feature, given so many users have already relied on this behavior even in many production environments (including us), finding a way that's backward compatible or an alternative (apparently kustomize edit is not) would be a better approach,

lingxiankong avatar Jan 16 '23 07:01 lingxiankong

@KnVerey, can you share with us any specific reasons for the removal of this feature, besides alignment with Kustomize's core principles?

Many organizations that use CI for deployment rely on this feature for their Kustomize usage, and without it, the use of Kustomize would be limited as the only alternative would be using sed. It would be great if you could consider the feedback from the community before making such a decision based on religious reasons.

max-wittig avatar Feb 03 '23 09:02 max-wittig

Thank you to everyone on this issue for taking the time to provide your feedback. Rest assured that we did read and consider it, but at the end of the day we still felt removal was the right decision for Kustomize. As a long-standing behavior, it is not surprising that some folks have found ways in which it is useful in their workflows–this speaks to the truth of Hyrum's law. This is why we put the behavior through a noisy (via logging) deprecation process and considered its removal a breaking change, even though it was never intended and on the contrary was something our own docs promise Kustomize will never do.

Our guiding principles aren't just dogma for its own sake–they're promises we make to our users about how Kustomize works. They define what distinguishes it among the vast array of options in the configuration management landscape, and they help us keep making a great tool with a consistent vision and user experience across time and leadership changes. We regularly get feature requests to reverse every single one of these principles, and I'm sure if we accommodated them, many users would find those changes useful as well. However, it would likely break workflows for those taking advantage of the distinguishing features Kustomize was designed to provide, and it would certainly degrade Kustomize's user experience in the long run.

In this case, we're mostly talking about the "no build side-effects" principle, which is a promise to our users regarding the stability of the output of kustomize build specifically. This promise relates to the workflow Kustomize is optimized for, that is, one where the best practice of storing one's entire configuration in version control is followed. We know that not everyone who uses Kustomize follows this practice, and indeed Kustomize provides multiple workarounds that can be used to bridge the gap. In this case, kustomize edit is one, and plugins are another.

With plugins, it should be possible to write a custom generator that replicates the previous behavior exactly. Our dream for the KRM Functions Registry project is that it will provide a place for community members to share functionality like this that is useful to many but is out of scope for Kustomize core for one reason or another. That project is currently owned by the same over-stretched people as Kustomize, and as such has not made progress in a while. If someone on this thread is interested in picking that up, we would love to support that. In the meantime, plugins can be shared from any repo. Kustomize's extension system is an investment area on our roadmap, and we would be more than happy to receive bug reports and PRs to fix any obstacles faced in doing this.

In case someone creates such a plugin or has another constructive idea to share, I will keep this issue unlocked for that purpose. Please realize that the Kustomize team is a tiny group of real human beings, and comments maligning our intentions are hurtful and discouraging. Although we can never make everyone happy, we work very hard to make Kustomize a great tool both now and in the long term. If you would like to have more insight into and influence on the direction of the project, we are always looking for more contributors with the bandwidth to grow into maintainership roles!

KnVerey avatar Feb 08 '23 19:02 KnVerey

Thank you for taking the time to communicate the team's reasons and thought process, @KnVerey . It's funny you bring up Hyrum's law, as I was considering citing it myself in this discussion. Because there isn't really another way in Kustomize to replicate the removed behavior. Back when I started using Kustomize, the code's comments around this behavior mentioned it explicitly, so i always considered it a "known hack". I would have preferred that a stable alternative was present before it was removed. Currently, the company I work at has really bent-over backwards trying to come up with all kinds of hacky solutions, just so that unsuspecting developers wouldn't get caught in a snag when they updated their local Kustomize installations. While I'd love for every developer to be up-to-speed with every tool we use, the reality is quite different.

While plugins sort-of work, they require two additional flags everywhere Kustomize is used, and they require the plugin executable themselves to be present on each users machines (or, in this case, added to each source repository that might need it, which in our case are quite a few). And plugins aren't stable, as noted in their docs, so investing a lot of work is a major risk since it can change fast.

The KRM Function Registry would be a great addition to Kustomize, but as you say, the project hasn't been able to move forward in a while, so we can't rely on it either.

We ended up relying on pre-render hooks in Skaffold to pull env variables and create a configmap, and then use vars (even though it is deprecated too... one problem at a time I guess).

I see the vision Kustomize has, and mostly I agree. Excessive templating is a pain to deal with, and thus I still think Kustomize is a better and easier tool to work with than Helm. But reality keeps showing up, and I often encounter a few cases where some templating is needed. kustomize edit is a handy, though not quite enough I often feel (it tend to lead to very clutered CI scripts, that's more confusing than helpful). The host field in Ingresses are a recurring thing that needs templating, or the cluster host address needs to be available to the running app, so it can produce callback URLs to be sent to apps in other clusters.

My dream would be for Kustomize to adopt something similar to what Terraform does with variables. Explicitly declared variables, that are easy to see and work with. The Terraform CLI can even tell you what input variables are required. To me, it is the best middle ground I've encountered.

Thank you and the entire team for your hard work and your communication. Kustomize is a great tool, and I look forward to keep using it for a long time. I might not agree wholeheartedly with the vision, but Kustomize is still the best alternative out there.

Gikkman avatar Feb 16 '23 10:02 Gikkman

@ the rest of you I hope this helps, I believe it duplicates the old functionality in a pretty generic way

# replace all instances of "FOO=" in files ending in .env with "FOO=<value of $FOO>"
perl -pi -e 's/^(\w+)=$/$1=$ENV{$1}/g' `find ./ -name *.env`

those of you using this to inject secrets, please consider using something like external-secrets-operator instead - hydrating your manifests with secrets closes a lot of doors.

@KnVerey thank you very much for all your hard work and for your thoughtful response. Like others, the main use case for us is populating ingress resources, something like this:

- source:
    fieldPath: data.hostname
    kind: ConfigMap
    name: environment
  targets:
  - fieldPaths:
    - spec.tls.0.hosts.0
    - spec.rules.0.host
    select:
      kind: Ingress
      name: echo-server

afirth avatar Feb 22 '23 17:02 afirth

Given the fact that many have voiced the use of this 'previously considered a feature', that there is no replacement functionality, and that it had existed for some time, I think it would have been prudent to take a bit more time before ripping it out and at least consider offering an actual solution rather than just tossing it over a fence in the name of dogma.

tcarland avatar Feb 27 '23 14:02 tcarland

No worries, just use helm, argo or sed/bash - These tools have this feature because it is needed.

Whoever eschewed the academic edict to shun this feature may have been right when this was a virgin opensource hobby project but it is 2023, all other pipeline/manifest tools have the ability to accept env or cli parameter's, hoping you can stay current.

Thanks for your good work.

doug62 avatar Sep 06 '23 04:09 doug62