Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Set default values in API rather than in webhooks #5111

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Set default values in API rather than in webhooks #5111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 0 additions & 8 deletions api/v1beta1/azuremachine_default.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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