WIP ARO-13667 hypershift: support disabling image registry via capability

[flavianmissi]

No description provided.

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from flavianmissi. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

• enhancements/hypershift/OWNERS

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

[tjungblu]

I think I got this one backward. You apparently have to set the baselineCapabilitySet to None.

spec:
  clusterID: %s
  capabilities:
    baselineCapabilitySet: 'None'
    additionalEnabledCapabilities: [ALL but ImageRegistry]

Alternatively, and maybe cleaner, we can introduce a Hypershift capability set just for this usecase

[tjungblu]

source, if anyone else wants to look into it
https://github.com/openshift/hypershift/pull/5315/files

[flavianmissi]

Nice catch, you are right: https://docs.openshift.com/container-platform/4.17/installing/overview/cluster-capabilities.html - I have updated this accordingly.
As for introducing a Hypershift specific capability set, I think that will end up constraining what can be disabled in the future. If we go for the on the fly calculation (base capability set - disabled capability set = final capability set) then we open up for extending the set of disabled capabilities in the future.
The one question that remains is whether ClusterVersionCapabilitySets[ClusterVersionCapabilitySetCurrent] is the correct base capability set.
We must also find a good way to constraint the set of disabled capabilities to only those supported by Hypershift. I know we can use CEL validation for this, but I'm uncertain whether we want to constrain this in more places in the code.

[tjungblu]

this is not strictly related to this enhancement, but working through the caps I've noticed that the installer has a dedicated (non-API based) validation we might need to replicate:
https://github.com/openshift/installer/blob/main/pkg/types/validation/installconfig.go#L205-L253

Things like

if c.Platform.BareMetal != nil && !enabledCaps.Has(configv1.ClusterVersionCapabilityBaremetal)

are obviously bogus for Hypershift, but the ClusterVersionCapabilitySetCurrent will still define BareMetal, so we're also setting this implicitly.

It seems to me that a decoupled and dedicated CapabilitySet for Hypershift would be a better idea, but I defer this to @deads2k - this is clearly more work

[wking]

it's not clear to me why the installer's baremetal logic wouldn't apply to HyperShift. Does HyperShift not run on baremetal? Or does it run on baremetal but somehow allow the baremetal component to be disabled? I wonder what things like bring-your-own-nodes workflows might look like on HyperShift, but I could see some future where folks want just the Kube API and don't need the MachineAPI (or some future ClusterAPI?) component enabled, to tell the HostedControlPlane controller that it can skip the Cluster-API Machine(Set), etc. reconcilers.

[tjungblu]

@wking what's the impact when we run a Hypershift control plane (e.g. on AWS) with enabled baremetal capability?

[flavianmissi]

@sjenning can you help us answer this question? this came up on a slack discussion.
I don't fully understand the role of the CVO in Hypershift so I'm having difficulties understanding how potential changes in the resources belonging to a capability might affect upgrades.

Here's the relevant section from the linked enhancement:

Updates

During updates, resources may move from being associated with one capability, to another, or move from being part of the core (always installed) to being part of an optional capability. In such cases, the governing logic is that once a resource has been applied to a cluster, it must continue to be applied to the cluster (unless it is removed from the payload entirely, of course). Furthermore, if any resource from a given capability is applied to a cluster, all other resources from that capability must be applied to the cluster, even if the cluster configuration would otherwise filter out that capability. These are referred to as implicitly enabled capabilities and the purpose is to avoid scenarios that would break cluster functionality. Once a resource is applied to a cluster, it will always be reconciled in future versions, and that if any part of a capability is present, the entire capability will be present.

This means the CVO must apply the following logic:

    For the outgoing payload, find all manifests that are [currently included](https://github.com/openshift/enhancements/blob/master/enhancements/installer/component-selection.md#manifest-annotations).
    For the new payload, find all the manifests that would be included, in the absence of capability filtering.
    Match manifests using group, kind, namespace, and name.
    Union all capabilities from matched manifests with the set of previously-enabled capabilities to compute the new enabledCapabilities set.

As in [the general case](https://github.com/openshift/enhancements/blob/master/enhancements/installer/component-selection.md#capabilities-cannot-be-uninstalled), when the requested spec.capabilities diverge from enabledCapabilities, the CVO will set ImplicitlyEnabledCapabilities=True in ClusterVersion's status.conditions with a message pointing out the divergence.

cc @tjungblu

[sjenning]

Let me start by saying that I didn't know about ImplicitlyEnabledCapabilities until just now, but I think the ramifications are this:

Capabilities cannot be disabled once enabled. That we already know. The effect is that if a user creates a cluster with the registry enabled or enables it after initially disabling it at install time, that cluster is locked to having the image registry enabled forever. The only possible toggle is from disabled -> enabled and it is a one-way trip.

ImplicitlyEnabledCapabilities is for converting more components into capabilities over time.

For example, in the y-release before image registry became a capability, ClusterVersionCapabilitySet4_13, ImageRegistry would not have been in the enabled capabilities set because it didn't exist yet. But once you upgraded to the y-stream where it was a capability, CV spec.capabilities would still be ClusterVersionCapabilitySet4_13 but CVO will set ImplicitlyEnabledCapabilities=True reflecting that ImageRegistry was not in the spec.capabilities but is still enabled because it was present pre-upgrade.

All that being said, I don't think we have to worry about this from an hypershift API perspective because we are not exposing the capabilities API directly on the HostedCluster resource. As long as the HostedCluster API reflects the limitations of the underlying capabilities API (i.e. that enabling is a one-way trip and disabling can only happen a cluster creation time) we should be fine. We can let the CVO deal with the ImplicitlyEnabledCapabilities and because we don't support any OCP versions before the ImageRegistry capabilities was created, we don't have to worry about the situation above.

[wking]

Summary

You'll want to figure out how to handle at least some of implicit-enablement, to avoid dropping DeploymentConfig in clusters updating into OCPSTRAT-1465.

Details

It's taken us a while to get real-world examples, but 4.18's OperatorLifecycleManagerV1 is the first capability born out of the blue (for GA clusters). The functionality was tech-preview in 4.17. So a GA 4.17 cluster updating to 4.18 with the None capability set would have not had the OperatorLifecycleManagerV1 capability enabled. You don't currently allow anything like a None capability set, so this case isn't relevant for you yet.

But the other case, where a capability is removed from vCurrent, is relevant. We're expecting DeploymentConfig to be dropped soon (say in 4.20, just to simplify discussion, but that's not a commitment, I'm not sure if or when it will actually happen). Clusters updating from 4.19 to 4.20 in vCurrent will implicitly enable the capability. If you leave the cluster in vCurrent without implicitly enabling the capability, the OpenShift API server says "hmm, I guess I reject this DeploymentConfig API request..." and all the DeploymentConfig-based Pods in the cluster get garbage collected, or something. Which would probably upset the workload admins who'd been using DeploymentConfigs pre-update.

• You could maybe narrowly dance around it by unioning ClusterVersion's status.capabilities.enabledCapabilities with the vCurrent - disabledCapabilities set, but that would be feeding hosted-cluster-side ClusterVersion status back into management-cluster-side HostedControlPlane controler activities, which we try to avoid.
• The CVO avoids that kind of feedback loop at update-time by holding both the outgoing and incoming manifest sets in memory at once, and comparing them to see which capabilities need enabling. But the CVO is still pulling ClusterVersion status.capabilities.enabledCapabilities back into its control loop during CVO-container-restart, because we don't have an alternative location to store that state. And having status that cannot be reconstructed without a time machine to see what older clusters were reconciling can make some backup/recovery flows more difficult.
• Another possible workaround is adding your own status property, e.g. to HostedControlPlane, to hold the implicitly-enabled capabilities on the management cluster without tainting them with hosted-cluster-side data. But this still suffers from the "control loop fed by status" backup/recovery exposure that the current standalone CVO is exposed to.
• Another possible workaround would be to grow the enabledCapabilities property you float now, in this initial phase, and then you can hard-code logic like "when a cluster updates from 4.19, always add DeploymentConfig to enabledCapabilities" to the 4.20 HostedControlPlane controller. Or other permutations of that kind of hard-coding, which is tedious to maintain, but possibly less brittle than relying on the preservation of status properties.

Aside from moving whole capabilities like that, the current CVO code handles cases like:

1. There was a component built into OCP, without an associated ClusterVersion capability knob in 4.y.
2. The component maintainers try to create a new ClusterVersion capability for their component in 4.(y+1).
3. But they missed a manifest! The PrometheusRule gets the capability annotation in 4.(y+2).

Folks with the capability disabled on 4.(y+1) will have it implicitly enabled upon updating to 4.(y+2), because we don't support having a half-enabled capability. This situation doesn't come up that often, but it has happened. If you're willing to have the occasional cluster ambiguously running a partial capability, you can probably ignore it.

[flavianmissi]

Thank you both for your thoughtful answers.

The functionality was tech-preview in 4.17. So a GA 4.17 cluster updating to 4.18 with the None capability set would have not had the OperatorLifecycleManagerV1 capability enabled. You don't currently allow anything like a None capability set, so this case isn't relevant for you yet.

This EP proposes we start setting BaselineCapabilitySet to None, so this would become relevant...

Clusters updating from 4.19 to 4.20 in vCurrent will implicitly enable the capability. If you leave the cluster in vCurrent without implicitly enabling the capability, the OpenShift API server says "hmm, I guess I reject this DeploymentConfig API request..." and all the DeploymentConfig-based Pods in the cluster get garbage collected, or something.

Given that CVO in Hypershift (as well as Hypershift itself) currently don't handle implicitly enabled capabilities, doesn't it mean that we would already be in the state you describe if the DeploymentConfig capability gets removed from vCurrent today, even without the changes proposed in this EP?

---

I'm still processing the suggestions, will probably have more questions once I have fully digested each.

[wking]

Given that CVO in Hypershift (as well as Hypershift itself) currently don't handle implicitly enabled capabilities, doesn't it mean that we would already be in the state you describe if the DeploymentConfig capability gets removed from vCurrent today, even without the changes proposed in this EP?

Yikes, you're right! I've warned the workloads folks here and here.

[flavianmissi]

I've added a section about this under Risks and Mitigations: https://github.com/openshift/enhancements/pull/1729/files#diff-36fbf863a674d8de905f7aa65cdb6158ec8c27b70d2fe5eed4272ca0a964a730R233

I'm still uncertain about what the correct approach here is. Would be great if you could give us your input @deads2k.

[wking]

Just wiring threads together, David's response seems to be over here.

[sanchezl]

We should be clear about the distinction between the Image Registry Capability vs an Image Registry instance throughout this enhancement.

For example, for the goals:

Suggested change

	* Cluster instance admins can create clusters with the Image Registry disabled
	* Cluster instance admins can re-enable the Image Registry without recreating
	the cluster
	* Components in the hosted control plane are not impacted by the Image Registry
	absence
	* Image Registry related assets such as workloads, storage account and
	pull-secrets are not provisioned in clusters without the Image Registry
	* Cluster instance admins can create clusters with the Image Registry Capability disabled
	* Cluster instance admins can enable the Image Registry Capability without recreating
	the cluster
	* Components in the hosted control plane are not impacted by the absence of the Image Registry Capability.
	* Assets compromising the Image Registry Capability such as workloads, storage accounts, and
	pull-secrets are not provisioned in clusters without the Image Registry Capability.

[flavianmissi]

I agree about being more clear about component vs capability, thanks for pointing that out. In some of your suggestions I did mean to refer to the component though, so I incorporated it partially. PTAL.

[sanchezl]

Suggested change

	clusters without the Image Registry, so that my hosted clusters do not contain
	clusters without the Image Registry Capability, so that my hosted clusters do not contain

[flavianmissi]

I really do mean the component here, not the capability. Is there anything else I can do to improve clarity here? 🤔

[wking]

nit: in this enhancement context, you might want a local link, like [cluster capabilities](/enhancements/installer/component-selection.md), because that will have more technical/design context than the admin-facing docs. You have that already in see-also, but I'm not sure what additional context the version-pinned admin docs are adding.

[wking]

I'm not particularly expressive in XValidation:rule, but "size got smaller" is leaning hard on "the only allowed value is ImageRegistry" to ensure the new value is a subset of the old. E.g. if a future enum allows both ImageRegistry and NewCap, this rule would allow someone to replace disabledCapabilities: ["ImageRegistry"] with disabledCapabilities: ["NewCap"], when we'd like that blocked (because we can't disable the previously-enabled NewCap). So this is fine for now, but will probably need refining if we ever expand the enum. And since we think expanding the enum is likely, if anyone has ideas about how we could be more explicitly checking for subset-ness, we could pivot this rule now too.

[tjungblu]

I think we're adding those business rules into the cluster-service as the entry point, no need to handle this on the hosted cluster spec

[flavianmissi]

Ah, yes that was a bit optimistic of me wasn't it 😅
I'm not an expert either. After some research I couldn't find a way to check if a list is a subset of another using CEL. This is what I found on list operations, so not much.

I think we're adding those business rules into the cluster-service as the entry point, no need to handle this on the hosted cluster spec

Do users need to go through the clusters-service API to edit the HostedCluster object of a running cluster?

[tjungblu]

as per their architecture documentation [1], yes.

[1] https://docs.google.com/presentation/d/1aM_CRkC2NL-aTDv7wQRjatv8EXoacV7vdcOCPo_9sr8/edit#slide=id.g2f160991407_0_7

[wking]

Managed Hosted ROSA customers, and eventually Hosted ARO customers too, will not have direct access to HostedCluster. But I thought we also ship HyperShift via the multi-cluster engine operator and in that case we do expect customers to directly manipulate HostedCluster spec (e.g. see these HostedCluster update docs).

In the cluster-version operator, we don't enforce our cannot-be-uninstalled constraint via XValidation:rule though, and we don't punt that to higher-level components who we hope will prevent cluster admins from asking for unsupported things. Instead, we read in the spec (which has some non-exhaustive XValidation:rule guardrails), and then provision for things like the CapabilitiesImplicitlyEnabled condition where we can say things like "you know that thing you're asking for? We're not doing it entirely that way, because...". Perhaps you could do something like that in the HostedCluster and HostedControlPlane controllers?

[openshift-ci]

@flavianmissi: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[deads2k]

is this simply a matter of including the information about capabilities in a manifest in the payload for consumption by the CVO? We do something similar for featuregates today.

[wking]

No, implicit capabilities depend on the past state of the cluster compared with the current target release. We already have all the information we need about the current target release. And we can't bake the past state of the cluster (which is cluster-specific) into release images.

Oh, but yes, HyperShift could come up with some non-ClusterVersion way to track implicitly-enabled capabilities, if it gets taught how to calculate them. And we could pass a ClusterVersion manifest through to the CVO to steer it in the direction we want it to go (via a volume-mounted ConfigMap or some such?), if we wanted the HostedControlPlane <-ClusterVersion-> CVO communication to not flow through the hosted-Kube-API.

[wking]

nit: Drop the dangling "This"?

[tjungblu]

fyi, I've already created the API changes in openshift/hypershift#5415