From 3dd1c0088e94acd95958709e22f1d5e598a57d51 Mon Sep 17 00:00:00 2001 From: Quinton Hoole Date: Tue, 28 Jun 2016 13:30:33 -0700 Subject: [PATCH] Use ResourceRecordSetsEquivalent() instead of == to compare rrsets. Fixes #28135 --- federation/pkg/dnsprovider/dns.go | 22 ++++- federation/pkg/dnsprovider/dns_test.go | 96 +++++++++++++++++++ .../pkg/federation-controller/service/dns.go | 14 +-- 3 files changed, 124 insertions(+), 8 deletions(-) create mode 100644 federation/pkg/dnsprovider/dns_test.go diff --git a/federation/pkg/dnsprovider/dns.go b/federation/pkg/dnsprovider/dns.go index ade3e6a43b9..6a04c35dd9d 100644 --- a/federation/pkg/dnsprovider/dns.go +++ b/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 +} diff --git a/federation/pkg/dnsprovider/dns_test.go b/federation/pkg/dnsprovider/dns_test.go new file mode 100644 index 00000000000..b5a2344c35b --- /dev/null +++ b/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) + } + } +} diff --git a/federation/pkg/federation-controller/service/dns.go b/federation/pkg/federation-controller/service/dns.go index 63454d3da8b..9b51f62197a 100644 --- a/federation/pkg/federation-controller/service/dns.go +++ b/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 }