From d13c9206061e9f95a4438e60148b5f6bb24c20d5 Mon Sep 17 00:00:00 2001
From: Tim Hockin <thockin@google.com>
Date: Mon, 12 Jul 2021 12:34:05 -0700
Subject: [PATCH] Svc: Move ETP clearing to dropTypeDependentFields

I  am not sure why ExternalTrafficPolicy was different, but this is more
consistent with other field clearing logic.
---
 pkg/registry/core/service/storage/rest.go | 23 +----------------------
 pkg/registry/core/service/strategy.go     | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 22 deletions(-)

diff --git a/pkg/registry/core/service/storage/rest.go b/pkg/registry/core/service/storage/rest.go
index 5c499be9a97..0b30eb7cf07 100644
--- a/pkg/registry/core/service/storage/rest.go
+++ b/pkg/registry/core/service/storage/rest.go
@@ -346,25 +346,6 @@ func shouldAllocateNodePorts(service *api.Service) bool {
 	return false
 }
 
-// externalTrafficPolicyUpdate adjusts ExternalTrafficPolicy during service update if needed.
-// It is necessary because we default ExternalTrafficPolicy field to different values.
-// (NodePort / LoadBalancer: default is Global; Other types: default is empty.)
-func externalTrafficPolicyUpdate(oldService, service *api.Service) {
-	var neededExternalTraffic, needsExternalTraffic bool
-	if oldService.Spec.Type == api.ServiceTypeNodePort ||
-		oldService.Spec.Type == api.ServiceTypeLoadBalancer {
-		neededExternalTraffic = true
-	}
-	if service.Spec.Type == api.ServiceTypeNodePort ||
-		service.Spec.Type == api.ServiceTypeLoadBalancer {
-		needsExternalTraffic = true
-	}
-	if neededExternalTraffic && !needsExternalTraffic {
-		// Clear ExternalTrafficPolicy to prevent confusion from ineffective field.
-		service.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyType("")
-	}
-}
-
 // healthCheckNodePortUpdate handles HealthCheckNodePort allocation/release
 // and adjusts HealthCheckNodePort during service update if needed.
 func (rs *REST) healthCheckNodePortUpdate(oldService, service *api.Service, nodePortOp *portallocator.PortAllocationOperation) (bool, error) {
@@ -390,13 +371,12 @@ func (rs *REST) healthCheckNodePortUpdate(oldService, service *api.Service, node
 		klog.Infof("Transition to non LoadBalancer type service or LoadBalancer type service with ExternalTrafficPolicy=Global")
 		klog.V(4).Infof("Releasing healthCheckNodePort: %d", oldHealthCheckNodePort)
 		nodePortOp.ReleaseDeferred(int(oldHealthCheckNodePort))
-		// Clear the HealthCheckNodePort field.
-		service.Spec.HealthCheckNodePort = 0
 
 	// Case 3: Remain in needs HealthCheckNodePort.
 	// Reject changing the value of the HealthCheckNodePort field.
 	case neededHealthCheckNodePort && needsHealthCheckNodePort:
 		if oldHealthCheckNodePort != newHealthCheckNodePort {
+			//FIXME: Let validation do this.
 			klog.Warningf("Attempt to change value of health check node port DENIED")
 			fldPath := field.NewPath("spec", "healthCheckNodePort")
 			el := field.ErrorList{field.Invalid(fldPath, newHealthCheckNodePort,
@@ -499,7 +479,6 @@ func (rs *REST) Update(ctx context.Context, name string, objInfo rest.UpdatedObj
 	if !success || err != nil {
 		return nil, false, err
 	}
-	externalTrafficPolicyUpdate(oldService, service)
 	if errs := validation.ValidateServiceExternalTrafficFieldsCombination(service); len(errs) > 0 {
 		return nil, false, errors.NewInvalid(api.Kind("Service"), service.Name, errs)
 	}
diff --git a/pkg/registry/core/service/strategy.go b/pkg/registry/core/service/strategy.go
index 53fe07cdbec..10f1f985339 100644
--- a/pkg/registry/core/service/strategy.go
+++ b/pkg/registry/core/service/strategy.go
@@ -509,6 +509,12 @@ func dropTypeDependentFields(newSvc *api.Service, oldSvc *api.Service) {
 		newSvc.Spec.LoadBalancerClass = nil
 	}
 
+	// If a user is switching to a type that doesn't need ExternalTrafficPolicy
+	// AND they did not change this field, it is safe to drop it.
+	if needsExternalTrafficPolicy(oldSvc) && !needsExternalTrafficPolicy(newSvc) && sameExternalTrafficPolicy(oldSvc, newSvc) {
+		newSvc.Spec.ExternalTrafficPolicy = api.ServiceExternalTrafficPolicyType("")
+	}
+
 	// NOTE: there are other fields like `selector` which we could wipe.
 	// Historically we did not wipe them and they are not allocated from
 	// finite pools, so we are (currently) choosing to leave them alone.
@@ -597,6 +603,14 @@ func sameLoadBalancerClass(oldSvc, newSvc *api.Service) bool {
 	return *oldSvc.Spec.LoadBalancerClass == *newSvc.Spec.LoadBalancerClass
 }
 
+func needsExternalTrafficPolicy(svc *api.Service) bool {
+	return svc.Spec.Type == api.ServiceTypeNodePort || svc.Spec.Type == api.ServiceTypeLoadBalancer
+}
+
+func sameExternalTrafficPolicy(oldSvc, newSvc *api.Service) bool {
+	return oldSvc.Spec.ExternalTrafficPolicy == newSvc.Spec.ExternalTrafficPolicy
+}
+
 // this func allows user to downgrade a service by just changing
 // IPFamilyPolicy to SingleStack
 func trimFieldsForDualStackDowngrade(newService, oldService *api.Service) {