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: Update KEP for move to implementable #2910

Merged
merged 1 commit into from
Sep 22, 2021

Conversation

KnVerey
Copy link
Contributor

@KnVerey KnVerey commented Aug 27, 2021

  • One-line PR description: Update KEP contents and add information required to move it to implementable.
  • Other comments: This update does not change the core of the proposal, though it rewords it significantly for clarity and scope. Notably, terminology has been updated for better consistency with existing Kustomize features (e.g. module->transformer). Content that actually belonged in the Catalog KEP instead of here has been removed. This KEP's implications no longer extend beyond the introduction of a new Kind that is processed differently during Kustomize builds.

Please note that this is a Kustomize-internal feature that does not introduce any novel risks. I think it would have been a good candidate for the in-repo mini proposal process discussed with the SIG and proposed in kubernetes-sigs/kustomize#4128, had that existed at the time of initial drafting. I'm open to moving it there now rather than promoting to implementable, if that is what reviewers desire. IMO the most compelling reason for it to remain on k/enhancements is that a couple other in-flight KEPs that do have broader implications mention it.

/cc @jeremyrickard @monopole @natasha41575

@k8s-ci-robot k8s-ci-robot added 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 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 Aug 27, 2021
@wojtek-t
Copy link
Member

/assign @ehashman

# generate resources for a Java application
- apiVersion: example.com/v1
kind: JavaApplication
provider: {container: {image: example/module_providers/java:v1.0.0}}
provider: {container: {image: docker.example.co/kustomize_transformers/java:v1.0.0}}
Copy link
Contributor

@natasha41575 natasha41575 Aug 31, 2021

Choose a reason for hiding this comment

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

Apologies if this is addressed elsewhere (I could not find it), but is this provider configuration intended to be identical to the provider configuration proposed by catalog?

Copy link
Contributor Author

@KnVerey KnVerey Aug 31, 2021

Choose a reason for hiding this comment

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

This is just https://github.com/kubernetes-sigs/kustomize/blob/master/kyaml/fn/runtime/runtimeutil/functiontypes.go#L128, which currently surfaces as an annotation containing JSON data. This KEP is proposing that that information be moved from the annotation to this reserved field, at least within Composition. It is not proposing any particular changes to the data within.

Edit: Catalog might need to propose some changes (particularly to improve exec functionality), but generally speaking I think this should be the same structure everywhere it ultimately surfaces.

specific allowlists compiled in, and for additional sources to be allowlisted at
invocation time.
This proposal effectively takes a feature set that exists in Kustomize today and makes it much more
usable in certain workflows. As such, it does not come with any novel technical risks.
Copy link
Contributor

@natasha41575 natasha41575 Aug 31, 2021

Choose a reason for hiding this comment

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

I noticed you removed the section stating that container-based functions will be run without networking or volume access. Does that mean Composition will support the network and mounts fields?

This question is coming in part from wanting clarity about the syntax proposed in https://github.com/kubernetes/enhancements/pull/2908/files#r699656279, which seems to suggest changing the container spec; I'm wondering if Composition is assuming the same changed UX.

EDIT: Thinking about it more and I guess this is out of scope for this KEP. So long as Catalog and Composition are using the same UX this is not an issue and can be discussed on the catalog KEP instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, this KEP itself doesn't change the fields required/supported for any particular plugin type. It will support whatever options are present when it merges.

@natasha41575
Copy link
Contributor

natasha41575 commented Aug 31, 2021

/lgtm

It would be good to also have feedback from @monopole and @yuwenma.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2021
@ehashman
Copy link
Member

ehashman commented Sep 1, 2021

This is not currently being tracked by RT, can you add to https://docs.google.com/spreadsheets/d/1P1J1QpayRmh2SNjs8T-wBCb6SgEOdWTRQ7MBol7yibk/edit#gid=1954476102

@KnVerey
Copy link
Contributor Author

KnVerey commented Sep 1, 2021

@ehashman I don't think it belongs in that spreadsheet. It's a Kustomize-internal feature, and Kustomize has an independent release cycle. As I said in the PR description, this KEP exists in k/enhancements because we didn't have a Kustomize-internal process until extremely recently, and I'm open to moving this KEP to the Kustomize repo if that is preferred.

@jeremyrickard
Copy link
Contributor

This is not currently being tracked by RT, can you add to https://docs.google.com/spreadsheets/d/1P1J1QpayRmh2SNjs8T-wBCb6SgEOdWTRQ7MBol7yibk/edit#gid=1954476102

This enhancement is really out of tree, it's purely an implementation of something within Kustomize (and won't land in kubectl), so I don't think this needs to be tracked in the release spreadsheet. I think we'd track it if there was something interesting to drop in the release notes, but I don't think we need to...I also don't think it needs a PRR? Probably gets back to the proposed scope field ref: #2311 ?

@ehashman
Copy link
Member

ehashman commented Sep 8, 2021

@ehashman I don't think it belongs in that spreadsheet. It's a Kustomize-internal feature, and Kustomize has an independent release cycle. As I said in the PR description, this KEP exists in k/enhancements because we didn't have a Kustomize-internal process until extremely recently, and I'm open to moving this KEP to the Kustomize repo if that is preferred.

I assume design docs in k/enhancements generally require wider discussion outside the sponsoring SIG and if that's not the case this probably isn't the right place.

Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

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

This appears to be fully out of tree and the PRR questionnaire has not been completed... ack as PRR team member that this is not subject to PRR review.

/approve

@monopole
Copy link
Contributor

/approve

Copy link
Contributor

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ehashman, KnVerey, monopole, soltysh

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 Sep 22, 2021
@k8s-ci-robot k8s-ci-robot merged commit c57c443 into kubernetes:master Sep 22, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 22, 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants