Set default values in API rather than in webhooks

This commit removes default values previously set in webhooks and adds
the default values directly in the API through the kubebuilder
annotation, `kubebuilder:default`.

Signed-off-by: Bryan Cox <[email protected]>

Author: bryan-cox

api/v1beta1/azuremachine_default.go

@@ -49,13 +49,6 @@ func (s *AzureMachineSpec) SetDefaultSSHPublicKey() error {
 	return nil
 }
 
-// SetDefaultCachingType sets the default cache type for an AzureMachine.
-func (s *AzureMachineSpec) SetDefaultCachingType() {
-	if s.OSDisk.CachingType == "" {
-		s.OSDisk.CachingType = "None"
-	}
-}
-
 // SetDataDisksDefaults sets the data disk defaults for an AzureMachine.
 func (s *AzureMachineSpec) SetDataDisksDefaults() {
 	set := make(map[int32]struct{})
@@ -245,7 +238,6 @@ func (m *AzureMachine) SetDefaults(client client.Client) error {
 		errs = append(errs, errors.Wrapf(err, "failed to fetch subscription ID for AzureMachine %s/%s", m.Namespace, m.Name))
 	}
 
-	m.Spec.SetDefaultCachingType()
 	m.Spec.SetDataDisksDefaults()
 	m.Spec.SetIdentityDefaults(subscriptionID)
 	m.Spec.SetSpotEvictionPolicyDefaults()

api/v1beta1/azuremachine_webhook_test.go

@@ -988,11 +988,6 @@ func TestAzureMachine_Default(t *testing.T) {
 	g.Expect(err).NotTo(HaveOccurred())
 	g.Expect(publicKeyNotExistTest.machine.Spec.SSHPublicKey).To(Not(BeEmpty()))
 
-	cacheTypeNotSpecifiedTest := test{machine: &AzureMachine{ObjectMeta: testObjectMeta, Spec: AzureMachineSpec{OSDisk: OSDisk{CachingType: ""}}}}
-	err = mw.Default(context.Background(), cacheTypeNotSpecifiedTest.machine)
-	g.Expect(err).NotTo(HaveOccurred())
-	g.Expect(cacheTypeNotSpecifiedTest.machine.Spec.OSDisk.CachingType).To(Equal("None"))
-
 	for _, possibleCachingType := range armcompute.PossibleCachingTypesValues() {
 		cacheTypeSpecifiedTest := test{machine: &AzureMachine{ObjectMeta: testObjectMeta, Spec: AzureMachineSpec{OSDisk: OSDisk{CachingType: string(possibleCachingType)}}}}
 		err = mw.Default(context.Background(), cacheTypeSpecifiedTest.machine)

api/v1beta1/azuremachinetemplate_webhook.go

@@ -152,7 +152,6 @@ func (r *AzureMachineTemplate) Default(_ context.Context, obj runtime.Object) er
 	if err := t.Spec.Template.Spec.SetDefaultSSHPublicKey(); err != nil {
 		ctrl.Log.WithName("SetDefault").Error(err, "SetDefaultSSHPublicKey failed")
 	}
-	t.Spec.Template.Spec.SetDefaultCachingType()
 	t.Spec.Template.Spec.SetDataDisksDefaults()
 	t.Spec.Template.Spec.SetNetworkInterfacesDefaults()
 	return nil

api/v1beta1/azuremachinetemplate_webhook_test.go

@@ -346,53 +346,6 @@ func TestAzureMachineTemplate_ValidateUpdate(t *testing.T) {
 			},
 			wantErr: false,
 		},
-		{
-			name: "AzureMachineTemplate with default mismatch",
-			oldTemplate: &AzureMachineTemplate{
-				Spec: AzureMachineTemplateSpec{
-					Template: AzureMachineTemplateResource{
-						Spec: AzureMachineSpec{
-							VMSize:        "size",
-							FailureDomain: &failureDomain,
-							OSDisk: OSDisk{
-								OSType:      "type",
-								DiskSizeGB:  ptr.To[int32](11),
-								CachingType: "",
-							},
-							DataDisks:    []DataDisk{},
-							SSHPublicKey: "",
-						},
-					},
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "OldTemplate",
-				},
-			},
-			template: &AzureMachineTemplate{
-				Spec: AzureMachineTemplateSpec{
-					Template: AzureMachineTemplateResource{
-						Spec: AzureMachineSpec{
-							VMSize:        "size",
-							FailureDomain: &failureDomain,
-							OSDisk: OSDisk{
-								OSType:      "type",
-								DiskSizeGB:  ptr.To[int32](11),
-								CachingType: "None",
-							},
-							DataDisks:    []DataDisk{},
-							SSHPublicKey: "fake ssh key",
-							NetworkInterfaces: []NetworkInterface{{
-								PrivateIPConfigs: 1,
-							}},
-						},
-					},
-				},
-				ObjectMeta: metav1.ObjectMeta{
-					Name: "NewTemplate",
-				},
-			},
-			wantErr: false,
-		},
 		{
 			name: "AzureMachineTemplate ssh key removed",
 			oldTemplate: &AzureMachineTemplate{

api/v1beta1/azuremanagedcontrolplane_default.go

@@ -106,9 +106,6 @@ func setDefaultFleetsMember(fleetsMember *FleetsMember, labels map[string]string
 		if clusterName, ok := labels[clusterv1.ClusterNameLabel]; ok && fleetsMember.Name == "" {
 			result.Name = clusterName
 		}
-		if fleetsMember.Group == "" {
-			result.Group = "default"
-		}
 	}
 	return result
 }
@@ -133,79 +130,10 @@ func setDefaultVersion(version string) string {
 	return version
 }
 
-func setDefaultAutoScalerProfile(autoScalerProfile *AutoScalerProfile) *AutoScalerProfile {
-	if autoScalerProfile == nil {
-		return nil
-	}
-
-	result := autoScalerProfile.DeepCopy()
-
-	// Default values are from https://learn.microsoft.com/en-us/azure/aks/cluster-autoscaler#using-the-autoscaler-profile
-	// If any values are set, they all need to be set.
-	if autoScalerProfile.BalanceSimilarNodeGroups == nil {
-		result.BalanceSimilarNodeGroups = (*BalanceSimilarNodeGroups)(ptr.To(string(BalanceSimilarNodeGroupsFalse)))
-	}
-	if autoScalerProfile.Expander == nil {
-		result.Expander = (*Expander)(ptr.To(string(ExpanderRandom)))
-	}
-	if autoScalerProfile.MaxEmptyBulkDelete == nil {
-		result.MaxEmptyBulkDelete = ptr.To("10")
-	}
-	if autoScalerProfile.MaxGracefulTerminationSec == nil {
-		result.MaxGracefulTerminationSec = ptr.To("600")
-	}
-	if autoScalerProfile.MaxNodeProvisionTime == nil {
-		result.MaxNodeProvisionTime = ptr.To("15m")
-	}
-	if autoScalerProfile.MaxTotalUnreadyPercentage == nil {
-		result.MaxTotalUnreadyPercentage = ptr.To("45")
-	}
-	if autoScalerProfile.NewPodScaleUpDelay == nil {
-		result.NewPodScaleUpDelay = ptr.To("0s")
-	}
-	if autoScalerProfile.OkTotalUnreadyCount == nil {
-		result.OkTotalUnreadyCount = ptr.To("3")
-	}
-	if autoScalerProfile.ScanInterval == nil {
-		result.ScanInterval = ptr.To("10s")
-	}
-	if autoScalerProfile.ScaleDownDelayAfterAdd == nil {
-		result.ScaleDownDelayAfterAdd = ptr.To("10m")
-	}
-	if autoScalerProfile.ScaleDownDelayAfterDelete == nil {
-		// Default is the same as the ScanInterval so default to that same value if it isn't set
-		result.ScaleDownDelayAfterDelete = result.ScanInterval
-	}
-	if autoScalerProfile.ScaleDownDelayAfterFailure == nil {
-		result.ScaleDownDelayAfterFailure = ptr.To("3m")
-	}
-	if autoScalerProfile.ScaleDownUnneededTime == nil {
-		result.ScaleDownUnneededTime = ptr.To("10m")
-	}
-	if autoScalerProfile.ScaleDownUnreadyTime == nil {
-		result.ScaleDownUnreadyTime = ptr.To("20m")
-	}
-	if autoScalerProfile.ScaleDownUtilizationThreshold == nil {
-		result.ScaleDownUtilizationThreshold = ptr.To("0.5")
-	}
-	if autoScalerProfile.SkipNodesWithLocalStorage == nil {
-		result.SkipNodesWithLocalStorage = (*SkipNodesWithLocalStorage)(ptr.To(string(SkipNodesWithLocalStorageFalse)))
-	}
-	if autoScalerProfile.SkipNodesWithSystemPods == nil {
-		result.SkipNodesWithSystemPods = (*SkipNodesWithSystemPods)(ptr.To(string(SkipNodesWithSystemPodsTrue)))
-	}
-
-	return result
-}
-
 func (m *AzureManagedControlPlane) setDefaultOIDCIssuerProfile() {
 	if m.Spec.OIDCIssuerProfile == nil {
 		m.Spec.OIDCIssuerProfile = &OIDCIssuerProfile{}
 	}
-
-	if m.Spec.OIDCIssuerProfile.Enabled == nil {
-		m.Spec.OIDCIssuerProfile.Enabled = ptr.To(false)
-	}
 }
 
 func (m *AzureManagedControlPlane) setDefaultDNSPrefix() {
@@ -219,8 +147,5 @@ func (m *AzureManagedControlPlane) setDefaultAKSExtensions() {
 		if extension.Plan != nil && extension.Plan.Name == "" {
 			extension.Plan.Name = fmt.Sprintf("%s-%s", m.Name, extension.Plan.Product)
 		}
-		if extension.AutoUpgradeMinorVersion == nil {
-			extension.AutoUpgradeMinorVersion = ptr.To(true)
-		}
 	}
 }

api/v1beta1/azuremanagedcontrolplane_default_test.go

@@ -20,7 +20,6 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
-	"k8s.io/utils/ptr"
 )
 
 func TestAzureManagedControlPlane_SetDefaultSSHPublicKey(t *testing.T) {
@@ -54,87 +53,3 @@ func hardcodedAzureManagedControlPlaneWithSSHKey(sshPublicKey string) *AzureMana
 		},
 	}
 }
-
-func TestSetDefaultAutoScalerProfile(t *testing.T) {
-	g := NewWithT(t)
-
-	type test struct {
-		amcp *AzureManagedControlPlane
-	}
-
-	defaultAMP := &AzureManagedControlPlane{
-		Spec: AzureManagedControlPlaneSpec{
-			AzureManagedControlPlaneClassSpec: AzureManagedControlPlaneClassSpec{
-				AutoScalerProfile: &AutoScalerProfile{
-					BalanceSimilarNodeGroups:      (*BalanceSimilarNodeGroups)(ptr.To(string(BalanceSimilarNodeGroupsFalse))),
-					Expander:                      (*Expander)(ptr.To(string(ExpanderRandom))),
-					MaxEmptyBulkDelete:            ptr.To("10"),
-					MaxGracefulTerminationSec:     ptr.To("600"),
-					MaxNodeProvisionTime:          ptr.To("15m"),
-					MaxTotalUnreadyPercentage:     ptr.To("45"),
-					NewPodScaleUpDelay:            ptr.To("0s"),
-					OkTotalUnreadyCount:           ptr.To("3"),
-					ScanInterval:                  ptr.To("10s"),
-					ScaleDownDelayAfterAdd:        ptr.To("10m"),
-					ScaleDownDelayAfterDelete:     ptr.To("10s"),
-					ScaleDownDelayAfterFailure:    ptr.To("3m"),
-					ScaleDownUnneededTime:         ptr.To("10m"),
-					ScaleDownUnreadyTime:          ptr.To("20m"),
-					ScaleDownUtilizationThreshold: ptr.To("0.5"),
-					SkipNodesWithLocalStorage:     (*SkipNodesWithLocalStorage)(ptr.To(string(SkipNodesWithLocalStorageFalse))),
-					SkipNodesWithSystemPods:       (*SkipNodesWithSystemPods)(ptr.To(string(SkipNodesWithSystemPodsTrue))),
-				},
-			},
-		},
-	}
-
-	allFieldsAreNilTest := test{amcp: &AzureManagedControlPlane{
-		Spec: AzureManagedControlPlaneSpec{
-			AzureManagedControlPlaneClassSpec: AzureManagedControlPlaneClassSpec{
-				AutoScalerProfile: &AutoScalerProfile{},
-			},
-		},
-	}}
-
-	allFieldsAreNilTest.amcp.Spec.AutoScalerProfile = setDefaultAutoScalerProfile(allFieldsAreNilTest.amcp.Spec.AutoScalerProfile)
-
-	g.Expect(allFieldsAreNilTest.amcp.Spec.AutoScalerProfile).To(Equal(defaultAMP.Spec.AutoScalerProfile))
-
-	expectedNotNil := &AzureManagedControlPlane{
-		Spec: AzureManagedControlPlaneSpec{
-			AzureManagedControlPlaneClassSpec: AzureManagedControlPlaneClassSpec{
-				AutoScalerProfile: &AutoScalerProfile{
-					BalanceSimilarNodeGroups:      (*BalanceSimilarNodeGroups)(ptr.To(string(BalanceSimilarNodeGroupsTrue))),
-					Expander:                      (*Expander)(ptr.To(string(ExpanderLeastWaste))),
-					MaxEmptyBulkDelete:            ptr.To("5"),
-					MaxGracefulTerminationSec:     ptr.To("300"),
-					MaxNodeProvisionTime:          ptr.To("10m"),
-					MaxTotalUnreadyPercentage:     ptr.To("30"),
-					NewPodScaleUpDelay:            ptr.To("30s"),
-					OkTotalUnreadyCount:           ptr.To("5"),
-					ScanInterval:                  ptr.To("20s"),
-					ScaleDownDelayAfterAdd:        ptr.To("5m"),
-					ScaleDownDelayAfterDelete:     ptr.To("1m"),
-					ScaleDownDelayAfterFailure:    ptr.To("2m"),
-					ScaleDownUnneededTime:         ptr.To("5m"),
-					ScaleDownUnreadyTime:          ptr.To("10m"),
-					ScaleDownUtilizationThreshold: ptr.To("0.4"),
-					SkipNodesWithLocalStorage:     (*SkipNodesWithLocalStorage)(ptr.To(string(SkipNodesWithLocalStorageTrue))),
-					SkipNodesWithSystemPods:       (*SkipNodesWithSystemPods)(ptr.To(string(SkipNodesWithSystemPodsFalse))),
-				},
-			},
-		},
-	}
-
-	allFieldsAreNotNilTest := test{amcp: &AzureManagedControlPlane{
-		Spec: AzureManagedControlPlaneSpec{
-			AzureManagedControlPlaneClassSpec: AzureManagedControlPlaneClassSpec{
-				AutoScalerProfile: ptr.To(*expectedNotNil.Spec.AutoScalerProfile),
-			},
-		},
-	}}
-
-	allFieldsAreNotNilTest.amcp.Spec.AutoScalerProfile = setDefaultAutoScalerProfile(allFieldsAreNotNilTest.amcp.Spec.AutoScalerProfile)
-
-	g.Expect(allFieldsAreNotNilTest.amcp.Spec.AutoScalerProfile).To(Equal(expectedNotNil.Spec.AutoScalerProfile))
-}