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

Replacement poc #1631

Merged
merged 2 commits into from
Nov 21, 2019
Merged

Conversation

Liujingfang1
Copy link
Contributor

Some examples:

substitute the image for container where name=nginx

replacements:
  - from:
      value: nginx:newtag
    to:
      target:
        kind: Deployment
      fieldrefs:
        - spec.template.spec.containers[name=nginx].image

substitute the image for container with index 1.

replacements:
  - from:
      value: busybox:specific-version
    to:
      target:
        kind: Deployment
      fieldrefs:
        - spec.template.spec.containers.1.image

substitute the whole containers.

replacements:
  - from:
      objref:
        kind: Deployment
        name: deploy
      fieldref: spec.template.spec.containers
    to:
      target:
        kind: Deployment
        name: deploy2
      fieldrefs:
      - spec.template.spec.containers

@Liujingfang1 Liujingfang1 added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2019
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 14, 2019
@k8s-ci-robot k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 14, 2019
@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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2019
@monopole
Copy link
Contributor

please make this a plugin-only PR - a PR to integrate it can be made once the plugin is done.

pkg/types/replacement.go Outdated Show resolved Hide resolved
pkg/types/replacement.go Outdated Show resolved Hide resolved
pkg/types/replacement.go Outdated Show resolved Hide resolved
@Liujingfang1 Liujingfang1 force-pushed the replacement-poc branch 3 times, most recently from c4703ae to 8dd469e Compare October 14, 2019 22:13
@Liujingfang1
Copy link
Contributor Author

Restrict the change to plugin only by removing the adding this field to Kustomization type.

@tkellen
Copy link
Contributor

tkellen commented Oct 15, 2019

Can you please explain in detail why you are not considering adding the functionality of sourcing external files for the value in the from field?

@Liujingfang1
Copy link
Contributor Author

@tkellen Yes, I can explain that. With the proposed replacements, there is already way of sourcing the external files through a ConfigMap. This is only one time set up in the kustomization.yaml. Then the external file can be modified or updated by scripts. Besides that, the ConfigMap generator have other benefits that have already been developed. It handles different patterns of files such as loading the whole file content as the value or loading a key=value pairs into data. It allows different behavior such as Merge and Replace.

@tkellen
Copy link
Contributor

tkellen commented Oct 15, 2019

@tkellen Yes, I can explain that. With the proposed replacements, there is already way of sourcing the external files through a ConfigMap. This is only one time set up in the kustomization.yaml. Then the external file can be modified or updated by scripts. Besides that, the ConfigMap generator have other benefits that have already been developed. It handles different patterns of files such as loading the whole file content as the value or loading a key=value pairs into data. It allows different behavior such as Merge and Replace.

I agree that the primitive of sourcing external flat-files exists: use a configMapGenerator in a kustomization.

I also agree that the primitive of sharing those values widely exists: include the configMapGenerator-having kustomization in any other kustomization as a resource.

If we move to a world where Vars are removed (I think they should be) can we please include a pure-yaml form for, at a minimum, prepending or appending literal values to values pulled from object references? The example I have been using is a org/cluster/env-wide secret prefix that a fleet of a few hundred microservices need to interpolate application-specific values to.

@tkellen
Copy link
Contributor

tkellen commented Oct 15, 2019

Basically, to live with out Vars, I need some non-insanely verbose method of producing this:

envs:
  - name: APP_SECRET_PATH_PGPASSWORD
    value: /org-project/rds/app-name/pgpassword
  - name: APP_SECRET_PATH_KAFKA_PASSWORD
    value: /org-project/kafka/app-name/password
...

...where /org-project/ is referenced from somewhere else and app-name is referenced from somewhere else.

If I can get some agreement from you @Liujingfang1 or @monopole (that we intend to support this), I will give some thought to possible config formats that could satisfy this need and follow up here tomorrow.

@monopole
Copy link
Contributor

non-insanely verbose is a goal.

a form like this is possible

replacements:
- from: someConfigmapNameSource:someKey
  to:
  - someConfigmapNameTarget:someKey1
  - someConfigmapNameTarget:someKey2
  - someConfigmapNameTarget:someKey3
- from: someConfigmapNameSource:someOtherKey
  to:
  - someConfigmapNameTarget:someKey5
  - someConfigmapNameTarget:someKey6
  - someConfigmapNameTarget:someKey7

Everything here is an indirection. The value to inject (an int, a complex patch, whatever) comes from a value in a config map. Likewise the ns:GVK:name/path to multiple targets comes from some other value in some config map.

Why use configmaps? As a mechanism to not repeat yourself, because those same config maps can be used in multiple places and up the overlay stack.

The alternative is inlined patches, which we can already do, but there tends to be repetition.

@tkellen
Copy link
Contributor

tkellen commented Oct 16, 2019

So, what I hear is that a ConfigMap is the recommended method of transporting variables through the kustomization stack. This doesn't seem so awful... after you chew on it for a minute. Hell, it's infinitely recursive (if you can stand the directory structure required to make the recursion happen anyway).

As for your example, I'm having a hard time imagining constraints beyond pure masochism where I would choose to put my replacement targets in one file, my replacement values in another, and then bring them together by referencing them indirectly into yet another so I can indirectly replace values yet somewhere else. That said, I'm going to assume you mean it literally when you say "this form is possible" and you're just illustrating that the abstraction will be (or maybe already is, I haven't read the code yet) beautifully implemented.

Still nowhere in your response do I see how the form we have today can produce a replacement in the form: referencedValueOneliteralOnereferencedValueTwoliteralTwo.

Perhaps I'm just missing the obvious. Could you show me explicitly how you'd do this, or confirm that this isn't yet possible but should be?

@tkellen
Copy link
Contributor

tkellen commented Oct 16, 2019

replacements:
- from: [someConfigmapNameSource:someKey,thingOne,someConfigmapNameSource:someOtherKey,thingTwo]
  to:
  - someConfigmapNameTarget:someKey1
  - someConfigmapNameTarget:someKey2
  - someConfigmapNameTarget:someKey3

...could do what I've described above.

@tkellen
Copy link
Contributor

tkellen commented Oct 16, 2019

...and if you like that, what do you think about this?

replacements:
- from: someConfigmapNameSource:missingKey
  whenFromEmpty: default-to-this-instead
  to:
  - someConfigmapNameTarget:someKey1
  - someConfigmapNameTarget:someKey2
  - someConfigmapNameTarget:someKey3

...and this?

replacements:
- from: [someConfigmapNameSource:rootFilePathKey,someConfigmapNameSource:fileName]
  joinWith: /
  to:
  - someConfigmapNameTarget:someKey1
  - someConfigmapNameTarget:someKey2
  - someConfigmapNameTarget:someKey3

@Liujingfang1
Copy link
Contributor Author

@tkellen With this replacement POC, you can put your kustomization.yaml as this

configMapGenerator:
- name: cm
  envs:
  - env.txt

replacements:
- source:
    objref:
      kind: ConfigMap
      name: cm
    fieldref: APP_SECRET_PATH_PGPASSWORD
  target:
    objref:
      kind: Deployment
    fieldrefs:
    - where.the.ref.is.used
- source:
    objref:
      kind: ConfigMap
      name: cm
    fieldref: data.APP_SECRET_PATH_KAFKA_PASSWORD
  target:
    objref:
      kind: Deployment
    fieldrefs:
    - spec.where.it.is.used
    - another.place.where.it.is.used

Then the env.txt file contains

 APP_SECRET_PATH_PGPASSWORD= /org-project/rds/app-name/pgpassword
 APP_SECRET_PATH_KAFKA_PASSWORD=/org-project/kafka/app-name/password

You can have your script update /org-project/rds/app-name/pgpassword or /org-project/kafka/app-name/password.

With the approach @monopole suggested, all the source and target will be inside a ConfigMap. The kustomization.yaml have a clean interface as this.

@Liujingfang1
Copy link
Contributor Author

Added more tests for reading data from configmap for replacement.

@tkellen
Copy link
Contributor

tkellen commented Oct 16, 2019

@Liujingfang1 First, let me say how much I appreciate you taking the time to respond to my requests about management of environmental config. I really do appreciate it. I am not typing that to be ingratiating.

Next, I need to ask you to read this closely because you've told me what you just told me at least three times and each time I have given a different reason for why what you are describing is not a viable option for my work.

Before I explain why, let me also say that I do understand that what you describe is technically possible. I 100% hear you, know and have known for years that I can populate a file with fully specified environment variables and use them to configure a Kubernetes deployment.

There are a number of reasons I am not doing this. Let me enumerate a few:

  1. I am not using configMapGenerator to manage environmental config. This is intentional. All of my k8s Deployment environmental configs can be and are fully specified/inlined into the Deployment itself. From an operational perspective this meaningfully reduces complexity. When I roll back a Deployment that operation is atomic. There are no Kubernetes objects required for the rollback to complete.

  2. I am not using envFrom to manage environmental config. This is intentional. When using the kubectl command line the values for the environment are not shown when you describe a pod, it shows which ConfigMap to reference. In the k8s dashboard environments defined from a ConfigMap show the keys but the value is displayed as the number of bytes in the ConfigMap, not the literal value. When environment variables are defined inline it is immediately clear what they are configured to. Additionally, when the environment variables are defined inline it is exceedingly easy to modify them by hand using kubectl edit (only in ephemeral development contexts) to test new values. I could go on but I'll leave it at that.

  3. Every application I operate is configured to retrieve at least one secret (many have 3 or more) and the path to that secret needs to be derived dynamically based on a global secret prefix (specific to the cluster, environment, organization and project), a local value (the application name) and a literal string indicating the secret. Here is an example secret path, I have bolded the sections that need to be derived and left unbold the sections that are unique and must be specified literally: /orgname-projectname/provider-name/app-name/serect-path. This cannot be accomplished with an external file. It also, to my understanding thus far, cannot be accomplished by the feature this PR implements without adding something like what I have mentioned here: Replacement poc #1631 (comment)

  4. My justification for the need in item fixStuffDueToKubectlDeletion #3 is that developers quite often copy applications to use them as a template (even though templates may already exist). App names often change. Lessening the amount of duplication in config is absolutely essential to sanely operating a large and complex system. I know you know this because you are a core committer to this project.

Having read all of this, can you please recognize my request and tell me if a) what I am requesting is possible with this feature already and b) if it is not, do you think my suggested additions would be an acceptable addition to make it possible?

@Liujingfang1
Copy link
Contributor Author

@tkellen Thank you for explaining your requests and reasons behind. I read through them. First let me clarify something. The ConfigMap I listed in the comment is not necessary to be used together with a Deployment or envFrom. It's just an object which stores certain key:value pairs. You can keep your Deployemtn objects unchanged. The replacment POC is unrelated to that.

Then for the pattern /orgname-projectname/provider-name/app-name/serect-path, if you view it as 2 vars and kustomize replace those two vars, this is not supported in the replacement POC. We we design this new approach, we want to get rid of replacing a var in a fieldref. The targets are only scoped to be the whole fieldref. That's why I suggested to view the whole value of /orgname-projectname/provider-name/app-name/serect-path as one literal value that you can replace. Even with what you suggested, I don't think it's possible.

@tkellen
Copy link
Contributor

tkellen commented Oct 16, 2019

Thanks @Liujingfang1! I understand that you're recommending I could encode the values I ultimately want to replace into my Deployments in an external file, reference that using configMapGenerator and finally use the so-created ConfigMap to pluck values from it and place them into my Deployment. While I feel this is fairly convoluted, I'm fully on board with the ideals of this project and I am happy to accept that complexity. I will pursue my "please let me source my replacement value from a file directly" conversation at some future date.

All of that being said, I'm actually sort of excited about the syntax I proposed. You're right that what I posted above doesn't make sense now that I look at it more closely. Let me try again.

replacements:
- from: ["/","environmentConfigMap:secretPrefix","kafka","appConfigMap:name","password"]
  transform:
    join: "/"
    to: [0].value
    value:
    - name: APP_SECRET_PATH_KAFKA_PASSWORD
  to:
    target:
      kind: Deployment
    fieldrefs:
      - spec.template.spec.containers[name=nginx].env

...I think this form could be really powerful (and confusing, but, I think that is a given for this sort of config management). By analogy it's like saying users of kustomize have to write an AST if they need template-like constructs.

@tkellen
Copy link
Contributor

tkellen commented Oct 16, 2019

Just to state it explictly in case it isn't obvious, the above form transforms to this:

replacements:
- from:
  - name: APP_SECRET_PATH_KAFKA_PASSWORD:
    value: /org-project/something-ingestor/kafka/password
  to:
    target:
      kind: Deployment
    fieldrefs:
      - spec.template.spec.containers[name=nginx].env

...there are still some questions about how to get multiple values into the env but if this basic form is acceptable I am sure I could come up with some definition that would get it done.

@tkellen
Copy link
Contributor

tkellen commented Oct 17, 2019

Seems like you're flat out re-inventing the world today @monopole, but would you mind weighing in on the proposed syntax above?

@k8s-ci-robot
Copy link
Contributor

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.


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 k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 11, 2019
@Liujingfang1 Liujingfang1 force-pushed the replacement-poc branch 2 times, most recently from 4d59605 to 5cb4175 Compare November 11, 2019 16:44
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 11, 2019
@Liujingfang1
Copy link
Contributor Author

Added a test for the diamond shape base and overlays.

         base
        /    \
       a       b
        \    /
        combined

In this test, the replacement transformer is declared in overlay a and b, not in base.

@monopole
Copy link
Contributor

/lgtm

lets get it in as an example so its easier to try, and modify per @tkellen 's suggestions

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 21, 2019
@monopole monopole merged commit b86bea9 into kubernetes-sigs:master Nov 21, 2019
@tkellen
Copy link
Contributor

tkellen commented Nov 21, 2019

Thanks for the callout @monopole, was feeling sheepish about my hyperbolic comment a few weeks back. Almost out of the woods here work wise and ready to re-engage on some ideas. Is there any venue to find out about what kyaml is beyond reading the code? I've been watching that roll in.

@monopole
Copy link
Contributor

kyaml basically wraps and enhances sigs.k8s.io/yaml. some documentation coming in #1794
it's not a quick fix to vars.

@mattmceuen
Copy link

@monopole @Liujingfang1 I'm gathering from this and monopole's "state of the vars" issue that the replacement transformer is targeted to move from a POC example plugin into a built-in plugin, correct? Are there any plans/timeline/critical path you can share -- is it just a matter of time, or is there more evaluation that needs to happen first (or are you just waiting for someone to pick the work up)? Thanks!

@natasha41575
Copy link
Contributor

picking this up

@mattmceuen
Copy link

That's great to hear, thank you @natasha41575.

The airship project has been building additional features into our fork of the replacementtransformer; feel free to use this as a base or as inspiration! We'd love to adopt/contribute to a community supported transformer if there was feature parity.

https://github.com/airshipit/airshipctl/tree/master/krm-functions/replacement-transformer

@ghost
Copy link

ghost commented Mar 19, 2021

@Liujingfang1 is there going to be any documentation changes with your PR?

@natasha41575
Copy link
Contributor

@whillas see #3492 to track the state of this
We are rewriting it and building on it. Documentation will come after implementation is done

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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants