Make the SPO webhooks opt-in

[jhrozek]

This is a work-in-progress. I want to get early feedback before I fix up tests
and adds docs etc. Feedback is appreciated!

As a fallback if others are not happy with this change, I can always just change
the OCP default namespace selector, since that's the distro I care about the most,
but I think for stability reasons, this change might be beneficial for upstream
as a whole.

What type of PR is this?

/kind feature

What this PR does / why we need it:

The SPO webhooks have been cluster-wide by default. This is great for
functionality as both recording and binding just works, but might be
dangerous - if there is a bug in the webhook or the webhook is not
replying, then all namespaces might be affected.

This patch switches the default to opt-in, meaning that only namespaces
labeled with spo.x-k8s.io/enable-binding or
spo.x-k8s.io/enable-recording would be listened on.

Which issue(s) this PR fixes:

None

Does this PR have test?

Not yet (it's a WIP)

Special notes for your reviewer:

N/A

Does this PR introduce a user-facing change?

SPO changed the way the webhooks are enabled from listening on all namespaces by default
to explicitly listening for activity on namespaces labeled with spo.x-k8s.io/enable-binding
and spo.x-k8s.io/enable-recording respectively.

[jhrozek]

No longer a WIP.
Since this is a breaking change, I'd prefer a thorough review.

@ccojocar your opinion is also welcome because you worked on the static webhook configuration. Do you think the static webhooks would benefit from being opt-in, too?

[saschagrunert]

/test pull-security-profiles-operator-test-e2e

[saschagrunert]

Code LGTM and I'd like to see that as part of 0.5.0. Can we add an e2e test which verifies that it does not work when the label is not set?

[ccojocar]

@ccojocar your opinion is also welcome because you worked on the static webhook configuration. Do you think the static webhooks would benefit from being opt-in, too?

Definitely, we already made this change in our clusters to prevent availability issues when the webhook is down. We made it even on a pod level basis. The webhook listens only to pods with a certain label.

[ccojocar]

The implementation looks good to me. I am wondering if we should add in the docs that the object selector can be also add to the static webhook configuration. We did exactly the same change but on the static webhook manifest.

[jhrozek]

Code LGTM and I'd like to see that as part of 0.5.0. Can we add an e2e test which verifies that it does not work when the label is not set?

Will do, thank you for the review.

[jhrozek]

@ccojocar your opinion is also welcome because you worked on the static webhook configuration. Do you think the static webhooks would benefit from being opt-in, too?

Definitely, we already made this change in our clusters to prevent availability issues when the webhook is down.

I'm not sure if I'm reading your "definitely" right :-) do I understand correctly that you're saying that I should fixup the static webhook manifest by only making it listen for the labels?

We made it even on a pod level basis. The webhook listens only to pods with a certain label.

Oh nice, we should suggest that in docs as well as additional "hardening".

[ccojocar]

I'm not sure if I'm reading your "definitely" right :-) do I understand correctly that you're saying that I should fixup the static webhook manifest by only making it listen for the labels?

yeah, that's even better than just documenting. Let's add it to the static webhook to be consistent.

[jhrozek]

I'm not sure if I'm reading your "definitely" right :-) do I understand correctly that you're saying that I should fixup the static webhook manifest by only making it listen for the labels?

yeah, that's even better than just documenting. Let's add it to the static webhook to be consistent.

I agree, will do. Thanks!

[jhrozek]

Another iteration: I haven't amended the docs with object selector yet, but the tests are amended now. I've only had the chance to run the local fedora based tests so far, let's see how the other fare in CI.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhrozek, saschagrunert

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:

• OWNERS [jhrozek,saschagrunert]

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