diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go index d76d2381f4..8050e1464e 100644 --- a/pkg/kn/commands/service/configuration_edit_flags.go +++ b/pkg/kn/commands/service/configuration_edit_flags.go @@ -412,10 +412,23 @@ func (p *ConfigurationEditFlags) Apply( } annotationsToRemove := util.ParseMinusSuffix(annotationsMap) - err = servinglib.UpdateAnnotations(service, template, annotationsMap, annotationsToRemove) + // Service Annotations can't contain Autoscaling ones + serviceAnnotations := make(map[string]string) + for key, value := range annotationsMap { + if !strings.HasPrefix(key, autoscaling.GroupName) { + serviceAnnotations[key] = value + } + } + // Add all user provided annotations to RevisionTemplate + err = servinglib.UpdateRevisionTemplateAnnotations(template, annotationsMap, annotationsToRemove) + if err != nil { + return err + } + err = servinglib.UpdateServiceAnnotations(service, serviceAnnotations, annotationsToRemove) if err != nil { return err } + } if cmd.Flags().Changed("service-account") { @@ -446,11 +459,8 @@ func (p *ConfigurationEditFlags) Apply( if cmd.Flags().Changed("annotation") && containsAnnotation(p.Annotations, autoscaling.InitialScaleAnnotationKey) { return fmt.Errorf("only one of the --scale-init or --annotation %s can be specified", autoscaling.InitialScaleAnnotationKey) } - annotationsMap := map[string]string{ - autoscaling.InitialScaleAnnotationKey: strconv.Itoa(p.ScaleInit), - } - - err = servinglib.UpdateAnnotations(service, template, annotationsMap, []string{}) + // Autoscaling annotations are only applicable on Revision Template, not Service + err = servinglib.UpdateRevisionTemplateAnnotation(template, autoscaling.InitialScaleAnnotationKey, strconv.Itoa(p.ScaleInit)) if err != nil { return err } diff --git a/pkg/kn/commands/service/create_mock_test.go b/pkg/kn/commands/service/create_mock_test.go index 627c86fd6c..0c919ee76b 100644 --- a/pkg/kn/commands/service/create_mock_test.go +++ b/pkg/kn/commands/service/create_mock_test.go @@ -18,6 +18,8 @@ import ( "testing" "time" + "knative.dev/serving/pkg/apis/autoscaling" + "gotest.tools/assert" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -545,6 +547,38 @@ func TestServiceCreateWithBothAnnotationAndInitScaleAsOption(t *testing.T) { r.Validate() } +func TestServiceCreateWithAnnotations(t *testing.T) { + client := knclient.NewMockKnServiceClient(t) + + r := client.Recorder() + r.GetService("foo", nil, errors.NewNotFound(servingv1.Resource("service"), "foo")) + + service := getService("foo") + template := &service.Spec.Template + + service.ObjectMeta.Annotations = map[string]string{ + "foo": "bar", + } + + template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz" + template.ObjectMeta.Annotations = map[string]string{ + autoscaling.InitialScaleAnnotationKey: "1", // autoscaling in only added Revision Template + "foo": "bar", + servinglib.UserImageAnnotationKey: "gcr.io/foo/bar:baz", + } + + r.CreateService(service, nil) + + output, err := executeServiceCommand(client, "create", "foo", "--image", "gcr.io/foo/bar:baz", + "--annotation", "foo=bar", + "--annotation", autoscaling.InitialScaleAnnotationKey+"=1", + "--no-wait", "--revision-name=") + assert.NilError(t, err) + assert.Assert(t, util.ContainsAll(output, "created", "foo", "default")) + + r.Validate() +} + func getServiceWithUrl(name string, urlName string) *servingv1.Service { service := servingv1.Service{} url, _ := apis.ParseURL(urlName) diff --git a/pkg/kn/commands/service/service_update_mock_test.go b/pkg/kn/commands/service/service_update_mock_test.go index 783ac7c817..885647f6c4 100644 --- a/pkg/kn/commands/service/service_update_mock_test.go +++ b/pkg/kn/commands/service/service_update_mock_test.go @@ -17,6 +17,8 @@ package service import ( "testing" + "knative.dev/serving/pkg/apis/autoscaling" + "gotest.tools/assert" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/errors" @@ -77,10 +79,11 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) { "an3": "getsRemoved", } template.ObjectMeta.Annotations = map[string]string{ - "an1": "staysConstant", - "an2": "getsUpdated", - "an3": "getsRemoved", - clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", + "an1": "staysConstant", + "an2": "getsUpdated", + "an3": "getsRemoved", + clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", + autoscaling.InitialScaleAnnotationKey: "1", } updatedService := getService(svcName) @@ -91,9 +94,10 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) { "an2": "isUpdated", } template.ObjectMeta.Annotations = map[string]string{ - "an1": "staysConstant", - "an2": "isUpdated", - clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", + "an1": "staysConstant", + "an2": "isUpdated", + clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", + autoscaling.InitialScaleAnnotationKey: "2", } r := client.Recorder() @@ -104,6 +108,7 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) { "--annotation", "an1=staysConstant", "--annotation", "an2=getsUpdated", "--annotation", "an3=getsRemoved", + "--annotation", autoscaling.InitialScaleAnnotationKey+"=1", "--no-wait", "--revision-name=", ) assert.NilError(t, err) @@ -113,6 +118,7 @@ func TestServiceUpdateAnnotationsMock(t *testing.T) { "update", svcName, "--annotation", "an2=isUpdated", "--annotation", "an3-", + "--annotation", autoscaling.InitialScaleAnnotationKey+"=2", "--no-wait", "--revision-name=", ) assert.NilError(t, err) @@ -1480,9 +1486,6 @@ func TestServiceUpdateInitialScaleMock(t *testing.T) { newService := getService(svcName) template := &newService.Spec.Template template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz" - newService.ObjectMeta.Annotations = map[string]string{ - "autoscaling.knative.dev/initialScale": "1", - } template.ObjectMeta.Annotations = map[string]string{ "autoscaling.knative.dev/initialScale": "1", clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", @@ -1491,9 +1494,6 @@ func TestServiceUpdateInitialScaleMock(t *testing.T) { updatedService := getService(svcName) template = &updatedService.Spec.Template template.Spec.Containers[0].Image = "gcr.io/foo/bar:baz" - updatedService.ObjectMeta.Annotations = map[string]string{ - "autoscaling.knative.dev/initialScale": "2", - } template.ObjectMeta.Annotations = map[string]string{ "autoscaling.knative.dev/initialScale": "2", clientserving.UserImageAnnotationKey: "gcr.io/foo/bar:baz", diff --git a/pkg/serving/config_changes.go b/pkg/serving/config_changes.go index 057fc32aca..74fcb32b3f 100644 --- a/pkg/serving/config_changes.go +++ b/pkg/serving/config_changes.go @@ -216,30 +216,6 @@ func UpdateConcurrencyLimit(template *servingv1.RevisionTemplateSpec, limit int6 return nil } -// UpdateRevisionTemplateAnnotation updates an annotation for the given Revision Template. -// Also validates the autoscaling annotation values -func UpdateRevisionTemplateAnnotation(template *servingv1.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 - // The boolean indicates whether or not the init-scale annotation can be set to 0. - // Since we don't have the config handy, err towards allowing it. The API will - // correctly fail the request if it's forbidden. - if err := autoscaling.ValidateAnnotations(true, in); err != nil { - return err - } - - annoMap[annotation] = value - return nil -} - // 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) { @@ -452,33 +428,30 @@ func UpdateLabels(labelsMap map[string]string, add map[string]string, remove []s return labelsMap } -// UpdateAnnotations updates the annotations identically on a service and template. -// Does not overwrite the entire Annotations field, only makes the requested updates. -func UpdateAnnotations( - service *servingv1.Service, - template *servingv1.RevisionTemplateSpec, - toUpdate map[string]string, - toRemove []string) error { - - if service.ObjectMeta.Annotations == nil { - service.ObjectMeta.Annotations = make(map[string]string) - } - - if template.ObjectMeta.Annotations == nil { - template.ObjectMeta.Annotations = make(map[string]string) +// UpdateServiceAnnotations updates annotations for the given Service Metadata. +func UpdateServiceAnnotations(service *servingv1.Service, toUpdate map[string]string, toRemove []string) error { + if service.Annotations == nil && len(toUpdate) > 0 { + service.Annotations = make(map[string]string) } + return updateAnnotations(service.Annotations, toUpdate, toRemove) +} - for key, value := range toUpdate { - service.ObjectMeta.Annotations[key] = value - template.ObjectMeta.Annotations[key] = value +// UpdateRevisionTemplateAnnotations updates annotations for the given Revision Template. +// Also validates the autoscaling annotation values +func UpdateRevisionTemplateAnnotations(template *servingv1.RevisionTemplateSpec, toUpdate map[string]string, toRemove []string) error { + if err := autoscaling.ValidateAnnotations(true, toUpdate); err != nil { + return err } - - for _, key := range toRemove { - delete(service.ObjectMeta.Annotations, key) - delete(template.ObjectMeta.Annotations, key) + if template.Annotations == nil { + template.Annotations = make(map[string]string) } + return updateAnnotations(template.Annotations, toUpdate, toRemove) +} - return nil +// UpdateRevisionTemplateAnnotation updates an annotation for the given Revision Template. +// Also validates the autoscaling annotation values +func UpdateRevisionTemplateAnnotation(template *servingv1.RevisionTemplateSpec, annotation string, value string) error { + return UpdateRevisionTemplateAnnotations(template, map[string]string{annotation: value}, []string{}) } // UpdateServiceAccountName updates the service account name used for the corresponding knative service @@ -543,6 +516,16 @@ func GenerateVolumeName(path string) string { // ======================================================================================= +func updateAnnotations(annotations map[string]string, toUpdate map[string]string, toRemove []string) error { + for key, value := range toUpdate { + annotations[key] = value + } + for _, key := range toRemove { + delete(annotations, key) + } + return nil +} + func updateEnvVarsFromMap(env []corev1.EnvVar, toUpdate map[string]string) []corev1.EnvVar { set := sets.NewString() for i := range env { diff --git a/pkg/serving/config_changes_test.go b/pkg/serving/config_changes_test.go index bd985e0055..eed1b75cc0 100644 --- a/pkg/serving/config_changes_test.go +++ b/pkg/serving/config_changes_test.go @@ -579,38 +579,81 @@ func TestUpdateImagePullSecrets(t *testing.T) { assert.Check(t, template.Spec.ImagePullSecrets == nil) } +func TestUpdateRevisionTemplateAnnotationsNew(t *testing.T) { + _, template, _ := getService() + + annotations := map[string]string{ + autoscaling.InitialScaleAnnotationKey: "1", + autoscaling.MaxScaleAnnotationKey: "2", + } + err := UpdateRevisionTemplateAnnotations(template, annotations, []string{}) + assert.NilError(t, err) + + actual := template.ObjectMeta.Annotations + assert.DeepEqual(t, annotations, actual) +} + +func TestUpdateRevisionTemplateAnnotationsExisting(t *testing.T) { + _, template, _ := getService() + template.ObjectMeta.Annotations = map[string]string{ + autoscaling.InitialScaleAnnotationKey: "1", + autoscaling.MaxScaleAnnotationKey: "2", + } + + annotations := map[string]string{ + autoscaling.InitialScaleAnnotationKey: "5", + autoscaling.MaxScaleAnnotationKey: "10", + } + err := UpdateRevisionTemplateAnnotations(template, annotations, []string{}) + assert.NilError(t, err) + + actual := template.ObjectMeta.Annotations + assert.DeepEqual(t, annotations, actual) +} + +func TestUpdateRevisionTemplateAnnotationsRemoveExisting(t *testing.T) { + _, template, _ := getService() + template.ObjectMeta.Annotations = map[string]string{ + autoscaling.InitialScaleAnnotationKey: "1", + autoscaling.MaxScaleAnnotationKey: "2", + } + expectedAnnotations := map[string]string{ + autoscaling.InitialScaleAnnotationKey: "1", + } + remove := []string{autoscaling.MaxScaleAnnotationKey} + err := UpdateRevisionTemplateAnnotations(template, map[string]string{}, remove) + assert.NilError(t, err) + + actual := template.ObjectMeta.Annotations + assert.DeepEqual(t, expectedAnnotations, actual) +} + func TestUpdateAnnotationsNew(t *testing.T) { - service, template, _ := getService() + service, _, _ := getService() annotations := map[string]string{ "a": "foo", "b": "bar", } - err := UpdateAnnotations(service, template, annotations, []string{}) + err := UpdateServiceAnnotations(service, annotations, []string{}) assert.NilError(t, err) actual := service.ObjectMeta.Annotations if !reflect.DeepEqual(annotations, actual) { t.Fatalf("Service annotations did not match expected %v found %v", annotations, actual) } - - actual = template.ObjectMeta.Annotations - if !reflect.DeepEqual(annotations, actual) { - t.Fatalf("Template annotations did not match expected %v found %v", annotations, actual) - } } func TestUpdateAnnotationsExisting(t *testing.T) { - service, template, _ := getService() + service, _, _ := getService() service.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"} - template.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"} annotations := map[string]string{ "a": "notfoo", "c": "bat", "d": "", } - err := UpdateAnnotations(service, template, annotations, []string{}) + err := UpdateServiceAnnotations(service, annotations, []string{}) assert.NilError(t, err) expected := map[string]string{ "a": "notfoo", @@ -621,18 +664,14 @@ func TestUpdateAnnotationsExisting(t *testing.T) { actual := service.ObjectMeta.Annotations assert.DeepEqual(t, expected, actual) - - actual = template.ObjectMeta.Annotations - assert.DeepEqual(t, expected, actual) } func TestUpdateAnnotationsRemoveExisting(t *testing.T) { - service, template, _ := getService() + service, _, _ := getService() service.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"} - template.ObjectMeta.Annotations = map[string]string{"a": "foo", "b": "bar"} remove := []string{"b"} - err := UpdateAnnotations(service, template, map[string]string{}, remove) + err := UpdateServiceAnnotations(service, map[string]string{}, remove) assert.NilError(t, err) expected := map[string]string{ "a": "foo", @@ -640,9 +679,6 @@ func TestUpdateAnnotationsRemoveExisting(t *testing.T) { actual := service.ObjectMeta.Annotations assert.DeepEqual(t, expected, actual) - - actual = template.ObjectMeta.Annotations - assert.DeepEqual(t, expected, actual) } func TestGenerateVolumeName(t *testing.T) { diff --git a/test/e2e/service_options_test.go b/test/e2e/service_options_test.go index 586eb43042..c0f5ffdb43 100644 --- a/test/e2e/service_options_test.go +++ b/test/e2e/service_options_test.go @@ -25,6 +25,8 @@ import ( "strings" "testing" + "knative.dev/serving/pkg/apis/autoscaling" + "gotest.tools/assert" "knative.dev/client/lib/test" @@ -156,6 +158,11 @@ func TestServiceOptions(t *testing.T) { t.Log("delete service") test.ServiceDelete(r, "svc10") + t.Log("create and validate service with scale init option via --annotation flag") + serviceCreateWithOptions(r, "svc11", "--annotation", autoscaling.InitialScaleAnnotationKey+"=2") + validateServiceInitScale(r, "svc11", "2") + t.Log("delete service") + test.ServiceDelete(r, "svc11") } func serviceCreateWithOptions(r *test.KnRunResultCollector, serviceName string, options ...string) {