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

fix: unreliable webhook behaviour on gatekeeper pod startup and shutdown #3780

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix: unreliable webhook behaviour on gatekeeper pod startup and shutdown #3780

wants to merge 6 commits into from
+34 −1

Conversation

l0wl3vel
Copy link

@l0wl3vel l0wl3vel commented Jan 15, 2025 •
edited
Loading

What this PR does / why we need it:

This PR fixes webhook requests getting routed to starting and stopping gatekeeper pods, that are not ready to serve requests. The implications of this behaviour are explained in #3776

Which issue(s) this PR fixes:
Fixes #3776

Special notes for your reviewer:

The grace period for handling termination will not work when using Tilt to test. It will terminate instantly, like it did before.

We tracked this down to the rerun-process-handler Tilt uses to wrap the application to test.

The start.sh script traps SIGTERM and INTERRUPTS, the two signals we handle, and replaces them with SIGKILL, which we do not intercept.

Running make run yields the intended behaviour.

Co-authored by Nils Federle (@nilsfed) and Paweł Kukliński (@pawelkuk)

@l0wl3vel l0wl3vel force-pushed the fix/webhook-availability branch 4 times, most recently from 46de743 to 937a91e Compare January 20, 2025 08:58
@l0wl3vel l0wl3vel marked this pull request as ready for review January 20, 2025 09:06
@l0wl3vel l0wl3vel requested a review from a team as a code owner January 20, 2025 09:06
@l0wl3vel l0wl3vel force-pushed the fix/webhook-availability branch from 937a91e to fcd6e13 Compare January 20, 2025 09:07
@l0wl3vel l0wl3vel mentioned this pull request Jan 20, 2025
Open
@l0wl3vel l0wl3vel changed the title fix: add upstream TLS check to readiness probe fix: fix unreliable webhook behaviour on gatekeeper pod statup and shutdown Jan 20, 2025
@l0wl3vel l0wl3vel changed the title fix: fix unreliable webhook behaviour on gatekeeper pod statup and shutdown fix: unreliable webhook behaviour on gatekeeper pod statup and shutdown Jan 20, 2025
JaydipGabani
JaydipGabani approved these changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@JaydipGabani JaydipGabani left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

JaydipGabani
JaydipGabani reviewed Jan 21, 2025
View reviewed changes
@l0wl3vel l0wl3vel force-pushed the fix/webhook-availability branch from fe4bc04 to 262f5c4 Compare January 22, 2025 13:44
@l0wl3vel
Copy link
Author

@JaydipGabani Integrated your suggestion. Could you please rerun the workflows?

@l0wl3vel l0wl3vel changed the title fix: unreliable webhook behaviour on gatekeeper pod statup and shutdown fix: unreliable webhook behaviour on gatekeeper pod startup and shutdown Jan 22, 2025
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.72%. Comparing base (3350319) to head (7da14f3).
Report is 265 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (3350319) and HEAD (7da14f3). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (3350319) HEAD (7da14f3)
unittests 2 1
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #3780      +/-   ##
==========================================
- Coverage   54.49%   47.72%   -6.77%     
==========================================
  Files         134      234     +100     
  Lines       12329    19864    +7535     
==========================================
+ Hits         6719     9481    +2762     
- Misses       5116     9494    +4378     
- Partials      494      889     +395
Flag Coverage Δ
unittests 47.72% <ø> (-6.77%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JaydipGabani JaydipGabani requested a review from a team January 22, 2025 23:28
@l0wl3vel l0wl3vel force-pushed the fix/webhook-availability branch from 3f4870c to 8b50591 Compare January 24, 2025 15:27
@l0wl3vel
Copy link
Author

@JaydipGabani Could you please rerun the pipelines and merge this? I missed the window where I could have merged without rebase.

@JaydipGabani
Copy link
Contributor

@l0wl3vel we want approval from one more maintainer to merge.

cc: @ritazh @maxsmythe @sozercan PTAL.

@l0wl3vel l0wl3vel force-pushed the fix/webhook-availability branch 2 times, most recently from 5e8f3c4 to f0fafbf Compare January 28, 2025 07:21
@l0wl3vel l0wl3vel force-pushed the fix/webhook-availability branch 2 times, most recently from 4783868 to ccbc7d1 Compare February 12, 2025 17:53
ritazh
ritazh reviewed Feb 12, 2025
View reviewed changes
ritazh
ritazh reviewed Feb 12, 2025
View reviewed changes
main.go
// Derived from how the controller-runtime sets up a signal handler with ctrl.SetupSignalHandler()
ctx, cancel := context.WithCancel(context.Background())

c := make(chan os.Signal, 2)
Copy link
Member

@ritazh ritazh Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

lets add a comment to https://github.com/kubernetes-sigs/controller-runtime/blob/894df3a7e664eb08c0ae0e0154a1e4204fd6729e/pkg/manager/signals/signal.go#L35 for this

sozercan
sozercan reviewed Feb 12, 2025
View reviewed changes
maxsmythe
maxsmythe approved these changes Feb 13, 2025
View reviewed changes
Copy link
Contributor

@maxsmythe maxsmythe left a comment

Choose a reason for hiding this comment

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

LGTM once nits from community call are addressed:

  • configurable shutdown delay
  • comment adding reference to issue/pr in controller-runtime for later cleanup

@l0wl3vel
Copy link
Author

controller-runtime upstream issue: kubernetes-sigs/controller-runtime#3113

There are two PRs for this issue in the controller-runtime already. The PRs fix it directly in the webhook server, not in the signal handler. I think there was interest in getting it fixed at some point, but the second PR was auto-closed without review.

I think we should go ahead with our hotfix.

@l0wl3vel l0wl3vel force-pushed the fix/webhook-availability branch from 2b20db7 to 82705f8 Compare February 17, 2025 07:25
@l0wl3vel l0wl3vel force-pushed the fix/webhook-availability branch from 82705f8 to 5df3e0a Compare February 17, 2025 08:35
@JaydipGabani
Copy link
Contributor

@l0wl3vel can you fix lint?

@l0wl3vel l0wl3vel force-pushed the fix/webhook-availability branch 2 times, most recently from dbe18d6 to 98bea30 Compare February 21, 2025 12:27
Benjamin Ritter and others added 5 commits February 21, 2025 13:29
@l0wl3vel
fix: add upstream TLS check to readiness probe
ebd1d12 
Signed-off-by: Benjamin Ritter <[email protected]>
@l0wl3vel
Add termination grace period
2de0c78 
Signed-off-by: Benjamin Ritter <[email protected]>
@l0wl3vel @JaydipGabani
fix: comment formatting
5b4169d 
Co-authored-by: Jaydip Gabani <[email protected]>
Signed-off-by: Benjamin Ritter <[email protected]>
@l0wl3vel
fix: remove uneccesary logging
e7744b2 
Signed-off-by: Benjamin Ritter <[email protected]>
@l0wl3vel
feat: add --shutdown-delay flag
5c03825 
Signed-off-by: Benjamin Ritter <[email protected]>
@l0wl3vel l0wl3vel force-pushed the fix/webhook-availability branch from 98bea30 to 5c03825 Compare February 21, 2025 12:29
@l0wl3vel
Copy link
Author

@JaydipGabani Fixed the linting error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sozercan sozercan sozercan left review comments

@ritazh ritazh ritazh left review comments

@JaydipGabani JaydipGabani JaydipGabani approved these changes

@maxsmythe maxsmythe maxsmythe 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.

Webhook requests fail on gatekeeper pod startup/shutdown
6 participants
@l0wl3vel @codecov-commenter @JaydipGabani @sozercan @ritazh @maxsmythe