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.

Sign up for GitHub

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

Jump to bottom

Do not block pod creating on internal error in webhook #811

Merged
merged 5 commits into from
Apr 25, 2022
Merged

Do not block pod creating on internal error in webhook #811

merged 5 commits into from
Apr 25, 2022

Conversation

pavolloffay
Copy link
Member

Signed-off-by: Pavol Loffay [email protected]

Resolves #802

@pavolloffay pavolloffay requested a review from a team April 6, 2022 17:05
@pavolloffay
Copy link
Member Author

@trutty would you like to review?

frzifus
frzifus reviewed Apr 6, 2022
View reviewed changes
Copy link
Member

@frzifus frzifus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could it lead to some other issues, if we return code 500 and an error message in the status result?

internal/webhookhandler/webhookhandler.go Show resolved Hide resolved
@pavolloffay
Fix
a729b12 
Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Copy link
Member Author

Could it lead to some other issues, if we return code 500 and an error message in the status result?

I cannot think of any. Do you have anything in mind?

The error still gets propagated to the events but it does not block pod creation.

@frzifus
Copy link
Member

frzifus commented Apr 6, 2022

I cannot think of any. Do you have anything in mind?

No, i was just wondering. 👍

@pavolloffay pavolloffay changed the title Do not block pod creating on internal error Do not block pod creating on internal error in webhook Apr 7, 2022
@pavolloffay
Fix
84082e2 
Signed-off-by: Pavol Loffay <[email protected]>
pavolloffay
pavolloffay commented Apr 7, 2022
View reviewed changes
internal/webhookhandler/webhookhandler_test.go
name: "empty payload",
req: admission.Request{},
expected: http.StatusBadRequest,
allowed: false,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the webhook fails the pod creation only if it cannot decode the request, which should not happen in practice.

trutty
trutty reviewed Apr 8, 2022
View reviewed changes
Copy link

@trutty trutty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@trutty would you like to review?

Thanks @pavolloffay for the PR.
As stated, I am new to the inner workings of admission controller/webooks so my suggestion below could have faulty reasoning. Just wondering whether it is the best practice implementation 😉

internal/webhookhandler/webhookhandler.go Show resolved Hide resolved
@pavolloffay
Add more docs
c8167a8 
Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Fix
a0f168d 
Signed-off-by: Pavol Loffay <[email protected]>
@pavolloffay
Copy link
Member Author

@open-telemetry/operator-maintainers could you please review?

@pavolloffay
Copy link
Member Author

@jpkrohling @VineethReddy02 could you please review?

jpkrohling
jpkrohling reviewed Apr 25, 2022
View reviewed changes
Copy link
Member

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original report is about auto-instrumentation webhooks, while this PR seems to fix this behavior for the sidecar injection. Are they both sharing the same function?

@pavolloffay
Copy link
Member Author

The original report is about auto-instrumentation webhooks, while this PR seems to fix this behavior for the sidecar injection. Are they both sharing the same function?

yes, both are sharing the same pod webhook skeleton.

@jpkrohling jpkrohling merged commit 5b1c7b5 into open-telemetry:main Apr 25, 2022
ItielOlenick pushed a commit to ItielOlenick/opentelemetry-operator that referenced this pull request May 1, 2024
@pavolloffay
Do not block pod creating on internal error in webhook (open-telemetr…
d665a60 
…y#811)

* Do not block pod creating on internal error

Signed-off-by: Pavol Loffay <[email protected]>

* Fix

Signed-off-by: Pavol Loffay <[email protected]>

* Fix

Signed-off-by: Pavol Loffay <[email protected]>

* Add more docs

Signed-off-by: Pavol Loffay <[email protected]>

* Fix

Signed-off-by: Pavol Loffay <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@frzifus frzifus frzifus left review comments

@trutty trutty trutty left review comments

@jpkrohling jpkrohling jpkrohling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error of admission webhook prevents scaling of deployment despite falurePolicy: Ignore
4 participants
@pavolloffay @frzifus @jpkrohling @trutty