Enforce good selectors on MachineDeployments

[killianmuldoon]

As documented in #5957 it is possible to configure MachineDeployment Selectors in Cluster API minimally e.g. using the cluster name only, so that they are created correctly but are not unique enough to ensure correct adoption, for example in the event of a backup and restore. The default selector, set when no selector is set on the MachineDeployment, contains both the Cluster name and the MachineDeployment name and is sufficient to ensure adoption occurs correctly.

This configuration can cause unexpected rollouts to occur when attempting to adopt a cluster by a restored management cluster. It can also cause Machines to be adopted by the wrong MachineSet, putting the cluster in a working but invalid state.

Right now the correct form of selector is opaque to users and it only causes an issue on Day N when MachineDeployment and MachineSet adoption unexpectedly shows errors.

To solve this we could (from strongest to weakest):

1. Add the default selectors to the MachineDeployment on creation, even if another selector is defined.
2. Block the creation of MachineDeployments with insufficiently specific selectors?
3. Create a webhook warning (requires Support warnings in webhook utils controller-runtime#1788) to advise users on creating MachineDeployments
4. Document the requirement for MachineDeployment selectors.

I'd prefer to got with the stronger version of this to preclude any future occurrence of this issue. I'm not sure if there's workflows that would be blocked by making strong assumptions about the MachineDeployment / MachineSet selector.

Is there any reason we shouldn't default the selector to always include both the Cluster name and the MachineDeployment name?

[fabriziopandini]

/kind feature
/milestone v1.2

I think the current implementation derives from Kubernetes Deployments, and AFAIK also there it is up to the users to provide not overlapping selectors, but at the same time it allows the users to choose their own criteria for uniqueness.

I'm also worried by the effect of enforcing all the selectors on existing MD (it is probably a breaking change, and as far as I remember it will trigger rollouts on upgrade).

given that my first reaction is to go for option 3, but probably this requires some more thinking/different opinions

[sbueringer]

Same here. I would prefer option 3. I think it should be enough to nudge users a bit.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[fabriziopandini]

/lifecycle frozen

[fabriziopandini]

/milestone Next

[fabriziopandini]

/triage accepted
/help
There is ongoing work in kubernetes-sigs/controller-runtime#2014 to implement warning in controller runtime, this could potentially unblock this issue

[k8s-ci-robot]

@fabriziopandini:
This request has been marked as needing help from a contributor.

Guidelines

Please ensure that the issue body includes answers to the following questions:

• Why are we solving this issue?
• To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
• Does this issue have zero to low barrier of entry?
• How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/triage accepted
/help
There is ongoing work in kubernetes-sigs/controller-runtime#2014 to implement warning in controller runtime, this could potentially unblock this issue

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/test-infra repository.

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[fabriziopandini]

/priority important-longterm

[fabriziopandini]

The Cluster API project currently lacks enough contributors to adequately respond to all issues and PRs.

The idea of having a webhook providing a warning is still interesting, but it has a downside that we need to use an APi reader inside a webhook + no one is volounteering for this work/showing interest on the issue since a long time

/close

[k8s-ci-robot]

@fabriziopandini: Closing this issue.

In response to this:

The Cluster API project currently lacks enough contributors to adequately respond to all issues and PRs.

The idea of having a webhook providing a warning is still interesting, but it has a downside that we need to use an APi reader inside a webhook + no one is volounteering for this work/showing interest on the issue since a long time

/close

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/test-infra repository.