Refactor desired DNS functions

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)
 	}

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")
@@ -112,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
 }

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
 

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,65 @@ 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.ARecordType,
+		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
-	}
-
-	// If no zones are configured, there's nothing to do.
-	if dnsConfig.Spec.PrivateZone == nil && dnsConfig.Spec.PublicZone == nil {
-		return records, nil
+		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
 }

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)
+}