From d2960114309fdcfa9d46f5e9577e8101510647e0 Mon Sep 17 00:00:00 2001 From: Matt Moore Date: Wed, 29 Apr 2020 16:03:19 -0700 Subject: [PATCH] Refactor how/where ko:// is handled. 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 | 48 +++++++++++++++++++-------------- pkg/build/strict.go | 49 +++++++++++++++++++++++++++++++++ pkg/build/strict_test.go | 51 +++++++++++++++++++++++++++++++++++ pkg/internal/testing/fixed.go | 4 +++ pkg/resolve/resolve.go | 9 +++---- pkg/resolve/resolve_test.go | 7 ++--- 6 files changed, 139 insertions(+), 29 deletions(-) create mode 100644 pkg/build/strict.go create mode 100644 pkg/build/strict_test.go diff --git a/pkg/build/gobuild.go b/pkg/build/gobuild.go index 38b88fb8ea..ca12ca0ae8 100644 --- a/pkg/build/gobuild.go +++ b/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 p.IsCommand() + return false } -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, }, }) diff --git a/pkg/build/strict.go b/pkg/build/strict.go new file mode 100644 index 0000000000..331daa3a85 --- /dev/null +++ b/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() + } +} diff --git a/pkg/build/strict_test.go b/pkg/build/strict_test.go new file mode 100644 index 0000000000..69b01926c2 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/pkg/internal/testing/fixed.go b/pkg/internal/testing/fixed.go index 472b0bb949..18f13bb7ea 100644 --- a/pkg/internal/testing/fixed.go +++ b/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) diff --git a/pkg/resolve/resolve.go b/pkg/resolve/resolve.go index 8d7387668e..20c5e04ecc 100644 --- a/pkg/resolve/resolve.go +++ b/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 diff --git a/pkg/resolve/resolve_test.go b/pkg/resolve/resolve_test.go index ba6fe9e6a7..cbf701496d 100644 --- a/pkg/resolve/resolve_test.go +++ b/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)