diff --git a/azure/scope/managedcontrolplane.go b/azure/scope/managedcontrolplane.go index 5b1c3b2d8df..b9680ab79e6 100644 --- a/azure/scope/managedcontrolplane.go +++ b/azure/scope/managedcontrolplane.go @@ -437,37 +437,16 @@ func (s *ManagedControlPlaneScope) SetAutoUpgradeVersionStatus(version string) { s.ControlPlane.Status.AutoUpgradeVersion = version } -// IsManagedVersionUpgrade checks if auto upgrade profile is set and if the upgradeChannel is of the type patch, stable or rapid. +// IsManagedVersionUpgrade checks if version is auto managed by AKS. func (s *ManagedControlPlaneScope) IsManagedVersionUpgrade() bool { - return s.IsPatchAutoUpgrade() || s.IsStableAutoUpgrade() || s.IsRapidAutoUpgrade() + return isManagedVersionUpgrade(s.ControlPlane) } -// IsAutoUpgradeStable checks if auto upgrade channel is stable. -func (s *ManagedControlPlaneScope) IsStableAutoUpgrade() bool { - if s.ControlPlane.Spec.AutoUpgradeProfile != nil { - if upgradeChannel := s.ControlPlane.Spec.AutoUpgradeProfile.UpgradeChannel; upgradeChannel == infrav1exp.UpgradeChannelStable { - return true - } - } - return false -} - -// IsAutoUpgradeStable checks if auto upgrade channel is rapid. -func (s *ManagedControlPlaneScope) IsRapidAutoUpgrade() bool { - if s.ControlPlane.Spec.AutoUpgradeProfile != nil { - if upgradeChannel := s.ControlPlane.Spec.AutoUpgradeProfile.UpgradeChannel; upgradeChannel == infrav1exp.UpgradeChannelRapid { - return true - } - } - return false -} - -// IsAutoUpgradeStable checks if auto upgrade channel is patch. -func (s *ManagedControlPlaneScope) IsPatchAutoUpgrade() bool { - if s.ControlPlane.Spec.AutoUpgradeProfile != nil { - if upgradeChannel := s.ControlPlane.Spec.AutoUpgradeProfile.UpgradeChannel; upgradeChannel == infrav1exp.UpgradeChannelPatch { - return true - } +func isManagedVersionUpgrade(managedControlPlane *infrav1exp.AzureManagedControlPlane) bool { + if managedControlPlane.Spec.AutoUpgradeProfile != nil && + (managedControlPlane.Spec.AutoUpgradeProfile.UpgradeChannel != infrav1exp.UpgradeChannelNone && + managedControlPlane.Spec.AutoUpgradeProfile.UpgradeChannel != infrav1exp.UpgradeChannelNodeImage) { + return true } return false } diff --git a/azure/scope/managedcontrolplane_test.go b/azure/scope/managedcontrolplane_test.go index acb384f98a1..b8db1b0d4d5 100644 --- a/azure/scope/managedcontrolplane_test.go +++ b/azure/scope/managedcontrolplane_test.go @@ -21,11 +21,10 @@ import ( "testing" "github.com/Azure/go-autorest/autorest" - "github.com/Azure/go-autorest/autorest/to" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/utils/pointer" + pointer "k8s.io/utils/ptr" "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/services/managedclusters" infrav1 "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" @@ -81,6 +80,7 @@ func TestManagedControlPlaneScope_PoolVersion(t *testing.T) { Mode: "System", Cluster: "cluster1", VnetSubnetID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups//providers/Microsoft.Network/virtualNetworks//subnets/", + OSType: pointer.To("Linux"), }, }, }, @@ -119,9 +119,10 @@ func TestManagedControlPlaneScope_PoolVersion(t *testing.T) { SKU: "Standard_D2s_v3", Mode: "System", Replicas: 1, - Version: to.StringPtr("1.21.1"), + Version: pointer.To("1.21.1"), Cluster: "cluster1", VnetSubnetID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups//providers/Microsoft.Network/virtualNetworks//subnets/", + OSType: pointer.To("Linux"), }, }, }, @@ -515,7 +516,7 @@ func TestManagedControlPlaneScope_DisableLocalAccounts(t *testing.T) { }, Spec: infrav1.AzureManagedControlPlaneSpec{ SubscriptionID: "00000000-0000-0000-0000-000000000000", - DisableLocalAccounts: pointer.BoolPtr(true), + DisableLocalAccounts: pointer.To(true), }, }, ManagedMachinePools: []ManagedMachinePool{ @@ -550,7 +551,7 @@ func TestManagedControlPlaneScope_DisableLocalAccounts(t *testing.T) { Managed: true, AdminGroupObjectIDs: []string{"00000000-0000-0000-0000-000000000000"}, }, - DisableLocalAccounts: pointer.BoolPtr(true), + DisableLocalAccounts: pointer.To(true), }, }, ManagedMachinePools: []ManagedMachinePool{ @@ -560,7 +561,7 @@ func TestManagedControlPlaneScope_DisableLocalAccounts(t *testing.T) { }, }, }, - Expected: pointer.BoolPtr(true), + Expected: pointer.To(true), }, } for _, c := range cases { @@ -642,7 +643,7 @@ func TestIsAaDEnabled(t *testing.T) { Managed: true, AdminGroupObjectIDs: []string{"00000000-0000-0000-0000-000000000000"}, }, - DisableLocalAccounts: pointer.BoolPtr(true), + DisableLocalAccounts: pointer.To(true), }, }, ManagedMachinePools: []ManagedMachinePool{ @@ -766,7 +767,7 @@ func TestIsLocalAcountsDisabled(t *testing.T) { Managed: true, AdminGroupObjectIDs: []string{"00000000-0000-0000-0000-000000000000"}, }, - DisableLocalAccounts: pointer.BoolPtr(true), + DisableLocalAccounts: pointer.To(true), }, }, ManagedMachinePools: []ManagedMachinePool{ diff --git a/azure/scope/managedmachinepool.go b/azure/scope/managedmachinepool.go index ae6ccd85961..1da1d3070df 100644 --- a/azure/scope/managedmachinepool.go +++ b/azure/scope/managedmachinepool.go @@ -23,11 +23,13 @@ import ( "github.com/Azure/go-autorest/autorest/to" "github.com/pkg/errors" + "k8s.io/utils/ptr" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" infrav1exp "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/util/futures" "sigs.k8s.io/cluster-api-provider-azure/util/tele" + "sigs.k8s.io/cluster-api-provider-azure/util/versions" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" clusterv1exp "sigs.k8s.io/cluster-api/exp/api/v1beta1" "sigs.k8s.io/cluster-api/util/conditions" @@ -135,11 +137,7 @@ func (s *ManagedMachinePoolScope) AgentPoolSpec() azure.AgentPoolSpec { func buildAgentPoolSpec(managedControlPlane *infrav1exp.AzureManagedControlPlane, machinePool *clusterv1exp.MachinePool, managedMachinePool *infrav1exp.AzureManagedMachinePool) azure.AgentPoolSpec { - var normalizedVersion *string - if machinePool.Spec.Template.Spec.Version != nil { - v := strings.TrimPrefix(*machinePool.Spec.Template.Spec.Version, "v") - normalizedVersion = &v - } + normalizedVersion := getManagedMachinePoolVersion(managedControlPlane, machinePool) replicas := int32(1) if machinePool.Spec.Replicas != nil { @@ -300,3 +298,24 @@ func (s *ManagedMachinePoolScope) UpdateCAPIMachinePoolAnnotations(ctx context.C func (s *ManagedMachinePoolScope) GetCAPIMachinePoolAnnotation(ctx context.Context, key string) string { return s.MachinePool.Annotations[key] } + +// IsManagedVersionUpgrade checks if version is auto managed by AKS. +func (s *ManagedMachinePoolScope) IsManagedAutoUpgrade() bool { + return isManagedVersionUpgrade(s.ControlPlane) +} + +func getManagedMachinePoolVersion(managedControlPlane *infrav1exp.AzureManagedControlPlane, + machinePool *clusterv1exp.MachinePool) *string { + var v, av string + if machinePool != nil { + v = ptr.Deref(machinePool.Spec.Template.Spec.Version, "") + } + if managedControlPlane != nil { + av = managedControlPlane.Status.AutoUpgradeVersion + } + higherVersion, err := versions.GetHigherK8sVersion(v, av) + if err != nil { + return nil + } + return ptr.To(strings.TrimPrefix(higherVersion, "v")) +} diff --git a/azure/scope/managedmachinepool_test.go b/azure/scope/managedmachinepool_test.go index 907ca55a31a..dba02a8837c 100644 --- a/azure/scope/managedmachinepool_test.go +++ b/azure/scope/managedmachinepool_test.go @@ -25,6 +25,7 @@ import ( . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/utils/ptr" "sigs.k8s.io/cluster-api-provider-azure/azure" infrav1 "sigs.k8s.io/cluster-api-provider-azure/exp/api/v1beta1" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" @@ -66,7 +67,7 @@ func TestManagedMachinePoolScope_Autoscaling(t *testing.T) { }, }, Expected: azure.AgentPoolSpec{ - + OSType: ptr.To("Linux"), Name: "pool0", SKU: "Standard_D2s_v3", Replicas: 1, @@ -108,6 +109,7 @@ func TestManagedMachinePoolScope_Autoscaling(t *testing.T) { MinCount: to.Int32Ptr(2), MaxCount: to.Int32Ptr(10), VnetSubnetID: "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups//providers/Microsoft.Network/virtualNetworks//subnets/", + OSType: ptr.To("Linux"), }, }, } @@ -160,7 +162,7 @@ func TestManagedMachinePoolScope_NodeLabels(t *testing.T) { }, }, Expected: azure.AgentPoolSpec{ - + OSType: ptr.To("Linux"), Name: "pool0", SKU: "Standard_D2s_v3", Replicas: 1, @@ -195,6 +197,7 @@ func TestManagedMachinePoolScope_NodeLabels(t *testing.T) { }, }, Expected: azure.AgentPoolSpec{ + OSType: ptr.To("Linux"), Name: "pool1", SKU: "Standard_D2s_v3", Mode: "System", @@ -256,6 +259,7 @@ func TestManagedMachinePoolScope_MaxPods(t *testing.T) { }, }, Expected: azure.AgentPoolSpec{ + OSType: ptr.To("Linux"), Name: "pool0", SKU: "Standard_D2s_v3", Replicas: 1, @@ -288,6 +292,7 @@ func TestManagedMachinePoolScope_MaxPods(t *testing.T) { }, }, Expected: azure.AgentPoolSpec{ + OSType: ptr.To("Linux"), Name: "pool1", SKU: "Standard_D2s_v3", Mode: "System", @@ -347,7 +352,7 @@ func TestManagedMachinePoolScope_Taints(t *testing.T) { }, }, Expected: azure.AgentPoolSpec{ - + OSType: ptr.To("Linux"), Name: "pool0", SKU: "Standard_D2s_v3", Replicas: 1, @@ -386,6 +391,7 @@ func TestManagedMachinePoolScope_Taints(t *testing.T) { }, }, Expected: azure.AgentPoolSpec{ + OSType: ptr.To("Linux"), Name: "pool1", SKU: "Standard_D2s_v3", Mode: "User", @@ -445,6 +451,7 @@ func TestManagedMachinePoolScope_OSDiskType(t *testing.T) { }, }, Expected: azure.AgentPoolSpec{ + OSType: ptr.To("Linux"), Name: "pool0", SKU: "Standard_D2s_v3", Replicas: 1, @@ -477,6 +484,7 @@ func TestManagedMachinePoolScope_OSDiskType(t *testing.T) { }, }, Expected: azure.AgentPoolSpec{ + OSType: ptr.To("Linux"), Name: "pool1", SKU: "Standard_D2s_v3", Mode: "User", @@ -502,6 +510,122 @@ func TestManagedMachinePoolScope_OSDiskType(t *testing.T) { } } +func Test_getManagedMachinePoolVersion(t *testing.T) { + cases := []struct { + Name string + managedControlPlane *infrav1.AzureManagedControlPlane + machinePool *capiv1exp.MachinePool + Expected *string + }{ + { + Name: "Empty configs", + managedControlPlane: nil, + machinePool: nil, + Expected: nil, + }, + { + Name: "Empty mp", + managedControlPlane: &infrav1.AzureManagedControlPlane{}, + machinePool: nil, + Expected: nil, + }, + { + Name: "Only machine pool is available", + managedControlPlane: nil, + machinePool: &capiv1exp.MachinePool{ + Spec: capiv1exp.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.15.0"), + }, + }, + }, + }, + Expected: ptr.To("1.15.0"), + }, + { + Name: "Only machine pool is available and cp is nil", + managedControlPlane: nil, + machinePool: &capiv1exp.MachinePool{ + Spec: capiv1exp.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.15.0"), + }, + }, + }, + }, + Expected: ptr.To("1.15.0"), + }, + { + Name: "mcp.status.autoUpgradeVersion > mp.spec.template.spec.version", + managedControlPlane: &infrav1.AzureManagedControlPlane{ + Status: infrav1.AzureManagedControlPlaneStatus{ + AutoUpgradeVersion: "1.20.3", + }, + }, + machinePool: &capiv1exp.MachinePool{ + Spec: capiv1exp.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.15.0"), + }, + }, + }, + }, + Expected: ptr.To("1.20.3"), + }, + { + Name: "suffix + mcp.status.autoUpgradeVersion > mp.spec.template.spec.version", + managedControlPlane: &infrav1.AzureManagedControlPlane{ + Status: infrav1.AzureManagedControlPlaneStatus{ + AutoUpgradeVersion: "v1.20.3", + }, + }, + machinePool: &capiv1exp.MachinePool{ + Spec: capiv1exp.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.15.0"), + }, + }, + }, + }, + Expected: ptr.To("1.20.3"), + }, + { + Name: "mcp.status.autoUpgradeVersion < mp.spec.template.spec.version", + managedControlPlane: &infrav1.AzureManagedControlPlane{ + Status: infrav1.AzureManagedControlPlaneStatus{ + AutoUpgradeVersion: "v1.20.3", + }, + }, + machinePool: &capiv1exp.MachinePool{ + Spec: capiv1exp.MachinePoolSpec{ + Template: clusterv1.MachineTemplateSpec{ + Spec: clusterv1.MachineSpec{ + Version: ptr.To("v1.21.0"), + }, + }, + }, + }, + Expected: ptr.To("1.21.0"), + }, + } + + for _, c := range cases { + t.Run(c.Name, func(t *testing.T) { + g := NewWithT(t) + v := getManagedMachinePoolVersion(c.managedControlPlane, c.machinePool) + if c.Expected != nil { + g.Expect(*v).To(Equal(*c.Expected)) + } else { + g.Expect(v).To(Equal(c.Expected)) + } + }) + } +} + func getAzureMachinePool(name string, mode infrav1.NodePoolMode) *infrav1.AzureManagedMachinePool { return &infrav1.AzureManagedMachinePool{ ObjectMeta: metav1.ObjectMeta{ diff --git a/azure/services/agentpools/agentpools.go b/azure/services/agentpools/agentpools.go index 8953586c921..4664d80851c 100644 --- a/azure/services/agentpools/agentpools.go +++ b/azure/services/agentpools/agentpools.go @@ -25,11 +25,13 @@ import ( "github.com/Azure/go-autorest/autorest/to" "github.com/google/go-cmp/cmp" "github.com/pkg/errors" + "k8s.io/utils/ptr" infrav1 "sigs.k8s.io/cluster-api-provider-azure/api/v1beta1" "sigs.k8s.io/cluster-api-provider-azure/azure" "sigs.k8s.io/cluster-api-provider-azure/azure/converters" "sigs.k8s.io/cluster-api-provider-azure/util/maps" "sigs.k8s.io/cluster-api-provider-azure/util/tele" + "sigs.k8s.io/cluster-api-provider-azure/util/versions" ) const serviceName = "agentpools" @@ -47,6 +49,7 @@ type ManagedMachinePoolScope interface { UpdateCAPIMachinePoolReplicas(ctx context.Context, replicas *int32) UpdateCAPIMachinePoolAnnotations(ctx context.Context, key, value string) GetCAPIMachinePoolAnnotation(ctx context.Context, key string) string + IsManagedAutoUpgrade() bool } // Service provides operations on Azure resources. @@ -78,7 +81,6 @@ func (s *Service) Reconcile(ctx context.Context) error { agentPoolSpec := s.scope.AgentPoolSpec() profile := converters.AgentPoolToContainerServiceAgentPool(agentPoolSpec) - existingPool, err := s.Client.Get(ctx, agentPoolSpec.ResourceGroup, agentPoolSpec.Cluster, agentPoolSpec.Name) if err != nil && !azure.ResourceNotFound(err) { return errors.Wrap(err, "failed to get existing agent pool") @@ -91,6 +93,9 @@ func (s *Service) Reconcile(ctx context.Context) error { customHeaders := maps.FilterByKeyPrefix(s.scope.AgentPoolAnnotations(), azure.CustomHeaderPrefix) if isCreate := azure.ResourceNotFound(err); isCreate { + if s.scope.IsManagedAutoUpgrade() { + profile.OrchestratorVersion = nil + } err = s.Client.CreateOrUpdate(ctx, agentPoolSpec.ResourceGroup, agentPoolSpec.Cluster, agentPoolSpec.Name, profile, customHeaders) if err != nil && azure.ResourceNotFound(err) { @@ -105,6 +110,14 @@ func (s *Service) Reconcile(ctx context.Context) error { log.V(2).Info(msg) return azure.WithTransientError(errors.New(msg), 20*time.Second) } + // Get the higher version out of the existing and new version + profileOrchestratorVersion, err := versions.GetHigherK8sVersion( + ptr.Deref(existingPool.OrchestratorVersion, ""), + ptr.Deref(profile.OrchestratorVersion, "")) + + if err != nil { + return errors.Wrap(err, "error while calculating k8s version") + } // Normalize individual agent pools to diff in case we need to update existingProfile := containerservice.AgentPool{ @@ -122,7 +135,7 @@ func (s *Service) Reconcile(ctx context.Context) error { normalizedProfile := containerservice.AgentPool{ ManagedClusterAgentPoolProfileProperties: &containerservice.ManagedClusterAgentPoolProfileProperties{ Count: profile.Count, - OrchestratorVersion: profile.OrchestratorVersion, + OrchestratorVersion: ptr.To(profileOrchestratorVersion), Mode: profile.Mode, EnableAutoScaling: profile.EnableAutoScaling, MinCount: profile.MinCount, diff --git a/azure/services/managedclusters/spec.go b/azure/services/managedclusters/spec.go index f8b140b2523..1d62a19b765 100644 --- a/azure/services/managedclusters/spec.go +++ b/azure/services/managedclusters/spec.go @@ -590,13 +590,15 @@ func computeDiffOfNormalizedClusters(managedCluster containerservice.ManagedClus clusterNormalized.AutoUpgradeProfile = &containerservice.ManagedClusterAutoUpgradeProfile{ UpgradeChannel: managedCluster.AutoUpgradeProfile.UpgradeChannel, } - } else { - clusterNormalized.AutoUpgradeProfile = &containerservice.ManagedClusterAutoUpgradeProfile{} } if existingMC.AutoUpgradeProfile != nil { - existingMCClusterNormalized.AutoUpgradeProfile = &containerservice.ManagedClusterAutoUpgradeProfile{ - UpgradeChannel: existingMC.AutoUpgradeProfile.UpgradeChannel, + if existingMC.AutoUpgradeProfile.UpgradeChannel == "" { + existingMC.AutoUpgradeProfile = nil + } else { + existingMCClusterNormalized.AutoUpgradeProfile = &containerservice.ManagedClusterAutoUpgradeProfile{ + UpgradeChannel: existingMC.AutoUpgradeProfile.UpgradeChannel, + } } } if managedCluster.DisableLocalAccounts != nil { diff --git a/exp/api/v1beta1/azuremanagedcontrolplane_webhook.go b/exp/api/v1beta1/azuremanagedcontrolplane_webhook.go index 434b476d520..6d397a37821 100644 --- a/exp/api/v1beta1/azuremanagedcontrolplane_webhook.go +++ b/exp/api/v1beta1/azuremanagedcontrolplane_webhook.go @@ -161,6 +161,16 @@ func (m *AzureManagedControlPlane) ValidateUpdate(oldRaw runtime.Object, client } } + if old.Spec.AutoUpgradeProfile != nil && m.Spec.AutoUpgradeProfile == nil { + // Prevent AutoUpgradeProfile to be set to nil. + // unsetting the field is not allowed + allErrs = append(allErrs, + field.Invalid( + field.NewPath("Spec", "AutoUpgradeProfile"), + m.Spec.AutoUpgradeProfile, + "field is cannot be set to nil, to disable auto upgrades set the channel to none.")) + } + if old.Spec.NetworkPlugin != nil { // Prevent NetworkPlugin modification if it was already set to some value if m.Spec.NetworkPlugin == nil { @@ -252,12 +262,23 @@ func (m *AzureManagedControlPlane) ValidateUpdate(oldRaw runtime.Object, client "DisableLocalAccounts can be set only for AAD enabled clusters")) } - if old.Spec.DisableLocalAccounts != nil && m.Spec.DisableLocalAccounts == nil { - allErrs = append(allErrs, - field.Invalid( - field.NewPath("Spec", "DisableLocalAccounts"), - m.Spec.DisableLocalAccounts, - "field cannot be disabled")) + if old.Spec.DisableLocalAccounts != nil { + // Prevent DisableLocalAccounts modification if it was already set to some value + if m.Spec.DisableLocalAccounts == nil { + // unsetting the field is not allowed + allErrs = append(allErrs, + field.Invalid( + field.NewPath("Spec", "DisableLocalAccounts"), + m.Spec.DisableLocalAccounts, + "field is immutable, unsetting is not allowed")) + } else if *m.Spec.DisableLocalAccounts != *old.Spec.DisableLocalAccounts { + // changing the field is not allowed + allErrs = append(allErrs, + field.Invalid( + field.NewPath("Spec", "DisableLocalAccounts"), + *m.Spec.DisableLocalAccounts, + "field is immutable")) + } } if old.Spec.OutboundType != nil { diff --git a/exp/api/v1beta1/azuremanagedcontrolplane_webhook_test.go b/exp/api/v1beta1/azuremanagedcontrolplane_webhook_test.go index ec08aa9fa0b..c5481550ddf 100644 --- a/exp/api/v1beta1/azuremanagedcontrolplane_webhook_test.go +++ b/exp/api/v1beta1/azuremanagedcontrolplane_webhook_test.go @@ -22,7 +22,7 @@ import ( "github.com/Azure/go-autorest/autorest/to" . "github.com/onsi/gomega" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/utils/pointer" + "k8s.io/utils/ptr" ) func TestDefaultingWebhook(t *testing.T) { @@ -86,7 +86,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Testing inValid DNSPrefix for starting with invalid characters", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("-thisi$"), + DNSPrefix: ptr.To("-thisi$"), Version: "v1.17.8", }, }, @@ -96,7 +96,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Testing inValid DNSPrefix with more then 54 characters", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("thisisaverylong$^clusternameconsistingofmorethan54characterswhichshouldbeinvalid"), + DNSPrefix: ptr.To("thisisaverylong$^clusternameconsistingofmorethan54characterswhichshouldbeinvalid"), Version: "v1.17.8", }, }, @@ -106,7 +106,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Testing inValid DNSPrefix with underscore", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("no_underscore"), + DNSPrefix: ptr.To("no_underscore"), Version: "v1.17.8", }, }, @@ -116,7 +116,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Testing inValid DNSPrefix with special characters", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("no-dollar$@%"), + DNSPrefix: ptr.To("no-dollar$@%"), Version: "v1.17.8", }, }, @@ -126,7 +126,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Testing Valid DNSPrefix with hyphen characters", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("hyphen-allowed"), + DNSPrefix: ptr.To("hyphen-allowed"), Version: "v1.17.8", }, }, @@ -136,7 +136,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Testing Valid DNSPrefix with hyphen characters", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("palette-test07"), + DNSPrefix: ptr.To("palette-test07"), Version: "v1.17.8", }, }, @@ -146,7 +146,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Testing valid DNSPrefix ", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("thisisavlerylongclu7l0sternam3leconsistingofmorethan54"), + DNSPrefix: ptr.To("thisisavlerylongclu7l0sternam3leconsistingofmorethan54"), Version: "v1.17.8", }, }, @@ -156,7 +156,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Testing valid DNSServiceIP", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSServiceIP: pointer.StringPtr("192.168.0.0"), + DNSServiceIP: ptr.To("192.168.0.0"), Version: "v1.17.8", }, }, @@ -166,7 +166,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Testing invalid DNSServiceIP", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSServiceIP: pointer.StringPtr("192.168.0.0.3"), + DNSServiceIP: ptr.To("192.168.0.0.3"), Version: "v1.17.8", }, }, @@ -176,7 +176,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Invalid Version", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSServiceIP: pointer.StringPtr("192.168.0.0"), + DNSServiceIP: ptr.To("192.168.0.0"), Version: "honk", }, }, @@ -186,7 +186,7 @@ func TestValidatingWebhook(t *testing.T) { name: "not following the Kubernetes Version pattern", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSServiceIP: pointer.StringPtr("192.168.0.0"), + DNSServiceIP: ptr.To("192.168.0.0"), Version: "1.19.0", }, }, @@ -196,7 +196,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Version not set", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSServiceIP: pointer.StringPtr("192.168.0.0"), + DNSServiceIP: ptr.To("192.168.0.0"), Version: "", }, }, @@ -206,7 +206,7 @@ func TestValidatingWebhook(t *testing.T) { name: "Valid Version", amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSServiceIP: pointer.StringPtr("192.168.0.0"), + DNSServiceIP: ptr.To("192.168.0.0"), Version: "v1.17.8", }, }, @@ -309,7 +309,7 @@ func TestValidatingWebhook(t *testing.T) { amcp: AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ Version: "v1.21.2", - DisableLocalAccounts: pointer.BoolPtr(true), + DisableLocalAccounts: ptr.To(true), }, }, expectErr: true, @@ -323,7 +323,7 @@ func TestValidatingWebhook(t *testing.T) { Managed: true, AdminGroupObjectIDs: []string{"00000000-0000-0000-0000-000000000000"}, }, - DisableLocalAccounts: pointer.BoolPtr(true), + DisableLocalAccounts: ptr.To(true), }, }, expectErr: false, @@ -988,13 +988,13 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { name: "AzureManagedControlPlane DNSPrefix is immutable", oldAMCP: &AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("capz-aks-1"), + DNSPrefix: ptr.To("capz-aks-1"), Version: "v1.18.0", }, }, amcp: &AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("capz-aks"), + DNSPrefix: ptr.To("capz-aks"), Version: "v1.18.0", }, }, @@ -1004,13 +1004,13 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { name: "AzureManagedControlPlane DNSPrefix is immutable", oldAMCP: &AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("capz-aks"), + DNSPrefix: ptr.To("capz-aks"), Version: "v1.18.0", }, }, amcp: &AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - DNSPrefix: pointer.StringPtr("capz-aks"), + DNSPrefix: ptr.To("capz-aks"), Version: "v1.18.0", }, }, @@ -1020,14 +1020,14 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { name: "AzureManagedControlPlane FqdnSubdomain is immutable", oldAMCP: &AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - FqdnSubdomain: pointer.StringPtr("capzaks.api"), + FqdnSubdomain: ptr.To("capzaks.api"), Version: "v1.18.0", }, }, amcp: &AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ Version: "v1.18.0", - FqdnSubdomain: pointer.StringPtr("capzaks.com"), + FqdnSubdomain: ptr.To("capzaks.com"), }, }, wantErr: true, @@ -1036,14 +1036,14 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { name: "AzureManagedControlPlane FqdnSubdomain is immutable", oldAMCP: &AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ - FqdnSubdomain: pointer.StringPtr("capzaks.api"), + FqdnSubdomain: ptr.To("capzaks.api"), Version: "v1.18.0", }, }, amcp: &AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ Version: "v1.18.0", - FqdnSubdomain: pointer.StringPtr("capzaks.api"), + FqdnSubdomain: ptr.To("capzaks.api"), }, }, wantErr: false, @@ -1053,7 +1053,7 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { oldAMCP: &AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ APIServerAccessProfile: &APIServerAccessProfile{ - EnablePrivateCluster: pointer.Bool(true), + EnablePrivateCluster: ptr.To(true), }, Version: "v1.18.0", }, @@ -1061,8 +1061,8 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { amcp: &AzureManagedControlPlane{ Spec: AzureManagedControlPlaneSpec{ APIServerAccessProfile: &APIServerAccessProfile{ - EnablePrivateCluster: pointer.Bool(true), - PrivateDNSZone: pointer.StringPtr("None"), + EnablePrivateCluster: ptr.To(true), + PrivateDNSZone: ptr.To("None"), }, Version: "v1.18.0", }, @@ -1075,8 +1075,8 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { Spec: AzureManagedControlPlaneSpec{ Version: "v1.18.0", APIServerAccessProfile: &APIServerAccessProfile{ - EnablePrivateCluster: pointer.Bool(true), - PrivateDNSZone: pointer.StringPtr("example-resource-id"), + EnablePrivateCluster: ptr.To(true), + PrivateDNSZone: ptr.To("example-resource-id"), }, }, }, @@ -1084,8 +1084,8 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { Spec: AzureManagedControlPlaneSpec{ Version: "v1.18.0", APIServerAccessProfile: &APIServerAccessProfile{ - EnablePrivateCluster: pointer.Bool(true), - PrivateDNSZone: pointer.StringPtr("None"), + EnablePrivateCluster: ptr.To(true), + PrivateDNSZone: ptr.To("None"), }, }, }, @@ -1097,8 +1097,8 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { Spec: AzureManagedControlPlaneSpec{ Version: "v1.18.0", APIServerAccessProfile: &APIServerAccessProfile{ - EnablePrivateCluster: pointer.Bool(true), - PrivateDNSZone: pointer.StringPtr("example-resource-id"), + EnablePrivateCluster: ptr.To(true), + PrivateDNSZone: ptr.To("example-resource-id"), }, }, }, @@ -1106,7 +1106,7 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { Spec: AzureManagedControlPlaneSpec{ Version: "v1.18.0", APIServerAccessProfile: &APIServerAccessProfile{ - EnablePrivateCluster: pointer.Bool(true), + EnablePrivateCluster: ptr.To(true), }, }, }, @@ -1118,8 +1118,8 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { Spec: AzureManagedControlPlaneSpec{ Version: "v1.18.0", APIServerAccessProfile: &APIServerAccessProfile{ - EnablePrivateCluster: pointer.Bool(true), - PrivateDNSZone: pointer.StringPtr("example-resource-id"), + EnablePrivateCluster: ptr.To(true), + PrivateDNSZone: ptr.To("example-resource-id"), }, }, }, @@ -1127,8 +1127,8 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { Spec: AzureManagedControlPlaneSpec{ Version: "v1.18.0", APIServerAccessProfile: &APIServerAccessProfile{ - EnablePrivateCluster: pointer.Bool(true), - PrivateDNSZone: pointer.StringPtr("example-resource-id-1"), + EnablePrivateCluster: ptr.To(true), + PrivateDNSZone: ptr.To("example-resource-id-1"), }, }, }, @@ -1198,7 +1198,7 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { }, Spec: AzureManagedControlPlaneSpec{ Version: "v1.18.0", - DisableLocalAccounts: pointer.BoolPtr(true), + DisableLocalAccounts: ptr.To(true), AADProfile: &AADProfile{ Managed: true, AdminGroupObjectIDs: []string{"00000000-0000-0000-0000-000000000000"}, @@ -1223,7 +1223,7 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { }, Spec: AzureManagedControlPlaneSpec{ Version: "v1.18.0", - DisableLocalAccounts: pointer.BoolPtr(true), + DisableLocalAccounts: ptr.To(true), }, }, wantErr: true, @@ -1240,7 +1240,7 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { Managed: true, AdminGroupObjectIDs: []string{"00000000-0000-0000-0000-000000000000"}, }, - DisableLocalAccounts: pointer.BoolPtr(true), + DisableLocalAccounts: ptr.To(true), }, }, amcp: &AzureManagedControlPlane{ @@ -1257,6 +1257,111 @@ func TestAzureManagedControlPlane_ValidateUpdate(t *testing.T) { }, wantErr: true, }, + { + name: "DisableLocalAccounts cannot change the value AAD clusters", + oldAMCP: &AzureManagedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + Spec: AzureManagedControlPlaneSpec{ + Version: "v1.18.0", + AADProfile: &AADProfile{ + Managed: true, + AdminGroupObjectIDs: []string{"00000000-0000-0000-0000-000000000000"}, + }, + DisableLocalAccounts: ptr.To(true), + }, + }, + amcp: &AzureManagedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + Spec: AzureManagedControlPlaneSpec{ + Version: "v1.18.0", + AADProfile: &AADProfile{ + Managed: true, + AdminGroupObjectIDs: []string{"00000000-0000-0000-0000-000000000000"}, + }, + DisableLocalAccounts: ptr.To(false), + }, + }, + wantErr: true, + }, + { + name: "Auto upgrade profile cannot be disabled", + oldAMCP: &AzureManagedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + Spec: AzureManagedControlPlaneSpec{ + Version: "v1.18.0", + AutoUpgradeProfile: &ManagedClusterAutoUpgradeProfile{ + UpgradeChannel: UpgradeChannelNone, + }, + }, + }, + amcp: &AzureManagedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + Spec: AzureManagedControlPlaneSpec{ + Version: "v1.18.0", + }, + }, + wantErr: true, + }, + { + name: "Auto upgrade profile channel can be updated", + oldAMCP: &AzureManagedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + Spec: AzureManagedControlPlaneSpec{ + Version: "v1.18.0", + AutoUpgradeProfile: &ManagedClusterAutoUpgradeProfile{ + UpgradeChannel: UpgradeChannelPatch, + }, + }, + }, + amcp: &AzureManagedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + Spec: AzureManagedControlPlaneSpec{ + Version: "v1.18.0", + AutoUpgradeProfile: &ManagedClusterAutoUpgradeProfile{ + UpgradeChannel: UpgradeChannelStable, + }, + }, + }, + wantErr: false, + }, + { + name: "Auto upgrade profile channel can remain same", + oldAMCP: &AzureManagedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + Spec: AzureManagedControlPlaneSpec{ + Version: "v1.18.0", + AutoUpgradeProfile: &ManagedClusterAutoUpgradeProfile{ + UpgradeChannel: UpgradeChannelPatch, + }, + }, + }, + amcp: &AzureManagedControlPlane{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-cluster", + }, + Spec: AzureManagedControlPlaneSpec{ + Version: "v1.18.0", + AutoUpgradeProfile: &ManagedClusterAutoUpgradeProfile{ + UpgradeChannel: UpgradeChannelPatch, + }, + }, + }, + wantErr: false, + }, { name: "OutboundType update", oldAMCP: &AzureManagedControlPlane{ diff --git a/util/versions/version.go b/util/versions/version.go index 0b83c8acec0..4646c3c8cfe 100644 --- a/util/versions/version.go +++ b/util/versions/version.go @@ -5,16 +5,20 @@ import ( "github.com/pkg/errors" ) -// GetHigherK8sVersion returns the higher k8s version out of a and b +// GetHigherK8sVersion returns the higher k8s version out of a and b. func GetHigherK8sVersion(a, b string) (string, error) { - v1, err := semverv4.ParseTolerant(a) - if err != nil { - return "", errors.Wrap(err, "error parsing k8s version") + v1, errv1 := semverv4.ParseTolerant(a) + v2, errv2 := semverv4.ParseTolerant(b) + if errv1 != nil && errv2 != nil { + return "", errors.Wrapf(errv1, "error parsing k8s version %s, %v error parsing k8s version %s", a, errv2, b) } - v2, err := semverv4.ParseTolerant(b) - if err != nil { - return "", errors.Wrap(err, "error parsing k8s version") + if errv1 != nil { + return b, nil } + if errv2 != nil { + return a, nil + } + if v1.GTE(v2) { return a, nil } diff --git a/util/versions/version_test.go b/util/versions/version_test.go index e18991176bb..738b39e6ba4 100644 --- a/util/versions/version_test.go +++ b/util/versions/version_test.go @@ -60,13 +60,27 @@ func TestGetHigherK8sVersion(t *testing.T) { name: "a is invalid", a: "1.18.", b: "v1.17.8", - output: "", - expectErr: true, + output: "v1.17.8", + expectErr: false, }, { name: "b is invalid", a: "1.18.1", b: "v1.17.8.", + output: "1.18.1", + expectErr: false, + }, + { + name: "b is invalid", + a: "9.99.9999", + b: "v1.17.8.", + output: "9.99.9999", + expectErr: false, + }, + { + name: "a & b is invalid", + a: "", + b: "v1.17.8.", output: "", expectErr: true, },