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

Jump to bottom

🌱 Add NamingStrategy to KubeadmControlPlane #11123

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🌱 Add NamingStrategy to KubeadmControlPlane #11123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 22 additions & 0 deletions controlplane/kubeadm/api/v1beta1/kubeadm_control_plane_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,11 @@ type KubeadmControlPlaneSpec struct {
// The RemediationStrategy that controls how control plane machine remediation happens.
// +optional
RemediationStrategy *RemediationStrategy `json:"remediationStrategy,omitempty"`

// MachineNamingStrategy allows changing the naming pattern used when creating Machines.
// InfraMachines & KubeadmConfigs will use the same name as the corresponding Machines.
// +optional
MachineNamingStrategy *MachineNamingStrategy `json:"machineNamingStrategy,omitempty"`
chrischdi marked this conversation as resolved.
Show resolved Hide resolved
}

// KubeadmControlPlaneMachineTemplate defines the template for Machines
Expand Down Expand Up @@ -234,6 +239,23 @@ type RemediationStrategy struct {
MinHealthyPeriod *metav1.Duration `json:"minHealthyPeriod,omitempty"`
}

// MachineNamingStrategy allows changing the naming pattern used when creating Machines.
// InfraMachines & KubeadmConfigs will use the same name as the corresponding Machines.
type MachineNamingStrategy struct {
// Template defines the template to use for generating the names of the Machine objects.
// If not defined, it will fallback to `{{ .cluster.name }}-{{ .kubeadmcontrolplane.name }}-{{ .random }}`.
// If the generated name string exceeds 63 characters, it will be trimmed to 58 characters and will
// get concatenated with a random suffix of length 5.
// Length of the template string must not exceed 93 characters.
// The template allows the following variables `.cluster.name`, `.kubeadmcontrolplane.name` and `.random`.
// The variable `.cluster.name` retrieves the name of the cluster object that owns the Machines being created.
// The variable `.kubeadmcontrolplane.name` retrieves the name of the KubeadmControlPlane object that owns the Machines being created.
// The variable `.random` is substituted with random alphanumeric string, without vowels, of length 5.
// +optional
// +kubebuilder:validation:MaxLength=93
Template string `json:"template,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

You should impose a maximal length on this string, you know that the rendered value must not exceed 63 chars, so it needs to be at least that in length, assuming a worst case, the value of any substituted variable could be 1 character right?

So it needs to be 62 plus the length in the template of representing the longest variable. Currently that is {{ .kubeadmcontrolplane.name }} which is 31 characters, so, a limit of 93 would be suitable presently

Copy link
Member

@sbueringer sbueringer Sep 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can do this slightly better and easier. We already have similar validation for the other namingStrategies we have:

cluster-api/internal/webhooks/clusterclass.go

Line 437 in 8752f4f

name, err := names.ControlPlaneNameGenerator(*clusterClass.Spec.ControlPlane.NamingStrategy.Template, "cluster").GenerateName()

So we can verify:

  • that the template actually renders
  • that the rendered output is a valid name
  • use the minimal length for our parameters and then check if the generated output is too long
    • Not sure if we actually ever hit that case, because we have that safeguard that we just cut if rendered string gets too long. So I think we don't need this validation?
    • Note: You can do a bunch of stuff in templates, so I think the template can be a lot longer without actually leading to long rendered values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I am a bit confused, do we need restrictions on minimum chars too for the generated name?

Copy link
Member

Choose a reason for hiding this comment

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

@JoelSpeed wdyt based on my comment above?

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 that sounds reasonable yes! So that would be webhook based validation, you should still also include an API side maximum length though (since we can calculate a reasonable maximum)

Having maximum lengths on all fields is good practice and helps with the runtime cost analysis that's built into the CEL libraries, should we ever get to using those.

You can add length limits when the API is introduced, restricting them later is harder

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah, good point. I forgot the CEL part

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The validation webhook that is in clusterclass I believe is checking all the things needed. I have added the same here, let me know if anything needs to be updated.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see that the webhook is checking the template, but, I think we do still want to have a length limit as a marker on the API field here, for completeness

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the marker for max length please check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to impose a pattern validation on this string?

Is there any admission time validation at present that checks that this compiles and would produce a valid template?

Copy link
Member

Choose a reason for hiding this comment

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

Good catch!
(I didn't review the implementation myself yet, only the API)

We should have a similar validation than what we have for other naming strategies:

cluster-api/internal/webhooks/clusterclass.go

Line 437 in 8752f4f

name, err := names.ControlPlaneNameGenerator(*clusterClass.Spec.ControlPlane.NamingStrategy.Template, "cluster").GenerateName()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the validation webhook, please check.

}

// KubeadmControlPlaneStatus defines the observed state of KubeadmControlPlane.
type KubeadmControlPlaneStatus struct {
// Selector is the label selector in string format to avoid introspection
Expand Down
5 changes: 5 additions & 0 deletions controlplane/kubeadm/api/v1beta1/kubeadmcontrolplanetemplate_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,11 @@ type KubeadmControlPlaneTemplateResourceSpec struct {
// The RemediationStrategy that controls how control plane machine remediation happens.
// +optional
RemediationStrategy *RemediationStrategy `json:"remediationStrategy,omitempty"`

// MachineNamingStrategy allows changing the naming pattern used when creating Machines.
// InfraMachines & KubeadmConfigs will use the same name as the corresponding Machines.
// +optional
MachineNamingStrategy *MachineNamingStrategy `json:"machineNamingStrategy,omitempty"`
}

// KubeadmControlPlaneTemplateMachineTemplate defines the template for Machines
Expand Down
25 changes: 25 additions & 0 deletions controlplane/kubeadm/api/v1beta1/zz_generated.deepcopy.go
View file
Open in desktop

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

19 changes: 19 additions & 0 deletions ...rolplane/kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanes.yaml
View file
Open in desktop

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

19 changes: 19 additions & 0 deletions .../kubeadm/config/crd/bases/controlplane.cluster.x-k8s.io_kubeadmcontrolplanetemplates.yaml
View file
Open in desktop

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

12 changes: 11 additions & 1 deletion controlplane/kubeadm/internal/controllers/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"sigs.k8s.io/cluster-api/controllers/external"
controlplanev1 "sigs.k8s.io/cluster-api/controlplane/kubeadm/api/v1beta1"
"sigs.k8s.io/cluster-api/controlplane/kubeadm/internal"
topologynames "sigs.k8s.io/cluster-api/internal/topology/names"
"sigs.k8s.io/cluster-api/internal/util/ssa"
"sigs.k8s.io/cluster-api/util"
"sigs.k8s.io/cluster-api/util/certs"
Expand Down Expand Up @@ -184,6 +185,7 @@ func (r *KubeadmControlPlaneReconciler) cloneConfigsAndGenerateMachine(ctx conte
if r.DeprecatedInfraMachineNaming {
infraMachineName = names.SimpleNameGenerator.GenerateName(kcp.Spec.MachineTemplate.InfrastructureRef.Name + "-")
}

// Clone the infrastructure template
infraRef, err := external.CreateFromTemplate(ctx, &external.CreateFromTemplateInput{
Client: r.Client,
Expand Down Expand Up @@ -349,7 +351,15 @@ func (r *KubeadmControlPlaneReconciler) computeDesiredMachine(kcp *controlplanev
annotations := map[string]string{}
if existingMachine == nil {
// Creating a new machine
machineName = names.SimpleNameGenerator.GenerateName(kcp.Name + "-")
nameTemplate := "{{ .cluster.name }}-{{ .kubeadmcontrolplane.name }}-{{ .random }}"
if kcp.Spec.MachineNamingStrategy != nil && kcp.Spec.MachineNamingStrategy.Template != "" {
nameTemplate = kcp.Spec.MachineNamingStrategy.Template
}
generatedMachineName, err := topologynames.KCPMachineNameGenerator(nameTemplate, cluster.Name, kcp.Name).GenerateName()
if err != nil {
return nil, errors.Wrap(err, "failed to generate name for KCP machine")
}
machineName = generatedMachineName
version = &kcp.Spec.Version

// Machine's bootstrap config may be missing ClusterConfiguration if it is not the first machine in the control plane.
Expand Down
Loading
Loading