NE-1402: Add service endpoint override capability to IBM DNS provider by marcsello · Pull Request #990 · openshift/cluster-ingress-operator

marcsello · 2023-10-25T09:59:00Z

This PR adds the ability for custom service endpoints to be configured and used by the IBM DNS provider. If no overrides configured, it falls back to the default endpoints it used before.

The overrides are read from the cluster's Infrastrucutre configuration (Specifically from .status.platformStatus.ibmcloud.serviceEndpoints): https://github.com/openshift/cluster-ingress-operator/blob/dev-add-service-overrides/vendor/github.com/openshift/api/config/v1/types_infrastructure.go#L1218

Since PowerVS uses IBM DNS provider, and uses the same configuration format, this capability is implemented for it as well.

Jira reference: https://issues.redhat.com/browse/NE-1402

openshift-ci · 2023-10-25T10:01:49Z

Hi @marcsello. 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.

cjschaef · 2023-10-31T13:41:14Z

/retitle NE-1402: Add service endpoint override capability to IBM DNS provider

openshift-ci-robot · 2023-10-31T13:42:28Z

@marcsello: This pull request references NE-1402 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

Details

In response to this:

This PR adds the ability for custom service endpoints to be configured and used by the IBM DNS provider. If no overrides configured, it falls back to the default endpoints it used before.

The overrides are read from the cluster's Infrastrucutre configuration (Specifically from .status.platformStatus.ibmcloud.serviceEndpoints): https://github.com/openshift/cluster-ingress-operator/blob/dev-add-service-overrides/vendor/github.com/openshift/api/config/v1/types_infrastructure.go#L1218

Since PowerVS uses IBM DNS provider, and uses the same configuration format, this capability is implemented for it as well.

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.

Miciah

Please copy the description from the PR to the commit message, and add a reference to https://issues.redhat.com/browse/NE-1402 in commit message as well.

I have some other comments inline, mostly trivial suggestions, but we should codify the service endpoint names in openshift/api if they aren't already defined somewhere.

It's unfortunate that we don't have E2E for IBM Cloud or Power VS, but we can at least make sure verify, unit tests, etc. pass and that nothing breaks on other platforms.
/ok-to-test

pkg/dns/ibm/common.go

pkg/operator/controller/dns/controller.go

Miciah · 2023-10-31T17:50:18Z

pkg/operator/controller/dns/controller.go

+		// Read custom service endpoints from the infra status and set when configured
+		// When none configured, the provider will use the default values
+		var serviceEndpointOverrides ibm.ServiceEndpointOverrides
+		for _, endpointConfig := range infraStatus.PlatformStatus.PowerVS.ServiceEndpoints {
+			switch endpointConfig.Name {
+			case "cis":
+				serviceEndpointOverrides.CIS = endpointConfig.URL
+			case "dnsservices":
+				serviceEndpointOverrides.DNS = endpointConfig.URL
+			case "iam":
+				serviceEndpointOverrides.IAM = endpointConfig.URL
+			}
+		}


I was going to suggest writing a helper using Go generics, but it turns out that generics isn't flexible enough for this case (see golang/go#48522). Given that, I'll just note that the comment here should be updated per my earlier suggestion.

Miciah · 2023-10-31T18:00:22Z

Oh, and thanks for the PR! It looks good overall, aside from some trivial things as I mentioned.

cjschaef · 2023-11-07T01:03:21Z

Incorporating the changes to the installer, CCM, and MAPI, in a Disconnected environment (no Internet access), I tested these changes successfully. The Ingress operator successfully deployed and created the two router pods, interacting with IBM Cloud Services, via private endpoints (overrides).

# oc --kubeconfig cluster-deploys/us-east-splat-1097-26/auth/kubeconfig get co ingress; curl -m3 https://iam.cloud.ibm.com
NAME      VERSION                              AVAILABLE   PROGRESSING   DEGRADED   SINCE   MESSAGE
ingress   4.15.0-0.nightly-2023-11-06-142702   True        False         False      14m     
curl: (28) Failed to connect to iam.cloud.ibm.com port 443 after 2955 ms: Connection timed out

# oc --kubeconfig cluster-deploys/us-east-splat-1097-26/auth/kubeconfig get po -n openshift-ingress
NAME                              READY   STATUS    RESTARTS   AGE
router-default-6f88d989dc-8sq92   1/1     Running   0          32m
router-default-6f88d989dc-tlmb8   1/1     Running   0          32m

# oc --kubeconfig cluster-deploys/us-east-splat-1097-26/auth/kubeconfig get infrastructure/cluster -oyaml | yq .status.platformStatus.ibmcloud.serviceEndpoints
- name: iam
  url: https://private.us-east.iam.cloud.ibm.com
- name: vpc
  url: https://us-east.private.iaas.cloud.ibm.com/v1
- name: resourcecontroller
  url: https://private.resource-controller.cloud.ibm.com
- name: resourcemanager
  url: https://private.resource-controller.cloud.ibm.com
- name: dnsservices
  url: https://api.private.dns-svcs.cloud.ibm.com/v1
- name: cos
  url: https://s3.direct.us-east.cloud-object-storage.appdomain.cloud

I will run some regression tests for Public, Private, and BYON with these changes and report back this week.

Regression tests completed, everything appears fine.

openshift-ci-robot · 2023-11-07T13:34:22Z

@marcsello: This pull request references NE-1402 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set.

Details

In response to this:

This PR adds the ability for custom service endpoints to be configured and used by the IBM DNS provider. If no overrides configured, it falls back to the default endpoints it used before.

The overrides are read from the cluster's Infrastrucutre configuration (Specifically from .status.platformStatus.ibmcloud.serviceEndpoints): https://github.com/openshift/cluster-ingress-operator/blob/dev-add-service-overrides/vendor/github.com/openshift/api/config/v1/types_infrastructure.go#L1218

Since PowerVS uses IBM DNS provider, and uses the same configuration format, this capability is implemented for it as well.

Jira reference: https://issues.redhat.com/browse/NE-1402

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.

marcsello · 2023-11-08T15:26:20Z

Thank you for all comments! I've fixed most of them. One exception is the named constants, I'll wait for what the outcome of @cjschaef's PR openshift/library-go#1612 with that.

Regarding the failing test, I can not see any relation to my changes, I'll try re-running the test to see if it was an intermittent issue.

/retest

marcsello · 2023-11-08T16:19:13Z

This time different, unrelated tests have failed.

/retest

cjschaef · 2023-11-09T17:21:00Z

Looks like this revert has caused us to lose the necessary openshift/api change we need (for ServiceEndpoints)
#982

So we no longer have access to the IBMCloudServiceEndpoint
openshift/api@2f259e8

marcsello · 2023-11-09T17:59:58Z

I have rebased my commit off master, and added a new commit to update the openshift/api dependency to the first version that includes IBMCloudServiceEndpoint.

marcsello · 2023-11-10T09:39:57Z

There seems to be a temporarily issue with the build.

/retest

marcsello · 2023-11-10T14:58:39Z

It seems like unrelated tests have failed again.

/retest

marcsello · 2023-11-13T09:22:08Z

/retest

marcsello · 2023-11-13T12:32:22Z

/retest

jeffnowicki · 2023-11-22T19:32:38Z

/retest

Miciah

A few additional comments:

Please put the openshift/api bump commit first and the implementation commit second. The goal is that every commit should build and pass tests (this is helpful to anyone using git bisect in the future, for example).
Please mention in the bump commit message that the purpose of the bump is to get the IBM Cloud service override API.
Would you mind adding the go get commands that you used to bump the vendored packages to the commit message for the openshift/api bump? See e501577 for an example. (Sorry for adding this chore; this is a relatively new expectation regarding commit messages that the team has agreed on.)

go.mod

pkg/operator/controller/dns/controller.go

marcsello · 2023-11-27T14:03:28Z

Thanks for the feedback @Miciah! I'll try to sort all this out by today.

…1402) Bump the vendored github.com/openshift/api to v0.0.0-20231113114413-39964e6af314 and it's peer dependencies in order to be able to implement the IBM Cloud service override API for NE-1402 This commit updates k8s.io/client-go as well, (and k8s.io/apiextensions-apiserver and k8s.io/apiserver as well) because the previous version is incompatible with the dependencies of the new api version go mod edit -replace github.com/openshift/api=github.com/openshift/api@v0.0.0-20231113114413-39964e6af314 go mod edit -replace k8s.io/client-go=k8s.io/client-go@v0.28.0 go get k8s.io/apiextensions-apiserver@v0.28.0 go get k8s.io/apiserver@v0.28.0 go mod tidy go mod vendor * go.mod: Bump. * go.sum: * vendor/*: Regenerate.

This PR adds the ability for custom service endpoints to be configured and used by the IBM DNS provider. If no overrides configured, it falls back to the default endpoints it used before. The overrides are read from the cluster's `Infrastrucutre` configuration (Specifically from `.status.platformStatus.ibmcloud.serviceEndpoints`): https://github.com/openshift/cluster-ingress-operator/blob/dev-add-service-overrides/vendor/github.com/openshift/api/config/v1/types_infrastructure.go#L1218 Since PowerVS uses IBM DNS provider, and uses the same configuration format, this capability is implemented for it as well. Jira reference: https://issues.redhat.com/browse/NE-1402

Miciah · 2023-11-27T15:11:45Z

Excellent! Thank you very much!
/approve
/lgtm

Miciah · 2023-11-27T15:23:26Z

The bot must be having a bad case of the Mondays!
/joke
/approve
/lgtm

openshift-ci · 2023-11-27T15:23:54Z

@Miciah: Where does Napoleon keep his armies? In his sleevies.

Details

In response to this:

The bot must be having a bad case of the Mondays!
/joke
/approve
/lgtm

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.

openshift-ci · 2023-11-27T15:29:58Z

[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

openshift-ci-robot · 2023-11-27T16:35:37Z

/retest-required

Remaining retests: 0 against base HEAD 9e77590 and 2 for PR HEAD 21971b4 in total

jeffnowicki · 2023-11-27T19:17:47Z

/retest-required

Miciah · 2023-11-27T20:54:28Z

Other than the vendored packages, all of the changes in this PR are specific to IBM Cloud. Moreover, TestUnmanagedDNSToManagedDNSInternalIngressController is failing on other PRs, so I am confident that the failures are not caused by changes in this PR. Let's override the failing test.
/override ci/prow/e2e-gcp-operator
(In the meantime, there is an ongoing investigation into why TestUnmanagedDNSToManagedDNSInternalIngressController is failing on various PRs.)

openshift-ci · 2023-11-27T20:56:48Z

@Miciah: Overrode contexts on behalf of Miciah: ci/prow/e2e-gcp-operator

Details

In response to this:

Other than the vendored packages, all of the changes in this PR are specific to IBM Cloud. Moreover, TestUnmanagedDNSToManagedDNSInternalIngressController is failing on other PRs, so I am confident that the failures are not caused by changes in this PR. Let's override the failing test.
/override ci/prow/e2e-gcp-operator
(In the meantime, there is an ongoing investigation into why TestUnmanagedDNSToManagedDNSInternalIngressController is failing on various PRs.)

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.

openshift-ci · 2023-11-27T21:00:27Z

@marcsello: 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-ovn-single-node	`21971b4`	link	false	`/test e2e-aws-ovn-single-node`
ci/prow/e2e-azure-ovn	`21971b4`	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.

openshift-bot · 2023-11-28T04:04:12Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-ingress-operator-container-v4.15.0-202311280332.p0.g30833ad.assembly.stream for distgit ose-cluster-ingress-operator.
All builds following this will include this PR.

mjturek · 2024-04-18T14:04:13Z

I just saw this, and it is very useful for us. Thank you!!

openshift-ci bot requested review from candita and rfredette October 25, 2023 10:01

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 Oct 25, 2023

marcsello force-pushed the dev-add-service-overrides branch from cef226d to 9e58915 Compare October 25, 2023 14:47

openshift-ci bot changed the title ~~Add service endpoint override capability to IBM DNS provider~~ NE-1402: Add service endpoint override capability to IBM DNS provider Oct 31, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 31, 2023

Miciah reviewed Oct 31, 2023

View reviewed changes

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 Oct 31, 2023

cjschaef mentioned this pull request Nov 6, 2023

Add IBM Cloud Service constants openshift/library-go#1612

Closed

marcsello force-pushed the dev-add-service-overrides branch from 61b6c82 to 4900812 Compare November 7, 2023 13:43

marcsello force-pushed the dev-add-service-overrides branch from 4900812 to f8e81bb Compare November 9, 2023 17:48

marcsello force-pushed the dev-add-service-overrides branch 2 times, most recently from e3df281 to 10b74db Compare November 13, 2023 17:09

Miciah reviewed Nov 27, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/operator/controller/dns/controller.go Outdated Show resolved Hide resolved

marcsello force-pushed the dev-add-service-overrides branch from 10b74db to 6ceb97d Compare November 27, 2023 14:05

marcsello added 2 commits November 27, 2023 15:25

marcsello force-pushed the dev-add-service-overrides branch from 6ceb97d to 21971b4 Compare November 27, 2023 14:31

openshift-ci bot assigned Miciah Nov 27, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2023

openshift-merge-bot bot merged commit 30833ad into openshift:master Nov 27, 2023

Miciah mentioned this pull request Nov 27, 2023

OCPBUGS-23999: Set owner for default ingress configmap #1000

Closed

Comments

Conversation

marcsello commented Oct 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Oct 25, 2023

Uh oh!

cjschaef commented Oct 31, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Oct 31, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Miciah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Miciah Oct 31, 2023

Choose a reason for hiding this comment

Uh oh!

Miciah commented Oct 31, 2023

Uh oh!

cjschaef commented Nov 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 7, 2023 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcsello commented Nov 8, 2023

Uh oh!

marcsello commented Nov 8, 2023

Uh oh!

cjschaef commented Nov 9, 2023

Uh oh!

marcsello commented Nov 9, 2023

Uh oh!

marcsello commented Nov 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marcsello commented Nov 10, 2023

Uh oh!

marcsello commented Nov 13, 2023

Uh oh!

marcsello commented Nov 13, 2023

Uh oh!

jeffnowicki commented Nov 22, 2023

Uh oh!

Miciah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marcsello commented Nov 27, 2023

Uh oh!

Miciah commented Nov 27, 2023

Uh oh!

Miciah commented Nov 27, 2023

Uh oh!

openshift-ci bot commented Nov 27, 2023

Uh oh!

openshift-ci bot commented Nov 27, 2023

Uh oh!

openshift-ci-robot commented Nov 27, 2023

Uh oh!

jeffnowicki commented Nov 27, 2023

Uh oh!

Miciah commented Nov 27, 2023

Uh oh!

openshift-ci bot commented Nov 27, 2023

Uh oh!

openshift-ci bot commented Nov 27, 2023

Uh oh!

openshift-bot commented Nov 28, 2023

Uh oh!

mjturek commented Apr 18, 2024

Uh oh!

Reviewers

marcsello commented Oct 25, 2023 •

edited

Loading

cjschaef commented Oct 31, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Oct 31, 2023 •

edited by openshift-ci bot

Loading

cjschaef commented Nov 7, 2023 •

edited

Loading

openshift-ci-robot commented Nov 7, 2023 •

edited by openshift-ci bot

Loading

marcsello commented Nov 10, 2023 •

edited

Loading