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

Validating Admission Webhook #13503

Open
agilgur5 opened this issue Aug 26, 2024 · 2 comments · May be fixed by #13879
Open

Validating Admission Webhook #13503

agilgur5 opened this issue Aug 26, 2024 · 2 comments · May be fixed by #13879
Labels
type/feature Feature request

Comments

@agilgur5
Copy link
Contributor

agilgur5 commented Aug 26, 2024

Summary

We currently have various scenarios where an invalid Workflow will be allowed in the cluster, causing bugs to be found much later instead of upfront. The CLI and API handle (some) validation, but direct to k8s will miss some of this.

Use Cases

The lack of any validation when submitting directly to k8s can cause some unexpected scenarios like #6781 , #12693, #10630. In all those issues and some others, I've mentioned the concept of a Validating Admission Webhook to prevent these scenarios and give clearer error messages: #12693 (comment), #10630 (comment), #6781 (comment)

Note that even if we do have a validating admission webhook, it may be optional, it may not handle resources that were created before it existed, and it may not be able to validate all scenarios, so the Controller and Informers would still have to be "tolerant" (the word the codebase uses) of invalid resources. This is primarily a feature to improve UX

Implementation Details

There are 2 primary ways of doing Validating Admission in k8s these days:

  1. Admission Webhooks

    • This is simple in terms of logic, in that we'd just re-use the existing validation logic, either with the Controller or the Server
      • Typically admission is a Controller responsibility, but hosting this behavior in the Controller would require that it respond to API requests. I have seen other tools host as a separate component entirely as well, e.g. in a Helm Chart as argo-workflows.validatingAdmissionWebhook.enabled: true or similar.
    • Problematically, k8s webhooks require TLS certs that the k8s control plane trusts, which complicates Deployment quite a bit as it requires cert-manager etc.
    • There is also a question of failing open or failing closed. I think we can fail closed, and latency sensitive use-cases could just not deploy / not enable the webhook entirely.
  2. Admission Policies (stable as of k8s 1.30)

    • This is basically a policy engine built-in to k8s (instead of external like Kyverno, OPA Gatekeeper, etc) that supports CEL
    • This would require rewriting some validation in CEL, but is otherwise quite straightforward to deploy:
      1. Can be used directly in-line with CRDs
      2. Or can be separate resources. Given the problems with CRD size, this might be the best possible solution
    • There are some more dynamic validation scenarios that would be hard to cover via CEL, but it could probably still cover a decent bit

I would probably suggest an optional of version of 1 for consistency, and then 2.b. as something deployed with all manifests as a baseline. We could even start making simple policies already and add them to the manifests and then gradually build them up -- #6781 is a very simple length check, for instance


Message from the maintainers:

Love this feature request? Give it a 👍. We prioritise the proposals with the most 👍.

@agilgur5 agilgur5 added the type/feature Feature request label Aug 26, 2024
@Joibel
Copy link
Member

Joibel commented Aug 27, 2024

I would like this to happen, especially Admission Webhooks.

There are certain classes of error that Admission Policies couldn't be crafted to help with such as invalid fields in yaml dictionaries which are a common class of error - either just misspellings or using a field which isn't valid in this location.

@agilgur5
Copy link
Contributor Author

agilgur5 commented Aug 27, 2024

invalid fields in yaml dictionaries

That one could be detected actually as CEL Macros include has() and exists(). But detecting all invalid schemas is likely non-trivial

More dynamic things though, like detecting certain kinds of invalid expr expression, are not possible.
Technically even some simple invalid expressions (like #12899) could be detected with plain string parsing, but it would make for some convoluted/hacky code.

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

Successfully merging a pull request may close this issue.

2 participants