OPRUN-4133: OLMv1 single/own namespace install mode support #1849
Conversation
anik120
force-pushed
the
olmv1-single-own-namespace
branch
from
c9b7c0f to
ac0ba97
Compare
anik120
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@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. DetailsIn 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
force-pushed
the
olmv1-single-own-namespace
branch
from
ac0ba97 to
8d51ffd
Compare
anik120
left a comment
anik120
left a comment
There was a problem hiding this comment.
@perdasilva addressed your feedback, PTAL
| // | ||
| // inline must be set if configType is 'Inline'. | ||
| // | ||
| // +kubebuilder:validation:Type=object |
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
anik120
force-pushed
the
olmv1-single-own-namespace
branch
from
8d51ffd to
a93ffa3
Compare
| - Re-introducing multi-tenancy features or supporting multiple installations of the same operator | ||
| - Supporting MultiNamespace install mode (watching multiple namespaces) | ||
| - Modifying the fundamental OLM v1 architecture or adding complex multi-tenancy logic | ||
| - Supporting install mode switching after initial installation as a first-class feature |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@perdasilva could you chime in here (I'm going off of Per's instruction to include this point here)
There was a problem hiding this comment.
I think there's a distinction that could be better worded here. Supporting install mode switching could mean a couple of things:
- you can change the value of/remove watchNamespace
- 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.
everettraven
left a comment
everettraven
left a comment
There was a problem hiding this comment.
Some general high-level comments/questions.
Nothing that I would block merging of the EP on.
Thanks for bearing with me and making all the updates you did.
I'm looking forward to promoting this feature to GA in 4.22 :).
/lgtm
| ### Workflow Description | ||
|
|
||
| The standard OLMv1 installation and upgrade workflows stay largely the same. Where it will differ is: | ||
| - users will need to consult the [OpenShift documentation](https://docs.redhat.com/en/documentation/openshift_container_platform/4.20/html-single/extensions/index#olmv1-deploying-a-ce-in-a-specific-namespace_managing-ce) to understand how packages/bundles can be configured. The OpenShift documentation will include a capability matrix (derived from the table in the "registry+v1 Configuration Schema" section below) that maps bundle install mode support to configuration requirements, specifically: |
There was a problem hiding this comment.
Just a note that the currently linked documentation does not explain the new configuration interface and instead relies on an annotation-based API for configuring the watch namespace.
We should ensure this documentation is updated to reflect the current state of the feature usage and how an end-user determines what is and is not valid to put in the configuration.
|
|
||
| OLM's JSON Schema validator will support several custom formats that bundle authors can opt-in to use in their configuration schemas: | ||
|
|
||
| - `namespaceName`: Will validate that the input is a valid Kubernetes namespace name (follows DNS-1123 subdomain format). This will be useful for any configuration field that accepts a namespace name. |
There was a problem hiding this comment.
No strong preference here, but just wanted to note that the format for a Kubernetes namespace is well known and there is a concrete regular expression that can be used in a pattern validation.
Might save you some overhead in custom validator code if the recommendation is for bundle authors to just use that regex instead of a custom validation format.
| - [ ] Documentation for configuration and usage | ||
|
|
||
| ### Tech Preview -> GA | ||
| - [ ] 1 OCP release of alpha feedback |
There was a problem hiding this comment.
Just a general pulse, has the feature in it's current state with the opaque configuration field been shipped in a TPNU release yet?
The 4.20 documentation referenced earlier in this EP seemed to use an annotation-based API for the watch namespace configuration.
There was a problem hiding this comment.
That's a good call out. It would be going out TPNU for 4.21. It was using an annotation-based configuration in 4.20, I believe. I'll talk with Mike to ensure we get gud docs for this in for 4.21.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven, oceanc80 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
| - User does not provide required configuration fields | ||
| - User provides configuration keys that are not existent | ||
| - User provides configuration but bundle does not provide a configuration schema - note that | ||
| while for registry+v1 bundles this is deemed a good level of restriction to ship the configuration feature with initially, for future bundle formats this restriction may be loosed (eg OLMv1 could allow users to provide configuration for "Helm bundles" without any configuration schema). Users will likely be required to express their intent to configure such bundles explicity and have OLM install anyway, possibly via a ClusterExtension API field (eg `dangerouslyAcceptBundlesWithoutConfigSchema`). Such considerations however are for future considerations. |
There was a problem hiding this comment.
Do we have an idea where in the API we'd put knobs like this? Do we need to make sure there is space in the spec.config struct since this is a config-specific knob? Or do we envision putting this kind of thing directly in .spec?
There was a problem hiding this comment.
probably around the install configuration like respecting upgrade graph, etc.
|
|
||
| *Mitigations*: | ||
| - Don't give read access to ClusterExtensions, or ClusterExtensions with privileged information to anyone without access to the secret information. | ||
| - Don't install the application and contact the supplier and ask for a change in the configuration s.t. a secret reference can be given instead. This also solves the replication issue and ensures no additional copies of the information exist on the cluster between the source (Secret) and the point of use. |
There was a problem hiding this comment.
Is there somewhere we would be able to document this for operator authors? Or maybe implement a linter that checks configuration schemas for fields that look like they are named in a way that would contain inlined secrets?
There was a problem hiding this comment.
Not crucial right now, since we don't support bundles with embedded schemas, but something we would ideally get ahead of to avoid these problems in the first place.
| Because we are enabling namespace-scoped operator installations, there are operational implications that could impact cluster management. These risks are mitigated by: | ||
|
|
||
| - **RBAC Misconfiguration**: Install mode validation ensures operators only receive permissions appropriate for their scope | ||
| - **Namespace Dependency**: Clear error messages when target namespaces don't exist or aren't accessible |
There was a problem hiding this comment.
Is there any other mention of erroring when the target namespace doesn't exist? I suppose at a minimum we would fail at the point of trying to create Role/RoleBinding in that namespace? Is that what we're talking about here, or are we talking about a proactive check that happens separate from the normal flow of applying resources?
What if the CSV contains no permissions, which would mean we wouldn't actually need to create anything in the watch namespace (iirc)?
There was a problem hiding this comment.
The installation would probably work. But it wouldn't be useful in any meaningful way. We probably need to think about checking these kinds of dependencies during the runtime though. I'll add a note to say, when there are bundle generated resources that should be applied to the watchNamespace
There was a problem hiding this comment.
And to be clear, I don't think we should preflight check this. If the namespace exists now, but then is deleted in the future, we'll have already installed and we'll run into the issue of "can't create resource X in namespace Y" anyway. And we won't want to have a preflight check that fails in the future cause us to not be able to reconcile unaffected objects in other namespaces.
|
|
||
| **Installation Success Rate:** | ||
|
|
||
| * **RBAC Validation Complexity:** Namespace-scoped installations require more complex RBAC validation to ensure the ServiceAccount has appropriate permissions for the target namespace. RBAC misconfigurations that work in AllNamespaces mode may fail in Single/OwnNamespace modes. |
There was a problem hiding this comment.
Is there a particular OLMv1-specific concern here? Or are we trying to reiterate something about the existing complexity of registry+v1 bundles in general?
There was a problem hiding this comment.
I think more of the latter. Just calling out RBAC gets more complex for these install modes
|
|
||
| **Installation Time:** | ||
|
|
||
| * **RBAC Generation Complexity:** Converting CSV `permission` to namespace-scoped Role/RoleBinding resources requires additional processing time. Complex operators with extensive permission requirements will see increased installation duration. |
There was a problem hiding this comment.
This is not true, is it?
- When watching all namespaces: Create a ClusterRole/ClusterRoleBinding for each set of
permissions - When watching a single namespace: Create a Role/RoleBinding in the watched namespace for each set of
permissions
Either way the computational complexity is identical, is it not?
There was a problem hiding this comment.
Yeah, let's remove this. I don't think the install time difference would be measurable/noticeable in any meaningful way
| **Operator Availability:** | ||
|
|
||
| * **ServiceAccount Permission Dependencies:** Namespace-scoped operators depend on ServiceAccount permissions that may be modified by namespace administrators, creating additional failure points not present in cluster-scoped installations. | ||
| * Example: Namespace admin removes critical RoleBinding, causing operator to lose access to required resources. |
There was a problem hiding this comment.
Wouldn't OLMv1 re-create this RoleBinding?
There was a problem hiding this comment.
Also, how is this different than a cluster admin deleting a ClusterRoleBinding when an operator is watching all namespaces?
Signed-off-by: Per Goncalves da Silva <[email protected]>
|
@joelanford PTAL |
|
@anik120: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/lgtm |
9 participants
No description provided.