Validate scale and container concurrency options when updating configuration resource (#279)

* Validate invalid container concurrency options

* Use assert package

* Use FlagSet.Changed and don't care about default values

* Update dependency

* Follow e2e test changes

* Return error if invalid value is specified by users

* Fix broken e2e test

* Add more unit tests

* Fix error message

* Fix comment statement

* Revert back unrelated changes

* Fix typo

pkg/kn/commands/service/configuration_edit_flags.go

@@ -111,7 +111,33 @@ func (p *ConfigurationEditFlags) Apply(service *servingv1alpha1.Service, cmd *co
 		}
 	}
 
-	servinglib.UpdateConcurrencyConfiguration(template, p.MinScale, p.MaxScale, p.ConcurrencyTarget, p.ConcurrencyLimit)
+	if cmd.Flags().Changed("min-scale") {
+		err = servinglib.UpdateMinScale(template, p.MinScale)
+		if err != nil {
+			return err
+		}
+	}
+
+	if cmd.Flags().Changed("max-scale") {
+		err = servinglib.UpdateMaxScale(template, p.MaxScale)
+		if err != nil {
+			return err
+		}
+	}
+
+	if cmd.Flags().Changed("concurrency-target") {
+		err = servinglib.UpdateConcurrencyTarget(template, p.ConcurrencyTarget)
+		if err != nil {
+			return err
+		}
+	}
+
+	if cmd.Flags().Changed("concurrency-limit") {
+		err = servinglib.UpdateConcurrencyLimit(template, p.ConcurrencyLimit)
+		if err != nil {
+			return err
+		}
+	}
 
 	return nil
 }

pkg/serving/config_changes.go

@@ -15,15 +15,17 @@
 package serving
 
 import (
+	"context"
 	"fmt"
 	"strconv"
 
+	"github.com/knative/serving/pkg/apis/autoscaling"
 	servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	servingv1beta1 "github.com/knative/serving/pkg/apis/serving/v1beta1"
 	corev1 "k8s.io/api/core/v1"
 )
 
-// Give the configuration all the env var values listed in the given map of
+// UpdateEnvVars gives the configuration all the env var values listed in the given map of
 // vars.  Does not touch any environment variables not mentioned, but it can add
 // new env vars and change the values of existing ones.
 func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, vars map[string]string) error {
@@ -35,34 +37,61 @@ func UpdateEnvVars(template *servingv1alpha1.RevisionTemplateSpec, vars map[stri
 	return nil
 }
 
-// Update min and max scale annotation if larger than 0
+// UpdateMinScale updates min scale annotation
-func UpdateConcurrencyConfiguration(template *servingv1alpha1.RevisionTemplateSpec, minScale int, maxScale int, target int, limit int) {
+func UpdateMinScale(template *servingv1alpha1.RevisionTemplateSpec, min int) error {
-	if minScale != 0 {
+	return UpdateAnnotation(template, autoscaling.MinScaleAnnotationKey, strconv.Itoa(min))
-		UpdateAnnotation(template, "autoscaling.knative.dev/minScale", strconv.Itoa(minScale))
-	}
-	if maxScale != 0 {
-		UpdateAnnotation(template, "autoscaling.knative.dev/maxScale", strconv.Itoa(maxScale))
-	}
-	if target != 0 {
-		UpdateAnnotation(template, "autoscaling.knative.dev/target", strconv.Itoa(target))
-	}
-
-	if limit != 0 {
-		template.Spec.ContainerConcurrency = servingv1beta1.RevisionContainerConcurrencyType(limit)
-	}
 }
 
-// Updater (or add) an annotation to the given service
+// UpdatMaxScale updates max scale annotation
-func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation string, value string) {
+func UpdateMaxScale(template *servingv1alpha1.RevisionTemplateSpec, max int) error {
+	return UpdateAnnotation(template, autoscaling.MaxScaleAnnotationKey, strconv.Itoa(max))
+}
+
+// UpdateConcurrencyTarget updates container concurrency annotation
+func UpdateConcurrencyTarget(template *servingv1alpha1.RevisionTemplateSpec, target int) error {
+	// TODO(toVersus): Remove the following validation once serving library is updated to v0.8.0
+	// and just rely on ValidateAnnotations method.
+	if target < autoscaling.TargetMin {
+		return fmt.Errorf("Invalid 'concurrency-target' value: must be an integer greater than 0: %s",
+			autoscaling.TargetAnnotationKey)
+	}
+
+	return UpdateAnnotation(template, autoscaling.TargetAnnotationKey, strconv.Itoa(target))
+}
+
+// UpdateConcurrencyLimit updates container concurrency limit
+func UpdateConcurrencyLimit(template *servingv1alpha1.RevisionTemplateSpec, limit int) error {
+	cc := servingv1beta1.RevisionContainerConcurrencyType(limit)
+	// Validate input limit
+	ctx := context.Background()
+	if err := cc.Validate(ctx).ViaField("spec.containerConcurrency"); err != nil {
+		return fmt.Errorf("Invalid 'concurrency-limit' value: %s", err)
+	}
+	template.Spec.ContainerConcurrency = cc
+	return nil
+}
+
+// UpdateAnnotation updates (or adds) an annotation to the given service
+func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation string, value string) error {
 	annoMap := template.Annotations
 	if annoMap == nil {
 		annoMap = make(map[string]string)
 		template.Annotations = annoMap
 	}
+
+	// Validate autoscaling annotations and returns error if invalid input provided
+	// without changing the existing spec
+	in := make(map[string]string)
+	in[annotation] = value
+	if err := autoscaling.ValidateAnnotations(in); err != nil {
+		return err
+	}
+
 	annoMap[annotation] = value
+	return nil
 }
 
-// Utility function to translate between the API list form of env vars, and the
+// EnvToMap is an utility function to translate between the API list form of env vars, and the
 // more convenient map form.
 func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) {
 	result := map[string]string{}
@@ -76,7 +105,7 @@ func EnvToMap(vars []corev1.EnvVar) (map[string]string, error) {
 	return result, nil
 }
 
-// Update a given image
+// UpdateImage a given image
 func UpdateImage(template *servingv1alpha1.RevisionTemplateSpec, image string) error {
 	container, err := ContainerOfRevisionTemplate(template)
 	if err != nil {
@@ -98,6 +127,7 @@ func UpdateContainerPort(template *servingv1alpha1.RevisionTemplateSpec, port in
 	return nil
 }
 
+// UpdateResources updates resources as requested
 func UpdateResources(template *servingv1alpha1.RevisionTemplateSpec, requestsResourceList corev1.ResourceList, limitsResourceList corev1.ResourceList) error {
 	container, err := ContainerOfRevisionTemplate(template)
 	if err != nil {

pkg/serving/config_changes_test.go

@@ -16,8 +16,12 @@ package serving
 
 import (
 	"reflect"
+	"strconv"
 	"testing"
 
+	"gotest.tools/assert"
+
+	"github.com/knative/serving/pkg/apis/autoscaling"
 	"github.com/knative/serving/pkg/apis/serving/v1beta1"
 
 	servingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1"
@@ -26,15 +30,35 @@ import (
 
 func TestUpdateAutoscalingAnnotations(t *testing.T) {
 	template := &servingv1alpha1.RevisionTemplateSpec{}
-	UpdateConcurrencyConfiguration(template, 10, 100, 1000, 1000)
+	updateConcurrencyConfiguration(template, 10, 100, 1000, 1000)
 	annos := template.Annotations
-	if annos["autoscaling.knative.dev/minScale"] != "10" {
+	if annos[autoscaling.MinScaleAnnotationKey] != "10" {
 		t.Error("minScale failed")
 	}
-	if annos["autoscaling.knative.dev/maxScale"] != "100" {
+	if annos[autoscaling.MaxScaleAnnotationKey] != "100" {
 		t.Error("maxScale failed")
 	}
-	if annos["autoscaling.knative.dev/target"] != "1000" {
+	if annos[autoscaling.TargetAnnotationKey] != "1000" {
+		t.Error("target failed")
+	}
+	if template.Spec.ContainerConcurrency != 1000 {
+		t.Error("limit failed")
+	}
+}
+
+func TestUpdateInvalidAutoscalingAnnotations(t *testing.T) {
+	template := &servingv1alpha1.RevisionTemplateSpec{}
+	updateConcurrencyConfiguration(template, 10, 100, 1000, 1000)
+	// Update with invalid concurrency options
+	updateConcurrencyConfiguration(template, -1, -1, 0, -1)
+	annos := template.Annotations
+	if annos[autoscaling.MinScaleAnnotationKey] != "10" {
+		t.Error("minScale failed")
+	}
+	if annos[autoscaling.MaxScaleAnnotationKey] != "100" {
+		t.Error("maxScale failed")
+	}
+	if annos[autoscaling.TargetAnnotationKey] != "1000" {
 		t.Error("target failed")
 	}
 	if template.Spec.ContainerConcurrency != 1000 {
@@ -58,13 +82,9 @@ func testUpdateEnvVarsNew(t *testing.T, template *servingv1alpha1.RevisionTempla
 		"b": "bar",
 	}
 	err := UpdateEnvVars(template, env)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
 	found, err := EnvToMap(container.Env)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
 	if !reflect.DeepEqual(env, found) {
 		t.Fatalf("Env did not match expected %v found %v", env, found)
 	}
@@ -88,9 +108,7 @@ func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.Revision
 		"b": "bar",
 	}
 	err := UpdateEnvVars(template, env)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
 
 	expected := map[string]string{
 		"a": "foo",
@@ -98,9 +116,7 @@ func testUpdateEnvVarsAppendOld(t *testing.T, template *servingv1alpha1.Revision
 	}
 
 	found, err := EnvToMap(container.Env)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
 	if !reflect.DeepEqual(expected, found) {
 		t.Fatalf("Env did not match expected %v, found %v", env, found)
 	}
@@ -123,43 +139,99 @@ func testUpdateEnvVarsModify(t *testing.T, revision *servingv1alpha1.RevisionTem
 		"a": "fancy",
 	}
 	err := UpdateEnvVars(revision, env)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
 
 	expected := map[string]string{
 		"a": "fancy",
 	}
 
 	found, err := EnvToMap(container.Env)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
 	if !reflect.DeepEqual(expected, found) {
 		t.Fatalf("Env did not match expected %v, found %v", env, found)
 	}
 }
 
+func TestUpdateMinScale(t *testing.T) {
+	template, _ := getV1alpha1RevisionTemplateWithOldFields()
+	err := UpdateMinScale(template, 10)
+	assert.NilError(t, err)
+	// Verify update is successful or not
+	checkAnnotationValue(t, template, autoscaling.MinScaleAnnotationKey, 10)
+	// Update with invalid value
+	err = UpdateMinScale(template, -1)
+	assert.ErrorContains(t, err, "Invalid")
+}
+
+func TestUpdateMaxScale(t *testing.T) {
+	template, _ := getV1alpha1RevisionTemplateWithOldFields()
+	err := UpdateMaxScale(template, 10)
+	assert.NilError(t, err)
+	// Verify update is successful or not
+	checkAnnotationValue(t, template, autoscaling.MaxScaleAnnotationKey, 10)
+	// Update with invalid value
+	err = UpdateMaxScale(template, -1)
+	assert.ErrorContains(t, err, "Invalid")
+}
+
+func TestUpdateConcurrencyTarget(t *testing.T) {
+	template, _ := getV1alpha1RevisionTemplateWithOldFields()
+	err := UpdateConcurrencyTarget(template, 10)
+	assert.NilError(t, err)
+	// Verify update is successful or not
+	checkAnnotationValue(t, template, autoscaling.TargetAnnotationKey, 10)
+	// Update with invalid value
+	err = UpdateConcurrencyTarget(template, -1)
+	assert.ErrorContains(t, err, "Invalid")
+}
+
+func TestUpdateConcurrencyLimit(t *testing.T) {
+	template, _ := getV1alpha1RevisionTemplateWithOldFields()
+	err := UpdateConcurrencyLimit(template, 10)
+	assert.NilError(t, err)
+	// Verify update is successful or not
+	checkContainerConcurrency(t, template, 10)
+	// Update with invalid value
+	err = UpdateConcurrencyLimit(template, -1)
+	assert.ErrorContains(t, err, "Invalid")
+}
+
+func TestUpdateContainerImage(t *testing.T) {
+	template, _ := getV1alpha1RevisionTemplateWithOldFields()
+	err := UpdateImage(template, "gcr.io/foo/bar:baz")
+	assert.NilError(t, err)
+	// Verify update is successful or not
+	checkContainerImage(t, template, "gcr.io/foo/bar:baz")
+	// Update template with container image info
+	template.Spec.GetContainer().Image = "docker.io/foo/bar:baz"
+	err = UpdateImage(template, "query.io/foo/bar:baz")
+	assert.NilError(t, err)
+	// Verify that given image overrides the existing container image
+	checkContainerImage(t, template, "query.io/foo/bar:baz")
+}
+
+func checkContainerImage(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, image string) {
+	if got, want := template.Spec.GetContainer().Image, image; got != want {
+		t.Errorf("Failed to update the container image: got=%s, want=%s", got, want)
+	}
+}
+
 func TestUpdateContainerPort(t *testing.T) {
 	template, _ := getV1alpha1Config()
 	err := UpdateContainerPort(template, 8888)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
 	// Verify update is successful or not
 	checkPortUpdate(t, template, 8888)
 	// update template with container port info
 	template.Spec.Containers[0].Ports[0].ContainerPort = 9090
 	err = UpdateContainerPort(template, 80)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
 	// Verify that given port overrides the existing container port
 	checkPortUpdate(t, template, 80)
 }
 
 func checkPortUpdate(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, port int32) {
-	if len(template.Spec.Containers) != 1 || template.Spec.Containers[0].Ports[0].ContainerPort != port {
+	if template.Spec.GetContainer().Ports[0].ContainerPort != port {
 		t.Error("Failed to update the container port")
 	}
 }
@@ -183,9 +255,7 @@ func testUpdateEnvVarsBoth(t *testing.T, template *servingv1alpha1.RevisionTempl
 		"b": "boo",
 	}
 	err := UpdateEnvVars(template, env)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
 
 	expected := map[string]string{
 		"a": "fancy",
@@ -194,9 +264,7 @@ func testUpdateEnvVarsBoth(t *testing.T, template *servingv1alpha1.RevisionTempl
 	}
 
 	found, err := EnvToMap(container.Env)
-	if err != nil {
+	assert.NilError(t, err)
-		t.Fatal(err)
-	}
 	if !reflect.DeepEqual(expected, found) {
 		t.Fatalf("Env did not match expected %v, found %v", env, found)
 	}
@@ -239,3 +307,23 @@ func assertNoV1alpha1(t *testing.T, template *servingv1alpha1.RevisionTemplateSp
 		t.Error("Assuming only old v1alpha1 fields but found spec.template")
 	}
 }
+
+func checkAnnotationValue(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, key string, value int) {
+	anno := template.GetAnnotations()
+	if v, ok := anno[key]; !ok && v != strconv.Itoa(value) {
+		t.Errorf("Failed to update %s annotation key: got=%s, want=%d", key, v, value)
+	}
+}
+
+func checkContainerConcurrency(t *testing.T, template *servingv1alpha1.RevisionTemplateSpec, value int) {
+	if got, want := template.Spec.ContainerConcurrency, value; got != v1beta1.RevisionContainerConcurrencyType(want) {
+		t.Errorf("Failed to update containerConcurrency value: got=%d, want=%d", got, want)
+	}
+}
+
+func updateConcurrencyConfiguration(template *servingv1alpha1.RevisionTemplateSpec, minScale int, maxScale int, target int, limit int) {
+	UpdateMinScale(template, minScale)
+	UpdateMaxScale(template, maxScale)
+	UpdateConcurrencyTarget(template, target)
+	UpdateConcurrencyLimit(template, limit)
+}

pkg/serving/revision_template.go

@@ -29,16 +29,11 @@ func ContainerOfRevisionSpec(revisionSpec *servingv1alpha1.RevisionSpec) (*corev
 	if usesOldV1alpha1ContainerField(revisionSpec) {
 		return revisionSpec.DeprecatedContainer, nil
 	}
-	containers := revisionSpec.Containers
+	container := revisionSpec.GetContainer()
-	if len(containers) == 0 {
+	if container == nil {
 		return nil, fmt.Errorf("internal: no container set in spec.template.spec.containers")
 	}
-	if len(containers) > 1 {
+	return container, nil
-		return nil, fmt.Errorf("internal: can't extract container for updating environment"+
-			" variables as the configuration contains "+
-			"more than one container (i.e. %d containers)", len(containers))
-	}
-	return &containers[0], nil
 }
 
 // =======================================================================================

test/e2e/service_options_test.go

@@ -46,6 +46,15 @@ func TestServiceOptions(t *testing.T) {
 		test.serviceDescribeConcurrencyLimit(t, "hello", "300")
 	})
 
+	t.Run("update concurrency options with invalid value for hello service and returns error", func(t *testing.T) {
+		test.serviceUpdateWithInvalidValue(t, "hello", []string{"--concurrency-limit", "-1", "--concurrency-target", "0"})
+	})
+
+	t.Run("returns steady concurrency options for hello service", func(t *testing.T) {
+		test.serviceDescribeConcurrencyLimit(t, "hello", "300")
+		test.serviceDescribeConcurrencyTarget(t, "hello", "300")
+	})
+
 	t.Run("delete hello service and returns no error", func(t *testing.T) {
 		test.serviceDelete(t, "hello")
 	})
@@ -77,3 +86,8 @@ func (test *e2eTest) serviceDescribeConcurrencyTarget(t *testing.T, serviceName,
 	expectedOutput := fmt.Sprintf("autoscaling.knative.dev/target: \"%s\"", concurrencyTarget)
 	assert.Check(t, util.ContainsAll(out, expectedOutput))
 }
+
+func (test *e2eTest) serviceUpdateWithInvalidValue(t *testing.T, serviceName string, args []string) {
+	_, err := test.kn.RunWithOpts(append([]string{"service", "update", serviceName}, args...), runOpts{NoNamespace: false, AllowError: true})
+	assert.ErrorContains(t, err, "Invalid")
+}

vendor/modules.txt

@@ -77,9 +77,9 @@ github.com/knative/serving/pkg/client/clientset/versioned/typed/serving/v1alpha1
 github.com/knative/serving/pkg/client/clientset/versioned/typed/serving/v1alpha1/fake
 github.com/knative/serving/pkg/apis/serving
 github.com/knative/serving/pkg/apis/serving/v1alpha1
+github.com/knative/serving/pkg/apis/autoscaling
 github.com/knative/serving/pkg/apis/serving/v1beta1
 github.com/knative/serving/pkg/client/clientset/versioned/scheme
-github.com/knative/serving/pkg/apis/autoscaling
 github.com/knative/serving/pkg/apis/networking
 github.com/knative/serving/pkg/apis/networking/v1alpha1
 github.com/knative/serving/pkg/apis/config