Refactor how/where ko:// is handled. (#153)

This change more or less completely changes how `ko://` is handled internally to `ko`, but the user-facing changes should only be net-positive.  `ko://` was previously stripped at the highest level, and the build logic was unaware, which had some undesirable diagnostic/functional implications that are collectively addressed in this change.

With this change, the `ko://` prefix is preserved and passed to the build logic, which internally parses a new `reference` type (this was useful to have Go's type checker find all of the places that needed fixing).  The main functional differences are:
1. If a reference is prefixed with `ko://` we will now fail fast in `IsSupportedReference` regardless of whether `--strict` is passed.
2. If a reference is prefixed with `ko://` it will bypass the prefix check, which allows the use of `ko://github.com/another/repo` that references a vendored binary package.

For `2.` the absence of the module prefix causes the filtering logic Jon introduced to avoid the reference.  This was critical for efficiency when `ko://` isn't around because we feed every string in the yaml through it, but when the user has explicitly decorated things it's the perfect thing to be sensitive to.

Fixes: https://github.com/google/ko/issues/146
Fixes: https://github.com/google/ko/issues/152

pkg/build/gobuild.go

@@ -122,32 +122,38 @@ 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 {
-	p, err := g.importPackage(s)
-	if err != nil {
+	ref := newRef(s)
+	if p, err := g.importPackage(ref); err != nil {
+		if ref.IsStrict() {
+			log.Fatalf("%q is not supported: %v", ref.String(), err)
+		}
+		return false
+	} else if p.IsCommand() {
+		return true
+	} else if ref.IsStrict() {
+		log.Fatalf(`%q does not have "package main"`, ref.String())
+	}
 	return false
 }
-	return p.IsCommand()
-}
-
-var moduleErr = errors.New("unmatched importPackage with gomodules")
 
 // importPackage wraps go/build.Import to handle go modules.
 //
 // Note that we will fall back to GOPATH if the project isn't using go modules.
-func (g *gobuild) importPackage(s string) (*gb.Package, error) {
+func (g *gobuild) importPackage(ref reference) (*gb.Package, error) {
 	if g.mod == nil {
-		return gb.Import(s, gb.Default.GOPATH, gb.ImportComment)
+		return gb.Import(ref.Path(), gb.Default.GOPATH, gb.ImportComment)
 	}
 
 	// If we're inside a go modules project, try to use the module's directory
 	// as our source root to import:
+	// * any strict reference we get
 	// * paths that match module path prefix (they should be in this project)
 	// * relative paths (they should also be in this project)
-	if strings.HasPrefix(s, g.mod.Path) || gb.IsLocalImport(s) {
-		return gb.Import(s, g.mod.Dir, gb.ImportComment)
+	if ref.IsStrict() || strings.HasPrefix(ref.Path(), g.mod.Path) || gb.IsLocalImport(ref.Path()) {
+		return gb.Import(ref.Path(), g.mod.Dir, gb.ImportComment)
 	}
 
-	return nil, moduleErr
+	return nil, fmt.Errorf("unmatched importPackage %q with gomodules", ref.String())
 }
 
 func build(ctx context.Context, ip string, platform v1.Platform, disableOptimizations bool) (string, error) {
@@ -273,8 +279,8 @@ func tarBinary(name, binary string) (*bytes.Buffer, error) {
 	return buf, nil
 }
 
-func (g *gobuild) kodataPath(s string) (string, error) {
-	p, err := g.importPackage(s)
+func (g *gobuild) kodataPath(ref reference) (string, error) {
+	p, err := g.importPackage(ref)
 	if err != nil {
 		return "", err
 	}
@@ -348,7 +354,7 @@ func walkRecursive(tw *tar.Writer, root, chroot string) error {
 	})
 }
 
-func (g *gobuild) tarKoData(importpath string) (*bytes.Buffer, error) {
+func (g *gobuild) tarKoData(ref reference) (*bytes.Buffer, error) {
 	buf := bytes.NewBuffer(nil)
 	// Compress this before calling tarball.LayerFromOpener, since it eagerly
 	// calculates digests and diffids. This prevents us from double compressing
@@ -360,7 +366,7 @@ func (g *gobuild) tarKoData(importpath string) (*bytes.Buffer, error) {
 	tw := tar.NewWriter(gw)
 	defer tw.Close()
 
-	root, err := g.kodataPath(importpath)
+	root, err := g.kodataPath(ref)
 	if err != nil {
 		return nil, err
 	}
@@ -370,8 +376,10 @@ func (g *gobuild) tarKoData(importpath string) (*bytes.Buffer, error) {
 
 // Build implements build.Interface
 func (gb *gobuild) Build(ctx context.Context, s string) (v1.Image, error) {
+	ref := newRef(s)
+
 	// Determine the appropriate base image for this import path.
-	base, err := gb.getBase(s)
+	base, err := gb.getBase(ref.Path())
 	if err != nil {
 		return nil, err
 	}
@@ -385,7 +393,7 @@ func (gb *gobuild) Build(ctx context.Context, s string) (v1.Image, error) {
 	}
 
 	// Do the build into a temporary file.
-	file, err := gb.build(ctx, s, platform, gb.disableOptimizations)
+	file, err := gb.build(ctx, ref.Path(), platform, gb.disableOptimizations)
 	if err != nil {
 		return nil, err
 	}
@@ -393,7 +401,7 @@ func (gb *gobuild) Build(ctx context.Context, s string) (v1.Image, error) {
 
 	var layers []mutate.Addendum
 	// Create a layer from the kodata directory under this import path.
-	dataLayerBuf, err := gb.tarKoData(s)
+	dataLayerBuf, err := gb.tarKoData(ref)
 	if err != nil {
 		return nil, err
 	}
@@ -408,12 +416,12 @@ func (gb *gobuild) Build(ctx context.Context, s string) (v1.Image, error) {
 		Layer: dataLayer,
 		History: v1.History{
 			Author:    "ko",
-			CreatedBy: "ko publish " + s,
+			CreatedBy: "ko publish " + ref.String(),
 			Comment:   "kodata contents, at $KO_DATA_PATH",
 		},
 	})
 
-	appPath := path.Join(appDir, appFilename(s))
+	appPath := path.Join(appDir, appFilename(ref.Path()))
 
 	// Construct a tarball with the binary and produce a layer.
 	binaryLayerBuf, err := tarBinary(appPath, file)
@@ -431,7 +439,7 @@ func (gb *gobuild) Build(ctx context.Context, s string) (v1.Image, error) {
 		Layer: binaryLayer,
 		History: v1.History{
 			Author:    "ko",
-			CreatedBy: "ko publish " + s,
+			CreatedBy: "ko publish " + ref.String(),
 			Comment:   "go build output, at " + appPath,
 		},
 	})

pkg/build/strict.go

@@ -0,0 +1,49 @@
+// Copyright 2020 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 build
+
+import "strings"
+
+// StrictScheme is a prefix that can be placed on import paths that users
+// think MUST be supported references.
+const StrictScheme = "ko://"
+
+type reference struct {
+	strict bool
+	path   string
+}
+
+func newRef(s string) reference {
+	return reference{
+		strict: strings.HasPrefix(s, StrictScheme),
+		path:   strings.TrimPrefix(s, StrictScheme),
+	}
+}
+
+func (r reference) IsStrict() bool {
+	return r.strict
+}
+
+func (r reference) Path() string {
+	return r.path
+}
+
+func (r reference) String() string {
+	if r.IsStrict() {
+		return StrictScheme + r.Path()
+	} else {
+		return r.Path()
+	}
+}

pkg/build/strict_test.go

@@ -0,0 +1,51 @@
+// Copyright 2020 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 build
+
+import "testing"
+
+func TestStrictReference(t *testing.T) {
+	tests := []struct {
+		name   string
+		input  string
+		strict bool
+		path   string
+	}{{
+		name:   "loose",
+		input:  "github.com/foo/bar",
+		strict: false,
+		path:   "github.com/foo/bar",
+	}, {
+		name:   "strict",
+		input:  "ko://github.com/foo/bar",
+		strict: true,
+		path:   "github.com/foo/bar",
+	}}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			ref := newRef(test.input)
+			if got, want := ref.IsStrict(), test.strict; got != want {
+				t.Errorf("got: %v, want: %v", got, want)
+			}
+			if got, want := ref.Path(), test.path; got != want {
+				t.Errorf("got: %v, want: %v", got, want)
+			}
+			if got, want := ref.String(), test.input; got != want {
+				t.Errorf("got: %v, want: %v", got, want)
+			}
+		})
+	}
+}

pkg/internal/testing/fixed.go

@@ -17,6 +17,7 @@ package testing
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
@@ -36,12 +37,14 @@ func NewFixedBuild(entries map[string]v1.Image) build.Interface {
 
 // IsSupportedReference implements build.Interface
 func (f *fixedBuild) IsSupportedReference(s string) bool {
+	s = strings.TrimPrefix(s, build.StrictScheme)
 	_, ok := f.entries[s]
 	return ok
 }
 
 // Build implements build.Interface
 func (f *fixedBuild) Build(_ context.Context, s string) (v1.Image, error) {
+	s = strings.TrimPrefix(s, build.StrictScheme)
 	if img, ok := f.entries[s]; ok {
 		return img, nil
 	}
@@ -61,6 +64,7 @@ func NewFixedPublish(base name.Repository, entries map[string]v1.Hash) publish.I
 
 // Publish implements publish.Interface
 func (f *fixedPublish) Publish(_ v1.Image, s string) (name.Reference, error) {
+	s = strings.TrimPrefix(s, build.StrictScheme)
 	h, ok := f.entries[s]
 	if !ok {
 		return nil, fmt.Errorf("unsupported importpath: %q", s)

pkg/resolve/resolve.go

@@ -27,8 +27,6 @@ import (
 	"gopkg.in/yaml.v3"
 )
 
-const koPrefix = "ko://"
-
 // ImageReferences resolves supported references to images within the input yaml
 // to published image digests.
 //
@@ -42,10 +40,9 @@ func ImageReferences(ctx context.Context, docs []*yaml.Node, strict bool, builde
 
 		for node, ok := it(); ok; node, ok = it() {
 			ref := strings.TrimSpace(node.Value)
-			tref := strings.TrimPrefix(ref, koPrefix)
 
-			if builder.IsSupportedReference(tref) {
-				refs[tref] = append(refs[tref], node)
+			if builder.IsSupportedReference(ref) {
+				refs[ref] = append(refs[ref], node)
 			} else if strict {
 				return fmt.Errorf("found strict reference but %s is not a valid import path", ref)
 			}
@@ -96,7 +93,7 @@ func refsFromDoc(doc *yaml.Node, strict bool) yit.Iterator {
 		Filter(yit.StringValue)
 
 	if strict {
-		return it.Filter(yit.WithPrefix(koPrefix))
+		return it.Filter(yit.WithPrefix(build.StrictScheme))
 	}
 
 	return it

pkg/resolve/resolve_test.go

@@ -25,6 +25,7 @@ import (
 	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/random"
+	"github.com/google/ko/pkg/build"
 	kotesting "github.com/google/ko/pkg/internal/testing"
 	"gopkg.in/yaml.v3"
 )
@@ -249,8 +250,8 @@ func TestYAMLObject(t *testing.T) {
 
 func TestStrict(t *testing.T) {
 	refs := []string{
-		"ko://" + fooRef,
-		"ko://" + barRef,
+		build.StrictScheme + fooRef,
+		build.StrictScheme + barRef,
 	}
 	buf := bytes.NewBuffer(nil)
 	encoder := yaml.NewEncoder(buf)
@@ -270,7 +271,7 @@ func TestStrict(t *testing.T) {
 }
 
 func TestNoStrictKoPrefixRemains(t *testing.T) {
-	ref := "ko://" + fooRef
+	ref := build.StrictScheme + fooRef
 
 	buf := bytes.NewBuffer(nil)
 	encoder := yaml.NewEncoder(buf)