diff --git a/pkg/dns/ibm/private/client/fake_client.go b/pkg/dns/ibm/private/client/fake_client.go index 25b4b53af..1dcf71393 100644 --- a/pkg/dns/ibm/private/client/fake_client.go +++ b/pkg/dns/ibm/private/client/fake_client.go @@ -5,7 +5,6 @@ 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 { @@ -13,25 +12,30 @@ type FakeDnsClient struct { 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,22 +81,17 @@ 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 @@ -113,8 +99,15 @@ func (FakeDnsClient) NewResourceRecordInputRdataRdataCnameRecord(cname string) ( 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 diff --git a/pkg/dns/ibm/private/dnssvcs_provider.go b/pkg/dns/ibm/private/dnssvcs_provider.go index 3b6adacc5..384836799 100644 --- a/pkg/dns/ibm/private/dnssvcs_provider.go +++ b/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,15 @@ 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.Error(nil, "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 { + // 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 +211,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 408d8de86..8a0010506 100644 --- a/pkg/dns/ibm/private/dnssvcs_provider_test.go +++ b/pkg/dns/ibm/private/dnssvcs_provider_test.go @@ -6,10 +6,16 @@ import ( "strings" "testing" + "github.com/IBM/go-sdk-core/v5/core" + "github.com/IBM/networking-go-sdk/dnssvcsv1" + configv1 "github.com/openshift/api/config/v1" iov1 "github.com/openshift/api/operatoringress/v1" dnsclient "github.com/openshift/cluster-ingress-operator/pkg/dns/ibm/private/client" + "github.com/stretchr/testify/assert" + + "k8s.io/utils/pointer" ) func Test_Delete(t *testing.T) { @@ -35,82 +41,199 @@ func Test_Delete(t *testing.T) { expectErrorContains string }{ { - desc: "happy path", + desc: "delete happy path A record type", recordedCall: "DELETE", DNSName: "testDelete", target: "11.22.33.44", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + ID: pointer.String("testDelete"), + Name: pointer.String("testDelete"), + Rdata: map[string]interface{}{"ip": "11.22.33.44"}, + Type: pointer.String(string(iov1.ARecordType)), + }}, + }, }, deleteDnsRecordInputOutput: dnsclient.DeleteDnsRecordInputOutput{ - InputId: "testDelete", - OutputError: nil, - OutputStatusCode: http.StatusOK, + InputId: "testDelete", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, }, }, { - desc: "listFail", + desc: "delete happy path CNAME record type", recordedCall: "DELETE", DNSName: "testDelete", - target: "11.22.33.44", + target: "example.com", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusNotFound, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + ID: pointer.String("testDelete"), + Name: pointer.String("testDelete"), + Rdata: map[string]interface{}{"cname": "example.com"}, + Type: pointer.String(string(iov1.CNAMERecordType)), + }}, + }, }, deleteDnsRecordInputOutput: dnsclient.DeleteDnsRecordInputOutput{ - InputId: "testDelete", - OutputError: nil, - OutputStatusCode: http.StatusOK, + InputId: "testDelete", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, }, }, { - desc: "listFailError", - recordedCall: "DELETE", - DNSName: "testDelete", - target: "11.22.33.44", + desc: "list failure with 404", + DNSName: "testDelete", + target: "11.22.33.44", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: errors.New("error in ListAllDnsRecords"), - OutputStatusCode: http.StatusRequestTimeout, + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusNotFound}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{}, + }, }, - deleteDnsRecordInputOutput: dnsclient.DeleteDnsRecordInputOutput{ - InputId: "testDelete", - OutputError: nil, - OutputStatusCode: http.StatusOK, + }, + { + desc: "list failure (not 404)", + DNSName: "testDelete", + target: "11.22.33.44", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, }, expectErrorContains: "error in ListAllDnsRecords", }, { - desc: "deleteRecordNotFound", + 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", DNSName: "testDelete", target: "11.22.33.44", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + ID: pointer.String("testDelete"), + Name: pointer.String("testDelete"), + Rdata: map[string]interface{}{"ip": "11.22.33.44"}, + Type: pointer.String(string(iov1.ARecordType)), + }}, + }, }, deleteDnsRecordInputOutput: dnsclient.DeleteDnsRecordInputOutput{ - InputId: "testDelete", - OutputError: errors.New("error in DeleteDnsRecord"), - OutputStatusCode: http.StatusNotFound, + InputId: "testDelete", + OutputError: errors.New("error in DeleteDnsRecord"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusNotFound}, }, }, { - desc: "deleteError", + desc: "delete failure (not 404)", recordedCall: "DELETE", DNSName: "testDelete", target: "11.22.33.44", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + ID: pointer.String("testDelete"), + Name: pointer.String("testDelete"), + Rdata: map[string]interface{}{"ip": "11.22.33.44"}, + Type: pointer.String(string(iov1.ARecordType)), + }}, + }, }, deleteDnsRecordInputOutput: dnsclient.DeleteDnsRecordInputOutput{ - InputId: "testDelete", - OutputError: errors.New("error in DeleteDnsRecord"), - OutputStatusCode: http.StatusRequestTimeout, + InputId: "testDelete", + OutputError: errors.New("error in DeleteDnsRecord"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, }, expectErrorContains: "error in DeleteDnsRecord", }, + { + desc: "list success but mismatching target", + DNSName: "testDelete", + target: "11.22.33.44", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + ID: pointer.String("testDelete"), + Name: pointer.String("testDelete"), + Rdata: map[string]interface{}{"ip": "22.33.44.55"}, + Type: pointer.String(string(iov1.ARecordType)), + }}, + }, + }, + }, + { + desc: "list success but record type is nil", + DNSName: "testDelete", + target: "11.22.33.44", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + ID: pointer.String("testDelete"), + Name: pointer.String("testDelete"), + Rdata: map[string]interface{}{"ip": "11.22.33.44"}, + }}, + }, + }, + expectErrorContains: "delete: failed to get resource type, resourceRecord.Type is nil", + }, + { + desc: "list success but nil results", + DNSName: "testDelete", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + }, + expectErrorContains: "delete: ListResourceRecords returned nil as result", + }, + { + desc: "list success but nil dns record list", + DNSName: "testDelete", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{}, + }, + }, + { + desc: "list failure and nil response", + DNSName: "testList", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: nil, + OutputResult: &dnssvcsv1.ListResourceRecords{}, + }, + expectErrorContains: "delete: failed to list the dns record: error in ListAllDnsRecords", + }, { desc: "empty DNSName", DNSName: "", @@ -121,6 +244,7 @@ func Test_Delete(t *testing.T) { } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + dnsService.ClearCallHistory() record := iov1.DNSRecord{ Spec: iov1.DNSRecordSpec{ @@ -131,8 +255,6 @@ func Test_Delete(t *testing.T) { }, } - tc.listAllDnsRecordsInputOutput.RecordName = tc.DNSName - tc.listAllDnsRecordsInputOutput.RecordTarget = tc.target dnsService.ListAllDnsRecordsInputOutput = tc.listAllDnsRecordsInputOutput dnsService.DeleteDnsRecordInputOutput = tc.deleteDnsRecordInputOutput @@ -176,65 +298,207 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { recordedCall string listAllDnsRecordsInputOutput dnsclient.ListAllDnsRecordsInputOutput updateDnsRecordInputOutput dnsclient.UpdateDnsRecordInputOutput + createDnsRecordInputOutput dnsclient.CreateDnsRecordInputOutput expectErrorContains string }{ { - desc: "happy path", + desc: "update happy path A record type", DNSName: "testUpdate", target: "11.22.33.44", recordedCall: "PUT", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + ID: pointer.String("testUpdate"), + Name: pointer.String("testUpdate"), + Rdata: map[string]interface{}{"ip": "11.22.33.44"}, + Type: pointer.String(string(iov1.ARecordType)), + }}, + }, }, updateDnsRecordInputOutput: dnsclient.UpdateDnsRecordInputOutput{ - InputId: "testUpdate", - OutputError: nil, - OutputStatusCode: http.StatusOK, + InputId: "testUpdate", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, }, }, { - desc: "listFail", + desc: "update happy path CNAME record type", DNSName: "testUpdate", - target: "11.22.33.44", + target: "example.com", recordedCall: "PUT", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: errors.New("error in ListAllDnsRecords"), - OutputStatusCode: http.StatusNotFound, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + ID: pointer.String("testUpdate"), + Name: pointer.String("testUpdate"), + Rdata: map[string]interface{}{"cname": "example.com"}, + Type: pointer.String(string(iov1.CNAMERecordType)), + }}, + }, }, updateDnsRecordInputOutput: dnsclient.UpdateDnsRecordInputOutput{ - InputId: "testUpdate", - OutputError: nil, - OutputStatusCode: http.StatusOK, + InputId: "testUpdate", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, }, }, { - desc: "listFailError", - DNSName: "testUpdate", + desc: "list failure with 404", + DNSName: "testCreate", target: "11.22.33.44", - recordedCall: "PUT", + recordedCall: "CREATE", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusNotFound}, + OutputResult: &dnssvcsv1.ListResourceRecords{}, + }, + createDnsRecordInputOutput: dnsclient.CreateDnsRecordInputOutput{ + InputId: "testCreate", + OutputError: nil, + OutputResponse: &core.DetailedResponse{ + StatusCode: http.StatusOK, + }, + }, + }, + { + desc: "list failure (not 404)", + DNSName: "testUpdate", + target: "11.22.33.44", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: errors.New("error in ListAllDnsRecords"), - OutputStatusCode: http.StatusRequestTimeout, + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, }, expectErrorContains: "error in ListAllDnsRecords", }, { - desc: "updateError", + 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", target: "11.22.33.44", recordedCall: "PUT", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + ID: pointer.String("testUpdate"), + Name: pointer.String("testUpdate"), + Rdata: map[string]interface{}{"ip": "11.22.33.44"}, + Type: pointer.String(string(iov1.ARecordType)), + }}, + }, }, updateDnsRecordInputOutput: dnsclient.UpdateDnsRecordInputOutput{ - InputId: "testUpdate", - OutputError: errors.New("error in UpdateDnsRecord"), - OutputStatusCode: http.StatusRequestTimeout, + InputId: "testUpdate", + OutputError: errors.New("error in UpdateDnsRecord"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, }, expectErrorContains: "error in UpdateDnsRecord", }, + { + desc: "create happy path", + DNSName: "testCreate", + target: "11.22.33.44", + recordedCall: "CREATE", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{}, + }, + createDnsRecordInputOutput: dnsclient.CreateDnsRecordInputOutput{ + InputId: "testCreate", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + }, + }, + { + desc: "create error", + DNSName: "testCreate", + recordedCall: "CREATE", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{}, + }, + createDnsRecordInputOutput: dnsclient.CreateDnsRecordInputOutput{ + InputId: "testCreate", + OutputError: errors.New("error in CreateDnsRecord"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, + }, + expectErrorContains: "error in CreateDnsRecord", + }, + { + desc: "list success but record type is nil", + DNSName: "testList", + target: "11.22.33.44", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{ + ResourceRecords: []dnssvcsv1.ResourceRecord{{ + ID: pointer.String("testList"), + Name: pointer.String("testList"), + Rdata: map[string]interface{}{"ip": "11.22.33.44"}, + }}, + }, + }, + expectErrorContains: "createOrUpdateDNSRecord: failed to get resource type, resourceRecord.Type is nil", + }, + { + desc: "list success but nil results", + DNSName: "testList", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + }, + expectErrorContains: "createOrUpdateDNSRecord: ListResourceRecords returned nil as result", + }, + { + desc: "list success but nil dns record list", + DNSName: "testCreate", + recordedCall: "CREATE", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnssvcsv1.ListResourceRecords{}, + }, + createDnsRecordInputOutput: dnsclient.CreateDnsRecordInputOutput{ + InputId: "testCreate", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + }, + }, + { + desc: "list failure and nil response", + DNSName: "testList", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: nil, + OutputResult: &dnssvcsv1.ListResourceRecords{}, + }, + expectErrorContains: "createOrUpdateDNSRecord: failed to list the dns record: error in ListAllDnsRecords", + }, { desc: "empty DNSName", DNSName: "", @@ -245,6 +509,7 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + dnsService.ClearCallHistory() record := iov1.DNSRecord{ Spec: iov1.DNSRecordSpec{ @@ -255,12 +520,9 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { }, } - tc.listAllDnsRecordsInputOutput.RecordName = tc.DNSName - tc.listAllDnsRecordsInputOutput.RecordTarget = tc.target - dnsService.ListAllDnsRecordsInputOutput = tc.listAllDnsRecordsInputOutput - dnsService.UpdateDnsRecordInputOutput = tc.updateDnsRecordInputOutput + dnsService.CreateDnsRecordInputOutput = tc.createDnsRecordInputOutput err = provider.createOrUpdateDNSRecord(&record, zone) diff --git a/pkg/dns/ibm/public/cis_provider.go b/pkg/dns/ibm/public/cis_provider.go index 65b89fe77..58dd00fbc 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.Error(nil, "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,45 @@ 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 { + // Avoid continuing with an invalid list response, as we can't determine + // whether to create or update the DNS record, leading to further issues. + 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 30101d2a8..67355f604 100644 --- a/pkg/dns/ibm/public/cis_provider_test.go +++ b/pkg/dns/ibm/public/cis_provider_test.go @@ -6,11 +6,16 @@ import ( "strings" "testing" + "github.com/IBM/go-sdk-core/v5/core" + "github.com/IBM/networking-go-sdk/dnsrecordsv1" + configv1 "github.com/openshift/api/config/v1" iov1 "github.com/openshift/api/operatoringress/v1" + dnsclient "github.com/openshift/cluster-ingress-operator/pkg/dns/ibm/public/client" + "github.com/stretchr/testify/assert" - dnsclient "github.com/openshift/cluster-ingress-operator/pkg/dns/ibm/public/client" + "k8s.io/utils/pointer" ) func Test_Delete(t *testing.T) { @@ -37,77 +42,133 @@ func Test_Delete(t *testing.T) { expectErrorContains string }{ { - desc: "happy path", + desc: "delete happy path", DNSName: "testDelete", recordedCall: "DELETE", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{{ + ID: pointer.String("testDelete"), + Name: pointer.String("testDelete"), + Content: pointer.String("11.22.33.44"), + }}, + }, }, deleteDnsRecordInputOutput: dnsclient.DeleteDnsRecordInputOutput{ - InputId: "testDelete", - OutputError: nil, - OutputStatusCode: http.StatusOK, + InputId: "testDelete", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, }, }, { - desc: "listFailNotFound", - DNSName: "testDelete", - recordedCall: "DELETE", + desc: "list failure with 404", + DNSName: "testDelete", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusNotFound, - }, - deleteDnsRecordInputOutput: dnsclient.DeleteDnsRecordInputOutput{ - InputId: "testDelete", - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusNotFound}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{{}}, + }, }, }, { - desc: "listFailError", - DNSName: "testDelete", - recordedCall: "DELETE", + desc: "list failure (not 404)", + DNSName: "testDelete", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: errors.New("error in ListAllDnsRecords"), - OutputStatusCode: http.StatusRequestTimeout, - }, - deleteDnsRecordInputOutput: dnsclient.DeleteDnsRecordInputOutput{ - InputId: "testDelete", - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, }, expectErrorContains: "error in ListAllDnsRecords", }, { - desc: "deleteRecordNotFound", + desc: "delete failure with 404", DNSName: "testDelete", recordedCall: "DELETE", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{{ + ID: pointer.String("testDelete"), + Name: pointer.String("testDelete"), + Content: pointer.String("11.22.33.44"), + }}, + }, }, deleteDnsRecordInputOutput: dnsclient.DeleteDnsRecordInputOutput{ - InputId: "testDelete", - OutputError: errors.New("error in DeleteDnsRecord"), - OutputStatusCode: http.StatusNotFound, + InputId: "testDelete", + OutputError: errors.New("error in DeleteDnsRecord"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusNotFound}, }, }, { - desc: "deleteError", + desc: "delete failure (not 404)", DNSName: "testDelete", recordedCall: "DELETE", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{{ + ID: pointer.String("testDelete"), + Name: pointer.String("testDelete"), + Content: pointer.String("11.22.33.44"), + }}, + }, }, deleteDnsRecordInputOutput: dnsclient.DeleteDnsRecordInputOutput{ - InputId: "testDelete", - OutputError: errors.New("error in DeleteDnsRecord"), - OutputStatusCode: http.StatusRequestTimeout, + InputId: "testDelete", + OutputError: errors.New("error in DeleteDnsRecord"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, }, expectErrorContains: "error in DeleteDnsRecord", }, + { + desc: "list success but missing ID", + DNSName: "testDelete", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{{ + Name: pointer.String("testDelete"), + Content: pointer.String("11.22.33.44"), + }}, + }, + }, + expectErrorContains: "delete: record id is nil", + }, + { + desc: "list success but nil results", + DNSName: "testDelete", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + }, + expectErrorContains: "delete: ListAllDnsRecords returned nil as result", + }, + { + desc: "list success but nil dns record list", + DNSName: "testDelete", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{}, + }, + expectErrorContains: "delete: ListAllDnsRecords returned nil as result", + }, + { + desc: "list failure and nil response", + DNSName: "testDelete", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: nil, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{}, + }, + expectErrorContains: "delete: failed to list the dns record: error in ListAllDnsRecords", + }, { desc: "empty DNSName", DNSName: "", @@ -117,6 +178,7 @@ func Test_Delete(t *testing.T) { } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + dnsService.ClearCallHistory() record := iov1.DNSRecord{ Spec: iov1.DNSRecordSpec{ @@ -171,6 +233,7 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { recordedCall string listAllDnsRecordsInputOutput dnsclient.ListAllDnsRecordsInputOutput updateDnsRecordInputOutput dnsclient.UpdateDnsRecordInputOutput + createDnsRecordInputOutput dnsclient.CreateDnsRecordInputOutput expectErrorContains string }{ { @@ -178,54 +241,148 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { DNSName: "testUpdate", recordedCall: "PUT", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{{ + ID: pointer.String("testUpdate"), + Name: pointer.String("testUpdate"), + Content: pointer.String("11.22.33.44"), + }}, + }, }, updateDnsRecordInputOutput: dnsclient.UpdateDnsRecordInputOutput{ - InputId: "testUpdate", - OutputError: nil, - OutputStatusCode: http.StatusOK, + InputId: "testUpdate", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, }, }, { - desc: "listFail", - DNSName: "testUpdate", - recordedCall: "PUT", + desc: "list failure with 404", + DNSName: "testCreate", + recordedCall: "CREATE", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: errors.New("Error in ListAllDnsRecords"), - OutputStatusCode: http.StatusNotFound, + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusNotFound}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{}, + }, }, - updateDnsRecordInputOutput: dnsclient.UpdateDnsRecordInputOutput{ - InputId: "testUpdate", - OutputError: nil, - OutputStatusCode: http.StatusOK, + createDnsRecordInputOutput: dnsclient.CreateDnsRecordInputOutput{ + InputId: "testCreate", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, }, }, { - desc: "listFailError", - DNSName: "testUpdate", - recordedCall: "PUT", + desc: "list failure (not 404)", + DNSName: "testUpdate", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: errors.New("error in ListAllDnsRecords"), - OutputStatusCode: http.StatusRequestTimeout, + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, }, expectErrorContains: "error in ListAllDnsRecords", }, { - desc: "updateError", + desc: "update error", DNSName: "testUpdate", recordedCall: "PUT", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ - OutputError: nil, - OutputStatusCode: http.StatusOK, + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{{ + ID: pointer.String("testUpdate"), + Name: pointer.String("testUpdate"), + Content: pointer.String("11.22.33.44"), + }}, + }, }, updateDnsRecordInputOutput: dnsclient.UpdateDnsRecordInputOutput{ - InputId: "testUpdate", - OutputError: errors.New("error in UpdateDnsRecord"), - OutputStatusCode: http.StatusRequestTimeout, + InputId: "testUpdate", + OutputError: errors.New("error in UpdateDnsRecord"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, }, expectErrorContains: "error in UpdateDnsRecord", }, + { + desc: "create happy path", + DNSName: "testCreate", + recordedCall: "CREATE", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{}, + }, + }, + createDnsRecordInputOutput: dnsclient.CreateDnsRecordInputOutput{ + InputId: "testCreate", + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + }, + }, + { + desc: "create error", + DNSName: "testCreate", + recordedCall: "CREATE", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{ + Result: []dnsrecordsv1.DnsrecordDetails{}, + }}, + createDnsRecordInputOutput: dnsclient.CreateDnsRecordInputOutput{ + InputId: "testCreate", + OutputError: errors.New("error in CreateDnsRecord"), + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusRequestTimeout}, + }, + 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", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + }, + expectErrorContains: "createOrUpdateDNSRecord: ListAllDnsRecords returned nil as result", + }, + { + desc: "list success but nil dns record list", + DNSName: "testList", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: nil, + OutputResponse: &core.DetailedResponse{StatusCode: http.StatusOK}, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{}, + }, + expectErrorContains: "createOrUpdateDNSRecord: ListAllDnsRecords returned nil as result", + }, + { + desc: "list failure and nil response", + DNSName: "testList", + listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ + OutputError: errors.New("error in ListAllDnsRecords"), + OutputResponse: nil, + OutputResult: &dnsrecordsv1.ListDnsrecordsResp{}, + }, + expectErrorContains: "createOrUpdateDNSRecord: failed to list the dns record: error in ListAllDnsRecords", + }, { desc: "empty DNSName", DNSName: "", @@ -235,6 +392,7 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { } for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { + dnsService.ClearCallHistory() record := iov1.DNSRecord{ Spec: iov1.DNSRecordSpec{ @@ -244,10 +402,9 @@ func Test_createOrUpdateDNSRecord(t *testing.T) { RecordTTL: 120, }, } - dnsService.ListAllDnsRecordsInputOutput = tc.listAllDnsRecordsInputOutput - dnsService.UpdateDnsRecordInputOutput = tc.updateDnsRecordInputOutput + dnsService.CreateDnsRecordInputOutput = tc.createDnsRecordInputOutput err = provider.createOrUpdateDNSRecord(&record, zone) diff --git a/pkg/dns/ibm/public/client/fake_client.go b/pkg/dns/ibm/public/client/fake_client.go index 13ccd1cd0..fa6c19646 100644 --- a/pkg/dns/ibm/public/client/fake_client.go +++ b/pkg/dns/ibm/public/client/fake_client.go @@ -10,25 +10,33 @@ import ( type FakeDnsClient struct { CallHistory map[string]string DeleteDnsRecordInputOutput DeleteDnsRecordInputOutput + CreateDnsRecordInputOutput CreateDnsRecordInputOutput ListAllDnsRecordsInputOutput ListAllDnsRecordsInputOutput UpdateDnsRecordInputOutput UpdateDnsRecordInputOutput } 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 { - OutputError error - OutputStatusCode int + OutputResult *dnsrecordsv1.ListDnsrecordsResp + OutputError error + OutputResponse *core.DetailedResponse } func NewFake() (*FakeDnsClient, error) { @@ -40,55 +48,46 @@ func (c *FakeDnsClient) RecordedCall(zoneID string) (string, bool) { return call, ok } -func (fdc FakeDnsClient) ListAllDnsRecords(listAllDnsRecordsOptions *dnsrecordsv1.ListAllDnsRecordsOptions) (result *dnsrecordsv1.ListDnsrecordsResp, response *core.DetailedResponse, err error) { - fakeListDnsrecordsResp := &dnsrecordsv1.ListDnsrecordsResp{} +func (c *FakeDnsClient) ClearCallHistory() { + c.CallHistory = map[string]string{} +} - fakeListDnsrecordsResp.Result = append(fakeListDnsrecordsResp.Result, dnsrecordsv1.DnsrecordDetails{ID: listAllDnsRecordsOptions.Name}) +func (fdc FakeDnsClient) ListAllDnsRecords(listAllDnsRecordsOptions *dnsrecordsv1.ListAllDnsRecordsOptions) (result *dnsrecordsv1.ListDnsrecordsResp, response *core.DetailedResponse, err error) { + // 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 +} - resp := &core.DetailedResponse{ - StatusCode: fdc.ListAllDnsRecordsInputOutput.OutputStatusCode, - Headers: map[string][]string{}, - Result: result, - RawResult: []byte{}, +func (fdc FakeDnsClient) CreateDnsRecord(createDnsRecordOptions *dnsrecordsv1.CreateDnsRecordOptions) (result *dnsrecordsv1.DnsrecordResp, response *core.DetailedResponse, err error) { + // Check InputID against the incoming createDnsRecordOptions to ensure the + // createOrUpdateDNSRecord method is using the correct record name in createDnsRecordOptions. + if fdc.CreateDnsRecordInputOutput.InputId != *createDnsRecordOptions.Name { + return nil, nil, errors.New("createDnsRecord: inputs don't match") } - return fakeListDnsrecordsResp, resp, fdc.ListAllDnsRecordsInputOutput.OutputError -} - -func (FakeDnsClient) CreateDnsRecord(createDnsRecordOptions *dnsrecordsv1.CreateDnsRecordOptions) (result *dnsrecordsv1.DnsrecordResp, response *core.DetailedResponse, err error) { - return nil, nil, nil + fdc.CallHistory[*createDnsRecordOptions.Name] = "CREATE" + return nil, fdc.CreateDnsRecordInputOutput.OutputResponse, fdc.CreateDnsRecordInputOutput.OutputError } func (fdc FakeDnsClient) DeleteDnsRecord(deleteDnsRecordOptions *dnsrecordsv1.DeleteDnsRecordOptions) (result *dnsrecordsv1.DeleteDnsrecordResp, response *core.DetailedResponse, err error) { + // Check InputID against the incoming deleteDnsRecordOptions to ensure the + // Delete method is using the correct dnsrecordIdentifier in deleteDnsRecordOptions. if fdc.DeleteDnsRecordInputOutput.InputId != *deleteDnsRecordOptions.DnsrecordIdentifier { return nil, nil, errors.New("deleteDnsRecord: inputs don't match") } - resp := &core.DetailedResponse{ - StatusCode: fdc.DeleteDnsRecordInputOutput.OutputStatusCode, - Headers: map[string][]string{}, - Result: result, - RawResult: []byte{}, - } - fdc.CallHistory[*deleteDnsRecordOptions.DnsrecordIdentifier] = "DELETE" - return nil, resp, fdc.DeleteDnsRecordInputOutput.OutputError + return nil, fdc.DeleteDnsRecordInputOutput.OutputResponse, fdc.DeleteDnsRecordInputOutput.OutputError } func (fdc FakeDnsClient) UpdateDnsRecord(updateDnsRecordOptions *dnsrecordsv1.UpdateDnsRecordOptions) (result *dnsrecordsv1.DnsrecordResp, response *core.DetailedResponse, err error) { + // Check InputID against the incoming updateDnsRecordOptions to ensure the + // createOrUpdateDNSRecord method is using the correct dnsrecordIdentifier in updateDnsRecordOptions. if fdc.UpdateDnsRecordInputOutput.InputId != *updateDnsRecordOptions.DnsrecordIdentifier { return nil, nil, errors.New("updateDnsRecord: inputs don't match") } - resp := &core.DetailedResponse{ - StatusCode: fdc.UpdateDnsRecordInputOutput.OutputStatusCode, - Headers: map[string][]string{}, - Result: result, - RawResult: []byte{}, - } - fdc.CallHistory[*updateDnsRecordOptions.DnsrecordIdentifier] = "PUT" - return nil, resp, fdc.UpdateDnsRecordInputOutput.OutputError + return nil, fdc.UpdateDnsRecordInputOutput.OutputResponse, fdc.UpdateDnsRecordInputOutput.OutputError } func (FakeDnsClient) NewCreateDnsRecordOptions() *dnsrecordsv1.CreateDnsRecordOptions { diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 08cd3a06d..0a98a6d74 100644 --- a/test/e2e/operator_test.go +++ b/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