Skip to content

Commit 248b31f

Browse files
sircthulhusircthulhu
committed
add percentage PDB validation and rewrite tests
1 parent a3107c9 commit 248b31f

File tree

2 files changed

+172
-109
lines changed

2 files changed

+172
-109
lines changed

‎api/v1alpha1/etcdcluster_webhook.go

+64-30
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,9 @@ limitations under the License.
1717
package v1alpha1
1818

1919
import (
20+
"fmt"
21+
"math"
22+
2023
corev1 "k8s.io/api/core/v1"
2124
"k8s.io/apimachinery/pkg/api/errors"
2225
"k8s.io/apimachinery/pkg/api/resource"
@@ -130,51 +133,82 @@ func (r *EtcdCluster) validatePdb() (admission.Warnings, field.ErrorList) {
130133
pdb := r.Spec.PodDisruptionBudget
131134
var warnings admission.Warnings
132135
var allErrors field.ErrorList
136+
133137
if pdb.Spec.MinAvailable != nil && pdb.Spec.MaxUnavailable != nil {
134138
allErrors = append(allErrors, field.Invalid(
135139
field.NewPath("spec", "podDisruptionBudget", "minAvailable"),
136140
pdb.Spec.MinAvailable.IntValue(),
137141
"minAvailable is mutually exclusive with maxUnavailable"),
138142
)
139-
} else {
140-
minQuorumSize := r.CalculateQuorumSize()
141-
if pdb.Spec.MinAvailable != nil {
142-
if pdb.Spec.MinAvailable.IntValue() < 0 {
143-
allErrors = append(allErrors, field.Invalid(
144-
field.NewPath("spec", "podDisruptionBudget", "minAvailable"),
145-
pdb.Spec.MinAvailable.IntValue(),
146-
"value cannot be less than zero"),
147-
)
148-
}
149-
if pdb.Spec.MinAvailable.IntValue() > int(*r.Spec.Replicas) {
143+
return nil, allErrors
144+
}
145+
146+
minQuorumSize := r.CalculateQuorumSize()
147+
if pdb.Spec.MinAvailable != nil {
148+
minAvailable := pdb.Spec.MinAvailable.IntValue()
149+
if pdb.Spec.MinAvailable.IntVal == 0 && pdb.Spec.MinAvailable.IntValue() == 0 {
150+
var percentage int
151+
_, err := fmt.Sscanf(pdb.Spec.MinAvailable.StrVal, "%d%%", &percentage)
152+
if err != nil {
150153
allErrors = append(allErrors, field.Invalid(
151154
field.NewPath("spec", "podDisruptionBudget", "minAvailable"),
152-
pdb.Spec.MinAvailable.IntValue(),
153-
"value cannot be larger than number of replicas"),
155+
pdb.Spec.MinAvailable.StrVal,
156+
"invalid percentage value"),
154157
)
155-
}
156-
if pdb.Spec.MinAvailable.IntValue() < minQuorumSize {
157-
warnings = append(warnings, "current number of spec.podDisruptionBudget.minAvailable can lead to loss of quorum")
158+
} else {
159+
minAvailable = int(math.Ceil(float64(*r.Spec.Replicas) * (float64(percentage) / 100)))
158160
}
159161
}
160-
if pdb.Spec.MaxUnavailable != nil {
161-
if pdb.Spec.MaxUnavailable.IntValue() < 0 {
162-
allErrors = append(allErrors, field.Invalid(
163-
field.NewPath("spec", "podDisruptionBudget", "maxUnavailable"),
164-
pdb.Spec.MaxUnavailable.IntValue(),
165-
"value cannot be less than zero"),
166-
)
167-
}
168-
if pdb.Spec.MaxUnavailable.IntValue() > int(*r.Spec.Replicas) {
162+
163+
if minAvailable < 0 {
164+
allErrors = append(allErrors, field.Invalid(
165+
field.NewPath("spec", "podDisruptionBudget", "minAvailable"),
166+
pdb.Spec.MinAvailable.IntValue(),
167+
"value cannot be less than zero"),
168+
)
169+
}
170+
if minAvailable > int(*r.Spec.Replicas) {
171+
allErrors = append(allErrors, field.Invalid(
172+
field.NewPath("spec", "podDisruptionBudget", "minAvailable"),
173+
pdb.Spec.MinAvailable.IntValue(),
174+
"value cannot be larger than number of replicas"),
175+
)
176+
}
177+
if minAvailable < minQuorumSize {
178+
warnings = append(warnings, "current number of spec.podDisruptionBudget.minAvailable can lead to loss of quorum")
179+
}
180+
}
181+
if pdb.Spec.MaxUnavailable != nil {
182+
maxUnavailable := pdb.Spec.MaxUnavailable.IntValue()
183+
if pdb.Spec.MaxUnavailable.IntVal == 0 && pdb.Spec.MaxUnavailable.IntValue() == 0 {
184+
var percentage int
185+
_, err := fmt.Sscanf(pdb.Spec.MaxUnavailable.StrVal, "%d%%", &percentage)
186+
if err != nil {
169187
allErrors = append(allErrors, field.Invalid(
170188
field.NewPath("spec", "podDisruptionBudget", "maxUnavailable"),
171-
pdb.Spec.MaxUnavailable.IntValue(),
172-
"value cannot be larger than number of replicas"),
189+
pdb.Spec.MaxUnavailable.StrVal,
190+
"invalid percentage value"),
173191
)
192+
} else {
193+
maxUnavailable = int(math.Ceil(float64(*r.Spec.Replicas) * (float64(percentage) / 100)))
174194
}
175-
if int(*r.Spec.Replicas)-pdb.Spec.MaxUnavailable.IntValue() < minQuorumSize {
176-
warnings = append(warnings, "current number of spec.podDisruptionBudget.maxUnavailable can lead to loss of quorum")
177-
}
195+
}
196+
if maxUnavailable < 0 {
197+
allErrors = append(allErrors, field.Invalid(
198+
field.NewPath("spec", "podDisruptionBudget", "maxUnavailable"),
199+
pdb.Spec.MaxUnavailable.IntValue(),
200+
"value cannot be less than zero"),
201+
)
202+
}
203+
if maxUnavailable > int(*r.Spec.Replicas) {
204+
allErrors = append(allErrors, field.Invalid(
205+
field.NewPath("spec", "podDisruptionBudget", "maxUnavailable"),
206+
pdb.Spec.MaxUnavailable.IntValue(),
207+
"value cannot be larger than number of replicas"),
208+
)
209+
}
210+
if int(*r.Spec.Replicas)-maxUnavailable < minQuorumSize {
211+
warnings = append(warnings, "current number of spec.podDisruptionBudget.maxUnavailable can lead to loss of quorum")
178212
}
179213
}
180214

‎api/v1alpha1/etcdcluster_webhook_test.go

+108-79
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"k8s.io/apimachinery/pkg/api/errors"
2424
"k8s.io/apimachinery/pkg/api/resource"
2525
"k8s.io/apimachinery/pkg/util/intstr"
26+
"k8s.io/apimachinery/pkg/util/validation/field"
2627
"k8s.io/utils/ptr"
2728
)
2829

@@ -92,85 +93,6 @@ var _ = Describe("EtcdCluster Webhook", func() {
9293
Expect(err).To(Succeed())
9394
Expect(w).To(BeEmpty())
9495
})
95-
It("Should admit enabled empty PDB", func() {
96-
etcdCluster := &EtcdCluster{
97-
Spec: EtcdClusterSpec{
98-
Replicas: ptr.To(int32(1)),
99-
PodDisruptionBudget: &EmbeddedPodDisruptionBudget{},
100-
},
101-
}
102-
w, err := etcdCluster.ValidateCreate()
103-
Expect(err).To(Succeed())
104-
Expect(w).To(BeEmpty())
105-
})
106-
It("Should reject if negative spec.podDisruptionBudget.minAvailable", func() {
107-
etcdCluster := &EtcdCluster{
108-
Spec: EtcdClusterSpec{
109-
Replicas: ptr.To(int32(1)),
110-
PodDisruptionBudget: &EmbeddedPodDisruptionBudget{
111-
Spec: PodDisruptionBudgetSpec{
112-
MinAvailable: ptr.To(intstr.FromInt32(int32(-1))),
113-
},
114-
},
115-
},
116-
}
117-
_, err := etcdCluster.ValidateCreate()
118-
if Expect(err).To(HaveOccurred()) {
119-
statusErr := err.(*errors.StatusError)
120-
Expect(statusErr.ErrStatus.Message).To(ContainSubstring("value cannot be less than zero"))
121-
}
122-
})
123-
It("Should reject if negative spec.podDisruptionBudget.maxUnavailable", func() {
124-
etcdCluster := &EtcdCluster{
125-
Spec: EtcdClusterSpec{
126-
Replicas: ptr.To(int32(1)),
127-
PodDisruptionBudget: &EmbeddedPodDisruptionBudget{
128-
Spec: PodDisruptionBudgetSpec{
129-
MaxUnavailable: ptr.To(intstr.FromInt32(int32(-1))),
130-
},
131-
},
132-
},
133-
}
134-
_, err := etcdCluster.ValidateCreate()
135-
if Expect(err).To(HaveOccurred()) {
136-
statusErr := err.(*errors.StatusError)
137-
Expect(statusErr.ErrStatus.Message).To(ContainSubstring("maxUnavailable: Invalid value: -1: value cannot be less than zero"))
138-
}
139-
})
140-
It("Should reject if min available field larger than replicas", func() {
141-
etcdCluster := &EtcdCluster{
142-
Spec: EtcdClusterSpec{
143-
Replicas: ptr.To(int32(1)),
144-
PodDisruptionBudget: &EmbeddedPodDisruptionBudget{
145-
Spec: PodDisruptionBudgetSpec{
146-
MinAvailable: ptr.To(intstr.FromInt32(int32(2))),
147-
},
148-
},
149-
},
150-
}
151-
_, err := etcdCluster.ValidateCreate()
152-
if Expect(err).To(HaveOccurred()) {
153-
statusErr := err.(*errors.StatusError)
154-
Expect(statusErr.ErrStatus.Message).To(ContainSubstring("minAvailable: Invalid value: 2: value cannot be larger than number of replicas"))
155-
}
156-
})
157-
It("Should reject if max unavailable field larger than replicas", func() {
158-
etcdCluster := &EtcdCluster{
159-
Spec: EtcdClusterSpec{
160-
Replicas: ptr.To(int32(1)),
161-
PodDisruptionBudget: &EmbeddedPodDisruptionBudget{
162-
Spec: PodDisruptionBudgetSpec{
163-
MaxUnavailable: ptr.To(intstr.FromInt32(int32(2))),
164-
},
165-
},
166-
},
167-
}
168-
_, err := etcdCluster.ValidateCreate()
169-
if Expect(err).To(HaveOccurred()) {
170-
statusErr := err.(*errors.StatusError)
171-
Expect(statusErr.ErrStatus.Message).To(ContainSubstring("maxUnavailable: Invalid value: 2: value cannot be larger than number of replicas"))
172-
}
173-
})
17496
})
17597

17698
Context("When updating EtcdCluster under Validating Webhook", func() {
@@ -211,4 +133,111 @@ var _ = Describe("EtcdCluster Webhook", func() {
211133
Expect(err).To(Succeed())
212134
})
213135
})
136+
137+
Context("Validate PDB", func() {
138+
etcdCluster := &EtcdCluster{
139+
Spec: EtcdClusterSpec{
140+
Replicas: ptr.To(int32(3)),
141+
PodDisruptionBudget: &EmbeddedPodDisruptionBudget{},
142+
},
143+
}
144+
It("Should admit enabled empty PDB", func() {
145+
localCluster := etcdCluster.DeepCopy()
146+
w, err := localCluster.validatePdb()
147+
Expect(err).To(BeNil())
148+
Expect(w).To(BeEmpty())
149+
})
150+
It("Should reject if negative spec.podDisruptionBudget.minAvailable", func() {
151+
localCluster := etcdCluster.DeepCopy()
152+
localCluster.Spec.PodDisruptionBudget.Spec.MinAvailable = ptr.To(intstr.FromInt32(int32(-1)))
153+
_, err := localCluster.validatePdb()
154+
if Expect(err).NotTo(BeNil()) {
155+
expectedFieldErr := field.Invalid(
156+
field.NewPath("spec", "podDisruptionBudget", "minAvailable"),
157+
-1,
158+
"value cannot be less than zero",
159+
)
160+
if Expect(err).To(HaveLen(1)) {
161+
Expect(*(err[0])).To(Equal(*expectedFieldErr))
162+
}
163+
}
164+
})
165+
It("Should reject if negative spec.podDisruptionBudget.maxUnavailable", func() {
166+
localCluster := etcdCluster.DeepCopy()
167+
localCluster.Spec.PodDisruptionBudget.Spec.MaxUnavailable = ptr.To(intstr.FromInt32(int32(-1)))
168+
_, err := localCluster.validatePdb()
169+
if Expect(err).NotTo(BeNil()) {
170+
expectedFieldErr := field.Invalid(
171+
field.NewPath("spec", "podDisruptionBudget", "maxUnavailable"),
172+
-1,
173+
"value cannot be less than zero",
174+
)
175+
if Expect(err).To(HaveLen(1)) {
176+
Expect(*(err[0])).To(Equal(*expectedFieldErr))
177+
}
178+
}
179+
})
180+
It("Should reject if min available field larger than replicas", func() {
181+
localCluster := etcdCluster.DeepCopy()
182+
localCluster.Spec.Replicas = ptr.To(int32(1))
183+
localCluster.Spec.PodDisruptionBudget.Spec.MinAvailable = ptr.To(intstr.FromInt32(int32(2)))
184+
_, err := localCluster.validatePdb()
185+
if Expect(err).NotTo(BeNil()) {
186+
expectedFieldErr := field.Invalid(
187+
field.NewPath("spec", "podDisruptionBudget", "minAvailable"),
188+
2,
189+
"value cannot be larger than number of replicas",
190+
)
191+
if Expect(err).To(HaveLen(1)) {
192+
Expect(*(err[0])).To(Equal(*expectedFieldErr))
193+
}
194+
}
195+
})
196+
It("Should reject if max unavailable field larger than replicas", func() {
197+
localCluster := etcdCluster.DeepCopy()
198+
localCluster.Spec.Replicas = ptr.To(int32(1))
199+
localCluster.Spec.PodDisruptionBudget.Spec.MaxUnavailable = ptr.To(intstr.FromInt32(int32(2)))
200+
_, err := localCluster.validatePdb()
201+
if Expect(err).NotTo(BeNil()) {
202+
expectedFieldErr := field.Invalid(
203+
field.NewPath("spec", "podDisruptionBudget", "maxUnavailable"),
204+
2,
205+
"value cannot be larger than number of replicas",
206+
)
207+
if Expect(err).To(HaveLen(1)) {
208+
Expect(*(err[0])).To(Equal(*expectedFieldErr))
209+
}
210+
}
211+
})
212+
It("should accept correct percentage value for minAvailable", func() {
213+
localCluster := etcdCluster.DeepCopy()
214+
localCluster.Spec.Replicas = ptr.To(int32(4))
215+
localCluster.Spec.PodDisruptionBudget.Spec.MinAvailable = ptr.To(intstr.FromString("50%"))
216+
warnings, err := localCluster.validatePdb()
217+
Expect(err).To(BeNil())
218+
Expect(warnings).To(ContainElement("current number of spec.podDisruptionBudget.minAvailable can lead to loss of quorum"))
219+
})
220+
It("should accept correct percentage value for maxUnavailable", func() {
221+
localCluster := etcdCluster.DeepCopy()
222+
localCluster.Spec.PodDisruptionBudget.Spec.MaxUnavailable = ptr.To(intstr.FromString("50%"))
223+
warnings, err := localCluster.validatePdb()
224+
Expect(err).To(BeNil())
225+
Expect(warnings).To(ContainElement("current number of spec.podDisruptionBudget.maxUnavailable can lead to loss of quorum"))
226+
})
227+
It("Should reject incorrect value for maxUnavailable", func() {
228+
localCluster := etcdCluster.DeepCopy()
229+
localCluster.Spec.PodDisruptionBudget.Spec.MaxUnavailable = ptr.To(intstr.FromString("50$"))
230+
_, err := localCluster.validatePdb()
231+
if Expect(err).NotTo(BeNil()) {
232+
expectedFieldErr := field.Invalid(
233+
field.NewPath("spec", "podDisruptionBudget", "maxUnavailable"),
234+
"50$",
235+
"invalid percentage value",
236+
)
237+
if Expect(err).To(HaveLen(1)) {
238+
Expect(*(err[0])).To(Equal(*expectedFieldErr))
239+
}
240+
}
241+
})
242+
})
214243
})

0 commit comments

Comments
 (0)