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 5805b926d7..a114acee8d 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") @@ -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 } 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 bddee0f53e..d558ed74d7 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,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 } diff --git a/pkg/operator/controller/controller_dns_test.go b/pkg/operator/controller/controller_dns_test.go new file mode 100644 index 0000000000..7a132309cb --- /dev/null +++ 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) +} 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()