From 9257efdd04845f34ee96ac3d3802dba6902d8b45 Mon Sep 17 00:00:00 2001 From: Emilia Desch Date: Thu, 9 Jul 2026 15:54:16 +0200 Subject: [PATCH 1/9] Replace maxUnavailable auto-correction with validation that fails NNCP when value is 0 Signed-off-by: Emilia Desch --- .../nodenetworkconfigurationpolicy_types.go | 2 ++ ...denetworkconfigurationpolicy_controller.go | 23 +++++++++++++++++++ ...workconfigurationpolicy_controller_test.go | 3 +++ pkg/node/nodes.go | 10 ++------ pkg/node/nodes_test.go | 6 +++-- 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/api/shared/nodenetworkconfigurationpolicy_types.go b/api/shared/nodenetworkconfigurationpolicy_types.go index 2113810069..511d7ed913 100644 --- a/api/shared/nodenetworkconfigurationpolicy_types.go +++ b/api/shared/nodenetworkconfigurationpolicy_types.go @@ -46,6 +46,8 @@ type NodeNetworkConfigurationPolicySpec struct { // 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 available for updates. + // If set to 0 or a percentage that rounds to 0, the policy will fail with FailedToConfigure. // +optional MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` } diff --git a/controllers/handler/nodenetworkconfigurationpolicy_controller.go b/controllers/handler/nodenetworkconfigurationpolicy_controller.go index 1f94e6c997..e2be315b9a 100644 --- a/controllers/handler/nodenetworkconfigurationpolicy_controller.go +++ b/controllers/handler/nodenetworkconfigurationpolicy_controller.go @@ -591,6 +591,17 @@ func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount ) } + if maxUnavailable < 1 { + errMsg := fmt.Sprintf("invalid maxUnavailable configuration: computed value is %d. The maxUnavailable field must result in at least 1 node. Either increase the value (e.g., set to 1 or higher) or use a higher percentage (e.g., \"1%%\")", maxUnavailable) + r.Log.Error(errors.New(errMsg), "Invalid maxUnavailable configuration") + policyconditions.SetPolicyFailedToConfigure(&policy.Status.Conditions, errMsg) + updateErr := r.Client.Status().Update(ctx, policy) + if updateErr != nil { + return updateErr + } + return errors.New(errMsg) + } + if policy.Status.UnavailableNodeCountMap == nil { policy.Status.UnavailableNodeCountMap = map[string]int{} } @@ -682,6 +693,18 @@ func (r *NodeNetworkConfigurationPolicyReconciler) shouldAbortReconcile( logger.Info("Error getting max unavailable count") return false, err } + + if maxUnavailable < 1 { + errMsg := fmt.Sprintf("invalid maxUnavailable configuration: computed value is %d. The maxUnavailable field must result in at least 1 node. Either increase the value (e.g., set to 1 or higher) or use a higher percentage (e.g., \"1%%\")", maxUnavailable) + logger.Error(errors.New(errMsg), "Invalid maxUnavailable configuration") + policyconditions.SetPolicyFailedToConfigure(&instance.Status.Conditions, errMsg) + updateErr := r.Client.Status().Update(ctx, instance) + if updateErr != nil { + return false, updateErr + } + return false, errors.New(errMsg) + } + 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 1d831518b6..e7af2de2f0 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/pkg/node/nodes.go b/pkg/node/nodes.go index d2f7b69f8e..243593e4b2 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 7fcb6a80f4..64131bca76 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()), })) }) From 2e947fe38d62b8a09d0c42db5efd38ed04f0edcf Mon Sep 17 00:00:00 2001 From: Emilia Desch Date: Thu, 9 Jul 2026 16:57:18 +0200 Subject: [PATCH 2/9] Fix infinite requeue and redundant retries with custom InvalidMaxUnavailableError type Signed-off-by: Emilia Desch --- .../nodenetworkconfigurationpolicy_types.go | 2 +- ...denetworkconfigurationpolicy_controller.go | 33 +++++++++++++++---- ...e.io_nodenetworkconfigurationpolicies.yaml | 4 +++ pkg/node/nodes.go | 6 ++++ 4 files changed, 37 insertions(+), 8 deletions(-) diff --git a/api/shared/nodenetworkconfigurationpolicy_types.go b/api/shared/nodenetworkconfigurationpolicy_types.go index 511d7ed913..107f7299de 100644 --- a/api/shared/nodenetworkconfigurationpolicy_types.go +++ b/api/shared/nodenetworkconfigurationpolicy_types.go @@ -46,7 +46,7 @@ type NodeNetworkConfigurationPolicySpec struct { // 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 available for updates. + // 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. // +optional MaxUnavailable *intstr.IntOrString `json:"maxUnavailable,omitempty"` diff --git a/controllers/handler/nodenetworkconfigurationpolicy_controller.go b/controllers/handler/nodenetworkconfigurationpolicy_controller.go index e2be315b9a..7ded074d2f 100644 --- a/controllers/handler/nodenetworkconfigurationpolicy_controller.go +++ b/controllers/handler/nodenetworkconfigurationpolicy_controller.go @@ -231,11 +231,20 @@ func (r *NodeNetworkConfigurationPolicyReconciler) Reconcile(ctx context.Context if r.shouldIncrementUnavailableNodeCount(previousConditions) { err = r.incrementUnavailableNodeCount(ctx, instance, generationKey) if err != nil { + if errors.Is(err, node.InvalidMaxUnavailableError{}) { + // Policy status already set to FailedToConfigure in incrementUnavailableNodeCount + log.Error(err, "Invalid maxUnavailable configuration, stopping reconciliation") + return ctrl.Result{}, nil + } if apierrors.IsConflict(err) || errors.Is(err, node.MaxUnavailableLimitReachedError{}) { enactmentConditions.NotifyPending(ctx) log.Info(err.Error()) shouldAbortEnactment, err := r.shouldAbortReconcile(ctx, instance) if err != nil { + if errors.Is(err, node.InvalidMaxUnavailableError{}) { + // Policy status already set, stop reconciliation + return ctrl.Result{}, nil + } return ctrl.Result{}, err } if shouldAbortEnactment { @@ -579,7 +588,9 @@ func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount policy *nmstatev1.NodeNetworkConfigurationPolicy, generationKey string) error { policyKey := types.NamespacedName{Name: policy.GetName(), Namespace: policy.GetNamespace()} - return retry.OnError(retry.DefaultRetry, func(error) bool { return true }, func() error { + return retry.OnError(retry.DefaultRetry, func(err error) bool { + return !errors.Is(err, node.InvalidMaxUnavailableError{}) + }, func() error { err := r.Get(ctx, policyKey, policy) if err != nil { return err @@ -592,14 +603,18 @@ func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount } if maxUnavailable < 1 { - errMsg := fmt.Sprintf("invalid maxUnavailable configuration: computed value is %d. The maxUnavailable field must result in at least 1 node. Either increase the value (e.g., set to 1 or higher) or use a higher percentage (e.g., \"1%%\")", maxUnavailable) - r.Log.Error(errors.New(errMsg), "Invalid maxUnavailable configuration") + errMsg := fmt.Sprintf( + "invalid maxUnavailable configuration: computed value is %d. "+ + "The maxUnavailable field must result in at least 1 node. "+ + "Either increase the value (e.g., set to 1 or higher) or use a higher percentage (e.g., \"1%%\")", + maxUnavailable) + r.Log.Error(node.InvalidMaxUnavailableError{}, "Invalid maxUnavailable configuration") policyconditions.SetPolicyFailedToConfigure(&policy.Status.Conditions, errMsg) updateErr := r.Client.Status().Update(ctx, policy) if updateErr != nil { return updateErr } - return errors.New(errMsg) + return node.InvalidMaxUnavailableError{} } if policy.Status.UnavailableNodeCountMap == nil { @@ -695,14 +710,18 @@ func (r *NodeNetworkConfigurationPolicyReconciler) shouldAbortReconcile( } if maxUnavailable < 1 { - errMsg := fmt.Sprintf("invalid maxUnavailable configuration: computed value is %d. The maxUnavailable field must result in at least 1 node. Either increase the value (e.g., set to 1 or higher) or use a higher percentage (e.g., \"1%%\")", maxUnavailable) - logger.Error(errors.New(errMsg), "Invalid maxUnavailable configuration") + errMsg := fmt.Sprintf( + "invalid maxUnavailable configuration: computed value is %d. "+ + "The maxUnavailable field must result in at least 1 node. "+ + "Either increase the value (e.g., set to 1 or higher) or use a higher percentage (e.g., \"1%%\")", + maxUnavailable) + logger.Error(node.InvalidMaxUnavailableError{}, "Invalid maxUnavailable configuration") policyconditions.SetPolicyFailedToConfigure(&instance.Status.Conditions, errMsg) updateErr := r.Client.Status().Update(ctx, instance) if updateErr != nil { return false, updateErr } - return false, errors.New(errMsg) + return false, node.InvalidMaxUnavailableError{} } filter := enactmentconditions.LogicalConditionCountFilter{ diff --git a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml index 94378b12c5..1df387aee8 100644 --- a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/deploy/crds/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: diff --git a/pkg/node/nodes.go b/pkg/node/nodes.go index 243593e4b2..8853867478 100644 --- a/pkg/node/nodes.go +++ b/pkg/node/nodes.go @@ -43,6 +43,12 @@ func (f MaxUnavailableLimitReachedError) Error() string { return "maximal number of nodes are already processing policy configuration" } +type InvalidMaxUnavailableError struct{} + +func (e InvalidMaxUnavailableError) Error() string { + return "invalid maxUnavailable configuration: computed value is 0" +} + func NodesRunningNmstate(ctx context.Context, cli client.Reader, nodeSelector map[string]string) ([]corev1.Node, error) { nodes := corev1.NodeList{} err := cli.List(ctx, &nodes, client.MatchingLabels(nodeSelector)) From 6c65e009858fae1ac4969567caf940ea29b082d7 Mon Sep 17 00:00:00 2001 From: Emilia Desch Date: Thu, 9 Jul 2026 17:15:09 +0200 Subject: [PATCH 3/9] Update bundle manifests with new maxUnavailable documentation Signed-off-by: Emilia Desch --- .../nmstate.io_nodenetworkconfigurationpolicies.yaml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml b/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml index 7d42f96f47..ac9e66e91c 100644 --- a/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/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: From 480e072efa3b3620a137ad4166c574b2b90744dc Mon Sep 17 00:00:00 2001 From: Emilia Desch Date: Tue, 28 Jul 2026 14:55:46 +0200 Subject: [PATCH 4/9] Replace controller-based maxUnavailable validation with CEL validation Signed-off-by: Emilia Desch --- .../nodenetworkconfigurationpolicy_types.go | 5 ++- ...denetworkconfigurationpolicy_controller.go | 43 +------------------ ...e.io_nodenetworkconfigurationpolicies.yaml | 14 ++++-- pkg/node/nodes.go | 6 --- test/e2e/handler/nnce_conditions_test.go | 27 ++++++++++++ test/e2e/handler/nodes_test.go | 2 +- test/e2e/handler/utils.go | 24 ++++++++++- 7 files changed, 65 insertions(+), 56 deletions(-) diff --git a/api/shared/nodenetworkconfigurationpolicy_types.go b/api/shared/nodenetworkconfigurationpolicy_types.go index 107f7299de..95c2bb9f0c 100644 --- a/api/shared/nodenetworkconfigurationpolicy_types.go +++ b/api/shared/nodenetworkconfigurationpolicy_types.go @@ -46,9 +46,10 @@ type NodeNetworkConfigurationPolicySpec struct { // 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. + // 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"` } diff --git a/controllers/handler/nodenetworkconfigurationpolicy_controller.go b/controllers/handler/nodenetworkconfigurationpolicy_controller.go index 7ded074d2f..8c17bb7b02 100644 --- a/controllers/handler/nodenetworkconfigurationpolicy_controller.go +++ b/controllers/handler/nodenetworkconfigurationpolicy_controller.go @@ -231,20 +231,11 @@ func (r *NodeNetworkConfigurationPolicyReconciler) Reconcile(ctx context.Context if r.shouldIncrementUnavailableNodeCount(previousConditions) { err = r.incrementUnavailableNodeCount(ctx, instance, generationKey) if err != nil { - if errors.Is(err, node.InvalidMaxUnavailableError{}) { - // Policy status already set to FailedToConfigure in incrementUnavailableNodeCount - log.Error(err, "Invalid maxUnavailable configuration, stopping reconciliation") - return ctrl.Result{}, nil - } if apierrors.IsConflict(err) || errors.Is(err, node.MaxUnavailableLimitReachedError{}) { enactmentConditions.NotifyPending(ctx) log.Info(err.Error()) shouldAbortEnactment, err := r.shouldAbortReconcile(ctx, instance) if err != nil { - if errors.Is(err, node.InvalidMaxUnavailableError{}) { - // Policy status already set, stop reconciliation - return ctrl.Result{}, nil - } return ctrl.Result{}, err } if shouldAbortEnactment { @@ -588,9 +579,7 @@ func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount policy *nmstatev1.NodeNetworkConfigurationPolicy, generationKey string) error { policyKey := types.NamespacedName{Name: policy.GetName(), Namespace: policy.GetNamespace()} - return retry.OnError(retry.DefaultRetry, func(err error) bool { - return !errors.Is(err, node.InvalidMaxUnavailableError{}) - }, func() error { + return retry.OnError(retry.DefaultRetry, func(error) bool { return true }, func() error { err := r.Get(ctx, policyKey, policy) if err != nil { return err @@ -602,21 +591,6 @@ func (r *NodeNetworkConfigurationPolicyReconciler) incrementUnavailableNodeCount ) } - if maxUnavailable < 1 { - errMsg := fmt.Sprintf( - "invalid maxUnavailable configuration: computed value is %d. "+ - "The maxUnavailable field must result in at least 1 node. "+ - "Either increase the value (e.g., set to 1 or higher) or use a higher percentage (e.g., \"1%%\")", - maxUnavailable) - r.Log.Error(node.InvalidMaxUnavailableError{}, "Invalid maxUnavailable configuration") - policyconditions.SetPolicyFailedToConfigure(&policy.Status.Conditions, errMsg) - updateErr := r.Client.Status().Update(ctx, policy) - if updateErr != nil { - return updateErr - } - return node.InvalidMaxUnavailableError{} - } - if policy.Status.UnavailableNodeCountMap == nil { policy.Status.UnavailableNodeCountMap = map[string]int{} } @@ -709,21 +683,6 @@ func (r *NodeNetworkConfigurationPolicyReconciler) shouldAbortReconcile( return false, err } - if maxUnavailable < 1 { - errMsg := fmt.Sprintf( - "invalid maxUnavailable configuration: computed value is %d. "+ - "The maxUnavailable field must result in at least 1 node. "+ - "Either increase the value (e.g., set to 1 or higher) or use a higher percentage (e.g., \"1%%\")", - maxUnavailable) - logger.Error(node.InvalidMaxUnavailableError{}, "Invalid maxUnavailable configuration") - policyconditions.SetPolicyFailedToConfigure(&instance.Status.Conditions, errMsg) - updateErr := r.Client.Status().Update(ctx, instance) - if updateErr != nil { - return false, updateErr - } - return false, node.InvalidMaxUnavailableError{} - } - filter := enactmentconditions.LogicalConditionCountFilter{ nmstateapi.NodeNetworkConfigurationEnactmentConditionFailing: corev1.ConditionTrue, nmstateapi.NodeNetworkConfigurationEnactmentConditionProgressing: corev1.ConditionFalse, diff --git a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml index 1df387aee8..7ee08f63bc 100644 --- a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml @@ -70,9 +70,12 @@ 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. + 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 @@ -192,9 +195,12 @@ 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. + 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 diff --git a/pkg/node/nodes.go b/pkg/node/nodes.go index 8853867478..243593e4b2 100644 --- a/pkg/node/nodes.go +++ b/pkg/node/nodes.go @@ -43,12 +43,6 @@ func (f MaxUnavailableLimitReachedError) Error() string { return "maximal number of nodes are already processing policy configuration" } -type InvalidMaxUnavailableError struct{} - -func (e InvalidMaxUnavailableError) Error() string { - return "invalid maxUnavailable configuration: computed value is 0" -} - func NodesRunningNmstate(ctx context.Context, cli client.Reader, nodeSelector map[string]string) ([]corev1.Node, error) { nodes := corev1.NodeList{} err := cli.List(ctx, &nodes, client.MatchingLabels(nodeSelector)) diff --git a/test/e2e/handler/nnce_conditions_test.go b/test/e2e/handler/nnce_conditions_test.go index 1b0967ed20..d4bb2fbbc1 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,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")) + }) + }) }) diff --git a/test/e2e/handler/nodes_test.go b/test/e2e/handler/nodes_test.go index 5330ffad1b..f282723459 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 46a8550f65..7ca54d6ed7 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()) From 1438a1519d65ab9a6f9bd72efddbc09af6dc4bdf Mon Sep 17 00:00:00 2001 From: Emilia Desch Date: Tue, 28 Jul 2026 15:00:52 +0200 Subject: [PATCH 5/9] regenerate bundle files Signed-off-by: Emilia Desch --- ...mstate.io_nodenetworkconfigurationpolicies.yaml | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml b/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml index ac9e66e91c..f438e41c72 100644 --- a/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml @@ -70,9 +70,12 @@ 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. + 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 @@ -192,9 +195,12 @@ 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. + 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 From b3e7b40722e386af641cc9077540ded834e76c39 Mon Sep 17 00:00:00 2001 From: Emilia Desch Date: Wed, 29 Jul 2026 12:36:17 +0200 Subject: [PATCH 6/9] Added BeforeEach hook Signed-off-by: Emilia Desch --- test/e2e/handler/nnce_conditions_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/e2e/handler/nnce_conditions_test.go b/test/e2e/handler/nnce_conditions_test.go index d4bb2fbbc1..40b9b9f9b8 100644 --- a/test/e2e/handler/nnce_conditions_test.go +++ b/test/e2e/handler/nnce_conditions_test.go @@ -211,6 +211,11 @@ var _ = Describe("EnactmentCondition", func() { }) 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( From fdf68caf9bff3f4ae7d15ebf552b214e730ef1c2 Mon Sep 17 00:00:00 2001 From: Emilia Desch Date: Wed, 29 Jul 2026 13:31:37 +0200 Subject: [PATCH 7/9] Fix CEL validation rule to treat maxUnavailable IntOrString as scalar value Signed-off-by: Emilia Desch --- api/shared/nodenetworkconfigurationpolicy_types.go | 2 +- .../nmstate.io_nodenetworkconfigurationpolicies.yaml | 8 ++++---- .../crds/nmstate.io_nodenetworkconfigurationpolicies.yaml | 8 ++++---- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/api/shared/nodenetworkconfigurationpolicy_types.go b/api/shared/nodenetworkconfigurationpolicy_types.go index 95c2bb9f0c..c2be4f3a21 100644 --- a/api/shared/nodenetworkconfigurationpolicy_types.go +++ b/api/shared/nodenetworkconfigurationpolicy_types.go @@ -49,7 +49,7 @@ type NodeNetworkConfigurationPolicySpec struct { // 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" + // +kubebuilder:validation:XValidation:rule="!has(self) || (type(self) == int && self > 0) || (type(self) == string && int(self.replace('%', '')) > 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 f438e41c72..64e9089417 100644 --- a/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml @@ -74,8 +74,8 @@ spec: 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)' + rule: '!has(self) || (type(self) == int && self > 0) || (type(self) + == string && int(self.replace(''%'', '''')) > 0)' nodeSelector: additionalProperties: type: string @@ -199,8 +199,8 @@ spec: 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)' + rule: '!has(self) || (type(self) == int && self > 0) || (type(self) + == string && int(self.replace(''%'', '''')) > 0)' nodeSelector: additionalProperties: type: string diff --git a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml index 7ee08f63bc..8d3ff32491 100644 --- a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml @@ -74,8 +74,8 @@ spec: 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)' + rule: '!has(self) || (type(self) == int && self > 0) || (type(self) + == string && int(self.replace(''%'', '''')) > 0)' nodeSelector: additionalProperties: type: string @@ -199,8 +199,8 @@ spec: 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)' + rule: '!has(self) || (type(self) == int && self > 0) || (type(self) + == string && int(self.replace(''%'', '''')) > 0)' nodeSelector: additionalProperties: type: string From 26ab7085df04ed873ab21ccb43adc3acf6f2d485 Mon Sep 17 00:00:00 2001 From: Emilia Desch Date: Wed, 29 Jul 2026 14:16:55 +0200 Subject: [PATCH 8/9] Fix CEL validation rule to treat maxUnavailable IntOrString as scalar value Signed-off-by: Emilia Desch --- api/shared/nodenetworkconfigurationpolicy_types.go | 2 +- .../nmstate.io_nodenetworkconfigurationpolicies.yaml | 4 ++-- deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/api/shared/nodenetworkconfigurationpolicy_types.go b/api/shared/nodenetworkconfigurationpolicy_types.go index c2be4f3a21..106e7aa16d 100644 --- a/api/shared/nodenetworkconfigurationpolicy_types.go +++ b/api/shared/nodenetworkconfigurationpolicy_types.go @@ -49,7 +49,7 @@ type NodeNetworkConfigurationPolicySpec struct { // 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 && int(self.replace('%', '')) > 0)",message="maxUnavailable must be greater than 0" + // +kubebuilder:validation:XValidation:rule="!has(self) || (type(self) == int && self > 0) || (type(self) == string && self.matches('^[1-9][0-9]*%?$'))",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 64e9089417..1943eb9a12 100644 --- a/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml @@ -75,7 +75,7 @@ spec: x-kubernetes-validations: - message: maxUnavailable must be greater than 0 rule: '!has(self) || (type(self) == int && self > 0) || (type(self) - == string && int(self.replace(''%'', '''')) > 0)' + == string && self.matches(''^[1-9][0-9]*%?$''))' nodeSelector: additionalProperties: type: string @@ -200,7 +200,7 @@ spec: x-kubernetes-validations: - message: maxUnavailable must be greater than 0 rule: '!has(self) || (type(self) == int && self > 0) || (type(self) - == string && int(self.replace(''%'', '''')) > 0)' + == string && self.matches(''^[1-9][0-9]*%?$''))' nodeSelector: additionalProperties: type: string diff --git a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml index 8d3ff32491..847f81ec67 100644 --- a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml @@ -75,7 +75,7 @@ spec: x-kubernetes-validations: - message: maxUnavailable must be greater than 0 rule: '!has(self) || (type(self) == int && self > 0) || (type(self) - == string && int(self.replace(''%'', '''')) > 0)' + == string && self.matches(''^[1-9][0-9]*%?$''))' nodeSelector: additionalProperties: type: string @@ -200,7 +200,7 @@ spec: x-kubernetes-validations: - message: maxUnavailable must be greater than 0 rule: '!has(self) || (type(self) == int && self > 0) || (type(self) - == string && int(self.replace(''%'', '''')) > 0)' + == string && self.matches(''^[1-9][0-9]*%?$''))' nodeSelector: additionalProperties: type: string From f543260478989faa2a44c9ded65931951f3290fd Mon Sep 17 00:00:00 2001 From: Emilia Desch Date: Wed, 29 Jul 2026 15:17:00 +0200 Subject: [PATCH 9/9] Fix CEL validation rule to treat maxUnavailable IntOrString as scalar value Signed-off-by: Emilia Desch --- api/shared/nodenetworkconfigurationpolicy_types.go | 2 +- .../nmstate.io_nodenetworkconfigurationpolicies.yaml | 4 ++-- deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/api/shared/nodenetworkconfigurationpolicy_types.go b/api/shared/nodenetworkconfigurationpolicy_types.go index 106e7aa16d..7a9a4655ea 100644 --- a/api/shared/nodenetworkconfigurationpolicy_types.go +++ b/api/shared/nodenetworkconfigurationpolicy_types.go @@ -49,7 +49,7 @@ type NodeNetworkConfigurationPolicySpec struct { // 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.matches('^[1-9][0-9]*%?$'))",message="maxUnavailable must be greater than 0" + // +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 1943eb9a12..0b42d61d49 100644 --- a/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/bundle/manifests/nmstate.io_nodenetworkconfigurationpolicies.yaml @@ -75,7 +75,7 @@ spec: x-kubernetes-validations: - message: maxUnavailable must be greater than 0 rule: '!has(self) || (type(self) == int && self > 0) || (type(self) - == string && self.matches(''^[1-9][0-9]*%?$''))' + == string && self != ''0'' && self != ''0%'')' nodeSelector: additionalProperties: type: string @@ -200,7 +200,7 @@ spec: x-kubernetes-validations: - message: maxUnavailable must be greater than 0 rule: '!has(self) || (type(self) == int && self > 0) || (type(self) - == string && self.matches(''^[1-9][0-9]*%?$''))' + == string && self != ''0'' && self != ''0%'')' nodeSelector: additionalProperties: type: string diff --git a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml index 847f81ec67..c2cd83a256 100644 --- a/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml +++ b/deploy/crds/nmstate.io_nodenetworkconfigurationpolicies.yaml @@ -75,7 +75,7 @@ spec: x-kubernetes-validations: - message: maxUnavailable must be greater than 0 rule: '!has(self) || (type(self) == int && self > 0) || (type(self) - == string && self.matches(''^[1-9][0-9]*%?$''))' + == string && self != ''0'' && self != ''0%'')' nodeSelector: additionalProperties: type: string @@ -200,7 +200,7 @@ spec: x-kubernetes-validations: - message: maxUnavailable must be greater than 0 rule: '!has(self) || (type(self) == int && self > 0) || (type(self) - == string && self.matches(''^[1-9][0-9]*%?$''))' + == string && self != ''0'' && self != ''0%'')' nodeSelector: additionalProperties: type: string