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 @@ -48,13 +48,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 @@ -244,7 +237,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(ctx context.Context, obj runtime.Object)
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 @@ -347,53 +347,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
94 changes: 5 additions & 89 deletions api/v1beta1/azuremanagedcontrolplane_default.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,11 +74,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 @@ -90,11 +87,8 @@ func (m *AzureManagedControlPlane) setDefaultSubnet() {
if m.Spec.VirtualNetwork.Subnet.Name == "" {
m.Spec.VirtualNetwork.Subnet.Name = m.Name
}
if m.Spec.VirtualNetwork.Subnet.CIDRBlock == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

For pull-cluster-api-provider-azure-e2e-aks

Expected success, but got an error:
    <errors.aggregate | len:1, cap:1>: 
    admission webhook "validation.azuremanagedcontrolplanes.infrastructure.cluster.x-k8s.io" denied the request: [spec.virtualNetwork.CIDRBlock: Invalid value: "": pre-existing virtual networks CIDR block is invalid, spec.virtualNetwork.Subnet.CIDRBlock: Invalid value: "": pre-existing subnets CIDR block is invalid]
    [

Not sure how these would be empty when they are defaulted here and here.

Does the e2e use any API changes from the PR?

Webhooks are 100% invoked by the e2e tests. I'm not exactly sure why this specific error is cropping up, but the logic around here seems non-trivial so I'd be okay to omit the CIDR block changes from this PR. My main idea with #4204 was to pick off the low-hanging fruit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok sounds good. I'll put those back.

m.Spec.VirtualNetwork.Subnet.CIDRBlock = defaultAKSNodeSubnetCIDR
if ptr.Deref(m.Spec.NetworkPluginMode, "") == NetworkPluginModeOverlay {
m.Spec.VirtualNetwork.Subnet.CIDRBlock = defaultAKSNodeSubnetCIDRForOverlay
}
if ptr.Deref(m.Spec.NetworkPluginMode, "") == NetworkPluginModeOverlay {
m.Spec.VirtualNetwork.Subnet.CIDRBlock = defaultAKSNodeSubnetCIDRForOverlay
}
}

Expand All @@ -105,19 +99,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 @@ -132,79 +120,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 @@ -218,8 +137,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