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

[RFC] Add support for variable substitutions #253

Merged
merged 3 commits into from
Feb 12, 2021
Merged

[RFC] Add support for variable substitutions #253

merged 3 commits into from
Feb 12, 2021

Conversation

stefanprodan
Copy link
Member

@stefanprodan stefanprodan commented Jan 23, 2021

Problem

With Flux v1, users were able to perform string replacements on their Kubernetes manifests, before they were applied on cluster, by configuring Flux to run bash scripts with .flux.yaml. In Flux v2, we (the maintainers) made the decision to drop support for running arbitrary scripts inside the controller's pods, as this poses high security risks and makes the reconciliation process hard to debug and error prone.

While Helm offers advanced templating capabilities, we don't want to force our users into rewriting their Kubernetes manifests with Helm. If users need bash style variable substitutions, Flux v2 could offer this functionality in a declarative manner, without running arbitrary scrips inside the cluster.

Proposed solution

To support string replacements and other string manipulation functions, the GitOps Toolkit Kustomization API can be extended with post-build actions.

type KustomizationSpec struct {
	// PostBuild describes which actions to perform on the multi-doc 
	// YAML manifest generated by building the kustomize overlay.
	// +optional
	PostBuild *PostBuild `json:"postBuild,omitempty"`
}

type PostBuild struct {
	// Substitute holds a map of key/value pairs.
	// The variables defined in your YAML manifests
	// that match any of the keys defined in the map
	// will be substituted with the set value.
	// +optional
	Substitute map[string]string `json:"substitute,omitempty"`
}

With Kustomization.spec.postBuild.substitute users can provide a map of key/value pairs holding the
variables to be substituted in the final multi-doc YAML manifest, after kustomize build.

This offers basic templating for Kubernetes manifests including support
for bash string replacement functions e.g.:

  • ${var:=default}
  • ${var:position}
  • ${var:position:length}
  • ${var/substring/replacement}

The post-build substitutions are implemented with Drone's envsubst Go package.

Example

Assuming you have manifests with the following variables:

apiVersion: v1
kind: Namespace
metadata:
  name: apps
  labels:
    environment: ${env:=dev}
    region: "${region}"
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cluster-info
data:
  cluster-info.json: |
    {
      "cluster": {
        "name": "${cluster}"
      },
      "logs": {
        "region": "${region}"
      }
    }

You can specify the variables and their values in the Kustomization definition under the substitute post build section:

apiVersion: kustomize.toolkit.fluxcd.io/v1beta1
kind: Kustomization
metadata:
  name: apps
spec:
  interval: 5m
  path: "./apps/"
  postBuild:
    substitute:
      env: "prod"
      cluster: "eu-prod-1"
      region: "eu-central-1"

You can replicate the controller post-build substitutions locally using kustomize and Drone's envsubst:

$ go install github.com/drone/envsubst/cmd/envsubst

$ export env=prod
$ export cluster=eu-prod-1
$ export region=eu-central-1
$ kustomize build ./apps/ | $GOPATH/bin/envsubst 
---
apiVersion: v1
kind: Namespace
metadata:
  name: apps
  labels:
    environment: prod
    region: eu-central-1
---
apiVersion: v1
kind: ConfigMap
metadata:
  name: cluster-info
data:
  cluster-info.json: |
    {
      "cluster": {
        "name": "eu-prod-1"
      },
      "logs": {
        "region": "eu-central-1"
      }
    }

Feedback & Testing

Please comment on this PR and let us know your thoughts about this feature.

To install the kustomize-controller with post-build capabilities:

  1. deploy the GitOps Toolkit controllers
flux install
  1. apply the CRDs from this branch
kubectl apply -f https://raw.githubusercontent.com/fluxcd/kustomize-controller/envsubst/config/crd/bases/kustomize.toolkit.fluxcd.io_kustomizations.yaml
  1. deploy kustomize-controller AMD64 build of this branch
kubectl -n flux-system set image deployment/kustomize-controller manager=docker.io/stefanprodan/kustomize-controller:envsubst.1

@stefanprodan stefanprodan added the enhancement New feature or request label Jan 23, 2021
@stefanprodan stefanprodan requested review from a team, squaremo and hiddeco and removed request for a team January 23, 2021 09:42
docs/spec/v1beta1/kustomization.md Outdated Show resolved Hide resolved
docs/spec/v1beta1/kustomization.md Outdated Show resolved Hide resolved
@squaremo
Copy link
Member

I can see why there's a strong desire to have this kind of feature. I worry that

a. this design is not blessed by kustomize, so it locks people's configs to kustomize-controller; and,
b. it would preclude other such mechanisms, since having more than one feature that does a similar thing just makes life difficult.

I admit those are undirected concerns, i.e., they don't point at a better solution. This just seems like a moment for caution -- as it was with .flux.yaml, you probably won't get another run at this feature once it's released.

Regarding the specifics of the design, I am not a big fan of stringly substitutions in structured data. This is what makes Helm templating much more fragile than it needs to be. The limited power of bash-style replacement works to counter that misgiving though, because you can in practice (or by specification? unclear) only replace within a field value.

It does mean your sources are not (usually) themselves valid configurations, which is anathema to Kustomize (see the paragraphs under "Isn't this templating?" here; the discussion that follows is quite illuminating). Though I suppose you could patch in the variable-bearing values as a last layer of kustomization, so everything up to there is valid.

Thinking abut alternatives, the kustomize-blessed solutions are no use -- see the discussion linked just above for why "vars" are probably no good (and seem to have a precarious status); setters also have at best an ambiguous status, and won't work if you're applying to kustomize output since it doesn't preserve comments; and in general, the kustomize ethos is against anything that's too much like templating. The "escape hatch" provided is plugins, but they can't be used without making arbitrary binaries available to the controller, which usually considered a non-starter. Ho hum.

Regarding "other such mechanisms", I think there's two questions: 1. is the replacement mechanism good enough to serve the majority of uses? 2. can the API be extended to serve other purposes? (e.g., if the values to be substituted come from elsewhere, rather than being inline in the resource).

@scottrigby
Copy link
Member

scottrigby commented Jan 25, 2021

@squaremo nice timing! I was just following this rabbit hole and was about to post what I grokked from kubernetes-sigs/kustomize#2052. Could we not support kyaml setters (some of the comments from that linked discussion point there - ultimately kubernetes-sigs/kustomize#3492), for more consistency with what we do with image update automation?

@stefanprodan
Copy link
Member Author

stefanprodan commented Jan 27, 2021

  1. is the replacement mechanism good enough to serve the majority of uses?

For the users that I've talked to yes, I wouldn't have created this RFC from my own use. It's hard to say what the "majority" wants and if it fits all their use-cases.

  1. can the API be extended to serve other purposes? (e.g., if the values to be substituted come from elsewhere, rather than being inline in the resource).

Yes, like Helm valuesFrom, we will provide a postBuild.substituteFrom that can reference a ConfigMap or a Secret containing the key/value pairs.

Could we not support kyaml setters

The kyaml setters can't be used to achieve the same functionality as offered by this PR (default values, patch object API version, patch non-yaml ConfigMap data, preserve formatting).

@hiddeco
Copy link
Member

hiddeco commented Jan 27, 2021

a. this design is not blessed by kustomize, so it locks people's configs to kustomize-controller;

The part about it not being blessed by kustomize is true, but it does not lock people's configs to the kustomize-controller. As shown here, and by the fact that people are already using envsubst in their .flux.yaml (or are trying to).

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

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

The technical contents of this PR are okay, awesome work @stefanprodan 🍺

Before merge, I would love to see some more confirmation from users (preferably with examples on how it would be used).

@stevehipwell
Copy link

Without this general functionality (e.g. envsubst or gomplate/sprig templating) to work around the Kustomize limitations Flux v2 isn't usable out of the box; we would need to write a custom controller or potentially stick with v1 while we look for alternatives.

Our use case is managing multiple EKS K8s clusters with shared configuration; we have multiple Kustomize layers but we need a final variable substitution pass to set things like cluster name, account id and region as Kustomize can't handle this.

@zemaj
Copy link

zemaj commented Jan 28, 2021

We have a similar use case to @stevehipwell - managing multiple clusters (blue, green, staging etc...) from a single Flux repo. While most configs can be handled by stage specific patches, there are certain places where string substitution is the only sensible option. For example, in a large ConfigMap you might have a single line referencing the cluster name. A patch for each cluster with the entire ConfigMap in it is unmaintainable.

While string substitution might not be blessed by Kustomize, Flux has all the other pieces needed to provide multi cluster management from a single config. Without providing some kind of solution here, it breaks the last part of that chain for multi cluster management Flux.

This PR is a great solution to that problem and would allow us to use v2 which we could not currently.

@dieend
Copy link

dieend commented Feb 8, 2021

Hi! This is great! One small concern is how would we address the following case?

# in a configmap (for whatever reason)
myscript.sh: |
  echo ${my_env}
variable: ${my_env:=dev}

When we substitute it seems like it'll replace the defined variable in the scripts. We probably can ignore, as long as ignoring is the decided solution and work accommodating on that limitation.

@stefanprodan
Copy link
Member Author

When we substitute it seems like it'll replace the defined variable in the scripts.

The solution here is to prefix the vars replaced by Flux to avoid conflicts, for example flux_env instead of env.

@hiddeco
Copy link
Member

hiddeco commented Feb 10, 2021

@stefanprodan it might be an idea to document this in the spec to warn people about it.

Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
@stefanprodan
Copy link
Member Author

@hiddeco I've added a note on prefixing variables.

Signed-off-by: Stefan Prodan <stefan.prodan@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants