CORS-2933: IBMCloud: Basic service endpoint override by cjschaef · Pull Request #7632 · openshift/installer

cjschaef · 2023-10-25T20:25:13Z

Adding basic support to allow overrides of IBM Cloud service endpoints. This is a hard requirement for disconnected cluster support, to allow use of non-default endpoints for IBM Cloud services.

rvanderp3

overall LGTM

rvanderp3 · 2023-10-27T19:26:41Z

pkg/asset/installconfig/ibmcloud/validation.go

+		}
+	}
+
+	// TODO(cjschaef): See about performing additional endpoint validation (all vs. subsets, regional endpoint validation, etc.)


what is the status of this TODO?

I was expecting to follow up on this later, as I wait for details from other development teams on possible configurations, environments, etc. that would determine if we wish to perform additional validation, or leave the endpoint override possibilities broader, as the are currently.

I can get a Jira open to track that work, and link it here, if that makes sense.

I removed the TODO, as we will be leaving the configuration options more broad, due to the variety of environments/restrictions we can cover.

pkg/asset/machines/ibmcloud/zones.go

pkg/asset/manifests/ibmcloud/cloudproviderconfig.go

rvanderp3 · 2023-10-27T19:32:04Z

pkg/asset/manifests/ibmcloud/cloudproviderconfig.go

+			config.Provider.VPCEndpointOverride = suffixRegex.ReplaceAllString(endpoint.URL, "${1}")
+		case ibmcloud.IBMCloudServiceResourceManager:
+			config.Provider.RMEndpointOverride = endpoint.URL
+		}


would there be any value in logging when this switch falls through?

Maybe. Other endpoints the CCM doesn't use (COS, DNS Services, etc.), would fall through, so we'd ignore them in terms of building the CCM config.
But perhaps logging the "ignored" overrides could be helpful if the user has incorrectly configured those required for the CCM, but I'd expect the validation steps should filter those issues out
https://github.com/openshift/installer/pull/7632/files#diff-e6306df593d77282680d1864f5e1504e044abdbe681195fbcbeaab0cc3c45cc2R437

I'll add a default log call I think though.

Added a log call

pkg/destroy/ibmcloud/ibmcloud.go

pkg/tfvars/ibmcloud/ibmcloud.go

rvanderp3 · 2023-10-27T19:35:06Z

pkg/tfvars/ibmcloud/ibmcloud.go

+
+	// Following the format provided by the IBM Cloud Terraform Provider documentation:
+	// https://registry.terraform.io/providers/IBM-Cloud/ibm/latest/docs/guides/custom-service-endpoints#file-structure-for-endpoints-file
+	endpointContents := make(map[string]map[string]map[string]string)


this seems like a fairly complex way of representing this data structure. would it be possible to create a type representing it?

Sure, I'll see about doing that instead, to get the JSON format correct.

I made the design a bit better I hope.

pkg/types/ibmcloud/validation/platform.go

rvanderp3 · 2023-10-27T19:38:16Z

pkg/types/ibmcloud/validation/platform.go

+	httpsScheme := "https"
+
+	// Regexp for a trailing API version in URL path ('/v1', '/v22/', etc.)
+	versionPath := regexp.MustCompile(`(/v\d+[/]{0,1})$`)


would it make sense to compile this regex once as in line 93?

Oh yeah, I can move that.

pkg/types/ibmcloud/validation/platform_test.go

cjschaef · 2023-11-06T19:56:08Z

Init container clonerefs not ready: (state: running) Init container initupload not ready: (state: waiting, reason: "PodInitializing", message: "") Init container place-entrypoint not ready: (state: waiting, reason: "PodInitializing", message: "")

/retest

cjschaef · 2023-11-07T16:57:28Z

/retitle CORS-2934: IBMCloud: Basic service endpoint override

openshift-ci-robot · 2023-11-07T16:58:21Z

@cjschaef: This pull request references CORS-2934 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:

Adding basic support to allow overrides of IBM Cloud service endpoints. This is a hard requirement for disconnected cluster support, to allow use of non-default endpoints for IBM Cloud services.

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.

rvanderp3 · 2023-11-07T21:52:12Z

/retest-required

cjschaef · 2023-11-07T23:31:48Z

/retest

cjschaef · 2023-11-08T17:13:03Z

/retest

cjschaef · 2023-11-10T15:20:14Z

/retest-required

cjschaef · 2023-11-13T18:12:28Z

I am updating to pull in the changes from api, for service names.
openshift/api@2155ee0

Done and fixed bootstrap destroy issue, requiring endpoint override solution.

cjschaef · 2023-11-15T14:52:04Z

/retest

cjschaef · 2023-11-15T18:26:41Z

/retest

cjschaef · 2023-11-16T15:43:01Z

/retest

r4f4 · 2023-11-22T10:37:59Z

/hold cancel
openstack-manifests fix has merged.

cjschaef · 2023-11-22T22:44:44Z

I've rebased and pulled in a new openshift/api release for the IBMCloudServiceName enum needed.

r4f4

We usually prefer that vendoring changes are in their own commit. It also helps with code reviews, since we can skip them when looking at the commits until all the other parts have been reviewed.

I really wish this whole feature was not in a single commit but broken down into multiple logical units. For examples, 1 commit for terraform configs changes, 1 commit for adding a new field to install-config, 1 commit for replacing existing errors.Wrap by fmt.Errorf, etc. It would make reviewing much easier.

r4f4 · 2023-11-22T23:24:59Z

pkg/asset/installconfig/ibmcloud/validation.go

It would be nice to add a comment explaining why it's ok to silence the linter

Suggested change

_, err = http.Head(endpoint) //nolint:gosec

_, err = http.Head(endpoint) //nolint:gosec // reason goes here

Sure, I'll add.

r4f4 · 2023-11-22T23:28:53Z

data/data/ibmcloud/variables-ibmcloud.tf

Is this change related to this PR?

No, I was cleaning up the reference as I found null isn't desired for string.
I will drop and fix later.

cjschaef · 2023-11-25T17:25:17Z

I can attempt to break the PR up into multiple PR's if that helps then.

I would open separate PR's then from this PR, as not to change this one until I know those are valid/expected.

r4f4 · 2023-11-25T17:53:58Z

I can attempt to break the PR up into multiple PR's if that helps then.

I would open separate PR's then from this PR, as not to change this one until I know those are valid/expected.

That would be nice but if you're aiming to get this merged before FF, just breaking this PR down into commits is enough. Unless you think some part of this work can be merged independently, in which case a separate PR might help to push those along.

cjschaef · 2023-11-25T21:29:44Z

Okay, I went with that, and updated this PR with those changes (3 commits, without the other cleanup changes).

I've opened a separate draft PR for those cleanup related changes.
#7766

patrickdillon · 2023-11-26T22:03:57Z

/approve

openshift-ci · 2023-11-26T22:04:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: patrickdillon

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~~ [patrickdillon]

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

patrickdillon · 2023-11-26T22:24:09Z

This LGTM
looks like there is a small linter error

cjschaef · 2023-11-27T02:16:31Z

I'll see about rebasing and rebuilding the go mod conflicts.

Updated

I dropped the go mod changes, as the api release was updated to a newer release than I had, to get the IBMCloudServiceName enums.

Adding support to terraform to allow overrides of IBM Cloud service endpoints. This is a hard requirement for disconnected cluster support, to allow use of non-default endpoints for IBM Cloud services. Related: https://issues.redhat.com/browse/CORS-2933

Adding support to allow overrides of IBM Cloud service endpoints in installconfig. This is a hard requirement for disconnected cluster support, to allow use of non-default endpoints for IBM Cloud services. Related: https://issues.redhat.com/browse/CORS-2933

openshift-ci · 2023-11-27T06:55:01Z

@cjschaef: 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/okd-e2e-aws-ovn	`03a6738`	link	false	`/test okd-e2e-aws-ovn`
ci/prow/okd-scos-e2e-aws-ovn	`03a6738`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-openstack-nfv-intel	58deb39162704bd6fe868e5d0596d05f0ce41960	link	false	`/test e2e-openstack-nfv-intel`
ci/prow/e2e-openstack-proxy	58deb39162704bd6fe868e5d0596d05f0ce41960	link	false	`/test e2e-openstack-proxy`
ci/prow/e2e-gcp-secureboot	58deb39162704bd6fe868e5d0596d05f0ce41960	link	false	`/test e2e-gcp-secureboot`
ci/prow/e2e-gcp-ovn-xpn	58deb39162704bd6fe868e5d0596d05f0ce41960	link	false	`/test e2e-gcp-ovn-xpn`
ci/prow/e2e-azure-ovn-shared-vpc	58deb39162704bd6fe868e5d0596d05f0ce41960	link	false	`/test e2e-azure-ovn-shared-vpc`
ci/prow/e2e-gcp-ovn-shared-vpc	58deb39162704bd6fe868e5d0596d05f0ce41960	link	false	`/test e2e-gcp-ovn-shared-vpc`
ci/prow/e2e-azure-ovn	58deb39162704bd6fe868e5d0596d05f0ce41960	link	true	`/test e2e-azure-ovn`
ci/prow/e2e-ibmcloud-ovn	`8909227`	link	false	`/test e2e-ibmcloud-ovn`
ci/prow/e2e-gcp-ovn	58deb39162704bd6fe868e5d0596d05f0ce41960	link	true	`/test e2e-gcp-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.

r4f4

/lgtm

openshift-bot · 2023-11-27T18:08:41Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-installer-altinfra-container-v4.15.0-202311271609.p0.g29ce6ef.assembly.stream for distgit ose-installer-altinfra.
All builds following this will include this PR.

openshift-ci bot requested review from pawanpinjarkar and rvanderp3 October 25, 2023 20:27

cjschaef force-pushed the splat-1097 branch 4 times, most recently from e26a23b to 03a6738 Compare October 26, 2023 20:20

rvanderp3 reviewed Oct 27, 2023

View reviewed changes

cjschaef force-pushed the splat-1097 branch from 03a6738 to d99cecc Compare November 6, 2023 19:22

cjschaef force-pushed the splat-1097 branch 2 times, most recently from a85470d to 334255e Compare November 6, 2023 22:51

openshift-ci bot changed the title ~~IBMCloud: Basic service endpoint override~~ CORS-2934: IBMCloud: Basic service endpoint override Nov 7, 2023

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

cjschaef force-pushed the splat-1097 branch 3 times, most recently from 4e2f04f to 24036a9 Compare November 7, 2023 20:32

cjschaef mentioned this pull request Nov 10, 2023

IR-412: IBMCloud: Add support for endpoint overrides openshift/cluster-image-registry-operator#955

Merged

cjschaef force-pushed the splat-1097 branch 3 times, most recently from d4c8a5b to 1e746f9 Compare November 14, 2023 22:00

cjschaef force-pushed the splat-1097 branch from a8186fe to aa2f2e7 Compare November 21, 2023 18:39

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

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2023

cjschaef force-pushed the splat-1097 branch from aa2f2e7 to e86a8c0 Compare November 22, 2023 22:44

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2023

r4f4 reviewed Nov 22, 2023

View reviewed changes

cjschaef force-pushed the splat-1097 branch from e86a8c0 to 7547399 Compare November 25, 2023 21:27

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

cjschaef force-pushed the splat-1097 branch from 7547399 to 58deb39 Compare November 27, 2023 02:15

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2023

cjschaef added 2 commits November 26, 2023 20:17

cjschaef force-pushed the splat-1097 branch from 58deb39 to 8909227 Compare November 27, 2023 02:23

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 27, 2023

r4f4 reviewed Nov 27, 2023

View reviewed changes

openshift-ci bot assigned r4f4 Nov 27, 2023

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

openshift-merge-bot bot merged commit 29ce6ef into openshift:master Nov 27, 2023

cjschaef deleted the splat-1097 branch November 28, 2023 19:46

	_, err = http.Head(endpoint) //nolint:gosec
	_, err = http.Head(endpoint) //nolint:gosec // reason goes here

Comments

Conversation

cjschaef commented Oct 25, 2023

Uh oh!

rvanderp3 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjschaef Oct 30, 2023 • 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!

Uh oh!

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!

Uh oh!

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!

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!

Uh oh!

cjschaef commented Nov 6, 2023

Uh oh!

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

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!

rvanderp3 commented Nov 7, 2023

Uh oh!

cjschaef commented Nov 7, 2023

Uh oh!

cjschaef commented Nov 8, 2023

Uh oh!

cjschaef commented Nov 10, 2023

Uh oh!

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

Uh oh!

Uh oh!

cjschaef commented Nov 15, 2023

Uh oh!

cjschaef commented Nov 15, 2023

Uh oh!

cjschaef commented Nov 16, 2023

Uh oh!

r4f4 commented Nov 22, 2023

Uh oh!

cjschaef commented Nov 22, 2023

Uh oh!

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

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

Uh oh!

Choose a reason for hiding this comment

cjschaef Oct 30, 2023 •

edited

Loading

cjschaef commented Nov 7, 2023 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Nov 7, 2023 •

edited by openshift-ci bot

Loading

cjschaef commented Nov 13, 2023 •

edited

Loading

cjschaef Nov 25, 2023 •

edited

Loading

cjschaef commented Nov 27, 2023 •

edited

Loading