From d61cd1bf2cef51af8f4a8f0503cc6756da28e4d2 Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Tue, 28 May 2019 17:21:07 -0400 Subject: [PATCH 1/3] Refactor desired DNS functions Refactor DNS handling to consolidate how the desired set of DNS records is computed. This ensures that the same function can be used for both setting up and tearing down DNS. Add some unit testing around the desired DNS function. --- pkg/operator/controller/controller_dns.go | 103 ++++++++---------- .../controller/controller_dns_test.go | 0 pkg/operator/controller/controller_lb.go | 26 ++--- 3 files changed, 55 insertions(+), 74 deletions(-) create mode 100644 pkg/operator/controller/controller_dns_test.go diff --git a/pkg/operator/controller/controller_dns.go b/pkg/operator/controller/controller_dns.go index bddee0f53e..cf596faf32 100644 --- a/pkg/operator/controller/controller_dns.go +++ b/pkg/operator/controller/controller_dns.go @@ -14,31 +14,8 @@ import ( // ensureDNS will create DNS records for the given LB service. If service is // nil, nothing is done. func (r *reconciler) ensureDNS(ci *operatorv1.IngressController, service *corev1.Service, dnsConfig *configv1.DNS) error { - // If no load balancer has been provisioned, we can't do anything with the - // configured DNS zones. - ingress := service.Status.LoadBalancer.Ingress - if len(ingress) == 0 || (len(ingress[0].Hostname) == 0 && len(ingress[0].IP) == 0) { - return fmt.Errorf("no load balancer is assigned to service %s/%s", service.Namespace, service.Name) - } - - var dnsRecords []*dns.Record - if len(ingress[0].Hostname) != 0 { - aliasRecords, err := desiredDNSAliasRecords(ci, ingress[0].Hostname, dnsConfig) - if err != nil { - return err - } - dnsRecords = append(dnsRecords, aliasRecords...) - } - - if len(ingress[0].IP) != 0 { - aRecords, err := desiredDNSARecords(ci, ingress[0].IP, dnsConfig) - if err != nil { - return err - } - dnsRecords = append(dnsRecords, aRecords...) - } - - for _, record := range dnsRecords { + records := desiredDNSRecords(ci, dnsConfig, service) + for _, record := range records { err := r.DNSManager.Ensure(record) if err != nil { return fmt.Errorf("failed to ensure DNS record %v for %s/%s: %v", record, ci.Namespace, ci.Name, err) @@ -48,65 +25,75 @@ func (r *reconciler) ensureDNS(ci *operatorv1.IngressController, service *corev1 return nil } -type makeRecordFunc func(zone *configv1.DNSZone) *dns.Record - -func desiredDNSAliasRecords(ci *operatorv1.IngressController, hostname string, dnsConfig *configv1.DNS) ([]*dns.Record, error) { - domain := fmt.Sprintf("*.%s", ci.Status.Domain) - makeRecord := func(zone *configv1.DNSZone) *dns.Record { - return &dns.Record{ - Zone: *zone, - Type: dns.ALIASRecord, - Alias: &dns.AliasRecord{ - Domain: domain, - Target: hostname, - }, - } +func newAliasRecord(domain, target string, zone configv1.DNSZone) *dns.Record { + return &dns.Record{ + Zone: zone, + Type: dns.ALIASRecord, + Alias: &dns.AliasRecord{ + Domain: domain, + Target: target, + }, } - return desiredDNSRecords(ci, dnsConfig, makeRecord) } -func desiredDNSARecords(ci *operatorv1.IngressController, ip string, dnsConfig *configv1.DNS) ([]*dns.Record, error) { - domain := fmt.Sprintf("*.%s", ci.Status.Domain) - makeRecord := func(zone *configv1.DNSZone) *dns.Record { - return &dns.Record{ - Zone: *zone, - Type: dns.ARecordType, - ARecord: &dns.ARecord{ - Domain: domain, - Address: ip, - }, - } +func newARecord(domain, target string, zone configv1.DNSZone) *dns.Record { + return &dns.Record{ + Zone: zone, + Type: dns.ALIASRecord, + ARecord: &dns.ARecord{ + Domain: domain, + Address: target, + }, } - return desiredDNSRecords(ci, dnsConfig, makeRecord) } // desiredDNSRecords will return any necessary DNS records for the given inputs. // If an ingress domain is in use, records are desired in every specified zone // present in the cluster DNS configuration. -func desiredDNSRecords(ci *operatorv1.IngressController, dnsConfig *configv1.DNS, makeRecord makeRecordFunc) ([]*dns.Record, error) { +func desiredDNSRecords(ci *operatorv1.IngressController, dnsConfig *configv1.DNS, service *corev1.Service) []*dns.Record { records := []*dns.Record{} // If the ingresscontroller has no ingress domain, we cannot configure any // DNS records. if len(ci.Status.Domain) == 0 { - return records, nil + return records } // If the HA type is not cloud, then we don't manage DNS. if ci.Status.EndpointPublishingStrategy.Type != operatorv1.LoadBalancerServiceStrategyType { - return records, nil + return records } // If no zones are configured, there's nothing to do. if dnsConfig.Spec.PrivateZone == nil && dnsConfig.Spec.PublicZone == nil { - return records, nil + return records } + ingress := service.Status.LoadBalancer.Ingress + if len(ingress) == 0 || (len(ingress[0].Hostname) == 0 && len(ingress[0].IP) == 0) { + return records + } + + name := fmt.Sprintf("*.%s", ci.Status.Domain) + zones := []configv1.DNSZone{} if dnsConfig.Spec.PrivateZone != nil { - records = append(records, makeRecord(dnsConfig.Spec.PrivateZone)) + zones = append(zones, *dnsConfig.Spec.PrivateZone) } if dnsConfig.Spec.PublicZone != nil { - records = append(records, makeRecord(dnsConfig.Spec.PublicZone)) + zones = append(zones, *dnsConfig.Spec.PublicZone) } - return records, nil + for _, ingress := range service.Status.LoadBalancer.Ingress { + if len(ingress.Hostname) != 0 { + for _, zone := range zones { + records = append(records, newAliasRecord(name, ingress.Hostname, zone)) + } + } + if len(ingress.IP) != 0 { + for _, zone := range zones { + records = append(records, newARecord(name, ingress.IP, zone)) + } + } + } + + return records } diff --git a/pkg/operator/controller/controller_dns_test.go b/pkg/operator/controller/controller_dns_test.go new file mode 100644 index 0000000000..e69de29bb2 diff --git a/pkg/operator/controller/controller_lb.go b/pkg/operator/controller/controller_lb.go index 0ed0d2c5f4..c33c211816 100644 --- a/pkg/operator/controller/controller_lb.go +++ b/pkg/operator/controller/controller_lb.go @@ -125,24 +125,18 @@ func (r *reconciler) finalizeLoadBalancerService(ci *operatorv1.IngressControlle // at the service, we should be maintaining state with any DNS records // that we have created for the ingresscontroller, for example by using // an annotation on the ingresscontroller. - ingress := service.Status.LoadBalancer.Ingress - if len(ingress) > 0 && len(ingress[0].Hostname) > 0 { - records, err := desiredDNSAliasRecords(ci, ingress[0].Hostname, dnsConfig) - if err != nil { - return err - } - dnsErrors := []error{} - for _, record := range records { - if err := r.DNSManager.Delete(record); err != nil { - dnsErrors = append(dnsErrors, fmt.Errorf("failed to delete DNS record %v for ingress %s/%s: %v", record, ci.Namespace, ci.Name, err)) - } else { - log.Info("deleted DNS record for ingress", "namespace", ci.Namespace, "name", ci.Name, "record", record) - } - } - if err := utilerrors.NewAggregate(dnsErrors); err != nil { - return err + records := desiredDNSRecords(ci, dnsConfig, service) + dnsErrors := []error{} + for _, record := range records { + if err := r.DNSManager.Delete(record); err != nil { + dnsErrors = append(dnsErrors, fmt.Errorf("failed to delete DNS record %v for ingress %s/%s: %v", record, ci.Namespace, ci.Name, err)) + } else { + log.Info("deleted DNS record for ingress", "namespace", ci.Namespace, "name", ci.Name, "record", record) } } + if err := utilerrors.NewAggregate(dnsErrors); err != nil { + return err + } // Mutate a copy to avoid assuming we know where the current one came from // (i.e. it could have been from a cache). updated := service.DeepCopy() From 272d9b4ef3c2fbdfeacafa769f131ad1a9cfbe71 Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Wed, 29 May 2019 09:46:31 -0400 Subject: [PATCH 2/3] Add DNS unit tests --- pkg/dns/azure/dns.go | 4 + pkg/dns/dns.go | 4 + pkg/operator/controller/controller_dns.go | 12 +- .../controller/controller_dns_test.go | 201 ++++++++++++++++++ 4 files changed, 210 insertions(+), 11 deletions(-) diff --git a/pkg/dns/azure/dns.go b/pkg/dns/azure/dns.go index 5805b926d7..7659e7658b 100644 --- a/pkg/dns/azure/dns.go +++ b/pkg/dns/azure/dns.go @@ -58,6 +58,10 @@ func userAgent(operatorReleaseVersion string) string { } func (m *manager) Ensure(record *dns.Record) error { + if record.Type != dns.ARecordType { + return fmt.Errorf("only A record types are supported") + } + targetZone, err := client.ParseZone(record.Zone.ID) if err != nil { return errors.Wrap(err, "failed to parse zoneID") diff --git a/pkg/dns/dns.go b/pkg/dns/dns.go index 37d12b840a..727da50975 100644 --- a/pkg/dns/dns.go +++ b/pkg/dns/dns.go @@ -36,6 +36,10 @@ type Record struct { ARecord *ARecord } +func (r *Record) String() string { + return fmt.Sprintf("Zone: %v, Type: %v, Alias: %s, A: %s", r.Zone, r.Type, r.Alias, r.ARecord) +} + // RecordType is a DNS record type. type RecordType string diff --git a/pkg/operator/controller/controller_dns.go b/pkg/operator/controller/controller_dns.go index cf596faf32..c43f4f1060 100644 --- a/pkg/operator/controller/controller_dns.go +++ b/pkg/operator/controller/controller_dns.go @@ -39,7 +39,7 @@ func newAliasRecord(domain, target string, zone configv1.DNSZone) *dns.Record { func newARecord(domain, target string, zone configv1.DNSZone) *dns.Record { return &dns.Record{ Zone: zone, - Type: dns.ALIASRecord, + Type: dns.ARecordType, ARecord: &dns.ARecord{ Domain: domain, Address: target, @@ -64,16 +64,6 @@ func desiredDNSRecords(ci *operatorv1.IngressController, dnsConfig *configv1.DNS return records } - // If no zones are configured, there's nothing to do. - if dnsConfig.Spec.PrivateZone == nil && dnsConfig.Spec.PublicZone == nil { - return records - } - - ingress := service.Status.LoadBalancer.Ingress - if len(ingress) == 0 || (len(ingress[0].Hostname) == 0 && len(ingress[0].IP) == 0) { - return records - } - name := fmt.Sprintf("*.%s", ci.Status.Domain) zones := []configv1.DNSZone{} if dnsConfig.Spec.PrivateZone != nil { diff --git a/pkg/operator/controller/controller_dns_test.go b/pkg/operator/controller/controller_dns_test.go index e69de29bb2..7a132309cb 100644 --- a/pkg/operator/controller/controller_dns_test.go +++ b/pkg/operator/controller/controller_dns_test.go @@ -0,0 +1,201 @@ +package controller + +import ( + "testing" + + configv1 "github.com/openshift/api/config/v1" + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/cluster-ingress-operator/pkg/dns" + + corev1 "k8s.io/api/core/v1" + + "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" +) + +var privateZone = configv1.DNSZone{ + ID: "private", +} +var publicZone = configv1.DNSZone{ + ID: "public", +} + +var globalConfig *configv1.DNS = &configv1.DNS{ + Spec: configv1.DNSSpec{ + PrivateZone: &privateZone, + PublicZone: &publicZone, + }, +} + +var privateConfig *configv1.DNS = &configv1.DNS{ + Spec: configv1.DNSSpec{ + PrivateZone: &privateZone, + }, +} + +var publicConfig *configv1.DNS = &configv1.DNS{ + Spec: configv1.DNSSpec{ + PublicZone: &publicZone, + }, +} + +func TestDesiredDNSRecords(t *testing.T) { + type ingress struct { + host string + ip string + } + type record struct { + typ dns.RecordType + name string + target string + zone configv1.DNSZone + } + makeService := func(ingresses []ingress) *corev1.Service { + service := &corev1.Service{} + for _, ingress := range ingresses { + service.Status.LoadBalancer.Ingress = append(service.Status.LoadBalancer.Ingress, corev1.LoadBalancerIngress{ + IP: ingress.ip, + Hostname: ingress.host, + }) + } + return service + } + makeRecords := func(records []record) []*dns.Record { + dnsRecords := []*dns.Record{} + for _, r := range records { + record := &dns.Record{ + Zone: r.zone, + Type: r.typ, + } + switch record.Type { + case dns.ALIASRecord: + record.Alias = &dns.AliasRecord{ + Domain: r.name, + Target: r.target, + } + case dns.ARecordType: + record.ARecord = &dns.ARecord{ + Domain: r.name, + Address: r.target, + } + } + dnsRecords = append(dnsRecords, record) + } + return dnsRecords + } + tests := []struct { + description string + domain string + publish operatorv1.EndpointPublishingStrategyType + dnsConfig *configv1.DNS + ingresses []ingress + expect []record + }{ + { + description: "no domain", + domain: "", + publish: operatorv1.LoadBalancerServiceStrategyType, + dnsConfig: globalConfig, + ingresses: []ingress{ + {host: "lb.cloud.example.com", ip: ""}, + {host: "lb.cloud.example.com", ip: "192.0.2.1"}, + }, + expect: []record{}, + }, + { + description: "not a load balancer", + domain: "apps.openshift.example.com", + publish: operatorv1.HostNetworkStrategyType, + dnsConfig: globalConfig, + expect: []record{}, + }, + { + description: "no zones", + domain: "apps.openshift.example.com", + publish: operatorv1.LoadBalancerServiceStrategyType, + dnsConfig: &configv1.DNS{}, + ingresses: []ingress{ + {host: "lb.cloud.example.com"}, + }, + expect: []record{}, + }, + { + description: "public only ALIAS", + publish: operatorv1.LoadBalancerServiceStrategyType, + domain: "apps.openshift.example.com", + dnsConfig: publicConfig, + ingresses: []ingress{ + {host: "lb.cloud.example.com"}, + }, + expect: []record{ + {typ: dns.ALIASRecord, name: "*.apps.openshift.example.com", target: "lb.cloud.example.com", zone: publicZone}, + }, + }, + { + description: "private only A", + publish: operatorv1.LoadBalancerServiceStrategyType, + domain: "apps.openshift.example.com", + dnsConfig: privateConfig, + ingresses: []ingress{ + {ip: "192.0.2.1"}, + }, + expect: []record{ + {typ: dns.ARecordType, name: "*.apps.openshift.example.com", target: "192.0.2.1", zone: privateZone}, + }, + }, + { + description: "global ALIAS", + publish: operatorv1.LoadBalancerServiceStrategyType, + domain: "apps.openshift.example.com", + dnsConfig: globalConfig, + ingresses: []ingress{ + {host: "lb.cloud.example.com"}, + }, + expect: []record{ + {typ: dns.ALIASRecord, name: "*.apps.openshift.example.com", target: "lb.cloud.example.com", zone: publicZone}, + {typ: dns.ALIASRecord, name: "*.apps.openshift.example.com", target: "lb.cloud.example.com", zone: privateZone}, + }, + }, + { + description: "global A", + publish: operatorv1.LoadBalancerServiceStrategyType, + domain: "apps.openshift.example.com", + dnsConfig: globalConfig, + ingresses: []ingress{ + {ip: "192.0.2.1"}, + }, + expect: []record{ + {typ: dns.ARecordType, name: "*.apps.openshift.example.com", target: "192.0.2.1", zone: publicZone}, + {typ: dns.ARecordType, name: "*.apps.openshift.example.com", target: "192.0.2.1", zone: privateZone}, + }, + }, + } + + for _, test := range tests { + t.Logf("testing %s", test.description) + controller := &operatorv1.IngressController{ + Status: operatorv1.IngressControllerStatus{ + Domain: test.domain, + EndpointPublishingStrategy: &operatorv1.EndpointPublishingStrategy{ + Type: test.publish, + }, + }, + } + actual := desiredDNSRecords(controller, test.dnsConfig, makeService(test.ingresses)) + expected := makeRecords(test.expect) + if !cmp.Equal(actual, expected, cmpopts.EquateEmpty(), cmpopts.SortSlices(cmpRecords)) { + t.Errorf("expected:") + for _, r := range expected { + t.Errorf("\t%s", r) + } + t.Errorf("actual:") + for _, r := range actual { + t.Errorf("\t%s", r) + } + } + } +} + +func cmpRecords(a, b *dns.Record) bool { + return string(a.Zone.ID) < string(b.Zone.ID) +} From a4771d3b4ae8eb76dbc539af6e37b215f1e7b918 Mon Sep 17 00:00:00 2001 From: Dan Mace Date: Wed, 29 May 2019 10:14:27 -0400 Subject: [PATCH 3/3] dns/azure: fix delete handling --- pkg/dns/azure/client/client.go | 7 ++++++- pkg/dns/azure/dns.go | 4 ---- pkg/operator/controller/controller_dns.go | 4 ++-- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/pkg/dns/azure/client/client.go b/pkg/dns/azure/client/client.go index 3079630e6c..2033be5318 100644 --- a/pkg/dns/azure/client/client.go +++ b/pkg/dns/azure/client/client.go @@ -70,7 +70,12 @@ func (c *dnsClient) Put(ctx context.Context, zone Zone, arec ARecord) error { } func (c *dnsClient) Delete(ctx context.Context, zone Zone, arec ARecord) error { - _, err := c.recordSets.Delete(ctx, zone.ResourceGroup, zone.Name, arec.Name, dns.A, "") + _, err := c.recordSets.Get(ctx, zone.ResourceGroup, zone.Name, arec.Name, dns.A) + if err != nil { + // TODO: How do we interpret this as a notfound error? + return nil + } + _, err = c.recordSets.Delete(ctx, zone.ResourceGroup, zone.Name, arec.Name, dns.A, "") if err != nil { return errors.Wrapf(err, "failed to delete dns a record: %s.%s", arec.Name, zone.Name) } diff --git a/pkg/dns/azure/dns.go b/pkg/dns/azure/dns.go index 7659e7658b..a114acee8d 100644 --- a/pkg/dns/azure/dns.go +++ b/pkg/dns/azure/dns.go @@ -116,9 +116,5 @@ func (m *manager) Delete(record *dns.Record) error { // getARecordName extracts the ARecord subdomain name from the full domain string. // azure defines the ARecord Name as the subdomain name only. func getARecordName(recordDomain string, zoneName string) (string, error) { - if !strings.HasSuffix(recordDomain, zoneName) { - return "", errors.Errorf("the record %s cannot be part of zone %s", recordDomain, zoneName) - } - return strings.TrimSuffix(recordDomain, zoneName), nil } diff --git a/pkg/operator/controller/controller_dns.go b/pkg/operator/controller/controller_dns.go index c43f4f1060..d558ed74d7 100644 --- a/pkg/operator/controller/controller_dns.go +++ b/pkg/operator/controller/controller_dns.go @@ -73,12 +73,12 @@ func desiredDNSRecords(ci *operatorv1.IngressController, dnsConfig *configv1.DNS zones = append(zones, *dnsConfig.Spec.PublicZone) } for _, ingress := range service.Status.LoadBalancer.Ingress { - if len(ingress.Hostname) != 0 { + if len(ingress.Hostname) > 0 { for _, zone := range zones { records = append(records, newAliasRecord(name, ingress.Hostname, zone)) } } - if len(ingress.IP) != 0 { + if len(ingress.IP) > 0 { for _, zone := range zones { records = append(records, newARecord(name, ingress.IP, zone)) }