feat: Close validation gap with AKS checking supported Ubuntu version - #1895
Open
Bevan Arps (theunrepentantgeek) wants to merge 16 commits into
Open
feat: Close validation gap with AKS checking supported Ubuntu version#1895Bevan Arps (theunrepentantgeek) wants to merge 16 commits into
Bevan Arps (theunrepentantgeek) wants to merge 16 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Only validate Kubernetes version compatibility when spec.imageFamily explicitly pins an Ubuntu version (Ubuntu2204 or Ubuntu2404). The generic Ubuntu family and an unset image family are a contract to run an AKS-supported Ubuntu, resolved per Kubernetes version and potentially resolved differently by different provisioning modes, so they must never be rejected based on what the resolver happens to pick. Add a typed ImageFamilyKubernetesVersionIncompatibleError carrying the requested family, discovered version and applied bounds, and match on it with errors.As in the status reconciler so only that error latches ValidationSucceeded=False with ImageFamilyKubernetesVersionIncompatible. Malformed and otherwise unexpected errors are returned for retry instead. Because a pinned-family incompatibility is static, stop requeueing it every minute: the NodeClass spec watch and the Kubernetes version reconciliation already drive reevaluation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a status controller regression that reconciles a NodeClass pinning Ubuntu2404 against a stubbed Kubernetes 1.31 cluster and asserts the aggregate Ready condition ends up False because of ValidationSucceeded, with the companion case at 1.32.0 showing the same NodeClass becomes Ready. This closes the chain the cloudprovider suites only characterize with a hand-set condition. Align the hand-set condition message in the VM and AKS Machine API cloudprovider cases with the message the reconciler now produces. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ValidationReconciler called GetKubernetesVersion() unconditionally, so a generic/unset Ubuntu or AzureLinux NodeClass failed reconciliation and skipped Disk Encryption Set validation whenever the Kubernetes version was not ready yet, even though no image family compatibility policy applies to it. Add imagefamily.RequiresKubernetesVersionCompatibility() as the single source of truth for which spec.imageFamily values are explicitly version pinned, use it in ValidateImageFamilyCompatibility(), and gate the version lookup on it in the status validation path so unpinned families proceed straight to DES validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
A NodeClass whose image family is incompatible with the discovered Kubernetes version has ValidationSucceeded=False, which makes its aggregate Ready condition false. Nodes that were already launched must not be disrupted for that: the incompatibility is a forward-looking provisioning constraint, so treating it as drift would replace healthy nodes with nodes that cannot be provisioned. Cover both provisioning families by extending the existing VM and AKS Machine API drift suites, asserting Ready goes false and IsDrifted still reports no drift. No production drift logic changed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace the separate pinned-family list and per-family bounds switch with a single kubernetesVersionPinnedImageFamilies registry that both RequiresKubernetesVersionCompatibility and ValidateImageFamilyCompatibility derive from, so the predicate and the enforced bounds cannot disagree. Add an internal test that walks every registered policy and checks the predicate, the accepted and rejected versions, and the bounds reported on the typed error all match the registry. Also narrow the drift test comments to what they actually prove, restore an unrelated blank line, reuse newAuthorizationError across the DES cases, and cover the unset image family in the DES authorization failure table. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Bevan Arps (theunrepentantgeek)
September 7, 2026 00:06
View session
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The compatibility policy is centralized, the controller wiring is straightforward, and the PR adds thorough tests covering validation, readiness, create-blocking, and drift behavior across provisioning paths.
Pull request overview
Adds a centralized image-family/Kubernetes-version compatibility policy for explicitly version-pinned Ubuntu families, and wires it into the NodeClass status validation path so incompatible combinations fail early (without attempting Azure resource reads/creates) and do not induce drift for already-launched nodes.
Changes:
- Introduces
imagefamily.ValidateImageFamilyCompatibility+ typed errors, backed by a single pinned-family policy registry. - Updates the NodeClass status
ValidationReconcilerto gate pinned families on the discovered Kubernetes version and latch a stable condition reason for static incompatibilities. - Adds unit/integration coverage across imagefamily validation, NodeClass readiness, CloudProvider create blocking, and drift non-behavior.
File summaries
| File | Description |
|---|---|
| pkg/providers/imagefamily/compatibility.go | Adds pinned Ubuntu family compatibility policy and validation helpers with typed errors. |
| pkg/providers/imagefamily/compatibility_unit_test.go | Verifies the policy registry is the single source of truth for bounds/behavior. |
| pkg/providers/imagefamily/compatibility_test.go | Black-box tests for parsing, boundary behavior, and typed error reporting. |
| pkg/controllers/nodeclass/status/validation.go | Enforces pinned-family compatibility during validation and sets a stable condition reason on static incompatibility. |
| pkg/controllers/nodeclass/status/validation_test.go | Ensures validation condition behavior, DES-call suppression on static incompatibility, and status-controller readiness outcomes. |
| pkg/cloudprovider/suite_test.go | Ensures Create fails fast with NodeClassNotReadyError and no instance creation when validation is latched false. |
| pkg/cloudprovider/suite_drift_test.go | Asserts ValidationSucceeded=false due to incompatibility does not create drift for already-launched NodeClaims (VM path). |
| pkg/cloudprovider/suite_aksmachineapi_offerings_test.go | Mirrors fast-fail/no-create coverage for AKS Machine API offering path. |
| pkg/cloudprovider/suite_aksmachineapi_drift_test.go | Asserts no drift for already-launched NodeClaims in AKS Machine API path when ValidationSucceeded=false. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Member
Author
|
Copilot resolve the merge conflicts in this pull request |
Copilot started work on behalf of
Bevan Arps (theunrepentantgeek)
September 7, 2026 02:39
View session
…lid-potato # Conflicts: # pkg/controllers/nodeclass/status/validation.go # pkg/controllers/nodeclass/status/validation_test.go Co-authored-by: theunrepentantgeek <1272094+theunrepentantgeek@users.noreply.github.com>
Contributor
Copilot started reviewing on behalf of
Bevan Arps (theunrepentantgeek)
September 7, 2026 02:48
View session
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
A unit test assertion uses ContainSubstring with a format string, which prevents it from validating the computed expected range correctly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Lite
#1897) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Bevan Arps (theunrepentantgeek)
September 7, 2026 05:44
View session
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The behavior change is narrowly scoped, integrated at the status-validation layer, and is backed by unit and integration tests covering provisioning and drift expectations.
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 2
- Review effort level: Lite
Comment on lines
+38
to
+40
| // ImageFamilyKubernetesVersionIncompatible is the stable reason used when | ||
| // spec.imageFamily explicitly pins an OS version that the ready discovered | ||
| // Kubernetes version does not support. |
Comment on lines
+29
to
+30
| // kubernetesVersionPolicy is the Kubernetes version window in which one explicitly | ||
| // version-pinned spec.imageFamily value is usable. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Bevan Arps (theunrepentantgeek)
September 7, 2026 05:54
View session
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes provisioning-gating validation behavior and upgrades multiple dependencies, so it warrants final human review to confirm the AKS support-window bounds and broader dependency impact.
Review details
- Files reviewed: 9/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This pull request introduces improved validation logic for image family and Kubernetes version compatibility in the Azure cloud provider, along with comprehensive test coverage for these scenarios. The main focus is to ensure that when a
NodeClassspecifies an incompatible image family for the discovered Kubernetes version, appropriate errors are surfaced early, and no disruptive drift or resource creation occurs. The changes also clarify that such validation failures do not cause drift for already-launched nodes.How was this change tested?
Does this change impact docs?
Release Note