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-2906: Adding initial KEP artifacts #2908

Merged
merged 4 commits into from
Oct 12, 2021

Conversation

jeremyrickard
Copy link
Contributor

Signed-off-by: Jeremy jeremyrrickard@gmail.com

  • One-line PR description: Add an initial provisional KEP defining a KRM plugin (transformer) catalog capability for discovery/installation experience improvement
  • Other comments:

/sig cli

/cc @KnVerey

@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 27, 2021
Signed-off-by: Jeremy <jeremyrrickard@gmail.com>
# app/kustomization.yaml
kind: Kustomization
catalogs:
- https://example.com/plugins/catalog.yaml
Copy link

Choose a reason for hiding this comment

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

my concern here is the network dependency. I think we have two flows: pipeline design and pipeline execution. I would be reluctant to add a network call for something that can be in my CI/CD pipeline. I'd suggest splitting up discovery of the plugins from their use.

If pointing to a plugin definition is sufficient to deduce execution options, we might also just want to simplify the syntax for usage in general (provide reasonable defaults).

Copy link
Contributor

Choose a reason for hiding this comment

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

One potential option is to extend the kustomize localize command proposal (kubernetes-sigs/kustomize#3980) to also localize catalogs, so that subsequent calls to kustomize build do not have to refetch the catalog file. The proposal there is to capture remote data for use cases that preclude network use, so I wonder if it could also resolve the concern here.

Copy link

Choose a reason for hiding this comment

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

it is an option, I fear that that functionality has a larger discussion but could be used to accomplish all of the remote resource pulling.

Copy link

Choose a reason for hiding this comment

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

@mikebz raises a good point. basically, how self-contained is this config now ?
Looking at this http url, another concern I have is about immutability of a config package. Given the resource on this http url can change, that can affect the resultant config. immutability is highly desirable property in the config workflows.

May be this is covered later in the doc but one way to address this will be one can specify the catalogs on the filesystem like we list resources in a kustomization to make it self contained. One can achieve immutability by referring a versioned OCI artifact.

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 think that it would be desirable to support both local and remote references, although we could further restrict this to only remote references if absolutely required. I've updated the wording to specify that both are allowed, happy to iterate further on the text.

I do like the kustomize localize suggestion, I think that would be an interesting way to handle it.

I think that a versioned OCI artifact would be a good (perhaps ideal) way to reference these in an immutable way, but it would probably be best to allow some flexibility for end users. We could restrict this to Local, Git, or OCI based references?

keps/sig-cli/2906-kustomize-function-catalog/README.md Outdated Show resolved Hide resolved

When this Kustomization is processed by `kustomize build`, the referenced catalog (or catalogs) will be used to locate a plugin provider that supports the apiVersion `example.com/v1` and kind `JavaApplication`. If found in one of the referenced catalogs, kustomize can determine the provider configuration without the need for the user to specify it in the kustomization resources directly. The catalogs will be searched in order specified. If more than one catalog defines the target apiVersion and kind, the first will be selected.

In addition to the new catalog kind, `kustomize build` will accept a repeatable flag `--trusted-plugin-catalog=""`. When present, this flag instructs `kustomize build` to automatically fetch and execute plugins that are defined by the catalog and referenced within the Kustomization, Component or Composition. When a resource is processed by `kustomize build` and a catalog is referenced but not specified using the `--trusted-plugin-catalog=""` flag, an error will occur. Kustomize will provide a built in Catalog for supporting official extensions, published to a well publicized endpoint. This catalog will _NOT_ require the user to explicitly trust it. Users can provide the `apiVersion` and `kind` of the official extensions in kustomize resource and these will be resolved by the official catalog.
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor nit, I find --trusted-plugin-catalog="" a bit verbose. Is just --catalog="" not sufficient?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also -

Kustomize will provide a built in Catalog for supporting official extensions, published to a well publicized endpoint.

Will this builtin catalog have docker images/starlark functions? Though I don't oppose it, I think we should be careful about introducing a docker/starlark requirement for users without them being explicitly aware of it. In particular, we should make sure the documentation is clear about what builtin functionality will depend on docker.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion: We should consider adding a command like kustomize view catalog that shows the catalogs trusted vs. required for the current configuration object, along with publisher information retrieved from the catalog itself.

Minor nit, I find --trusted-plugin-catalog="" a bit verbose. Is just --catalog="" not sufficient?

That verbosity is coming from my input. The idea is to make sure the user is aware that using this flag is a declaration of trust in the plugins the referenced file includes. I feel like --catalog= could be interpreted as "I need this" rather than "I trust this". I could be convinced that "plugin" is unnecessary though. The whole purpose of the flag as opposed to the inline field is for the user to prove to us that they know what they're doing, the same way they do by installing the plugin to a particular location themselves in the legacy flow.

Will this builtin catalog have docker images/starlark functions?

Totally up to us, and I think that decision doesn't need to be in scope of the KEP. FWIW I think we would want to use docker containers (e.g. your new helm transformer implementation), but not starlark.


*KRM*: [Kubernetes Resource Model](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/architecture/resource-management.md)

*Plugin*: User-authored generator, transformer or validator (refers to both the "provider" program that implements it and the "config" YAML required to use it).

Choose a reason for hiding this comment

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

I suggest rationalizing the concepts with the KRM Functions Spec:

https://github.com/kubernetes-sigs/kustomize/blob/master/cmd/config/docs/api-conventions/functions-spec.md#krm-functions-specification

Is Plugin a Function, implementing the wire format described in that doc?

Copy link

Choose a reason for hiding this comment

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

+1 , as I was reading this, I was using modules, plugins and functions interchangeably in my head.

Copy link
Member

Choose a reason for hiding this comment

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

+1

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 pushed a commit that updates all the wording to center around functions, thanks for the suggestion, I think it's much better to follow the spec wording.

path: /etc/secrets
```

Unlike the previous execution of Kustomize, Kustomize will not use the catalog to resolve the `SecretSidecar`, as the provider configuration was specified.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also provide an example of how to override the provider configuration with kustomization? Is there an easy way to do this?

Copy link

Choose a reason for hiding this comment

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

+1

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'll update this to use a kustomization example instead, I think that makes sense especially since Composition isn't implemented yet.

exec:
- os: darwin
arch: arm64
path: https://example.com/java
Copy link

Choose a reason for hiding this comment

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

In our offline discussion, there was a consensus that for the use case of packaging and sharing programs in a public catalog, and executing such untrusted third-party programs, we want containerization which solves {packaging, pulling, executing hermetically, explicitly enabling privileges, scanning for vulnerabilities, OCI metadata, ...}

Executing non-containerized executables may make sense when dealing with first-party/trusted programs inside an organization, but that's a different use case than the effort to create a thriving ecosystem and a public catalog. Another way to state this: If you want to create the mechanism for sharing programs like this publicly, you'll end up re-inventing containers ;)

Copy link

Choose a reason for hiding this comment

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

I agree that pulling down a binary based on a 3rd party registry is a risk. Potentially another reason to figure out whether using catalog metadata for design time and runtime is different.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to support exec in catalog?

Another option to consider is disallowing exec in catalog, but continuing to allow users to specify exec functions in their kustomizations and Compositions (with the same syntax as the override described in User Story 3).

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel strongly that exec should be supported by Catalog. The Plugin Graduation KEP proposes using Catalog as the one trust model for plugins, so if it did not support exec, exec would not work. Given that Kustomization authors and the user running build can be different people, and that it's the latter who needs to authorize the plugin running, I think it is better security-wise to have exec enabled through Catalog (which is provided by the person running build) than embedded in the Kustomization (which they may not have written). If they were to stay embedded in the Kustomization, we'd in practice need a separate flow for authorizing them. Either it would be a generic authorization for all exec like we have now (unacceptably dangerous, IMO) or they'd effectively end up having to write out plugin identifiers in a CLI flag, which would be similar to what we're doing with Catalog, but much harder for the user to do.

For the record, some exec use cases from out of band discussions among this group included using Kustomize from contexts (e.g. CI providers) that cannot run docker containers (e.g. because they themselves are containerized), as well as small, bespoke plugins embedded with a Kustomization, where an extremely low barrier to entry is highly desirable and distribution is not relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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


Once the explicit container reference is provided, Kustomize is able to download and run this image as part of the Kustomize build step, by invoking the user installed Docker client and leveraging local images or OCI registries. In addition to container based plugins, ad-hoc functions can also currently be written using the [Starlark programming language](https://github.com/bazelbuild/starlark) and other non-container based mechanisms. Unlike container based plugins, these plugins do not currently have an associated registry concept and they must be stored locally. Discovery and installation of these providers are both currently left to the users.

In addition to these user defined plugins, this KEP is also partially motivated by a need to change how Kustomize provides officially supported functionality. Currently, to support a given piece of functionality officially a built-in function must be created (and typically also added to the Kustomization API). Some of these features would be better implemented as extensions for security reasons or to limit the dependency graph of Kustomize and the integration with kubectl. No mechanism currently exists, however, to support official distribution of these capabilities, so they are instead built into Kustomize.
Copy link

Choose a reason for hiding this comment

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

one of the other reasons is that one might want to mix and match the kustomize binary version with the plugin version. This frees up the core kustomize team to introduce breaking changes that can clean up legacy design decisions without having to rev absolutely everything.

kind: GroovyApplication
description: "A Kustomize plugin provider that can handle groovy apps"
provider:
starlark: https://example.co/module_providers/starlark-func:v1.0.0
Copy link

Choose a reason for hiding this comment

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

I am a fan of not providing built in execution for any particular stack. The starlark function exists in the kpt catalog: https://catalog.kpt.dev/starlark/v0.2/ . Execution environments can carry an additional attack and supportability burden.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Copy link
Contributor

Choose a reason for hiding this comment

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

Since the over-arching KEP is proposing deprecation of starlark, I think it would be reasonable to update this one to not use Starlark examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, that makes sense. Do you feel like any of the examples should involve exec style or should the focus be containerized?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should include at least some exec examples

Copy link

@droot droot left a comment

Choose a reason for hiding this comment

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

Did a quick pass and left some early comments.

metadata:
name: "example-co-plugins"
spec:
modules:
Copy link

Choose a reason for hiding this comment

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

I found the term modules to be unintuitive in this case. Shouldn't this be called providers ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the wording to reflect this, thanks for the suggestion.

# app/kustomization.yaml
kind: Kustomization
catalogs:
- https://example.com/plugins/catalog.yaml
Copy link

Choose a reason for hiding this comment

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

@mikebz raises a good point. basically, how self-contained is this config now ?
Looking at this http url, another concern I have is about immutability of a config package. Given the resource on this http url can change, that can affect the resultant config. immutability is highly desirable property in the config workflows.

May be this is covered later in the doc but one way to address this will be one can specify the catalogs on the filesystem like we list resources in a kustomization to make it self contained. One can achieve immutability by referring a versioned OCI artifact.

This concept can be extended later to support additional plugin provider packaging, but is out of scope for the current proposal.

When HTTP(s) references are used, the HTTP(s) endpoint must support anonymous access for reading resources. Resources will be expected to be stored as a single file, such as `catalog.yaml`. Git or OCI references can be authenticated or anonymous, and will use appropriate configuration from the users file-system.

Copy link

Choose a reason for hiding this comment

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

Being able to download exec binaries on the fly from http/oci-artifact is making me nervous :)

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've added stronger wording regarding verification of binary based functions: the catalog must contain a sha256 and they would need to be verified prior to running. I think supporting them remains important, but we could make this a more out of band process (like the kustomize localize command up above) so that users need to run a command that would fetch any providers (Container, exec, whatever) prior to running kustomize build?

Copy link
Contributor

Choose a reason for hiding this comment

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

we could make this a more out of band process (like the kustomize localize command up above) so that users need to run a command that would fetch any providers (Container, exec, whatever) prior to running kustomize build?

Does adding an out-of-band command to run improve the security posture if it's still automated?

I'd like to better understand what in particular makes you nervous @droot. The end user is authorizing the particular exec function by explicitly pointing to the catalogue containing it, and we're verifying the sha256 for the binary in question against the catalog entry before running it. The latter is likely more than the average user bothers to do in the current install-it-yourself flows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like reasonable risk mitigation.

image: docker.example.co/plugins/secrets:v1.0.0
```

I then publish this catalog to https://example.co/kustomize/catalog.yaml, for use by Example Co Kustomize users.
Copy link

Choose a reason for hiding this comment

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

It will be good to use OCI registry and use versioned form of OCI URL in the default examples to capture the best practice.

I then run `kustomize build app/ --trusted-plugin-catalog=https://example.co/kustomize/catalog.yaml`.

When this command is run, Kustomize detects the use of `example.com/v1/JavaApplication` and `example.com/v1/SecretSidecar`. As these are not built in transformers and there was no explicit provider configuration specified, Kustomize will check for any referenced catalogs. It will see that I have specified `https://example.co/kustomize/catalog.yaml` and allowed it as a trusted plugin catalog. It will then fetch the catalog and attempt to resolve these two transformers. It will match the specified apiVersion and Kinds with entries in the catalog and utilize the referenced docker images for the provider configuration.

Copy link

Choose a reason for hiding this comment

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

I have been looking at the use-cases related to helm chart and found the workflow to be, that I want to render the chart only chart content changes, so I invoked it imperatively, but rest of the operations on the rendered manifest need to be invoked repeatedly (so prefer declarative workflow for that). But once I drop the RenderHelmChart resource in here, kustomize build will always try to render the chart. Is there a way I can exclude (I guess one can comment out the resource entry for the chart in that case).

Copy link
Contributor

Choose a reason for hiding this comment

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

Kustomize itself doesn't have any imperative workflows (other than to help author Kustomization fields, if that counts), so if the user wants to avoid the render, they'd have to do it out of band themselves and commit the resulting files to a directory Kustomize consumes instead of using the generator function declaratively. Note that Catalog doesn't change anything about this.

path: /etc/secrets
```

Unlike the previous execution of Kustomize, Kustomize will not use the catalog to resolve the `SecretSidecar`, as the provider configuration was specified.
Copy link

Choose a reason for hiding this comment

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

+1

container:
image: example/module_providers/java:v1.0.0
requireNetwork: true
requireFilesystem: true
Copy link

Choose a reason for hiding this comment

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

Do we cover how filesystem access will be done ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this different from network and mounts fields currently supported in the container function spec? https://github.com/kubernetes-sigs/kustomize/blob/f122fb12f3d0d234564b664c48fdb8d93ba8a7c6/kyaml/fn/runtime/runtimeutil/functiontypes.go#L149

Is requiring access to the entire filesystem necessary vs mounting the necessary files into the container?

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'll reconcile this with the functiontypes.go struct. I think it will need to deviate a little bit from the StorageMounts field.

I think there are two aspects to this:

1.) the catalog needs to express that yes, this function needs to access files
2.) these are the destinations that need to be populated

Within a kustomization (as far as I understand) it's going to mount within the kustomization root, not arbitrary files or the entire filesystem. So we could leverage the existing StorageMount type, but omit the src elements, as those will be determined by the user of the function + the kustomization, not the catalog.
So perhaps

provider: 
       container: 
         image: example/module_providers/java:v1.0.0
         network: true
         mounts: 
           - type: bind
             dst: /var/whatever

wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

Within a kustomization (as far as I understand) it's going to mount within the kustomization root, not arbitrary files or the entire filesystem.

That's what is intended but I actually don't think that's how it works today. I think we need to fix the code to ensure that the files mounted into the container are (a) within the kustomization root and (b) relative to the kustomization that refers to them - but this is an orthogonal issue that can be addressed separately.

So we could leverage the existing StorageMount type, but omit the src elements, as those will be determined by the user of the function + the kustomization, not the catalog.

That sounds reasonable to me. Do you have an idea for what the syntax should look like in a Kustomization or Composition to specify mounts.src? It's fine if this is out of scope for the KEP but I'm wondering if you had a plan for it.


*KRM*: [Kubernetes Resource Model](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/architecture/resource-management.md)

*Plugin*: User-authored generator, transformer or validator (refers to both the "provider" program that implements it and the "config" YAML required to use it).
Copy link

Choose a reason for hiding this comment

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

+1 , as I was reading this, I was using modules, plugins and functions interchangeably in my head.


Introduce a new API (`kind`) that will provide a mechanism to improve distribution and discovery of Kustomize plugins, for use with `Kustomization`, `Components`, and `Composition` resources.

This new API will provide a standardized way to define a collection of one or more Kustomize plugins, as well as supporting KRM-style configuration resources, that can be consumed by Kustomize in order to automate the use of plugins and eliminate manual out-of-band discovery and installation steps, regardless of the packaging format. All Kustomize configuration objects (i.e. Kustomization, Component and Composition) will support plugin source configuration via this new kind. Ideally, we would like the new API to become a standard that other KRM-style transformer orchestrators such as KPT can adopt as well.
Copy link
Member

Choose a reason for hiding this comment

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

A KRM function is the minimum unit that an orchestrator operates on. Why don't we also provide an abstraction for one single KRM function?

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'm not sure what you mean by this question. Could you provide some more clarification about what this looks like, beyond the KRM function specification? The catalog is really intended to be a collection of KRM functions, so I'm having trouble understanding the goal of the abstraction for the single KRM function?

Copy link
Member

@mengqiy mengqiy Sep 3, 2021

Choose a reason for hiding this comment

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

I imagine we can define a KRMFunction CRD which is orchestrator neutral and is the smallest building blocking for other CRDs.

It may look like the following. It contains a metadata of a function and its behavior (e.g. idempotent, network).

# krm-fn.yaml
apiVersion: config.k8s.io/v1alpha1
kind: KRMFunction
description: <1 or 2 sentences>
sourceURL: <URL>
exampleURLs:
  - example-url-1
  - example-url-2
type: container|exec (container is default if omit)
container:
  image: <image-name>
  version: <semver>
  functionConfig:
    - apiVersion: example.com/v1
      kind: JavaApplication
      schemaURL: <http/file/OCI url>
    - apiVersion: v1
      kind: ConfigMap
      schemaURL: <http/file/OCI url>
  idempotent: true|false
  requireNetwork: true|false
  requireVolumeMount: true|false
  maintainers:
    - foo@bar.com
  license: Apache 2.0
  tags:
    - mutator
exec:
  platforms:
    - bin: foo-amd64-linux
      os: linux
      arch: amd64
      uri: https://example.com/foo-amd64-linux.tar.gz
      sha256: <hash>
    - bin: foo-darwin-linux
      os: darwin
      arch: amd64
      uri: https://example.com/foo-darwin-linux.tar.gz
      sha256: <hash>

Then, we can potentially use a corev1.List to wrap multiple KRMFunction CRs. Or we can define a KRMFunctionList.

For example:

apiVersion: v1
kind: List
items:
  - apiVersion: config.k8s.io/v1alpha1
    kind: KRMFunction
    container:
      ...
  - apiVersion: config.k8s.io/v1alpha1
    kind: KRMFunction
    container:
      ...

The list is purely a collection of KRMFunction. It can be used to:

  • Build a KRM function catalog website
  • Consumed by a orchestrator or a IDE to enhance editing. (e.g. VSCode can suggest the available functions when a user editing a kustomize related file; auto-completion for kustomize fn run --image=foo<TAB><TAB>)

The KRMFunction CRD can also be used by the kustomize Catalog CRD. The spec.modules.provider field can be one of container, containerRef, exec or execRef.

apiVersion: kustomize.io/v1
kind: Catalog
metadata: 
  name: "example-co-plugins"
spec: 
  modules: 
  - apiVersion: example.com/v1
    kind: JavaApplication
    provider: 
      containerRef: krm-fn.yaml
  - apiVersion: example.com/v1
    kind: AnotherJavaApplication
    provider: 
      execRef: another-krm-fn.yaml

@jeremyrickard @KnVerey WDYT?


*KRM*: [Kubernetes Resource Model](https://github.com/kubernetes/community/blob/master/contributors/design-proposals/architecture/resource-management.md)

*Plugin*: User-authored generator, transformer or validator (refers to both the "provider" program that implements it and the "config" YAML required to use it).
Copy link
Member

Choose a reason for hiding this comment

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

+1


In addition to these user defined plugins, this KEP is also partially motivated by a need to change how Kustomize provides officially supported functionality. Currently, to support a given piece of functionality officially a built-in function must be created (and typically also added to the Kustomization API). Some of these features would be better implemented as extensions for security reasons or to limit the dependency graph of Kustomize and the integration with kubectl. No mechanism currently exists, however, to support official distribution of these capabilities, so they are instead built into Kustomize.

As an example, the Helm functionality built into Kustomize currently relies on the user to install Helm as a separate step. This in turn requires the use of special flags on invocation to use, for security reasons. If the Helm integration was instead built and distributed as a container based plugin, the implementation could instead use the Helm Go packages and be built independently of Kustomize and distributed to users of Kustomize through an official channel.
Copy link
Member

Choose a reason for hiding this comment

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

We currently have an example of helm function in https://catalog.kpt.dev/render-helm-chart/v0.1/.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, we would love for it to be included as part of the default catalog; it has all of the same supported fields as the Helm plugin.

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 think that's a good goal to have, but the default catalog actually being implemented was declared as not in scope as part of this KEP (unless we need it to be as part of this KEP), and then we'd need to look at migration of things.

name: "example-co-plugins"
spec:
modules:
- apiVersion: example.com/v1
Copy link
Member

Choose a reason for hiding this comment

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

Does the order in the modules list matter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The order shouldn't matter, but they should be unique. I'll add some wording to clarify this, thanks for asking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correction, I think order does matter. I added some clarifying text to the resolution of functions to address this.

@mengqiy
Copy link
Member

mengqiy commented Aug 31, 2021

/hold
Adding the hold label to ensure all the comments are addressed before merging.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2021
When HTTP(s) references are used, the HTTP(s) endpoint must support anonymous access for reading resources. Resources will be expected to be stored as a single file, such as `catalog.yaml`. Git or OCI references can be authenticated or anonymous, and will use appropriate configuration from the users file-system.


### User Stories (Optional)
Copy link
Member

@mengqiy mengqiy Sep 9, 2021

Choose a reason for hiding this comment

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

IMO the following user stories missing:

Story 6

Assuming we will have a public KRM function catalog like https://catalog.kpt.dev/
As a kustomize plugin (KRM function) developer at company Example Co, I want to contribute one kustomize plugin (KRM function) to the public KRM function catalog. I want be publish all related metadata (if require network, if require volume mount, if it's idempotent) for this KRM function. I don't want to publish a catalog.

Story 7

As a platform developer at enterprise company Example Co, I want to create a catalog containing the following plugins

  • One plugin developed by Banana Co (Assuming they have published the metadata for it)
  • One plugin developed by Watermelon Co (Assuming they have published the metadata for it)
  • One plugin developed by my company Example Co

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mengqiy I'll update and address these in another commit!

Signed-off-by: Jeremy <jeremyrrickard@gmail.com>
provider:
container:
image: docker.example.co/functions/java:v1.0.0
- apiVersion: example.com/v1
Copy link
Member

Choose a reason for hiding this comment

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

Suppose we initially have docker.example.co/functions/logger:v1.0.0 that only supports functionConfig with apiVersion = example.com/v1alpha1 and kind=Logger.
We decide to promote the functionConfig from v1alpha1 to v1beta1 and we want to new function docker.example.co/functions/logger:v1.1.0 to support both apiVerisons.

As a platform developer, usually I cannot force all of my users in my organization to update the apiVersion from v1alpha1 to v1beta1. I will need to provide both apiVersions in my catalog.

How should we present it in the Catalog object? Will it look like the following? It's too verbose and I'm repeating myself.

apiVersion: kustomize.io/v1
kind: Catalog
metadata: 
  name: "example-co-functions"
spec: 
  providers: 
  - apiVersion: example.com/v1alpha1
    kind: Logger
    description: "A Kustomize function that adds our bespoke logging"
    provider: 
      container: 
        image: docker.example.co/functions/logger:v1.1.0
  - apiVersion: example.com/v1beta1
    kind: Logger
    description: "A Kustomize function that adds our bespoke logging"
    provider: 
      container: 
        image: docker.example.co/functions/logger:v1.1.0

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 are doing that currently in our internal proof of concept, I agree it gets verbose....but I think it's also needed to represent the available types. We could change the structure and have an array of apiVersions to allow multiple to be represented. In our internal prototype currently, we have them as separate blocks and that allows us to clearly mark one as deprecated (roughly as follows):

apiVersion: functions/v1alpha1
kind: Catalog
spec:
   providers:
  - apiVersion: v1alpha1
     kind: LoadBalancer
     deprecated: true
     description: Adds the necessary resources and configuration into an existing deployment to expose a service via the Load Balancer over HTTPs.
     providers:
       container:
          image: lb@sha256:23a7b7a1c
   - apiVersion: v1beta1
      kind: LoadBalancer
     description: Adds the necessary resources and configuration into an existing deployment to expose a service via the Load Balancer over HTTPs.
     providers:
        container:
          image: lb@sha256:23a7b7a1

We are currently generating this file, so the repetition isn't really an issue.

I'd be open to a more compact definition if you think we can still maintain the ability to add more information (perhaps the deprecation info, perhaps other things later) on a per version?

Maybe inverting it a little:

spec: 
  providers: 
     - kind: Logger
       versions:
           - apiVersion: v1alpha
              deprecated: true
           - apiVersion: v1beta1
       container:
            image:  docker.example.co/functions/logger:v1.1.0

wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

What do you think about something like the following?

apiVersion: kustomize.io/v1
kind: Catalog
metadata: 
  name: "example-co-functions"
spec: 
  providers: 
  - apiVersion: example.com/v1beta1    # primary apiVersion
    kind: Logger                       # primary kind
    additionalAPIVersionKinds:
      - apiVersion: example.com/v1alpha1
        kind: Logger
        deprecated: true
      - apiVersion: example.com/v1alpha1
        kind: LegacyLogger
        deprecated: true
    description: "A Kustomize function that adds our bespoke logging"
    provider: 
      container: 
        image: docker.example.co/functions/logger:v1.1.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that sounds good to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I like that the new design clearly shows the relationship between multiple API versions. Two questions:

  • In situations where a given provider supports multiple unrelated GVKs, we still want separate top-level entries, right? Might it be reasonable to not allow the Kind to differ in the "additional" subentries?
  • Consider the case where there are multiple providers (e.g. an exec and a container). This layout implies they will all support all listed GVs and be updated in lock step. Is that a reasonable requirement? E.g. what if you introduce an exec option at v1, and do not want to implement v1alpha1 support. Will we end up inlining all the top-level fields under "additional" as overrides?

Copy link
Member

Choose a reason for hiding this comment

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

In situations where a given provider supports multiple unrelated GVKs, we still want separate top-level entries, right? Might it be reasonable to not allow the Kind to differ in the "additional" subentries?

If a provider supports multiple GVKs that are completely unrelated, ideally they should be separate top-level entries. For example, if I have apiVersions example.com/v1 and example.com/v2, they should be listed as separate top-level entries, even though one single KRM function may support both of them.
v1.ConfigMap as functionConfig might be something special here. I think we all agreed that the primary GVK should always be a strong typed client-side CRD, andv1.ConfigMap should never be used as the primary GVK. But IMO it'd be valuable to support ConfigMap as a secondary GVK.
IMO we should allow some flexibility but also provide a best practice doc about how to organize them.
@KnVerey @jeremyrickard WDYT?

Consider the case where there are multiple providers (e.g. an exec and a container). This layout implies they will all support all listed GVs and be updated in lock step. Is that a reasonable requirement? E.g. what if you introduce an exec option at v1, and do not want to implement v1alpha1 support. Will we end up inlining all the top-level fields under "additional" as overrides?

When a user have a exec-based KRM function, it's fairly easy to put it in a container. Then they will support the same GVKs. But this practice doesn't apply to starlark provider. Same as above, we should provide a best practice doc about how to organize them.

@KnVerey @jeremyrickard Can someone please give me a pointer to the description of the lock step?

We will not end up inlining all the top-level fields under "additional" as overrides. The users must use different top-level entries in at least the following cases:

  • Major KRM version bump. e.g. from example.com/v1* to example.com/v2*.
  • The provider don't support all of these GVKs. e.g. I have docker.example.co/functions/logger:v1.0.0 that don't support example.com/v1beta1. Our catalog may looks like:
apiVersion: kustomize.io/v1
kind: Catalog
metadata: 
  name: "example-co-functions"
spec: 
  providers: 
  - apiVersion: example.com/v1beta1
    kind: Logger
    additionalAPIVersionKinds:
      - apiVersion: example.com/v1alpha1
        kind: Logger
        deprecated: true
      - apiVersion: example.com/v1alpha1
        kind: LegacyLogger
        deprecated: true
    description: "A Kustomize function that adds our bespoke logging"
    provider: 
      container: 
        image: docker.example.co/functions/logger:v1.1.0
  - apiVersion: example.com/v1alpha1
    kind: Logger
    additionalAPIVersionKinds:
      - apiVersion: example.com/v1alpha1
        kind: LegacyLogger
        deprecated: true
    description: "A Kustomize function that adds our bespoke logging"
    provider: 
      container: 
        image: docker.example.co/functions/logger:v1.0.0

As descrived in this KEP, the order matters in the spec.providers list. If a user specify apiVersion: example.com/v1alpha1 and kind: Logger, the provider docker.example.co/functions/logger:v1.1.0 is prefered over docker.example.co/functions/logger:v1.0.0.
Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Reading through this and the similar content on the other KEP, I'm leaning strongly towards the opinion that the items in the krmFunctions field (currently called providers in the examples but agreed to change in comments) are effectively the CRDs for these client-side types, and it would be beneficial to have the KRMFunction type (which each item in this list is, and which will appear independently in the Registry's files that are used to build Catalogs) be very close to the "real" CRD schema in structure. There are of course fields that don't apply (e.g. storage version) and additional fields we need to add (e.g. some things around publishers), but the core needs of the two types are the same, and there is some inherent benefit to be had from consistency. Notably, I think each KRMFunction should (like a CRD) define a single Group+Kind combination that has multiple versions, and have many/most of the fields move down under individual versions, since those properties can in principle change with the version.

This layout implies they will all support all listed GVs and be updated in lock step.
Can someone please give me a pointer to the description of the lock step?

Sorry for the confusion. There's no "lock step". What I meant was that under the proposal in your earlier comment, there would be no way for a given provider not to support all possible group-versions corresponding to the kind. You could never update the container-based version before the exec release, or more realistically, you could never start supporting exec without making your new binary support old versions you've already deprecated. The point is that the implementation is necessarily at the Group+Kind+Version level, so the provider field needs to be version-specific.

The users must use different top-level entries in at least the following cases:
Major KRM version bump. e.g. from example.com/v1* to example.com/v2*.

If anything is related, it's the new versions of the same GK. As in a CRD, these should be grouped together.

The provider don't support all of these GVKs. e.g. I have docker.example.co/functions/logger:v1.0.0 that don't support example.com/v1beta1.

Catalog should group based on identity (Group+Kind), not provider. This example relates to the point I made above: since implementations inherently support the GVK level, provider needs to be a version-level field. If the same provider is used for multiple versions, that's fine. It's even fine if an org chooses to publish a container that supports dozens of unrelated GVKs. Since we're API-oriented, not code-oriented, that isn't a problem: the provider in question simply appears under every version it supports.

v1.ConfigMap as functionConfig might be something special here. I think we all agreed that the primary GVK should always be a strong typed client-side CRD, andv1.ConfigMap should never be used as the primary GVK. But IMO it'd be valuable to support ConfigMap as a secondary GVK.
IMO we should allow some flexibility but also provide a best practice doc about how to organize them.

(This answer is essentially a cross-post of #2986 (comment))
Introducing a notion of a "secondary GVK" significantly complicates the mental model, and I'm not yet understanding the benefit we get from it. It would also constitute a divergence from the server-side version of the CRD pattern. IIUC all Kpt needs to know to support its "configmap" field is that the fields are map[string]string, which it could deduced from the provided schema, or perhaps could be signalled with a single boolean field on a particular version if necessary. What am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introducing a notion of a "secondary GVK" significantly complicates the mental model, and I'm not yet understanding the benefit we get from it. It would also constitute a divergence from the server-side version of the CRD pattern. IIUC all Kpt needs to know to support its "configmap" field is that the fields are map[string]string, which it could deduced from the provided schema, or perhaps could be signalled with a single boolean field on a particular version if necessary. What am I missing?

I think that additionalAPIVersionKinds complicates things a little too much and think we should opt to simplify it.

Reading through this and the similar content on the other KEP, I'm leaning strongly towards the opinion that the items in the krmFunctions field (currently called providers in the examples but agreed to change in comments) are effectively the CRDs for these client-side types, and it would be beneficial to have the KRMFunction type (which each item in this list is, and which will appear independently in the Registry's files that are used to build Catalogs) be very close to the "real" CRD schema in structure.

I think the KRMFunction type is more or less defined in the other KEP at the moment. Should we explicitly define it here, as part of the catalog format? I think with the comments below about Catalog defining the format and the other KEP being the implementation and mechanism, it would make sense.

Copy link
Member

Choose a reason for hiding this comment

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

I agree making it CRD-like makes sense to me. I have updated https://github.com/kubernetes/enhancements/pull/2986/files in the function metadata section.

I think the KRMFunction type is more or less defined in the other KEP at the moment. Should we explicitly define it here, as part of the catalog format? I think with the comments below about Catalog defining the format and the other KEP being the implementation and mechanism, it would make sense.

I think we all agree that Catalog and KRM function metadata should share the same underlying schema for a single KRM function.
Given that our plan is merge this KEP as provisional soon-ish, we can merge this KEP mostly as-is. I will create a PR to move the schema to this KEP.
WDYT?

metadata:
name: "example-co-functions"
spec:
providers:
Copy link
Contributor

@KnVerey KnVerey Sep 24, 2021

Choose a reason for hiding this comment

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

How about functions or krmFunctions for this key? The items are effectively a list of the (implicit) KRMFunction type @mengqiy has been suggesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works, do you have a preference on the term?

Copy link
Contributor

Choose a reason for hiding this comment

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

Very slight preference for krmFunctions

Copy link
Member

Choose a reason for hiding this comment

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

+1 on krmFunctions.

@mikebz
Copy link

mikebz commented Oct 5, 2021

Question: how do we plan to address the delta between plugin registry and plugin catalog? They seem to define a lot of the same stuff. If the Catalog is just around figuring out which plugins are allowed to run in my environment then maybe it becomes much thinner? Just the execution I decided to allow in my pipeline?

@KnVerey
Copy link
Contributor

KnVerey commented Oct 5, 2021

how do we plan to address the delta between plugin registry and plugin catalog? They seem to define a lot of the same stuff. If the Catalog is just around figuring out which plugins are allowed to run in my environment then maybe it becomes much thinner? Just the execution I decided to allow in my pipeline?

I'd say the Catalog is a standard format for listing plugins, which orchestrators can use for various purposes (discovery, distribution, trust...), and the Registry is a producer of that format. This KEP focuses on defining that standard format, such that Kustomize/Kpt etc. could build for consuming it. Actually creating any specific catalog or publication machinery is explicitly out of scope of this KEP, and is the focus of the other one: the Registry proposes metadata and machinery for a specific SIG-sponsored collection of catalogs compliant with the format proposed in this one.

Signed-off-by: Jeremy <jeremyrrickard@gmail.com>

In addition to the new catalog kind, `kustomize build` will accept a repeatable flag `--trusted-catalog=""`. The values passed to this flag should match the values declared within the catalog. When present, this flag instructs `kustomize build` to automatically fetch and execute functions that are defined by the catalog and referenced within the Kustomization, Component or Composition, if needed. When a resource is processed by `kustomize build` and a catalog is referenced but not specified using the `--trusted-catalog=""` flag, an error will occur.

To support the user experience of using catalogs, we will also add a `kustomize view catalog` command that will display catalog configuration, including publisher information retrieved from the catalog itself, in order to support the trust determination process.
Copy link
Member

Choose a reason for hiding this comment

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

Is designing the UX of kustomize view catalog one of the goals of this KEP?

provider:
container:
image: docker.example.co/functions/java:v1.0.0
- apiVersion: example.com/v1
Copy link
Member

Choose a reason for hiding this comment

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

I agree making it CRD-like makes sense to me. I have updated https://github.com/kubernetes/enhancements/pull/2986/files in the function metadata section.

I think the KRMFunction type is more or less defined in the other KEP at the moment. Should we explicitly define it here, as part of the catalog format? I think with the comments below about Catalog defining the format and the other KEP being the implementation and mechanism, it would make sense.

I think we all agree that Catalog and KRM function metadata should share the same underlying schema for a single KRM function.
Given that our plan is merge this KEP as provisional soon-ish, we can merge this KEP mostly as-is. I will create a PR to move the schema to this KEP.
WDYT?


Alternatively, this could be published as an external resource that can be pulled by Kustomize as would any other catalog. This would decouple the release cadence of Kustomize and the official extensions, but would introduce extra latency for the end user.

#### Story 5
Copy link
Member

Choose a reason for hiding this comment

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

There are 2 story 5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, thanks for catching that.


* One plugin developed by Banana Co
* One plugin developed by Watermelon Co
* One plugin developed by my company Example Co
Copy link
Member

Choose a reason for hiding this comment

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

This one doesn't appear in the Catalog resource below. It should, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, sorry about that.

| Kustomize Catalog | application/vnd.cncf.kubernetes.krm-func-catalog.layer.v1+yaml |
| Kustomize Function Definition | application/vnd.cncf.kubernetes.krm-func-definition.layer.v1+yaml |
| Kustomize Function (Starlark) | application/vnd.cncf.kubernetes.krm-func-provider-starlark.layer.v1 |
| Kustomize Function (Exec) | application/vnd.cncf.kubernetes.krm-func-provider-starlark.layer.v1 |
Copy link
Member

Choose a reason for hiding this comment

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

Why we need a separate one here? Can we reuse Kustomize Function Definition?

I feel the OCI Artifacts part are not thoroughly discussed.
Can you mark this section as unresolved?

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'll mark it as unresolved for now, we can iterate on all this later.

@mengqiy
Copy link
Member

mengqiy commented Oct 11, 2021

There are some minor unresolved bits. Nothing major to block this KEP to be merge as provisional.

/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 11, 2021
Signed-off-by: Jeremy <jeremyrrickard@gmail.com>
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.

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jeremyrickard, monopole

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

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants