✨ Promote Webhook FeatureGates to GA (OPRUN-4098)

[camilamacedo86]

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 perdasilva 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

[netlify]

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	af47a95
🔍 Latest deploy log	https://app.netlify.com/projects/olmv1/deploys/68f26adfaa2b590008bbf1f4
😎 Deploy Preview	https://deploy-preview-2267--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 70.18%. Comparing base (292c0db) to head (af47a95).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2267      +/-   ##
==========================================
- Coverage   73.00%   70.18%   -2.83%     
==========================================
  Files          88       88              
  Lines        8743     8743              
==========================================
- Hits         6383     6136     -247     
- Misses       1947     2192     +245     
- Partials      413      415       +2     

Flag	Coverage Δ
e2e	45.23% <ø> (+5.81%)	⬆️
experimental-e2e	14.30% <ø> (-32.50%)	⬇️
unit	58.11% <ø> (ø)

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tmshort]

/hold
We can't have both feature gates enabled simultaneously, as one is meant for upstream (WebhookProviderCertManager), and the other is meant for downstream (WebhookProviderOpenshiftServiceCA).

[tmshort]

I think my preference would be to leave the feature-gates off by default, and put just the one we want into the helm/olmv1/values.yaml file, but this would mean also leaving them in the experimental.yaml file as well, which is not desirable (i.e. have it in two places). The alternative is to use an if/.Values.openshift.enabled/else sequence to separate the two flags.

BUT, the strangeness comes from the fact that we have cluster-olm-operator getting feature-gates from Openshift. I have a downstream change that would "remove" all upstream gates from values.yaml, and that might be needed before this is merged downstream.

BUT, another strangeness occurs because helm does not merge/combine lists; they are overwritten.

I need to think about this some more.

[camilamacedo86]

/hold

Until we decide how to do

[tmshort]

That we have two feature-gates that can't be enabled simultaneously is causing us this headache (this should not be an issue with 99%of other features).

Although the CertManager one takes priority:

operator-controller/cmd/operator-controller/main.go

Lines 517 to 524 in 1d685b1

	func getCertificateProvider() render.CertificateProvider {
	if features.OperatorControllerFeatureGate.Enabled(features.WebhookProviderCertManager) {
	return certproviders.CertManagerCertificateProvider{}
	} else if features.OperatorControllerFeatureGate.Enabled(features.WebhookProviderOpenshiftServiceCA) {
	return certproviders.OpenshiftServiceCaCertificateProvider{}
	}
	return nil
	}

Which I guess is a reasonable default. But it means we have to explicitly turn off CertManager to get OpenshiftServiceCA, and we don't really have that capability downstream.

Due to the downstream mapping of feature-gates, we only handle "enabled" flags (i.e. we can't turn things off), and we couldn't turn things off without a way in the helm chart to do so (i.e. additional upstream effort).

And disabling features just sounds wrong to me. I really think we need to enable features.

Add in the fact that helm doesn't merge lists makes this more challenging.

I'm envisioning a number of possible solutions, none of which I really like, so I'm wondering if this is something to be discussed in a meeting (office hours)?

[tmshort]

Options?

1. Keep these feature-gates off, explicitly turn on CertManager upstream through a new standard.yaml file, which means that experimental.yaml would also need to include it. Downstream, the OpenshiftServiceCA value would be enabled via the Openshift feature-gates.

1a. Add a new "standardFeatures" list that parallels the existing helm feature lists so that the value doesn't have to be duplicated in both standard.yaml and experimental.yaml. (this is a bunch of effort to avoid duplication of a value in a list).

2. Change the feature-gates to be on, but then add upstream and downstream and cluster-olm-operator mechanisms to be able to disable the feature gates. This would be a lot of additional work.

3. Add a new --webhook-provider option to operator-controller to select between CertManager and OpenshiftServiceCA (the default would be no provider). This effectively replaces the feature-gates with a tri-state, and would be enabled in the chart via .Values.openshift.enabled or .Values.certManager.enabled settings.

[tmshort]

There is no default certificate provider in OLMv1. We provide explicit options to turn on and use cert-manager and/or Openshift serviceca, and I think we need to be able to reflect that.

Some fairly small changes would be needed to make the --certificate-provider a true tri-state.

[camilamacedo86]

Hey folks 👋

@perdasilva @tmshort

I’ve updated this PR — it now reflects what we discussed.
It looks good to merge once we get the green light from you and downstream is ready to receive it.

Please feel free to review when you get a chance 🙏

[michaelryanpeter]

Some suggestions for clarity. Feel free to apply what makes sense and disregard any comments that feel off topic.

[michaelryanpeter]

Suggested change

	OLMv1 supports the installation of bundles containing webhooks by default.
	The controller uses the `WebhookProviderCertManager`
	feature-gate unless you override it. To switch to the OpenShift Service CA provider,
	start the controller with `--feature-gates=WebhookProviderCertManager=false` and enable `--feature-gates=WebhookProviderOpenshiftServiceCA=true`.
	OLM v1 supports installing bundles that use admission webhooks.
	By default, OLM v1 uses the community Cert Manager package for admission webhook validation. To use the OpenShift Service CA provider, set the `--feature-gates=WebhookProviderOpenshiftServiceCA=true` flag at startup.

[camilamacedo86]

We have more than one kind of webhook that is supported.
certManager is used only for the admission webhook
So, I think it would not be very accurate.

The other suggestion was accepted :-)

[michaelryanpeter]

I'm confused, isn't that what I wrote?
"By default, OLM v1 uses the community Cert Manager package for admission webhook validation."

(I'm totally fine if you don't take the suggestion, but I thought we are both saying the same thing?)