Add IBMCloud DNS support #630

SzucsAti · 2021-06-29T09:13:52Z

With this PR, ingress-operators running on IBMCloud IPI/UPI clusters will be able to use CIS as their DNS provider.

This PR is required to implement IBMCloud IPI/UPI.
Enhancement doc: openshift/enhancements#773  
Relevant Installer PR: openshift/installer#4923

openshift-ci · 2021-06-29T09:14:07Z

Hi @SzucsAti. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

sgreene570 · 2021-06-29T13:50:38Z

/ok-to-test

SzucsAti · 2021-06-30T11:06:10Z

/retest

SzucsAti · 2021-06-30T15:37:11Z

/retest

SzucsAti · 2021-07-05T09:54:01Z

Removed unnecessary changes from go.mod.

SzucsAti · 2021-07-05T12:37:46Z

/retest

SzucsAti · 2021-07-05T15:05:01Z

/retest

jeffnowicki · 2021-07-06T16:06:28Z

/cc @Miciah

fabianofranz · 2021-07-07T19:26:11Z

/assign @Miciah

sgreene570 · 2021-07-07T20:04:40Z

manifests/00-ingress-credentials-request.yaml

+    kind: IBMCloudProviderSpec
+  secretRef:
+    name: cloud-credentials
+    namespace: openshift-ingress-operator


Could you please add a newline to end of this file?

sgreene570 · 2021-07-07T21:41:06Z

pkg/dns/ibm/dns.go

+		}
+		if result != nil && result.Result != nil {
+			for _, resultData := range result.Result {
+				if resultData.ID != nil {


Might be cleaner to check if resultData.ID == nil here and return the error on line 121 if it is (rather than indent the rest of the code path)

This suggestion might apply to other place as well

Miciah

Please run go mod tidy and squash.

Miciah · 2021-07-08T03:10:13Z

pkg/dns/ibm/dns.go

+
+		dnsService, err := dnsrecordsv1.NewDnsRecordsV1(options)
+		if err != nil {
+			return nil, fmt.Errorf("failed to create a new DNS Service instance: %v", err)


We've been moving towards using %w where possible with fmt.Errorf and the like:

Suggested change

return nil, fmt.Errorf("failed to create a new DNS Service instance: %v", err)

return nil, fmt.Errorf("failed to create a new DNS Service instance: %w", err)

That said, we already have a lot of old code that we need to update to use %w, so we can get this too in a follow-up if you prefer.

Miciah · 2021-07-08T03:13:28Z

pkg/dns/ibm/dns.go

+	if err := validateInputDNSData(record, zone); err != nil {
+		return fmt.Errorf("delete: invalid dns input data: %v", err)
+	}
+	dnsService := p.dnsServices[zone.ID]


We cannot be sure at this point that we have a DNS service instance for the given zone, can we?

Suggested change

dnsService := p.dnsServices[zone.ID]

dnsService, ok := p.dnsServices[zone.ID]

if !ok {

return fmt.Errorf("delete: unknown zone: %v", zone.ID)

}

Miciah · 2021-07-08T03:14:54Z

pkg/dns/ibm/dns.go

+	if err := validateInputDNSData(record, zone); err != nil {
+		return fmt.Errorf("createOrUpdateDNSRecord: invalid dns input data: %v", err)
+	}
+	dnsService := p.dnsServices[zone.ID]


Suggested change

dnsService := p.dnsServices[zone.ID]

dnsService, ok := p.dnsServices[zone.ID]

if !ok {

return fmt.Errorf("createOrUpdateDNSRecord: unknown zone: %v", zone.ID)

}

Miciah · 2021-07-08T03:15:53Z

pkg/dns/ibm/dns.go

+		listOpt.SetContent(target)
+		result, _, err := dnsService.ListAllDnsRecords(listOpt)
+		if err != nil {
+			return fmt.Errorf("createOrUpdateDNSRecord: failed to list the dns record: %v", err)


Why don't you check for http.StatusNotFound here as you do in the delete logic? Especially in the case of a create, don't you expect to get a "not found" response to this list operation?

Miciah · 2021-07-08T03:23:11Z

pkg/dns/ibm/dns.go

+	if record == nil {
+		return fmt.Errorf("validateInputDNSData: dns record is nil")
+	}
+	if record.Spec.DNSName == "" {
+		return fmt.Errorf("validateInputDNSData: dns record name is empty")
+	}
+	if record.Spec.RecordType == "" {
+		return fmt.Errorf("validateInputDNSData: dns record type is empty")
+	}
+	if len(record.Spec.Targets) == 0 {
+		return fmt.Errorf("validateInputDNSData: dns record content is empty")
+	}
+	if zone.ID == "" {
+		return fmt.Errorf("validateInputDNSData: dns zone id is empty")
+	}
+	return nil


Would it be worth aggregating errors like you do in validateDNSServices?

var errs []error if record == nil { errs = append(errs, fmt.Errorf("validateInputDNSData: dns record is nil")) } else { if len(record.Spec.DNSName) == 0 { errs = append(errs, fmt.Errorf("validateInputDNSData: dns record name is empty")) } if len(record.Spec.RecordType) == 0 { errs = append(errs, fmt.Errorf("validateInputDNSData: dns record type is empty")) } if len(record.Spec.Targets) == 0 { errs = append(errs, fmt.Errorf("validateInputDNSData: dns record content is empty")) } } if len(zone.ID) == 0 { errs = append(errs, fmt.Errorf("validateInputDNSData: dns zone id is empty")) } return kerrors.NewAggregate(errs)

Miciah · 2021-07-08T03:45:05Z

pkg/dns/ibm/dns_test.go

+
+	provider := &Provider{}
+
+	dnsService, err := dnsclient.NewFake()
+
+	provider.dnsServices = make(map[string]dnsclient.DnsClient)
+
+	zone := configv1.DNSZone{
+		ID: "zoneID",
+	}
+
+	provider.dnsServices[zone.ID] = dnsService
+
+	if err != nil {
+		t.Error("failed to create fakeClient")
+		return
+	}


You can use t.Fatal instead of t.Error plus return, and the initialization can be re-ordered and more logically grouped:

zone := configv1.DNSZone{ ID: "zoneID", } dnsService, err := dnsclient.NewFake() if err != nil { t.Fatal("failed to create fakeClient") } provider := &Provider{} provider.dnsServices = map[string]dnsclient.DnsClient{ zone.ID: dnsService, }

Likewise in TestCreateOrUpdate.

Miciah · 2021-07-08T03:53:36Z

pkg/dns/ibm/dns.go

+			if len(result.Result) == 0 {
+				createOpt := dnsService.NewCreateDnsRecordOptions()
+				createOpt.SetName(record.Spec.DNSName)
+				createOpt.SetType(string(record.Spec.RecordType))
+				createOpt.SetContent(target)
+				_, _, err := dnsService.CreateDnsRecord(createOpt)
+				if err != nil {
+					return fmt.Errorf("createOrUpdateDNSRecord: failed to create the dns record: %v", err)
+				}
+			} else {
+				updateOpt := dnsService.NewUpdateDnsRecordOptions(*result.Result[0].ID)
+				updateOpt.SetName(record.Spec.DNSName)
+				updateOpt.SetType(string(record.Spec.RecordType))
+				updateOpt.SetContent(record.Spec.Targets[0])
+				_, _, err := dnsService.UpdateDnsRecord(updateOpt)
+				if err != nil {
+					return fmt.Errorf("createOrUpdateDNSRecord: failed to update the dns record: %v", err)
+				}
+			}


Why not set TTL?

createOpt.SetContent(target) createOpt.SetTTL(record.Spec.RecordTTL) // ... } else { // ... updateOpt.SetContent(target) updateOpt.SetTTL(record.Spec.RecordTTL)

Miciah · 2021-07-08T03:54:52Z

pkg/dns/ibm/dns_test.go

+			if tc.expectedErr {
+				if err == nil {
+					t.Error("Expected error, but err is nil")
+				}
+			}


Why not also check !tc.expectedErr && err != nil?

Miciah · 2021-07-08T03:56:17Z

pkg/dns/ibm/dns_test.go

+			recordedCall, _ := dnsService.RecordedCall(record.Spec.DNSName)
+
+			if recordedCall != tc.recordedCall {
+				t.Errorf("expected the dns client %s func to be called, but found %s instead", tc.recordedCall, recordedCall)


Using %q may make this more readable, especially if one or both values are empty:

Suggested change

t.Errorf("expected the dns client %s func to be called, but found %s instead", tc.recordedCall, recordedCall)

t.Errorf("expected the dns client %q func to be called, but found %q instead", tc.recordedCall, recordedCall)

Miciah · 2021-07-08T04:00:33Z

pkg/dns/ibm/dns_test.go

+			if tc.expectedErr {
+				if err == nil {
+					t.Error("Expected error, but err is nil")
+				}
+			}


Check !tc.expectedErr && err != nil:

if !tc.expectedErr && err != nil { t.Errorf("Expected nil err, got %v", err) }

If you do that, I think you'll find that my other comment about checking for "not found" errors in the create/update logic has merit. * grin *.

Hm, on second look, maybe the "not found" errors aren't relevant. This check causes the following failure:

--- FAIL: TestCreateOrUpdate (0.00s) --- FAIL: TestCreateOrUpdate/listFail (0.00s) dns_test.go:261: Expected nil err, got createOrUpdateDNSRecord: failed to update the dns record: updateDnsRecord: inputs don't match

That can be fixed by adding updateDnsRecordInputOutput to the test case:

{ desc: "listFail", DNSName: "testUpdate", recordedCall: "PUT", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ OutputError: nil, OutputStatusCode: http.StatusNotFound, }, updateDnsRecordInputOutput: dnsclient.UpdateDnsRecordInputOutput{ InputId: "testUpdate", OutputError: nil, OutputStatusCode: http.StatusOK, }, expectedErr: false, },

Ahh, I think the test case should actually look like this:

{ desc: "listFail", DNSName: "testUpdate", recordedCall: "PUT", listAllDnsRecordsInputOutput: dnsclient.ListAllDnsRecordsInputOutput{ OutputError: errors.New("Error in ListAllDnsRecords"), OutputStatusCode: http.StatusNotFound, }, updateDnsRecordInputOutput: dnsclient.UpdateDnsRecordInputOutput{ InputId: "testUpdate", OutputError: nil, OutputStatusCode: http.StatusOK, }, expectedErr: false, },

Then I get the following failure:

--- FAIL: TestCreateOrUpdate (0.00s) --- FAIL: TestCreateOrUpdate/listFail (0.00s) dns_test.go:266: Expected nil err, got createOrUpdateDNSRecord: failed to list the dns record: Error in ListAllDnsRecords

But if I fix createOrUpdateDNSRecord as follows:

result, response, err := dnsService.ListAllDnsRecords(listOpt) if err != nil { if response.StatusCode != http.StatusNotFound { return fmt.Errorf("createOrUpdateDNSRecord: failed to list the dns record: %w", err) } continue }

Then the test passes again.

SzucsAti · 2021-07-08T14:48:08Z

Thank you for the reviews! Finished with resolving your comments and implemented the requested changes.

Miciah · 2021-07-08T15:39:20Z

pkg/dns/ibm/dns.go

+			if err != nil && delResponse.StatusCode != http.StatusNotFound {
+				return fmt.Errorf("delete: failed to delete the dns record: %w", err)
+			}
+			log.Info("deleted DNS record", "record", record.Spec, "zone", zone)


This will log "deleted DNS record" even if the record didn't exist. Maybe that's all right? ¯\_(ツ)_/¯

Miciah · 2021-07-08T15:55:48Z

pkg/operator/controller/dns/controller.go

 		}
 		dnsProvider = provider
+	case configv1.IBMCloudPlatformType:
+		if infraStatus.ControlPlaneTopology != configv1.ExternalTopologyMode {


I asked asked about a similar if infraStatus.ControlPlaneTopology != configv1.ExternalTopologyMode check in createDNSProviderIfNeeded, but I forgot to comment on this. Can this check be done for all providers?

Yes it can. Changed it accordingly.

Miciah · 2021-07-09T20:56:55Z

pkg/dns/ibm/dns.go

+			if err != nil && delResponse.StatusCode != http.StatusNotFound {
+				return fmt.Errorf("delete: failed to delete the dns record: %w", err)
+			}
+			if delResponse.StatusCode != http.StatusNotFound {


Do we need to check to make sure delResponse isn't nil? Come to think of it, we might need to check for nil values where we use the result or response value from dnsService.ListAllDnsRecords as well.

Miciah · 2021-07-12T23:27:03Z

pkg/operator/controller/dns/controller.go

-		name := types.NamespacedName{Namespace: r.config.Namespace, Name: cloudCredentialsSecretName}
-		if err := r.cache.Get(context.TODO(), name, creds); err != nil {
-			return fmt.Errorf("failed to get cloud credentials from secret %s: %v", name, err)
+		if (r.infraConfig != nil && infraConfig.Status.ControlPlaneTopology != configv1.ExternalTopologyMode) || r.infraConfig == nil {


The condition can be simplified:

Suggested change

if (r.infraConfig != nil && infraConfig.Status.ControlPlaneTopology != configv1.ExternalTopologyMode) || r.infraConfig == nil {

if r.infraConfig == nil || infraConfig.Status.ControlPlaneTopology != configv1.ExternalTopologyMode {

But really, why check r.infraConfig at all in this switch statement? If the condition r.infraConfig == nil is true, then that means that the if r.infraConfig == nil || ... statement below will set needUpdate = true, and if infraConfig.Status.ControlPlaneTopology == configv1.ExternalTopologyMode is true, then when we call createDNSProvider, it will return without using creds. So I think what you had in the previous version of this PR is sufficient:

Suggested change

if (r.infraConfig != nil && infraConfig.Status.ControlPlaneTopology != configv1.ExternalTopologyMode) || r.infraConfig == nil {

if infraConfig.Status.ControlPlaneTopology != configv1.ExternalTopologyMode {

Miciah · 2021-07-12T23:30:07Z

pkg/operator/controller/dns/controller.go

+			}
 		}
+	case configv1.IBMCloudPlatformType:
+		if r.infraConfig != nil && infraConfig.Status.ControlPlaneTopology != configv1.ExternalTopologyMode {


In https://github.com/openshift/cluster-ingress-operator/compare/634e4b2c2a6f74899a3e310bc0c5c28d449d611d..7000e8a956b886d99353c456ddbb4a2744389b2f, you deleted this separate case branch and added configv1.IBMCloudPlatformType to the case above, and then in https://github.com/openshift/cluster-ingress-operator/compare/75f62202398fee26e230891ec54bc2878f67ba88..ca476de7671bcaadaef41712615bc9db0361d29f, you added this case back with this condition. I don't see why we need the two separate case branches. Is it necessary to skip initialization of the DNS provider on IBM Cloud when r.infraConfig == nil? We should avoid special cases where possible (and it would be nice to avoid the code duplication as well if the special casing isn't required).

Sorry for the trouble. Internally we had a misunderstanding. Changes reverted.

Miciah · 2021-07-12T23:41:47Z

pkg/operator/controller/dns/controller.go

 		return &dns.FakeProvider{}, nil
 	}

+	if infraStatus != nil && infraStatus.ControlPlaneTopology == configv1.ExternalTopologyMode {


infraStatus cannot be nil.

Suggested change

if infraStatus != nil && infraStatus.ControlPlaneTopology == configv1.ExternalTopologyMode {

if infraStatus.ControlPlaneTopology == configv1.ExternalTopologyMode {

Miciah · 2021-07-14T13:52:42Z

Looks great! Thanks for your patience!
/approve/
/lgtm

Miciah · 2021-07-14T13:52:47Z

/approve

openshift-ci · 2021-07-14T13:52:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah, SzucsAti

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Miciah]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 29, 2021

openshift-ci bot requested review from alebedev87 and sgreene570 June 29, 2021 09:14

openshift-ci bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 29, 2021

openshift-ci bot requested a review from Miciah July 6, 2021 16:06

openshift-ci bot assigned Miciah Jul 7, 2021

sgreene570 reviewed Jul 7, 2021

View reviewed changes

Miciah reviewed Jul 8, 2021

View reviewed changes

Miciah reviewed Jul 9, 2021

View reviewed changes

Miciah reviewed Jul 13, 2021

View reviewed changes

Add IBMCloud dns support

eb0ef52

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2021

openshift-merge-robot merged commit fd7937d into openshift:master Jul 14, 2021

Miciah mentioned this pull request Oct 18, 2021

[release-4.9] Bug 2014938: Use fake dns provider with external cp topology only in IBM Cloud case #666

Merged

	return nil, fmt.Errorf("failed to create a new DNS Service instance: %v", err)
	return nil, fmt.Errorf("failed to create a new DNS Service instance: %w", err)

-	dnsService := p.dnsServices[zone.ID]
+	dnsService, ok := p.dnsServices[zone.ID]
+	if !ok {
+		return fmt.Errorf("delete: unknown zone: %v", zone.ID)
+	}

	t.Errorf("expected the dns client %s func to be called, but found %s instead", tc.recordedCall, recordedCall)
	t.Errorf("expected the dns client %q func to be called, but found %q instead", tc.recordedCall, recordedCall)

	if (r.infraConfig != nil && infraConfig.Status.ControlPlaneTopology != configv1.ExternalTopologyMode) \|\| r.infraConfig == nil {
	if r.infraConfig == nil \|\| infraConfig.Status.ControlPlaneTopology != configv1.ExternalTopologyMode {

	if (r.infraConfig != nil && infraConfig.Status.ControlPlaneTopology != configv1.ExternalTopologyMode) \|\| r.infraConfig == nil {
	if infraConfig.Status.ControlPlaneTopology != configv1.ExternalTopologyMode {

	if infraStatus != nil && infraStatus.ControlPlaneTopology == configv1.ExternalTopologyMode {
	if infraStatus.ControlPlaneTopology == configv1.ExternalTopologyMode {

Add IBMCloud DNS support #630

Add IBMCloud DNS support #630

Uh oh!

Conversation

SzucsAti commented Jun 29, 2021

Uh oh!

openshift-ci bot commented Jun 29, 2021

Uh oh!

sgreene570 commented Jun 29, 2021

Uh oh!

SzucsAti commented Jun 30, 2021

Uh oh!

SzucsAti commented Jun 30, 2021

Uh oh!

SzucsAti commented Jul 5, 2021

Uh oh!

SzucsAti commented Jul 5, 2021

Uh oh!

SzucsAti commented Jul 5, 2021

Uh oh!

jeffnowicki commented Jul 6, 2021

Uh oh!

fabianofranz commented Jul 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Miciah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SzucsAti commented Jul 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Miciah commented Jul 14, 2021

Uh oh!

Miciah commented Jul 14, 2021

Uh oh!

openshift-ci bot commented Jul 14, 2021