From 9e571c04240729dfd6401eacac39135203723450 Mon Sep 17 00:00:00 2001
From: Rob Scott <robertjscott@google.com>
Date: Tue, 14 Mar 2023 20:08:05 +0000
Subject: [PATCH] Adding validation for topology annotations

Change-Id: I50b3b05b859c69e98daca7c8fca0d3a76024eb80
---
 pkg/api/service/warnings.go                 |  4 +++
 pkg/api/service/warnings_test.go            | 34 +++++++++++++++++++++
 pkg/apis/core/validation/validation.go      | 11 ++++++-
 pkg/apis/core/validation/validation_test.go | 16 ++++++++++
 4 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/pkg/api/service/warnings.go b/pkg/api/service/warnings.go
index 6d0ff8e6651..c99553367b7 100644
--- a/pkg/api/service/warnings.go
+++ b/pkg/api/service/warnings.go
@@ -31,6 +31,10 @@ func GetWarningsForService(service, oldService *api.Service) []string {
 	}
 	var warnings []string
 
+	if _, ok := service.Annotations[api.DeprecatedAnnotationTopologyAwareHints]; ok {
+		warnings = append(warnings, fmt.Sprintf("annotation %s is deprecated, please use %s instead", api.DeprecatedAnnotationTopologyAwareHints, api.AnnotationTopologyMode))
+	}
+
 	if helper.IsServiceIPSet(service) {
 		for i, clusterIP := range service.Spec.ClusterIPs {
 			warnings = append(warnings, getWarningsForIP(field.NewPath("spec").Child("clusterIPs").Index(i), clusterIP)...)
diff --git a/pkg/api/service/warnings_test.go b/pkg/api/service/warnings_test.go
index b3ca4b09cb4..8b48a6d0d08 100644
--- a/pkg/api/service/warnings_test.go
+++ b/pkg/api/service/warnings_test.go
@@ -26,6 +26,40 @@ import (
 	api "k8s.io/kubernetes/pkg/apis/core"
 )
 
+func TestGetWarningsForService(t *testing.T) {
+	testCases := []struct {
+		name        string
+		tweakSvc    func(svc *api.Service) // Given a basic valid service, each test case can customize it.
+		numWarnings int
+	}{
+		{
+			name: "new topology mode set",
+			tweakSvc: func(s *api.Service) {
+				s.Annotations = map[string]string{api.AnnotationTopologyMode: "foo"}
+			},
+			numWarnings: 0,
+		},
+		{
+			name: "deprecated hints annotation set",
+			tweakSvc: func(s *api.Service) {
+				s.Annotations = map[string]string{api.DeprecatedAnnotationTopologyAwareHints: "foo"}
+			},
+			numWarnings: 1,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			svc := &api.Service{}
+			tc.tweakSvc(svc)
+			warnings := GetWarningsForService(svc, svc)
+			if len(warnings) != tc.numWarnings {
+				t.Errorf("Unexpected warning list: %v", warnings)
+			}
+		})
+	}
+}
+
 func TestGetWarningsForServiceClusterIPs(t *testing.T) {
 	service := func(clusterIPs []string) *api.Service {
 		svc := api.Service{
diff --git a/pkg/apis/core/validation/validation.go b/pkg/apis/core/validation/validation.go
index 6a6e1083b3a..fca02606a96 100644
--- a/pkg/apis/core/validation/validation.go
+++ b/pkg/apis/core/validation/validation.go
@@ -4892,7 +4892,16 @@ var supportedServiceIPFamilyPolicy = sets.NewString(string(core.IPFamilyPolicySi
 
 // ValidateService tests if required fields/annotations of a Service are valid.
 func ValidateService(service *core.Service) field.ErrorList {
-	allErrs := ValidateObjectMeta(&service.ObjectMeta, true, ValidateServiceName, field.NewPath("metadata"))
+	metaPath := field.NewPath("metadata")
+	allErrs := ValidateObjectMeta(&service.ObjectMeta, true, ValidateServiceName, metaPath)
+
+	topologyHintsVal, topologyHintsSet := service.Annotations[core.DeprecatedAnnotationTopologyAwareHints]
+	topologyModeVal, topologyModeSet := service.Annotations[core.AnnotationTopologyMode]
+
+	if topologyModeSet && topologyHintsSet && topologyModeVal != topologyHintsVal {
+		message := fmt.Sprintf("must match annotations[%s] when both are specified", core.DeprecatedAnnotationTopologyAwareHints)
+		allErrs = append(allErrs, field.Invalid(metaPath.Child("annotations").Key(core.AnnotationTopologyMode), topologyModeVal, message))
+	}
 
 	specPath := field.NewPath("spec")
 
diff --git a/pkg/apis/core/validation/validation_test.go b/pkg/apis/core/validation/validation_test.go
index ab3ff729d03..1093ad0a507 100644
--- a/pkg/apis/core/validation/validation_test.go
+++ b/pkg/apis/core/validation/validation_test.go
@@ -15961,6 +15961,14 @@ func TestValidateServiceCreate(t *testing.T) {
 			},
 			numErrs: 1,
 		},
+		{
+			name: "topology annotations are mismatched",
+			tweakSvc: func(s *core.Service) {
+				s.Annotations[core.DeprecatedAnnotationTopologyAwareHints] = "original"
+				s.Annotations[core.AnnotationTopologyMode] = "different"
+			},
+			numErrs: 1,
+		},
 	}
 
 	for _, tc := range testCases {
@@ -18597,6 +18605,14 @@ func TestValidateServiceUpdate(t *testing.T) {
 			},
 			numErrs: 0,
 		},
+		{
+			name: "topology annotations are mismatched",
+			tweakSvc: func(oldSvc, newSvc *core.Service) {
+				newSvc.Annotations[core.DeprecatedAnnotationTopologyAwareHints] = "original"
+				newSvc.Annotations[core.AnnotationTopologyMode] = "different"
+			},
+			numErrs: 1,
+		},
 	}
 
 	for _, tc := range testCases {