From 2223383ef099e7e6fc3c49f5c45934da44044998 Mon Sep 17 00:00:00 2001 From: Grant Spence Date: Thu, 22 Aug 2024 18:02:15 -0400 Subject: [PATCH] OCPBUGS-32776: Fix IBM Public Cloud DNS Provider Update Logic The IBM Public Cloud DNS provider (`cis_provider.go`) had a bug in `createOrUpdateDNSRecord` where it checked for the existence of a DNS record by filtering both DNS name and target. If the target was updated (e.g., due to a load balancer recreation), the logic would not match the existing DNS record. As a result, the function would attempt to create a new record, but fail because a record with that name already existed, as multiple DNS records with the same name are not allowed in IBM Cloud DNS providers. The fix is to remove the filtering by target and rely solely on filtering by name, as the name is the only attribute that needs to be unique. Additionally, the IBM DNS logic doesn't work for multiple targets and this creates unexpected and problematic results. The logic has been refactored to only create using the first target and it warns the user when multiple targets are set. This change is low risk since the Ingress Operator will never create a DNSRecord with multiple targets in `desiredDNSRecord`. --- pkg/dns/ibm/private/dnssvcs_provider.go | 118 ++++++++++--------- pkg/dns/ibm/private/dnssvcs_provider_test.go | 34 ++++++ pkg/dns/ibm/public/cis_provider.go | 70 ++++++----- pkg/dns/ibm/public/cis_provider_test.go | 26 +++- 4 files changed, 161 insertions(+), 87 deletions(-) diff --git a/pkg/dns/ibm/private/dnssvcs_provider.go b/pkg/dns/ibm/private/dnssvcs_provider.go index 3b6adacc5..771b34444 100644 --- a/pkg/dns/ibm/private/dnssvcs_provider.go +++ b/pkg/dns/ibm/private/dnssvcs_provider.go @@ -126,13 +126,17 @@ func (p *Provider) Delete(record *iov1.DNSRecord, zone configv1.DNSZone) error { default: return fmt.Errorf("delete: resource data has record with unknown type: %v", *resourceRecord.Type) } - + // While creating DNS records with multiple targets is unsupported, we still + // iterate through all targets during deletion to be extra cautious. for _, target := range record.Spec.Targets { if *resourceRecord.Name == dnsName { if resourceRecordTarget != target { log.Info("delete: ignoring record with matching name but unexpected target", "record", record, "target", resourceRecordTarget) continue } + if resourceRecord.ID == nil { + return fmt.Errorf("delete: record id is nil") + } delOpt := p.dnsService.NewDeleteResourceRecordOptions(p.config.InstanceID, zone.ID, *resourceRecord.ID) delResponse, err := p.dnsService.DeleteResourceRecord(delOpt) if err != nil { @@ -190,6 +194,10 @@ func (p *Provider) createOrUpdateDNSRecord(record *iov1.DNSRecord, zone configv1 log.Info("Warning: TTL must be one of [1 60 120 300 600 900 1800 3600 7200 18000 43200]. RecordTTL set to default", "default DSNSVCS record TTL", defaultDNSSVCSRecordTTL) record.Spec.RecordTTL = defaultDNSSVCSRecordTTL } + // We only support one target, warn the user. + if len(record.Spec.Targets) > 1 { + log.Info("Warning: Only one DNSRecord target is supported. Additional targets will be ignored.", "targets", record.Spec.Targets) + } listResult, response, err := p.dnsService.ListResourceRecords(listOpt) if err != nil { @@ -201,72 +209,74 @@ func (p *Provider) createOrUpdateDNSRecord(record *iov1.DNSRecord, zone configv1 return fmt.Errorf("createOrUpdateDNSRecord: ListResourceRecords returned nil as result") } - for _, target := range record.Spec.Targets { - updated := false - for _, resourceRecord := range listResult.ResourceRecords { - if *resourceRecord.Name == dnsName { - updateOpt := p.dnsService.NewUpdateResourceRecordOptions(p.config.InstanceID, zone.ID, *resourceRecord.ID) - updateOpt.SetName(dnsName) - - if resourceRecord.Type == nil { - return fmt.Errorf("createOrUpdateDNSRecord: failed to get resource type, resourceRecord.Type is nil") - } + target := record.Spec.Targets[0] + updated := false + for _, resourceRecord := range listResult.ResourceRecords { + if *resourceRecord.Name == dnsName { + if resourceRecord.ID == nil { + return fmt.Errorf("createOrUpdateDNSRecord: record id is nil") + } + updateOpt := p.dnsService.NewUpdateResourceRecordOptions(p.config.InstanceID, zone.ID, *resourceRecord.ID) + updateOpt.SetName(dnsName) - // TODO DNS record update should handle the case where we have an A record and want a CNAME record or vice versa - switch *resourceRecord.Type { - case string(iov1.CNAMERecordType): - inputRData, err := p.dnsService.NewResourceRecordUpdateInputRdataRdataCnameRecord(target) - if err != nil { - return fmt.Errorf("createOrUpdateDNSRecord: failed to create CNAME inputRData for the dns record: %w", err) - } - updateOpt.SetRdata(inputRData) - case string(iov1.ARecordType): - inputRData, err := p.dnsService.NewResourceRecordUpdateInputRdataRdataARecord(target) - if err != nil { - return fmt.Errorf("createOrUpdateDNSRecord: failed to create A inputRData for the dns record: %w", err) - } - updateOpt.SetRdata(inputRData) - default: - return fmt.Errorf("createOrUpdateDNSRecord: resource data has record with unknown type: %v", *resourceRecord.Type) - } - updateOpt.SetTTL(record.Spec.RecordTTL) - _, _, err := p.dnsService.UpdateResourceRecord(updateOpt) - if err != nil { - return fmt.Errorf("createOrUpdateDNSRecord: failed to update the dns record: %w", err) - } - updated = true - log.Info("updated DNS record", "record", record.Spec, "zone", zone, "target", target) + if resourceRecord.Type == nil { + return fmt.Errorf("createOrUpdateDNSRecord: failed to get resource type, resourceRecord.Type is nil") } - } - if !updated { - createOpt := p.dnsService.NewCreateResourceRecordOptions(p.config.InstanceID, zone.ID) - createOpt.SetName(dnsName) - createOpt.SetType(string(record.Spec.RecordType)) - - switch record.Spec.RecordType { - case iov1.CNAMERecordType: - inputRData, err := p.dnsService.NewResourceRecordInputRdataRdataCnameRecord(target) + + // TODO DNS record update should handle the case where we have an A record and want a CNAME record or vice versa + switch *resourceRecord.Type { + case string(iov1.CNAMERecordType): + inputRData, err := p.dnsService.NewResourceRecordUpdateInputRdataRdataCnameRecord(target) if err != nil { return fmt.Errorf("createOrUpdateDNSRecord: failed to create CNAME inputRData for the dns record: %w", err) } - createOpt.SetRdata(inputRData) - case iov1.ARecordType: - inputRData, err := p.dnsService.NewResourceRecordInputRdataRdataARecord(target) + updateOpt.SetRdata(inputRData) + case string(iov1.ARecordType): + inputRData, err := p.dnsService.NewResourceRecordUpdateInputRdataRdataARecord(target) if err != nil { return fmt.Errorf("createOrUpdateDNSRecord: failed to create A inputRData for the dns record: %w", err) } - createOpt.SetRdata(inputRData) + updateOpt.SetRdata(inputRData) default: - return fmt.Errorf("createOrUpdateDNSRecord: resource data has record with unknown type: %v", record.Spec.RecordType) - + return fmt.Errorf("createOrUpdateDNSRecord: resource data has record with unknown type: %v", *resourceRecord.Type) + } + updateOpt.SetTTL(record.Spec.RecordTTL) + _, _, err := p.dnsService.UpdateResourceRecord(updateOpt) + if err != nil { + return fmt.Errorf("createOrUpdateDNSRecord: failed to update the dns record: %w", err) + } + updated = true + log.Info("updated DNS record", "record", record.Spec, "zone", zone, "target", target) + } + } + if !updated { + createOpt := p.dnsService.NewCreateResourceRecordOptions(p.config.InstanceID, zone.ID) + createOpt.SetName(dnsName) + createOpt.SetType(string(record.Spec.RecordType)) + + switch record.Spec.RecordType { + case iov1.CNAMERecordType: + inputRData, err := p.dnsService.NewResourceRecordInputRdataRdataCnameRecord(target) + if err != nil { + return fmt.Errorf("createOrUpdateDNSRecord: failed to create CNAME inputRData for the dns record: %w", err) } - createOpt.SetTTL(record.Spec.RecordTTL) - _, _, err := p.dnsService.CreateResourceRecord(createOpt) + createOpt.SetRdata(inputRData) + case iov1.ARecordType: + inputRData, err := p.dnsService.NewResourceRecordInputRdataRdataARecord(target) if err != nil { - return fmt.Errorf("createOrUpdateDNSRecord: failed to create the dns record: %w", err) + return fmt.Errorf("createOrUpdateDNSRecord: failed to create A inputRData for the dns record: %w", err) } - log.Info("created DNS record", "record", record.Spec, "zone", zone, "target", target) + createOpt.SetRdata(inputRData) + default: + return fmt.Errorf("createOrUpdateDNSRecord: resource data has record with unknown type: %v", record.Spec.RecordType) + + } + createOpt.SetTTL(record.Spec.RecordTTL) + _, _, err := p.dnsService.CreateResourceRecord(createOpt) + if err != nil { + return fmt.Errorf("createOrUpdateDNSRecord: failed to create the dns record: %w", err) } + log.Info("created DNS record", "record", record.Spec, "zone", zone, "target", target) } return nil } diff --git a/pkg/dns/ibm/private/dnssvcs_provider_test.go b/pkg/dns/ibm/private/dnssvcs_provider_test.go index 68c7e286b..b0130612b 100644 --- a/pkg/dns/ibm/private/dnssvcs_provider_test.go +++ b/pkg/dns/ibm/private/dnssvcs_provider_test.go @@ -107,6 +107,23 @@ func Test_Delete(t *testing.T) { }, expectErrorContains: "error in ListAllDnsRecords", }, + { + desc: "list success but missing ID", + DNSName: "testList", + target: "11.22.33.44", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + Name: pointer.String("testList"), + Rdata: map[string]interface{}{"ip": "11.22.33.44"}, + Type: pointer.String(string(iov1.ARecordType)), + }}, + }, + }, + expectErrorContains: "delete: record id is nil", + }, { desc: "delete failure with 404", recordedCall: "DELETE", @@ -357,6 +374,23 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { }, expectErrorContains: "error in ListAllDnsRecords", }, + { + desc: "list success but missing ID", + DNSName: "testList", + target: "11.22.33.44", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + Name: pointer.String("testList"), + Rdata: map[string]interface{}{"ip": "11.22.33.44"}, + Type: pointer.String(string(iov1.ARecordType)), + }}, + }, + }, + expectErrorContains: "createOrUpdateDNSRecord: record id is nil", + }, { desc: "update error", DNSName: "testUpdate", diff --git a/pkg/dns/ibm/public/cis_provider.go b/pkg/dns/ibm/public/cis_provider.go index 65b89fe77..f2d1e019f 100644 --- a/pkg/dns/ibm/public/cis_provider.go +++ b/pkg/dns/ibm/public/cis_provider.go @@ -107,6 +107,8 @@ func (p *Provider) Delete(record *iov1.DNSRecord, zone configv1.DNSZone) error { // "." when it creates a wildcard DNS record. dnsName := strings.TrimSuffix(record.Spec.DNSName, ".") opt.SetName(dnsName) + // While creating DNS records with multiple targets is unsupported, we still + // iterate through all targets during deletion to be extra cautious. for _, target := range record.Spec.Targets { opt.SetContent(target) result, response, err := dnsService.ListAllDnsRecords(opt) @@ -152,6 +154,10 @@ func (p *Provider) createOrUpdateDNSRecord(record *iov1.DNSRecord, zone configv1 log.Info("Warning: TTL must be between 120 and 2,147,483,647 seconds, or 1 for Automatic. RecordTTL set to default", "default CIS record TTL", defaultCISRecordTTL) record.Spec.RecordTTL = defaultCISRecordTTL } + // We only support one target, warn the user. + if len(record.Spec.Targets) > 1 { + log.Info("Warning: Only one DNSRecord target is supported. Additional targets will be ignored.", "targets", record.Spec.Targets) + } listOpt := dnsService.NewListAllDnsRecordsOptions() listOpt.SetType(string(record.Spec.RecordType)) @@ -160,41 +166,43 @@ func (p *Provider) createOrUpdateDNSRecord(record *iov1.DNSRecord, zone configv1 // "." when it creates a wildcard DNS record. dnsName := strings.TrimSuffix(record.Spec.DNSName, ".") listOpt.SetName(dnsName) - for _, target := range record.Spec.Targets { - listOpt.SetContent(target) - result, response, err := dnsService.ListAllDnsRecords(listOpt) + + result, response, err := dnsService.ListAllDnsRecords(listOpt) + if err != nil { + if response == nil || response.StatusCode != http.StatusNotFound { + return fmt.Errorf("createOrUpdateDNSRecord: failed to list the dns record: %w", err) + } + } + if result == nil || result.Result == nil { + return fmt.Errorf("createOrUpdateDNSRecord: ListAllDnsRecords returned nil as result") + } + + target := record.Spec.Targets[0] + if len(result.Result) == 0 { + createOpt := dnsService.NewCreateDnsRecordOptions() + createOpt.SetName(record.Spec.DNSName) + createOpt.SetType(string(record.Spec.RecordType)) + createOpt.SetContent(target) + createOpt.SetTTL(record.Spec.RecordTTL) + _, _, err := dnsService.CreateDnsRecord(createOpt) if err != nil { - if response != nil && response.StatusCode != http.StatusNotFound { - return fmt.Errorf("createOrUpdateDNSRecord: failed to list the dns record: %w", err) - } - continue + return fmt.Errorf("createOrUpdateDNSRecord: failed to create the dns record: %w", err) } - if result == nil || result.Result == nil { - return fmt.Errorf("createOrUpdateDNSRecord: ListAllDnsRecords returned nil as result") + log.Info("created DNS record", "record", record.Spec, "zone", zone, "target", target) + } else { + if result.Result[0].ID == nil { + return fmt.Errorf("createOrUpdateDNSRecord: record id is nil") } - if len(result.Result) == 0 { - createOpt := dnsService.NewCreateDnsRecordOptions() - createOpt.SetName(record.Spec.DNSName) - createOpt.SetType(string(record.Spec.RecordType)) - createOpt.SetContent(target) - createOpt.SetTTL(record.Spec.RecordTTL) - _, _, err := dnsService.CreateDnsRecord(createOpt) - if err != nil { - return fmt.Errorf("createOrUpdateDNSRecord: failed to create the dns record: %w", err) - } - log.Info("created DNS record", "record", record.Spec, "zone", zone, "target", target) - } else { - updateOpt := dnsService.NewUpdateDnsRecordOptions(*result.Result[0].ID) - updateOpt.SetName(record.Spec.DNSName) - updateOpt.SetType(string(record.Spec.RecordType)) - updateOpt.SetContent(target) - updateOpt.SetTTL(record.Spec.RecordTTL) - _, _, err := dnsService.UpdateDnsRecord(updateOpt) - if err != nil { - return fmt.Errorf("createOrUpdateDNSRecord: failed to update the dns record: %w", err) - } - log.Info("updated DNS record", "record", record.Spec, "zone", zone, "target", target) + updateOpt := dnsService.NewUpdateDnsRecordOptions(*result.Result[0].ID) + updateOpt.SetName(record.Spec.DNSName) + updateOpt.SetType(string(record.Spec.RecordType)) + updateOpt.SetContent(target) + updateOpt.SetTTL(record.Spec.RecordTTL) + _, _, err := dnsService.UpdateDnsRecord(updateOpt) + if err != nil { + return fmt.Errorf("createOrUpdateDNSRecord: failed to update the dns record: %w", err) } + log.Info("updated DNS record", "record", record.Spec, "zone", zone, "target", target) } return nil diff --git a/pkg/dns/ibm/public/cis_provider_test.go b/pkg/dns/ibm/public/cis_provider_test.go index 12eae8b54..6b559c39b 100644 --- a/pkg/dns/ibm/public/cis_provider_test.go +++ b/pkg/dns/ibm/public/cis_provider_test.go @@ -257,8 +257,9 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { }, }, { - desc: "list failure with 404", - DNSName: "testUpdate", + desc: "list failure with 404", + DNSName: "testCreate", + recordedCall: "CREATE", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ OutputError: errors.New("error in ListAllDnsRecords"), OutputResponse: &core.DetailedResponse{StatusCode: http.StatusNotFound}, @@ -266,6 +267,11 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { Result: []dnsrecordsv1.DnsrecordDetails{}, }, }, + createDnsRecordInputOutput: dnsclient.CreateDnsRecordInputOutput{ + InputId: "testCreate", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + }, }, { desc: "list failure (not 404)", @@ -332,6 +338,21 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { }, expectErrorContains: "error in CreateDnsRecord", }, + { + desc: "list success but missing ID", + DNSName: "testList", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{{ + Name: pointer.String("testList"), + Content: pointer.String("11.22.33.44"), + }}, + }, + }, + expectErrorContains: "createOrUpdateDNSRecord: record id is nil", + }, { desc: "list success but nil results", DNSName: "testList", @@ -359,6 +380,7 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { OutputResponse: nil, OutputResult: &dnsrecordsv1.ListDnsrecordsResp{}, }, + expectErrorContains: "createOrUpdateDNSRecord: failed to list the dns record: error in ListAllDnsRecords", }, { desc: "empty DNSName",