From b7808b0fa2f578b98c2c566e806cbfff9007ebca Mon Sep 17 00:00:00 2001 From: Tsubasa Nagasawa Date: Thu, 25 Jul 2019 18:57:35 +0900 Subject: [PATCH] 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 --- .../service/configuration_edit_flags.go | 28 +++- pkg/serving/config_changes.go | 64 +++++-- pkg/serving/config_changes_test.go | 158 ++++++++++++++---- pkg/serving/revision_template.go | 11 +- test/e2e/service_options_test.go | 14 ++ vendor/modules.txt | 2 +- 6 files changed, 215 insertions(+), 62 deletions(-) diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index e92f7f9838..01b5a7a37e 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/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 } diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 7fdee53729..7cc8bfbf1c 100644 --- a/pkg/serving/config_changes.go +++ b/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 -func UpdateConcurrencyConfiguration(template *servingv1alpha1.RevisionTemplateSpec, minScale int, maxScale int, target int, limit int) { - if minScale != 0 { - 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)) +// UpdateMinScale updates min scale annotation +func UpdateMinScale(template *servingv1alpha1.RevisionTemplateSpec, min int) error { + return UpdateAnnotation(template, autoscaling.MinScaleAnnotationKey, strconv.Itoa(min)) +} + +// UpdatMaxScale updates max scale annotation +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) } - if limit != 0 { - template.Spec.ContainerConcurrency = servingv1beta1.RevisionContainerConcurrencyType(limit) + 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 } -// Updater (or add) an annotation to the given service -func UpdateAnnotation(template *servingv1alpha1.RevisionTemplateSpec, annotation string, value string) { +// 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 { diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index 51dbcda9a7..e07dc5e089 100644 --- a/pkg/serving/config_changes_test.go +++ b/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 { - t.Fatal(err) - } + assert.NilError(t, err) found, err := EnvToMap(container.Env) - if err != nil { - t.Fatal(err) - } + assert.NilError(t, 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 { - t.Fatal(err) - } + assert.NilError(t, 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 { - t.Fatal(err) - } + assert.NilError(t, 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 { - t.Fatal(err) - } + assert.NilError(t, err) expected := map[string]string{ "a": "fancy", } found, err := EnvToMap(container.Env) - if err != nil { - t.Fatal(err) - } + assert.NilError(t, 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 { - t.Fatal(err) - } + assert.NilError(t, 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 { - t.Fatal(err) - } + assert.NilError(t, 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 { - t.Fatal(err) - } + assert.NilError(t, 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 { - t.Fatal(err) - } + assert.NilError(t, 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) +} diff --git a/pkg/serving/revision_template.go b/pkg/serving/revision_template.go index becff47631..af6d722053 100644 --- a/pkg/serving/revision_template.go +++ b/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 - if len(containers) == 0 { + container := revisionSpec.GetContainer() + if container == nil { return nil, fmt.Errorf("internal: no container set in spec.template.spec.containers") } - if len(containers) > 1 { - 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 + return container, nil } // ======================================================================================= diff --git a/test/e2e/service_options_test.go b/test/e2e/service_options_test.go index 96514d057b..c28cafcc2e 100644 --- a/test/e2e/service_options_test.go +++ b/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") +} diff --git a/vendor/modules.txt b/vendor/modules.txt index d761a43c0e..80d5e0a381 100644 --- a/vendor/modules.txt +++ b/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