changed default image pull policy to if-not-present (#2600)

Author: droot

e2e/testdata/fn-eval/error-in-pipe/.expected/config.yaml

@@ -21,8 +21,11 @@ stdErr: |
     Stderr:
       "[error] /// : failed to configure function: input namespace cannot be empty"
     Exit code: 1
-
+  
    
   [RUNNING] "gcr.io/kpt-fn/dne"
   [FAIL] "gcr.io/kpt-fn/dne" in 0s
-  Error: Function image "gcr.io/kpt-fn/dne" doesn't exist
+    Stderr:
+      "docker: Error response from daemon: manifest for gcr.io/kpt-fn/dne:latest not found: manifest unknown: Failed to fetch \"latest\" from request \"/v2/kpt-fn/dne/manifests/latest\"."
+      "See 'docker run --help'."
+    Exit code: 125

e2e/testdata/fn-eval/fn-success-with-stderr/pkg/.expected/results.yaml

@@ -5,6 +5,5 @@ metadata:
 exitCode: 0
 items:
   - image: gcr.io/kpt-fn/starlark:v0.2
-    stderr: |
-      function succeeded, reporting it on stderr
+    stderr: function succeeded, reporting it on stderr
     exitCode: 0

e2e/testdata/fn-eval/missing-fn-image/.expected/config.yaml

@@ -17,4 +17,4 @@ exitCode: 1
 image: gcr.io/kpt-fn/dne # non-existing image
 args:
   namespace: staging
-stdErr: 'Function image "gcr.io/kpt-fn/dne" doesn''t exist'
+stdErr: 'gcr.io/kpt-fn/dne:latest not found'

e2e/testdata/fn-render/fn-success-with-stderr/.expected/results.yaml

@@ -5,6 +5,5 @@ metadata:
 exitCode: 0
 items:
   - image: gcr.io/kpt-fn/starlark:v0.2
-    stderr: |
-      function succeeded, reporting it on stderr
+    stderr: function succeeded, reporting it on stderr
     exitCode: 0

e2e/testdata/fn-render/missing-fn-image/.expected/config.yaml

@@ -13,4 +13,4 @@
 # limitations under the License.
 
 exitCode: 1
-stdErr: 'Error: Function image "gcr.io/kpt-fn/dne" doesn''t exist'
+stdErr: 'gcr.io/kpt-fn/dne:latest not found'

internal/cmdrender/cmdrender.go

@@ -45,7 +45,7 @@ func NewRunner(ctx context.Context, parent string) *Runner {
 		"path to a directory to save function results")
 	c.Flags().StringVarP(&r.dest, "output", "o", "",
 		fmt.Sprintf("output resources are written to provided location. Allowed values: %s|%s|<OUT_DIR_PATH>", cmdutil.Stdout, cmdutil.Unwrap))
-	c.Flags().StringVar(&r.imagePullPolicy, "image-pull-policy", string(fnruntime.AlwaysPull),
+	c.Flags().StringVar(&r.imagePullPolicy, "image-pull-policy", string(fnruntime.IfNotPresentPull),
 		fmt.Sprintf("pull image before running the container. It must be one of %s, %s and %s.", fnruntime.AlwaysPull, fnruntime.IfNotPresentPull, fnruntime.NeverPull))
 	cmdutil.FixDocs("kpt", parent, c)
 	r.Command = c

internal/fnruntime/container.go

@@ -15,6 +15,7 @@
 package fnruntime
 
 import (
+	"bufio"
 	"bytes"
 	"context"
 	goerrors "errors"
@@ -34,11 +35,10 @@ import (
 type containerNetworkName string
 
 const (
-	networkNameNone     containerNetworkName = "none"
-	networkNameHost     containerNetworkName = "host"
-	defaultLongTimeout  time.Duration        = 5 * time.Minute
-	defaultShortTimeout time.Duration        = 5 * time.Second
-	dockerBin           string               = "docker"
+	networkNameNone    containerNetworkName = "none"
+	networkNameHost    containerNetworkName = "host"
+	defaultLongTimeout time.Duration        = 5 * time.Minute
+	dockerBin          string               = "docker"
 
 	AlwaysPull       ImagePullPolicy = "Always"
 	IfNotPresentPull ImagePullPolicy = "IfNotPresent"
@@ -85,13 +85,6 @@ type ContainerFn struct {
 // It reads the input from the given reader and writes the output
 // to the provided writer.
 func (f *ContainerFn) Run(reader io.Reader, writer io.Writer) error {
-	// check and pull image before running to avoid polluting CLI
-	// output
-	err := f.prepareImage()
-	if err != nil {
-		return err
-	}
-
 	errSink := bytes.Buffer{}
 	cmd, cancel := f.getDockerCmd()
 	defer cancel()
@@ -105,15 +98,15 @@ func (f *ContainerFn) Run(reader io.Reader, writer io.Writer) error {
 			return &ExecError{
 				OriginalErr:    exitErr,
 				ExitCode:       exitErr.ExitCode(),
-				Stderr:         errSink.String(),
+				Stderr:         filterDockerCLIOutput(&errSink),
 				TruncateOutput: printer.TruncateOutput,
 			}
 		}
 		return fmt.Errorf("unexpected function error: %w", err)
 	}
 
 	if errSink.Len() > 0 {
-		f.FnResult.Stderr = errSink.String()
+		f.FnResult.Stderr = filterDockerCLIOutput(&errSink)
 	}
 	return nil
 }
@@ -135,8 +128,15 @@ func (f *ContainerFn) getDockerCmd() (*exec.Cmd, context.CancelFunc) {
 		"--user", uidgid,
 		"--security-opt=no-new-privileges",
 	}
-	if f.ImagePullPolicy == NeverPull {
+	switch f.ImagePullPolicy {
+	case NeverPull:
 		args = append(args, "--pull", "never")
+	case AlwaysPull:
+		args = append(args, "--pull", "pull")
+	case IfNotPresentPull:
+		args = append(args, "--pull", "missing")
+	default:
+		args = append(args, "--pull", "missing")
 	}
 	for _, storageMount := range f.StorageMounts {
 		args = append(args, "--mount", storageMount.String())
@@ -173,62 +173,6 @@ func NewContainerEnvFromStringSlice(envStr []string) *runtimeutil.ContainerEnv {
 	return ce
 }
 
-// prepareImage will check local images and pull it if it doesn't
-// exist.
-func (f *ContainerFn) prepareImage() error {
-	// If ImagePullPolicy is set to "never", we don't need to do anything here.
-	if f.ImagePullPolicy == NeverPull {
-		return nil
-	}
-
-	// If ImagePullPolicy is set to "ifNotPresent", we scan the local images
-	// first. If there is a match, we just return. This can be useful for local
-	// development to prevent the remote image to accidentally override the
-	// local image when they use the same name and tag.
-	if f.ImagePullPolicy == IfNotPresentPull {
-		if foundInLocalCache := f.checkImageExistence(); foundInLocalCache {
-			return nil
-		}
-	}
-
-	// If ImagePullPolicy is set to always (which is the default), we will try
-	// to pull the image regardless if the tag has been seen in the local cache.
-	// This can help to ensure we have the latest release for "moving tags" like
-	// v1 and v1.2. The performance cost is very minimal, since `docker pull`
-	// checks the SHA first and only pull the missing docker layer(s).
-	args := []string{"image", "pull", f.Image}
-	// setup timeout
-	timeout := defaultLongTimeout
-	if f.Timeout != 0 {
-		timeout = f.Timeout
-	}
-	ctx, cancel := context.WithTimeout(context.Background(), timeout)
-	defer cancel()
-	cmd := exec.CommandContext(ctx, dockerBin, args...)
-	output, err := cmd.CombinedOutput()
-	if err != nil {
-		return &ContainerImageError{
-			Image:  f.Image,
-			Output: string(output),
-		}
-	}
-	return nil
-}
-
-// checkImageExistence returns true if the image does exist in
-// local cache
-func (f *ContainerFn) checkImageExistence() bool {
-	args := []string{"image", "inspect", f.Image}
-	ctx, cancel := context.WithTimeout(context.Background(), defaultShortTimeout)
-	defer cancel()
-	cmd := exec.CommandContext(ctx, dockerBin, args...)
-	if _, err := cmd.CombinedOutput(); err == nil {
-		// image exists locally
-		return true
-	}
-	return false
-}
-
 // AddDefaultImagePathPrefix adds default gcr.io/kpt-fn/ path prefix to image if only image name is specified
 func AddDefaultImagePathPrefix(image string) string {
 	if !strings.Contains(image, "/") {
@@ -250,3 +194,48 @@ func (e *ContainerImageError) Error() string {
 		"Error: Function image %q doesn't exist remotely. If you are developing new functions locally, you can choose to set the image pull policy to ifNotPresent or never.\n%v",
 		e.Image, e.Output)
 }
+
+// filterDockerCLIOutput filters out docker CLI messages
+// from the given buffer.
+func filterDockerCLIOutput(in io.Reader) string {
+	s := bufio.NewScanner(in)
+	var lines []string
+
+	for s.Scan() {
+		txt := s.Text()
+		if !isdockerCLIoutput(txt) {
+			lines = append(lines, txt)
+		}
+	}
+	return strings.Join(lines, "\n")
+}
+
+// isdockerCLIoutput is helper method to determine if
+// the given string is a docker CLI output message.
+// Example docker output:
+//	"Unable to find image 'gcr.io/kpt-fn/starlark:v0.3' locally"
+//  "v0.3: Pulling from kpt-fn/starlark"
+//  "4e9f2cdf4387: Already exists"
+//  "aafbf7df3ddf: Pulling fs layer"
+//  "aafbf7df3ddf: Verifying Checksum"
+//  "aafbf7df3ddf: Download complete"
+//  "6b759ab96cb2: Waiting"
+//  "aafbf7df3ddf: Pull complete"
+//  "Digest: sha256:c347e28606fa1a608e8e02e03541a5a46e4a0152005df4a11e44f6c4ab1edd9a"
+//  "Status: Downloaded newer image for gcr.io/kpt-fn/starlark:v0.3"
+//
+func isdockerCLIoutput(s string) bool {
+	if strings.Contains(s, ": Already exists") ||
+		strings.Contains(s, ": Pulling fs layer") ||
+		strings.Contains(s, ": Verifying Checksum") ||
+		strings.Contains(s, ": Download complete") ||
+		strings.Contains(s, ": Pulling from") ||
+		strings.Contains(s, ": Waiting") ||
+		strings.Contains(s, ": Pull complete") ||
+		strings.Contains(s, "Digest: sha256") ||
+		strings.Contains(s, "Status: Downloaded newer image") ||
+		strings.Contains(s, "Unable to find image") {
+		return true
+	}
+	return false
+}

internal/fnruntime/container_test.go

@@ -24,6 +24,7 @@ import (
 
 	"github.com/GoogleContainerTools/kpt/internal/fnruntime"
 	"github.com/GoogleContainerTools/kpt/internal/printer"
+	fnresult "github.com/GoogleContainerTools/kpt/pkg/api/fnresult/v1"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -54,6 +55,9 @@ func TestContainerFn(t *testing.T) {
 			instance := fnruntime.ContainerFn{
 				Ctx:   printer.WithContext(ctx, printer.New(nil, errBuff)),
 				Image: tt.image,
+				FnResult: &fnresult.Result{
+					Image: tt.image,
+				},
 			}
 			input := bytes.NewBufferString(tt.input)
 			output := &bytes.Buffer{}

internal/fnruntime/fnerrors_test.go

@@ -15,6 +15,7 @@
 package fnruntime
 
 import (
+	"bytes"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -138,3 +139,76 @@ error message`,
 		})
 	}
 }
+
+func TestDockerCLIOutputFilter(t *testing.T) {
+
+	testcases := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name: "should filter docker CLI output successfully",
+			input: `Unable to find image 'gcr.io/kpt-fn/starlark:v0.3' locally
+v0.3: Pulling from kpt-fn/starlark
+4e9f2cdf4387: Already exists
+aafbf7df3ddf: Pulling fs layer
+aafbf7df3ddf: Verifying Checksum
+aafbf7df3ddf: Download complete
+aafbf7df3ddf: Pull complete
+6b759ab96cb2: Waiting
+Digest: sha256:c347e28606fa1a608e8e02e03541a5a46e4a0152005df4a11e44f6c4ab1edd9a
+Status: Downloaded newer image for gcr.io/kpt-fn/starlark:v0.3
+`,
+			expected: "",
+		},
+		{
+			name: "should filter docker messages and shouldn't truncate trailing lines",
+			input: `Unable to find image 'gcr.io/kpt-fn/starlark:v0.3' locally
+v0.3: Pulling from kpt-fn/starlark
+4e9f2cdf4387: Already exists
+aafbf7df3ddf: Pulling fs layer
+aafbf7df3ddf: Verifying Checksum
+aafbf7df3ddf: Download complete
+aafbf7df3ddf: Pull complete
+6b759ab96cb2: Waiting
+Digest: sha256:c347e28606fa1a608e8e02e03541a5a46e4a0152005df4a11e44f6c4ab1edd9a
+Status: Downloaded newer image for gcr.io/kpt-fn/starlark:v0.3
+line before last line
+lastline
+
+`,
+			expected: `line before last line
+lastline
+`,
+		},
+		{
+			name: "should filter interleaved docker messages",
+			input: `firstline
+Unable to find image 'gcr.io/kpt-fn/starlark:v0.3' locally
+v0.3: Pulling from kpt-fn/starlark
+4e9f2cdf4387: Already exists
+aafbf7df3ddf: Pulling fs layer
+aafbf7df3ddf: Verifying Checksum
+line in the middle
+aafbf7df3ddf: Download complete
+aafbf7df3ddf: Pull complete
+6b759ab96cb2: Waiting
+Digest: sha256:c347e28606fa1a608e8e02e03541a5a46e4a0152005df4a11e44f6c4ab1edd9a
+Status: Downloaded newer image for gcr.io/kpt-fn/starlark:v0.3
+lastline
+`,
+			expected: `firstline
+line in the middle
+lastline`,
+		},
+	}
+
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			s := filterDockerCLIOutput(bytes.NewBufferString(tc.input))
+			assert.Equal(t, tc.expected, s)
+		})
+	}
+}

thirdparty/cmdconfig/commands/cmdeval/cmdeval.go

@@ -61,7 +61,7 @@ func GetEvalFnRunner(ctx context.Context, parent string) *EvalFnRunner {
 		"a list of environment variables to be used by functions")
 	r.Command.Flags().BoolVar(
 		&r.AsCurrentUser, "as-current-user", false, "use the uid and gid that kpt is running with to run the function in the container")
-	r.Command.Flags().StringVar(&r.ImagePullPolicy, "image-pull-policy", string(fnruntime.AlwaysPull),
+	r.Command.Flags().StringVar(&r.ImagePullPolicy, "image-pull-policy", string(fnruntime.IfNotPresentPull),
 		fmt.Sprintf("pull image before running the container. It must be one of %s, %s and %s.", fnruntime.AlwaysPull, fnruntime.IfNotPresentPull, fnruntime.NeverPull))
 	r.Command.Flags().StringVar(
 		&r.Selector.APIVersion, "match-api-version", "", "select resources matching the given apiVersion")