CFE-1166: Customizing control plane machine names #1714
Conversation
Signed-off-by: chiragkyal <[email protected]>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
@chiragkyal: all tests passed! 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. |
| // +kubebuilder:validation:Pattern=`^[a-z0-9]([-a-z0-9]*[a-z0-9])?$` | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=32 |
There was a problem hiding this comment.
This was inspired by the ClusterID validation which follows the RFC4122 UUID value. However I'm open to suggestions if it needs to be modified.
There was a problem hiding this comment.
The name prefix should be validated as a dns1123 subdomain name, normally the max length is 253 chars, but, we need to suffix this string with 5 random chars and the index (e.g. -abcde-0) so we should reduce the max length to meet that need, 245 chars should be the max length.
You can use CEL to validate the subdomain name format now
// +kubebuilder:validation:XValidation:rule="format.dns1123label.validate(self).hasValue()",messageExpression="format.dns1123label.validate(self).value().join(\"\\n\")"
There was a problem hiding this comment.
I think the prefix validation should work for all cloud providers. I was checking GCP's naming convention https://cloud.google.com/compute/docs/naming-resources#resource-name-format, and it looks like dns1123 subdomain check will fail there.
There was a problem hiding this comment.
The name here is the name of the Machine object, which can be DNS1123 subdomain, it is up to the provider implementation to make sure that the name of the Machine can be translated to the cloud provider naming schemes. The GCP provider for example will have logic to convert the name should it not be compliant.
I'd prefer to start with a kube compatible naming scheme, and change while in TP if we find issues
Can you please check if the installer does any validation of the cluster ID, that way we can see what is there already in the fleet
There was a problem hiding this comment.
The name here is the name of the Machine object, which can be DNS1123 subdomain, it is up to the provider implementation to make sure that the name of the Machine can be translated to the cloud provider naming schemes. The GCP provider for example will have logic to convert the name should it not be compliant.
I'd prefer to start with a kube compatible naming scheme, and change while in TP if we find issues
That sounds good to me. We can start with DNS1123 subdomain validation while in TP.
So, should we use format.dns1123label or format.dns1123Subdomain for doing the CEL validation?
Can you please check if the installer does any validation of the cluster ID, that way we can see what is there already in the fleet
I checked the installer code, and started a thread with the team : https://redhat-internal.slack.com/archives/C68TNFWA2/p1732184902373169
Summary: Installer uses the ClusterName which is then transformed to create ClusterID of length 27 and follow DNS1123Subdomain
There was a problem hiding this comment.
Use subdomain for the formatting
There was a problem hiding this comment.
I tried using the CEL rule in the API. But it wasn't able to recognize format. Do we need to import some package?
compilation failed: ERROR: <input>:1:14: undeclared reference to 'format' (in container '')
There was a problem hiding this comment.
Another question: Is it acceptable to keep the index as a single-digit (e.g. -abcde-0)? I think in that case (having 245 characters as max length prefix) a cluster can have a maximum of 9 control plane nodes, right?
There was a problem hiding this comment.
OpenShift 4 only supports 3 control plane nodes, it will not support more than that in the near future, and even if it did, I would not expect us to ever support more than 5 control plane nodes (note that's the most we've ever supported across any OpenShift)
There was a problem hiding this comment.
Okay, thanks for the confirmation. Then we are good with that validation.
chiragkyal
changed the title
|
@chiragkyal: This pull request references CFE-1166 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.18.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. |
openshift-ci-robot
added
the
jira/valid-reference
|
@chiragkyal: This pull request references CFE-1166 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.18.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. |
|
/assign @JoelSpeed |
This enhancement proposal
ControlPlaneMachineSet.MachineNamePrefixfield allowing custom prefixes to be used.CMPSMachineNamePrefixfeature gate.Part of CFE-1166