NE-969: Add IBM DNS Services provider support for private clusters by SzucsAti · Pull Request #796 · openshift/cluster-ingress-operator

SzucsAti · 2022-06-30T14:01:27Z

With this PR, cluster-ingress-operators running on Private IBMCloud IPI/UPI clusters will be able to use DNS Services (https://cloud.ibm.com/docs/dns-svcs?topic=dns-svcs-about-dns-services) as their DNS provider.

Pulling in latest openshift/api changes as it's needed for this PR.

This PR is required for Private IBMCloud IPI/UPI clusters.

jeffnowicki · 2022-06-30T16:44:19Z

/cc @Miciah

Miciah · 2022-06-30T19:05:18Z

/retitle NE-969: Add IBM DNS Services provider support for private clusters

jeffnowicki · 2022-06-30T20:41:22Z

/retest-required

gcs278 · 2022-07-06T16:44:58Z

/assign @gcs278

gcs278 · 2022-07-06T22:13:51Z

Thanks for the PR. We discussed this in our PR review meeting. I'll do a review sometime next week.

gcs278

Looks pretty good, thanks for the PR. I'm have a small concern with code reuse and consistency here. Seems code was adapted from existing code and there are a few duplicate logic sections. I'm just wonder if we could structure to enable better code reuse and less unique logic paths. I will get back to you on maybe a potential suggestion, but at the moment, I don't have any other suggestions other than my comments.

gcs278 · 2022-07-12T23:09:43Z

pkg/operator/controller/dns/controller.go

+		if err != nil {
+			return nil, fmt.Errorf("failed to create IBM CIS DNS manager: %v", err)
+		}
+		log.Info("successfully initialized ibm cis dns provider")


nit IBM CIS DNS is capitalized in error message but not the info, i don't really mind, but I do like consistency.

Personally, I'd prefer using proper names ("IBM CIS DNS", "IBM Cloud DNS Services", and so on) in error messages and log messages. We should use whatever will be most familiar to users or easiest for users to look up in a web search or in product documentation.

gcs278 · 2022-07-12T23:10:11Z

pkg/operator/controller/dns/controller.go

+		if err != nil {
+			return nil, fmt.Errorf("failed to create IBM DNSSVCS DNS manager: %v", err)
+		}
+		log.Info("successfully initialized ibm dnssvcs dns provider")


nit IBM DNSSVCS is capitlized in the error message, but no the info.

gcs278 · 2022-07-12T23:18:47Z

pkg/operator/controller/dns/controller.go

+		log.Info("successfully initialized ibm cis dns provider")
+		return provider, nil
+	} else {
+		provider, err := ibmprivatedns.NewProvider(ibmprivatedns.Config{


This is another nit, but these if statements have a few lines of repeated logic, minus the specific error message. Have you thought about just using the if statement to create the ibm DNS configs then using common logic for ibmprivatedns.NewProvider? If it seems reasonable.

Suggested change

provider, err := ibmprivatedns.NewProvider(ibmprivatedns.Config{

providerCfg := ibmprivatedns.Config{

gcs278 · 2022-07-13T23:12:28Z

pkg/dns/ibm/private/dnssvcs_provider_test.go

+		recordedCall                 string
+		listAllDnsRecordsInputOutput dnsclient.ListAllDnsRecordsInputOutput
+		updateDnsRecordInputOutput   dnsclient.UpdateDnsRecordInputOutput
+		expectedErr                  bool


I've seen some of our test code use test parameters like expectMessageContains. Do you think that would be beneficial to be more granular here? I suppose it would protect against false positives in which expectedErr true, but not necessarily the error we expect.

Makes sense. Added expectErrorContains and updated the tests accordingly.

gcs278 · 2022-07-13T23:14:54Z

pkg/dns/ibm/private/dnssvcs_provider.go

+		}
+	}
+	if result == nil {
+		return fmt.Errorf("delete: invalid result")


Could you make this error more descriptive? It's a bit vague. Result of what operation?
edit: I realize this is adapted from cis_provider.go, but still think they both are vague error message.

gcs278 · 2022-07-13T23:46:18Z

pkg/dns/ibm/private/dnssvcs_provider.go

+	"time"
+
+	"github.com/IBM/go-sdk-core/v5/core"
+	dnssvcsv1 "github.com/IBM/networking-go-sdk/dnssvcsv1"


nit goland is saying this is a "duplicate alias" since the name of the package is the same as the alias.

gcs278 · 2022-07-13T23:49:02Z

pkg/dns/ibm/private/dnssvcs_provider.go

+)
+
+type Provider struct {
+	dnsService dnsclient.DnsClient


something isn't super clear to me, why is there only 1 DnsClient when in cis_provider.go has a map of DnsClient? Maybe that would make a good comment - describing why/the difference.

In case of CIS sdk, zoneID is stored and used in the provider options which is set during client creation. So we decided to create a zoneID to client map, so we don't have to create a client each time there is a dns operation. https://github.com/SzucsAti/cluster-ingress-operator/blob/ibm-dnssvc-support/pkg/dns/ibm/public/cis_provider.go#L45-#L52

In case of DNS-SVCS sdk, zoneID is a required parameter in every function and it's not needed to set in client options. https://github.com/SzucsAti/cluster-ingress-operator/blob/ibm-dnssvc-support/pkg/dns/ibm/private/dnssvcs_provider.go#L44-#L49

Got it, thanks for the explanation.

gcs278 · 2022-07-13T23:56:08Z

pkg/dns/ibm/private/dnssvcs_provider.go

+	config     Config
+}
+
+type Config struct {


this config struct is nearly common with the one in cis_provider.go. I'm curious if we could share.

Couldn't really find a place to put these common functions/structures, so ended up creating common.go in the dns/ibm folder. Moved Config there to be used by both providers.

I think that makes sense to me. I see InstanceID is a bit overloaded since it could be different types of ID's for public or private (CRN ID vs. IstanceID?). But I think this is okay.

gcs278 · 2022-07-14T00:02:22Z

pkg/dns/ibm/private/dnssvcs_provider.go

+	return kerrors.NewAggregate(errs)
+}
+
+func (p *Provider) createOrUpdateDNSRecord(record *iov1.DNSRecord, zone configv1.DNSZone) error {


nit does zone need to be pass-by-value here? Doesn't that do an unnecessary memory copy?
Same comment goes for cis_provider.go to be consistent.

Changed it. (Side note: as I see this change is inconsistent with the way other platform providers do it.)

@SzucsAti yea reviewing this again, I was asking the question, not necessarily suggesting it to be changed. Sorry I should have been more clear with my statement that I wasn't 100% sure.

I was originally thinking pointers are just overall better, but after reading https://medium.com/@meeusdylan/when-to-use-pointers-in-go-44c15fe04eac, it really only matters when there are large complex structures.

If you see this change as inconsistent, I'd recommend changing back. Sorry about that.

Reverted the change.

gcs278 · 2022-07-14T00:02:32Z

pkg/dns/ibm/private/dnssvcs_provider.go

+	return nil
+}
+
+func validateInputDNSData(record *iov1.DNSRecord, zone configv1.DNSZone) error {


nit does zone need to be pass-by-value here? Doesn't that do an unnecessary memory copy?
Same comment goes for cis_provider.go to be consistent.

SzucsAti · 2022-07-14T14:42:10Z

@gcs278 Thank you for the review. Addressed your comments.

I agree that the usage of the providers looks similar. Unfortunately there are noticeable differences when it comes to the providers' sdks, which makes it hard to reuse code for both providers without overengineering, but I'm open to suggestions. I created a common.go file and moved duplicated config/function there based on your comments.

gcs278

thanks for the comment responses. Looking good.

gcs278 · 2022-07-14T20:29:19Z

pkg/operator/controller/dns/controller.go


-//getIbmDNSProvider will intialize the IBM DNS provider
-func getIbmDNSProvider(dnsConfig *configv1.DNS, creds *corev1.Secret, cisInstanceCRN, userAgent string) (*ibmdns.Provider,
+//getIbmDNSProvider will intialize an IBM DNS provider


typo: initialize

gcs278 · 2022-07-14T20:37:54Z

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

+	"errors"
+
+	"github.com/IBM/go-sdk-core/v5/core"
+	dnssvcsv1 "github.com/IBM/networking-go-sdk/dnssvcsv1"


nit this is also a redundant alias.

gcs278 · 2022-07-14T20:39:15Z

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

+
+import (
+	"github.com/IBM/go-sdk-core/v5/core"
+	dnssvcsv1 "github.com/IBM/networking-go-sdk/dnssvcsv1"


nit this is a redundant alias

gcs278 · 2022-07-14T20:41:28Z

pkg/dns/ibm/private/dnssvcs_provider.go

+			}
+		}
+		if listResult == nil {
+			return fmt.Errorf("validateDNSServices: NewListResourceRecordsOptions returned nil as result")


Is a nil results always a bad thing? I noticed in cis_provider.go, they don't validate if the results is nil (but maybe it's different).

Right, this check is not needed. Realised that errors were not aggregated, just returned.. fixed that as well.

gcs278 · 2022-07-14T20:43:47Z

pkg/dns/ibm/private/dnssvcs_provider.go

+
+	// TTL must be one of [1 60 120 300 600 900 1800 3600 7200 18000 43200]
+	useDefaultTTL := true
+	for _, v := range []int64{1, 60, 120, 300, 600, 900, 1800, 3600, 7200, 18000, 43200} {


Got it - thanks I see that now, I confused the logic, but that makes sense now.

gcs278 · 2022-07-14T20:52:54Z

pkg/operator/controller/dns/controller.go

-		dnsProvider, err = getIbmDNSProvider(dnsConfig, creds, cisInstanceCRN, userAgent)
-		if err != nil {
-			return nil, fmt.Errorf("failed to create IBM DNS manager: %v", err)
+		if platformStatus.IBMCloud.CISInstanceCRN != "" {


Now that I look at this more, the isPublic bool feels sorta like leaky abstraction. Should at the function getIbmDNSProvider really be the one determining if it is private or public with operating on platformStatus instead of a specific instanceId? I could be over simplifying, but see if it makes sense.

Not sure if I understand this comment correctly. From just an instanceID value we can't decide on which provider to use. Can't really create getPublicDNSProvider and getPrivateDNSProvider, because there would be too much duplicated logic, also I'm hesitant to make changes to getIbmDNSProvider as PowerVS Platform also uses it.

As far as I know PowerVS also plans to adopt using DNS Services provider in the near future, which will make it easy to simplify and revisit this part of the code. I'd prefer to leave it as it is for now.

Oh okay I missed the PowerVSPlatformType that uses platformStatus.PowerVS.CISInstanceCRN. Never mind, this pattern is fine.

gcs278 · 2022-07-14T20:56:09Z

pkg/dns/ibm/public/cis_provider_test.go

 		listAllDnsRecordsInputOutput dnsclient.ListAllDnsRecordsInputOutput
 		deleteDnsRecordInputOutput   dnsclient.DeleteDnsRecordInputOutput
-		expectedErr                  bool
+		expectErrorContains          string


thanks for also updating this! I like the consistency.

gcs278 · 2022-07-15T15:20:48Z

@Miciah I've done my most of my reviewing, mind taking a look when you get a chance?

gcs278 · 2022-07-27T17:40:24Z

I took another pass. The I did miss your comment response about the pointers, but I agree with your comment about inconsistency. I think we should revert those pointer updates.

Otherwise just waiting on a second pass from Miciah.

Miciah · 2022-07-28T00:47:27Z

pkg/dns/ibm/private/dnssvcs_provider.go

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


If the first outer if condition err != nil is true and the inner if condition response != nil && response.StatusCode != http.StatusNotFound is false because response is nil, then the error is ignored. Should the logic be the following?

Suggested change

if err != nil {

if response != nil && response.StatusCode != http.StatusNotFound {

return fmt.Errorf("delete: failed to list the dns record: %w", err)

}

}

if err != nil {

if response == nil || response.StatusCode != http.StatusNotFound {

return fmt.Errorf("delete: failed to list the dns record: %w", err)

}

}

Agree, changed it.

Miciah · 2022-07-28T01:02:58Z

pkg/dns/ibm/private/dnssvcs_provider.go

+			if *resourceRecord.Type == string(iov1.CNAMERecordType) {
+				resourceRecordTarget = fmt.Sprint(rData["cname"])
+			}
+			if *resourceRecord.Type == string(iov1.ARecordType) {
+				resourceRecordTarget = fmt.Sprint(rData["ip"])
+			}


Can we be absolutely certain that resourceRecord.Type is non-nil? I'd prefer this logic include a nil check.

What if the record has some other type? I'd prefer this logic use a switch with a default clause.

What are the possible types of rData["cname"] and rData["ip"]? Using fmt.Sprint here seems slightly odd.

Suggested change

if *resourceRecord.Type == string(iov1.CNAMERecordType) {

resourceRecordTarget = fmt.Sprint(rData["cname"])

}

if *resourceRecord.Type == string(iov1.ARecordType) {

resourceRecordTarget = fmt.Sprint(rData["ip"])

}

if resourceRecord.Type == nil {

return fmt.Errorf("delete: resource data has record with nil type: %v", resourceRecord)

}

switch *resourceRecord.Type {

case string(iov1.CNAMERecordType):

resourceRecordTarget = fmt.Sprint(rData["cname"])

case string(iov1.ARecordType):

resourceRecordTarget = fmt.Sprint(rData["ip"])

default:

return fmt.Errorf("delete: resource data has record with unknown type: %v", resourceRecord)

// Or should we log and continue?

}

Can we be absolutely certain that resourceRecord.Type is non-nil? I'd prefer this logic include a nil check.

There is a ValidateInputDNSData(record, zone) call at the beginning of the functions that does nil checks.

What if the record has some other type? I'd prefer this logic use a switch with a default clause.

Makes sense. Changed it to switch-case.

What are the possible types of rData["cname"] and rData["ip"]? Using fmt.Sprint here seems slightly odd.

It supposed to be string only. Added a type check just to be safe and changed the way we convert it.

Can we be absolutely certain that resourceRecord.Type is non-nil? I'd prefer this logic include a nil check.

There is a ValidateInputDNSData(record, zone) call at the beginning of the functions that does nil checks.

That checks the input that is used for the ListResourceRecords call, but it doesn't validate the result, right? Is it safe to assume that the result is valid if the input was valid?

Oh correct. Added nil check validation for list result.

Miciah · 2022-07-28T01:03:28Z

pkg/dns/ibm/private/dnssvcs_provider.go

+				resourceRecordTarget = fmt.Sprint(rData["ip"])
+			}
+
+			if resourceRecordTarget == target && *resourceRecord.Name == DNSName {


Is it possible for resourceRecord.Name to be nil? I'd prefer a nil check, just in case.

There is a ValidateInputDNSData(record, zone) call at the beginning of the functions that does nil checks.

Miciah · 2022-07-28T01:04:02Z

pkg/dns/ibm/private/dnssvcs_provider.go

+				if err != nil {
+					if delResponse != nil && delResponse.StatusCode != http.StatusNotFound {


What if err is non-nil and delResponse is nil?

I believe we should return error in that case. Changed it accordingly.

Miciah · 2022-07-28T01:08:17Z

pkg/dns/ibm/private/dnssvcs_provider.go

+	for _, target := range record.Spec.Targets {
+		for _, resourceRecord := range result.ResourceRecords {


Could you swap the inner and outer for loops and move the unmarshaling logic before the loop over targets to avoid repeatedly unmarshaling each resourceRecord when there are multiple targets?

Miciah · 2022-07-28T01:58:19Z

pkg/operator/controller/dns/controller.go

+		if err != nil {
+			return nil, fmt.Errorf("failed to create IBM CIS DNS manager: %v", err)
+		}
+		log.Info("successfully initialized ibm cis dns provider")


Personally, I'd prefer using proper names ("IBM CIS DNS", "IBM Cloud DNS Services", and so on) in error messages and log messages. We should use whatever will be most familiar to users or easiest for users to look up in a web search or in product documentation.

Miciah · 2022-07-28T02:04:05Z

pkg/dns/ibm/common.go

+// Common configuration of ibm providers
+// InstanceID is GUID in case of dns svcs and CRN in case of cis
+type Config struct {
+	APIKey     string
+	InstanceID string
+	UserAgent  string
+	Zones      []string


This would be more Go idiomatic:

Suggested change

// Common configuration of ibm providers

// InstanceID is GUID in case of dns svcs and CRN in case of cis

type Config struct {

APIKey string

InstanceID string

UserAgent string

Zones []string

// Config holds common configuration of IBM DNS providers.

type Config struct {

// APIKey is the IBM Cloud API key that the provider will use.

APIKey string

// InstanceID is GUID in case of dns svcs and CRN in case of cis.

InstanceID string

// UserAgent is the user-agent identifier that the client will use in

// HTTP requests to the IBM Cloud API.

UserAgent string

// Zones is a list of DNS zones in which the DNS provider manages DNS records.

Zones []string

Thank you. Applied your suggestion.

Miciah · 2022-07-28T02:04:21Z

pkg/dns/ibm/common.go

+	Zones      []string
+}
+
+func ValidateInputDNSData(record *iov1.DNSRecord, zone *configv1.DNSZone) error {


Please add a godoc comment.

Miciah · 2022-07-28T02:10:03Z

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

+type DnsClient interface {
+	NewListResourceRecordsOptions(instanceID string, dnszoneID string) *dnssvcsv1.ListResourceRecordsOptions
+	ListResourceRecords(listResourceRecordsOptions *dnssvcsv1.ListResourceRecordsOptions) (result *dnssvcsv1.ListResourceRecords, response *core.DetailedResponse, err error)
+	NewDeleteResourceRecordOptions(instanceID string, dnszoneID string, recordID string) *dnssvcsv1.DeleteResourceRecordOptions
+	DeleteResourceRecord(deleteResourceRecordOptions *dnssvcsv1.DeleteResourceRecordOptions) (response *core.DetailedResponse, err error)
+	NewUpdateResourceRecordOptions(instanceID string, dnszoneID string, recordID string) *dnssvcsv1.UpdateResourceRecordOptions
+	NewResourceRecordUpdateInputRdataRdataCnameRecord(cname string) (_model *dnssvcsv1.ResourceRecordUpdateInputRdataRdataCnameRecord, err error)
+	NewResourceRecordUpdateInputRdataRdataARecord(ip string) (_model *dnssvcsv1.ResourceRecordUpdateInputRdataRdataARecord, err error)
+	UpdateResourceRecord(updateResourceRecordOptions *dnssvcsv1.UpdateResourceRecordOptions) (result *dnssvcsv1.ResourceRecord, response *core.DetailedResponse, err error)
+	NewCreateResourceRecordOptions(instanceID string, dnszoneID string) *dnssvcsv1.CreateResourceRecordOptions
+	NewResourceRecordInputRdataRdataCnameRecord(cname string) (_model *dnssvcsv1.ResourceRecordInputRdataRdataCnameRecord, err error)
+	NewResourceRecordInputRdataRdataARecord(ip string) (_model *dnssvcsv1.ResourceRecordInputRdataRdataARecord, err error)
+	CreateResourceRecord(createResourceRecordOptions *dnssvcsv1.CreateResourceRecordOptions) (result *dnssvcsv1.ResourceRecord, response *core.DetailedResponse, err error)
+	NewGetDnszoneOptions(instanceID string, dnszoneID string) *dnssvcsv1.GetDnszoneOptions
+	GetDnszone(getDnszoneOptions *dnssvcsv1.GetDnszoneOptions) (result *dnssvcsv1.Dnszone, response *core.DetailedResponse, err error)
+}


Seems like this interface should be defined in "github.com/IBM/networking-go-sdk/dnssvcsv1", but I guess we can't do anything about that. Maybe add a comment to say that we're defining this interface to describe the methods defined in dnssvcsv1 for the purpose of having an interface that we can use in the implementation and test code for cluster-ingress-operator's provider logic.

Exactly. Added a comment.

Miciah · 2022-07-28T02:14:03Z

pkg/dns/ibm/private/dnssvcs_provider.go

+	defaultDNSSVCSRecordTTL              = int64(120)
+	DNSSVCSApiEndpoint                   = "https://api.dns-svcs.cloud.ibm.com/v1"
+)


defaultDNSSVCSRecordTTL and DNSSVCSApiEndpoint can be declared as consts. Since DNSSVCSApiEndpoint isn't used outside this package, it doesn't need to be exported. Please add godoc!

Suggested change

defaultDNSSVCSRecordTTL = int64(120)

DNSSVCSApiEndpoint = "https://api.dns-svcs.cloud.ibm.com/v1"

)

)

const (

// defaultDNSSVCSRecordTTL is the default TTL used when a DNS record

// does not specify a valid TTL.

defaultDNSSVCSRecordTTL = int64(120)

// dnsSvcsApiEndpoint is the API endpoint for IBM Cloud DNS Services.

dnsSvcsApiEndpoint = "https://api.dns-svcs.cloud.ibm.com/v1"

)

Actually, could you use dnssvcsv1.DefaultServiceURL and do away with DNSSVCSApiEndpoint entirely?

Using DefaultServiceURL.

cjschaef · 2022-08-08T13:59:11Z

@Miciah @SzucsAti Any further progress on getting this reviewed/approved?
This will soon become a blocker for the IBM Cloud IPI GA, once we start adding Private cluster support in the installer and I'm hoping to avoid that if possible.

SzucsAti · 2022-08-09T14:04:53Z

Created an IPI cluster on IBMCloud and verified that the changes we made don't break the existing CIS integration. DNS Services integration seems to work properly as well. Waiting if there are more suggestions or concerns.

jeffnowicki · 2022-08-10T13:36:25Z

/retest-required

SzucsAti · 2022-08-12T13:02:36Z

Resolved the conflicts. We no longer need to update the openshift/api dependency as it was updated on the master branch.

jeffnowicki · 2022-08-15T13:12:07Z

/retest-required

Miciah · 2022-08-17T23:58:50Z

pkg/dns/ibm/private/dnssvcs_provider.go

+			if value, ok := rData["cname"].(string); ok {
+				resourceRecordTarget = value
+			} else {
+				return fmt.Errorf("delete: resource data has record with unknown rData cname type: %v ", err)


Suggested change

return fmt.Errorf("delete: resource data has record with unknown rData cname type: %v ", err)

return fmt.Errorf("delete: resource data has record with unknown rData cname type: %T", rData["cname"])

Miciah · 2022-08-18T00:00:48Z

pkg/dns/ibm/private/dnssvcs_provider.go

+			if value, ok := rData["ip"].(string); ok {
+				resourceRecordTarget = value
+			} else {
+				return fmt.Errorf("delete: resource data has record with unknown rData ip type: %v ", err)


Suggested change

return fmt.Errorf("delete: resource data has record with unknown rData ip type: %v ", err)

return fmt.Errorf("delete: resource data has record with unknown rData ip type: %T", rData["ip"])

Miciah · 2022-08-18T00:13:31Z

pkg/dns/ibm/private/dnssvcs_provider.go

+				return fmt.Errorf("delete: resource data has record with unknown rData ip type: %v ", err)
+			}
+		default:
+			return fmt.Errorf("delete: resource data has record with unknown type: %v", resourceRecord)


This is going to return an error message that contains pointer addresses. You need to dereference resourceRecord.Type to include useful information:

Suggested change

return fmt.Errorf("delete: resource data has record with unknown type: %v", resourceRecord)

return fmt.Errorf("delete: resource data has record with unknown type: %v", *resourceRecord.Type)

If you need to provide more information, you'll need to dereference each field that you want to include in the error message (or use something more sophisticated, such as go-spew):

Suggested change

return fmt.Errorf("delete: resource data has record with unknown type: %v", resourceRecord)

return fmt.Errorf("delete: resource data has record with unknown type: %v, id: %v", *resourceRecord.Type, *resourceRecord.ID)

Miciah · 2022-08-18T00:21:05Z

pkg/dns/ibm/private/dnssvcs_provider.go

+		}
+
+		for _, target := range record.Spec.Targets {
+			if resourceRecordTarget == target && *resourceRecord.Name == dnsName {


Would it make sense to log something if we're trying to delete a record and we find one that doesn't match?

Suggested change

if resourceRecordTarget == target && *resourceRecord.Name == dnsName {

if *resourceRecord.Name == dnsName {

if resourceRecordTarget != target {

log.Info("delete: ignoring record with matching name but unexpected target", "record", record, "target", resourceRecordTarget)

continue

}

Makes sense. Applied the suggestion.

Miciah · 2022-08-18T00:22:56Z

pkg/dns/ibm/private/dnssvcs_provider.go

+				delOpt := p.dnsService.NewDeleteResourceRecordOptions(p.config.InstanceID, zone.ID, *resourceRecord.ID)
+				delResponse, err := p.dnsService.DeleteResourceRecord(delOpt)
+				if err != nil {
+					if (delResponse != nil && delResponse.StatusCode != http.StatusNotFound) || delResponse == nil {


Suggested change

if (delResponse != nil && delResponse.StatusCode != http.StatusNotFound) || delResponse == nil {

if delResponse == nil || delResponse.StatusCode != http.StatusNotFound {

Miciah · 2022-08-18T01:00:53Z

pkg/operator/controller/dns/controller.go

-//getIbmDNSProvider will intialize the IBM DNS provider
-func getIbmDNSProvider(dnsConfig *configv1.DNS, creds *corev1.Secret, cisInstanceCRN, userAgent string) (*ibmdns.Provider,
-	error) {
+//getIbmDNSProvider initializes and returns an IBM DNS provider instance.


Sorry for being so persnickety, but...

Suggested change

//getIbmDNSProvider initializes and returns an IBM DNS provider instance.

// getIbmDNSProvider initializes and returns an IBM DNS provider instance.

Miciah · 2022-08-18T01:03:37Z

pkg/operator/controller/dns/controller.go

+		if platformStatus.IBMCloud.CISInstanceCRN != "" {
+			dnsProvider, err = getIbmDNSProvider(dnsConfig, creds, platformStatus.IBMCloud.CISInstanceCRN, userAgent, true)
+			if err != nil {
+				return nil, fmt.Errorf("failed to create ibm cis dns provider: %v", err)


Looks like you're changing "IBM CIS DNS" to upper-case elsewhere, right? Anyway, might as well change to using %w while you're updating this code.

Suggested change

return nil, fmt.Errorf("failed to create ibm cis dns provider: %v", err)

return nil, fmt.Errorf("failed to create IBM CIS DNS provider: %w", err)

Actually, getIbmDNSProvider already wraps whatever error it returns with "failed to initialize IBM CIS DNS provider", so is it necessary to wrap the error here? Might as well just return it as-is, no?

Suggested change

return nil, fmt.Errorf("failed to create ibm cis dns provider: %v", err)

return nil, err

Correct, changed it.

Miciah · 2022-08-18T01:05:42Z

pkg/operator/controller/dns/controller.go

+			}
+			dnsProvider, err = getIbmDNSProvider(dnsConfig, creds, matches[rExp.SubexpIndex("guid")], userAgent, false)
+			if err != nil {
+				return nil, fmt.Errorf("failed to create ibm dnssvcs dns provider: %v", err)


Suggested change

return nil, fmt.Errorf("failed to create ibm dnssvcs dns provider: %v", err)

return nil, fmt.Errorf("failed to create IBM Cloud DNS Services provider: %w", err)

or

Suggested change

return nil, fmt.Errorf("failed to create ibm dnssvcs dns provider: %v", err)

return nil, err

per my other comment: #796 (comment)

Changed it.

Miciah · 2022-08-18T01:06:40Z

pkg/operator/controller/dns/controller.go

+	if isPublic {
+		provider, err := ibmpublicdns.NewProvider(providerCfg)
+		if err != nil {
+			return nil, fmt.Errorf("failed to initialize IBM CIS DNS provider: %v", err)


Suggested change

return nil, fmt.Errorf("failed to initialize IBM CIS DNS provider: %v", err)

return nil, fmt.Errorf("failed to initialize IBM CIS DNS provider: %w", err)

Miciah · 2022-08-18T01:06:49Z

pkg/operator/controller/dns/controller.go

+	} else {
+		provider, err := ibmprivatedns.NewProvider(providerCfg)
+		if err != nil {
+			return nil, fmt.Errorf("failed to initialize IBM Cloud DNS Services provider: %v", err)


Suggested change

return nil, fmt.Errorf("failed to initialize IBM Cloud DNS Services provider: %v", err)

return nil, fmt.Errorf("failed to initialize IBM Cloud DNS Services provider: %w", err)

gcs278 · 2022-08-18T19:40:15Z

/lgtm but Miciah should review the responses to his comments and approve

Miciah · 2022-08-19T03:03:14Z

It's looking really good! Thanks!
/approve
/lgtm

We have an issue in CI, which is being addressed here: #818
I'm putting a hold on this PR while that issue is sorted out so the PR doesn't needlessly rerun CI. Once the CI issue is resolved, the hold can be removed with /hold cancel.
/hold

openshift-ci · 2022-08-19T03:03:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Miciah

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

jeffnowicki · 2022-08-19T18:34:26Z

@Miciah / @gcs278 looks like all tests for #818 passed... hopefully it can get reviewed/merged soon so this PR can progress/merge. Thank you.

Miciah · 2022-08-19T19:37:43Z

#818 merged. E2E should pass now.
/hold cancel
/retest-required

Miciah · 2022-08-19T21:39:26Z

e2e-aws-operator failed on must-gather.
/test e2e-aws-operator

jeffnowicki · 2022-08-20T21:16:05Z

/retest-required

openshift-ci · 2022-08-21T00:16:27Z

@SzucsAti: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-single-node	`3ef4123`	link	false	`/test e2e-aws-single-node`
ci/prow/e2e-gcp-serial	`3ef4123`	link	true	`/test e2e-gcp-serial`
ci/prow/e2e-azure-ovn	`1096655`	link	false	`/test e2e-azure-ovn`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

SzucsAti · 2022-08-21T07:55:30Z

/retest-required

mkumatag · 2022-08-21T08:25:02Z

we need these required flags: px-approved, qe-approved, docs-approved as well @jeffnowicki @SzucsAti

jeffnowicki · 2022-08-22T13:08:45Z

@mkumatag I have slack DM'd with @Miciah to get the right people notified so we can get these labels added and PR merged.

ahardin-rh · 2022-08-22T14:27:27Z

/label docs-approved

jeffnowicki · 2022-08-22T18:01:29Z

@sferich888 @ShudiLi would you please review/add px-approved and qe-approved labels accordingly?

ShudiLi · 2022-08-23T02:15:02Z

/label qe-approved
@jeffnowicki thanks

CFields651 · 2022-08-23T13:50:14Z

/label px-approved

SzucsAti force-pushed the ibm-dnssvc-support branch from b475fc0 to 1404834 Compare June 30, 2022 14:16

openshift-ci bot requested review from gcs278 and miheer June 30, 2022 14:17

openshift-ci bot requested a review from Miciah June 30, 2022 16:44

openshift-ci bot changed the title ~~Add IBM DNS Services provider support for private clusters~~ NE-969: Add IBM DNS Services provider support for private clusters Jun 30, 2022

openshift-ci bot assigned gcs278 Jul 6, 2022

gcs278 reviewed Jul 14, 2022

View reviewed changes

SzucsAti force-pushed the ibm-dnssvc-support branch from 1404834 to 057d33a Compare July 14, 2022 13:51

openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2022

SzucsAti force-pushed the ibm-dnssvc-support branch from 057d33a to 2b08a9e Compare July 14, 2022 14:33

openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2022

gcs278 reviewed Jul 14, 2022

View reviewed changes

SzucsAti force-pushed the ibm-dnssvc-support branch from 2b08a9e to d57fa69 Compare July 15, 2022 11:11

Miciah reviewed Jul 28, 2022

View reviewed changes

SzucsAti force-pushed the ibm-dnssvc-support branch 2 times, most recently from cab49db to 392ba3d Compare July 28, 2022 15:18

SzucsAti force-pushed the ibm-dnssvc-support branch from 392ba3d to 3ef4123 Compare August 12, 2022 12:57

Miciah reviewed Aug 18, 2022

View reviewed changes

Add ibm dns-svcs support for private clusters

1096655

SzucsAti force-pushed the ibm-dnssvc-support branch from 3ef4123 to 1096655 Compare August 18, 2022 11:52

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 18, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2022

openshift-ci bot assigned Miciah Aug 19, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 19, 2022

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Aug 22, 2022

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Aug 23, 2022

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Aug 23, 2022

openshift-merge-robot merged commit e72d681 into openshift:master Aug 23, 2022

Karthik-K-N mentioned this pull request Aug 24, 2022

Add IBM Cloud DNS support for Power VS #819

Merged

Miciah mentioned this pull request Aug 28, 2024

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

Merged

	provider, err := ibmprivatedns.NewProvider(ibmprivatedns.Config{
	providerCfg := ibmprivatedns.Config{

		if err != nil {
		if delResponse != nil && delResponse.StatusCode != http.StatusNotFound {

		for _, target := range record.Spec.Targets {
		for _, resourceRecord := range result.ResourceRecords {

-	defaultDNSSVCSRecordTTL              = int64(120)
-	DNSSVCSApiEndpoint                   = "https://api.dns-svcs.cloud.ibm.com/v1"
-)
+)
+const (
+	// defaultDNSSVCSRecordTTL is the default TTL used when a DNS record
+	// does not specify a valid TTL.
+	defaultDNSSVCSRecordTTL = int64(120)
+	// dnsSvcsApiEndpoint is the API endpoint for IBM Cloud DNS Services.
+	dnsSvcsApiEndpoint      = "https://api.dns-svcs.cloud.ibm.com/v1"
+)

	return fmt.Errorf("delete: resource data has record with unknown rData cname type: %v ", err)
	return fmt.Errorf("delete: resource data has record with unknown rData cname type: %T", rData["cname"])

-			if resourceRecordTarget == target && *resourceRecord.Name == dnsName {
+			if *resourceRecord.Name == dnsName {
+				if resourceRecordTarget != target {
+					log.Info("delete: ignoring record with matching name but unexpected target", "record", record, "target", resourceRecordTarget)
+					continue
+				}

	if (delResponse != nil && delResponse.StatusCode != http.StatusNotFound) \|\| delResponse == nil {
	if delResponse == nil \|\| delResponse.StatusCode != http.StatusNotFound {

	//getIbmDNSProvider initializes and returns an IBM DNS provider instance.
	// getIbmDNSProvider initializes and returns an IBM DNS provider instance.

	return nil, fmt.Errorf("failed to create ibm cis dns provider: %v", err)
	return nil, fmt.Errorf("failed to create IBM CIS DNS provider: %w", err)

	return nil, fmt.Errorf("failed to create ibm cis dns provider: %v", err)
	return nil, err

	return nil, fmt.Errorf("failed to initialize IBM CIS DNS provider: %v", err)
	return nil, fmt.Errorf("failed to initialize IBM CIS DNS provider: %w", err)

Conversation

SzucsAti commented Jun 30, 2022

Uh oh!

jeffnowicki commented Jun 30, 2022

Uh oh!

Miciah commented Jun 30, 2022 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jeffnowicki commented Jun 30, 2022

Uh oh!

gcs278 commented Jul 6, 2022

Uh oh!

gcs278 commented Jul 6, 2022

Uh oh!

gcs278 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!

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!

gcs278 Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 14, 2022

Uh oh!

gcs278 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!

Miciah commented Jun 30, 2022 •

edited by openshift-ci bot

Loading

gcs278 Jul 13, 2022 •

edited

Loading