Fix labels and annotations propagation to k8s service on update (#15907)

Co-authored-by: David Simansky <[email protected]>

Author: knative-prow-robot

pkg/reconciler/route/reconcile_resources.go

@@ -37,6 +37,7 @@ import (
 	"knative.dev/networking/pkg/apis/networking"
 	netv1alpha1 "knative.dev/networking/pkg/apis/networking/v1alpha1"
 	"knative.dev/pkg/controller"
+	"knative.dev/pkg/kmap"
 	"knative.dev/pkg/logging"
 	"knative.dev/serving/pkg/apis/serving"
 	v1 "knative.dev/serving/pkg/apis/serving/v1"
@@ -256,10 +257,14 @@ func (c *Reconciler) updatePlaceholderServices(ctx context.Context, route *v1.Ro
 
 			if canUpdate {
 				// Make sure that the service has the proper specification.
-				if !equality.Semantic.DeepEqual(from.Service.Spec, to.Service.Spec) {
+				if !equality.Semantic.DeepEqual(from.Spec, to.Spec) ||
+					!equality.Semantic.DeepEqual(from.Service.Annotations, kmap.Union(from.Service.Annotations, to.Service.Annotations)) ||
+					!equality.Semantic.DeepEqual(from.Service.Labels, kmap.Union(from.Service.Labels, to.Service.Labels)) {
 					// Don't modify the informers copy.
 					existing := from.Service.DeepCopy()
 					existing.Spec = to.Service.Spec
+					existing.Annotations = kmap.Union(from.Service.Annotations, to.Service.Annotations)
+					existing.Labels = kmap.Union(from.Service.Labels, to.Service.Labels)
 					if _, err := c.kubeclient.CoreV1().Services(ns).Update(ctx, existing, metav1.UpdateOptions{}); err != nil {
 						return err
 					}

pkg/reconciler/route/table_test.go

@@ -786,7 +786,7 @@ func TestReconcile(t *testing.T) {
 				},
 				withReadyIngress,
 			),
-			simpleK8sService(Route("default", "different-domain", WithConfigTarget("config"))),
+			simpleK8sService(Route("default", "different-domain", WithConfigTarget("config"), WithRouteLabel(map[string]string{"app": "prod"}))),
 		},
 		WantUpdates: []clientgotesting.UpdateActionImpl{{
 			Object: simpleIngress(

pkg/testing/v1/service.go

@@ -145,7 +145,7 @@ func WithServiceAnnotation(k, v string) ServiceOption {
 	}
 }
 
-// WithServiceAnnotationRemoved adds the given annotation to the service.
+// WithServiceAnnotationRemoved removes the given annotation from the service.
 func WithServiceAnnotationRemoved(k string) ServiceOption {
 	return func(svc *v1.Service) {
 		svc.Annotations = kmeta.FilterMap(svc.Annotations, func(s string) bool {
@@ -154,6 +154,15 @@ func WithServiceAnnotationRemoved(k string) ServiceOption {
 	}
 }
 
+// WithServiceLabelRemoved removes the given label from the service.
+func WithServiceLabelRemoved(k string) ServiceOption {
+	return func(svc *v1.Service) {
+		svc.Labels = kmeta.FilterMap(svc.Labels, func(s string) bool {
+			return k == s
+		})
+	}
+}
+
 // WithServiceImage sets the container image to be the provided string.
 func WithServiceImage(img string) ServiceOption {
 	return func(svc *v1.Service) {

test/conformance/api/v1/service_test.go

@@ -21,6 +21,7 @@ package v1
 
 import (
 	"context"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -618,6 +619,9 @@ func TestAnnotationPropagation(t *testing.T) {
 	if err := validateAnnotations(objects); err != nil {
 		t.Error("Annotations are incorrect:", err)
 	}
+	if err := validateK8sServiceAnnotations(t, clients, names); err != nil {
+		t.Error(err)
+	}
 
 	if objects.Service, err = v1test.UpdateService(t, clients, names,
 		rtesting.WithServiceAnnotation("juicy", "jamba")); err != nil {
@@ -651,6 +655,9 @@ func TestAnnotationPropagation(t *testing.T) {
 	if err := validateAnnotations(objects, "juicy"); err != nil {
 		t.Error("Annotations are incorrect:", err)
 	}
+	if err := validateK8sServiceAnnotations(t, clients, names, "juicy"); err != nil {
+		t.Error(err)
+	}
 
 	if objects.Service, err = v1test.UpdateService(t, clients, names,
 		rtesting.WithServiceAnnotationRemoved("juicy")); err != nil {
@@ -684,12 +691,20 @@ func TestAnnotationPropagation(t *testing.T) {
 	if err := validateAnnotations(objects); err != nil {
 		t.Error("Annotations are incorrect:", err)
 	}
+	if err := validateK8sServiceAnnotations(t, clients, names, "juicy"); err != nil {
+		t.Error(err)
+	}
 	if _, ok := objects.Config.Annotations["juicy"]; ok {
 		t.Error("Config still has `juicy` annotation")
 	}
 	if _, ok := objects.Route.Annotations["juicy"]; ok {
 		t.Error("Route still has `juicy` annotation")
 	}
+	if err := validateK8sServiceAnnotations(t, clients, names, "juicy"); err != nil {
+		if !strings.Contains(err.Error(), "was empty") {
+			t.Error(err)
+		}
+	}
 }
 
 // TestServiceCreateWithMultipleContainers tests both Creation paths for a service.
@@ -750,6 +765,126 @@ func TestServiceCreateWithMultipleContainers(t *testing.T) {
 	}
 }
 
+func TestLabelPropagation(t *testing.T) {
+	t.Parallel()
+	clients := test.Setup(t)
+
+	names := test.ResourceNames{
+		Service: test.ObjectNameForTest(t),
+		Image:   test.PizzaPlanet1,
+	}
+
+	// Clean up on test failure or interrupt
+	test.EnsureTearDown(t, clients, &names)
+
+	// Setup initial Service
+	objects, err := v1test.CreateServiceReady(t, clients, &names)
+	if err != nil {
+		t.Fatalf("Failed to create initial Service %v: %v", names.Service, err)
+	}
+
+	// Validate State after Creation
+	if err = validateControlPlane(t, clients, names, "1"); err != nil {
+		t.Error(err)
+	}
+
+	// Validate State after Creation
+
+	if err = validateControlPlane(t, clients, names, "1"); err != nil {
+		t.Error(err)
+	}
+
+	if err := validateLabelsPropagation(t, *objects, names); err != nil {
+		t.Error(err)
+	}
+	if err := validateK8sServiceLabels(t, clients, names); err != nil {
+		t.Error(err)
+	}
+
+	if objects.Service, err = v1test.UpdateService(t, clients, names,
+		rtesting.WithServiceLabel("juicy", "jamba")); err != nil {
+		t.Fatalf("Service %s was not updated with new annotation: %v", names.Service, err)
+	}
+
+	// Trigger new Revision generation
+	t.Log("Updating the Service to use a different image.")
+	image2 := pkgtest.ImagePath(test.PizzaPlanet2)
+	if objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithServiceImage(image2)); err != nil {
+		t.Fatalf("Update for Service %s with new image %s failed: %v", names.Service, image2, err)
+	}
+	t.Log("Service should reflect new revision created and ready in status.")
+	names.Revision, err = v1test.WaitForServiceLatestRevision(clients, names)
+	if err != nil {
+		t.Fatal("New image not reflected in Service:", err)
+	}
+	t.Log("Waiting for Service to transition to Ready.")
+	if err := v1test.WaitForServiceState(clients.ServingClient, names.Service, v1test.IsServiceReady, "ServiceIsReady"); err != nil {
+		t.Fatal("Error waiting for the service to become ready for the latest revision:", err)
+	}
+	objects, err = v1test.GetResourceObjects(clients, names)
+	if err != nil {
+		t.Error("Error getting objects:", err)
+	}
+
+	// Validate updated labels
+	if err := validateLabelsPropagation(t, *objects, names); err != nil {
+		t.Error("Annotations are incorrect:", err)
+	}
+	if _, ok := objects.Config.Labels["juicy"]; !ok {
+		t.Error("Config missing `juicy` label")
+	}
+	if _, ok := objects.Route.Labels["juicy"]; !ok {
+		t.Error("Route missing `juicy` label")
+	}
+	if err := validateK8sServiceLabels(t, clients, names, "juicy"); err != nil {
+		t.Error(err)
+	}
+
+	if objects.Service, err = v1test.UpdateService(t, clients, names,
+		rtesting.WithServiceLabelRemoved("juicy")); err != nil {
+		t.Fatalf("Service %s was not updated with annotation deleted: %v", names.Service, err)
+	}
+
+	// Trigger new Revision generation
+	t.Log("Updating the Service to use a different image.")
+	image3 := pkgtest.ImagePath(test.HelloWorld)
+	if objects.Service, err = v1test.UpdateService(t, clients, names, rtesting.WithServiceImage(image3)); err != nil {
+		t.Fatalf("Update for Service %s with new image %s failed: %v", names.Service, image3, err)
+	}
+	t.Log("Service should reflect new revision created and ready in status.")
+	names.Revision, err = v1test.WaitForServiceLatestRevision(clients, names)
+	if err != nil {
+		t.Fatal("New image not reflected in Service:", err)
+	}
+	t.Log("Waiting for Service to transition to Ready.")
+	if err := v1test.WaitForServiceState(clients.ServingClient, names.Service, v1test.IsServiceReady, "ServiceIsReady"); err != nil {
+		t.Fatal("Error waiting for the service to become ready for the latest revision:", err)
+	}
+	objects, err = v1test.GetResourceObjects(clients, names)
+	if err != nil {
+		t.Error("Error getting objects:", err)
+	}
+
+	// Now we can validate the annotations.
+	if err := validateLabelsPropagation(t, *objects, names); err != nil {
+		t.Error(err)
+	}
+	if err := validateK8sServiceLabels(t, clients, names, "juicy"); err != nil {
+		t.Error(err)
+	}
+	if _, ok := objects.Config.Labels["juicy"]; ok {
+		t.Error("Config still has `juicy` annotation")
+	}
+	if _, ok := objects.Route.Labels["juicy"]; ok {
+		t.Error("Route still has `juicy` annotation")
+	}
+	if err := validateK8sServiceLabels(t, clients, names, "juicy"); err != nil {
+		if !strings.Contains(err.Error(), "was empty") {
+			t.Error(err)
+		}
+	}
+}
+
 // TestServiceCreateWithEmptyDir tests both Creation paths for a service with emptydir enabled.
 // The test performs a series of Validate steps to ensure that the service transitions as expected during each step.
 func TestServiceCreateWithEmptyDir(t *testing.T) {

test/conformance/api/v1/util.go

@@ -218,6 +218,27 @@ func validateLabelsPropagation(t testing.TB, objects v1test.ResourceObjects, nam
 	return nil
 }
 
+func validateK8sServiceLabels(t *testing.T, clients *test.Clients, names test.ResourceNames, extraKeys ...string) error {
+	t.Log("Validate Labels on Kubernetes Service Object")
+	k8sService, err := clients.KubeClient.CoreV1().Services(test.ServingFlags.TestNamespace).Get(context.Background(), names.Service, metav1.GetOptions{})
+	if err != nil {
+		return fmt.Errorf("failed to get k8s service %v: %w", names.Service, err)
+	}
+	labels := k8sService.GetLabels()
+	if labels["serving.knative.dev/service"] != names.Service {
+		return fmt.Errorf("expected Service name in K8s Service label %q but got %q ", names.Service, labels["serving.knative.dev/service"])
+	}
+	if labels["serving.knative.dev/route"] != names.Service {
+		return fmt.Errorf("expected Route name in K8s Service label %q but got %q ", names.Service, labels["serving.knative.dev/route"])
+	}
+	for _, extraKey := range extraKeys {
+		if got := labels[extraKey]; got == "" {
+			return fmt.Errorf("expected %s label to be set, but was empty", extraKey)
+		}
+	}
+	return nil
+}
+
 func validateAnnotations(objs *v1test.ResourceObjects, extraKeys ...string) error {
 	// This checks whether the annotations are set on the resources that
 	// expect them to have.
@@ -244,6 +265,21 @@ func validateAnnotations(objs *v1test.ResourceObjects, extraKeys ...string) erro
 	return nil
 }
 
+func validateK8sServiceAnnotations(t *testing.T, clients *test.Clients, names test.ResourceNames, extraKeys ...string) error {
+	t.Log("Validate Annotations on Kubernetes Service Object")
+	k8sService, err := clients.KubeClient.CoreV1().Services(test.ServingFlags.TestNamespace).Get(context.Background(), names.Service, metav1.GetOptions{})
+	if err != nil {
+		return fmt.Errorf("failed to get k8s service %v: %w", names.Service, err)
+	}
+	annotations := k8sService.GetAnnotations()
+	for _, extraKey := range append([]string{serving.CreatorAnnotation, serving.UpdaterAnnotation}, extraKeys...) {
+		if got := annotations[extraKey]; got == "" {
+			return fmt.Errorf("expected %s annotation to be set, but was empty", extraKey)
+		}
+	}
+	return nil
+}
+
 func validateReleaseServiceShape(objs *v1test.ResourceObjects) error {
 	// Traffic should be routed to the lastest created revision.
 	if got, want := objs.Service.Status.Traffic[0].RevisionName, objs.Config.Status.LatestReadyRevisionName; got != want {