Kustomize API Proposal: Add an example with components #2438

apyrgio · 2020-05-05T13:13:05Z

Add a WIP example that showcases how components can be used. This example is purposefully similar to the multibases example, to showcase what's the current shortcoming of overlays, namely that we currently can't combine overlays that point to the same common base. Also, this example uses the KustomizationPatch notation to define a component, as seen in #2168.

For convenience, you can see the rendered example here.

k8s-ci-robot · 2020-05-05T13:13:13Z

Hi @apyrgio. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

apyrgio · 2020-05-05T13:41:38Z

For those who are not aware of the on-going discussion on this subject, let me add a preface:

This PR has been sent as a follow-up to a live discussion regarding composition support in Kustomize (#1251). See here for more details. Its purpose is to showcase how components would work, based on @pgpx's #2168 PR.

Feel free to comment on anything that seems off with this design, or if there's a use-case that you feel is not covered.

For instance, what I feel is not covered is the handling of relative paths. In this example, it's the secrets for LDAP/reCAPTCHA/etc. Since components are reusable, and probably provided by the upstream, they should be treated as read-only. So, we shouldn't store files in them so that the generators can pick them up. Instead, it may be better to resolve the file paths relative to the top-level overlay.

This of course is of lesser importance to the actual issue, but I'm mentioning it here for completeness.

apyrgio · 2020-05-05T14:02:43Z

Also, in case it's not clear. For those wandering if the example would work if we simply switched:

kind: KustomizationPatch

with:

kind: Kustomization

resources:
  - ../../base

the answer is:

$ kustomize build overlays/dev
Error: accumulating resources: recursed merging from path '../../components/recaptcha': may not add resource with an already registered id: apps_v1_Deployment|~X|example

The reason is that Kustomize processes overlays in parallel, so if two overlays point to the same base, it will create two copies of it but will not be able to merge them.

Add a WIP example that showcases how components can be used.

monopole · 2020-05-12T22:50:41Z

/ok-to-test

Describe in more detail the solution with stock kustomize variants to better showcase the benefits of kustomize components. Plus, revamp certain parts of the example with minor fixes.

Rename all `KustomizationPatch` instances to `v1alpha1/KustomizationPatch`, to reflect that it's an alpha feature.

ioandr · 2020-05-13T14:20:17Z

Hey there,

this is an update with regard to the action item from the previous community meeting: as agreed, with @apyrgio we have worked on a second iteration of the example document that describes Κustomize components.

We now provide 2 solutions to the same problem, one without and one with Kustomize components. We hope that they adequately highlight the value of the composition feature.

cc @pwittrock @monopole

pwittrock · 2020-05-13T19:58:43Z

@ioandr Thanks!

pwittrock · 2020-05-13T20:17:59Z

examples/components.md

+mkdir -p $RECAPTCHA
+
+cat <<EOF >$RECAPTCHA/kustomization.yaml
+kind: v1alpha1/KustomizationPatch


apiVersion: kustomize.config.k8s.io/v1alpha1 kind: Component

pwittrock · 2020-05-13T20:19:06Z

examples/components.md

+
+cat <<EOF >$COMMUNITY/kustomization.yaml
+kind: Kustomization
+resources:


Probably makes sense to have components separate from resources.

components: - ../../components/external_db - ../../components/recaptcha

Our approach was to stay as close as possible to the existing interface of kustomization files.

Since the bases keyword was also absorbed by resources quite recently we believe it is more straight forward to have the resources keyword cover all cases, including components.
Aside, such a change might have implications in the size and complexity of #2168.

In any case, we are definitely open to iterate on this and reach a common understanding.

A Component (aka KustomizationPatch) effectively become the parent resource of the resources that come before it in the resources list. They could be moved to a separate components list, which I think could then be evaluated as if it was appended to the end of the resources list. I guess this would make things a little clearer (and maybe opportunities for checking that only the correct types are in each list), and probably wouldn't make the code that much more complicated. The disadvantage is that you wouldn't be able to add any resources to be evaluated after the components, but I can't think of any examples that would actually need that (and the examples don't), so maybe that isn't a real concern?

I implemented support for a components list on a separate branch in my fork - doesn't seem to bad, with most of the added complexity needed to limit each kind to their own list. The code definitely could do with a slightly more extensive refactor if components is preferred, but I didn't do that now just to minimise the extent of the changes.

I think this is reasonable for now. It has an alpha apiVersion so we aren't commitment to keeping the API in this form.

Should I merge that in to the main PR branch? I still need to make the change so make it use alpha instead of beta.

I rebased the components branch and added v1alpha1 support.

pwittrock · 2020-05-13T20:21:17Z

examples/components.md

+LDAP=$DEMO_HOME/components/ldap
+mkdir -p $LDAP
+
+cat <<EOF >$LDAP/kustomization.yaml


apiVersion: kustomize.config.k8s.io/v1alpha1 kind: Component

pwittrock · 2020-05-13T20:21:26Z

examples/components.md

+mkdir -p $EXT_DB
+
+cat <<EOF >$EXT_DB/kustomization.yaml
+kind: v1alpha1/KustomizationPatch  # <-- Component notation


apiVersion: kustomize.config.k8s.io/v1alpha1 kind: Component

pwittrock · 2020-05-13T20:22:11Z

examples/components.md

+mkdir $BASE
+
+cat <<EOF >$BASE/kustomization.yaml
+resources:


apiVersion: kustomize.config.k8s.io/v1alpha1 kind: Customization

Done, s/Customization/Kustomization

Yup, silly auto correct.

pwittrock · 2020-05-13T20:25:19Z

examples/components.md

+EOF
+```
+
+Define a `community` overlay, that bundles the external DB and reCAPTCHA


overlay -> variant

pgpx · 2020-05-14T16:09:16Z

examples/components.md

+  files:
+    - ldappass.txt
+
+patchesStrategicMerge:


Maybe use a configMapGenerator in one of the components to show that it would work as well? Something like the following (or use a files: generator):

Suggested change

patchesStrategicMerge:

configMapGenerator:

- name: conf

behavior: merge

literals:

- ldap.conf=|

endpoint=ldap://ldap.example.com

bindDN=cn=admin,dc=example,dc=com

pass=/var/run/secrets/ldap/ldappass.txt

Sure, let's spice this up.

pwittrock · 2020-05-14T18:27:48Z

Would you clarify in the doc if the ordering matters? I assume it doesn't, but would be good to clarify that resources behaves like a set.

pwittrock · 2020-05-14T18:39:11Z

Let's move forward with this as there have been no objections.

WDYT of the following proposal:

Create a KEP for this under kubernetes/enhancements. This gives visibility to folks who follow that repo
- You don't need to fill out all the KEP sections. Delete those that don't seem relevant.
- Copy and paste contents from this document into the KEP
- Link the KEP in this PR description
- Message sig-cli in Slack, mentioning @pwittrock and @soltysh
- Post KEP to sig-cli mailing list
- Add to next weeks sig-cli agenda
Update the implementation PR (don't block on the KEP, since this has already had extensive discussion)
- Link the implementation PR (code) in the description of this PR (docs)
- Add any tests that are missing
  - Test that ordering doesn't matter of resources, patches, components
  - Test for conflicting components -- e.g. specify different values for the same annotation, patch the same resource with different values
  - Test ordering with namePrefix / suffix
  - Test components specifying the same ConfigMap / Secret generator -- are they merged, overridden, error?
- Update with changes from this PR
Show up to sig-cli meetings until the feature is completed and part of a release

After the above are complete, let's merge this PR so the new capabilities are documented.

Consider promoting your new feature on twitter.

pwittrock · 2020-05-14T18:39:27Z

/lgtm

pwittrock · 2020-05-14T18:39:33Z

/approve

k8s-ci-robot · 2020-05-14T18:39:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: apyrgio, pwittrock

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:

~~OWNERS~~ [pwittrock]

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

pgpx · 2020-05-14T20:19:07Z

@pwittrock The ordering of Components does matter, because they are applied to the resources and components that came before them in the list (resources or components). Other resources (Kustomizations and files) are independent of each other. I guess that might be another argument to use components because then we can say that the order is significant for that list, but resources can then behave as a set (otherwise if we just have resources then order will matter).

pwittrock · 2020-05-14T20:30:09Z

Hm, my guess is that we want components to apply to all resources in the list -- otherwise I'd expect subtle bugs. Its not clear from looking at the resource list what is a resources vs a path vs a component.

Otherwise we might want a more explicit expression of what is applied and where.

Perhaps components should have a protocol as part of the URI so it is more clear.

kind: Kustomization
resources:
- https+component://github.com/....
- file+component://something

apyrgio · 2020-05-15T13:01:51Z

@pwittrock Indeed, it makes sense to spin this feature request into a separate KEP. Thanks for breaking down the process to steps, they seem pretty solid. We’ll follow them and try to have something ready by this Wednesday.

Incorporate the suggestion of the separate "components" field, that will hold references to resources of `Component` kind. Also, add a missing reference to the base, in the Kustomization files of the variants.

monopole · 2020-05-15T15:58:33Z

/hold
Looks good, no big objections, but would like clarity on resources:

Currently, it's a list, not a set, and don't want to change that accidentally.

apyrgio · 2020-05-15T17:44:05Z

It seems that the semantics of the components field are as important as how the Component kind will work, so let's weigh in on this:

I think we agree that components are a special case of overlays, that can modify the same set of resources without "cloning" them. In this example, the resources are the Deployment and ConfigMap, and the components extend those, without caring much for their order. However, if two components alter the same field, then we need a way to resolve this conflict. It seems that the most intuitive way to resolve it is by "applying" the components sequentially, which is what @pgpx's PR does.

Now, as for how we can communicate this to the users. I think that the proposal for a separate components field works well in this case, because we can put the components there and document that order matters. I also think that users can adapt to this field easily, because there's already a field that behaves similarly; the patches field. Thus, the elevator pitch to existing users could be, to some extent, "what patches do to resources, components do to kustomizations". Hopefully that clarifies things a bit.

Slightly alter the reCAPTCHA component to use a `configMapGenerator`, instead of patching the base `ConfigMap` as in the rest of the components, to show that this is supported as well.

apyrgio · 2020-05-15T18:03:14Z

I have updated the example to demonstrate how the components field would work, and to correct some omissions.

monopole · 2020-05-15T23:20:53Z

/lgtm

looks like fun, thanks for working on this.
The examples are terrific.

monopole

/lgtm

make ux happy

apyrgio · 2020-05-19T04:28:39Z

Just a quick note here; I'm a bit worried that there will be people who will go through the examples of Kustomize, see this example and try to use it. However, the corresponding functionality has not yet been merged. Should we warn in the example that this is a work in progress?

This enhancement proposal introduces components, a new kind of Kustomization that allows users to define reusable kustomizations. Main discussion: kubernetes-sigs/kustomize#1251 User story that spawned this KEP: kubernetes-sigs/kustomize#2438

apyrgio · 2020-05-20T12:26:05Z

Me, @ioandr and @pgpx have written the requested KEP. See kubernetes/enhancements#1803 for more details.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 5, 2020

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 5, 2020

k8s-ci-robot requested review from mengqiy and monopole May 5, 2020 13:13

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 5, 2020

apyrgio changed the title ~~Add an example with components~~ WIP: Add an example with components May 5, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 5, 2020

apyrgio mentioned this pull request May 5, 2020

Support composition in kustomize #1251

Closed

apyrgio force-pushed the feature-components-docs branch from 96034d1 to b5c3b87 Compare May 6, 2020 07:33

Add an example with components

6063a6b

Add a WIP example that showcases how components can be used.

apyrgio force-pushed the feature-components-docs branch from b5c3b87 to 6063a6b Compare May 6, 2020 07:44

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 12, 2020

Extend example with components

0152dbb

Describe in more detail the solution with stock kustomize variants to better showcase the benefits of kustomize components. Plus, revamp certain parts of the example with minor fixes.

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 13, 2020

Set API version of KustomizationPatch to v1alpha1

5884290

Rename all `KustomizationPatch` instances to `v1alpha1/KustomizationPatch`, to reflect that it's an alpha feature.

apyrgio mentioned this pull request May 13, 2020

Implement "kind: Component" to support composition #2168

Merged

pwittrock reviewed May 13, 2020

View reviewed changes

pgpx reviewed May 14, 2020

View reviewed changes

k8s-ci-robot assigned pwittrock May 14, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 14, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 14, 2020

apyrgio added 2 commits May 15, 2020 15:01

Add some missing overlay fields

8d8773f

Incorporate the "components" suggestion

844824e

Incorporate the suggestion of the separate "components" field, that will hold references to resources of `Component` kind. Also, add a missing reference to the base, in the Kustomization files of the variants.

Add a configMapGenerator to the example

e1087cd

Slightly alter the reCAPTCHA component to use a `configMapGenerator`, instead of patching the base `ConfigMap` as in the rest of the components, to show that this is supported as well.

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2020

k8s-ci-robot assigned monopole May 15, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 15, 2020

monopole approved these changes May 15, 2020

View reviewed changes

monopole merged commit 765a488 into kubernetes-sigs:master May 18, 2020

apyrgio mentioned this pull request May 19, 2020

Add KEP for Kustomize components arrikto/kubernetes-enhancements#1

Merged

apyrgio mentioned this pull request May 20, 2020

Add Kustomize components KEP kubernetes/enhancements#1803

Merged

-patchesStrategicMerge:
+configMapGenerator:
+  - name: conf
+    behavior: merge
+    literals:
+      - ldap.conf=|
+          endpoint=ldap://ldap.example.com
+          bindDN=cn=admin,dc=example,dc=com
+          pass=/var/run/secrets/ldap/ldappass.txt

Kustomize API Proposal: Add an example with components #2438

Kustomize API Proposal: Add an example with components #2438

Conversation

apyrgio commented May 5, 2020 • edited Loading

k8s-ci-robot commented May 5, 2020

apyrgio commented May 5, 2020

apyrgio commented May 5, 2020

monopole commented May 12, 2020

ioandr commented May 13, 2020 • edited Loading

pwittrock commented May 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ioandr May 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented May 14, 2020

pwittrock commented May 14, 2020 • edited Loading

pwittrock commented May 14, 2020

pwittrock commented May 14, 2020

k8s-ci-robot commented May 14, 2020

pgpx commented May 14, 2020

pwittrock commented May 14, 2020

apyrgio commented May 15, 2020 • edited Loading

monopole commented May 15, 2020

apyrgio commented May 15, 2020

apyrgio commented May 15, 2020 • edited Loading

monopole commented May 15, 2020

monopole left a comment

Choose a reason for hiding this comment

apyrgio commented May 19, 2020

apyrgio commented May 20, 2020

apyrgio commented May 5, 2020 •

edited

Loading

ioandr commented May 13, 2020 •

edited

Loading

ioandr May 14, 2020 •

edited

Loading

pwittrock commented May 14, 2020 •

edited

Loading

apyrgio commented May 15, 2020 •

edited

Loading

apyrgio commented May 15, 2020 •

edited

Loading