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

Implement "strict mode" which requires ko:// prefix for import paths #58

Merged
merged 5 commits into from
Aug 16, 2019
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions pkg/build/gobuild_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
1 change: 0 additions & 1 deletion pkg/build/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion pkg/commands/apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down
4 changes: 3 additions & 1 deletion pkg/commands/create.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.",
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down
4 changes: 2 additions & 2 deletions pkg/commands/options/local.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
29 changes: 29 additions & 0 deletions pkg/commands/options/strict.go
Original file line number Diff line number Diff line change
@@ -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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm mildly -1 on including -S -- we will eventually run out of flags if we keep this up :P

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I kind of agree. I'll remove -S until someone complains it's too much to type.

"If true, require package references to be explicitly noted")
imjasonh marked this conversation as resolved.
Show resolved Hide resolved
}
5 changes: 3 additions & 2 deletions pkg/commands/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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)
Expand All @@ -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)
}
8 changes: 4 additions & 4 deletions pkg/commands/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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)
}
20 changes: 10 additions & 10 deletions pkg/publish/default.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,23 @@ 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
}

// Option is a functional option for NewDefault.
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
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/publish/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,4 @@ func Insecure(b bool) Option {
i.insecure = b
return nil
}
}
}
14 changes: 11 additions & 3 deletions pkg/resolve/resolve.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"bytes"
"fmt"
"io"
"strings"
"sync"

"github.com/google/ko/pkg/build"
Expand All @@ -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.
Expand All @@ -45,8 +46,14 @@ 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{}{}
if strict && !strings.HasPrefix(ref, "ko://") {
imjasonh marked this conversation as resolved.
Show resolved Hide resolved
return ref, nil
}
tref := strings.TrimPrefix(ref, "ko://")
if builder.IsSupportedReference(tref) {
refs[tref] = struct{}{}
} else if strict && strings.HasPrefix(ref, "ko://") {
return "", fmt.Errorf("Found strict reference %q but %s is not a valid import path", ref, strings.TrimPrefix(ref, "ko://"))
}
return ref, nil
}); err != nil {
Expand Down Expand Up @@ -93,6 +100,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
}
Expand Down
35 changes: 27 additions & 8 deletions pkg/resolve/resolve_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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
Expand All @@ -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)
Expand All @@ -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)
}
Expand Down