Add changes to infrastructure object to contain service endpoints and feature flag added #2078

jared-hayes-dev · 2024-10-29T15:12:55Z

Related Jira: https://issues.redhat.com/browse/OCPCLOUD-2694

openshift-ci · 2024-10-29T15:13:10Z

Hello @jared-hayes-dev! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

JoelSpeed · 2024-11-05T10:51:32Z

config/v1/types_infrastructure.go

+	// ServiceEndpoints contains custom endpoints designated to
+	// override existing defaults of IBM Cloud Services.


The godoc on the status version of this field is different, shall we copy it here?

JoelSpeed · 2024-11-05T10:51:45Z

config/v1/types_infrastructure.go

+type IBMCloudPlatformSpec struct {
+	// ServiceEndpoints contains custom endpoints designated to
+	// override existing defaults of IBM Cloud Services.
+	// +listType=atomic


Status version is a map type, why would this one be different?

my bad, unintentionally messed that up when playing with some things - fixed

JoelSpeed · 2024-11-28T16:45:57Z

features/features.go

@@ -670,4 +670,10 @@ var (
 						enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).
 						enhancementPR("https://github.com/openshift/enhancements/pull/1697").
 						mustRegister()
+
+	FeatureGateDyanmicServiceEndpointIBMCloud = newFeatureGate("DyanmicServiceEndpointIBMCloud").


You'll want to include an enableIn and add this to DevPreview and TechPreview initially

Added, but just to confirm our goal would be to remove these post-dev as we wouldn't want to keep this feature within dev/tech preview long term.

Correct, once this is dev complete and testing in techpreview shows it's stable, you add the configv1.Default featureSet to enabledIn to promote the feature to the default feature set

JoelSpeed · 2024-11-28T16:46:30Z

config/v1/types_infrastructure.go

+type IBMCloudPlatformSpec struct {
+	// serviceEndpoints is a list of custom endpoints which will override the default
+	// service endpoints of an IBM Cloud service. These endpoints are consumed by
+	// components within the cluster to reach the respective IBM Cloud Services.


I think we should add some description here of what happens when you add values. They get verified and then copied to status by some controller right? And then consumed by?

openshift-ci · 2025-01-07T01:00:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jared-hayes-dev
Once this PR has been reviewed and has the lgtm label, please assign knobunc for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

…ent pr, feature gate requirements added

jeffnowicki · 2025-01-07T15:02:32Z

/retest-required

JoelSpeed · 2025-01-08T10:02:49Z

config/v1/types_infrastructure.go

 	//
 	// +required
 	// +kubebuilder:validation:Type=string
 	// +kubebuilder:validation:XValidation:rule="isURL(self)",message="url must be a valid absolute URL"
+	// +kubebuilder:validation:Pattern:=`https:\/\/.*(?:\/(api\/)?v\d+\/{0,1})$`


This struct is currently used in both spec and status. Ratcheting validation will work in spec, but there's a bug and it's currently not working for status.

There's two things you can do to move forward.

1.) Wait for the bug to be resolved, I'm working on that actively but don't have a timeline, hopefully not long
2.) Create a separate version of this struct for use in the spec with the improved validation

Ok thanks for the heads up. Is there a Jira for this that we could track this bug? And if I am to understand correctly the temporary solution is to duplicate the struct for use within the spec?

The PR to fix the status issue is openshift/kubernetes#2167, I'm hoping to have that merged this week, so potentially hold off on making further changes.

The upstream version of the PR has been merged which was the ask from the o/k team prior to merging this version

JoelSpeed · 2025-01-08T10:07:46Z

config/v1/types_infrastructure.go

 	//
 	// +required
 	// +kubebuilder:validation:Type=string
 	// +kubebuilder:validation:XValidation:rule="isURL(self)",message="url must be a valid absolute URL"
+	// +kubebuilder:validation:Pattern:=`https:\/\/.*(?:\/(api\/)?v\d+\/{0,1})$`


I'd suggest using CEL for these validations instead of a pattern

// +kubebuilder:validation:XValidation:rule="isURL(self)",message="url must be a valid URL, including a scheme" // +kubebuilder:validation:XValidation:rule="url(self).getScheme() == \"https\"",message="url scheme must be https" // +kubebuilder:validation:XValidation:rule="url(self).getEscapedPath().matches(\"\/(api\/)?v\d+\/{0,1}\")",message="url path must match /v[0,9]+ or /api/v[0,9]+"

You will also need a maximum length on the string to add these validations, choose something sensible, but long enough, perhaps 256 chars? Perhaps a little more to add some headroom for the path? 300?

Sounds good, 300 chars would work perfectly.

JoelSpeed · 2025-01-08T10:09:28Z

config/v1/types_infrastructure.go

+	// +listType=map
+	// +listMapKey=name
+	// +optional
+	ServiceEndpoints []IBMCloudServiceEndpoint `json:"serviceEndpoints,omitempty"`


All lists must have a maximum number of items marker, what is the rough number of items you'd expect to be in this list in the worst case scenario?

We chose to limit the list to 25 - this covers all current uses cases and gives good room for any future additions

You've added the limit to status only, add to spec as well please

…t of url overrides

JoelSpeed · 2025-01-14T09:59:22Z

config/v1/types_infrastructure.go

 	//
 	// +required
 	// +kubebuilder:validation:Type=string
 	// +kubebuilder:validation:XValidation:rule="isURL(self)",message="url must be a valid absolute URL"
+	// +kubebuilder:validation:XValidation:rule=`self.matches('https:\/\/.*(?:\/(api\/)?v\d+\/{0,1})$')`,message="Invalid URL pattern for IBM service overrides"


Any reason to use matches, over the URL helpers that I had suggested? In theory this could allow the path to be incorrect
E.g. https://a.b.c/my/malicious/path?pretend_that_i_am=/api/v1 passes your regex right now, but is not your intention.

The URL helpers I mentioned previously will accurately return you the escaped path, which you can then match against, and would avoid this kind of issue

My bad, somehow I misunderstood your comment when I first read it, and missed the portion in which you suggested the exact URL functions. I have updated the field to correctly use them over trying to pattern match the entire field.

JoelSpeed · 2025-01-14T10:00:16Z

config/v1/types_infrastructure.go

+	// serviceEndpoints is a list of custom endpoints which will override the default
+	// service endpoints of an IBM Cloud service. These endpoints are consumed by
+	// components within the cluster to reach the respective IBM Cloud Services.
+	// Once admitted, the CCCMO will furthger validate the endpoint exists by pinging it


Suggested change

// Once admitted, the CCCMO will furthger validate the endpoint exists by pinging it

// Once admitted, the CCCMO will further validate the endpoint exists by pinging it

Addressed below in rewrite

JoelSpeed · 2025-01-14T10:00:43Z

config/v1/types_infrastructure.go

+	// service endpoints of an IBM Cloud service. These endpoints are consumed by
+	// components within the cluster to reach the respective IBM Cloud Services.
+	// Once admitted, the CCCMO will furthger validate the endpoint exists by pinging it
+	// before processing using the provided endpoints to updates the platform status


This line isn't reading well to me, want to just double check the wording here?

Rewritten to hopefully address the confusing portions

JoelSpeed · 2025-01-14T10:01:22Z

config/v1/types_infrastructure.go

+	// +listType=map
+	// +listMapKey=name
+	// +optional
+	ServiceEndpoints []IBMCloudServiceEndpoint `json:"serviceEndpoints,omitempty"`


You've added the limit to status only, add to spec as well please

openshift-ci · 2025-01-15T11:20:19Z

@jared-hayes-dev: 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-scos-e2e-aws-ovn	`0a95665`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/verify	`0a95665`	link	true	`/test verify`
ci/prow/e2e-azure	`0a95665`	link	false	`/test e2e-azure`
ci/prow/e2e-aws-serial	`0a95665`	link	true	`/test e2e-aws-serial`
ci/prow/e2e-aws-serial-techpreview	`0a95665`	link	true	`/test e2e-aws-serial-techpreview`

Full PR test history. Your PR dashboard.

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

JoelSpeed · 2025-01-15T11:28:03Z

config/v1/types_infrastructure.go

+	// +kubebuilder:validation:XValidation:rule="url(self).getScheme() == \"https\"",message="url scheme must be https"
+	// +kubebuilder:validation:XValidation:rule="url(self).getEscapedPath().matches(\"\/(api\/)?v\d+\/{0,1}\")",message="url path must match /v[0,9]+ or /api/v[0,9]+"
+	// +kubebuilder:validation:MaxLength=300


We need to make sure that each of these validations is tested, and in particular, also tested with ratcheting, which is new to the test suite.

You can see some examples of adding ratcheting tests in #2142

You'll have to come up with a patch operation (likely a remove on the path to the maxLength, CEL validations etc) that allows you to persist something not allowed by the new validation, and then show through tests that this does not block writes.

Since this field is new in spec, you'll need to write those ratcheting tests only for the status. That's currently not going to ratchet because of the bug linked to those other tests, I'll let you know when they will pass

JoelSpeed · 2025-01-15T11:28:24Z

config/v1/types_infrastructure.go

+	// within the cluster when trying to reach the IBM Cloud Services that have been
+	// overriden. The CCCMO reads in the IBMCloudPlatformSpec and validates each
+	// endpoint is resolvable. Once validated, the cloud config and IBMCloudPlatformStatus
+	// are updated to reflect the same custom endpoints.


Suggested change

// are updated to reflect the same custom endpoints.

// are updated to reflect the same custom endpoints.

// A maximum of 25 service endpoints are supported.

openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 29, 2024

openshift-ci bot requested review from deads2k and knobunc October 29, 2024 15:15

openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 5, 2024

JoelSpeed reviewed Nov 5, 2024

View reviewed changes

JoelSpeed reviewed Nov 28, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 4, 2024

Jared Hayes added 5 commits January 6, 2025 17:04

API changes for supporting dynamic endpoint override for IBM cloud"

9d7a91e

run make command for feature

d0e9605

cleanup

d171ffb

sync w/ correct type and comment

a681e2b

fix description of items within platform spec, comments from enhancem…

3ee0be4

…ent pr, feature gate requirements added

jared-hayes-dev force-pushed the jh-340 branch from e5dceca to 3ee0be4 Compare January 7, 2025 01:09

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2025

Jared Hayes added 2 commits January 6, 2025 17:10

.

0b4fee3

further api definition fixes

4525152

JoelSpeed reviewed Jan 8, 2025

View reviewed changes

Jared Hayes added 2 commits January 13, 2025 11:53

fix to use correct validation mechanic for pattern, max length of lis…

698a39c

…t of url overrides

Merge branch 'master' into jh-340

7449b3b

JoelSpeed reviewed Jan 14, 2025

View reviewed changes

Jared Hayes added 2 commits January 14, 2025 23:29

address some complexity in wording and matching for path pattern

e5e3220

openapi update

0a95665

JoelSpeed reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add changes to infrastructure object to contain service endpoints and feature flag added #2078

Add changes to infrastructure object to contain service endpoints and feature flag added #2078

jared-hayes-dev commented Oct 29, 2024

openshift-ci bot commented Oct 29, 2024

JoelSpeed Nov 5, 2024

jared-hayes-dev Nov 18, 2024

JoelSpeed Nov 5, 2024

jared-hayes-dev Nov 18, 2024

JoelSpeed Nov 28, 2024

jared-hayes-dev Jan 7, 2025

JoelSpeed Jan 8, 2025

JoelSpeed Nov 28, 2024

openshift-ci bot commented Jan 7, 2025

jeffnowicki commented Jan 7, 2025

JoelSpeed Jan 8, 2025

jared-hayes-dev Jan 14, 2025

JoelSpeed Jan 14, 2025

JoelSpeed Jan 8, 2025

jared-hayes-dev Jan 14, 2025

JoelSpeed Jan 8, 2025

jared-hayes-dev Jan 14, 2025

JoelSpeed Jan 14, 2025

JoelSpeed Jan 14, 2025

jared-hayes-dev Jan 15, 2025

JoelSpeed Jan 14, 2025

jared-hayes-dev Jan 15, 2025

JoelSpeed Jan 14, 2025

jared-hayes-dev Jan 15, 2025 •

edited

Loading

JoelSpeed Jan 14, 2025

openshift-ci bot commented Jan 15, 2025

JoelSpeed Jan 15, 2025

JoelSpeed Jan 15, 2025

		// ServiceEndpoints contains custom endpoints designated to
		// override existing defaults of IBM Cloud Services.

	// Once admitted, the CCCMO will furthger validate the endpoint exists by pinging it
	// Once admitted, the CCCMO will further validate the endpoint exists by pinging it

	// are updated to reflect the same custom endpoints.
	// are updated to reflect the same custom endpoints.
	// A maximum of 25 service endpoints are supported.

Add changes to infrastructure object to contain service endpoints and feature flag added #2078

Are you sure you want to change the base?

Add changes to infrastructure object to contain service endpoints and feature flag added #2078

Conversation

jared-hayes-dev commented Oct 29, 2024

openshift-ci bot commented Oct 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 7, 2025

jeffnowicki commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jared-hayes-dev Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jared-hayes-dev Jan 15, 2025 •

edited

Loading