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

KEP 2299: Kustomize Plugin Composition API #2300

Merged
merged 4 commits into from
Apr 27, 2021

Conversation

KnVerey
Copy link
Contributor

@KnVerey KnVerey commented Jan 20, 2021

This KEP proposes a new Kustomize API (kind) that is oriented around Kustomize plugins, making them a first-class expression of a resource configuration bundle. This new API provides new sophisticated capabilities for using plugins as composable units and includes support for automatic discovery and installation of plugins. It showcases plugins as a way to implement composable, declarative client-side abstractions.

Enhancement tracking issue: #2299

cc @pwittrock @campoy

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 20, 2021
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 20, 2021
@KnVerey KnVerey mentioned this pull request Jan 20, 2021
4 tasks
@KnVerey KnVerey force-pushed the kustomize_composition_kep branch from 13b7a1a to 5aa9659 Compare January 20, 2021 23:11
@pwittrock
Copy link
Member

Thanks @KnVerey.

FYI @monopole

@dougsland
Copy link
Member

/cc

Copy link
Member

@dougsland dougsland left a comment

Choose a reason for hiding this comment

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

just reviewed the doc, looks good to me, specially having the allow-list for the plugins implemented as mentioned.
Leaving to @pwittrock @monopole

Copy link
Contributor

@monopole monopole left a comment

Choose a reason for hiding this comment

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

There's lots of good ideas to discuss here.

This KEP calls out some rough problems and awkward API around plugins - there's much there to improve.

The KEP also makes these assumptions: Transformers cannot generate, transformer configs themselves cannot be transformed, transformer configs cannot be inlined.

On these assumptions, the KEP proposes an entirely new (and nice!) API. The implementation of this API would be a new code path, fired up from the same main.go, branching off in the build command, based on what file (object) was detected - Kustomization or Composition. That's not so much an enhancement as it is a rewrite.

Given that, why do it in kustomize? To proceed in kustomize, would like to compare user journeys between what one can do now with a

  • Kustomization, transformers and overlay structure (or some relatively minor evolution of same) vs
  • a Composition and modules, and the modulesFrom/Overrides fields.

The goal would be to decide if an evolution (of Kustomization and transformers) is out of the question, and thus a rewrite and user re-education (to Compositions and modules) is the path forward.

Can this KEP add those comparisons? That would answer the objections I raise to the assumption in the motivation section.

operations applied to Kustomize bases. This API is suboptimal for workflows that
are primarily composed of Kustomize plugins. Challenges with the current approach include:

1. Packaging, distribution and installation of plugins is immature and non-declarative.
Copy link
Contributor

@monopole monopole Feb 4, 2021

Choose a reason for hiding this comment

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

Absolutely true (for Go and exec plugins, less true for container-based plugins) and in painful need of improvement.

In particular, Go plugins based on shared libs (.so files) do not work as a distribution mechanism and the docs are as clear on this as they can be:
https://kubectl.docs.kubernetes.io/guides/extending_kustomize/goplugincaveats/

They remain nice as a Go authoring (not distribution) framework because if things are set up correctly, one can step into a plugin in a debugger in the context of a full kustomization. Alas, that's not documented.
We only have this:https://github.com/kubernetes-sigs/kustomize/tree/cbb121e651c06097924979e3349c59da834bdeac/plugin#testing

Obviously step-debugging not even a possibility for a node-js plugin author, etc. so it's not hugely important.

Plugins need a developer and testing framework. There's a boatload of e2e tests in the api/krusty directory. They represent dozens (hundreds?) of questions answered and issues closed. New things need something at least that good.
A rewrite has to reinvent all of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're still working out the specifics of the design for the distribution mechanism, but the idea is that Kustomize (as a flag or a Composition field) can reference a trusted plugin "catalog" that provides content-addressable references to plugins of various sorts, using the OCI Artifacts model. In theory the catalog could list alternative implementations and Kustomize could choose the appropriate one based on the current execution context. cc @jeremyrickard

Plugins need a developer and testing framework.

100%. We've been contributing to the kyaml functions framework and accompanying test utils as a first step in this, and I expect that documentation for this feature will need to include a detailed module developer guide.

A rewrite has to reinvent all of that.

I really want to rewrite as little as possible. The Composition execution pipeline is fully compliant with the Configuration Functions Specification, and we're not changing any of the existing transformers at all. Our prototype is built from existing kyaml code and behaves (once resolved into a single layer) just like the transformers field.

2. Orchestration, ordering and dependencies is overly complex due to its integration
with the orchestration of built-in operations. For example, `Kustomization` requires
generators and transformers to be specified and executed separately, whereas a given
plugin may do both (or neither, as in a validator plugin).
Copy link
Contributor

Choose a reason for hiding this comment

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

The builtins have a specific order that hasn't, per issues being filed, been a problem.
User specified plugins run afterwards in the order they are specified in the transformers field in the kustomization file.

One is required to put a plugin into a list (generators, transformers, or validators) but that's to communicate intent, not to act as a straight jacket. It's hardly overly complex.

We've established that a transformer can be a generator :

https://github.com/kubernetes-sigs/kustomize/blob/master/plugin/someteam.example.com/v1/calvinduplicator/CalvinDuplicator.go

Likewise a transformer can be a validator. Everything is a transformer under the hood:

https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/#everything-is-a-transformer

Can we reword this bullet to acknowledge these things and still object to them? Or drop the bullet?

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, @KnVerey , thanks for fixing kubernetes-sigs/kustomize#3448.

The coverage we have for "advanced" plugin behavior is expressed in terms of Go plugins, not exec, and certainly not starlark, so we have blind spots - one less now, thanks!

Digging in to fix an issue, and a tricky one, is really really appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, "complex" was not a good choice of words on my part. Indeed, you can put everything in the transformers field, forget the other plugin fields exist, and add an additional Kustomization layer if you need built-ins to affect the results of transformers. The point is that if we optimize for abstraction-based plugins (as opposed to plugins that make granular transformations that are well known to the user), the distinction between these fields is more confusing than helpful. I.e. the end user may not know whether the plugin validates, generates or transformers, and indeed it may be more than one, may depend on input, or may change as the plugin matures. It's also likely that the extra layer to invoke the built-ins properly will be necessary a lot of the time, which is not ideal. I'll reword the bullet.

plugin may do both (or neither, as in a validator plugin).
3. Plugin execution happens during the evaluation of the `Kustomization` where it is
specified. Overlays cannot modify plugin values before they are evaluated,
which seriously hinders plugin usability for developing composable abstractions.
Copy link
Contributor

@monopole monopole Feb 4, 2021

Choose a reason for hiding this comment

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

If I'm reading this statement correctly I don't think it's true.

Again - poorly documented, which I'm trying to fix.

See
https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/#transformed-transformers

FWIW, this ability has been available for over a year, and i've not seen interest signals around it like questions or issues being filed. Perhaps it's too awkward, perhaps its undiscovered, or perhaps it doesn't fill an actual need people have.

Copy link
Contributor

Choose a reason for hiding this comment

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

And maybe it doesn't work - but if so, it's a bug, not an API impossibility! ;-P

Choose a reason for hiding this comment

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

it works. I'm using that approach a lot. E.g. it's possible to patch transformer/generator config this way

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, this does work and I just didn't know about it! However, the isolation of transformers from other configuration until they're expanded into resources makes the result inelegant in a few ways. Imagine a scenario where you have:

  1. A base layer that should include some resources, a transformer config, and a common label.
  2. A production layer that wants to change a field in the transformer config and add its own label.

With Composition, this can in fact be expressed in two layers. With Kustomization, it takes five, and some of the changes can't go where you'd expect. At the bottom, you have separate Kustomizations for transformer and resource config. Next, you immediately need to patch the transformer config with the production value, before bringing it together with the resource base config (there's no complete environment-agnostic Kustomization). Then, you bring the base resource and production transformer config together, turning it all into resources. Finally, you have to apply both your "base" and your "production" labels at the top level, or else the resources from the transformer won't get base labels.

image

I'll reorient the bullet around this challenge.

which seriously hinders plugin usability for developing composable abstractions.
4. The `Kustomization.yaml` format does not elegantly allow plugins to be
specified inline. Instead, they are defined in separate files, which obfuscates
the holistic user intent in workflows primarily driven by plugins.
Copy link
Contributor

@monopole monopole Feb 5, 2021

Choose a reason for hiding this comment

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

It might not be elegant, but it's allowed, see text here:

https://kubectl.docs.kubernetes.io/references/kustomize/kustomization/#everything-is-a-transformer
(same link as above)

Here's a code example:

https://github.com/kubernetes-sigs/kustomize/blob/master/api/krusty/inlinetransformer_test.go

It's possible to define an abbreviated form of inline that doesn't demand full KRM (apiVersion, kind,metadata, etc.) in the transformer's config, if that takes us in the right direction on the elegance scale.

That's an easy evolution of existing API, not a rewrite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. FWIW this seems to be broken for function-based plugins--I get a file not found error (seems like it is looking for an exec plugin) when I try to specify config inline. But in any case I'll remove this bullet.

1. The orchestration model used to evaluate the API must be simplified in a way
that is optimized for plugins. Notably, it must be possible for lists of plugins to be
recursively composed, and for overlay instances of the API to modify the
configuration of plugins they import.
Copy link
Contributor

@monopole monopole Feb 5, 2021

Choose a reason for hiding this comment

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

Per points made above I'm going to say this exists but isn't popular.

Maybe because its not elegant enough or discoverable. One must place transformer configs as resources in a base, and modify them in an overlay, then use them as transformers in yet another overlay.

Three levels or concepts are needed no matter how you slice it. So you do an overlay, or you introduce a modulesFrom and modulesOverride fields.

Placing transformer configs as resources in a base has the nice side effect of making them generally reusable, even if you don't choose to mutate them.

Choose a reason for hiding this comment

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

Even though it's possible, it's hard to do data deduplication (copy the same values to different yamls) without replacement plugin or something similar. With replacement plugin it was possible to make this approach for re-usable modules in the project that uses kustomize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed in the other thread, I agree that it is possible. For the purposes of this section, I think the point is that a successful implementation must have a very obvious increase in simplicity for these workflows. I'll update the wording.

Although direct integration with the existing `Kustomization` API could be done,
it is outside the scope of this proposal and carries risks that must be considered.
For example, given that `Kustomization` is integrated with existing workflows such as `kubectl apply -k`,
the introduction of automatic plugin installation and execution may be undesirable.
Copy link
Contributor

Choose a reason for hiding this comment

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

Flags to enable plugins exist, as well as to grant network access to containers.

Certainly UX and good defaults are crucial here.

The general goal of kustomize is to merge into kubectl as the primary means of feature delivery.
IMO we maintain kustomize as a standalone app for faster delivery of beta kustomization library features, a need that would go away if kubectl released more frequently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll update this to instead propose that the transformers in Composition should be gated the same way as those in Kustomization (which happens to nerf Composition in kubectl right now, since alpha plugins can't be enabled there)


This risk will be mitigated in the following ways:

1. Container-based functions will be run without network or volume access. This does
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying that we should keep this configurable with runtime flags instead of disabling it in the new kind? I was thinking that in addition to the security argument, disabling network and volume access avoids some common ways to cause functions to be poorly encapsulated (e.g. rely on something outside the well specified input) and/or have non-deterministic results. Can you provide context on the original motivation for making these capabilities available?

a pluggable trust model that enables users and organizations to allowlist plugin
sources. It should be possible for large organizations to distribute binaries with
specific allowlists compiled in, and for additional sources to be allowlisted at
invocation time.
Copy link
Contributor

Choose a reason for hiding this comment

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

So this is all great and not-inconsistent with the existing goals of kustomize plugins - there are already flags for much of this.

Can this KEP clearly state whether it seeks to improve the existing plugin loader (lift all the boats) or build a new one (twice the maintenance cost)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both for user experience and for maintainability, I agree it would be best if plugin configuration ultimately behaved in the same way in all contexts, e.g. by sharing a loader. I think the alternative is to use Composition as a proving ground for changes we want to make, before ultimately consolidating the behavior. How much latitude do we have to change existing plugin configuration overall, particularly in ways that add restriction? E.g. could we replace the env-based exec plugin lookup entirely with the explicit trusted catalog model described here? Could we add configuration requirements to network and volume access or remove them from functions?

a Kustomize plugin.
- Import modules from another Composition and add them to the list.
- Override an imported module's fields with new values.
- Reorder the list of modules prior to execution.
Copy link
Contributor

Choose a reason for hiding this comment

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

Modules are so close to transformers - we need to be clear on the difference to decide which is better

  • evolve/expand transformers - keeping one concept
  • make a new thing that's slightly different and have to support and explain it too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are indeed very close--as you pointed out elsewhere, everything is a transformer. It's kinda hard not to be, given that transformers in practice don't even need to transform per se--they are essentially anything that takes in and emits resource yaml, doing whatever it pleases in the middle! The reason for the new name is to convey the intent of transformers when used in this new context: they may generate, transform, validate, delete or whatever, but that operation is not important. What's important is that each one is providing a self-contained "module" of functionality that is configured via desired state, like how custom resources + operators work on the server side.

one of the primary proposed features--enabling module configuration to be overridden--requires
a completely different evaluation model. Namely, a topological sort is applied to the
entire tree of modules prior to evaluation, as opposed to Kustomization's current
model of executing generators, transformers and plugins in each level in isolation.
Copy link
Contributor

Choose a reason for hiding this comment

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

As noted above, i don't think this is a serious problem or blocker to the concept.

@monopole
Copy link
Contributor

/lgtm

This is a great step forward.

We can merge this and start working on the tech and the product plan.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2021
@KnVerey
Copy link
Contributor Author

KnVerey commented Apr 27, 2021

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Apr 27, 2021
@pwittrock
Copy link
Member

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KnVerey, 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:

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 Apr 27, 2021
@k8s-ci-robot k8s-ci-robot merged commit a20d3a6 into kubernetes:master Apr 27, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.22 milestone Apr 27, 2021
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. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants