Replace maxUnavailable auto-correction with validation that fails NNCP when value is 0

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) || (self.type == 0 && self.intVal > 0) || (self.type == 1 && int(self.strVal.replace('%', '')) > 0)",message="maxUnavailable must be greater than 0"
 	MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"`
 }
 

bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml

@@ -70,6 +70,8 @@ spec:
                 description: |-
                   MaxUnavailable specifies percentage or number
                   of machines that can be updating at a time. Default is "50%".
+                  The computed value must result in at least 1 node being allowed to undergo updates at a time.
+                  If set to 0 or a percentage that rounds to 0, the policy will fail with FailedToConfigure.
                 x-kubernetes-int-or-string: true
               nodeSelector:
                 additionalProperties:
@@ -190,6 +192,8 @@ spec:
                 description: |-
                   MaxUnavailable specifies percentage or number
                   of machines that can be updating at a time. Default is "50%".
+                  The computed value must result in at least 1 node being allowed to undergo updates at a time.
+                  If set to 0 or a percentage that rounds to 0, the policy will fail with FailedToConfigure.
                 x-kubernetes-int-or-string: true
               nodeSelector:
                 additionalProperties:

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,

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{},
 			}

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) || (self.type == 0 && self.intVal > 0) || (self.type
+                    == 1 && int(self.strVal.replace(''%'', '''')) > 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) || (self.type == 0 && self.intVal > 0) || (self.type
+                    == 1 && int(self.strVal.replace(''%'', '''')) > 0)'
               nodeSelector:
                 additionalProperties:
                   type: string

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

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()),
 			}))
 })

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,30 @@ 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() {
+		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"))
+		})
+	})
 })

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 {

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())