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`.

Modify TestScopeChange to check for `Available=True` after deleting the
service to add test coverage for this issue.

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
 }

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

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

pkg/dns/ibm/public/cis_provider_test.go

@@ -257,15 +257,21 @@ 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},
 				OutputResult: &dnsrecordsv1.ListDnsrecordsResp{
 					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",

test/e2e/operator_test.go

@@ -1575,7 +1575,7 @@ func TestScopeChange(t *testing.T) {
 			Status: operatorv1.ConditionFalse,
 		}
 		if err := waitForIngressControllerCondition(t, kclient, 1*time.Minute, name, progressingFalse); err != nil {
-			t.Fatalf("failed to observe the ingresscontroller report Progressing=True: %v", err)
+			t.Fatalf("failed to observe the ingresscontroller report Progressing=False: %v", err)
 		}
 	case configv1.AzurePlatformType, configv1.GCPPlatformType:
 		err := wait.PollImmediate(5*time.Second, 5*time.Minute, func() (bool, error) {
@@ -1636,6 +1636,12 @@ func TestScopeChange(t *testing.T) {
 	if err != nil {
 		t.Fatalf("expected load balancer to become internal: %v", err)
 	}
+
+	// Verify that the IngressController provisions successfully to identify bugs like OCPBUGS-32776.
+	if err := waitForIngressControllerCondition(t, kclient, 5*time.Minute, name, availableNotProgressingConditionsForIngressControllerWithLoadBalancer...); err != nil {
+		t.Fatalf("failed to observe expected conditions: %v", err)
+	}
+
 }
 
 // TestNodePortServiceEndpointPublishingStrategy creates an ingresscontroller