From fc5b459fcf12341a4045c37347b82891249c14fc Mon Sep 17 00:00:00 2001 From: Jimmi Dyson Date: Wed, 16 Jun 2021 18:53:36 +0100 Subject: [PATCH] fix: Retain service clusterIPs if set --- pkg/controller/sync/dispatch/retain.go | 13 ++- pkg/controller/sync/dispatch/retain_test.go | 119 ++++++++++++++++++++ pkg/kubefedctl/federate/federate.go | 1 + pkg/schedulingtypes/replicascheduler.go | 2 +- 4 files changed, 133 insertions(+), 2 deletions(-) diff --git a/pkg/controller/sync/dispatch/retain.go b/pkg/controller/sync/dispatch/retain.go index a258b16588..c22aabbbcc 100644 --- a/pkg/controller/sync/dispatch/retain.go +++ b/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") diff --git a/pkg/controller/sync/dispatch/retain_test.go b/pkg/controller/sync/dispatch/retain_test.go index 83ea57d59e..f2f2490cf2 100644 --- a/pkg/controller/sync/dispatch/retain_test.go +++ b/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) + } + }) + } +} diff --git a/pkg/kubefedctl/federate/federate.go b/pkg/kubefedctl/federate/federate.go index a0a9099b09..be5ac57fe8 100644 --- a/pkg/kubefedctl/federate/federate.go +++ b/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") } } diff --git a/pkg/schedulingtypes/replicascheduler.go b/pkg/schedulingtypes/replicascheduler.go index 05563cfb42..25e27968ca 100644 --- a/pkg/schedulingtypes/replicascheduler.go +++ b/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 }