test: add cross-major node pool downgrade validation cases

[Chai-bot (redhat-chai-bot)]

Adds two missing test cases to TestValidateNodePoolVersionChange for cross-major node pool downgrade scenarios, as requested in https://github.com/Azure/ARO-HCP/pull/6414#discussion_r3721430849.\n\n## New test cases\n\n1. cross-major downgrade requires major upgrade flag — Verifies that a cross-major downgrade (active NP at 5.0, desired 4.22) is rejected when the allowMajorUpgrade flag is not set.\n2. cross-major downgrade with unsupported node pool version is rejected — Verifies that a cross-major downgrade to a NP version not in AllowControlPlaneNodePoolMajorVersionSkew (4.20) is rejected even with the flag enabled.\n\nThese cases were originally part of PR #6414 but were removed during review. The follow-up was requested to ensure the downgrade rejection paths have explicit coverage.

---

AI-generated. Review for accuracy.

Chetan Giradkar (@cgiradkar) requested in Slack thread

[Copilot]

Pull request overview

Adds explicit unit-test coverage for cross-major node pool downgrade validation paths in internal/validation, ensuring downgrade rejection behavior is exercised (as follow-up coverage requested from PR #6414 discussion).

Changes:

• Add a test case asserting cross-major downgrade is rejected when allowMajorUpgrade is not enabled.
• Add a test case asserting cross-major downgrade to an unsupported minor line (not present in AllowControlPlaneNodePoolMajorVersionSkew) is rejected even when allowMajorUpgrade is enabled.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

internal/validation/validators.go:1033

• The new error message claims the control plane is on a different major version, but this branch is selected based on the active node pool highest major (desiredVersion.Major < highest.Major). If the control plane major happens to match the desired major (e.g., active versions already include a higher major than the control plane), the message can become self-contradictory/unhelpful (it may print the same major for node pool vs control plane). Consider rewording to describe the actual condition (cross-major downgrade relative to the active node pool) and/or mention the feature gate flag rather than asserting the CP major differs.

			if isCrossMajorDowngrade {
				return fmt.Errorf("node pool version changes are not supported while the control plane is on a different major version (node pool major version %d vs control plane major version %d)", desiredVersion.Major, highestCPVersion.Major)
			}

internal/validation/validators.go:1034

• PR description says this follow-up only adds test cases, but this change also modifies production validation behavior by introducing a new, more specific error string for cross-major downgrades. Either update the PR description/title to reflect the behavior change, or avoid changing the runtime error message and have the test assert the existing error string.

This issue also appears on line 1031 of the same file.

			if isCrossMajorDowngrade {
				return fmt.Errorf("node pool version changes are not supported while the control plane is on a different major version (node pool major version %d vs control plane major version %d)", desiredVersion.Major, highestCPVersion.Major)
			}
			return fmt.Errorf("major version changes are not supported")

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: redhat-chai-bot
Once this PR has been reviewed and has the lgtm label, please assign mbarnes for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• backend/OWNERS
• internal/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

internal/validation/validators.go:1035

• The cross-major downgrade error message claims the control plane is on a different major version, but the condition here only guarantees a node pool major change (desired < active highest). If the control plane major were equal to the desired major (in inconsistent/edge-state data), this would produce a contradictory message (e.g., "...different major..." while printing the same major). Gate this message on the control plane major actually differing, and fall back to the generic error otherwise.

		if !allowMajorUpgrade {
			if isCrossMajorDowngrade {
				return fmt.Errorf("node pool version changes are not supported while the control plane is on a different major version (node pool major version %d vs control plane major version %d)", desiredVersion.Major, highestCPVersion.Major)
			}
			return fmt.Errorf("major version changes are not supported")

internal/validation/validators_test.go:394

• This test is asserting the full error string including the specific major numbers. Since the admission test helper only requires a partial match, this can be made less brittle by matching just the stable portion of the message.

			allowMajor:     false,
			expectError:    true,
			errContains:    "node pool version changes are not supported while the control plane is on a different major version (node pool major version 4 vs control plane major version 5)",
		},

internal/admission/admit_nodepool_test.go:540

• The admission tests use partial matching for error messages; matching the entire formatted string (including the major numbers) makes this test unnecessarily brittle to future wording/format tweaks. Prefer matching only the stable portion of the message.

			clusterVersions: []string{"5.0.1"},
			desiredVersion:  "5.0.1",
			expectErrors: []utils.ExpectedError{
				{FieldPath: "properties.version.id", Message: "node pool version changes are not supported while the control plane is on a different major version (node pool major version 4 vs control plane major version 5)"},
			},

backend/pkg/controllers/nodepool/version/nodepool_version_controller_test.go:746

• This test is matching the full formatted error string including specific major numbers; using a shorter substring keeps the test focused on behavior and avoids churn if the message formatting changes.

			name:                 "major version downgrade - fail by default",
			desiredVersion:       "4.22.0",
			activeVersions:       []string{"5.0.1"},
			controlPlaneVersions: []string{"5.0.1"},
			expectError:          true,
			errorContains:        "node pool version changes are not supported while the control plane is on a different major version (node pool major version 4 vs control plane major version 5)",
		},

[David Eads (deads2k)]

this is the opposite of what we want.

[Chai-bot (redhat-chai-bot)]

Thanks for the feedback. Could you clarify the preferred direction? A couple of possible interpretations:

1. The descriptive error should apply to the upgrade path, not the downgrade path — I scoped it to downgrade only because in the upgrade case desiredVersion.Major == highestCPVersion.Major (e.g. both 5), making the message "node pool major version 5 vs control plane major version 5" contradictory. Should this be reworked?

2. This production code change shouldn't be in this PR at all — the original scope was test-only additions; the error format change was a follow-on request to align with PR fix: clarify error message for same-major NP change with different-major CP #6434. Should I revert the validators.go change and keep this PR test-only?

Happy to adjust either way.

---

AI-generated. Review for accuracy.