diff --git a/api/shared/nodenetworkconfigurationpolicy_types.go b/api/shared/nodenetworkconfigurationpolicy_types.go index 211381006..7a9a4655e 100644 --- a/api/shared/nodenetworkconfigurationpolicy_types.go +++ b/api/shared/nodenetworkconfigurationpolicy_types.go @@ -46,7 +46,10 @@ type NodeNetworkConfigurationPolicySpec struct { // MaxUnavailable specifies percentage or number // of machines that can be updating at a time. Default is "50%". + // Must be greater than 0. When set as a percentage, it must be a positive percentage. // +optional + //nolint:lll + // +kubebuilder:validation:XValidation:rule="!has(self) || (type(self) == int && self > 0) || (type(self) == string && self != '0' && self != '0%')",message="maxUnavailable must be greater than 0" MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` } diff --git a/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml b/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml index 7d42f96f4..0b42d61d4 100644 --- a/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml @@ -70,7 +70,12 @@ spec: description: |- MaxUnavailable specifies percentage or number of machines that can be updating at a time. Default is "50%". + Must be greater than 0. When set as a percentage, it must be a positive percentage. x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxUnavailable must be greater than 0 + rule: '!has(self) || (type(self) == int && self > 0) || (type(self) + == string && self != ''0'' && self != ''0%'')' nodeSelector: additionalProperties: type: string @@ -190,7 +195,12 @@ spec: description: |- MaxUnavailable specifies percentage or number of machines that can be updating at a time. Default is "50%". + Must be greater than 0. When set as a percentage, it must be a positive percentage. x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxUnavailable must be greater than 0 + rule: '!has(self) || (type(self) == int && self > 0) || (type(self) + == string && self != ''0'' && self != ''0%'')' nodeSelector: additionalProperties: type: string diff --git a/controllers/handler/nodenetworkconfigurationpolicy_controller.go b/controllers/handler/nodenetworkconfigurationpolicy_controller.go index 1f94e6c99..8c17bb7b0 100644 --- a/controllers/handler/nodenetworkconfigurationpolicy_controller.go +++ b/controllers/handler/nodenetworkconfigurationpolicy_controller.go @@ -682,6 +682,7 @@ func (r *NodeNetworkConfigurationPolicyReconciler) shouldAbortReconcile( logger.Info("Error getting max unavailable count") return false, err } + filter := enactmentconditions.LogicalConditionCountFilter{ nmstateapi.NodeNetworkConfigurationEnactmentConditionFailing: corev1.ConditionTrue, nmstateapi.NodeNetworkConfigurationEnactmentConditionProgressing: corev1.ConditionFalse, diff --git a/controllers/handler/nodenetworkconfigurationpolicy_controller_test.go b/controllers/handler/nodenetworkconfigurationpolicy_controller_test.go index 1d831518b..e7af2de2f 100644 --- a/controllers/handler/nodenetworkconfigurationpolicy_controller_test.go +++ b/controllers/handler/nodenetworkconfigurationpolicy_controller_test.go @@ -133,6 +133,9 @@ var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func() nnce := nmstatev1beta1.NodeNetworkConfigurationEnactment{ ObjectMeta: metav1.ObjectMeta{ Name: shared.EnactmentKey(nodeName, nncp.Name).Name, + Labels: map[string]string{ + shared.EnactmentPolicyLabel: nncp.Name, + }, }, Status: shared.NodeNetworkConfigurationEnactmentStatus{}, } diff --git a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml index 94378b12c..c2cd83a25 100644 --- a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml @@ -70,7 +70,12 @@ spec: description: |- MaxUnavailable specifies percentage or number of machines that can be updating at a time. Default is "50%". + Must be greater than 0. When set as a percentage, it must be a positive percentage. x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxUnavailable must be greater than 0 + rule: '!has(self) || (type(self) == int && self > 0) || (type(self) + == string && self != ''0'' && self != ''0%'')' nodeSelector: additionalProperties: type: string @@ -190,7 +195,12 @@ spec: description: |- MaxUnavailable specifies percentage or number of machines that can be updating at a time. Default is "50%". + Must be greater than 0. When set as a percentage, it must be a positive percentage. x-kubernetes-int-or-string: true + x-kubernetes-validations: + - message: maxUnavailable must be greater than 0 + rule: '!has(self) || (type(self) == int && self > 0) || (type(self) + == string && self != ''0'' && self != ''0%'')' nodeSelector: additionalProperties: type: string diff --git a/pkg/node/nodes.go b/pkg/node/nodes.go index d2f7b69f8..243593e4b 100644 --- a/pkg/node/nodes.go +++ b/pkg/node/nodes.go @@ -108,12 +108,6 @@ func MaxUnavailableNodeCount(ctx context.Context, cli client.Reader, policy *nms } func ScaledMaxUnavailableNodeCount(matchingNodes int, intOrPercent intstr.IntOrString) (int, error) { - correctMaxUnavailable := func(maxUnavailable int) int { - if maxUnavailable < 1 { - return MinMaxunavailable - } - return maxUnavailable - } maxUnavailable, err := intstr.GetScaledValueFromIntOrPercent(&intOrPercent, matchingNodes, true) if err != nil { defaultMaxUnavailable := intstr.FromString(DefaultMaxunavailable) @@ -122,9 +116,9 @@ func ScaledMaxUnavailableNodeCount(matchingNodes int, intOrPercent intstr.IntOrS matchingNodes, true, ) - return correctMaxUnavailable(maxUnavailable), err + return maxUnavailable, err } - return correctMaxUnavailable(maxUnavailable), nil + return maxUnavailable, nil } // Return true if the event name is the name of diff --git a/pkg/node/nodes_test.go b/pkg/node/nodes_test.go index 7fcb6a80f..64131bca7 100644 --- a/pkg/node/nodes_test.go +++ b/pkg/node/nodes_test.go @@ -32,6 +32,8 @@ var _ = Describe("MaxUnavailable nodes", func() { expectedScaledMaxUnavailable int expectedError types.GomegaMatcher } + // Note: ValidateMaxUnavailable is tested at the controller integration test level + // since it requires a full client context with policies and enactments DescribeTable("testing ScaledMaxUnavailableNodeCount", func(c maxUnavailableCase) { maxUnavailable, err := ScaledMaxUnavailableNodeCount(c.nmstateEnabledNodes, c.maxUnavailable) @@ -84,14 +86,14 @@ var _ = Describe("MaxUnavailable nodes", func() { maxUnavailableCase{ nmstateEnabledNodes: 5, maxUnavailable: intstr.FromString("0%"), - expectedScaledMaxUnavailable: 1, + expectedScaledMaxUnavailable: 0, expectedError: Not(HaveOccurred()), }), Entry("Zero value", maxUnavailableCase{ nmstateEnabledNodes: 5, maxUnavailable: intstr.FromInt(0), - expectedScaledMaxUnavailable: 1, + expectedScaledMaxUnavailable: 0, expectedError: Not(HaveOccurred()), })) }) diff --git a/test/e2e/handler/nnce_conditions_test.go b/test/e2e/handler/nnce_conditions_test.go index 1b0967ed2..40b9b9f9b 100644 --- a/test/e2e/handler/nnce_conditions_test.go +++ b/test/e2e/handler/nnce_conditions_test.go @@ -23,6 +23,7 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" + "k8s.io/apimachinery/pkg/util/intstr" nmstate "github.com/nmstate/kubernetes-nmstate/api/shared" enactmentconditions "github.com/nmstate/kubernetes-nmstate/pkg/enactmentstatus/conditions" @@ -208,4 +209,35 @@ var _ = Describe("EnactmentCondition", func() { }, 2*time.Second, 100*time.Millisecond).Should(policyconditions.ContainPolicyDegraded(), "policy should be marked as Degraded") }) }) + + Context("when maxUnavailable configuration is invalid", func() { + BeforeEach(func() { + By("Ensure policy doesn't exist") + deletePolicy(TestPolicy) + }) + + It("should be rejected by API validation when maxUnavailable is 0", func() { + By("Attempt to apply policy with maxUnavailable: 0") + err := setDesiredStateWithPolicyMaxUnavailableAndNodeSelector( + TestPolicy, + linuxBrUp(bridge1), + intstr.FromInt(0), + map[string]string{"node-role.kubernetes.io/worker": ""}, + ) + Expect(err).To(HaveOccurred(), "API should reject policy with maxUnavailable: 0") + Expect(err.Error()).To(ContainSubstring("maxUnavailable must be greater than 0")) + }) + + It("should be rejected by API validation when maxUnavailable is 0%", func() { + By("Attempt to apply policy with maxUnavailable: 0%") + err := setDesiredStateWithPolicyMaxUnavailableAndNodeSelector( + TestPolicy, + linuxBrUp(bridge1), + intstr.FromString("0%"), + map[string]string{"node-role.kubernetes.io/worker": ""}, + ) + Expect(err).To(HaveOccurred(), "API should reject policy with maxUnavailable: 0%") + Expect(err.Error()).To(ContainSubstring("maxUnavailable must be greater than 0")) + }) + }) }) diff --git a/test/e2e/handler/nodes_test.go b/test/e2e/handler/nodes_test.go index 5330ffad1..f28272345 100644 --- a/test/e2e/handler/nodes_test.go +++ b/test/e2e/handler/nodes_test.go @@ -35,7 +35,7 @@ var _ = Describe("Nodes", func() { }) Context("and node network state is deleted", func() { BeforeEach(func() { - deleteNodeNeworkStates() + deleteNodeNetworkStates() }) It("should recreate it with currentState", func() { for _, node := range nodes { diff --git a/test/e2e/handler/utils.go b/test/e2e/handler/utils.go index 46a8550f6..7ca54d6ed 100644 --- a/test/e2e/handler/utils.go +++ b/test/e2e/handler/utils.go @@ -165,6 +165,28 @@ func setDesiredStateWithPolicyAndCapture(name string, desiredState nmstate.State setDesiredStateWithPolicyAndCaptureAndNodeSelectorEventually(name, desiredState, capture, runAtWorkers) } +func setDesiredStateWithPolicyMaxUnavailableAndNodeSelector( + name string, + desiredState nmstate.State, + maxUnavailableValue intstr.IntOrString, + nodeSelector map[string]string, +) error { + policy := nmstatev1.NodeNetworkConfigurationPolicy{} + policy.Name = name + key := types.NamespacedName{Name: name} + err := testenv.Client.Get(context.TODO(), key, &policy) + policy.Spec.DesiredState = desiredState + policy.Spec.NodeSelector = nodeSelector + policy.Spec.MaxUnavailable = &maxUnavailableValue + if err != nil { + if apierrors.IsNotFound(err) { + return testenv.Client.Create(context.TODO(), &policy) + } + return err + } + return testenv.Client.Update(context.TODO(), &policy) +} + // nodeMatchesSelector checks if a node's labels match the given selector. // An empty selector matches all nodes. func nodeMatchesSelector(nodeName string, nodeSelector map[string]string) bool { @@ -279,7 +301,7 @@ func nodeNetworkConfigurationPolicy(policyName string) nmstatev1.NodeNetworkConf return policy } -func deleteNodeNeworkStates() { +func deleteNodeNetworkStates() { nodeNetworkStateList := &nmstatev1beta1.NodeNetworkStateList{} err := testenv.Client.List(context.TODO(), nodeNetworkStateList, &dynclient.ListOptions{}) Expect(err).ToNot(HaveOccurred())