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

Enable kustomize in kubectl #70875

Open
wants to merge 6 commits into
base: master
from

Conversation

@Liujingfang1
Contributor

Liujingfang1 commented Nov 9, 2018

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR is the implementation of KEP to enable kustomize in kubectl.

When -f <dir> is passed to a kubectl command, it will look for a kustomization.yaml file. If kustomization.yaml is found, a kustomize build will be run to get the list of expanded resources. This list of resources is then passed to kubectl commands. If there is no kustomization.yaml in the directory, kubectl will behave the same as current.

To apply a kustomization directory

kubectl apply -f <dir>

To get resources of a kustomization directory applied to a cluster

kubectl get -f <dir>

To delete a kustomization directory applied to a cluster

kubectl delete -f <dir>

Special notes for your reviewer:

This PR contains 6 commits.

  • The first three of them is to vendor kustomize.
  • The 4th commit is the change in resource builder to use Kustomization when it it enabled.
  • The 5th one adds some unit test for Builder.
  • The 6th commit is to enable kustomization in kubectl commands.

Kubectl will have kustomization enabled by default.
Other cli-runtime consumers may choose if they want to enable or not kustomization by a Boolean variable.

Does this PR introduce a user-facing change?:
NONE

Enable customize in kubectl: kubectl will be able to recognize directories with kustomization.YAML
@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Nov 9, 2018

Hi @Liujingfang1. Thanks for your PR.

I'm waiting for a kubernetes 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.

@Liujingfang1

This comment has been minimized.

Contributor

Liujingfang1 commented Nov 9, 2018

@k8s-ci-robot k8s-ci-robot requested review from pwittrock, seans3 and soltysh Nov 9, 2018

@Liujingfang1

This comment has been minimized.

Contributor

Liujingfang1 commented Nov 9, 2018

/cc @monopole

@k8s-ci-robot k8s-ci-robot requested a review from monopole Nov 9, 2018

@Liujingfang1 Liujingfang1 changed the title from Enable kustomize to Enable kustomize in kubectl Nov 9, 2018

@@ -452,7 +457,10 @@ func ExpandPathsToFileVisitors(mapper *mapper, paths string, recursive bool, ext
if err != nil {
return err
}
if isKustomizationDir(path) {

This comment has been minimized.

@smarterclayton

smarterclayton Nov 10, 2018

Contributor

I don't think this is backwards compatible. Anyone using kubectl create -f DIR will see something different happen after this change lands, which means existing CLI workflows could break.

This comment has been minimized.

@justinsb

justinsb Nov 11, 2018

Member

I just tested the behaviour. kubectl create and kubectl apply both fail with a kustomization.yaml present:

error: error validating ".../k8s.io/examples/guestbook-go/kustomization.yaml": error validating data: [apiVersion not set, kind not set]; if you choose to ignore these errors, turn validation off with --validate=false

It looks like we have a separate issue, which is that we don't validate all files before starting to apply, which is contrary to what I would have expected.

But I don't think users can be using kubectl create -f DIR or kubectl apply -f DIR today with a dir containing kustomization.yaml.

This comment has been minimized.

@Liujingfang1

Liujingfang1 Nov 12, 2018

Contributor

@smarterclayton The existing kubectl doesn't work with directories with a kustomization.yaml as @justinsb explained. With this PR, kubectl will be able to recognize a directory with a kustomization.yaml. For any directories without kustomization.yaml, there is no change in kubectl's behavior.

This comment has been minimized.

@smarterclayton

smarterclayton Nov 13, 2018

Contributor

If someone has validate=false off, what happens? If it also fails, then my primary concern is addressed.

This comment has been minimized.

@Liujingfang1

Liujingfang1 Nov 13, 2018

Contributor

With validate=false, it fails with similar error

error: unable to decode "kustomization.yaml": Object 'Kind' is missing in `<truncated>`

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:enable-kustomize branch 2 times, most recently from a401eca to b426d83 Nov 12, 2018

@seans3

This comment has been minimized.

Contributor

seans3 commented Nov 12, 2018

/ok-to-test

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:enable-kustomize branch 2 times, most recently from 90bac14 to 3c4901b Nov 12, 2018

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:enable-kustomize branch from dcb3ff7 to 4e1e8e9 Nov 26, 2018

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:enable-kustomize branch 3 times, most recently from 3e69aaf to 8292f48 Nov 26, 2018

@Liujingfang1

This comment has been minimized.

Contributor

Liujingfang1 commented Nov 27, 2018

Rebased for both kubernetes and kustomize changes. All tests passed. PTAL

@Liujingfang1

This comment has been minimized.

Contributor

Liujingfang1 commented Dec 4, 2018

@cblecker Can you take a look at the dep change? Only sigs.k8s.io/kustomize/pkg is added. Thank you.

@Liujingfang1

This comment has been minimized.

Contributor

Liujingfang1 commented Dec 4, 2018

@lavalamp Can you take a look at the staging change? The change is that sigs.k8s.io/kustomize/pkg is added to cli-runtime with its transitive dependencies. Thank you.

@cblecker

This comment has been minimized.

Member

cblecker commented Dec 5, 2018

No concerns with the Godep import changes. Can't /approve yet, as I can't speak to the import restriction changes (those should be reviewed by sig-cli).

@Liujingfang1

This comment has been minimized.

Contributor

Liujingfang1 commented Dec 5, 2018

@smarterclayton We're planning to merge this PR. Do you have any final comments?

@Liujingfang1

This comment has been minimized.

Contributor

Liujingfang1 commented Dec 5, 2018

@cblecker Thank you for reviewing it. I've discussed this PR with sig-cli people. They will put their comments later if there is any.

@@ -463,7 +471,10 @@ func ExpandPathsToFileVisitors(mapper *mapper, paths string, recursive bool, ext
if path != paths && ignoreFile(path, extensions) {
return nil
}
if strings.HasSuffix(path, "kustomization.yaml") {

This comment has been minimized.

@pwittrock

pwittrock Dec 5, 2018

Member

why not filepath.Base(path) == "kustomization.yaml"

This comment has been minimized.

@liggitt

liggitt Dec 5, 2018

Member

I thought the intent of cli-runtime was to be generic function for things consuming resource builder functionality. Hard-coding this seems extremely specific and not very scalable.

Can we pull this out to a package var or registration method that kubectl could inject special handling for kustomization.yaml into, rather than making all cli-runtime consumers pick up kustomize?

This comment has been minimized.

@juanvallejo

juanvallejo Dec 5, 2018

Member

It sounds to me like there are two distinct approaches to the kustomize implementation:

  1. kustomize remains unique to kubectl, it lives in kubectl, and any extra behavior it needs to introduce to the resource-builder (or any other generic utilities used to consume resources) is done from a "initialization" function within kubectl.

  2. kustomize becomes a part of the overall cli ecosystem, we keep it hardcoded within genericclioptions, and kubectl along with third-party clients and plugins all inherit this new capability of handling "kustomize" directories

I would be fine with either of these two approaches, however I do see value in propagating this behavior to consumers of genericclioptions, as it would maintain this similar behavior with kubectl.

cc @soltysh @smarterclayton @deads2k for additional thoughts on this

This comment has been minimized.

@Liujingfang1

Liujingfang1 Dec 5, 2018

Contributor

@pwittrock I'll update the statement. @liggitt We can change the hard coded kustomization.yaml by a var or registration method. That will require the change from the kustomize side. Can we do it in a following PR?

@juanvallejo I prefer it becomes a part of the overall cli ecosystem so that other clients can also use kustomize by default.

This comment has been minimized.

@soltysh

soltysh Dec 5, 2018

Contributor

So Juan and I discussed this in depth and here's our proposed solution. We picked a middle-ground solution from what he proposed in the previous comment. This means that cliruntime will have extension point (like Jordan proposes) where an author would be able to include additional functionality (eg. cliruntime.Addons["kustomization.yaml"] = kustomizationHandler()). Kustomize would be such an extensions, our first one. It would entirely live inside its current repo (https://github.com/kubernetes-sigs/kustomize).
This way kubectl (as its first consumer) would vendor both cliruntime and kustomize and initilize kustomize as an addon for cliruntime. This would allow few things:

  1. keep cliruntime generic and very minimal
  2. provide extensions points for cliruntime (builder to start with)
  3. give cliruntime consumers bigger flexibility over what they're getting.

@pwittrock @liggitt @juanvallejo @deads2k @smarterclayton what do you think
@Liujingfang1 I'm sorry we're changing the design one more time ☺️

This comment has been minimized.

@pwittrock

pwittrock Dec 5, 2018

Member

The goal of cli-runtime is provide libraries that promote consistent behavior across cli's. The goal of this PR is for kubectl commands to support the kustomization.yaml format. Do we think consumers of this library won't want the same handling as kubectl? If that is case, maybe this should be opt-out instead of always on.

This comment has been minimized.

@pwittrock

pwittrock Dec 6, 2018

Member

@soltysh Didn't see your comment, responses inlined.

  1. keep cliruntime generic and very minimal

This also makes the default behavior for clients inconsistent with the default behavior for kubectl commands such as apply, get, describe. I do like the idea of having a simple and modular architecture by making all of the Resource Builder targets be plugins (outside the scope of this PR).

  1. provide extensions points for cliruntime (builder to start with)

I'd like to see more discussion about this and what the interfaces might look like before committing to it. I do like the idea, but I think it deserves consideration in its own right.

  1. give cliruntime consumers bigger flexibility over what they're getting.

This can be done very simply by making it opt-out with an argument to the builder. Then we continue to the "consistency by default" without making it hardcoded to be on.

This comment has been minimized.

@monopole

monopole Dec 6, 2018

Contributor

For context, from the cli-runtime readme:

Purpose:
This library is a shared dependency for clients to work with Kubernetes API
infrastructure which allows to maintain kubectl compatible behavior.
Its first consumer is k8s.io/kubectl.

To be useful in a k8s context, cli-runtime must be cognizant of API guidelines and recognize (depend on) things like TypeMeta and ObjectMeta, and be able to wrangle YAML/JSON, rest, patching, etc, even as it avoids direct dependence on the API types.

kustomize was built with the same goals and restrictions. It was developed as a standalone project (outside k/k) as an expediency to release faster, get feedback faster, prove utility and to avoid making the kubectl extraction project more difficult. It was always intended to become part of the builder, to enhance the CLI’s ability to manipulate k8s-style objects in multiple commands and contexts.

It’s a cross-cutting enhancement to manipulate k8s YAML meant to collectively address old kubectl bugs mentioned in the KEP. We should fix these issues for all runtime consumers, not just kubectl users.

So please accept this PR (more or less as is, modulo how the file name is handled), and if there's some outcry from CLI users who don't want these fixes/features, we can factor it out of the runtime then.

@soltysh

/hold
until we agree on the final shape

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:enable-kustomize branch 3 times, most recently from 400d195 to ccd865d Dec 6, 2018

@pwittrock

This comment has been minimized.

Member

pwittrock commented Dec 7, 2018

Lets finalize decisions on this during the contributor summit or kubectl meeting next week.

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:enable-kustomize branch from ccd865d to 225af86 Dec 7, 2018

@Liujingfang1

This comment has been minimized.

Contributor

Liujingfang1 commented Dec 7, 2018

/retest

@Liujingfang1 Liujingfang1 force-pushed the Liujingfang1:enable-kustomize branch from 225af86 to 0ce8427 Dec 12, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 12, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Liujingfang1
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approvers: cblecker, smarterclayton

If they are not already assigned, you can assign the PR to them by writing /assign @cblecker @smarterclayton in a comment when ready.

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

This comment has been minimized.

Contributor

k8s-ci-robot commented Dec 12, 2018

@Liujingfang1: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-verify 0ce8427 link /test pull-kubernetes-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@Liujingfang1

This comment has been minimized.

Contributor

Liujingfang1 commented Dec 12, 2018

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment