Added pull-policy flag to service (#1644)

* Added pull-policy flag to service

* Added unit tests

* Added test to increase coverage

* Added unit test and made --pull-policy case insensitive

* Added config_changes test cases to increase coverage

* Improve help message for pull-policy flag

* Move image pull policy processing to podspec.go

* Add negative test case for pull policy

docs/cmd/kn_container_add.md

@@ -40,6 +40,7 @@ kn container add NAME
       --limit strings                The resource requirement limits for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource limit, append "-" to the resource name, e.g. '--limit memory-'.
       --mount stringArray            Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume.
   -p, --port string                  The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
+      --pull-policy string           Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent
       --pull-secret string           Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings              The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
       --service-account string       Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.

docs/cmd/kn_service_apply.md

@@ -56,6 +56,7 @@ kn service apply s0 --filename my-svc.yml
       --no-lock-to-digest                 Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
       --no-wait                           Do not wait for 'service apply' operation to be completed.
   -p, --port string                       The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
+      --pull-policy string                Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent
       --pull-secret string                Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings                   The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
       --revision-name string              The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants (e.g. {{.Service}}-{{.Random 5}}-{{.Generation}})

docs/cmd/kn_service_create.md

@@ -81,6 +81,7 @@ kn service create NAME --image IMAGE
       --no-lock-to-digest                 Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
       --no-wait                           Do not wait for 'service create' operation to be completed.
   -p, --port string                       The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
+      --pull-policy string                Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent
       --pull-secret string                Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings                   The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
       --revision-name string              The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants (e.g. {{.Service}}-{{.Random 5}}-{{.Generation}})

docs/cmd/kn_service_update.md

@@ -68,6 +68,7 @@ kn service update NAME
       --no-lock-to-digest                 Do not keep the running image for the service constant when not explicitly specifying the image. (--no-lock-to-digest pulls the image tag afresh with each new revision)
       --no-wait                           Do not wait for 'service update' operation to be completed.
   -p, --port string                       The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
+      --pull-policy string                Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent
       --pull-secret string                Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings                   The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
       --revision-name string              The revision name to set. Must start with the service name and a dash as a prefix. Empty revision name will result in the server generating a name for the revision. Accepts golang templates, allowing {{.Service}} for the service name, {{.Generation}} for the generation, and {{.Random [n]}} for n random consonants (e.g. {{.Service}}-{{.Random 5}}-{{.Generation}})

docs/cmd/kn_source_container_create.md

@@ -30,6 +30,7 @@ kn source container create NAME --image IMAGE --sink SINK
       --mount stringArray            Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume.
   -n, --namespace string             Specify the namespace to operate in.
   -p, --port string                  The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
+      --pull-policy string           Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent
       --pull-secret string           Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings              The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
       --service-account string       Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.

docs/cmd/kn_source_container_update.md

@@ -30,6 +30,7 @@ kn source container update NAME --image IMAGE
       --mount stringArray            Mount a ConfigMap (prefix cm: or config-map:), a Secret (prefix secret: or sc:), or an existing Volume (without any prefix) on the specified directory. Example: --mount /mydir=cm:myconfigmap, --mount /mydir=secret:mysecret, or --mount /mydir=myvolume. When a configmap or a secret is specified, a corresponding volume is automatically generated. You can use this flag multiple times. For unmounting a directory, append "-", e.g. --mount /mydir-, which also removes any auto-generated volume.
   -n, --namespace string             Specify the namespace to operate in.
   -p, --port string                  The port where application listens on, in the format 'NAME:PORT', where 'NAME' is optional. Examples: '--port h2c:8080' , '--port 8080'.
+      --pull-policy string           Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent
       --pull-secret string           Image pull secret to set. An empty argument ("") clears the pull secret. The referenced secret must exist in the service's namespace.
       --request strings              The resource requirement requests for this Service. For example, 'cpu=100m,memory=256Mi'. You can use this flag multiple times. To unset a resource request, append "-" to the resource name, e.g. '--request cpu-'.
       --service-account string       Service account name to set. An empty argument ("") clears the service account. The referenced service account must exist in the service's namespace.

pkg/kn/commands/service/configuration_edit_flags_test.go

@@ -0,0 +1,49 @@
+// Copyright © 2022 The Knative Authors
+//
+// 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 service
+
+import (
+	"testing"
+
+	"gotest.tools/v3/assert"
+	"knative.dev/client/pkg/kn/commands"
+	"knative.dev/client/pkg/util"
+)
+
+func TestApplyPullPolicyFlag(t *testing.T) {
+	var editFlags ConfigurationEditFlags
+	knParams := &commands.KnParams{}
+	cmd, _, _ := commands.CreateTestKnCommand(NewServiceCreateCommand(knParams), knParams)
+
+	editFlags.AddCreateFlags(cmd)
+	svc := createTestService("test-svc", []string{"test-svc-00001", "test-svc-00002"}, goodConditions())
+	cmd.SetArgs([]string{"--pull-policy", "Always"})
+	cmd.Execute()
+	err := editFlags.Apply(&svc, nil, cmd)
+	assert.NilError(t, err)
+}
+
+func TestApplyPullPolicyFlagError(t *testing.T) {
+	var editFlags ConfigurationEditFlags
+	knParams := &commands.KnParams{}
+	cmd, _, _ := commands.CreateTestKnCommand(NewServiceCreateCommand(knParams), knParams)
+
+	editFlags.AddCreateFlags(cmd)
+	svc := createTestService("test-svc", []string{"test-svc-00001", "test-svc-00002"}, goodConditions())
+	cmd.SetArgs([]string{"--pull-policy", "InvalidPolicy"})
+	cmd.Execute()
+	err := editFlags.Apply(&svc, nil, cmd)
+	assert.Assert(t, util.ContainsAll(err.Error(), "invalid", "InvalidPolicy", "Valid arguments (case insensitive): Always | Never | IfNotPresent"))
+}

pkg/kn/flags/podspec.go

@@ -29,6 +29,7 @@ import (
 type PodSpecFlags struct {
 	// Direct field manipulation
 	Image           uniqueStringArg
+	ImagePullPolicy string
 	Env             []string
 	EnvFrom         []string
 	EnvValueFrom    []string
@@ -129,6 +130,9 @@ func (p *PodSpecFlags) AddFlags(flagset *pflag.FlagSet) []string {
 	flagset.VarP(&p.Image, "image", "", "Image to run.")
 	flagNames = append(flagNames, "image")
 
+	flagset.StringVar(&p.ImagePullPolicy, "pull-policy", "",
+		"Image pull policy. Valid values (case insensitive): Always | Never | IfNotPresent")
+
 	flagset.StringVarP(&p.EnvFile, "env-file", "", "", "Path to a file containing environment variables (e.g. --env-file=/home/knative/service1/env).")
 	flagNames = append(flagNames, "env-file")
 
@@ -285,6 +289,14 @@ func (p *PodSpecFlags) ResolvePodSpec(podSpec *corev1.PodSpec, flags *pflag.Flag
 		}
 	}
 
+	if flags.Changed("pull-policy") {
+
+		err = UpdateImagePullPolicy(podSpec, p.ImagePullPolicy)
+		if err != nil {
+			return err
+		}
+	}
+
 	requestsToRemove, limitsToRemove, err := p.Resources.Validate()
 	if err != nil {
 		return err

pkg/kn/flags/podspec_helper.go

@@ -23,6 +23,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/yaml"
 
 	corev1 "k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 	"knative.dev/client/pkg/util"
 )
@@ -301,6 +302,35 @@ func UpdateContainers(spec *corev1.PodSpec, containers []corev1.Container) {
 	}
 }
 
+// UpdateImagePullPolicy updates the pull policy for the given revision template
+func UpdateImagePullPolicy(spec *corev1.PodSpec, imagePullPolicy string) error {
+	container := containerOfPodSpec(spec)
+
+	if !isValidPullPolicy(imagePullPolicy) {
+		return fmt.Errorf("invalid --pull-policy %s. Valid arguments (case insensitive): Always | Never | IfNotPresent", imagePullPolicy)
+	}
+	container.ImagePullPolicy = getPolicy(imagePullPolicy)
+	return nil
+}
+
+func getPolicy(policy string) v1.PullPolicy {
+	var ret v1.PullPolicy
+	switch strings.ToLower(policy) {
+	case "always":
+		ret = v1.PullAlways
+	case "ifnotpresent":
+		ret = v1.PullIfNotPresent
+	case "never":
+		ret = v1.PullNever
+	}
+	return ret
+}
+
+func isValidPullPolicy(policy string) bool {
+	validPolicies := []string{string(v1.PullAlways), string(v1.PullNever), string(v1.PullIfNotPresent)}
+	return util.SliceContainsIgnoreCase(validPolicies, policy)
+}
+
 // =======================================================================================
 func updateEnvVarsFromMap(env []corev1.EnvVar, toUpdate *util.OrderedMap) []corev1.EnvVar {
 	updated := sets.NewString()

pkg/kn/flags/podspec_helper_test.go

@@ -892,3 +892,66 @@ containers:
 	assert.Assert(t, err != nil)
 	assert.Assert(t, util.ContainsAll(err.Error(), "no", "file", "directory"))
 }
+
+func TestUpdateImagePullPolicy(t *testing.T) {
+	policyMap := make(map[string][]string)
+	policyMap["Always"] = []string{"always", "ALWAYS", "Always"}
+	policyMap["Never"] = []string{"never", "NEVER", "Never"}
+	policyMap["IfNotPresent"] = []string{"ifnotpresent", "IFNOTPRESENT", "IfNotPresent"}
+
+	for k, values := range policyMap {
+		expectedPodSpec := &corev1.PodSpec{
+			Containers: []corev1.Container{
+				{
+					Image:           "repo/user/imageID:tag",
+					ImagePullPolicy: corev1.PullPolicy(k),
+					Command:         []string{"/app/start"},
+					Args:            []string{"myArg1"},
+					Ports: []corev1.ContainerPort{
+						{
+							ContainerPort: 8080,
+						},
+					},
+				},
+			},
+		}
+		for _, v := range values {
+			podSpec := &corev1.PodSpec{
+				Containers: []corev1.Container{
+					{
+						Image:   "repo/user/imageID:tag",
+						Command: []string{"/app/start"},
+						Args:    []string{"myArg1"},
+						Ports: []corev1.ContainerPort{
+							{
+								ContainerPort: 8080,
+							},
+						},
+					},
+				},
+			}
+			err := UpdateImagePullPolicy(podSpec, v)
+			assert.NilError(t, err, "update pull policy failed")
+			assert.DeepEqual(t, expectedPodSpec, podSpec)
+		}
+	}
+}
+
+func TestUpdateImagePullPolicyError(t *testing.T) {
+	podSpec := &corev1.PodSpec{
+		Containers: []corev1.Container{
+			{
+				Image:   "repo/user/imageID:tag",
+				Command: []string{"/app/start"},
+				Args:    []string{"myArg1"},
+				Ports: []corev1.ContainerPort{
+					{
+						ContainerPort: 8080,
+					},
+				},
+			},
+		},
+	}
+	err := UpdateImagePullPolicy(podSpec, "InvalidPolicy")
+	assert.Assert(t, util.ContainsAll(err.Error(), "invalid --pull-policy", "Valid arguments", "Always | Never | IfNotPresent"))
+}

pkg/kn/flags/podspec_test.go

@@ -70,11 +70,12 @@ func TestPodSpecResolve(t *testing.T) {
 		"--port", "8080", "--limit", "cpu=1000m", "--limit", "memory=1024Mi",
 		"--cmd", "/app/start", "--arg", "myArg1", "--service-account", "foo-bar-account",
 		"--mount", "/mount/path=volume-name", "--volume", "volume-name=cm:config-map-name",
-		"--env-from", "config-map:config-map-name", "--user", "1001"}
+		"--env-from", "config-map:config-map-name", "--user", "1001", "--pull-policy", "always"}
 	expectedPodSpec := corev1.PodSpec{
 		Containers: []corev1.Container{
 			{
 				Image:           "repo/user/imageID:tag",
+				ImagePullPolicy: "Always",
 				Command:         []string{"/app/start"},
 				Args:            []string{"myArg1"},
 				Ports: []corev1.ContainerPort{
@@ -291,6 +292,28 @@ func TestPodSpecResolveReturnError(t *testing.T) {
 	assert.Assert(t, util.ContainsAll(out, "Invalid", "mount"))
 }
 
+func TestPodSpecResolveReturnErrorPullPolicy(t *testing.T) {
+	outBuf := bytes.Buffer{}
+	flags := &PodSpecFlags{}
+	inputArgs := []string{"--pull-policy", "invalidPolicy"}
+	testCmd := &cobra.Command{
+		Use: "test",
+		Run: func(cmd *cobra.Command, args []string) {
+			podSpec := &corev1.PodSpec{Containers: []corev1.Container{{}}}
+			err := flags.ResolvePodSpec(podSpec, cmd.Flags(), inputArgs)
+			fmt.Fprint(cmd.OutOrStdout(), "Return error: ", err)
+		},
+	}
+	testCmd.SetOut(&outBuf)
+
+	testCmd.SetArgs(inputArgs)
+	flags.AddFlags(testCmd.Flags())
+	flags.AddCreateFlags(testCmd.Flags())
+	testCmd.Execute()
+	out := outBuf.String()
+	assert.Assert(t, util.ContainsAll(out, "invalid --pull-policy", "Always | Never | IfNotPresent"))
+}
+
 func TestPodSpecResolveWithEnvFile(t *testing.T) {
 	file, err := ioutil.TempFile("", "envfile.env")
 	assert.NilError(t, err)

pkg/serving/config_changes.go

@@ -22,12 +22,11 @@ import (
 	"strings"
 	"time"
 
+	"knative.dev/client/pkg/kn/flags"
 	"knative.dev/pkg/ptr"
 	"knative.dev/serving/pkg/apis/autoscaling"
 	servingconfig "knative.dev/serving/pkg/apis/config"
 	servingv1 "knative.dev/serving/pkg/apis/serving/v1"
-
-	"knative.dev/client/pkg/kn/flags"
 )
 
 // VolumeSourceType is a type standing for enumeration of ConfigMap and Secret

pkg/serving/config_changes_test.go

@@ -151,6 +151,41 @@ func TestPinImageToDigestNilContainerStatus(t *testing.T) {
 	assert.NilError(t, err)
 }
 
+func TestPinImageToDigestNilContainerSpec(t *testing.T) {
+	template, _ := getRevisionTemplate()
+	template.Spec.Containers = nil
+	revision := &servingv1.Revision{}
+	revision.Spec = template.Spec
+	revision.ObjectMeta = template.ObjectMeta
+	revision.Status.ContainerStatuses = nil
+	err := PinImageToDigest(template, revision)
+	assert.ErrorContains(t, err, "no container given in current revision")
+}
+
+func TestPinImageToDigestNilBaseRevisionContainerSpec(t *testing.T) {
+	template, _ := getRevisionTemplate()
+	revision := &servingv1.Revision{}
+	revision.Spec = template.Spec
+	revision.ObjectMeta = template.ObjectMeta
+	revision.Status.ContainerStatuses = nil
+
+	revision.Spec.Containers = nil
+	err := PinImageToDigest(template, revision)
+	assert.ErrorContains(t, err, "no container found in base revision")
+}
+
+func TestPinImageToDigestImageMismatch(t *testing.T) {
+	template, _ := getRevisionTemplate()
+	revision := &servingv1.Revision{}
+	revision.Spec = template.Spec
+	revision.ObjectMeta = template.ObjectMeta
+	revision.Status.ContainerStatuses = nil
+
+	revision.Spec.Containers = []corev1.Container{{Image: "mock-image"}}
+	err := PinImageToDigest(template, revision)
+	assert.ErrorContains(t, err, "contains unexpected image")
+}
+
 func TestUpdateTimestampAnnotation(t *testing.T) {
 	template, _ := getRevisionTemplate()
 	UpdateTimestampAnnotation(template)