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

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

Merged
merged 11 commits into from
Sep 21, 2020
2 changes: 1 addition & 1 deletion pkg/skaffold/deploy/kpt.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ func (k *KptDeployer) kustomizeBuild(ctx context.Context) error {
return nil
}

cmd := exec.CommandContext(ctx, "kustomize", buildCommandArgs([]string{"-o", filepath.Join(pipeline, k.Dir)}, k.Dir)...)
cmd := exec.CommandContext(ctx, "kustomize", append([]string{"build"}, buildCommandArgs([]string{"-o", filepath.Join(pipeline, k.Dir)}, k.Dir)...)...)
if _, err := util.RunCmdOut(cmd); err != nil {
return err
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/skaffold/deploy/kubectl/cli.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,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...)...)
}

type getResult struct {
Items []struct {
Metadata struct {
Expand Down
62 changes: 49 additions & 13 deletions pkg/skaffold/deploy/kustomize.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,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 @@ -92,21 +93,49 @@ type secretGenerator struct {
// KustomizeDeployer deploys workflows using kustomize CLI.
type KustomizeDeployer struct {
*latest.KustomizeDeploy

kubectl deploy.CLI
insecureRegistries map[string]bool
labels map[string]string
globalConfig string
kubectl deploy.CLI
insecureRegistries map[string]bool
labels map[string]string
globalConfig string
useKubectlKustomize bool
}

func NewKustomizeDeployer(cfg Config, labels map[string]string) *KustomizeDeployer {
kubectl := deploy.NewCLI(cfg, cfg.Pipeline().Deploy.KustomizeDeploy.Flags)

// if user has kustomize binary, prioritize that over kubectl kustomize
useKubectlKustomize := !kustomizeBinaryCheck() && kubectlVersionCheck(kubectl)

return &KustomizeDeployer{
KustomizeDeploy: cfg.Pipeline().Deploy.KustomizeDeploy,
kubectl: deploy.NewCLI(cfg, cfg.Pipeline().Deploy.KustomizeDeploy.Flags),
insecureRegistries: cfg.GetInsecureRegistries(),
globalConfig: cfg.GlobalConfig(),
labels: labels,
KustomizeDeploy: cfg.Pipeline().Deploy.KustomizeDeploy,
kubectl: kubectl,
insecureRegistries: cfg.GetInsecureRegistries(),
globalConfig: cfg.GlobalConfig(),
labels: labels,
useKubectlKustomize: useKubectlKustomize,
}
}

// 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
Contributor

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(kubectl deploy.CLI) bool {
gt, err := kubectl.CompareVersionTo(context.Background(), 1, 14)
if err != nil {
return false
}
if gt == 1 {
Copy link
Contributor

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 @@ -345,8 +374,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.useKubectlKustomize {
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 @@ -361,7 +398,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
73 changes: 54 additions & 19 deletions pkg/skaffold/deploy/kustomize_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,23 @@ import (
"github.com/GoogleContainerTools/skaffold/testutil"
)

func kustomizeAbsent() bool {
return false
}

func kustomizePresent() bool {
return true
}

func TestKustomizeDeploy(t *testing.T) {
tests := []struct {
description string
kustomize latest.KustomizeDeploy
builds []build.Artifact
commands util.Command
shouldErr bool
forceDeploy bool
description string
kustomize latest.KustomizeDeploy
builds []build.Artifact
commands util.Command
shouldErr bool
forceDeploy bool
kustomizeCmdAbsent bool
}{
{
description: "no manifest",
Expand Down Expand Up @@ -90,10 +99,38 @@ func TestKustomizeDeploy(t *testing.T) {
},
forceDeploy: true,
},
{
description: "built-in kubectl kustomize",
kustomize: latest.KustomizeDeploy{
KustomizePaths: []string{"a", "b"},
},
commands: testutil.
CmdRunOut("kubectl version --client -ojson", kubectlVersion118).
AndRunOut("kubectl --context kubecontext --namespace testNamespace kustomize a", deploymentWebYAML).
AndRunOut("kubectl --context kubecontext --namespace testNamespace kustomize b", deploymentAppYAML).
AndRunInputOut("kubectl --context kubecontext --namespace testNamespace get -f - --ignore-not-found -ojson", deploymentWebYAMLv1+"\n---\n"+deploymentAppYAMLv1, "").
AndRun("kubectl --context kubecontext --namespace testNamespace apply -f - --force --grace-period=0"),
builds: []build.Artifact{
{
ImageName: "leeroy-web",
Tag: "leeroy-web:v1",
},
{
ImageName: "leeroy-app",
Tag: "leeroy-app:v1",
},
},
forceDeploy: true,
kustomizeCmdAbsent: true,
},
}

for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
t.Override(&util.DefaultExecCommand, test.commands)
if test.kustomizeCmdAbsent {
t.Override(&kustomizeBinaryCheck, kustomizeAbsent)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You can rename testKustomizeCmdAbsent to testKustomizeCmdPresent and instead override the function directly here.

Suggested change
if test.kustomizeCmdAbsent {
t.Override(&kustomizeBinaryCheck, kustomizeAbsent)
}
t.Override(&kustomizeBinaryCheck, func () bool { return test.kustomizeCmdPresent} )

t.NewTempDir().
Chdir()

Expand Down Expand Up @@ -157,17 +194,15 @@ func TestKustomizeCleanup(t *testing.T) {
kustomize: 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,
},
}
for _, test := range tests {
testutil.Run(t, test.description, func(t *testutil.T) {
t.Override(&util.DefaultExecCommand, test.commands)
t.Override(&kustomizeBinaryCheck, kustomizePresent)

k := NewKustomizeDeployer(&kustomizeConfig{
workingDir: tmpDir.Root(),
Expand Down Expand Up @@ -396,49 +431,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