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

Mutating webhooks should not continue if a handler throws..? #899

Open
btlghrants opened this issue Jun 17, 2024 · 1 comment · May be fixed by #1672
Open

Mutating webhooks should not continue if a handler throws..? #899

btlghrants opened this issue Jun 17, 2024 · 1 comment · May be fixed by #1672
Assignees
Milestone

Comments

@btlghrants
Copy link
Collaborator

Based on recent work in this issue, it appears that an unhandled failure within a mutating Action callback will be logged by Pepr but then will not prevent further processing of the associated admission request.

See the commented out test in this Excellent Example for a reproduction of the described behavior (where the failing Mutation() of a delete request still allows the resource to be deleted).

This might be a problem for a few of reasons:

  1. It is perfectly reasonable for a Module Author to want to run a series of mutating Action handlers against a resource in order to accomplish a desired goal and having one quietly fail while allowing others to act is likely to result in a resource with an intermediate state being submitted to the cluster.

  2. Relying on Pepr logs -- which end user's aren't really expected to have to check -- to identify issues regarding a resource request that explodes a failed mutating callback is misleading to the user (at best) -- as the user-facing request (via HTTP / kubectl) will appear to succeed -- even though not all the registered mutations will have occurred.

  3. Relying on Pepr logs to identify Action handler issues -- rather than rejecting admission requests outright -- loses out on the opportunity to provide direct feedback about errors occurring in the Pepr Action chain. This likely to greatly extend time-to-discovery of "bad handler" issues.

@btlghrants btlghrants converted this from a draft issue Jun 17, 2024
@btlghrants btlghrants added enhancement New feature or request and removed enhancement New feature or request labels Jun 17, 2024
@cmwylie19 cmwylie19 added this to the v0.33.0 milestone Jun 27, 2024
@cmwylie19 cmwylie19 moved this from 🆕 New to 📋 Backlog in Pepr Project Board Jun 27, 2024
@cmwylie19 cmwylie19 removed this from the v0.33.0 milestone Jun 28, 2024
@cmwylie19 cmwylie19 added the ON-HOLD Needs investigation label Nov 6, 2024
@cmwylie19
Copy link
Collaborator

Docs blurb around this behavior - why you should mutate, and then validate. Check Best practices docs to make sure this does not already exists.

@cmwylie19 cmwylie19 removed the ON-HOLD Needs investigation label Jan 15, 2025
@cmwylie19 cmwylie19 added this to the v0.43.0 milestone Jan 15, 2025
@cmwylie19 cmwylie19 moved this to 📋 Backlog in Pepr Project Board Jan 15, 2025
@cmwylie19 cmwylie19 self-assigned this Jan 15, 2025
@cmwylie19 cmwylie19 moved this from 📋 Backlog to 🏗 In progress in Pepr Project Board Jan 15, 2025
@cmwylie19 cmwylie19 linked a pull request Jan 15, 2025 that will close this issue
5 tasks
@cmwylie19 cmwylie19 moved this from 🏗 In progress to 👀 In review in Pepr Project Board Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 👀 In review
Development

Successfully merging a pull request may close this issue.

2 participants