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.

Sign up for GitHub

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

Jump to bottom

Add fallback to kubectl kustomize if kustomize binary isn't present #4484

Merged
merged 11 commits into from
Sep 21, 2020
Merged

Add fallback to kubectl kustomize if kustomize binary isn't present #4484

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
26ceb9e
add fallback to kubectl kustomize if kustomize binary isn't present i…
MarlonGamez Jul 16, 2020
0c8e998
use exec.LookPath("kustomize") over "which kustomize", add kubectl ve…
MarlonGamez Jul 23, 2020
aa3a086
fix merge conflicts
MarlonGamez Jul 23, 2020
2fa2c99
fix kubectl version double call, nits
MarlonGamez Sep 9, 2020
c5a49d6
trying to kickoff travis
MarlonGamez Sep 9, 2020
1a00075
fix merge conflicts
MarlonGamez Sep 9, 2020
61796f3
remove buildargs
MarlonGamez Sep 9, 2020
9d6967d
fix new test and kpt deployer kustomize call
MarlonGamez Sep 9, 2020
1853c63
add another mock kustomize check to fake true
MarlonGamez Sep 9, 2020
095909f
cleanup some functions and tests
MarlonGamez Sep 21, 2020
8706968
fix issues with rebase
MarlonGamez Sep 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions pkg/skaffold/deploy/kubectl/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ func (c *CLI) Apply(ctx context.Context, out io.Writer, manifests ManifestList)
return nil
}

// Kustomize runs `kubectl kustomize` with the provided args
func (c *CLI) Kustomize(ctx context.Context, args []string) ([]byte, error) {
return c.RunOut(ctx, "kustomize", c.args(nil, args...)...)
}

// ReadManifests reads a list of manifests in yaml format.
func (c *CLI) ReadManifests(ctx context.Context, manifests []string) (ManifestList, error) {
var list []string
Expand Down
47 changes: 44 additions & 3 deletions pkg/skaffold/deploy/kustomize.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ var (
DefaultKustomizePath = "."
kustomizeFilePaths = []string{"kustomization.yaml", "kustomization.yml", "Kustomization"}
basePath = "base"
kustomizeBinaryCheck = kustomizeBinaryExists // For testing
)

// kustomization is the content of a kustomization.yaml file.
Expand Down Expand Up @@ -99,9 +100,13 @@ type KustomizeDeployer struct {
labels map[string]string
BuildArgs []string
globalConfig string
useKubectl bool
}

func NewKustomizeDeployer(runCtx *runcontext.RunContext, labels map[string]string) *KustomizeDeployer {
// if user has kustomize binary, prioritize that over kubectl kustomize
useKubectl := !kustomizeBinaryCheck() && kubectlVersionCheck(runCtx)

return &KustomizeDeployer{
KustomizeDeploy: runCtx.Cfg.Deploy.KustomizeDeploy,
kubectl: deploy.CLI{
Copy link
Member

Choose a reason for hiding this comment

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

It seems odd to pass in a kubectl.CLI if we're not to use kubectl.

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 I should rename useKubectl to useKubectlKustomize. We still need the kubectl.CLI as the kustomize deployer still uses kubectl apply either way

Expand All @@ -113,7 +118,36 @@ func NewKustomizeDeployer(runCtx *runcontext.RunContext, labels map[string]strin
BuildArgs: runCtx.Cfg.Deploy.KustomizeDeploy.BuildArgs,
globalConfig: runCtx.Opts.GlobalConfig,
labels: labels,
useKubectl: useKubectl,
}
}

// Check for existence of kustomize binary in user's PATH
func kustomizeBinaryExists() bool {
if _, err := exec.LookPath("kustomize"); err != nil {
return false
}

return true
Copy link
Contributor

Choose a reason for hiding this comment

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

might also be worth logging the version of the binary to the user.

Copy link
Member

Choose a reason for hiding this comment

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

What if they have an old old version of kustomize?

Copy link
Member

Choose a reason for hiding this comment

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

i read this PR as if users have kustomize binary use it even if its older version.
Does not matter if they have higher version of kubectl 1.14.
it would be really hard for us to make sure the flags users have specified in the deploy.Kustomize.Flags config are for kustomize or kubectl kustomize

}

// Check that kubectl version is valid to use kubectl kustomize
func kubectlVersionCheck(runCtx *runcontext.RunContext) bool {
kubectl := deploy.CLI{
CLI: kubectl.NewFromRunContext(runCtx),
Flags: runCtx.Cfg.Deploy.KustomizeDeploy.Flags,
ForceDeploy: runCtx.Opts.Force,
}

gt, err := kubectl.CompareVersionTo(context.Background(), 1, 14)
if err != nil {
return false
}
if gt == 1 {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if gt == 1 {
return gt ==1

return true
}

return false
}

// Deploy runs `kubectl apply` on the manifest generated by kustomize.
Expand Down Expand Up @@ -353,8 +387,16 @@ func pathExistsLocally(filename string, workingDir string) (bool, os.FileMode) {
func (k *KustomizeDeployer) readManifests(ctx context.Context) (deploy.ManifestList, error) {
var manifests deploy.ManifestList
for _, kustomizePath := range k.KustomizePaths {
cmd := exec.CommandContext(ctx, "kustomize", buildCommandArgs(k.BuildArgs, kustomizePath)...)
out, err := util.RunCmdOut(cmd)
var out []byte
var err error

if k.useKubectl {
Copy link
Member

@briandealwis briandealwis Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Worth noting that kubectl kustomize only supports a subset of kustomize functionality?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, this could manifest through build args being rejected by the binary depending on which version the user thinks they're using - but I think that's ok left up to them to handle? we can't really know which version of kustomize a user is targeting with their arguments without asking them, so I think it's ok to assume that whatever they pass will be compatible with whatever kustomize binary we end up using. this is of course assuming that they can easily know which one we'll select based on good documentation :)

out, err = k.kubectl.Kustomize(ctx, buildCommandArgs(k.BuildArgs, kustomizePath))
} else {
cmd := exec.CommandContext(ctx, "kustomize", append([]string{"build"}, buildCommandArgs(k.BuildArgs, kustomizePath)...)...)
out, err = util.RunCmdOut(cmd)
}

if err != nil {
return nil, fmt.Errorf("kustomize build: %w", err)
}
Expand All @@ -369,7 +411,6 @@ func (k *KustomizeDeployer) readManifests(ctx context.Context) (deploy.ManifestL

func buildCommandArgs(buildArgs []string, kustomizePath string) []string {
var args []string
args = append(args, "build")

if len(buildArgs) > 0 {
for _, v := range buildArgs {
Expand Down
68 changes: 49 additions & 19 deletions pkg/skaffold/deploy/kustomize_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,12 +34,13 @@ import (

func TestKustomizeDeploy(t *testing.T) {
tests := []struct {
description string
cfg *latest.KustomizeDeploy
builds []build.Artifact
commands util.Command
shouldErr bool
forceDeploy bool
description string
cfg *latest.KustomizeDeploy
builds []build.Artifact
commands util.Command
shouldErr bool
forceDeploy bool
useMockKustomizeCheck bool
Copy link
Member

Choose a reason for hiding this comment

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

From a reading perspective, it might be more intuitive for this to be kustomizeCmdAbsent? Or move the mockKustomizeCheck above with a comment as to its use.

}{
{
description: "no manifest",
Expand Down Expand Up @@ -87,10 +88,42 @@ func TestKustomizeDeploy(t *testing.T) {
},
forceDeploy: true,
},
{
description: "built-in kubectl kustomize",
cfg: &latest.KustomizeDeploy{
KustomizePaths: []string{"a", "b"},
},
commands: testutil.
CmdRunOut("kubectl version --client -ojson", kubectlVersion118).
AndRunOut("kubectl version --client -ojson", kubectlVersion118).
Copy link
Member

Choose a reason for hiding this comment

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

Do we really run kubectl version twice? That seems … suboptimal.

AndRunOut("kubectl --context kubecontext --namespace testNamespace kustomize a", deploymentWebYAML).
AndRunOut("kubectl --context kubecontext --namespace testNamespace kustomize b", deploymentAppYAML).
AndRun("kubectl --context kubecontext --namespace testNamespace apply -f - --force --grace-period=0"),
builds: []build.Artifact{
{
ImageName: "leeroy-web",
Tag: "leeroy-web:123",
},
{
ImageName: "leeroy-app",
Tag: "leeroy-app:123",
},
},
forceDeploy: true,
useMockKustomizeCheck: true,
},
}

mockKustomizeCheck := func() bool {
return false
}

for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
t.Override(&util.DefaultExecCommand, test.commands)
if test.useMockKustomizeCheck {
t.Override(&kustomizeBinaryCheck, mockKustomizeCheck)
}
t.NewTempDir().
Chdir()

Expand Down Expand Up @@ -159,11 +192,8 @@ func TestKustomizeCleanup(t *testing.T) {
cfg: &latest.KustomizeDeploy{
KustomizePaths: []string{tmpDir.Root()},
},
commands: testutil.CmdRunOutErr(
"kustomize build "+tmpDir.Root(),
"",
errors.New("BUG"),
),
commands: testutil.
CmdRunOutErr("kustomize build "+tmpDir.Root(), "", errors.New("BUG")),
shouldErr: true,
},
}
Expand Down Expand Up @@ -415,49 +445,49 @@ func TestKustomizeBuildCommandArgs(t *testing.T) {
description: "no BuildArgs, empty KustomizePaths ",
buildArgs: []string{},
kustomizePath: "",
expectedArgs: []string{"build"},
expectedArgs: nil,
},
{
description: "One BuildArg, empty KustomizePaths",
buildArgs: []string{"--foo"},
kustomizePath: "",
expectedArgs: []string{"build", "--foo"},
expectedArgs: []string{"--foo"},
},
{
description: "no BuildArgs, non-empty KustomizePaths",
buildArgs: []string{},
kustomizePath: "foo",
expectedArgs: []string{"build", "foo"},
expectedArgs: []string{"foo"},
},
{
description: "One BuildArg, non-empty KustomizePaths",
buildArgs: []string{"--foo"},
kustomizePath: "bar",
expectedArgs: []string{"build", "--foo", "bar"},
expectedArgs: []string{"--foo", "bar"},
},
{
description: "Multiple BuildArg, empty KustomizePaths",
buildArgs: []string{"--foo", "--bar"},
kustomizePath: "",
expectedArgs: []string{"build", "--foo", "--bar"},
expectedArgs: []string{"--foo", "--bar"},
},
{
description: "Multiple BuildArg with spaces, empty KustomizePaths",
buildArgs: []string{"--foo bar", "--baz"},
kustomizePath: "",
expectedArgs: []string{"build", "--foo", "bar", "--baz"},
expectedArgs: []string{"--foo", "bar", "--baz"},
},
{
description: "Multiple BuildArg with spaces, non-empty KustomizePaths",
buildArgs: []string{"--foo bar", "--baz"},
kustomizePath: "barfoo",
expectedArgs: []string{"build", "--foo", "bar", "--baz", "barfoo"},
expectedArgs: []string{"--foo", "bar", "--baz", "barfoo"},
},
{
description: "Multiple BuildArg no spaces, non-empty KustomizePaths",
buildArgs: []string{"--foo", "bar", "--baz"},
kustomizePath: "barfoo",
expectedArgs: []string{"build", "--foo", "bar", "--baz", "barfoo"},
expectedArgs: []string{"--foo", "bar", "--baz", "barfoo"},
},
}

Expand Down