Use ResourceRecordSetsEquivalent() instead of == to compare rrsets. Fixes #28135

federation/pkg/dnsprovider/dns.go

@@ -16,7 +16,11 @@ limitations under the License.
 
 package dnsprovider
 
-import "k8s.io/kubernetes/federation/pkg/dnsprovider/rrstype"
+import (
+	"reflect"
+
+	"k8s.io/kubernetes/federation/pkg/dnsprovider/rrstype"
+)
 
 // Interface is an abstract, pluggable interface for DNS providers.
 type Interface interface {
@@ -66,3 +70,19 @@ type ResourceRecordSet interface {
 	// Type returns the type of the record set (A, CNAME, SRV, etc)
 	Type() rrstype.RrsType
 }
+
+/* ResourceRecordSetsEquivalent compares two ResourceRecordSets for semantic equivalence.
+   Go's equality operator doesn't work the way we want it to in this case,
+   hence the need for this function.
+   More specifically (from the Go spec):
+   "Two struct values are equal if their corresponding non-blank fields are equal."
+   In our case, there may be some private internal member variables that may not be not equal,
+   but we want the two structs to be considered equivalent anyway, if the fields exposed
+   via their interfaces are equal.
+*/
+func ResourceRecordSetsEquivalent(r1, r2 ResourceRecordSet) bool {
+	if r1.Name() == r2.Name() && reflect.DeepEqual(r1.Rrdatas(), r2.Rrdatas()) && r1.Ttl() == r2.Ttl() && r1.Type() == r2.Type() {
+		return true
+	}
+	return false
+}

federation/pkg/dnsprovider/dns_test.go

@@ -0,0 +1,96 @@
+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package dnsprovider
+
+import (
+	"testing"
+
+	"k8s.io/kubernetes/federation/pkg/dnsprovider/rrstype"
+)
+
+// Compile time interface check
+var _ ResourceRecordSet = record{}
+
+type record struct {
+	name    string
+	rrdatas []string
+	ttl     int64
+	type_   string
+}
+
+func (r record) Name() string {
+	return r.name
+}
+
+func (r record) Ttl() int64 {
+	return r.ttl
+}
+
+func (r record) Rrdatas() []string {
+	return r.rrdatas
+}
+
+func (r record) Type() rrstype.RrsType {
+	return rrstype.RrsType(r.type_)
+}
+
+const testDNSZone string = "foo.com"
+
+var testData = []struct {
+	inputs         [2]record
+	expectedOutput bool
+}{
+	{
+		[2]record{
+			{"foo", []string{"1.2.3.4", "5,6,7,8"}, 180, "A"}, // Identical
+			{"foo", []string{"1.2.3.4", "5,6,7,8"}, 180, "A"}}, true,
+	},
+	{
+		[2]record{
+			{"foo", []string{"1.2.3.4", "5,6,7,8"}, 180, "A"}, // Identical except Name
+			{"bar", []string{"1.2.3.4", "5,6,7,8"}, 180, "A"}}, false,
+	},
+	{
+		[2]record{
+			{"foo", []string{"1.2.3.4", "5,6,7,8"}, 180, "A"}, // Identical except Rrdata
+			{"foo", []string{"1.2.3.4", "5,6,7,9"}, 180, "A"}}, false,
+	},
+	{
+		[2]record{
+			{"foo", []string{"1.2.3.4", "5,6,7,8"}, 180, "A"}, // Identical except Rrdata ordering reversed
+			{"foo", []string{"5,6,7,8", "1.2.3.4"}, 180, "A"}}, false,
+	},
+	{
+		[2]record{
+			{"foo", []string{"1.2.3.4", "5,6,7,8"}, 180, "A"}, // Identical except TTL
+			{"foo", []string{"1.2.3.4", "5,6,7,8"}, 150, "A"}}, false,
+	},
+	{
+		[2]record{
+			{"foo", []string{"1.2.3.4", "5,6,7,8"}, 180, "A"}, // Identical except Type
+			{"foo", []string{"1.2.3.4", "5,6,7,8"}, 180, "CNAME"}}, false,
+	},
+}
+
+func TestEquivalent(t *testing.T) {
+	for _, test := range testData {
+		output := ResourceRecordSetsEquivalent(test.inputs[0], test.inputs[1])
+		if output != test.expectedOutput {
+			t.Errorf("Expected equivalence comparison of %q and %q to yield %v, but it vielded %v", test.inputs[0], test.inputs[1], test.expectedOutput, output)
+		}
+	}
+}

federation/pkg/federation-controller/service/dns.go

@@ -205,14 +205,14 @@ func (s *ServiceController) ensureDnsRrsets(dnsZoneName, dnsName string, endpoin
 			// Need an appropriate CNAME record.  Check that we have it.
 			newRrset := rrsets.New(dnsName, []string{uplevelCname}, minDnsTtl, rrstype.CNAME)
 			glog.V(4).Infof("No healthy endpoints for %s.  Have recordset %v. Need recordset %v", dnsName, rrset, newRrset)
-			if rrset == newRrset {
-				// The existing rrset is equal to the required one - our work is done here
-				glog.V(4).Infof("Existing recordset %v is equal to needed recordset %v, our work is done here.", rrset, newRrset)
+			if dnsprovider.ResourceRecordSetsEquivalent(rrset, newRrset) {
+				// The existing rrset is equivalent to the required one - our work is done here
+				glog.V(4).Infof("Existing recordset %v is equivalent to needed recordset %v, our work is done here.", rrset, newRrset)
 				return nil
 			} else {
 				// Need to replace the existing one with a better one (or just remove it if we have no healthy endpoints).
 				// TODO: Ideally do these inside a transaction, or do an atomic update, but dnsprovider interface doesn't support that yet.
-				glog.V(4).Infof("Existing recordset %v not equal to needed recordset %v removing existing and adding needed.", rrset, newRrset)
+				glog.V(4).Infof("Existing recordset %v not equivalent to needed recordset %v removing existing and adding needed.", rrset, newRrset)
 				if err = rrsets.Remove(rrset); err != nil {
 					return err
 				}
@@ -236,15 +236,15 @@ func (s *ServiceController) ensureDnsRrsets(dnsZoneName, dnsName string, endpoin
 			}
 			newRrset := rrsets.New(dnsName, resolvedEndpoints, minDnsTtl, rrstype.A)
 			glog.V(4).Infof("Have recordset %v. Need recordset %v", rrset, newRrset)
-			if rrset == newRrset {
-				glog.V(4).Infof("Existing recordset %v is equal to needed recordset %v, our work is done here.", rrset, newRrset)
+			if dnsprovider.ResourceRecordSetsEquivalent(rrset, newRrset) {
+				glog.V(4).Infof("Existing recordset %v is equivalent to needed recordset %v, our work is done here.", rrset, newRrset)
 				// TODO: We could be more thorough about checking for equivalence to avoid unnecessary updates, but in the
 				//       worst case we'll just replace what's there with an equivalent, if not exactly identical record set.
 				return nil
 			} else {
 				// Need to replace the existing one with a better one
 				// TODO: Ideally do these inside a transaction, or do an atomic update, but dnsprovider interface doesn't support that yet.
-				glog.V(4).Infof("Existing recordset %v is not equal to needed recordset %v, removing existing and adding needed.", rrset, newRrset)
+				glog.V(4).Infof("Existing recordset %v is not equivalent to needed recordset %v, removing existing and adding needed.", rrset, newRrset)
 				if err = rrsets.Remove(rrset); err != nil {
 					return err
 				}