Skip to content

Commit e1b4e4f

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #12467 from sbueringer/pr-allow-kcpt-transition
🐛 Allow transition of KubeadmControlPlaneTemplate from defaulted rolloutStrategy to unset
2 parents 05b0d2d + ad850d3 commit e1b4e4f

File tree

7 files changed

+72
-37
lines changed

7 files changed

+72
-37
lines changed

‎controlplane/kubeadm/internal/webhooks/kubeadm_control_plane.go

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -88,23 +88,19 @@ func defaultKubeadmControlPlaneSpec(s *controlplanev1.KubeadmControlPlaneSpec) {
8888
}
8989

9090
func defaultRolloutStrategy(rolloutStrategy *controlplanev1.RolloutStrategy) *controlplanev1.RolloutStrategy {
91-
ios1 := intstr.FromInt(1)
92-
9391
if rolloutStrategy == nil {
9492
rolloutStrategy = &controlplanev1.RolloutStrategy{}
9593
}
9694

9795
// Enforce RollingUpdate strategy and default MaxSurge if not set.
98-
if rolloutStrategy != nil {
99-
if len(rolloutStrategy.Type) == 0 {
100-
rolloutStrategy.Type = controlplanev1.RollingUpdateStrategyType
101-
}
102-
if rolloutStrategy.Type == controlplanev1.RollingUpdateStrategyType {
103-
if rolloutStrategy.RollingUpdate == nil {
104-
rolloutStrategy.RollingUpdate = &controlplanev1.RollingUpdate{}
105-
}
106-
rolloutStrategy.RollingUpdate.MaxSurge = intstr.ValueOrDefault(rolloutStrategy.RollingUpdate.MaxSurge, ios1)
96+
if len(rolloutStrategy.Type) == 0 {
97+
rolloutStrategy.Type = controlplanev1.RollingUpdateStrategyType
98+
}
99+
if rolloutStrategy.Type == controlplanev1.RollingUpdateStrategyType {
100+
if rolloutStrategy.RollingUpdate == nil {
101+
rolloutStrategy.RollingUpdate = &controlplanev1.RollingUpdate{}
107102
}
103+
rolloutStrategy.RollingUpdate.MaxSurge = intstr.ValueOrDefault(rolloutStrategy.RollingUpdate.MaxSurge, intstr.FromInt32(1))
108104
}
109105

110106
return rolloutStrategy
@@ -422,8 +418,8 @@ func validateRolloutStrategy(rolloutStrategy *controlplanev1.RolloutStrategy, re
422418
)
423419
}
424420

425-
ios1 := intstr.FromInt(1)
426-
ios0 := intstr.FromInt(0)
421+
ios1 := intstr.FromInt32(1)
422+
ios0 := intstr.FromInt32(0)
427423

428424
if rolloutStrategy.RollingUpdate.MaxSurge.IntValue() == ios0.IntValue() && (replicas != nil && *replicas < int32(3)) {
429425
allErrs = append(

‎controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate.go

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222

2323
apierrors "k8s.io/apimachinery/pkg/api/errors"
2424
"k8s.io/apimachinery/pkg/runtime"
25+
"k8s.io/apimachinery/pkg/util/intstr"
2526
"k8s.io/apimachinery/pkg/util/validation/field"
2627
ctrl "sigs.k8s.io/controller-runtime"
2728
"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -111,6 +112,19 @@ func (webhook *KubeadmControlPlaneTemplate) ValidateUpdate(ctx context.Context,
111112
return nil, apierrors.NewBadRequest(fmt.Sprintf("failed to compare old and new KubeadmControlPlaneTemplate: failed to default new object: %v", err))
112113
}
113114

115+
// In Cluster API < v1.11 the RolloutStrategy field was defaulted.
116+
// The defaulting was dropped with Cluster API v1.11.
117+
// To ensure users can still apply their KubeadmControlPlaneTemplate over pre-existing KubeadmControlPlaneTemplate
118+
// without setting rolloutStrategy we allow transitions from the old default value to unset.
119+
if newK.Spec.Template.Spec.RolloutStrategy == nil &&
120+
oldK.Spec.Template.Spec.RolloutStrategy != nil &&
121+
oldK.Spec.Template.Spec.RolloutStrategy.Type == controlplanev1.RollingUpdateStrategyType &&
122+
oldK.Spec.Template.Spec.RolloutStrategy.RollingUpdate != nil &&
123+
oldK.Spec.Template.Spec.RolloutStrategy.RollingUpdate.MaxSurge != nil &&
124+
*oldK.Spec.Template.Spec.RolloutStrategy.RollingUpdate.MaxSurge == intstr.FromInt32(1) {
125+
newK.Spec.Template.Spec.RolloutStrategy = oldK.Spec.Template.Spec.RolloutStrategy
126+
}
127+
114128
equal, diff, err := compare.Diff(oldK.Spec.Template.Spec, newK.Spec.Template.Spec)
115129
if err != nil {
116130
return nil, apierrors.NewBadRequest(fmt.Sprintf("failed to compare old and new KubeadmControlPlaneTemplate: %v", err))

‎controlplane/kubeadm/internal/webhooks/kubeadmcontrolplanetemplate_test.go

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222

2323
. "github.com/onsi/gomega"
2424
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
25+
"k8s.io/apimachinery/pkg/util/intstr"
2526
utilfeature "k8s.io/component-base/featuregate/testing"
2627
"k8s.io/utils/ptr"
2728

@@ -227,4 +228,34 @@ func TestKubeadmControlPlaneTemplateUpdateValidation(t *testing.T) {
227228
g.Expect(err.Error()).To(ContainSubstring("KubeadmControlPlaneTemplate spec.template.spec field is immutable"))
228229
g.Expect(warnings).To(BeEmpty())
229230
})
231+
t.Run("update KubeadmControlPlaneTemplate should pass when transitioning from the previously defaulted rolloutStrategy to an unset rolloutStrategy", func(t *testing.T) {
232+
g := NewWithT(t)
233+
oldKCPTemplate := &controlplanev1.KubeadmControlPlaneTemplate{
234+
Spec: controlplanev1.KubeadmControlPlaneTemplateSpec{
235+
Template: controlplanev1.KubeadmControlPlaneTemplateResource{
236+
Spec: controlplanev1.KubeadmControlPlaneTemplateResourceSpec{
237+
RolloutStrategy: &controlplanev1.RolloutStrategy{
238+
Type: controlplanev1.RollingUpdateStrategyType,
239+
RollingUpdate: &controlplanev1.RollingUpdate{
240+
MaxSurge: ptr.To(intstr.FromInt32(1)),
241+
},
242+
},
243+
},
244+
},
245+
},
246+
}
247+
newKCPTemplate := &controlplanev1.KubeadmControlPlaneTemplate{
248+
Spec: controlplanev1.KubeadmControlPlaneTemplateSpec{
249+
Template: controlplanev1.KubeadmControlPlaneTemplateResource{
250+
Spec: controlplanev1.KubeadmControlPlaneTemplateResourceSpec{
251+
RolloutStrategy: nil,
252+
},
253+
},
254+
},
255+
}
256+
webhook := &KubeadmControlPlaneTemplate{}
257+
warnings, err := webhook.ValidateUpdate(ctx, oldKCPTemplate, newKCPTemplate)
258+
g.Expect(err).ToNot(HaveOccurred())
259+
g.Expect(warnings).To(BeEmpty())
260+
})
230261
}

‎internal/controllers/machinedeployment/mdutil/util_test.go

Lines changed: 12 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -719,7 +719,7 @@ func TestNewMSNewReplicas(t *testing.T) {
719719
strategyType clusterv1.MachineDeploymentStrategyType
720720
depReplicas int32
721721
newMSReplicas int32
722-
maxSurge int
722+
maxSurge int32
723723
expected int32
724724
}{
725725
{
@@ -745,14 +745,8 @@ func TestNewMSNewReplicas(t *testing.T) {
745745
*(newDeployment.Spec.Replicas) = test.depReplicas
746746
newDeployment.Spec.Strategy = &clusterv1.MachineDeploymentStrategy{Type: test.strategyType}
747747
newDeployment.Spec.Strategy.RollingUpdate = &clusterv1.MachineRollingUpdateDeployment{
748-
MaxUnavailable: func(i int) *intstr.IntOrString {
749-
x := intstr.FromInt(i)
750-
return &x
751-
}(1),
752-
MaxSurge: func(i int) *intstr.IntOrString {
753-
x := intstr.FromInt(i)
754-
return &x
755-
}(test.maxSurge),
748+
MaxUnavailable: ptr.To(intstr.FromInt32(1)),
749+
MaxSurge: ptr.To(intstr.FromInt32(test.maxSurge)),
756750
}
757751
*(newRC.Spec.Replicas) = test.newMSReplicas
758752
ms, err := NewMSNewReplicas(&newDeployment, []*clusterv1.MachineSet{&rs5}, *newRC.Spec.Replicas)
@@ -769,8 +763,8 @@ func TestDeploymentComplete(t *testing.T) {
769763
Replicas: &desired,
770764
Strategy: &clusterv1.MachineDeploymentStrategy{
771765
RollingUpdate: &clusterv1.MachineRollingUpdateDeployment{
772-
MaxUnavailable: func(i int) *intstr.IntOrString { x := intstr.FromInt(i); return &x }(int(maxUnavailable)),
773-
MaxSurge: func(i int) *intstr.IntOrString { x := intstr.FromInt(i); return &x }(int(maxSurge)),
766+
MaxUnavailable: ptr.To(intstr.FromInt32(maxUnavailable)),
767+
MaxSurge: ptr.To(intstr.FromInt32(maxSurge)),
774768
},
775769
Type: clusterv1.RollingUpdateMachineDeploymentStrategyType,
776770
},
@@ -847,7 +841,7 @@ func TestMaxUnavailable(t *testing.T) {
847841
Replicas: func(i int32) *int32 { return &i }(replicas),
848842
Strategy: &clusterv1.MachineDeploymentStrategy{
849843
RollingUpdate: &clusterv1.MachineRollingUpdateDeployment{
850-
MaxSurge: func(i int) *intstr.IntOrString { x := intstr.FromInt(i); return &x }(int(1)),
844+
MaxSurge: ptr.To(intstr.FromInt32(1)),
851845
MaxUnavailable: &maxUnavailable,
852846
},
853847
Type: clusterv1.RollingUpdateMachineDeploymentStrategyType,
@@ -862,22 +856,22 @@ func TestMaxUnavailable(t *testing.T) {
862856
}{
863857
{
864858
name: "maxUnavailable less than replicas",
865-
deployment: deployment(10, intstr.FromInt(5)),
859+
deployment: deployment(10, intstr.FromInt32(5)),
866860
expected: int32(5),
867861
},
868862
{
869863
name: "maxUnavailable equal replicas",
870-
deployment: deployment(10, intstr.FromInt(10)),
864+
deployment: deployment(10, intstr.FromInt32(10)),
871865
expected: int32(10),
872866
},
873867
{
874868
name: "maxUnavailable greater than replicas",
875-
deployment: deployment(5, intstr.FromInt(10)),
869+
deployment: deployment(5, intstr.FromInt32(10)),
876870
expected: int32(5),
877871
},
878872
{
879873
name: "maxUnavailable with replicas is 0",
880-
deployment: deployment(0, intstr.FromInt(10)),
874+
deployment: deployment(0, intstr.FromInt32(10)),
881875
expected: int32(0),
882876
},
883877
{
@@ -938,8 +932,8 @@ func TestAnnotationUtils(t *testing.T) {
938932
func TestComputeMachineSetAnnotations(t *testing.T) {
939933
deployment := generateDeployment("nginx")
940934
deployment.Spec.Replicas = ptr.To[int32](3)
941-
maxSurge := intstr.FromInt(1)
942-
maxUnavailable := intstr.FromInt(0)
935+
maxSurge := intstr.FromInt32(1)
936+
maxUnavailable := intstr.FromInt32(0)
943937
deployment.Spec.Strategy = &clusterv1.MachineDeploymentStrategy{
944938
Type: clusterv1.RollingUpdateMachineDeploymentStrategyType,
945939
RollingUpdate: &clusterv1.MachineRollingUpdateDeployment{

‎internal/controllers/machinedeployment/suite_test.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ func TestMain(m *testing.M) {
148148

149149
func intOrStrPtr(i int32) *intstr.IntOrString {
150150
// FromInt takes an int that must not be greater than int32...
151-
res := intstr.FromInt(int(i))
151+
res := intstr.FromInt32(i)
152152
return &res
153153
}
154154

‎internal/webhooks/machinedeployment.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,11 +122,11 @@ func (webhook *MachineDeployment) Default(ctx context.Context, obj runtime.Objec
122122
m.Spec.Strategy.RollingUpdate = &clusterv1.MachineRollingUpdateDeployment{}
123123
}
124124
if m.Spec.Strategy.RollingUpdate.MaxSurge == nil {
125-
ios1 := intstr.FromInt(1)
125+
ios1 := intstr.FromInt32(1)
126126
m.Spec.Strategy.RollingUpdate.MaxSurge = &ios1
127127
}
128128
if m.Spec.Strategy.RollingUpdate.MaxUnavailable == nil {
129-
ios0 := intstr.FromInt(0)
129+
ios0 := intstr.FromInt32(0)
130130
m.Spec.Strategy.RollingUpdate.MaxUnavailable = &ios0
131131
}
132132
}

‎internal/webhooks/machinedeployment_test.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -354,9 +354,9 @@ func TestMachineDeploymentValidation(t *testing.T) {
354354
goodMaxUnavailablePercentage := intstr.FromString("0%")
355355
goodMaxInFlightPercentage := intstr.FromString("20%")
356356

357-
goodMaxSurgeInt := intstr.FromInt(1)
358-
goodMaxUnavailableInt := intstr.FromInt(0)
359-
goodMaxInFlightInt := intstr.FromInt(5)
357+
goodMaxSurgeInt := intstr.FromInt32(1)
358+
goodMaxUnavailableInt := intstr.FromInt32(0)
359+
goodMaxInFlightInt := intstr.FromInt32(5)
360360
tests := []struct {
361361
name string
362362
md *clusterv1.MachineDeployment

0 commit comments

Comments
 (0)