Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9d7a91e
API changes for supporting dynamic endpoint override for IBM cloud"
Oct 10, 2024
d0e9605
run make command for feature
Oct 10, 2024
d171ffb
cleanup
Nov 5, 2024
a681e2b
sync w/ correct type and comment
Nov 18, 2024
3ee0be4
fix description of items within platform spec, comments from enhancem…
Jan 7, 2025
0b4fee3
.
Jan 7, 2025
4525152
further api definition fixes
Jan 7, 2025
698a39c
fix to use correct validation mechanic for pattern, max length of lis…
Jan 13, 2025
7449b3b
Merge branch 'master' into jh-340
Jan 14, 2025
e5e3220
address some complexity in wording and matching for path pattern
Jan 15, 2025
0a95665
openapi update
Jan 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions config/v1/types_infrastructure.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1615,17 +1615,31 @@ type IBMCloudServiceEndpoint struct {

// url is fully qualified URI with scheme https, that overrides the default generated
// endpoint for a client.
// This must be provided and cannot be empty.
// This must be provided and cannot be empty. The path must follow the pattern
// /v[0,9]+ or /api/v[0,9]+
//
// +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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// +kubebuilder:validation:MaxLength=300
URL string `json:"url"`
}

// IBMCloudPlatformSpec holds the desired state of the IBMCloud infrastructure provider.
// This only includes fields that can be modified in the cluster.
type IBMCloudPlatformSpec struct{}
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed below in rewrite

// before processing using the provided endpoints to updates the platform status
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewritten to hopefully address the confusing portions

// as well as the cloud config.
// +listType=map
// +listMapKey=name
// +optional
ServiceEndpoints []IBMCloudServiceEndpoint `json:"serviceEndpoints,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// IBMCloudPlatformStatus holds the current status of the IBMCloud infrastructure provider.
type IBMCloudPlatformStatus struct {
Expand All @@ -1649,8 +1663,13 @@ type IBMCloudPlatformStatus 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.
// Once admitted, the CCCMO will furthger validate the endpoint exists by pinging it
// before processing using the provided endpoints to updates the platform status
// as well as the cloud config.
// platform status as well as the cloud config.
// +listType=map
// +listMapKey=name
// +kubebuilder:validation:MaxItems:=25
// +optional
ServiceEndpoints []IBMCloudServiceEndpoint `json:"serviceEndpoints,omitempty"`
}
Expand Down
1 change: 1 addition & 0 deletions features.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
| ClusterMonitoringConfig| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| ConsolePluginContentSecurityPolicy| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| DNSNameResolver| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| DyanmicServiceEndpointIBMCloud| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| DynamicResourceAllocation| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| EtcdBackendQuota| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
| Example| | | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> | <span style="background-color: #519450">Enabled</span> |
Expand Down
8 changes: 8 additions & 0 deletions features/features.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,4 +726,12 @@ var (
enhancementPR("https://github.com/openshift/enhancements/pull/1492").
enableIn(configv1.DevPreviewNoUpgrade).
mustRegister()

FeatureGateDyanmicServiceEndpointIBMCloud = newFeatureGate("DyanmicServiceEndpointIBMCloud").
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

reportProblemsToJiraComponent("Cloud Compute / IBM Provider").
contactPerson("jared-hayes-dev").
productScope(ocpSpecific).
enhancementPR("https://github.com/openshift/enhancements/pull/1712").
enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade).
mustRegister()
)
30 changes: 28 additions & 2 deletions openapi/generated_openapi/zz_generated.openapi.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 17 additions & 3 deletions openapi/openapi.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6862,7 +6862,21 @@
},
"com.github.openshift.api.config.v1.IBMCloudPlatformSpec": {
"description": "IBMCloudPlatformSpec holds the desired state of the IBMCloud infrastructure provider. This only includes fields that can be modified in the cluster.",
"type": "object"
"type": "object",
"properties": {
"serviceEndpoints": {
"description": "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 before processing using the provided endpoints to updates the platform status as well as the cloud config.",
"type": "array",
"items": {
"default": {},
"$ref": "#/definitions/com.github.openshift.api.config.v1.IBMCloudServiceEndpoint"
},
"x-kubernetes-list-map-keys": [
"name"
],
"x-kubernetes-list-type": "map"
}
}
},
"com.github.openshift.api.config.v1.IBMCloudPlatformStatus": {
"description": "IBMCloudPlatformStatus holds the current status of the IBMCloud infrastructure provider.",
Expand All @@ -6889,7 +6903,7 @@
"type": "string"
},
"serviceEndpoints": {
"description": "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.",
"description": "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 before processing using the provided endpoints to updates the platform status as well as the cloud config. platform status as well as the cloud config.",
"type": "array",
"items": {
"default": {},
Expand All @@ -6916,7 +6930,7 @@
"default": ""
},
"url": {
"description": "url is fully qualified URI with scheme https, that overrides the default generated endpoint for a client. This must be provided and cannot be empty.",
"description": "url is fully qualified URI with scheme https, that overrides the default generated endpoint for a client. This must be provided and cannot be empty. The path must follow the pattern /v[0,9]+ or /api/v[0,9]+",
"type": "string",
"default": ""
}
Expand Down
3 changes: 3 additions & 0 deletions payload-manifests/featuregates/featureGate-Hypershift-Default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@
{
"name": "DNSNameResolver"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down
3 changes: 3 additions & 0 deletions payload-manifests/featuregates/featureGate-Hypershift-DevPreviewNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@
{
"name": "DisableKubeletCloudCredentialProviders"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down
3 changes: 3 additions & 0 deletions payload-manifests/featuregates/featureGate-Hypershift-TechPreviewNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@
{
"name": "DisableKubeletCloudCredentialProviders"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down
3 changes: 3 additions & 0 deletions payload-manifests/featuregates/featureGate-SelfManagedHA-Default.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@
{
"name": "DNSNameResolver"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down
3 changes: 3 additions & 0 deletions payload-manifests/featuregates/featureGate-SelfManagedHA-DevPreviewNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@
{
"name": "DisableKubeletCloudCredentialProviders"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down
3 changes: 3 additions & 0 deletions payload-manifests/featuregates/featureGate-SelfManagedHA-TechPreviewNoUpgrade.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@
{
"name": "DisableKubeletCloudCredentialProviders"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down