feat: reject cluster version updates lacking a reachable update channel by redhat-chai-bot · Pull Request #6445 · Azure/ARO-HCP

Chai-bot (redhat-chai-bot) · 2026-08-05T22:38:30Z

Summary

Adds a validating admission check that prevents properties.version.id updates when the target version's update channel is not reachable for the cluster.

When a cluster's version.id is updated, the admission check builds the expected channel name (<channelGroup>-<major>.<minor> — e.g. stable-4.20 for version 4.20.8) and verifies it exists in the ServiceProviderCluster.Status.DesiredVersionChannels list. If the channel is absent from a non-empty list, the update is rejected with field.Invalid.

Channels only appear in the HostedCluster's status.version.desired.channels when the cluster's current desired release has a valid upgrade edge to a release in that channel — so a missing channel means there is no supported upgrade path.

Architecture

The HostedCluster channel data is mirrored into ServiceProviderCluster by the backend's ControlPlaneActiveVersions controller. The admission check reads from ServiceProviderCluster (which is prefetched into ClusterAdmissionContext by the frontend), preserving the architectural boundary: the frontend must never access kube-applier, ReadDesireLister, or management cluster APIs directly.

Behavior

Fail-open when data is not yet synced: Both ServiceProviderCluster == nil and len(DesiredVersionChannels) == 0 skip validation (return no error). Since DesiredVersionChannels is populated asynchronously by the backend, an empty list means the data hasn't been mirrored yet — not that no channels exist. A genuinely missing ServiceProviderCluster prefetch is still surfaced as InternalError by the existing version-skew check.
Reject when channel is genuinely absent: When the backend has published a non-empty channel list and the expected <channelGroup>-<major>.<minor> is not in it, returns field.Invalid.
Skip when ChannelGroup is empty: For backwards compatibility with clusters that predate channel groups.

Changes

ServiceProviderClusterStatus.DesiredVersionChannels — new []string field mirroring HostedCluster channels
ControlPlaneActiveVersions controller — syncs channels from HostedCluster into ServiceProviderCluster, included in change detection
admitClusterVersionID — new admission function with versionMajorMinor() helper for semver parsing
docs/cosmos-data-flow.md — updated with the new field
CLAUDE.md (root + internal/admission/) — architectural rules documented
10 test cases covering: matching channel, no match, empty list, nil SP cluster, empty ChannelGroup, unchanged ID, CREATE op, micro versions, nightly versions, pre-release versions

Copilot

Pull request overview

This PR adds an admission-time guardrail to prevent properties.version.id updates when the target version’s update channel is not reachable for the cluster, using HostedCluster-derived channel availability mirrored into ServiceProviderCluster by the backend (preserving the frontend’s isolation from management-cluster APIs).

Changes:

Mirror HostedCluster.Status.Version.Desired.Channels into ServiceProviderCluster.Status.DesiredVersionChannels via the backend ControlPlaneActiveVersions controller, and include it in change detection to avoid no-op Cosmos writes.
Add an admission check (admitClusterVersionChannel) that rejects version.id updates when the expected channel is not present in DesiredVersionChannels, plus unit tests for the new behavior.
Document/enforce the architectural boundary that frontend/admission must not access management-cluster state directly (must be mirrored through ServiceProviderCluster).

Reviewed changes

Copilot reviewed 6 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
internal/api/zz_generated.deepcopy.go	Deep-copy support for the new `DesiredVersionChannels` slice field.
internal/api/types_serviceprovider_cluster.go	Adds `ServiceProviderClusterStatus.DesiredVersionChannels` with semantics and writer annotation.
internal/admission/CLAUDE.md	Documents the “mirror through ServiceProviderCluster” architectural rule for admission.
internal/admission/admit_cluster.go	Implements `admitClusterVersionChannel` and wires it into version update validation.
internal/admission/admit_cluster_test.go	Adds unit tests covering the new version-channel admission behavior.
frontend/pkg/frontend/cluster.go	Adds an architectural comment near `newClusterAdmissionContext` reinforcing the isolation boundary.
backend/pkg/controllers/cluster/version/control_plane_active_version_controller.go	Mirrors HostedCluster desired channels into SPC status and includes them in update detection/logging.

Files not reviewed (1)

internal/api/zz_generated.deepcopy.go: Generated file

David Eads (deads2k) · 2026-08-06T14:15:52Z

+		{
+			name:         "version.id update with matching channel passes",
+			op:           operation.Operation{Type: operation.Update},
+			oldVersion:   &api.VersionProfile{ID: "4.18", ChannelGroup: "stable"},
+			newVersion:   &api.VersionProfile{ID: "4.19", ChannelGroup: "stable"},
+			spc:          spcWithChannels("stable-4.18", "candidate-4.19", "stable-4.19"),
+			expectErrors: []utils.ExpectedError{},
+		},
+		{
+			name:       "version.id update with no matching channel is rejected",
+			op:         operation.Operation{Type: operation.Update},
+			oldVersion: &api.VersionProfile{ID: "4.18", ChannelGroup: "stable"},
+			newVersion: &api.VersionProfile{ID: "4.19", ChannelGroup: "stable"},
+			spc:        spcWithChannels("stable-4.20", "candidate-4.20"),
+			expectErrors: []utils.ExpectedError{
+				{FieldPath: "properties.version.id", Message: `no upgrade path to update channel "stable-4.19"`},
+			},
+		},


Yup, definitely add. And add a test the nightly build versions you see on a page like https://amd64.ocp.releases.ci.openshift.org/#5.0.0-0.nightly

David Eads (deads2k) · 2026-08-06T14:14:49Z

+	expectedChannel := fmt.Sprintf("%s-%s", newObj.ChannelGroup, newObj.ID)
+	availableChannels := admissionContext.ServiceProviderCluster.Status.DesiredVersionChannels
+	if slices.Contains(availableChannels, expectedChannel) {


Comment is correct. Channels are only major.minor. Add a test to be sure this handles it properly.

+	// HostedCluster) so that DB-free cluster admission can reject a version.id
+	// change whose target channel ("<channelGroup>-<id>") is not reachable,
+	// without the frontend ever needing access to the management cluster.
+	// Written by: ControlPlaneActiveVersions
+	DesiredVersionChannels []string `json:"desiredVersionChannels,omitempty"`


Chai-bot (redhat-chai-bot) · 2026-08-05T23:03:05Z

/retest-required

AI-generated. Review for accuracy.

Copilot

Pull request overview

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

Files not reviewed (1)

internal/api/zz_generated.deepcopy.go: Generated file

Suppressed comments (3)

internal/admission/admit_cluster.go:565

The PR description says this check returns InternalError when ServiceProviderCluster is nil and rejects when the channel list is empty, but the implementation here is explicitly fail-open in both cases (and the tests reflect that). Please align the PR description with the actual behavior (or adjust code/tests if the stricter behavior is intended).

	// No channel data yet: fail open (see the doc comment above). A genuinely
	// missing ServiceProviderCluster prefetch is still surfaced as an
	// InternalError by the version-skew check in admitClusterVersionProfile, so
	// we do not duplicate that here.
	if admissionContext.ServiceProviderCluster == nil {
		return nil
	}
	availableChannels := admissionContext.ServiceProviderCluster.Status.DesiredVersionChannels
	if len(availableChannels) == 0 {
		return nil
	}

internal/api/types_serviceprovider_cluster.go:167

This adds a new Cosmos-persisted ServiceProviderCluster status field. Per the repo’s Cosmos data flow documentation, docs/cosmos-data-flow.md should be updated to reflect that ControlPlaneActiveVersions now also writes Status.DesiredVersionChannels (in addition to Status.ControlPlaneVersion.ActiveVersions).

	// DesiredVersionChannels mirrors the observed HostedCluster's
	// status.version.desired.channels (see hypershift ClusterVersionStatus ->
	// configv1.Release.Channels). Each entry is an OpenShift update channel name
	// such as "stable-4.19" or "candidate-4.20".
	//
	// A channel only appears in status.version.desired.channels when the
	// cluster's current desired release has a valid upgrade edge to a release
	// served by that channel. In other words, this is the set of channels the
	// cluster can currently move along; a channel being absent means there is no
	// supported upgrade path to that channel's release line yet.
	//
	// This list is mirrored here by the backend (which alone can observe the
	// HostedCluster) so that DB-free cluster admission can reject a version.id
	// change whose target channel ("<channelGroup>-<id>") is not reachable,
	// without the frontend ever needing access to the management cluster.
	// Written by: ControlPlaneActiveVersions
	DesiredVersionChannels []string `json:"desiredVersionChannels,omitempty"`

internal/admission/admit_cluster.go:506

This comment says the version change is rejected when the requested channel has no reachable upgrade edge, but the implementation intentionally fails open when DesiredVersionChannels is empty/unavailable. Consider rewording to reflect that rejection only happens once a non-empty channel list has been mirrored by the backend.

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

	// Reject the version change if the requested update channel has no reachable
	// upgrade edge for this cluster (see admitClusterVersionChannel).
	errs = append(errs, admitClusterVersionChannel(ctx, admissionContext, op, fldPath, newObj, oldObj)...)

David Eads (deads2k)

forgot to update the Cosmos Data Flow Documentation

David Eads (deads2k) · 2026-08-06T14:12:38Z

+// change until the mirror is populated, which is incorrect. This check therefore
+// only rejects when the backend has published a non-empty channel list that
+// genuinely lacks the requested channel.
+func admitClusterVersionChannel(_ context.Context, admissionContext *ClusterAdmissionContext, op operation.Operation, fldPath *field.Path, newObj, oldObj *api.VersionProfile) field.ErrorList {


rename to adminClusterVersionID

David Eads (deads2k) · 2026-08-06T14:13:48Z

 // records) so admission can validate version skew without hitting the DB itself.
 // On CREATE pass a nil clusterResourceID — no prior state exists to prefetch.
+//
+// Architectural rule: the frontend must NEVER reach the management cluster


Add this to the top level CLAUDE.md in a reasonable spot as a rule for the entire frontend.

David Eads (deads2k) · 2026-08-06T14:14:49Z

+	expectedChannel := fmt.Sprintf("%s-%s", newObj.ChannelGroup, newObj.ID)
+	availableChannels := admissionContext.ServiceProviderCluster.Status.DesiredVersionChannels
+	if slices.Contains(availableChannels, expectedChannel) {


Comment is correct. Channels are only major.minor. Add a test to be sure this handles it properly.

David Eads (deads2k) · 2026-08-06T14:15:52Z

+		{
+			name:         "version.id update with matching channel passes",
+			op:           operation.Operation{Type: operation.Update},
+			oldVersion:   &api.VersionProfile{ID: "4.18", ChannelGroup: "stable"},
+			newVersion:   &api.VersionProfile{ID: "4.19", ChannelGroup: "stable"},
+			spc:          spcWithChannels("stable-4.18", "candidate-4.19", "stable-4.19"),
+			expectErrors: []utils.ExpectedError{},
+		},
+		{
+			name:       "version.id update with no matching channel is rejected",
+			op:         operation.Operation{Type: operation.Update},
+			oldVersion: &api.VersionProfile{ID: "4.18", ChannelGroup: "stable"},
+			newVersion: &api.VersionProfile{ID: "4.19", ChannelGroup: "stable"},
+			spc:        spcWithChannels("stable-4.20", "candidate-4.20"),
+			expectErrors: []utils.ExpectedError{
+				{FieldPath: "properties.version.id", Message: `no upgrade path to update channel "stable-4.19"`},
+			},
+		},


Yup, definitely add. And add a test the nightly build versions you see on a page like https://amd64.ocp.releases.ci.openshift.org/#5.0.0-0.nightly

Copilot

Pull request overview

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

Files not reviewed (1)

internal/api/coreapi/zz_generated.deepcopy.go: Generated file

Suppressed comments (2)

internal/admission/CLAUDE.md:101

internal/admission/CLAUDE.md references admitClusterVersionChannel, but the implementation added in this PR is named admitClusterVersionID. This mismatch makes the guidance harder to follow and will confuse future readers/searches.

Concrete example in this package: `admitClusterVersionChannel` validates a
version.id change against `ServiceProviderCluster.Status.DesiredVersionChannels`.

internal/admission/admit_cluster.go:587

The PR description says the new channel validation returns an InternalError when ServiceProviderCluster is nil and rejects an empty DesiredVersionChannels list, but the code here explicitly fails open for both cases (ServiceProviderCluster == nil and len(DesiredVersionChannels) == 0 both skip validation). If fail-open is intended, the PR description (and the test-case list in it) should be updated to match; if not intended, the logic here needs to be tightened to enforce the described behavior.

	// No channel data yet: fail open (see the doc comment above). A genuinely
	// missing ServiceProviderCluster prefetch is still surfaced as an
	// InternalError by the version-skew check in admitClusterVersionProfile, so
	// we do not duplicate that here.
	if admissionContext.ServiceProviderCluster == nil {
		return nil
	}
	availableChannels := admissionContext.ServiceProviderCluster.Status.DesiredVersionChannels
	if len(availableChannels) == 0 {
		return nil
	}

Chai-bot (redhat-chai-bot) · 2026-08-07T13:38:04Z

/retest-required

AI-generated. Review for accuracy.

Manyanda Chitimbo (machi1990) · 2026-08-07T14:23:22Z

+	// whether a requested version.id has a reachable update channel. Doing the
+	// mirroring here keeps the frontend (and therefore admission) free of any
+	// management-cluster access; see internal/admission/CLAUDE.md.


This seems like an architectural rule that should live in a different file other than CLAUDE.md

Chai-bot (@redhat-chai-bot)

Ah okay, it was instructed here #6445 (comment) , ignore the comment.

Manyanda Chitimbo (machi1990) · 2026-08-07T14:37:28Z

+`ServiceProviderCluster` in `newClusterAdmissionContext` like any other
+server-side state, and admission reads it from the context struct.
+
+Concrete example in this package: `admitClusterVersionChannel` validates a


Suggested change

Concrete example in this package: `admitClusterVersionChannel` validates a

Concrete example in this package: `admitClusterVersionID` validates a

Good catch — this reference was missed during the rename. Fixing now along with updating the PR description to accurately reflect the fail-open behavior.

AI-generated. Review for accuracy.

Manyanda Chitimbo (machi1990) · 2026-08-07T14:40:16Z

Chai-bot (@redhat-chai-bot) for (ServiceProviderCluster == nil and len(DesiredVersionChannels) == 0 both skip validation) and we don't fail as stated in the PR's description: Returns field.InternalError if ServiceProviderCluster is nil, consider updating the PR's description

Manyanda Chitimbo (machi1990) · 2026-08-07T14:41:05Z

Only two minor comments, otherwise the code lgtm

Chai-bot (redhat-chai-bot) · 2026-08-07T14:43:24Z

Manyanda Chitimbo (@machi1990) You're right — the PR description is stale from the original implementation before the fail-open fix. Updating both the CLAUDE.md function name reference and the PR description now.

AI-generated. Review for accuracy.

…ades Add a validating admission check that fires when a cluster's version.id is updated. Once the backend has published the cluster's observed update channels, the check requires the target update channel ("<channelGroup>-<major>.<minor>") to be present in that list. A channel only appears in HostedCluster.status.version.desired.channels when the cluster's current version has a valid upgrade edge to a release served by that channel, so a missing channel means there is no supported upgrade path to the requested version line and the update is rejected. The requested version's release line is derived with versionMajorMinor, so patch ("4.20.8"), nightly ("5.0.0-0.nightly-...") and pre-release ("4.21.0-rc.1") IDs all resolve to their "<major>.<minor>" channel rather than being compared by raw ID. The check fails open until the channel data is synced: DesiredVersionChannels is populated asynchronously by the backend and is empty on freshly created clusters and until the first sync completes. When the list is unavailable (no ServiceProviderCluster prefetched, or an empty list) the check is skipped rather than blocking the update, so it never rejects a version change purely because the mirror has not been populated yet. It only rejects against a non-empty published channel list that genuinely lacks the requested channel. The observed HostedCluster lives on the management cluster, which the frontend must never access. Instead the backend ControlPlaneActiveVersions controller mirrors HostedCluster.status.version.desired.channels onto ServiceProviderCluster.Status.DesiredVersionChannels, which the frontend prefetches into the admission context. This "mirror through ServiceProviderCluster" rule is documented in internal/admission/CLAUDE.md, the root CLAUDE.md, and reiterated near newClusterAdmissionContext. - api: add DesiredVersionChannels to ServiceProviderClusterStatus (+ deepcopy) - backend: sync desired channels in control_plane_active_version_controller - admission: add admitClusterVersionID (major.minor channel lookup) wired into admitClusterVersionProfile - docs: document the new read/write in cosmos-data-flow.md and the frontend isolation boundary in the root CLAUDE.md - tests: table-driven coverage incl. micro/nightly/pre-release resolution Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Manyanda Chitimbo (machi1990)

/lgtm

openshift-ci · 2026-08-07T14:49:25Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: machi1990, redhat-chai-bot
Once this PR has been reviewed and has the lgtm label, please assign bennerv 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:

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 8 out of 9 changed files in this pull request and generated no new comments.

Files not reviewed (1)

internal/api/coreapi/zz_generated.deepcopy.go: Generated file

Suppressed comments (2)

internal/api/coreapi/types_serviceprovider_cluster.go:165

The comment describes the target channel as "-", but the admission check (and OpenShift channel naming) uses the release line "-." (e.g. stable-4.20). Keeping this precise avoids confusion for future readers and keeps the doc consistent with admitClusterVersionID.

	// This list is mirrored here by the backend (which alone can observe the
	// HostedCluster) so that DB-free cluster admission can reject a version.id
	// change whose target channel ("<channelGroup>-<id>") is not reachable,
	// without the frontend ever needing access to the management cluster.

backend/pkg/controllers/cluster/version/control_plane_active_version_controller.go:114

This controller now mirrors HostedCluster desired-version channels onto ServiceProviderCluster.Status.DesiredVersionChannels, but the existing unit tests in control_plane_active_version_controller_test.go only assert ActiveVersions updates. Adding a test case that seeds HostedCluster.Status.Version.Desired.Channels (and verifies the SPC Status field is updated, and that no Replace occurs when channels are unchanged) would prevent regressions in the admission data path.

	// Mirror the observed HostedCluster's status.version.desired.channels onto
	// the ServiceProviderCluster. Cluster admission consumes this list to decide
	// whether a requested version.id has a reachable update channel. Doing the
	// mirroring here keeps the frontend (and therefore admission) free of any
	// management-cluster access; see internal/admission/CLAUDE.md.
	newDesiredChannels := getHostedClusterDesiredVersionChannels(hostedCluster)

Copilot AI lite review requested due to automatic review settings August 5, 2026 22:38

openshift-ci Bot requested review from David Eads (deads2k) and Gerd Oberlechner (geoberle) August 5, 2026 22:38

Copilot started reviewing on behalf of Chai-bot (redhat-chai-bot) August 5, 2026 22:39 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 5, 2026 23:01

Chai-bot (redhat-chai-bot) force-pushed the feat/cluster-version-channel-admission branch from 19e2ad3 to afb8d12 Compare August 5, 2026 23:01

Copilot started reviewing on behalf of Chai-bot (redhat-chai-bot) August 5, 2026 23:02 View session

Copilot AI reviewed Aug 5, 2026

View reviewed changes

David Eads (deads2k) requested changes Aug 6, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 7, 2026 11:28

Chai-bot (redhat-chai-bot) force-pushed the feat/cluster-version-channel-admission branch from afb8d12 to c390bf0 Compare August 7, 2026 11:28

Copilot started reviewing on behalf of Chai-bot (redhat-chai-bot) August 7, 2026 11:30 View session

Copilot AI reviewed Aug 7, 2026

View reviewed changes

Manyanda Chitimbo (machi1990) reviewed Aug 7, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings August 7, 2026 14:46

Chai-bot (redhat-chai-bot) force-pushed the feat/cluster-version-channel-admission branch from c390bf0 to 09b0a3a Compare August 7, 2026 14:46

Copilot started reviewing on behalf of Chai-bot (redhat-chai-bot) August 7, 2026 14:46 View session

Manyanda Chitimbo (machi1990) approved these changes Aug 7, 2026

View reviewed changes

openshift-ci Bot assigned Manyanda Chitimbo (machi1990) Aug 7, 2026

openshift-ci Bot added the lgtm label Aug 7, 2026

Copilot AI reviewed Aug 7, 2026

View reviewed changes

	Concrete example in this package: `admitClusterVersionChannel` validates a
	Concrete example in this package: `admitClusterVersionID` validates a

Uh oh!

Conversation

Chai-bot (redhat-chai-bot) commented Aug 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Architecture

Behavior

Changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chai-bot (redhat-chai-bot) commented Aug 5, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

David Eads (deads2k) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Chai-bot (redhat-chai-bot) commented Aug 7, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Manyanda Chitimbo (machi1990) commented Aug 7, 2026

Uh oh!

Manyanda Chitimbo (machi1990) commented Aug 7, 2026

Uh oh!

Chai-bot (redhat-chai-bot) commented Aug 7, 2026

Uh oh!

Manyanda Chitimbo (machi1990) left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Aug 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Chai-bot (redhat-chai-bot) commented Aug 5, 2026 •

edited

Loading