fix: Retain service clusterIPs if set

Author: jimmidyson

pkg/controller/sync/dispatch/retain.go

@@ -59,7 +59,7 @@ func retainServiceFields(desiredObj, clusterObj *unstructured.Unstructured) erro
 
 	// ClusterIP and NodePort are allocated to Service by cluster, so retain the same if any while updating
 
-	// Retain clusterip
+	// Retain clusterip and clusterips
 	clusterIP, ok, err := unstructured.NestedString(clusterObj.Object, "spec", "clusterIP")
 	if err != nil {
 		return errors.Wrap(err, "Error retrieving clusterIP from cluster service")
@@ -71,6 +71,17 @@ func retainServiceFields(desiredObj, clusterObj *unstructured.Unstructured) erro
 			return errors.Wrap(err, "Error setting clusterIP for service")
 		}
 	}
+	clusterIPs, ok, err := unstructured.NestedStringSlice(clusterObj.Object, "spec", "clusterIPs")
+	if err != nil {
+		return errors.Wrap(err, "Error retrieving clusterIPs from cluster service")
+	}
+	// !ok could indicate that cluster ips was not assigned
+	if ok && len(clusterIPs) > 0 {
+		err := unstructured.SetNestedStringSlice(desiredObj.Object, clusterIPs, "spec", "clusterIPs")
+		if err != nil {
+			return errors.Wrap(err, "Error setting clusterIPs for service")
+		}
+	}
 
 	// Retain nodeports
 	clusterPorts, ok, err := unstructured.NestedSlice(clusterObj.Object, "spec", "ports")

pkg/controller/sync/dispatch/retain_test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package dispatch
 
 import (
+	"reflect"
 	"testing"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
@@ -171,3 +172,121 @@ func TestRetainHealthCheckNodePortInServiceFields(t *testing.T) {
 		})
 	}
 }
+
+func TestRetainClusterIPsInServiceFields(t *testing.T) {
+	tests := []struct {
+		name                    string
+		desiredObj              *unstructured.Unstructured
+		clusterObj              *unstructured.Unstructured
+		retainSucceed           bool
+		expectedClusterIPValue  *string
+		expectedClusterIPsValue []string
+	}{
+		{
+			"cluster object has no clusterIP or clusterIPs",
+			&unstructured.Unstructured{
+				Object: map[string]interface{}{},
+			},
+			&unstructured.Unstructured{
+				Object: map[string]interface{}{},
+			},
+			true,
+			nil,
+			nil,
+		},
+		{
+			"cluster object has clusterIP",
+			&unstructured.Unstructured{
+				Object: map[string]interface{}{},
+			},
+			&unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"spec": map[string]interface{}{
+						"clusterIP": -1000,
+					},
+				},
+			},
+			false,
+			nil,
+			nil,
+		},
+		{
+			"cluster object has clusterIP only",
+			&unstructured.Unstructured{
+				Object: map[string]interface{}{},
+			},
+			&unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"spec": map[string]interface{}{
+						"clusterIP": "1.2.3.4",
+					},
+				},
+			},
+			true,
+			pointer.String("1.2.3.4"),
+			nil,
+		},
+		{
+			"cluster object has clusterIPs only",
+			&unstructured.Unstructured{
+				Object: map[string]interface{}{},
+			},
+			&unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"spec": map[string]interface{}{
+						"clusterIPs": []interface{}{"1.2.3.4", "5.6.7.8"},
+					},
+				},
+			},
+			true,
+			nil,
+			[]string{"1.2.3.4", "5.6.7.8"},
+		},
+		{
+			"cluster object has both clusterIP and clusterIPs",
+			&unstructured.Unstructured{
+				Object: map[string]interface{}{},
+			},
+			&unstructured.Unstructured{
+				Object: map[string]interface{}{
+					"spec": map[string]interface{}{
+						"clusterIP":  "1.2.3.4",
+						"clusterIPs": []interface{}{"5.6.7.8", "9.10.11.12"},
+					},
+				},
+			},
+			true,
+			pointer.String("1.2.3.4"),
+			[]string{"5.6.7.8", "9.10.11.12"},
+		},
+	}
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			if err := retainServiceFields(test.desiredObj, test.clusterObj); (err == nil) != test.retainSucceed {
+				t.Fatalf("test %s fails: unexpected returned error %v", test.name, err)
+			}
+
+			currentClusterIPValue, ok, err := unstructured.NestedString(test.desiredObj.Object, "spec", "clusterIP")
+			if err != nil {
+				t.Fatalf("test %s fails: %v", test.name, err)
+			}
+			if !ok && test.expectedClusterIPValue != nil {
+				t.Fatalf("test %s fails: expect specified clusterIP but not found", test.name)
+			}
+			if ok && (test.expectedClusterIPValue == nil || *test.expectedClusterIPValue != currentClusterIPValue) {
+				t.Fatalf("test %s fails: unexpected current clusterIP %s", test.name, currentClusterIPValue)
+			}
+
+			currentClusterIPsValue, ok, err := unstructured.NestedStringSlice(test.desiredObj.Object, "spec", "clusterIPs")
+			if err != nil {
+				t.Fatalf("test %s fails: %v", test.name, err)
+			}
+			if !ok && test.expectedClusterIPsValue != nil {
+				t.Fatalf("test %s fails: expect specified clusterIPs but not found", test.name)
+			}
+			if ok && !reflect.DeepEqual(test.expectedClusterIPsValue, currentClusterIPsValue) {
+				t.Fatalf("test %s fails: unexpected current clusterIPs %v", test.name, currentClusterIPsValue)
+			}
+		})
+	}
+}

pkg/kubefedctl/federate/federate.go

@@ -390,6 +390,7 @@ func FederatedResourceFromTargetResource(typeConfig typeconfig.Interface, resour
 				}
 			}
 			unstructured.RemoveNestedField(targetResource.Object, "spec", "clusterIP")
+			unstructured.RemoveNestedField(targetResource.Object, "spec", "clusterIPs")
 		}
 	}
 

pkg/schedulingtypes/replicascheduler.go

@@ -198,7 +198,7 @@ func (s *ReplicaScheduler) Reconcile(obj runtimeclient.Object, qualifiedName ctl
 
 		resultClusters, err := plugin.(*Plugin).GetResourceClusters(qualifiedName, fedClusters)
 		if err != nil {
-			runtime.HandleError(errors.Wrapf(err, "Failed to get prefrerred clusters while reconciling RSP named %q", key))
+			runtime.HandleError(errors.Wrapf(err, "Failed to get preferred clusters while reconciling RSP named %q", key))
 			return ctlutil.StatusError
 		}