Repository: knative/client (go)

Issue:
autoscaling annotations must be put under "spec.template.metadata.annotations" to work
In client, we set (all) the annotations to service metadata annos as well as template metadata annotations.
see https://github.com/knative/client/blob/1607bf8c0c6404632d203f5a8794ac60d7c7659c/pkg/serving/config_changes.go#L457

With https://github.com/knative/serving/pull/9358 serving now validates the autoscaling related annotations, we'd need to stop setting the respective annotations at service metadata and only set for template metadata. The change in serving landed in master and would be part of v0.18.0 , so we'll need this for client v0.18.0 and probably backport to v0.17.x as well ? (cc: @rhuss )

The fix should be made in a manner to consider this related feature request https://github.com/knative/client/issues/1014 

Pull request:
fix: Fix autoscaling annotations in Service metadata
## Description

This fix should mitigate usage of autoscaling annotations in Service's `metadata.Annotations` field.

Related to split annotation flag mentioned in https://github.com/knative/client/issues/1014.


## Changes

<!-- Please add list of more detailed changes. These changes should be reflected also in the commit messages -->

* `UpdateAnnotations` can distinguish autoscaling annotations and add them to template only
* 

## Reference

<!-- Please add the corresponding issue number which this pull request is about to fix -->
Fixes #1020

<!--
Please add an entrty to CHANGELOG.adoc file, too, as part of your Pull Request.

In the following cases, add a short description of PR to the unreleased section in CHANGELOG.adoc:

- 🎁 New feature
- 🐛 Bug fix
- ✨ Feature Update
- 🐣 Refactoring
- 🗑️ Remove feature or internal logic

See other entries in CHANGELOG.adoc as an example for how to add the entry, including a reference to the corresponding issue/PR

PLEASE DON'T ADD THAT LINE HERE IN THE PULL-REQUEST DESCRIPTION BUT DIRECTLY IN CHANGELOG.ADOC AND ADD CHANGELOG.ADOC AS PART OF YOUR PULL-REQUEST.
-->

<!--
To automatically lint go code in this pull request uncomment the line below. You get feedback as comments on your pull request then -->

<!--
/lint
-->


Patch:
diff --git a/pkg/kn/commands/service/configuration_edit_flags.go b/pkg/kn/commands/service/configuration_edit_flags.go
index d76d2381..8050e146 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/serving/config_changes.go b/pkg/serving/config_changes.go
index 057fc32a..74fcb32b 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 {


Test patch:
diff --git a/pkg/kn/commands/service/create_mock_test.go b/pkg/kn/commands/service/create_mock_test.go
index 627c86fd..0c919ee7 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 783ac7c8..885647f6 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_test.go b/pkg/serving/config_changes_test.go
index bd985e00..eed1b75c 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 586eb430..c0f5ffdb 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) {