Merge pull request #1133 from gcs278/ibm-dns-createOrUpdateDNSRecord-bug

OCPBUGS-32776: Fix IBM Public Cloud DNS Provider Update Logic

Author: openshift-merge-bot[bot]

pkg/dns/ibm/private/client/fake_client.go

@@ -5,33 +5,37 @@ import (
 
 	"github.com/IBM/go-sdk-core/v5/core"
 	"github.com/IBM/networking-go-sdk/dnssvcsv1"
-	iov1 "github.com/openshift/api/operatoringress/v1"
 )
 
 type FakeDnsClient struct {
 	CallHistory                  map[string]string
 	DeleteDnsRecordInputOutput   DeleteDnsRecordInputOutput
 	ListAllDnsRecordsInputOutput ListAllDnsRecordsInputOutput
 	UpdateDnsRecordInputOutput   UpdateDnsRecordInputOutput
+	CreateDnsRecordInputOutput   CreateDnsRecordInputOutput
 }
 
 type DeleteDnsRecordInputOutput struct {
-	InputId          string
-	OutputError      error
-	OutputStatusCode int
+	InputId        string
+	OutputError    error
+	OutputResponse *core.DetailedResponse
 }
 
 type UpdateDnsRecordInputOutput struct {
-	InputId          string
-	OutputError      error
-	OutputStatusCode int
+	InputId        string
+	OutputError    error
+	OutputResponse *core.DetailedResponse
+}
+type CreateDnsRecordInputOutput struct {
+	InputId        string
+	OutputError    error
+	OutputResponse *core.DetailedResponse
 }
 
 type ListAllDnsRecordsInputOutput struct {
-	RecordName       string
-	RecordTarget     string
-	OutputError      error
-	OutputStatusCode int
+	OutputResult   *dnssvcsv1.ListResourceRecords
+	OutputError    error
+	OutputResponse *core.DetailedResponse
 }
 
 func NewFake() (*FakeDnsClient, error) {
@@ -43,42 +47,29 @@ func (c *FakeDnsClient) RecordedCall(zoneID string) (string, bool) {
 	return call, ok
 }
 
+func (c *FakeDnsClient) ClearCallHistory() {
+	c.CallHistory = map[string]string{}
+}
+
 func (FakeDnsClient) NewListResourceRecordsOptions(instanceID string, dnszoneID string) *dnssvcsv1.ListResourceRecordsOptions {
 	return &dnssvcsv1.ListResourceRecordsOptions{}
 }
 func (fdc FakeDnsClient) ListResourceRecords(listResourceRecordsOptions *dnssvcsv1.ListResourceRecordsOptions) (result *dnssvcsv1.ListResourceRecords, response *core.DetailedResponse, err error) {
-	fakeListDnsrecordsResp := &dnssvcsv1.ListResourceRecords{}
-	recordType := string(iov1.ARecordType)
-	rData := map[string]interface{}{"ip": fdc.ListAllDnsRecordsInputOutput.RecordTarget}
-
-	fakeListDnsrecordsResp.ResourceRecords = append(fakeListDnsrecordsResp.ResourceRecords, dnssvcsv1.ResourceRecord{ID: &fdc.ListAllDnsRecordsInputOutput.RecordName, Name: &fdc.ListAllDnsRecordsInputOutput.RecordName, Type: &recordType, Rdata: rData})
-
-	resp := &core.DetailedResponse{
-		StatusCode: fdc.ListAllDnsRecordsInputOutput.OutputStatusCode,
-		Headers:    map[string][]string{},
-		Result:     result,
-		RawResult:  []byte{},
-	}
-
-	return fakeListDnsrecordsResp, resp, fdc.ListAllDnsRecordsInputOutput.OutputError
+	// Return the fields in ListAllDnsRecordsInputOutput which will be populated in each of the unit test cases.
+	return fdc.ListAllDnsRecordsInputOutput.OutputResult, fdc.ListAllDnsRecordsInputOutput.OutputResponse, fdc.ListAllDnsRecordsInputOutput.OutputError
 }
 func (FakeDnsClient) NewDeleteResourceRecordOptions(instanceID string, dnszoneID string, recordID string) *dnssvcsv1.DeleteResourceRecordOptions {
 	return &dnssvcsv1.DeleteResourceRecordOptions{InstanceID: &instanceID, DnszoneID: &dnszoneID, RecordID: &recordID}
 }
 func (fdc FakeDnsClient) DeleteResourceRecord(deleteResourceRecordOptions *dnssvcsv1.DeleteResourceRecordOptions) (response *core.DetailedResponse, err error) {
+	// Check InputID against the incoming deleteResourceRecordOptions to ensure the
+	// Delete method is using the correct recordID in deleteResourceRecordOptions.
 	if fdc.DeleteDnsRecordInputOutput.InputId != *deleteResourceRecordOptions.RecordID {
 		return nil, errors.New("deleteDnsRecord: inputs don't match")
 	}
 
-	resp := &core.DetailedResponse{
-		StatusCode: fdc.DeleteDnsRecordInputOutput.OutputStatusCode,
-		Headers:    map[string][]string{},
-		Result:     response,
-		RawResult:  []byte{},
-	}
-
 	fdc.CallHistory[*deleteResourceRecordOptions.RecordID] = "DELETE"
-	return resp, fdc.DeleteDnsRecordInputOutput.OutputError
+	return fdc.DeleteDnsRecordInputOutput.OutputResponse, fdc.DeleteDnsRecordInputOutput.OutputError
 }
 func (FakeDnsClient) NewUpdateResourceRecordOptions(instanceID string, dnszoneID string, recordID string) *dnssvcsv1.UpdateResourceRecordOptions {
 	return &dnssvcsv1.UpdateResourceRecordOptions{InstanceID: &instanceID, DnszoneID: &dnszoneID, RecordID: &recordID}
@@ -90,31 +81,33 @@ func (FakeDnsClient) NewResourceRecordUpdateInputRdataRdataARecord(ip string) (_
 	return &dnssvcsv1.ResourceRecordUpdateInputRdataRdataARecord{Ip: &ip}, nil
 }
 func (fdc FakeDnsClient) UpdateResourceRecord(updateResourceRecordOptions *dnssvcsv1.UpdateResourceRecordOptions) (result *dnssvcsv1.ResourceRecord, response *core.DetailedResponse, err error) {
+	// Check InputID against the incoming updateResourceRecordOptions to ensure the
+	// createOrUpdateDNSRecord method is using the correct recordID in updateResourceRecordOptions.
 	if fdc.UpdateDnsRecordInputOutput.InputId != *updateResourceRecordOptions.RecordID {
 		return nil, nil, errors.New("updateDnsRecord: inputs don't match")
 	}
 
-	resp := &core.DetailedResponse{
-		StatusCode: fdc.UpdateDnsRecordInputOutput.OutputStatusCode,
-		Headers:    map[string][]string{},
-		Result:     response,
-		RawResult:  []byte{},
-	}
-
 	fdc.CallHistory[*updateResourceRecordOptions.RecordID] = "PUT"
-	return nil, resp, fdc.UpdateDnsRecordInputOutput.OutputError
+	return nil, fdc.UpdateDnsRecordInputOutput.OutputResponse, fdc.UpdateDnsRecordInputOutput.OutputError
 }
 func (FakeDnsClient) NewCreateResourceRecordOptions(instanceID string, dnszoneID string) *dnssvcsv1.CreateResourceRecordOptions {
-	return nil
+	return &dnssvcsv1.CreateResourceRecordOptions{}
 }
 func (FakeDnsClient) NewResourceRecordInputRdataRdataCnameRecord(cname string) (_model *dnssvcsv1.ResourceRecordInputRdataRdataCnameRecord, err error) {
 	return nil, nil
 }
 func (FakeDnsClient) NewResourceRecordInputRdataRdataARecord(ip string) (_model *dnssvcsv1.ResourceRecordInputRdataRdataARecord, err error) {
 	return nil, nil
 }
-func (FakeDnsClient) CreateResourceRecord(createResourceRecordOptions *dnssvcsv1.CreateResourceRecordOptions) (result *dnssvcsv1.ResourceRecord, response *core.DetailedResponse, err error) {
-	return nil, nil, nil
+func (fdc FakeDnsClient) CreateResourceRecord(createResourceRecordOptions *dnssvcsv1.CreateResourceRecordOptions) (result *dnssvcsv1.ResourceRecord, response *core.DetailedResponse, err error) {
+	// Check InputID against the incoming createResourceRecordOptions to ensure the
+	// createOrUpdateDNSRecord method is using the correct record name in createResourceRecordOptions.
+	if fdc.CreateDnsRecordInputOutput.InputId != *createResourceRecordOptions.Name {
+		return nil, nil, errors.New("createResourceRecord: inputs don't match")
+	}
+
+	fdc.CallHistory[*createResourceRecordOptions.Name] = "CREATE"
+	return nil, fdc.CreateDnsRecordInputOutput.OutputResponse, fdc.CreateDnsRecordInputOutput.OutputError
 }
 func (FakeDnsClient) NewGetDnszoneOptions(instanceID string, dnszoneID string) *dnssvcsv1.GetDnszoneOptions {
 	return nil

pkg/dns/ibm/private/dnssvcs_provider.go

@@ -103,6 +103,9 @@ func (p *Provider) Delete(record *iov1.DNSRecord, zone configv1.DNSZone) error {
 
 	for _, resourceRecord := range result.ResourceRecords {
 		var resourceRecordTarget string
+		if resourceRecord.ID == nil {
+			return fmt.Errorf("delete: record id is nil")
+		}
 		rData, ok := resourceRecord.Rdata.(map[string]interface{})
 		if !ok {
 			return fmt.Errorf("delete: failed to get resource data: %v", resourceRecord.Rdata)
@@ -126,7 +129,8 @@ 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 {
@@ -190,9 +194,14 @@ 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
 	}
+	if len(record.Spec.Targets) > 1 {
+		return fmt.Errorf("createOrUpdateDNSRecord: only one target is supported, but found %d: targets=%v", len(record.Spec.Targets), record.Spec.Targets)
+	}
 
 	listResult, response, err := p.dnsService.ListResourceRecords(listOpt)
 	if err != nil {
+		// Avoid continuing with an invalid list response, as we can't determine
+		// whether to create or update the DNS record, which may lead to further issues.
 		if response == nil || response.StatusCode != http.StatusNotFound {
 			return fmt.Errorf("createOrUpdateDNSRecord: failed to list the dns record: %w", err)
 		}
@@ -201,72 +210,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
 }