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",