OPRUN-4133: OLMv1 single/own namespace install mode support

[anik120]

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 assign coverprice for approval. 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:

• OWNERS

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

[openshift-ci-robot]

@anik120: This pull request references OPRUN-4133 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.21.0" version, but no target version was set.

In response to this:

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 openshift-eng/jira-lifecycle-plugin repository.

[anik120]

@perdasilva addressed your feedback, PTAL

[perdasilva]

It might be worth calling out that adding this annotation ensures that inputs that are valid json, but not key/value config, are rejected. E.g. something like just true is valid JSON (a boolean), but it wouldn't be valid configuration. Enforcing an object ensures that we have something that resembles a configuration/values files, e.g. key: value.

We should also think about empty object mechanics here, e.g. {}. Maybe that should be treated as no configuration and defaults used (cc @joelanford wdyt?).

[anik120]

Added a comment about the key/value config.

We should also think about empty object mechanics here, e.g. {}. Maybe that should be treated as no configuration and defaults used

Or should we prevent empty objects in the first place?

[perdasilva]

That's my initial instinct. I just don't know if there's an use-case where you'd want to specifically set an empty configuration. I can't think of one, but that doesn't mean there isn't one XD

[perdasilva]

I've updated the doc strings upstream, so we may want to update this section a little for those changes (although I'm still waiting on an approval for that PR) ref

Or, just put a link to the upstream definition at main and say that this excerpt is at the time or writing and that doc strings may vary (but the structure itself it this - unless new types are added)

[joelanford]

Since we are treating this as configuration, I think we would support install mode switching, right? Because, more generally, we're allowing configuration to be changed.

[anik120]

@perdasilva could you chime in here (I'm going off of Per's instruction to include this point here)

[perdasilva]

I think there's a distinction that could be better worded here. Supporting install mode switching could mean a couple of things:

1. you can change the value of/remove watchNamespace
2. that change will work

I think 1 is in scope and 2 is not. It would be up to the author to make that happen, or to be able to recover from that switch. OLMv1 will just compute the correct resources and pivot towards them.

[joelanford]

Terminology is important. Correct me if I'm misinterpreting. Resolution is the process by which we choose a bundle to install/upgrade to. The process of consuming the configuration and applying to the bundle in order to derive plain manifests happens after resolution. The resolver is not aware of the bundle's configuration schema and will not try to choose a bundle based on valid alignment of watchNamespace and install mode support.

Suggested change

	1. Validate bundle compatibility with the requested install mode during resolution
	1. Validate bundle compatibility with the requested install mode after resolution

[anik120]

I wasn't thinking about dependency resolution here. What I meant was "reconciliation". Updating the line with "during reconciliation", wdyt?

[anik120]

@perdasilva added a new commit to incorporate operator-framework/operator-controller#2283, PTAL.

[perdasilva]

/lgtm

[everettraven]

While I get the appeal of a configuration option like this - and it could make sense when you have arbitrary configuration on a case-by-case basis (like if you used a Helm values file) - I'm not quite sure this particular case is best suited for an arbitrary configuration object field.

My biggest concern here is that for the primary use case you've called out here, you've made a configuration option that is entirely validated at runtime instead of at admission time. This gives a delayed feedback cycle and is generally a worse user experience.

This also seems more like an "installation" configuration as opposed to your future cases which I see more like "operand" configurations. Put more simply, this reads as a "install the $thing in this way" vs "pass this configuration to $thing".

Have you considered providing a more explicit configuration field for this legacy install mode concept?

I suspect you'll end up having to carry this around for a while, but you may be able to influence people to use the more modern installation approach by literally naming the configuration field as such.

I recall there is an existing .spec.install field in the ClusterExtension API. As an example, what if you did something like:

spec:
  install:
    legacyConfig:
      mode: SingleNamespace
      singleNamespace:
        watchNamespace: something

and

spec:
  install:
    legacyConfig:
      mode: OwnNamespace
      # no discriminant because OwnNamespace requires installation namespace.

AllNamespace, being the desired long-term default can be what the default behavior is if legacyConfig is not specified.

This pattern allows you to be more explicit up front as to what is and is not a valid installation configuration for the chosen legacy installation mode.

You may still end up with runtime side validations that need to happen during bundle resolution to determine whether or not there is a valid bundle that supports the chosen installation option, but you limit it to that instead of all your validations becoming runtime validations.

[everettraven]

Another thought to layer on top here, this type of configuration also allows you to make this legacy installation mode option something that is considered in bundle resolution.

Instead of selecting a bundle to install and then identifying that it doesn't support this installation mode you can include the installation mode in your resolution criteria, returning a resolution error if no bundles match those criteria.

[perdasilva]

Yeah, we're aware of the runtime validation considerations of this approach. It also wasn't our favorite. But, we'd like to treat these concerns as configuration and not really have the user thinking in terms of legacy/new, registry+v1, helm, registry+vx, etc. or have peculiarities of those different formats bleed through the interface. Once we have the new bundle format, it will likely look at lot more like the Helm case, anyway. We're also trying to be careful and avoiding importing v0 verbiage into v1 - especially "install modes" which dovetails with v0's multi-tenancy feature (which we are trying to stay as far as possible from). Ultimately, from the product's perspective, configuration is defined by the bundle through an optional schema that is validated at runtime, and registry+v1 is just another bundle format.

[perdasilva]

This also seems more like an "installation" configuration as opposed to your future cases which I see more like "operand" configurations.

We may have poorly expressed our intention here, but I don't think we'll ever have operand configuration. Bundle configuration will always be an installation concern. The configuration is used to mutate the set of manifests that compose the application in some author defined way.

e.g. our nearest future case is SubscriptionConfig like configuration surface that can be used to modify the operator deployment manifest (add volumes, env vars, etc.)

[everettraven]

We may have poorly expressed our intention here, but I don't think we'll ever have operand configuration. Bundle configuration will always be an installation concern. The configuration is used to mutate the set of manifests that compose the application in some author defined way.

Sure. I think the "operand" vs "install" configuration is probably something I got hung up on and isn't actually applicable here. I can see the appeal of having a singular "configuration" field here for UX simplicity.

Yeah, we're aware of the runtime validation considerations of this approach. It also wasn't our favorite. But, we'd like to treat these concerns as configuration and not really have the user thinking in terms of legacy/new, registry+v1, helm, registry+vx, etc. or have peculiarities of those different formats bleed through the interface. Once we have the new bundle format, it will likely look at lot more like the Helm case, anyway. We're also trying to be careful and avoiding importing v0 verbiage into v1 - especially "install modes" which dovetails with v0's multi-tenancy feature (which we are trying to stay as far as possible from). Ultimately, from the product's perspective, configuration is defined by the bundle through an optional schema that is validated at runtime, and registry+v1 is just another bundle format

My confusion here is that you are saying that you want the configuration schema to be handled by the bundle, but OLM is the package manager. It defines the pattern in which things are packaged and as such you likely have control over how that configuration is handled.

If you know ahead of time the bundling formats you are going to support - and thus the general structure that those blobs will take - why not include those as explicit configuration formats that can be validated earlier in the loop where you can?

In this initial use case, you have a prime example of knowing installation scenarios where the watchNamespace is/is not required. You can enforce that behavior instead of having a swath of new runtime failure modes.

I still think something like:

spec:
  config:
    type: LegacySubscription
    legacySubscription:
      mode: SingleNamespace
      singleNamespace:
        watchNamespace: somenamespace

gives you the ability to add future configuration formats in the future that allows you to validate early when there is a configuration schema known ahead of time but doesn't preclude you from doing a delayed validation flow in the future when you support a format that doesn't have a known schema ahead of time.

For example, if you later support a Helm values file the shape could evolve to look something like:

spec:
  config:
    type: HelmValues
    helmValues:
      - key: some
         value: thing

where you have arbitrary key-value pairs that you pass into your templating engine.

I also think that the structured configuration approach gives you the ability to return a more targeted error message during bundle resolution if the bundle doesn't support that configuration format. If a user has declared the intent to install a bundle using that configuration it seems odd to me that you may resolve to a bundle that doesn't declare support for that general configuration format and then attempt to configure it.

[everettraven]

It is a core design decision of OLMv1 to ensure that watchNamespace is never a first class field in our API. See https://operator-framework.github.io/operator-controller/project/olmv1_design_decisions/#watched-namespaces-cannot-be-configured-in-a-first-class-api. Putting it in a structured field in the API would make it a first class field. Putting any reference to watchNamespace or installMode in our CRD is a non-starter.

Then you should not be supporting this functionality? If you want to support this functionality, I expect the configuration option to be made available in a supported way.

Discourage it's use all you want, name it Legacy to make users explicitly aware through naming they are using an old pattern. Continue limiting installations to one ClusterExtension per package. Making it a "first class api" doesn't mean you have to roll back your stance on multi-tenancy.

The core tenant of this API design is that bundles will define their contents and configuration schemas.

Do bundles do this today, or is it currently the responsibility of OLM to do the translation?

I think the main flaw of the argument for admission-time validation is that OLM and the user know what the format of the resolved bundle will be prior to resolution occurring, which happens during reconciliation of the ClusterExtension

Just to clarify - I am not fully against runtime validations. I accept that even if you implement admission-time validation there will have to be runtime validation as well.

My pushback here is specifically around having only an opaque configuration, when you don't know if you will support multiple formats, being a poor user experience because a user has no way to know whether or not OLM is going to attempt to install/upgrade in a way that respects that configuration. I outlined two situations above:

1. A user defines the configuration for the initial installation version and a future version of the package uses your latest and greatest bundling format. OLM attempts an automatic upgrade that fails. OLM had no knowledge of whether or not the existing configuration would have been compatible for the upgrade. OLM probably should not have even attempted this upgrade.
2. Another upgrade scenario that shouldn't be possible (not certain whether or not it is with this proposal): A user defined the configuration, OLM does an automatic upgrade, the configuration is not the right structure and gets ignored. User-defined configuration is no longer respected and their workflows break

What are your thoughts on these situations?

Even if we could build an admission webhook that performs resolution and schema validation at admission time, that wouldn't remove the need for runtime validation because:

1. The admission-time resolution might be different than the reconcile-time resolution, which means the admission time validation may be invalid by the time we reconcile and actually resolve.
2. The ClusterExtension can represent a range of versions that may not even exist yet. While it may be technically possible in the future to assess validity of the configuration based on the schemas of all of the bundles that match the filter criteria, it is impossible to assess configuration validity for yet unpublished bundles that will fall into the configured range in the future

I'm not saying you have to go and perform bundle resolution for admission validation, but if you are wanting to be declarative and avoid the scenarios I've outlined above a user defining their configuration format like:

apiVersion: olm.operatorframework.io/v1
kind: ClusterExtension
metadata:
  name: argocd
spec:
  namespace: argocd
  serviceAccount:
    name: argocd-installer
  config:
    type: LegacySubscription
    legacySubscription:
      mode: SingleNamespace
      singleNamespace:
        watchNamespace: argocd-watch
  source:
    sourceType: Catalog
    catalog:
      packageName: argocd-operator
      version: 0.6.0

would essentially mean "Install the argocd-operator package at version 0.6.0 that supports the legacy Subscription configuration format". If OLM doesn't resolve a bundle that matches that criteria, it fails resolution.

Any upgrades would be limited to the same resolution criteria so you never encounter a situation where a previous configuration is no longer respected.

We know from the experience of OLMv0 that restrictive and opinionated configuration schemas defined by the OLM team ultimately cause problems when customers ask extension authors to deliver a new feature related to how the operator itself is configured

No one is asking you to define a restrictive and opinionated configuration schema for OLMv1. If you later support another bundle format you also pick up the configuration schema there. If that new bundle format doesn't have an opinionated schema, neither do you. The configuration schema for the registry+v1 bundle format you support is restrictive and opinionated, so you do have an opinionated configuration stance for that bundle format.

You are probably going to be bound to this until you support a new bundle format.

[everettraven]

I do think that pushing the responsibility of configuration, templating, etc. to the bundle is a reasonable direction forward, but I think you need an answer for what that actually looks like before designing an API around it and how OLM is going to behave in that new world.

To me, it doesn't seem reasonable to have a singular opaque configuration field in a world where you support multiple distinct bundle formats for packaging.

In a world where you support a single bundle format where the configuration process is entirely offloaded to the bundle, I think it is reasonable.

[everettraven]

reject the idea of a discriminated union for different bundle formats because of the major friction point that would add for users to move off of our legacy bundle format and onto Helm

I'm curious, what is the major friction point in switching the configuration in the ClusterExtension API?

[everettraven]

I'm gonna jump off this thread. I don't think this back and forth is solving anything. Let's go the synchronous route. From our perspective, the earlier the better. But, if the best time for you is next Tuesday at the office hours, let's just do that...

Seeing as it is currently Shift Week, next Tuesday would be preferable.

[everettraven]

Had a chat with both @joelanford and @perdasilva to hash this out synchronously.

With a better understanding of the direction you folks are heading and having addressed my concerns about certain scenarios - if you folks feel as though this opaque configuration is what provides the best experience for your users and are ready to support it along with any challenges that come along in the future I won't stop it.

Please make sure that you update the EP as discussed to include:

• An explicit statement about the future you are trying to achieve and how this is the best path to achieving that
• How to ensure customers encountering issues here are getting the right support (i.e who is responsible for what failure modes)
• How users can prevent and/or remediate bad states caused by either:
  • a misconfiguration
  • a scenario where an end user had a valid configuration and an automatic upgrade happens where the configuration is now invalid and blocked
  • a scenario where a user had a valid configuration and an automatic upgrade happens where the previous configuration is no longer respected (i.e their workflows are now broken because their intended configuration is no longer in place)

[everettraven]

Customers do not have direct control of OpenShift features at a granular level. They can choose to flip to an unsupported mode that enables the ones in that level of maturity.

The goal of this section is to understand how, if for some reason, a customer would need to go about downgrading to a version where your feature is no longer enabled by default.

In my experience, for features that involve new opt-in API fields this ends up being something like:

• unset new fields
• downgrade

In your case, I'd include some considerations for how OLM will handle a ClusterExtension using a field it doesn't know about any more on a downgrade and what things a customer doing a downgrade may need to consider before going through with it.

[perdasilva]

I think it's as you describe. Ultimately, if a customer upgrades, makes use of the new field, then decided to downgrade, they will need to unset the new fields.

Going forward, as the registry+v1 configuration surface increases, it would be much of the same. After downgrade you'd get some runtime config schema violations like "unknown field ..." if you don't.

[anik120]

@everettraven @perdasilva rewrote this section PTAL.

Looked at the CRD and looks like there's

inline:
    type: object
    x-kubernetes-preserve-unknown-fields: true

for the .spec.config field.

[everettraven]

Is this the OLM service account or operand service account you are referring to?

If it is the OLM service account this sounds true regardless of namespace-scoped vs cluster-scoped installations because any extension in this context inherently requires stamping out some kind of namespaced resources.

[anik120]

This is the operand's service account.

[everettraven]

How are end-users going to be made aware of these issues?

[anik120]

Correct me if I'm wrong @perdasilva, but the configuration issues are being surfaced in the ClusterExtension in the status section.

For runtime errors, those are Extension concern. OLMv1 will install the ClusterExtension as long as there's no configuration issues. Extensions have to surface any runtime issues, eg namespace unreached due to restricted NP issues etc.

[perdasilva]

That's correct

[everettraven]

It sounds like this is isolated to a singular component managed by a singular cluster operator (cluster-olm-operator) and isn't prone to a state where you may need to deal with one component of the system being on a newer version than another component of the system?

[anik120]

Added a line saying we don't need a strategy because of the reason you mentioned, thanks for the pointer.

[everettraven]

This is just the existing tests no?

[anik120]

That's a good point. Also I just realized that we haven't actually talked about any regression tests yet, so I'm going to leave this section blank instead.

[everettraven]

How so? Is this not just configuring the singular instance of an operator to watch more than a singular namespace but less than the whole cluster?

You don't have to allow more than a single installation just because you support configuring an installation with the ability to watch N namespaces.

[perdasilva]

I think the issue is it starts to give people the impression we're going, or could go, down that way. Even this Single/OwnNamespace was already a bit of a stretch for us and we only did it because there because it was demanded by the business - and I hope will go away in time =S

[everettraven]

You already have a hard stance you will not go down that way?

How much of the possible extensions one might install on the cluster support this installation mode? How many only support this installation mode?

[perdasilva]

You already have a hard stance you will not go down that way?

That's certainly my understanding. This is not something we want to support in v1. We want to distance ourselves from these concerns as fast as possible and give them to the author.

So, maybe we just change the reasoning to reflect that. "OLMv1 does not want to support this use-case as it sees it as a configuration concern defined by the package author"

[everettraven]

Also looks to be some linting issues that need addressing.

[openshift-ci]

New changes are detected. LGTM label has been removed.

[openshift-ci]

@anik120: 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.