Repository: google/ko
Pull request #58 (2019-07-10 12:18:40)

Implement "strict mode" which requires `ko://` prefix for import paths
Demonstrating a strawman implementation and UX for "strict mode", as described in #9

In strict mode, image references are only considered if they begin with the string `ko://`. In loose mode, `ko://` refs are considered as image references and built and replaced during YAML resolution.

If we decide to pursue this model, we could start by making strictness opt-in, cut a versioned release of `ko`, then make `--strict=true` by default (opt-out), then eventually maybe even remove the option.

```
$ cat test.yaml
looseImage: github.com/google/ko/cmd/ko
strictImage: ko://github.com/google/ko/cmd/ko
$ KO_DOCKER_REPO=gcr.io/foo go run ./cmd/ko/ resolve -f test.yaml
...
looseImage: gcr.io/foo/ko-099ba5bcefdead87f92606265fb99ac0@sha256:dbe1a3e11de9e701c9e7a5e61d4f3c2dc93eee2552bb7f6831a23c1147e6899a
strictImage: gcr.io/foo/ko-099ba5bcefdead87f92606265fb99ac0@sha256:dbe1a3e11de9e701c9e7a5e61d4f3c2dc93eee2552bb7f6831a23c1147e6899a

---
$ KO_DOCKER_REPO=gcr.io/foo go run ./cmd/ko/ resolve -f test.yaml -S
...
looseImage: github.com/google/ko/cmd/ko
strictImage: gcr.io/foo/ko-099ba5bcefdead87f92606265fb99ac0@sha256:dbe1a3e11de9e701c9e7a5e61d4f3c2dc93eee2552bb7f6831a23c1147e6899a
---
```

Sending this out to get feedback on the overall idea; it sounds like Go modules is going to be pretty painful without this as an option at least.

ImJasonH: I'd like to push this forward soon, there have been a lot of issues that I think this would solve. The PR isn't ready yet -- it doesn't correctly fail when a `ko://`-prefixed path isn't supported -- but I'll ping when it's ready.
ImJasonH: ```
$ cat test.yaml 
looseImage: github.com/google/ko/cmd/ko          # unresolved in strict mode
strictImage: ko://github.com/google/ko/cmd/ko    # resolved in any mode
badImage: github.com/blargh/whatever             # unresolved in either mode
badStrictImage: ko://github.com/blargh/whatever  # resolved in loose mode; fails in strict mode

# loose mode
$ KO_DOCKER_REPO=gcr.io/my-project go run ./cmd/ko/ resolve -f test.yaml
badImage: github.com/blargh/whatever
badStrictImage: ko://github.com/blargh/whatever
looseImage: gcr.io/my-project/ko-099ba5bcefdead87f92606265fb99ac0@sha256:174089f40c1c0f9f5103c7858a39827ed50b80c6a7b877cf63714b8029b95fcf
strictImage: gcr.io/my-project/ko-099ba5bcefdead87f92606265fb99ac0@sha256:174089f40c1c0f9f5103c7858a39827ed50b80c6a7b877cf63714b8029b95fcf

# strict mode
$ KO_DOCKER_REPO=gcr.io/my-project go run ./cmd/ko/ resolve -S -f test.yaml
2019/08/15 09:42:47 error processing import paths in "test.yaml": Strict reference "ko://github.com/blargh/whatever" is not supported
```

If you remove `badStrictImage` the strict-mode resolved output is:

```
badImage: github.com/blargh/whatever
looseImage: github.com/google/ko/cmd/ko
strictImage: gcr.io/test-argo/ko-099ba5bcefdead87f92606265fb99ac0@sha256:174089f40c1c0f9f5103c7858a39827ed50b80c6a7b877cf63714b8029b95fcf
```

Non-`ko://`-prefixed paths are _ignored_ even if they're suppoorted. Only paths that begin with `ko://` are considered in strict mode.

Patch:
diff --git a/pkg/build/options.go b/pkg/build/options.go
index 63f667ca..4bcee0db 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
diff --git a/pkg/commands/apply.go b/pkg/commands/apply.go
index da76abe7..a03c65fe 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.",
@@ -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 92186c97..56bb0c89 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.",
@@ -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 d6e7fc81..967c8926 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 00000000..bdc528c6
--- /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", "", so.Strict,
+		`If true, require package references to be explicitly prefixed with "ko://"`)
+}
diff --git a/pkg/commands/resolve.go b/pkg/commands/resolve.go
index e8b290e2..82cd92c7 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",
@@ -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 c8eb98c4..f54d616c 100644
--- a/pkg/commands/resolver.go
+++ b/pkg/commands/resolver.go
@@ -113,7 +113,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 +194,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 +239,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 +256,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/publish/default.go b/pkg/publish/default.go
index 8457cd14..fcea4217 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 ddb3c697..5df8c98b 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 cdc213b6..1cb38a09 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.
@@ -45,8 +46,15 @@ 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) {
-				refs[ref] = struct{}{}
+			strictRef := strings.HasPrefix(ref, "ko://")
+			if strict && !strictRef {
+				return ref, nil
+			}
+			tref := strings.TrimPrefix(ref, "ko://")
+			if builder.IsSupportedReference(tref) {
+				refs[tref] = struct{}{}
+			} else if strict && strictRef {
+				return "", fmt.Errorf("Found strict reference %q but %s is not a valid import path", ref, tref)
 			}
 			return ref, nil
 		}); err != nil {
@@ -93,6 +101,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
 			}


Test patch:
diff --git a/pkg/build/gobuild_test.go b/pkg/build/gobuild_test.go
index a80d9279..76b7c02c 100644
--- a/pkg/build/gobuild_test.go
+++ b/pkg/build/gobuild_test.go
@@ -66,14 +66,12 @@ func TestGoBuildIsSupportedRefWithModules(t *testing.T) {
 	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/resolve/resolve_test.go b/pkg/resolve/resolve_test.go
index ab6b04cb..35e7e150 100644
--- a/pkg/resolve/resolve_test.go
+++ b/pkg/resolve/resolve_test.go
@@ -85,7 +85,7 @@ func TestYAMLArrays(t *testing.T) {
 				t.Fatalf("yaml.Marshal(%v) = %v", inputStructured, err)
 			}
 
-			outYAML, err := ImageReferences(inputYAML, testBuilder, newFixedPublish(test.base, testHashes))
+			outYAML, err := ImageReferences(inputYAML, false, testBuilder, newFixedPublish(test.base, testHashes))
 			if err != nil {
 				t.Fatalf("ImageReferences(%v) = %v", string(inputYAML), err)
 			}
@@ -158,7 +158,7 @@ func TestYAMLMaps(t *testing.T) {
 				t.Fatalf("yaml.Marshal(%v) = %v", inputStructured, err)
 			}
 
-			outYAML, err := ImageReferences(inputYAML, testBuilder, newFixedPublish(base, testHashes))
+			outYAML, err := ImageReferences(inputYAML, false, testBuilder, newFixedPublish(base, testHashes))
 			if err != nil {
 				t.Fatalf("ImageReferences(%v) = %v", string(inputYAML), err)
 			}
@@ -226,7 +226,7 @@ func TestYAMLObject(t *testing.T) {
 				t.Fatalf("yaml.Marshal(%v) = %v", inputStructured, err)
 			}
 
-			outYAML, err := ImageReferences(inputYAML, testBuilder, newFixedPublish(base, testHashes))
+			outYAML, err := ImageReferences(inputYAML, false, testBuilder, newFixedPublish(base, testHashes))
 			if err != nil {
 				t.Fatalf("ImageReferences(%v) = %v", string(inputYAML), err)
 			}
@@ -242,8 +242,29 @@ func TestYAMLObject(t *testing.T) {
 	}
 }
 
+func TestStrict(t *testing.T) {
+	refs := []string{
+		"ko://" + fooRef,
+		"ko://" + barRef,
+	}
+	buf := bytes.NewBuffer(nil)
+	encoder := yaml.NewEncoder(buf)
+	for _, input := range refs {
+		if err := encoder.Encode(input); err != nil {
+			t.Fatalf("Encode(%v) = %v", input, err)
+		}
+	}
+	inputYAML := buf.Bytes()
+	base := mustRepository("gcr.io/multi-pass")
+	outYAML, err := ImageReferences(inputYAML, true, testBuilder, newFixedPublish(base, testHashes))
+	if err != nil {
+		t.Fatalf("ImageReferences: %v", err)
+	}
+	t.Log(string(outYAML))
+}
+
 func TestMultiDocumentYAMLs(t *testing.T) {
-	tests := []struct {
+	for _, test := range []struct {
 		desc   string
 		refs   []string
 		hashes []v1.Hash
@@ -253,9 +274,7 @@ func TestMultiDocumentYAMLs(t *testing.T) {
 		refs:   []string{fooRef, barRef},
 		hashes: []v1.Hash{fooHash, barHash},
 		base:   mustRepository("gcr.io/multi-pass"),
-	}}
-
-	for _, test := range tests {
+	}} {
 		t.Run(test.desc, func(t *testing.T) {
 			buf := bytes.NewBuffer(nil)
 			encoder := yaml.NewEncoder(buf)
@@ -266,7 +285,7 @@ func TestMultiDocumentYAMLs(t *testing.T) {
 			}
 			inputYAML := buf.Bytes()
 
-			outYAML, err := ImageReferences(inputYAML, testBuilder, newFixedPublish(test.base, testHashes))
+			outYAML, err := ImageReferences(inputYAML, false, testBuilder, newFixedPublish(test.base, testHashes))
 			if err != nil {
 				t.Fatalf("ImageReferences(%v) = %v", string(inputYAML), err)
 			}