Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Single/Own Namespace Install Mode Support #1724

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

perdasilva
Copy link
Contributor

@perdasilva perdasilva commented Feb 6, 2025

Description

Part of #593
Closes #1751 and #1752

Adds initial implementation of SingleNamespace and OwnNamespace install mode support for registry+v1 bundle behind SingleOwnNamespaceInstallSupport alpha feature-gate.

The user can install a registry+v1 bundle in SingleNamespace or OwnNamespace install mode by setting the watch namespace through the olm.operatorframework.io/watch-namespace annotation on the ClusterExtension, as long as the following criteria is met:

  • bundle supports Single- and/or OwnNamespace install modes
  • SingleOwnNamespaceInstallSupport feature-gate is enabled on the controller
  • olm.operatorframework.io/watch-namespace annotation points to a single existing namespace

The annotation value will determine the install mode:

  • empty: bundle is installed in AllNamespaces mode
  • valid namespace name and is equal to the value the install namespace of (.spec.namespace) in the ClusterExtension: bundle is installed in OwnNamespace mode
  • valid namespace name and the value is not equal to the install namespace (.spec.namespace) in the ClusterExtension: bundle is installed in SingleNamespace mode

If the annotation is not present, standard behavior applies: AllNamespaces mode.
If annotation value is not a valid namespace name the installation will halt with an error.

Notes:

  • In order to facilitate testing, I've modified the applier adding the bundleToChart conversion function as a memeber (this way we can mock it). I've only tested the integration of this function into the applier. In a follow up PR mode robust testing will be applier to the conversion package to (further) ensure we're accurately rendering registry+v1 bundles.
  • I've also removed the context parameter since it only served to pass a logger that used to log an error when closing the manifest file. To me, it didn't seem worth it. Though I'd be ok with reverting that if there are any objections.

DEMOS

SingleNamespace

asciicast

OwnNamespace

asciicast

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 6, 2025
Copy link

netlify bot commented Feb 6, 2025

Deploy Preview for olmv1 ready!

Name Link
🔨 Latest commit dfa7040
🔍 Latest deploy log https://app.netlify.com/sites/olmv1/deploys/67b446dc4f570000083eea67
😎 Deploy Preview https://deploy-preview-1724--olmv1.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Feb 6, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 3 lines in your changes missing coverage. Please review.

Project coverage is 68.49%. Comparing base (82fbaf2) to head (dfa7040).

Files with missing lines Patch % Lines
internal/operator-controller/applier/helm.go 70.00% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1724      +/-   ##
==========================================
+ Coverage   68.37%   68.49%   +0.11%     
==========================================
  Files          63       64       +1     
  Lines        5117     5132      +15     
==========================================
+ Hits         3499     3515      +16     
+ Misses       1389     1388       -1     
  Partials      229      229              
Flag Coverage Δ
e2e 51.63% <51.85%> (-0.08%) ⬇️
unit 56.02% <77.77%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@joelanford
Copy link
Member

Wow, I am impressed with how simple that is with the existing code we have.

@LalatenduMohanty
Copy link
Member

@joelanford @perdasilva Are we planning to write a RFC for this feature Single own namespace. Also Is this PR related to #593 in some way. In that case, can we update the epic description too.

@perdasilva perdasilva changed the title [WIP] ✨ Single own namespace [WIP] ✨ Single/Own Namespace Install Mode Support Feb 6, 2025
@perdasilva perdasilva force-pushed the single-own-namespace branch 11 times, most recently from 8088b37 to 24d0e36 Compare February 12, 2025 15:11
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2025
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2025
@perdasilva perdasilva changed the title [WIP] ✨ Single/Own Namespace Install Mode Support ✨ Single/Own Namespace Install Mode Support Feb 13, 2025
@perdasilva perdasilva marked this pull request as ready for review February 13, 2025 09:19
@perdasilva perdasilva requested a review from a team as a code owner February 13, 2025 09:19
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 13, 2025
name: argocd-operator
annotations:
# watch-namespace is different from install namespace
olm.operatorframework.io/watch-namespace: argocd
Copy link
Contributor

@camilamacedo86 camilamacedo86 Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Today, if the Operator is using a single-namespace or own-namesapce mode, the namespace is already defined in the bundle-registry-v1 as an environment variable (e.g., falcon-operator).

  • Could we not simply retrieve the ENV VAR and use it in those modes instead of need a new annotation? Why do we need a new annotation?
  • Do we need the annotation because we want to allow users to switch between modes dynamically?

Copy link
Contributor Author

@perdasilva perdasilva Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that's necessarily a single-/own-namespace specific thing. I think it's how operators are configured for one install mode or another (i.e. when OLMv0 stamps out the deployments it will set that ENV VAR to the value of the namespace's operatorgroup's target namespaces. set the olm.targetNamespaces annotation to the value in the namespace's OperatorGroup)

What I think we may want to think about in the next iteration is verifying that the bundle supports the requested install mode. (edit: I think it's already the case anyway [ref])

Copy link
Contributor Author

@perdasilva perdasilva Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the second point: in a way, yes. But the way to frame it is that we're treating this as a kind of an install-time configuration and don't want to necessarily block the user from doing it. I still think this needs extra discussion, since allowing configuration change at the API level != the runtime allowing the installation to continue with a given configuration (and the check may or may not take into account the current state of the installation and the runtime may or may not allow a new configuration through depending on the current state of the operator). I think this will be explored more in-depth when we RFC the API changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@camilamacedo86 camilamacedo86 Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what I thought. Couldn't this be handled by the feature gate, where we perform the support check?

  • If the feature is enabled, it sets the namespace and doesn’t return an error.
  • If not, it continues working as it does today.

I do not remember it anymore, how we deal with it in OLMV0
When we change in the console the target namespace do we update the CSV?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the watch namespace is a parameter for the user to choose. I don't quite understand the suggestion. You might be thinking about the default (or suggested) namespace. That's some metadata on the CSV that can be used by console as the value for the install namespace to save the user having to input something. But, the user can still set the value.

In v0 to install an operator you need to:

  1. create the (install) Namespace
  2. create the OperatorGroup
  3. create the Subscription

The console basically automates those steps and uses the default namespace by default.

The OperatorGroup's .spec.targetNamespaces will then go into the olm.targetNamespaces annotation on the controller pod. The env var is then set using the downward api.

My guess is that the console also gives you different installation options depending on the install mode support. If you want to install it in SingleNamespace, you'll need to name the namespace (not sure if the console will also create it if it doesn't exist - I'd say so though). If the user wants OwnNamespace, then console will use the install namespace. If AllNamespace, then it will leave .spec.targetNamespace empty. If MultiNamespaces, then the user will probably have to enter in a list (or select from the list of existing namespace, I'm not sure).

The point is, both install and watch (or target) namespace(s) are user inputs. Does that help clarify everything?

Copy link
Contributor

@camilamacedo86 camilamacedo86 Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do not modify this part of the code, then I understand that:

  • If we continue using the same index that includes multiple operators targeting SingleNamespace or OwnNamespace modes,
  • And I attempt to install those operators, they will fail,
  • The current workaround would be to manually add the annotation in the Extension to make it work.

Given this, I believe we also need to modify this validation, right?
We should introduce a change that allows support for both SingleNamespace and OwnNamespace modes, gated under a feature flag to ensure compatibility something like:

func validateTargetNamespaces(supportedInstallModes sets.Set[string], installNamespace string, targetNamespaces []string) error {
	set := sets.New[string](targetNamespaces...)
	switch {
	case set.Len() == 0 || (set.Len() == 1 && set.Has("")):
		// Default behavior: Only supports InstallModeTypeAllNamespaces
		if supportedInstallModes.Has(string(v1alpha1.InstallModeTypeAllNamespaces)) {
			return nil
		}

		// Allow SingleNamespace and OwnNamespace if the feature flag is enabled
		if features.OperatorControllerFeatureGate.Enabled(features.SingleOwnNamespaceInstallSupport) {
			if supportedInstallModes.Has(string(v1alpha1.InstallModeTypeSingleNamespace)) ||
				supportedInstallModes.Has(string(v1alpha1.InstallModeTypeOwnNamespace)) {
				return nil
			}
		}

		return fmt.Errorf("supported install modes %v do not support targeting all namespaces", sets.List(supportedInstallModes))

....
}

Could we change it now OR do we need to change the API?

@@ -79,7 +80,7 @@ func shouldSkipPreflight(ctx context.Context, preflight Preflight, ext *ocv1.Clu
}

func (h *Helm) Apply(ctx context.Context, contentFS fs.FS, ext *ocv1.ClusterExtension, objectLabels map[string]string, storageLabels map[string]string) ([]client.Object, string, error) {
chrt, err := convert.RegistryV1ToHelmChart(ctx, contentFS, ext.Spec.Namespace, []string{corev1.NamespaceAll})
chrt, err := h.buildHelmChart(contentFS, ext)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 I see what you're doing here.

We now call RegistryV1ToHelmChart to handle the conversion and set it in the structure.
Then, after the check/convention, we override the watch namespace based on the annotation.

However, if the CVS is set to single namespace or own namespace, the bundle will still fail, right?
We're only allowing the switch to single/own namespace at runtime via the feature flag, but we still cannot manage/applying a bundle when the CVS is explicitly set to single/own namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The install mode validation is based on the install and watch namespace inputs. If the user tries to install in SingleNamespace mode but only OwnNamespace mode is supported, validation will fail.

The logic is:

  1. take registry+v1 bundle filesystem (with CSV and all other bundle resources), install namespace, and watch namespace
  2. generate a RegistryV1 representation
  3. feed that to Convert along with the install and watch namespace. This method generates all the plain manifest resources needed to install the bundle in the install namespace and watching the given install namespace ("" -> watch all namespaces). This includes parsing the CSV into the controller deployment(s), service account(s), and RBAC scoped depending on the install mode. This method also does all the validation to ensure that the install mode is supported by the bundle, and that it doesn't include any (currently) unsupported resources (e.g. Webhooks).
  4. the set of plain manifests is transformed into a Helm Chart
  5. Helm Chart is installed/upgraded.

The Apply method doesn't have any concept of watch namespaces. It just takes the ClusterExtension. Right now, we take the watch namespace input from the annotation. In the near future we'll take it directly from the ClusterExtension spec.

The CSV isn't set to one install mode or another. It just declares which install modes it supports. The value of watch namespace is what decides the install mode at install time. Does that make sense?

Per Goncalves da Silva added 6 commits February 18, 2025 09:37
Signed-off-by: Per Goncalves da Silva <[email protected]>
Signed-off-by: Per Goncalves da Silva <[email protected]>
Signed-off-by: Per Goncalves da Silva <[email protected]>
@perdasilva
Copy link
Contributor Author

@camilamacedo86 the comments I don't mark as resolved I'm leaving open to make sure you understand what's going on. If you're good, please mark them as resolved.

},
}

_, _, _ = helmApplier.Apply(context.TODO(), validFS, testExt, testObjectLabels, testStorageLabels)
Copy link
Contributor

@camilamacedo86 camilamacedo86 Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add a test here to ensure that if it is a MultiNamespace, it fails when the flag is enabled?
I think we also need add the validation to fail on this case right?

Comment on lines +113 to +116
- type: SingleNamespace
supported: true
- type: OwnNamespace
supported: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not those still fail when the flag is not used and only work when the feature flag is used?

@LalatenduMohanty
Copy link
Member

The user can install a registry+v1 bundle in SingleNamespace or OwnNamespace install mode by setting the watch namespace through the olm.operatorframework.io/watch-namespace annotation on the ClusterExtension, as long as the following criteria is met:
bundle supports Single- and/or OwnNamespace install modes
SingleOwnNamespaceInstallSupport feature-gate is enabled on the controller
olm.operatorframework.io/watch-namespace annotation points to a single existing namespace

@perdasilva Does OLM version update impacts this i.e. OLM v1.1 has SingleOwnNamespaceInstallSupport feature-gate and user is updating to OLM v1.2 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add alpha feature gate
5 participants