CORS-3842: Add API Updates for GCP Custom API Endpoints #2150
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@barbacbd: This pull request references CORS-3842 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.19.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Hello @barbacbd! Some important instructions when contributing to openshift/api: |
|
/cc @patrickdillon |
openshift-ci
bot
added
the
size/XXL
|
/cc @JoelSpeed |
|
/retest |
|
/retest-required |
|
/retest |
There was a problem hiding this comment.
Spotted a few more things. It would also be beneficial to add tests where possible to ensure any validations you are putting in place are acting as expected. https://github.com/openshift/api?tab=readme-ov-file#defining-api-validation-tests should be helpful in getting you started on the right track for adding any necessary tests.
There was a problem hiding this comment.
A few more things. Other than these I think this is shaping up well. After this round of comments is addressed I'll rope @JoelSpeed in to do a pass since he is the one with approval authority.
...tion/v1/tests/controllerconfigs.machineconfiguration.openshift.io/GCPCustomAPIEndpoints.yaml
Outdated
Show resolved
Hide resolved
barbacbd
force-pushed
the
CORS-3842
branch
2 times, most recently
from
04681c3 to
ea88b6c
Compare
There was a problem hiding this comment.
Changes look good to me.
Tagging in @JoelSpeed for a review.
|
/retest |
|
@JoelSpeed - it looks like the verify-crd-schema check is flagging fields untouched by this PR. Do these need to be addressed while touching this API? |
|
/label acknowledge-critical-fixes-only |
openshift-ci
bot
added
the
acknowledge-critical-fixes-only
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: barbacbd, everettraven The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
After chatting with Joel the verify-crd-schema check is failing because this is technically introducing a net new feature-gated CRD. Looks to be safe to ignore. /override verify-crd-schema |
|
@everettraven: everettraven unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers. In response to this:
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. |
There was a problem hiding this comment.
We have both spec and status versions here. What will validate the spec, and then update the status? Are we ok with allowing these values to be changed on day 2, or do we want these to be immutable once the cluster is installed?
Other platforms have them mutable, but I want to make sure this makes sense on GCP specifically
config/v1/types_infrastructure.go
Outdated
| // serviceEndpoints is optional. | ||
| // Only 1 endpoint override is permitted for each GCP service. | ||
| // The maximum number of endpoint overrides allowed is 9. | ||
| // +listType=atomic |
There was a problem hiding this comment.
Why atomic, and not map with the map key as the Name?
config/v1/types_infrastructure.go
Outdated
| // The maximum number of endpoint overrides allowed is 9. | ||
| // +listType=atomic | ||
| // +kubebuilder:validation:MaxItems=9 | ||
| // +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x.name == y.name))",message="only 1 endpoint override is permitted for a GCP service" |
There was a problem hiding this comment.
Maybe be explicit that it's the name that's unique?
| // +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x.name == y.name))",message="only 1 endpoint override is permitted for a GCP service" | |
| // +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x.name == y.name))",message="only 1 endpoint override is permitted per GCP service name" |
|
|
||
| // serviceEndpoints specifies endpoints that override the default endpoints | ||
| // used when creating clients to interact with GCP services. | ||
| // serviceEndpoints is optional. |
There was a problem hiding this comment.
When not specified, the behaviour is to use the default endpoints for the region that the cluster is deployed to? Is that correct?
There was a problem hiding this comment.
yes. The setup would end up looking something like https://github.com/barbacbd/k8s-cluster-api-provider-gcp/blob/main/cloud/scope/clients.go#L153. That way the option is just not set when it is not present.
There was a problem hiding this comment.
So would it be fair to say
| // serviceEndpoints is optional. | |
| // When not specified, the default endpoints for the GCP region will be used. |
@JoelSpeed I would probably make them immutable. I don't think we should change what values are there. I wasn't sure if we needed the status as that would indicate that something changed or could change. Can you think of a case where someone would want a different "custom" or default endpoint in one place and not in another? I would think we want to use the same endpoint anytime that the cluster components are supposed to access that service. |
|
New changes are detected. LGTM label has been removed. |
Typically for configuration like this that is provided at install time, we put the data into the status, and then the various components (CSI, CCM etc) consume the values from the status. Later, if a user wants to change the endpoints (which we have seen use cases for on IBM, but you may not need here), then we have added spec fields, and then added a control loop that watches updates to the spec field, validates the new endpoints, and then copies the validated endpoints to the status only once it's certain they are valid. The consumers then pick up the new value from status. If the request for this feature doesn't warrant the ability to change these endpoints on day 2, and you don't have a plan to create a control loop to validate the spec and copy it to status, then I'd suggest for now we move forward with adding only a status version of this API which the installer would populate from the install config |
** Add the Tech preview and No upgrade tags for the new feature GCP API Custom Endpoints. ** Add the ServiceEndpoint Structure that includes the api name and endpoint. ** Add the Service Endpoints to the GCP Spec and Status structs.
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:XValidation:rule="isURL(self)",message="must be a valid URL" | ||
| // +kubebuilder:validation:XValidation:rule="isURL(self) ? (url(self).getScheme() == \"https\") : true",message="scheme must be https" | ||
| URL string `json:"url"` |
There was a problem hiding this comment.
Out of interest, do we know if a URL here can have a path? Or should have a path? In other examples they required the path to have a specific prefix
|
|
||
| // serviceEndpoints specifies endpoints that override the default endpoints | ||
| // used when creating clients to interact with GCP services. | ||
| // serviceEndpoints is optional. |
There was a problem hiding this comment.
So would it be fair to say
| // serviceEndpoints is optional. | |
| // When not specified, the default endpoints for the GCP region will be used. |
|
@barbacbd: The following tests failed, say
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. |
** Add the Tech preview and No upgrade tags for the new feature GCP API Custom Endpoints.
** Add the ServiceEndpoint Structure that includes the api name and endpoint.
** Add the Service Endpoints to the GCP Spec and Status structs.