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

add replacements field #1594

Closed

Conversation

Liujingfang1
Copy link
Contributor

The replacement fields is to support substitution

  • Both the source and destination need to be specified in kustomization.yaml
  • The source can be from a fieldref of one resource
  • The source can be from a literal value as well
  • The destination has a Target and a list of fieldrefs, where Target uses Selector to filter targets, which is also used in the patches for multi objects.

Example:

replacements:
  - from:
      value: nginx:newtag
    to:
      target:
        kind: Deployment
      fieldrefs:
        - spec.template.spec.containers[name=nginx].image
  - from:
      value: busybox:latest
    to:
      target:
        kind: Deployment
      fieldrefs:
        - spec.template.spec.containers.1.image
  - from:
      objref:
        kind: Deployment
        name: deploy
      fieldref: spec.template.spec.containers
    to:
      target:
        kind: Deployment
        name: deploy2
      fieldrefs:
      - spec.template.spec.containers

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 7, 2019
@Liujingfang1 Liujingfang1 requested a review from monopole October 7, 2019 16:08
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Liujingfang1

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 7, 2019
@tkellen
Copy link
Contributor

tkellen commented Oct 7, 2019

If it was possible to source the value of a replacement from a file this could resolve a lot of additional use cases this feature currently can't cover.

@Liujingfang1
Copy link
Contributor Author

@tkellen Why do you need to put the literal values in a file? You can put them directly in kustomization.yaml. If you need to overwrite some of those values in overlay, you can replacements in the overlay kustomization.yaml as well.

@tkellen
Copy link
Contributor

tkellen commented Oct 8, 2019

Because I want to generate them in CI and commit them and re-use the values in numerous kustomization.yml files which effectively become boilerplate.

@tkellen
Copy link
Contributor

tkellen commented Oct 8, 2019

Or, if you imagine that I have a dev/preprod/prod cluster each with a different RDS host I want to check in a plain text file at the time the cluster is created and re-use that file in every application's kustomization yaml that requires a database connection.

@tkellen
Copy link
Contributor

tkellen commented Oct 8, 2019

Additionally and critically missing from this feature is the ability to interpolate the literal value with an existing other value. For example, when using an external secret store (e.g. hashicorp vault, aws secrets manager, etc) and authoring an application who looks up secrets at runtime, the secret paths for the application to retrieve are sent in as environment variables. I need to capture the prefix from a shared external file and append the application specific values to it.

@Liujingfang1
Copy link
Contributor Author

Because I want to generate them in CI and commit them and re-use the values in numerous kustomization.yml files which effectively become boilerplate.

Are those values different in every CI job? If most of them are the same, you can update your Kustomize base to use those values directly. If they are different, you can write some script to provide them to kustomization.yaml. Later we will develop the edit commands for replacements. You can leverage it in scripts.

More about the edit commands, it should support reading from an environment variable. For example

kustomize edit add replacements --from-value ${ENV} ...

will read the value of ${ENV} and write it into kustomization.yaml.

Or, if you imagine that I have a dev/preprod/prod cluster each with a different RDS host I want to check in a plain text file at the time the cluster is created and re-use that file in every application's kustomization yaml that requires a database connection.

Sounds like you can put this database connection into a ConfigMap or a Secret. You can put that ConfigMap or Secret in to base and use it for every application.

@tkellen
Copy link
Contributor

tkellen commented Oct 8, 2019

ConfigMaps refs for environment variables only cover the case where the entire value is used wholesale. K8s offers no affordance for interpolating values (e.g. the case where a cluster-wide secret prefix is consumed by hundreds of microservices who append application specific values to it). However, using ConfigMaps for this purpose does not work with kustomize today because it cannot handle the same configmap being used multiple times.

Here is a simple example:

mkdir test
cd test
mkdir -p projects/foo/manifests projects/bar/manifests environment
printf domain.com > environment/domain
printf dev > environment/name
printf -branch > environment/branch
cat <<EOF > environment/kustomization.yml
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
configMapGenerator:
  - name: environment
    files:
      - name
      - domain
      - branch
vars:
  - name: ENV
    objref:
      apiVersion: v1
      kind: ConfigMap
      name: environment
    fieldref:
      fieldpath: data.name
  - name: DOMAIN
    objref:
      apiVersion: v1
      kind: ConfigMap
      name: environment
    fieldref:
      fieldpath: data.domain
  - name: BRANCH
    objref:
      apiVersion: v1
      kind: ConfigMap
      name: environment
    fieldref:
      fieldpath: data.branch
generatorOptions:
  disableNameSuffixHash: true
EOF
cat <<EOF > projects/foo/kustomization.yml
namespace: foo
resources:
  - ../../environment
  - manifests/ingress.yml
EOF
cat <<EOF > projects/bar/kustomization.yml
namespace: bar
resources:
  - ../../environment
  - manifests/ingress.yml
EOF
cat <<'EOF' > projects/bar/manifests/ingress.yml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: bar
spec:
  rules:
    - host: bar$(BRANCH).$(ENV).$(DOMAIN)
      http:
        paths:
        - backend:
            serviceName: bar
            servicePort: http
EOF
cat <<'EOF' > projects/foo/manifests/ingress.yml
apiVersion: extensions/v1beta1
kind: Ingress
metadata:
  name: foo
spec:
  rules:
    - host: foo$(BRANCH).$(ENV).$(DOMAIN)
      http:
        paths:
        - backend:
            serviceName: foo
            servicePort: http
EOF
cat <<EOF > kustomization.yml
resources:
  - projects/foo
  - projects/bar
EOF
tree
kustomize build .

Finally, I don't want to use kustomize edit, ever. I want to write to plain text files (e.g. VERSION), and leave my kustomization yaml files alone.

@tkellen
Copy link
Contributor

tkellen commented Oct 8, 2019

Apologies if my comment about not wanting to use kustomize edit seemed adversarial.

I can explain why in a more nuanced fashion:

K8s isn't the only component in a modern infrastructure repo that needs access to shared environment/cluster-wide values. Operators should not be required to encode their source of truth in a kustomization.yml. Our shell scripts should not need to know how to parse YAML. The kustomize project does not need an edit command to cover every use case sourcing a file already solves.

I understand that refusing to succumb to dollar values and templating constructs is what paved the way for kustomize to come into being but you're throwing the baby out with the bath water if you don't put some of that power back into the kustomization.yml file proper.

I truly realize I should roll up my sleeves and start opening PRs if I really want to see things happen but would the maintainers of this project please read this and internalize what it does? It's likely an overreach but I just can't understand how the power and clarity of what it does isn't moving to you given what you're suggesting users do instead.

#318 (comment)

I agree wholeheartedly we shouldn't litter k8s manifest files with variables. I either have a fundamental misunderstanding about how this software works or there are massive feature gaps for cleanly handling a myriad standard real world use cases that operators encounter.

Note: I am aware that #1594 (comment) would be better expressed using patches but I hadn't realized that six months ago when I first shared it

@Liujingfang1
Copy link
Contributor Author

@tkellen The example you wrote perfectly explained that you can use values from files through ConfigMap. You can optimize it by

apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
configMapGenerator:
  - name: environment
    envs:
    - env.txt

where env.txt contains

name=somename
domain=somedomain
branch=somebranch

Your script only need to update the file env.txt and no need to update kustomization.yaml.

If you mean you have to use disableNameSuffixHash: true to make it work. It's something we can fix. Then for each application, you have different env.txt, you'll have different ConfigMaps generated.

@tkellen
Copy link
Contributor

tkellen commented Oct 8, 2019

@Liujingfang1 Yes, it's true that I can use files in a way that is very indirect and quite confusing for everyone on my team but me and a few people who have gone fully down the rabbit hole with me. My use-case above is broken but this conversation has illuminated for me that this would fix it: #1600. If you would accept a PR for that this would go a long way for my team. I still view it as unnecessary indirection but it answers my use-cases in a way the current implementation suggested here cannot.

@tkellen
Copy link
Contributor

tkellen commented Oct 8, 2019

If you mean you have to use disableNameSuffixHash: true to make it work. It's something we can fix. Then for each application, you have different env.txt, you'll have different ConfigMaps generated.

I do not use this ConfigMap at all for anything other than the variable references it enables. This means my deployments are totally self-contained and can be rolled back without producing a sea of intermediate ConfigMaps. In my example above (#1594 (comment)) the ConfigMap is applied into the cluster and completely ignored. I am aware I could use a ConfigMapKeyRef or envFrom for some values but having so many different ways of populating environment variables is exceedingly difficult for my development team to follow. Instead, we populate a few common variables and use them wherever we need them.

@tkellen
Copy link
Contributor

tkellen commented Oct 15, 2019

I believe I'll be able to satisfy the majority of my desired use-cases with the closing of gh-1600 but I am curious, @Liujingfang1 or @monopole, if you can explain why you are (if you are) against the notion of supporting the sourcing of values from external files for this feature? If you are not, would you accept a PR that expands upon this work to add it?

@monopole
Copy link
Contributor

superseded by #1631

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants