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.

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
28 changes: 24 additions & 4 deletions config/v1/types_infrastructure.go
Original file line number Diff line number Diff line change
Expand Up @@ -1615,17 +1615,33 @@ 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="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
Comment on lines +1624 to +1626
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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 service. These endpoints are used by components
// 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.
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
// are updated to reflect the same custom endpoints.
// are updated to reflect the same custom endpoints.
// A maximum of 25 service endpoints are supported.

// +listType=map
// +listMapKey=name
// +kubebuilder:validation:MaxItems:=25
// +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 @@ -1647,10 +1663,14 @@ type IBMCloudPlatformStatus struct {
DNSInstanceCRN string `json:"dnsInstanceCRN,omitempty"`

// 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.
// service endpoints of an IBM service. These endpoints are used by components
// 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.
// +listType=map
// +listMapKey=name
// +kubebuilder:validation:MaxItems:=25
// +optional
ServiceEndpoints []IBMCloudServiceEndpoint `json:"serviceEndpoints,omitempty"`
}
Expand Down
1 change: 1 addition & 0 deletions features.md
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
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

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
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 service. These endpoints are used by components 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.",
"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 service. These endpoints are used by components 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.",
"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
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@
{
"name": "DNSNameResolver"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@
{
"name": "DisableKubeletCloudCredentialProviders"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,9 @@
{
"name": "DisableKubeletCloudCredentialProviders"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@
{
"name": "DNSNameResolver"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@
{
"name": "DisableKubeletCloudCredentialProviders"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,9 @@
{
"name": "DisableKubeletCloudCredentialProviders"
},
{
"name": "DyanmicServiceEndpointIBMCloud"
},
{
"name": "DynamicResourceAllocation"
},
Expand Down