From 4342ceff74ecf2936b5c73b1a0b6b6bcb897a989 Mon Sep 17 00:00:00 2001 From: Jason Hall Date: Wed, 10 Jul 2019 01:11:20 -0400 Subject: [PATCH] Implement "strict mode" When ko is invoked in this mode, import paths must have the `ko://` prefix. If a human marks an import path with `ko://` and ko can't resolve the resulting import path, it fails. In "loose mode", such an import path would be silently ignored and passed on to the resolved YAML, often resulting in invalid image names (e.g., `image: github.com/foo/bar`) In loose mode, `ko://` prefixes are always ignored for backward-compatibility. --- pkg/build/gobuild.go | 7 ++++++ pkg/build/gobuild_test.go | 42 ++++++++++++++++++++++++++++++---- pkg/build/options.go | 10 +++++++- pkg/commands/apply.go | 6 +++-- pkg/commands/create.go | 6 +++-- pkg/commands/options/local.go | 4 ++-- pkg/commands/options/strict.go | 29 +++++++++++++++++++++++ pkg/commands/publish.go | 2 +- pkg/commands/resolve.go | 7 +++--- pkg/commands/resolver.go | 17 ++++++++------ pkg/commands/run.go | 2 +- pkg/publish/default.go | 20 ++++++++-------- pkg/publish/options.go | 2 +- pkg/resolve/resolve.go | 7 +++++- 14 files changed, 126 insertions(+), 35 deletions(-) create mode 100644 pkg/commands/options/strict.go diff --git a/pkg/build/gobuild.go b/pkg/build/gobuild.go index f6b11fbbee..9bbcf97aca 100644 --- a/pkg/build/gobuild.go +++ b/pkg/build/gobuild.go @@ -49,6 +49,7 @@ type gobuild struct { build builder disableOptimizations bool mod *modInfo + strict bool } // Option is a functional option for NewGo. @@ -60,6 +61,7 @@ type gobuildOpener struct { build builder disableOptimizations bool mod *modInfo + strict bool } func (gbo *gobuildOpener) Open() (Interface, error) { @@ -72,6 +74,7 @@ func (gbo *gobuildOpener) Open() (Interface, error) { build: gbo.build, disableOptimizations: gbo.disableOptimizations, mod: gbo.mod, + strict: gbo.strict, }, nil } @@ -119,6 +122,10 @@ func NewGo(options ...Option) (Interface, error) { // Only valid importpaths that provide commands (i.e., are "package main") are // supported. func (g *gobuild) IsSupportedReference(s string) bool { + if g.strict && !strings.HasPrefix(s, "ko://") { + return false + } + s = strings.TrimPrefix(s, "ko://") p, err := g.importPackage(s) if err != nil { return false diff --git a/pkg/build/gobuild_test.go b/pkg/build/gobuild_test.go index a80d927917..5f346531ab 100644 --- a/pkg/build/gobuild_test.go +++ b/pkg/build/gobuild_test.go @@ -39,7 +39,8 @@ func TestGoBuildIsSupportedRef(t *testing.T) { // Supported import paths. for _, importpath := range []string{ - filepath.FromSlash("github.com/google/ko/cmd/ko"), // ko can build itself. + filepath.FromSlash("github.com/google/ko/cmd/ko"), // ko can build itself. + filepath.FromSlash("ko://github.com/google/ko/cmd/ko"), // ko:// prefix is ignored in loose mode. } { t.Run(importpath, func(t *testing.T) { if !ng.IsSupportedReference(importpath) { @@ -61,19 +62,52 @@ func TestGoBuildIsSupportedRef(t *testing.T) { } } -func TestGoBuildIsSupportedRefWithModules(t *testing.T) { +func TestGoBuildIsSupportedReference_Strict(t *testing.T) { base, err := random.Image(1024, 3) if err != nil { t.Fatalf("random.Image() = %v", err) } + ng, err := NewGo(WithBaseImages(func(string) (v1.Image, error) { return base, nil }), WithStrict()) + if err != nil { + t.Fatalf("NewGo() = %v", err) + } + + // Supported import paths. + for _, importpath := range []string{ + filepath.FromSlash("ko://github.com/google/ko/cmd/ko"), // ko can build itself. + } { + t.Run(importpath, func(t *testing.T) { + if !ng.IsSupportedReference(importpath) { + t.Errorf("IsSupportedReference(%q) = false, want true", importpath) + } + }) + } + + // Unsupported import paths. + for _, importpath := range []string{ + filepath.FromSlash("github.com/google/ko/cmd/ko"), // In strict mode, without ko://, it's not supported. + filepath.FromSlash("github.com/google/ko/pkg/build"), // not a command. + filepath.FromSlash("github.com/google/ko/pkg/nonexistent"), // does not exist. + } { + t.Run(importpath, func(t *testing.T) { + if ng.IsSupportedReference(importpath) { + t.Errorf("IsSupportedReference(%v) = true, want false", importpath) + } + }) + } +} +func TestGoBuildIsSupportedRefWithModules(t *testing.T) { + base, err := random.Image(1024, 3) + if err != nil { + t.Fatalf("random.Image() = %v", err) + } mod := &modInfo{ Path: filepath.FromSlash("github.com/google/ko/cmd/ko/test"), Dir: ".", } - ng, err := NewGo(WithBaseImages(func(string) (v1.Image, error) { return base, nil }), - withModuleInfo(mod)) + ng, err := NewGo(WithBaseImages(func(string) (v1.Image, error) { return base, nil }), withModuleInfo(mod)) if err != nil { t.Fatalf("NewGo() = %v", err) } diff --git a/pkg/build/options.go b/pkg/build/options.go index 63f667ca8b..24e3a87719 100644 --- a/pkg/build/options.go +++ b/pkg/build/options.go @@ -47,7 +47,6 @@ func WithDisabledOptimizations() Option { // withBuilder is a functional option for overriding the way go binaries // are built. -// This is exposed for testing. func withBuilder(b builder) Option { return func(gbo *gobuildOpener) error { gbo.build = b @@ -64,3 +63,12 @@ func withModuleInfo(mi *modInfo) Option { return nil } } + +// WithStrict is a functional option for requiring that package references are +// explicitly noted. +func WithStrict() Option { + return func(g *gobuildOpener) error { + g.strict = true + return nil + } +} diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go index da76abe752..0f33ee4779 100644 --- a/pkg/commands/apply.go +++ b/pkg/commands/apply.go @@ -34,6 +34,7 @@ func addApply(topLevel *cobra.Command) { ta := &options.TagsOptions{} do := &options.DebugOptions{} so := &options.SelectorOptions{} + sto := &options.StrictOptions{} apply := &cobra.Command{ Use: "apply -f FILENAME", Short: "Apply the input files with image references resolved to built/pushed image digests.", @@ -63,7 +64,7 @@ func addApply(topLevel *cobra.Command) { cat config.yaml | ko apply -f -`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { - builder, err := makeBuilder(do) + builder, err := makeBuilder(do, sto) if err != nil { log.Fatalf("error creating builder: %v", err) } @@ -116,7 +117,7 @@ func addApply(topLevel *cobra.Command) { stdin.Write([]byte("---\n")) } // Once primed kick things off. - resolveFilesToWriter(builder, publisher, fo, so, stdin) + resolveFilesToWriter(builder, publisher, fo, so, sto, stdin) }() // Run it. @@ -131,6 +132,7 @@ func addApply(topLevel *cobra.Command) { options.AddTagsArg(apply, ta) options.AddDebugArg(apply, do) options.AddSelectorArg(apply, so) + options.AddStrictArg(apply, sto) // Collect the ko-specific apply flags before registering the kubectl global // flags so that we can ignore them when passing kubectl global flags through diff --git a/pkg/commands/create.go b/pkg/commands/create.go index 92186c978e..eac8f06726 100644 --- a/pkg/commands/create.go +++ b/pkg/commands/create.go @@ -34,6 +34,7 @@ func addCreate(topLevel *cobra.Command) { ta := &options.TagsOptions{} do := &options.DebugOptions{} so := &options.SelectorOptions{} + sto := &options.StrictOptions{} create := &cobra.Command{ Use: "create -f FILENAME", Short: "Create the input files with image references resolved to built/pushed image digests.", @@ -63,7 +64,7 @@ func addCreate(topLevel *cobra.Command) { cat config.yaml | ko create -f -`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { - builder, err := makeBuilder(do) + builder, err := makeBuilder(do, sto) if err != nil { log.Fatalf("error creating builder: %v", err) } @@ -116,7 +117,7 @@ func addCreate(topLevel *cobra.Command) { stdin.Write([]byte("---\n")) } // Once primed kick things off. - resolveFilesToWriter(builder, publisher, fo, so, stdin) + resolveFilesToWriter(builder, publisher, fo, so, sto, stdin) }() // Run it. @@ -131,6 +132,7 @@ func addCreate(topLevel *cobra.Command) { options.AddTagsArg(create, ta) options.AddDebugArg(create, do) options.AddSelectorArg(create, so) + options.AddStrictArg(create, sto) // Collect the ko-specific apply flags before registering the kubectl global // flags so that we can ignore them when passing kubectl global flags through diff --git a/pkg/commands/options/local.go b/pkg/commands/options/local.go index d6e7fc81b9..967c8926e5 100644 --- a/pkg/commands/options/local.go +++ b/pkg/commands/options/local.go @@ -21,13 +21,13 @@ import ( // LocalOptions represents options for the ko binary. type LocalOptions struct { // Local publishes images to a local docker daemon. - Local bool + Local bool InsecureRegistry bool } func AddLocalArg(cmd *cobra.Command, lo *LocalOptions) { cmd.Flags().BoolVarP(&lo.Local, "local", "L", lo.Local, "Whether to publish images to a local docker daemon vs. a registry.") - cmd.Flags().BoolVar(&lo.InsecureRegistry, "insecure-registry", lo.InsecureRegistry, + cmd.Flags().BoolVar(&lo.InsecureRegistry, "insecure-registry", lo.InsecureRegistry, "Whether to skip TLS verification on the registry") } diff --git a/pkg/commands/options/strict.go b/pkg/commands/options/strict.go new file mode 100644 index 0000000000..6b8c965219 --- /dev/null +++ b/pkg/commands/options/strict.go @@ -0,0 +1,29 @@ +// Copyright 2019 Google LLC All Rights Reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package options + +import ( + "github.com/spf13/cobra" +) + +// StrictOptions holds options to require strict references. +type StrictOptions struct { + Strict bool +} + +func AddStrictArg(cmd *cobra.Command, so *StrictOptions) { + cmd.Flags().BoolVarP(&so.Strict, "strict", "S", so.Strict, + "If true, require package references to be explicitly noted") +} diff --git a/pkg/commands/publish.go b/pkg/commands/publish.go index 5e32a23d7c..8478c6f686 100644 --- a/pkg/commands/publish.go +++ b/pkg/commands/publish.go @@ -60,7 +60,7 @@ func addPublish(topLevel *cobra.Command) { ko publish --local github.com/foo/bar/cmd/baz github.com/foo/bar/cmd/blah`, Args: cobra.MinimumNArgs(1), Run: func(_ *cobra.Command, args []string) { - builder, err := makeBuilder(do) + builder, err := makeBuilder(do, &options.StrictOptions{}) if err != nil { log.Fatalf("error creating builder: %v", err) } diff --git a/pkg/commands/resolve.go b/pkg/commands/resolve.go index e8b290e2cc..f176c60297 100644 --- a/pkg/commands/resolve.go +++ b/pkg/commands/resolve.go @@ -24,13 +24,13 @@ import ( // addResolve augments our CLI surface with resolve. func addResolve(topLevel *cobra.Command) { - lo := &options.LocalOptions{} no := &options.NameOptions{} fo := &options.FilenameOptions{} ta := &options.TagsOptions{} do := &options.DebugOptions{} so := &options.SelectorOptions{} + sto := &options.StrictOptions{} resolve := &cobra.Command{ Use: "resolve -f FILENAME", @@ -58,7 +58,7 @@ func addResolve(topLevel *cobra.Command) { ko resolve --local -f config/`, Args: cobra.NoArgs, Run: func(cmd *cobra.Command, args []string) { - builder, err := makeBuilder(do) + builder, err := makeBuilder(do, sto) if err != nil { log.Fatalf("error creating builder: %v", err) } @@ -66,7 +66,7 @@ func addResolve(topLevel *cobra.Command) { if err != nil { log.Fatalf("error creating publisher: %v", err) } - resolveFilesToWriter(builder, publisher, fo, so, os.Stdout) + resolveFilesToWriter(builder, publisher, fo, so, sto, os.Stdout) }, } options.AddLocalArg(resolve, lo) @@ -75,5 +75,6 @@ func addResolve(topLevel *cobra.Command) { options.AddTagsArg(resolve, ta) options.AddDebugArg(resolve, do) options.AddSelectorArg(resolve, so) + options.AddStrictArg(resolve, sto) topLevel.AddCommand(resolve) } diff --git a/pkg/commands/resolver.go b/pkg/commands/resolver.go index c8eb98c44d..bcdda66391 100644 --- a/pkg/commands/resolver.go +++ b/pkg/commands/resolver.go @@ -32,7 +32,7 @@ import ( "github.com/mattmoor/dep-notify/pkg/graph" ) -func gobuildOptions(do *options.DebugOptions) ([]build.Option, error) { +func gobuildOptions(do *options.DebugOptions, so *options.StrictOptions) ([]build.Option, error) { creationTime, err := getCreationTime() if err != nil { return nil, err @@ -46,11 +46,14 @@ func gobuildOptions(do *options.DebugOptions) ([]build.Option, error) { if do.DisableOptimizations { opts = append(opts, build.WithDisabledOptimizations()) } + if so.Strict { + opts = append(opts, build.WithStrict()) + } return opts, nil } -func makeBuilder(do *options.DebugOptions) (*build.Caching, error) { - opt, err := gobuildOptions(do) +func makeBuilder(do *options.DebugOptions, so *options.StrictOptions) (*build.Caching, error) { + opt, err := gobuildOptions(do, so) if err != nil { log.Fatalf("error setting up builder options: %v", err) } @@ -113,7 +116,7 @@ func makePublisher(no *options.NameOptions, lo *options.LocalOptions, ta *option // resolvedFuture represents a "future" for the bytes of a resolved file. type resolvedFuture chan []byte -func resolveFilesToWriter(builder *build.Caching, publisher publish.Interface, fo *options.FilenameOptions, so *options.SelectorOptions, out io.WriteCloser) { +func resolveFilesToWriter(builder *build.Caching, publisher publish.Interface, fo *options.FilenameOptions, so *options.SelectorOptions, sto *options.StrictOptions, out io.WriteCloser) { defer out.Close() // By having this as a channel, we can hook this up to a filesystem @@ -194,7 +197,7 @@ func resolveFilesToWriter(builder *build.Caching, publisher publish.Interface, f recordingBuilder := &build.Recorder{ Builder: builder, } - b, err := resolveFile(f, recordingBuilder, publisher, so) + b, err := resolveFile(f, recordingBuilder, publisher, so, sto) if err != nil { // Don't let build errors disrupt the watch. lg := log.Fatalf @@ -239,7 +242,7 @@ func resolveFilesToWriter(builder *build.Caching, publisher publish.Interface, f } } -func resolveFile(f string, builder build.Interface, pub publish.Interface, so *options.SelectorOptions) (b []byte, err error) { +func resolveFile(f string, builder build.Interface, pub publish.Interface, so *options.SelectorOptions, sto *options.StrictOptions) (b []byte, err error) { if f == "-" { b, err = ioutil.ReadAll(os.Stdin) } else { @@ -256,5 +259,5 @@ func resolveFile(f string, builder build.Interface, pub publish.Interface, so *o } } - return resolve.ImageReferences(b, builder, pub) + return resolve.ImageReferences(b, sto.Strict, builder, pub) } diff --git a/pkg/commands/run.go b/pkg/commands/run.go index 64d6564f6b..873e1bb3ef 100644 --- a/pkg/commands/run.go +++ b/pkg/commands/run.go @@ -45,7 +45,7 @@ func addRun(topLevel *cobra.Command) { # This supports relative import paths as well. ko run foo --image=./cmd/baz`, Run: func(cmd *cobra.Command, args []string) { - builder, err := makeBuilder(do) + builder, err := makeBuilder(do, &options.StrictOptions{}) if err != nil { log.Fatalf("error creating builder: %v", err) } diff --git a/pkg/publish/default.go b/pkg/publish/default.go index 8457cd148c..fcea42179d 100644 --- a/pkg/publish/default.go +++ b/pkg/publish/default.go @@ -28,11 +28,11 @@ import ( // defalt is intentionally misspelled to avoid keyword collision (and drive Jon nuts). type defalt struct { - base string - t http.RoundTripper - auth authn.Authenticator - namer Namer - tags []string + base string + t http.RoundTripper + auth authn.Authenticator + namer Namer + tags []string insecure bool } @@ -40,11 +40,11 @@ type defalt struct { type Option func(*defaultOpener) error type defaultOpener struct { - base string - t http.RoundTripper - auth authn.Authenticator - namer Namer - tags []string + base string + t http.RoundTripper + auth authn.Authenticator + namer Namer + tags []string insecure bool } diff --git a/pkg/publish/options.go b/pkg/publish/options.go index ddb3c6979f..5df8c98b22 100644 --- a/pkg/publish/options.go +++ b/pkg/publish/options.go @@ -86,4 +86,4 @@ func Insecure(b bool) Option { i.insecure = b return nil } -} \ No newline at end of file +} diff --git a/pkg/resolve/resolve.go b/pkg/resolve/resolve.go index cdc213b692..8347c56a0c 100644 --- a/pkg/resolve/resolve.go +++ b/pkg/resolve/resolve.go @@ -18,6 +18,7 @@ import ( "bytes" "fmt" "io" + "strings" "sync" "github.com/google/ko/pkg/build" @@ -28,7 +29,7 @@ import ( // ImageReferences resolves supported references to images within the input yaml // to published image digests. -func ImageReferences(input []byte, builder build.Interface, publisher publish.Interface) ([]byte, error) { +func ImageReferences(input []byte, strict bool, builder build.Interface, publisher publish.Interface) ([]byte, error) { // First, walk the input objects and collect a list of supported references refs := make(map[string]struct{}) // The loop is to support multi-document yaml files. @@ -46,7 +47,10 @@ func ImageReferences(input []byte, builder build.Interface, publisher publish.In // This simply returns the replaced object, which we discard during the gathering phase. if _, err := replaceRecursive(obj, func(ref string) (string, error) { if builder.IsSupportedReference(ref) { + ref = strings.TrimPrefix(ref, "ko://") refs[ref] = struct{}{} + } else if strict && strings.HasPrefix(ref, "ko://") { + return "", fmt.Errorf("Strict reference %q is not supported", ref) } return ref, nil }); err != nil { @@ -93,6 +97,7 @@ func ImageReferences(input []byte, builder build.Interface, publisher publish.In if !builder.IsSupportedReference(ref) { return ref, nil } + ref = strings.TrimPrefix(ref, "ko://") if val, ok := sm.Load(ref); ok { return val.(string), nil }