Migrated AzureASOManaged API to v1beta1

[alimaazamat]

What type of PR is this?
/kind feature

What this PR does / why we need it:
Based on this roadmap: https://capz.sigs.k8s.io/roadmap
We want to move AzureASOManaged API from v1alpha1 to v1beta1.
Edited aks-aso flavor template to use new v1beta1.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #5659

Special notes for your reviewer:

TODOs:

• squashed commits
• includes documentation
• adds unit tests
• cherry-pick candidate

Release note:

Moved AzureASOManaged API to v1beta1

[codecov]

Codecov Report

Attention: Patch coverage is 56.36364% with 48 lines in your changes missing coverage. Please review.

Project coverage is 52.83%. Comparing base (1d97b87) to head (5aafd56).
Report is 23 commits behind head on main.

Files with missing lines	Patch %	Lines
controllers/azureasomanagedcluster_controller.go	38.88%	10 Missing and 1 partial ⚠️
...trollers/azureasomanagedcontrolplane_controller.go	56.25%	6 Missing and 1 partial ⚠️
...ntrollers/azureasomanagedmachinepool_controller.go	45.45%	5 Missing and 1 partial ⚠️
api/v1beta1/azureasomanagedcluster_types.go	33.33%	4 Missing ⚠️
api/v1beta1/azureasomanagedcontrolplane_types.go	33.33%	4 Missing ⚠️
api/v1beta1/azureasomanagedmachinepool_types.go	33.33%	4 Missing ⚠️
main.go	0.00%	3 Missing ⚠️
api/v1beta1/azureasomanagedcluster_webhook.go	0.00%	2 Missing ⚠️
api/v1beta1/azureasomanagedcontrolplane_webhook.go	0.00%	2 Missing ⚠️
api/v1beta1/azureasomanagedmachinepool_webhook.go	0.00%	2 Missing ⚠️
... and 2 more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5660      +/-   ##
==========================================
- Coverage   53.20%   52.83%   -0.37%     
==========================================
  Files         272      278       +6     
  Lines       29583    29610      +27     
==========================================
- Hits        15740    15645      -95     
- Misses      13023    13148     +125     
+ Partials      820      817       -3     

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nojnhuh]

This is looking good so far!

I forgot about the None spec.conversion.strategy for CRDs which means we don't have to implement any conversion webhooks as long as there's no delta between v1alpha1 and v1beta1. We should make sure that if we do end up adding something to v1beta1 before we remove v1alpha1, we can change that and add conversion webhooks without causing any compatibility issues, and if CI will somehow flag that None is no longer viable when some change is introduced.

The other thing we should address is how to update the templates. I think the aks-aso flavor template can be updated to v1beta1 right away. For tests it would be best to cover both versions, but I'd probably prefer v1beta1 if we can only pick one.

[nojnhuh]

We should update these references here to the new API version too (and also for the other new _webhook.go files):

Suggested change

	// +kubebuilder:webhook:verbs=create,path=/validate-infrastructure-cluster-x-k8s-io-v1alpha1-azureasomanagedcluster,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=azureasomanagedclusters,versions=v1alpha1,name=validation.azureasomanagedcluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1
	// +kubebuilder:webhook:verbs=create,path=/validate-infrastructure-cluster-x-k8s-io-v1beta1-azureasomanagedcluster,mutating=false,failurePolicy=fail,groups=infrastructure.cluster.x-k8s.io,resources=azureasomanagedclusters,versions=v1beta1,name=validation.azureasomanagedcluster.infrastructure.cluster.x-k8s.io,sideEffects=None,admissionReviewVersions=v1;v1beta1

[nojnhuh]

Then we should be able to remove the v1alpha1 webhooks entirely.

I see CAPI only implements webhooks for v1beta2 Clusters. Conversion must automatically take care of the rest. I at least see the same validation applied to both v1alpha1 and v1beta1 of AzureASOManagedCluster with only the one webhook defined.

[nojnhuh]

Dropping this here for myself to make sure no unexpected changes sneak in:

for alpha in api/v1alpha1/azureasomanaged*; do beta=${alpha/v1alpha1/v1beta1}; [ -f $beta ] && git diff --no-index $alpha $beta; done

[nojnhuh]

This isn't covered in e2e atm, but we should at least manually verify that upgrading from a previous version of CAPZ that used v1alpha1 as the storage version doesn't break existing workload clusters when the CRD is updated to the new storage version.

We should definitely follow up soon after this though with automated tests if we absolutely have to cut the release with the rest of these changes before then.

[alimaazamat]

Finished manually verify that upgrading from a previous version of CAPZ that used v1alpha1 as the storage version doesn't break existing workload clusters when the CRD is updated to the new storage version.
Did this by

1. switching to main branch that uses v1alpha1 and spinning up my management and workload aks-aso cluster
2. switching to dev branch that uses v1beta1, rerunning make generate, and rerunning make tilt-up (this applied the updates correctly)
3. used cli to verify storage versions and logs
4. scaled the machine pools and worked
5. added tags in the ManagedCluster in the AzureASOManagedControlPlane and it shows up in Azure Portal
6. checked CAPZ and CAPI logs and only errors are the deprecated v1alpha1 warnings we added

[nojnhuh]

One other chunk here I forgot is updating the controllers to operate against the new API version. That should be as simple as updating all of the imports that look like this:

infrav1alpha "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha1"

to:

infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1"

The only place I think we still need that is in main.go.

Pro tip: I think vscode's go plugin knows how to rename imports, so the easiest way to go about this is:

1. Look for all instances of the "sigs.k8s.io/cluster-api-provider-azure/api/v1alpha1" import
2. Rename infrav1alpha to infrav1 (this should also update all the references in the file)
3. Change the import statement's path to sigs.k8s.io/cluster-api-provider-azure/api/v1beta1
4. Enjoy not getting carpal tunnel syndrome with one PR

[alimaazamat]

The only place I think we still need that is in main.go.

Can you elaborate on this @nojnhuh ? Are you saying to keep infrav1alpha in main.go?

[alimaazamat]

``Also editing resource_reconciler.go because ResourceStatus type uses v1alpha1 which is used in resourceStatusObject interface in controllers like in here

	r.newResourceReconciler = func(asoManagedCluster *infrav1.AzureASOManagedMachinePool, resources []*unstructured.Unstructured) resourceReconciler {
		return &ResourceReconciler{
			Client:    r.Client,
			resources: resources,
			owner:     asoManagedCluster,
			watcher:   externalTracker,
		}
	}

[nojnhuh]

The only place I think we still need that is in main.go.

Can you elaborate on this @nojnhuh ? Are you saying to keep infrav1alpha in main.go?

Yes that's what I meant. After playing around with it though, it looks like we don't need that if we update to use v1beta1 everywhere within the CAPZ binary. Let's try removing all imports to the v1alpha1 package (including removing it in main.go) and see how that fares in CI.

[nojnhuh]

Not to steal any of the fun here, but I noticed this extractable smaller chunk of work we need as part of making a new API version available but doesn't require any of the changes here to work on its own: #5662.

[nojnhuh]

We should mark each of the v1alpha1 types as deprecated per https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#version-deprecation.

I see kubebuilder makes that easy with a marker like this:

//+kubebuilder:deprecatedversion:warning="infrastructure.cluster.x-k8s.io/v1alpha1 AzureASOManagedCluster is deprecated. infrastructure.cluster.x-k8s.io/v1beta1 is equivalent and should be used instead."

Then that warning gets blasted whenever that API version is used to prompt people to update:

% kubectl get azureasomanagedcluster.v1alpha1.infrastructure.cluster.x-k8s.io  
Warning: infrastructure.cluster.x-k8s.io/v1alpha1 AzureASOManagedCluster is deprecated. infrastructure.cluster.x-k8s.io/v1beta1 should be used instead.
NAME           AGE
aks-aso-6548   45s

[willie-yao]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 4b29a0d5ee97971ff02db865317cf31d57a17a31

[nojnhuh]

I see the feature gate is already enabled by default, but let's also graduate the feature gate to GA to approach parity with the existing AKS things:

cluster-api-provider-azure/feature/feature.go

Line 74 in 31fcaaa

ASOAPI: {Default: true, PreRelease: featuregate.Alpha},

[alimaazamat]

ASOAPI: permanent reserved term for ffs, as APIs mature/removed the ff capabilities will change
default val false -> default val true -> lock val to true -> eventually remove the ff when api is stable
right now feature gate is enabled by default already so we can classify as GA

[nojnhuh]

One last nit: I see a couple lingering references to v1alpha1 in the docs that we might as well clean up:

• cluster-api-provider-azure/docs/book/src/managed/asomanagedcluster.md

  Line 21 in e941db9

  apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1

• cluster-api-provider-azure/docs/book/src/managed/asomanagedcluster.md

  Line 75 in e941db9

  apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1

[alimaazamat]

/retest

[nojnhuh]

/lgtm
/approve

Great work on this!

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: eefdb09b5f2a9aeab6702f86c16d3c235546236b

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nojnhuh

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [nojnhuh]

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

[nojnhuh]

/test pull-cluster-api-provider-azure-e2e

[nojnhuh]

/retest-required

[alimaazamat]

/retest-required

[nojnhuh]

#5686

/test pull-cluster-api-provider-azure-e2e

[nojnhuh]

#5686 again

/test pull-cluster-api-provider-azure-e2e

[k8s-ci-robot]

@alimaazamat: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-apidiff	5aafd56	link	false	/test pull-cluster-api-provider-azure-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

[nojnhuh]

#5687

/test pull-cluster-api-provider-azure-e2e