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
75 changes: 0 additions & 75 deletions api/v1beta1/azuremanagedcontrolplane_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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
bryan-cox marked this conversation as resolved.
Show resolved Hide resolved
// 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
Comment on lines -176 to -177
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this can be expressed in the kubebuilder annotations, so maybe best to keep just this field here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't setting this to the same default value in the kubebuilder annotations accomplishing the same purpose here though? It's just a default value if the value isn't set, which what was happening here before.

}
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 +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)
}
}
}
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