Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0b9e89b
Clear IBM DNS Provider Unit Tests CallHistory
gcs278 Aug 22, 2024
932deda
Add Missing Test Cases in IBM Cloud Unit Tests
gcs278 Aug 22, 2024
e486a51
OCPBUGS-32776: Fix IBM Public Cloud DNS Provider Update Logic
gcs278 Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 37 additions & 44 deletions pkg/dns/ibm/private/client/fake_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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}
Expand All @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we check the InputId against the Name here? Can you add a comment to explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this mainly to be consistent with the existing patterns, but it ensures that the
incoming createResourceRecordOptions.Name parameter is getting configured correctly. InputID is equivalent to the name, it's just named generically to keep all update/create/delete structs similar.

I'll add a comment.

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
Expand Down
120 changes: 66 additions & 54 deletions pkg/dns/ibm/private/dnssvcs_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand All @@ -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")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not continue here instead of returning an error? Wouldn't we want to find a matching resourceRecord if possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was added because if the resourceRecord.ID is nil, you'll get a nil pointer dereference later when going to delete it, so no value in continuing. I added it because I saw cis_provider.go had it, but this provider didn't have it:

cluster-ingress-operator/pkg/dns/ibm/public/cis_provider.go

Line 124 in dbdff16

return fmt.Errorf("delete: 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this is a carried-over TODO, but should it be done now in order to fix the bug? Or is it out of scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's out of scope for this bug since it can be fixed independently.

This TODO is for when someone changes the DNSRecord type from CNAME to A, or visa versa, that it won't update the dns record type. That's not a big deal for OpenShift actually, because IBMCloud only creates CNAME records, but I can see it as a low priority "completeness" TODO.

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
}
Loading