Skip to content

Commit

Permalink
Set default values in API rather than in webhooks
Browse files Browse the repository at this point in the history 
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]>
  • Loading branch information
@bryan-cox
bryan-cox committed Oct 17, 2024
1 parent fa94869 commit 317973f
Show file tree
Hide file tree
Showing 23 changed files with 98 additions and 350 deletions.
8 changes: 0 additions & 8 deletions api/v1beta1/azuremachine_default.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{})
Expand Down Expand Up @@ -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()
Expand Down
5 changes: 0 additions & 5 deletions api/v1beta1/azuremachine_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
1 change: 0 additions & 1 deletion api/v1beta1/azuremachinetemplate_webhook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
47 changes: 0 additions & 47 deletions api/v1beta1/azuremachinetemplate_webhook_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
87 changes: 3 additions & 84 deletions api/v1beta1/azuremanagedcontrolplane_default.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -75,11 +75,8 @@ func (m *AzureManagedControlPlane) setDefaultVirtualNetwork() {
if m.Spec.VirtualNetwork.Name == "" {
m.Spec.VirtualNetwork.Name = m.Name
}
if m.Spec.VirtualNetwork.CIDRBlock == "" {
m.Spec.VirtualNetwork.CIDRBlock = defaultAKSVnetCIDR
if ptr.Deref(m.Spec.NetworkPluginMode, "") == NetworkPluginModeOverlay {
m.Spec.VirtualNetwork.CIDRBlock = defaultAKSVnetCIDRForOverlay
}
if ptr.Deref(m.Spec.NetworkPluginMode, "") == NetworkPluginModeOverlay {
m.Spec.VirtualNetwork.CIDRBlock = defaultAKSVnetCIDRForOverlay
}
if m.Spec.VirtualNetwork.ResourceGroup == "" {
m.Spec.VirtualNetwork.ResourceGroup = m.Spec.ResourceGroupName
Expand All @@ -106,19 +103,13 @@ 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
}

func setDefaultSku(sku *AKSSku) *AKSSku {
result := sku.DeepCopy()
if sku == nil {
result = new(AKSSku)
result.Tier = FreeManagedControlPlaneTier
} else if sku.Tier == PaidManagedControlPlaneTier {
if sku.Tier == PaidManagedControlPlaneTier {
result.Tier = StandardManagedControlPlaneTier
ctrl.Log.WithName("AzureManagedControlPlaneWebHookLogger").Info("Paid SKU tier is deprecated and has been replaced by Standard")
}
Expand All @@ -133,79 +124,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() {
Expand All @@ -219,8 +141,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)
}
}
}
85 changes: 0 additions & 85 deletions api/v1beta1/azuremanagedcontrolplane_default_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import (
"testing"

. "github.com/onsi/gomega"
"k8s.io/utils/ptr"
)

func TestAzureManagedControlPlane_SetDefaultSSHPublicKey(t *testing.T) {
Expand Down Expand Up @@ -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))
}
Loading

0 comments on commit 317973f

Please sign in to comment.