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

ConfigMapGenerator should not load values from the build environment #4731

Closed
KnVerey opened this issue Jul 28, 2022 · 16 comments
Closed

ConfigMapGenerator should not load values from the build environment #4731

KnVerey opened this issue Jul 28, 2022 · 16 comments
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Milestone

Comments

@KnVerey
Copy link
Contributor

KnVerey commented Jul 28, 2022

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: kubernetes/website#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 kubernetes/website#35522 and emitting a warning to affected users in #4730. The final step is to actually fix the behaviour in the next major release, i.e. Kustomize v5.

@KnVerey KnVerey added the kind/bug Categorizes issue or PR as related to a bug. label Jul 28, 2022
@k8s-ci-robot
Copy link
Contributor

@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 k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Jul 28, 2022
@KnVerey KnVerey added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jul 28, 2022
@afirth
Copy link
Contributor

afirth commented Jul 29, 2022

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 👋

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
Copy link
Contributor

afirth commented Jul 29, 2022

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.

@Gikkman
Copy link

Gikkman commented Sep 15, 2022

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

@ciis0
Copy link

ciis0 commented Sep 16, 2022

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.

@cedric-orange
Copy link

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.

@max-wittig
Copy link

max-wittig commented Oct 20, 2022

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

@KnVerey
Copy link
Contributor Author

KnVerey commented Jan 6, 2023

Closed by #4957

@KnVerey KnVerey closed this as completed Jan 6, 2023
@Cquad
Copy link

Cquad commented Jan 6, 2023

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.

@lingxiankong
Copy link

lingxiankong commented Jan 16, 2023

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,

@max-wittig
Copy link

max-wittig commented Feb 3, 2023

@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.

@KnVerey
Copy link
Contributor Author

KnVerey commented Feb 8, 2023

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!

@Gikkman
Copy link

Gikkman commented Feb 16, 2023

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.

@afirth
Copy link
Contributor

afirth commented Feb 22, 2023

@ 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

@tcarland
Copy link

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.

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this issue Apr 15, 2023
v1.27.1

No CLI change.

v1.27.0

API Change

* Adds feature gate NodeLogQuery which provides cluster administrators with
a streaming view of logs using kubectl without them having to implement a
client side reader or logging into the node.

Feature

* Added "general", "baseline", and "restricted" debugging profiles for
kubectl debug.
* Added "netadmin" debugging profiles for kubectl debug.
* Added --output plaintext-openapiv2 argument to kubectl explain to use old
openapiv2 explain implementation.
* Added e2e tests for kubectl --subresource for beta graduation
* Changed kubectl --subresource flag to beta (#116595, @MadhavJivrajani)
* Enable external plugins can be used as subcommands for kubectl create
command if subcommand does not exist as builtin only when
KUBECTL_ENABLE_CMD_SHADOW environment variable is exported.
* Kubectl will now display SeccompProfile for pods, containers and
ephemeral containers, if values were set.
* Kubectl: added e2e test for default container annotation
* Made kubectl-convert binary linking static (also affects the deb and rpm
packages).
* Promoted whoami kubectl command.
* Since Kubernetes v1.5, kubectl apply has had an alpha-stage --prune flag
to support deleting previously applied objects that have been removed from
the input manifest. This feature has remained in alpha ever since due to
performance and correctness issues inherent in its design. This PR exposes
a second, independent pruning alpha powered by a new standard named
ApplySets. An ApplySet is a server-side object (by default, a Secret;
ConfigMaps are also allowed) that kubectl can use to accurately and
efficiently track set membership across apply operations. The format used
for ApplySet is set out in KEP 3659 as a low-level specification. Other
tools in the ecosystem can also build on this specification for improved
interoperability. To try the ApplySet-based pruning alpha, set
KUBECTL_APPLYSET=true and use the flags --prune --applyset=secret-name with
kubectl apply.
* Switched kubectl explain to use OpenAPIV3 information published by the
server. OpenAPIV2 backend can still be used with the --output
plaintext-openapiv2 argument
* Upgrades functionality of kubectl kustomize as described at
https://github.com/kubernetes-sigs/kustomize/releases/tag/kustomize%2Fv5.0.0
and
https://github.com/kubernetes-sigs/kustomize/releases/tag/kustomize%2Fv5.0.1.
This is a new major release of kustomize, so there are a few
backwards-incompatible changes, most of which are rare use cases, bug fixes
with side effects, or things that have been deprecated for multiple
releases already:

 - kubernetes-sigs/kustomize#4911: Drop support for a very old, legacy
 style of patches. patches used to be allowed to be used as an alias for
 patchesStrategicMerge in kustomize v3. You now have to use
 patchesStrategicMerge explicitly, or update to the new syntax supported by
 patches. See examples in the PR description of
 kubernetes-sigs/kustomize#4911.
 - kubernetes-sigs/kustomize#4731: Remove a potential build-time
 side-effect in ConfigMapGenerator and SecretGenerator, which loaded values
 from the local environment under some circumstances, breaking kustomize
 build's side-effect-free promise. While this behavior was never intended,
 we deprecated it and are announcing it as a breaking change since it
 existed for a long time. See also the Eschewed Features documentation.
 - kubernetes-sigs/kustomize#4985: If you previously included .git in an
 AWS or Azure URL, we will no longer automatically remove that suffix. You
 may need to add an extra / to replace the .git for the URL to properly
 resolve.
 - kubernetes-sigs/kustomize#4954: Drop support for using gh: as a
 host (e.g. gh:kubernetes-sigs/kustomize). We were unable to find any usage
 of or basis for this and believe it may have been targeting a custom
 gitconfig shorthand syntax.

* [alpha: kubectl apply --prune --applyset] Enabled certain custom
resources (CRs) to be used as ApplySet parent objects. To enable this for a
given CR, apply the label applyset.kubernetes.io/is-parent-type: true to
the CustomResourceDefinition (CRD) that defines it.
* kubectl now uses HorizontalPodAutoscaler v2 by default.

Documentation

* kubectl create rolebinding -h

Bug or Regression

* Added (dry run) and (server dry run) suffixes to kubectl scale command
when dry-run is passed
* Changed the error message of kubectl rollout restart when subsequent
kubectl rollout restart commands are executed within a second
* Changed the error message to cannot exec into multiple objects at a time
when file passed to kubectl exec contains multiple resources
* Kubectl: enabled usage of label selector for filtering out resources when
pruning for kubectl diff
* kubectl port-forward now exits with exit code 1 when remote connection is
lost
@doug62
Copy link

doug62 commented Sep 6, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests