✨ Add support for installing bundles with webhooks #1914
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
perdasilva
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1914 +/- ##
==========================================
+ Coverage 66.78% 68.92% +2.13%
==========================================
Files 75 76 +1
Lines 6337 6931 +594
==========================================
+ Hits 4232 4777 +545
- Misses 1841 1874 +33
- Partials 264 280 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openshift-merge-robot
added
the
needs-rebase
perdasilva
force-pushed
the
add-wh-support
branch
from
94e5940 to
af66253
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
openshift-merge-robot
removed
the
needs-rebase
| APIVersion: admissionregistrationv1.SchemeGroupVersion.String(), | ||
| }, | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| GenerateName: fmt.Sprintf("%s-", generateName), |
There was a problem hiding this comment.
Using GenerateName is what OLMv0 does, correct? I think it does this so that multiple validating/mutating webhook configuration objects can be created when own/single namespace bundles are installed multiple times.
That is an anti-goal for OLMv1, so perhaps it makes sense to deviate from OLMv0's use of GenerateName in this case.
Unless there is a really strong reason not to, I'd prefer to use a deterministic name here instead, as that would ultimately make the set of on-cluster objects deterministic as well. I'm sure there are many benefits, but one that comes to mind is that ClusterExtension SAs would be able to specify the deterministic name in advance when setting up RBAC for get/update/patch/delete verbs with the correct resourceName.
There was a problem hiding this comment.
Another benefit: using a deterministic name here means that this webhook can only be installed once, regardless of the install modes supported by the extension. That is directly in line with one of the stated invariants of OLMv1 (no two cluster extensions can manage the same object)
There was a problem hiding this comment.
I've changed it to use Name instead. I'm trying to think if there's foot gun potential. I might be a little more defensive here and ensure that any - suffix is removed from what is defined in the CSV to avoid any bad name errors.
We might already, but I'll double check and ensure that all webhook names defined in the CSV (for a particular type of webhook) are unique.
I guess if there is a naming clash, i.e. two different bundles call their webhook "my-webhook", the user can just install the bundle in another namespace as an escape hatch...wdyt?
There was a problem hiding this comment.
So, now I understand better the @joelanford recommendation/idea: https://redhat-internal.slack.com/archives/C0881N26CGP/p1745960594662879?thread_ts=1745942335.360109&cid=C0881N26CGP
I am OK with since in OLMv1 we should have one instance of each project only the cluster
👍
| @@ -17,6 +19,21 @@ func ObjectNameForBaseAndSuffix(base string, suffix string) string { | |||
| return fmt.Sprintf("%s-%s", base, suffix) | |||
| } | |||
|
|
|||
| func ToUnstructured(obj client.Object) (*unstructured.Unstructured, error) { | |||
| gvk := obj.GetObjectKind().GroupVersionKind() | |||
There was a problem hiding this comment.
A client.Object is not guaranteed to have a populated GVK. If we need guarantees about setting the GVK on the unstructured type, we should also pass in a scheme so that we can lookup the GVK based on the object type.
There was a problem hiding this comment.
There was a problem hiding this comment.
For now, I've just hardened it by checking if version and kind are empty. I don't think we need to add the complexity of passing a scheme. At least not for now =D
There was a problem hiding this comment.
If we want the GKV and use it is the right approach then, I think we should use the controller-runtime implementation to do so ( GVKForObject ): https://github.com/kubernetes-sigs/controller-runtime/blob/main/pkg/client/apiutil/apimachinery.go#L95C6-L95C18
There was a problem hiding this comment.
I don't think we need it right now. The precondition for the function is that the client.Object will carry the GVK. If not, then error. But, I'd say check how we're using it in this PR and let me know if that's not the right way to create an object with GVK.
There was a problem hiding this comment.
Do you think that's acceptable since we only use it with CertManager? That way, we'll have the GKV. Is that what you were thinking?
There was a problem hiding this comment.
I just don't get what it buys us apart from allowing the function to take client.Objects that don't have a gvk set.
I guess my question is, if we do:
ToUnstructured(&certmanagerv1.Certificate{
TypeMeta: metav1.TypeMeta{
APIVersion: certmanagerv1.SchemeGroupVersion.String(),
Kind: "Certificate",
}
...
)
do we get anything out of adding the scheme? Or would adding it just enable us to do something like:
ToUnstructured(&certmanagerv1.Certificate{}, scheme)
or take arbitrary client.Objects at the cost of creating a new scheme and registering the certmanagerv1 scheme to it?
Since we don't have a need for handling arbitrary client.Objects, is there any point in making the method more complex right now?
There was a problem hiding this comment.
Yeah, if we check/fail if GVK is unset, then I think that's sufficient.
perdasilva
force-pushed
the
add-wh-support
branch
from
af66253 to
165086d
Compare
| @@ -13,6 +13,7 @@ const ( | |||
| // Ex: SomeFeature featuregate.Feature = "SomeFeature" | |||
| PreflightPermissions featuregate.Feature = "PreflightPermissions" | |||
| SingleOwnNamespaceInstallSupport featuregate.Feature = "SingleOwnNamespaceInstallSupport" | |||
| WebhookSupport featuregate.Feature = "WebhookSupport" | |||
There was a problem hiding this comment.
If we want to add support for new providers later, how do you envision that looking from a feature gate standpoint?
There was a problem hiding this comment.
From what I understand, downstream will use a different provider, but the implementation remains the same.
I don’t see a strong need to support multiple providers upstream for now — downstream will likely stick with one as well. Since this is under an alpha flag, we have flexibility to adjust later.
So I think this is fine as-is and no need to optimize prematurely. We can revisit if we decide to support more providers in the future.
There was a problem hiding this comment.
I think you are missing my point (which I should have explained a bit better 😅 ). Let's say we implement webhook support now with cert-manager, but then later decide we want to add a separate implementation (e.g. BuiltIn or OPA's cert-controller)?
My point is, I think there is a difference between "we support webhooks" and "we support a certain certificate provider".
I would suggest calling this WebhookProviderCertManager so that if/when we introduce other cert providers upstream, we don't have a strange naming convention where WebhookSupport actually means cert-manager webhook support and WebhookSupportFooBar means "webhook support with the foobar provider".
And then once at least one provider's feature gate graduates to GA, then we can say that we support bundles with webhooks.
I also think we should explore implementing OCP's Service CA provider in our upstream.
That would benefit Red Hat: if Red Hat implements it only downstream, they (we) would have to carry patches in the downstream codebase that could invite conflicts when syncing. There is no precedent yet for carrying a patch in the downstream Go source code, and I'm not convinced it makes sense to change that for this feature.
It would also benefit other users of OCP. For example, if an OCP user wants to disable OCP's OLM and use upstream OLM instead, they would still be able to use the service-ca provider. We've talked about this possibility with SRE organizations that provide managed OpenShift and that want a consistent OLM surface across a fleet of OCP clusters with varying OCP versions.
There was a problem hiding this comment.
Another general benefit: If we use a naming convention like WebhookProvider<ProviderName> for the feature gates, we can directly translate the set of enabled webhook providers to a --webhook-provider flag's presence and the set of allowed values (i.e. the set of <ProviderName> suffixes).
Then, distro maintainers retain the choice of:
- no webhook support (don't set the flag)
- webhook support with whichever provider they prefer (they set the flag to that provider)
All this driven by the feature gate naming convention.
There was a problem hiding this comment.
I've updated the featuregate name
There was a problem hiding this comment.
This is great! My only open question (that I already left in a code comment) is how we would introduce more certificate providers in the future?
Presumably we'd want a feature gate per provider, so that each provider could mature independently? If so, should we have CertManagerWebhookSupport for the current feature gate name?
I don't think we have to answer that question or change what we've got in the PR right now, so approving!
perdasilva
force-pushed
the
add-wh-support
branch
from
0d8545d to
8e6b15e
Compare
openshift-merge-robot
added
the
needs-rebase
perdasilva
force-pushed
the
add-wh-support
branch
from
8e6b15e to
eed9c38
Compare
openshift-merge-robot
removed
the
needs-rebase
perdasilva
force-pushed
the
add-wh-support
branch
from
eed9c38 to
3cebd58
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
There was a problem hiding this comment.
Everything looks great to me! 👍
Awesome work 🥇
Just one thought: I think it would be valuable to have some e2e tests covering install/upgrade scenarios with webhooks.
Do you think we could add those before merging? Or, since the feature is behind a flag, maybe we could make sure to cover that in a follow-up?
Other small nits (nice-to-haves): #1914 (comment)
Aside from that, I'm all good with it.
/lgtm
/approved
|
New changes are detected. LGTM label has been removed. |
perdasilva
force-pushed
the
add-wh-support
branch
2 times, most recently
from
8338dc5 to
7d0dce0
Compare
I think this falls under the discussion around having e2e tests for feature-gated code. I don't know that we've landed on a solid answer yet, and I think we may want a separate e2e suite (maybe?) for FGs? Because of this, I think it will come in a follow-up. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: camilamacedo86 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
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]>
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]>
Signed-off-by: Per Goncalves da Silva <[email protected]>
perdasilva
force-pushed
the
add-wh-support
branch
from
1da5dd5 to
871dae2
Compare
Signed-off-by: Per G. da Silva <[email protected]>
Signed-off-by: Per G. da Silva <[email protected]>
Signed-off-by: Per G. da Silva <[email protected]>
Signed-off-by: Per G. da Silva <[email protected]>
perdasilva
force-pushed
the
add-wh-support
branch
from
d663ee0 to
5e3f09c
Compare
Description
Depends on #1893
Adds webhook support to bundle renderer with certificate management. Based on Joe's PoC #1506
Only the cert-manager provisioner is implemented in this PR. In a follow up, we'll add the openshift-serviceca provisioner and a way to configure OLM for one or the other.
Reviewer Checklist