From 704d13bfc8c13c85541df024f81533391e5cd64e Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Mon, 17 Apr 2017 18:31:41 -0700 Subject: [PATCH] Modify the DNS provider Rrset.Get(name) interface to return multiple records and update federated service controller. There can be multiple DNS resource records for a given name. They can vary by type, ttl, rrdata and a number of various other parameters. It is incorrect to return a single resource record for a given name. This change updates the Get interface to return multiple records for a given name and uses this list in the federated service controller to perform DNS operations. --- federation/pkg/dnsprovider/dns.go | 8 ++- .../providers/aws/route53/rrsets.go | 31 +++++++---- .../providers/coredns/coredns_test.go | 10 ++-- .../dnsprovider/providers/coredns/rrsets.go | 16 +++--- .../internal/interfaces/interfaces.go | 4 +- .../clouddns/internal/rrsets_service.go | 4 ++ .../clouddns/internal/stubs/rrsets_service.go | 44 +++++++++++---- .../providers/google/clouddns/rrsets.go | 24 +++++---- .../pkg/dnsprovider/tests/commontests.go | 18 ++++--- .../pkg/federation-controller/service/dns.go | 53 ++++++++++++------- 10 files changed, 138 insertions(+), 74 deletions(-) diff --git a/federation/pkg/dnsprovider/dns.go b/federation/pkg/dnsprovider/dns.go index f30f51627c7..d62a3fc51ed 100644 --- a/federation/pkg/dnsprovider/dns.go +++ b/federation/pkg/dnsprovider/dns.go @@ -52,8 +52,12 @@ type Zone interface { type ResourceRecordSets interface { // List returns the ResourceRecordSets of the Zone, or an error if the list operation failed. List() ([]ResourceRecordSet, error) - // Get returns the ResourceRecordSet with the name in the Zone. if the named resource record set does not exist, but no error occurred, the returned set, and error, are both nil. - Get(name string) (ResourceRecordSet, error) + // Get returns the ResourceRecordSet list with the name in the Zone. + // This is a list because there might be multiple records of different + // types for a given name. If the named resource record sets do not + // exist, but no error occurred, the returned set will be an empty set + // and error will be nil. + Get(name string) ([]ResourceRecordSet, error) // New allocates a new ResourceRecordSet, which can then be passed to ResourceRecordChangeset Add() or Remove() // Arguments are as per the ResourceRecordSet interface below. New(name string, rrdatas []string, ttl int64, rrstype rrstype.RrsType) ResourceRecordSet diff --git a/federation/pkg/dnsprovider/providers/aws/route53/rrsets.go b/federation/pkg/dnsprovider/providers/aws/route53/rrsets.go index 81b16dda61a..67470c8778f 100644 --- a/federation/pkg/dnsprovider/providers/aws/route53/rrsets.go +++ b/federation/pkg/dnsprovider/providers/aws/route53/rrsets.go @@ -48,19 +48,30 @@ func (rrsets ResourceRecordSets) List() ([]dnsprovider.ResourceRecordSet, error) return list, nil } -func (rrsets ResourceRecordSets) Get(name string) (dnsprovider.ResourceRecordSet, error) { - var newRrset dnsprovider.ResourceRecordSet - rrsetList, err := rrsets.List() +func (rrsets ResourceRecordSets) Get(name string) ([]dnsprovider.ResourceRecordSet, error) { + // This list implementation is very similar to the one implemented in + // the List() method above, but it restricts the retrieved list to + // the records whose name match the given `name`. + input := route53.ListResourceRecordSetsInput{ + HostedZoneId: rrsets.zone.impl.Id, + StartRecordName: aws.String(name), + } + + var list []dnsprovider.ResourceRecordSet + err := rrsets.zone.zones.interface_.service.ListResourceRecordSetsPages(&input, func(page *route53.ListResourceRecordSetsOutput, lastPage bool) bool { + for _, rrset := range page.ResourceRecordSets { + if aws.StringValue(rrset.Name) != name { + return false + } + list = append(list, &ResourceRecordSet{rrset, &rrsets}) + } + return true + }) if err != nil { return nil, err } - for _, rrset := range rrsetList { - if rrset.Name() == name { - newRrset = rrset - break - } - } - return newRrset, nil + + return list, nil } func (r ResourceRecordSets) StartChangeset() dnsprovider.ResourceRecordChangeset { diff --git a/federation/pkg/dnsprovider/providers/coredns/coredns_test.go b/federation/pkg/dnsprovider/providers/coredns/coredns_test.go index 51da32bee5f..031ac0c4e61 100644 --- a/federation/pkg/dnsprovider/providers/coredns/coredns_test.go +++ b/federation/pkg/dnsprovider/providers/coredns/coredns_test.go @@ -124,16 +124,16 @@ func listRrsOrFail(t *testing.T, rrsets dnsprovider.ResourceRecordSets) []dnspro return rrset } -func getRrOrFail(t *testing.T, rrsets dnsprovider.ResourceRecordSets, name string) dnsprovider.ResourceRecordSet { - rrset, err := rrsets.Get(name) +func getRrOrFail(t *testing.T, rrsets dnsprovider.ResourceRecordSets, name string) []dnsprovider.ResourceRecordSet { + rrsetList, err := rrsets.Get(name) if err != nil { t.Fatalf("Failed to get recordset: %v", err) - } else if rrset == nil { + } else if len(rrsetList) == 0 { t.Logf("Did not Get recordset: %v", name) } else { - t.Logf("Got recordset: %v", rrset.Name()) + t.Logf("Got recordsets: %v", rrsetList) } - return rrset + return rrsetList } func getExampleRrs(zone dnsprovider.Zone) dnsprovider.ResourceRecordSet { diff --git a/federation/pkg/dnsprovider/providers/coredns/rrsets.go b/federation/pkg/dnsprovider/providers/coredns/rrsets.go index 6ff6235e3f4..63d08a5598d 100644 --- a/federation/pkg/dnsprovider/providers/coredns/rrsets.go +++ b/federation/pkg/dnsprovider/providers/coredns/rrsets.go @@ -40,7 +40,7 @@ func (rrsets ResourceRecordSets) List() ([]dnsprovider.ResourceRecordSet, error) return list, fmt.Errorf("OperationNotSupported") } -func (rrsets ResourceRecordSets) Get(name string) (dnsprovider.ResourceRecordSet, error) { +func (rrsets ResourceRecordSets) Get(name string) ([]dnsprovider.ResourceRecordSet, error) { getOpts := &etcdc.GetOptions{ Recursive: true, } @@ -58,17 +58,16 @@ func (rrsets ResourceRecordSets) Get(name string) (dnsprovider.ResourceRecordSet return nil, nil } - rrset := ResourceRecordSet{name: name, rrdatas: []string{}, rrsets: &rrsets} - found := false + var list []dnsprovider.ResourceRecordSet + for _, node := range response.Node.Nodes { - found = true service := dnsmsg.Service{} err = json.Unmarshal([]byte(node.Value), &service) if err != nil { return nil, fmt.Errorf("Failed to unmarshall json data, err: %v", err) } - // assuming all rrdatas in a rrset will have same type + rrset := ResourceRecordSet{name: name, rrdatas: []string{}, rrsets: &rrsets} ip := net.ParseIP(service.Host) switch { case ip == nil: @@ -78,13 +77,10 @@ func (rrsets ResourceRecordSets) Get(name string) (dnsprovider.ResourceRecordSet } rrset.rrdatas = append(rrset.rrdatas, service.Host) rrset.ttl = int64(service.TTL) + list = append(list, rrset) } - if !found { - return nil, nil - } - - return rrset, nil + return list, nil } func (rrsets ResourceRecordSets) StartChangeset() dnsprovider.ResourceRecordChangeset { diff --git a/federation/pkg/dnsprovider/providers/google/clouddns/internal/interfaces/interfaces.go b/federation/pkg/dnsprovider/providers/google/clouddns/internal/interfaces/interfaces.go index 5fd51908394..c508f4c1116 100644 --- a/federation/pkg/dnsprovider/providers/google/clouddns/internal/interfaces/interfaces.go +++ b/federation/pkg/dnsprovider/providers/google/clouddns/internal/interfaces/interfaces.go @@ -191,8 +191,10 @@ type ( } ResourceRecordSetsService interface { - // NewResourceRecordSetsService(s *Service) *ResourceRecordSetsService // TODO: add to service as needed List(project string, managedZone string) ResourceRecordSetsListCall + // Get returns a list of resources records with the matching name + Get(project, managedZone, name string) ResourceRecordSetsListCall + // NewResourceRecordSetsService(s *Service) *ResourceRecordSetsService // TODO: add to service as needed NewResourceRecordSet(name string, rrdatas []string, ttl int64, type_ rrstype.RrsType) ResourceRecordSet } diff --git a/federation/pkg/dnsprovider/providers/google/clouddns/internal/rrsets_service.go b/federation/pkg/dnsprovider/providers/google/clouddns/internal/rrsets_service.go index 0b3f647db63..a971fd7513c 100644 --- a/federation/pkg/dnsprovider/providers/google/clouddns/internal/rrsets_service.go +++ b/federation/pkg/dnsprovider/providers/google/clouddns/internal/rrsets_service.go @@ -33,6 +33,10 @@ func (service ResourceRecordSetsService) List(project string, managedZone string return &ResourceRecordSetsListCall{service.impl.List(project, managedZone)} } +func (service ResourceRecordSetsService) Get(project, managedZone, name string) interfaces.ResourceRecordSetsListCall { + return &ResourceRecordSetsListCall{service.impl.List(project, managedZone).Name(name)} +} + func (service ResourceRecordSetsService) NewResourceRecordSet(name string, rrdatas []string, ttl int64, type_ rrstype.RrsType) interfaces.ResourceRecordSet { rrset := dns.ResourceRecordSet{Name: name, Rrdatas: rrdatas, Ttl: ttl, Type: string(type_)} return &ResourceRecordSet{&rrset} diff --git a/federation/pkg/dnsprovider/providers/google/clouddns/internal/stubs/rrsets_service.go b/federation/pkg/dnsprovider/providers/google/clouddns/internal/stubs/rrsets_service.go index 8c43e98f5e4..c7229abd616 100644 --- a/federation/pkg/dnsprovider/providers/google/clouddns/internal/stubs/rrsets_service.go +++ b/federation/pkg/dnsprovider/providers/google/clouddns/internal/stubs/rrsets_service.go @@ -31,21 +31,27 @@ type ResourceRecordSetsService struct { ListCall interfaces.ResourceRecordSetsListCall // Use to override response if required for testing } +func (s ResourceRecordSetsService) managedZone(project, managedZone string) (*ManagedZone, error) { + p := s.Service.ManagedZones_.Impl[project] + if p == nil { + return nil, fmt.Errorf("Project not found: %s", project) + } + z := s.Service.ManagedZones_.Impl[project][managedZone] + if z == nil { + return nil, fmt.Errorf("Zone %s not found in project %s", managedZone, project) + } + return s.Service.ManagedZones_.Impl[project][managedZone].(*ManagedZone), nil +} + func (s ResourceRecordSetsService) List(project string, managedZone string) interfaces.ResourceRecordSetsListCall { if s.ListCall != nil { return s.ListCall } - p := s.Service.ManagedZones_.Impl[project] - if p == nil { - return &ResourceRecordSetsListCall{Err_: fmt.Errorf("Project not found: %s", project)} + zone, err := s.managedZone(project, managedZone) + if err != nil { + return &ResourceRecordSetsListCall{Err_: err} } - z := s.Service.ManagedZones_.Impl[project][managedZone] - if z == nil { - return &ResourceRecordSetsListCall{ - Err_: fmt.Errorf("Zone %s not found in project %s", managedZone, project), - } - } - zone := s.Service.ManagedZones_.Impl[project][managedZone].(*ManagedZone) + response := &ResourceRecordSetsListResponse{} for _, set := range zone.Rrsets { response.impl = append(response.impl, set) @@ -53,6 +59,24 @@ func (s ResourceRecordSetsService) List(project string, managedZone string) inte return &ResourceRecordSetsListCall{Response_: response} } +func (s ResourceRecordSetsService) Get(project, managedZone, name string) interfaces.ResourceRecordSetsListCall { + if s.ListCall != nil { + return s.ListCall + } + zone, err := s.managedZone(project, managedZone) + if err != nil { + return &ResourceRecordSetsListCall{Err_: err} + } + + response := &ResourceRecordSetsListResponse{} + for _, set := range zone.Rrsets { + if set.Name_ == name { + response.impl = append(response.impl, set) + } + } + return &ResourceRecordSetsListCall{Response_: response} +} + func (service ResourceRecordSetsService) NewResourceRecordSet(name string, rrdatas []string, ttl int64, type_ rrstype.RrsType) interfaces.ResourceRecordSet { rrset := ResourceRecordSet{Name_: name, Rrdatas_: rrdatas, Ttl_: ttl, Type_: string(type_)} return rrset diff --git a/federation/pkg/dnsprovider/providers/google/clouddns/rrsets.go b/federation/pkg/dnsprovider/providers/google/clouddns/rrsets.go index 396ba2a1a29..c7cfa9731dd 100644 --- a/federation/pkg/dnsprovider/providers/google/clouddns/rrsets.go +++ b/federation/pkg/dnsprovider/providers/google/clouddns/rrsets.go @@ -42,19 +42,23 @@ func (rrsets ResourceRecordSets) List() ([]dnsprovider.ResourceRecordSet, error) return list, nil } -func (rrsets ResourceRecordSets) Get(name string) (dnsprovider.ResourceRecordSet, error) { - var newRrset dnsprovider.ResourceRecordSet - rrsetList, err := rrsets.List() +func (rrsets ResourceRecordSets) Get(name string) ([]dnsprovider.ResourceRecordSet, error) { + var list []dnsprovider.ResourceRecordSet + + ctx := context.Background() + + call := rrsets.impl.Get(rrsets.project(), rrsets.zone.impl.Name(), name) + err := call.Pages(ctx, func(page interfaces.ResourceRecordSetsListResponse) error { + for _, rrset := range page.Rrsets() { + list = append(list, ResourceRecordSet{rrset, &rrsets}) + } + return nil + }) if err != nil { return nil, err } - for _, rrset := range rrsetList { - if rrset.Name() == name { - newRrset = rrset - break - } - } - return newRrset, nil + + return list, nil } func (r ResourceRecordSets) StartChangeset() dnsprovider.ResourceRecordChangeset { diff --git a/federation/pkg/dnsprovider/tests/commontests.go b/federation/pkg/dnsprovider/tests/commontests.go index 9b604699a1d..02d8938539a 100644 --- a/federation/pkg/dnsprovider/tests/commontests.go +++ b/federation/pkg/dnsprovider/tests/commontests.go @@ -108,16 +108,16 @@ func rrs(t *testing.T, zone dnsprovider.Zone) (r dnsprovider.ResourceRecordSets) return rrsets } -func getRrOrFail(t *testing.T, rrsets dnsprovider.ResourceRecordSets, name string) dnsprovider.ResourceRecordSet { - rrset, err := rrsets.Get(name) +func getRrOrFail(t *testing.T, rrsets dnsprovider.ResourceRecordSets, name string) []dnsprovider.ResourceRecordSet { + rrsetList, err := rrsets.Get(name) if err != nil { t.Fatalf("Failed to get recordset: %v", err) - } else if rrset == nil { + } else if len(rrsetList) == 0 { t.Logf("Did not Get recordset: %v", name) } else { - t.Logf("Got recordset: %v", rrset.Name()) + t.Logf("Got recordset: %v", rrsetList[0].Name()) } - return rrset + return rrsetList } // assertHasRecord tests that rrsets has a record equivalent to rrset @@ -127,7 +127,13 @@ func assertHasRecord(t *testing.T, rrsets dnsprovider.ResourceRecordSets, rrset rrs, err := rrsets.List() if err != nil { if err.Error() == "OperationNotSupported" { - found = getRrOrFail(t, rrsets, rrset.Name()) + foundList := getRrOrFail(t, rrsets, rrset.Name()) + for i, elem := range foundList { + if elem.Name() == rrset.Name() && elem.Type() == rrset.Type() { + found = foundList[i] + break + } + } } else { t.Fatalf("Failed to list recordsets: %v", err) } diff --git a/federation/pkg/federation-controller/service/dns.go b/federation/pkg/federation-controller/service/dns.go index 789a45f162c..7104bb72720 100644 --- a/federation/pkg/federation-controller/service/dns.go +++ b/federation/pkg/federation-controller/service/dns.go @@ -154,11 +154,22 @@ func getDnsZone(dnsZoneName string, dnsZoneID string, dnsZonesInterface dnsprovi } } -// Note that if the named resource record set does not exist, but no error occurred, the returned set, and error, are both nil -func getRrset(dnsName string, rrsetsInterface dnsprovider.ResourceRecordSets) (dnsprovider.ResourceRecordSet, error) { +// NOTE: that if the named resource record set does not exist, but no +// error occurred, the returned list will be empty, and the error will +// be nil +func getRrset(dnsName string, rrsetsInterface dnsprovider.ResourceRecordSets) ([]dnsprovider.ResourceRecordSet, error) { return rrsetsInterface.Get(dnsName) } +func findRrset(list []dnsprovider.ResourceRecordSet, rrset dnsprovider.ResourceRecordSet) dnsprovider.ResourceRecordSet { + for i, elem := range list { + if dnsprovider.ResourceRecordSetsEquivalent(rrset, elem) { + return list[i] + } + } + return nil +} + /* getResolvedEndpoints performs DNS resolution on the provided slice of endpoints (which might be DNS names or IPv4 addresses) and returns a list of IPv4 addresses. If any of the endpoints are neither valid IPv4 addresses nor resolvable DNS names, non-nil error is also returned (possibly along with a partially complete list of resolved endpoints. @@ -190,11 +201,11 @@ func (s *ServiceController) ensureDnsRrsets(dnsZone dnsprovider.Zone, dnsName st if !supported { return fmt.Errorf("Failed to ensure DNS records for %s. DNS provider does not support the ResourceRecordSets interface.", dnsName) } - rrset, err := getRrset(dnsName, rrsets) // TODO: rrsets.Get(dnsName) + rrsetList, err := getRrset(dnsName, rrsets) // TODO: rrsets.Get(dnsName) if err != nil { return err } - if rrset == nil { + if len(rrsetList) == 0 { glog.V(4).Infof("No recordsets found for DNS name %q. Need to add either A records (if we have healthy endpoints), or a CNAME record to %q", dnsName, uplevelCname) if len(endpoints) < 1 { glog.V(4).Infof("There are no healthy endpoint addresses at level %q, so CNAME to %q, if provided", dnsName, uplevelCname) @@ -228,57 +239,59 @@ func (s *ServiceController) ensureDnsRrsets(dnsZone dnsprovider.Zone, dnsName st glog.V(4).Infof("Successfully added recordset %v", newRrset) } } else { - // the rrset already exists, so make it right. - glog.V(4).Infof("Recordset %v already exists. Ensuring that it is correct.", rrset) + // the rrsets already exists, so make it right. + glog.V(4).Infof("Recordset %v already exists. Ensuring that it is correct.", rrsetList) if len(endpoints) < 1 { // 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 dnsprovider.ResourceRecordSetsEquivalent(rrset, newRrset) { + glog.V(4).Infof("No healthy endpoints for %s. Have recordsets %v. Need recordset %v", dnsName, rrsetList, newRrset) + found := findRrset(rrsetList, newRrset) + if found != nil { // 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) + glog.V(4).Infof("Existing recordset %v is equivalent to needed recordset %v, our work is done here.", rrsetList, newRrset) return nil } else { // Need to replace the existing one with a better one (or just remove it if we have no healthy endpoints). - glog.V(4).Infof("Existing recordset %v not equivalent 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.", rrsetList, newRrset) changeSet := rrsets.StartChangeset() - changeSet.Remove(rrset) + changeSet.Remove(found) if uplevelCname != "" { changeSet.Add(newRrset) if err := changeSet.Apply(); err != nil { return err } - glog.V(4).Infof("Successfully replaced needed recordset %v -> %v", rrset, newRrset) + glog.V(4).Infof("Successfully replaced needed recordset %v -> %v", found, newRrset) } else { if err := changeSet.Apply(); err != nil { return err } - glog.V(4).Infof("Successfully removed existing recordset %v", rrset) + glog.V(4).Infof("Successfully removed existing recordset %v", found) glog.V(4).Infof("Uplevel CNAME is empty string. Not adding recordset %v", newRrset) } } } else { // We have an rrset in DNS, possibly with some missing addresses and some unwanted addresses. // And we have healthy endpoints. Just replace what's there with the healthy endpoints, if it's not already correct. - glog.V(4).Infof("%s: Healthy endpoints %v exist. Recordset %v exists. Reconciling.", dnsName, endpoints, rrset) + glog.V(4).Infof("%s: Healthy endpoints %v exist. Recordset %v exists. Reconciling.", dnsName, endpoints, rrsetList) resolvedEndpoints, err := getResolvedEndpoints(endpoints) if err != nil { // Some invalid addresses or otherwise unresolvable DNS names. return err // TODO: We could potentially add the ones we did get back, even if some of them failed to resolve. } newRrset := rrsets.New(dnsName, resolvedEndpoints, minDnsTtl, rrstype.A) - glog.V(4).Infof("Have recordset %v. Need recordset %v", 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) + glog.V(4).Infof("Have recordset %v. Need recordset %v", rrsetList, newRrset) + found := findRrset(rrsetList, newRrset) + if found != nil { + glog.V(4).Infof("Existing recordset %v is equivalent to needed recordset %v, our work is done here.", found, 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 - glog.V(4).Infof("Existing recordset %v is not equivalent to needed recordset %v, removing existing and adding needed.", rrset, newRrset) - if err = rrsets.StartChangeset().Remove(rrset).Add(newRrset).Apply(); err != nil { + glog.V(4).Infof("Existing recordset %v is not equivalent to needed recordset %v, removing existing and adding needed.", found, newRrset) + if err = rrsets.StartChangeset().Remove(found).Add(newRrset).Apply(); err != nil { return err } - glog.V(4).Infof("Successfully replaced recordset %v -> %v", rrset, newRrset) + glog.V(4).Infof("Successfully replaced recordset %v -> %v", found, newRrset) } } }