Remove kubeadm dependency when machinepool enabled

[salasberryfin]

/kind bug

What this PR does / why we need it:

When installing the provider with the MachinePool feature enabled, the controller watches Kubeadm resources which forces the user to exclusively use Kubeadm as a bootstrap provider. As specified in #4854, users may need to use any bootstrap providers of their choice. Other infrastructure providers, such as CAPA don't have this dependency on Kubeadm. Unless there is a specific need for this watcher, I suggest we remove this dependency.

Which issue(s) this PR fixes:
Fixes #4854

Special notes for your reviewer:

TODOs:

• squashed commits
• includes documentation
• adds unit tests

Release note:

kubeadm is no longer required when enabling `MachinePool`

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign sbueringer for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[salasberryfin]

/retest

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.01%. Comparing base (3c61283) to head (0155ed9).
Report is 92 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4868      +/-   ##
==========================================
- Coverage   62.04%   62.01%   -0.03%     
==========================================
  Files         201      201              
  Lines       16878    16873       -5     
==========================================
- Hits        10472    10464       -8     
- Misses       5623     5627       +4     
+ Partials      783      782       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jackfrancis]

/assign @nojnhuh @willie-yao

[willie-yao]

This change was made in #3134 to fix issues with custom data changes. @jackfrancis Since it looks like you took a look at that PR, wouldn't this break that use-case?

[mboersma]

AFAICT merging this would break the fix in #3134, as without the watch the token won't be refreshed. Having said that, it's bad design to tie the CAPZ implementation to Kubeadm, as @salasberryfin points out.

Could we somehow make adding the watch conditional on whether kubeadm is actually being used for bootstrap? I assume it's not the import statements that are specifically problematic.

Or is there another fix we can imagine for #3134 that avoids the watch entirely?

[salasberryfin]

Hey @mboersma, thanks for the detailed explanation. I think it is worth investigating how to avoid this dependency as this is a blocker for anyone wanting to use CAPZ + MachinePool without Kubeadm. I'd say if there is a way we can make the watch conditional, as you mentioned, it would be great, but I don't know if this would be a feasible change. Do you have any proposals on how to implement this? Perhaps any other components in other CAPI projects that implement a similar solution?

[mboersma]

any other components in other CAPI projects that implement a similar solution?

I've done some looking around, but don't yet see an obvious way to know about the BootstrapProvider from down in the AMP controller code. Nor have I found any relevant code in CAPI...but it's entirely possible I missed something.

What if we moved the kubeadm watch code out of this block where we call Build() and into its own code block just afterward, where it can log a warning instead of returning an error?

[mboersma]

@salasberryfin maybe moving the kubeadm watch code into its own block that just logs the error would work? Something like this:

...
		// watch for changes in AzureManagedControlPlane resources
		Watches(
			&infrav1.AzureManagedControlPlane{},
			handler.EnqueueRequestsFromMapFunc(azureManagedControlPlaneMapper),
		).
		Build(r)
	if err != nil {
		return errors.Wrap(err, "error creating controller")
	}

	// watch for changes in KubeadmConfig to sync bootstrap token
	if err := c.Watch(
		source.Kind(mgr.GetCache(), &kubeadmv1.KubeadmConfig{}),
		handler.EnqueueRequestsFromMapFunc(KubeadmConfigToInfrastructureMapFunc(ctx, ampr.Client, log)),
		predicate.ResourceVersionChangedPredicate{},
	); err != nil {
		log.Error(err, "failed adding a watch for KubeadmConfig")
	}

	if err := c.Watch(
		source.Kind(mgr.GetCache(), &infrav1exp.AzureMachinePoolMachine{}),
...

[alexander-demicev]

@mboersma That should work, can we maybe log a warning instead of an error?

[mboersma]

maybe log a warning instead of an error?

Yes, it's meant to be a warning. I think this logger only gives us Info() and Error(), so maybe use Info() instead?

[alexander-demicev]

@mboersma +1, I also think this watch has to be configurable depending on what bootstrap provider people are using, kubeadm is the most popular one, but for example, the same problem can be found when using K3S provider. Ideally, we need to find a way to make it configurable somehow on the user side. I believe the issue will appear for every other provider that supports machine pools.

[mboersma]

See #4931

[k8s-ci-robot]

PR needs rebase.

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.

[nojnhuh]

Per #4931 (review),

/close

[k8s-ci-robot]

@nojnhuh: Closed this PR.

In response to this:

Per #4931 (review),

/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-sigs/prow repository.