Keep queue-proxy admin server on HTTP for PreStop hooks #16163

Fedosin · 2025-10-16T15:38:38Z

Proposed Changes

The queue-proxy admin server now always serves HTTP on port 8022, even when system-internal-tls is enabled. This simplifies the PreStop hook configuration and fixes graceful shutdown issues.

Why this approach:

PreStop hooks are called by kubelet locally within the pod (localhost)
No network traffic leaves the pod, so TLS isn't needed for security
Simpler implementation with no dynamic scheme configuration
Prevents TLS handshake errors during pod shutdown

This fixes the issue where pods would receive HTTP 502 errors during scale-down operations when system-internal-tls was enabled. The error occurred because the PreStop hook was trying to connect with HTTP to a TLS-enabled admin server, causing immediate SIGTERM and dropped requests.

dprotaso · 2025-10-16T15:46:37Z

To simplify this I think we should just have the preStop hook on a non-TLS server. I'd expect it to only be hit by the kubelet.

codecov · 2025-10-16T15:50:58Z

Codecov Report

❌ Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.08%. Comparing base (0ee5fc0) to head (b2d3c47).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
pkg/queue/sharedmain/main.go	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16163      +/-   ##
==========================================
- Coverage   80.09%   80.08%   -0.01%     
==========================================
  Files         214      214              
  Lines       16943    16940       -3     
==========================================
- Hits        13570    13566       -4     
- Misses       3013     3014       +1     
  Partials      360      360

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

dprotaso · 2025-10-16T17:16:07Z

@Fedosin do we have a test that can catch this regression?

Fedosin · 2025-10-16T18:33:12Z

@dprotaso yes, sure. I created a new function setupServers in servers.go that is responsible for server creation. After that I added tests to prevent regressions there.

The patch became bigger though...

dprotaso · 2025-10-17T01:50:21Z

The patch became bigger though...

Yeah I was thinking less about a unit test and wondering if there's an easy way to see if preStop hook was invoked successfully. For example if there's a Kubernetes event that's emitted when it fails etc.

Let me know what you think

The queue-proxy admin server now always serves HTTP on port 8022, even when system-internal-tls is enabled. This simplifies the PreStop hook configuration and fixes graceful shutdown issues. Changes: - Queue-proxy admin server always uses HTTP, only main server uses TLS - PreStop hooks always use HTTP scheme (removed dynamic configuration) - Updated tests to reflect that admin server is always HTTP Why this approach: - PreStop hooks are called by kubelet locally within the pod (localhost) - No network traffic leaves the pod, so TLS isn't needed for security - Simpler implementation with no dynamic scheme configuration - Prevents TLS handshake errors during pod shutdown This fixes the issue where pods would receive HTTP 502 errors during scale-down operations when system-internal-tls was enabled. The error occurred because the PreStop hook was trying to connect with HTTP to a TLS-enabled admin server, causing immediate SIGTERM and dropped requests.

maschmid · 2025-10-17T12:23:01Z

/test ?

knative-prow · 2025-10-17T12:23:05Z

@maschmid: The following commands are available to trigger required jobs:

/test build-tests

/test contour-latest

/test contour-tls

/test gateway-api-latest

/test istio-latest-no-mesh

/test istio-latest-no-mesh-tls

/test kourier-stable

/test kourier-stable-tls

/test unit-tests

/test upgrade-tests

The following commands are available to trigger optional jobs:

/test gateway-api-latest-and-contour

/test https

/test istio-latest-mesh

/test istio-latest-mesh-short

/test istio-latest-mesh-tls

/test performance-tests

Use /test all to run the following jobs that were automatically triggered:

build-tests_serving_main

istio-latest-no-mesh-tls_serving_main

istio-latest-no-mesh_serving_main

unit-tests_serving_main

upgrade-tests_serving_main

Details

In response to this:

/test ?

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

maschmid · 2025-10-17T12:28:03Z

/test kourier-stable

maschmid · 2025-10-17T12:56:13Z

/test contour-latest

maschmid · 2025-10-17T14:15:01Z

/test kourier-stable

Fedosin · 2025-10-17T18:11:24Z

PASS test/e2e/systeminternaltls.TestGracefulShutdownWithTLS (21.20s) but test/e2e: TestDestroyPodInflight is flaky :(

Fedosin · 2025-10-17T18:11:32Z

/test kourier-stable

dprotaso · 2025-10-18T02:46:05Z

/lgtm
/approve

thanks for the e2e test looks great

knative-prow · 2025-10-18T02:46:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, Fedosin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso]

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

The queue-proxy admin server now always serves HTTP on port 8022, even when system-internal-tls is enabled. This simplifies the PreStop hook configuration and fixes graceful shutdown issues. Changes: - Queue-proxy admin server always uses HTTP, only main server uses TLS - PreStop hooks always use HTTP scheme (removed dynamic configuration) - Updated tests to reflect that admin server is always HTTP Why this approach: - PreStop hooks are called by kubelet locally within the pod (localhost) - No network traffic leaves the pod, so TLS isn't needed for security - Simpler implementation with no dynamic scheme configuration - Prevents TLS handshake errors during pod shutdown This fixes the issue where pods would receive HTTP 502 errors during scale-down operations when system-internal-tls was enabled. The error occurred because the PreStop hook was trying to connect with HTTP to a TLS-enabled admin server, causing immediate SIGTERM and dropped requests.

… (#1614) The queue-proxy admin server now always serves HTTP on port 8022, even when system-internal-tls is enabled. This simplifies the PreStop hook configuration and fixes graceful shutdown issues. Changes: - Queue-proxy admin server always uses HTTP, only main server uses TLS - PreStop hooks always use HTTP scheme (removed dynamic configuration) - Updated tests to reflect that admin server is always HTTP Why this approach: - PreStop hooks are called by kubelet locally within the pod (localhost) - No network traffic leaves the pod, so TLS isn't needed for security - Simpler implementation with no dynamic scheme configuration - Prevents TLS handshake errors during pod shutdown This fixes the issue where pods would receive HTTP 502 errors during scale-down operations when system-internal-tls was enabled. The error occurred because the PreStop hook was trying to connect with HTTP to a TLS-enabled admin server, causing immediate SIGTERM and dropped requests.

knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 16, 2025

knative-prow bot requested review from dsimansk, gauron99 and skonto October 16, 2025 15:38

Fedosin force-pushed the https-prehook branch from c8ec4eb to a2021a7 Compare October 16, 2025 15:45

Fedosin force-pushed the https-prehook branch from a2021a7 to c318ee8 Compare October 16, 2025 16:04

knative-prow bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2025

Fedosin changed the title ~~Fix PreStop hook to use HTTPS with system-internal-tls~~ Keep queue-proxy admin server on HTTP for PreStop hooks Oct 16, 2025

dprotaso mentioned this pull request Oct 16, 2025

Istio now handles PreStop hooks correctly so we can drop our workaround #16164

Merged

Fedosin force-pushed the https-prehook branch from c318ee8 to 6653de8 Compare October 16, 2025 18:31

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 16, 2025

Fedosin force-pushed the https-prehook branch 2 times, most recently from ac4b4d8 to a0d06bf Compare October 17, 2025 10:21

Fedosin force-pushed the https-prehook branch from a0d06bf to b2d3c47 Compare October 17, 2025 11:27

knative-prow bot assigned dprotaso Oct 18, 2025

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 18, 2025

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 18, 2025

knative-prow bot merged commit 54868c2 into knative:main Oct 18, 2025
93 checks passed

Fedosin mentioned this pull request Oct 21, 2025

[release-v1.17] Keep queue-proxy admin server on HTTP for PreStop hooks (#16163) openshift-knative/serving#1614

Merged

Keep queue-proxy admin server on HTTP for PreStop hooks #16163

Keep queue-proxy admin server on HTTP for PreStop hooks #16163

Uh oh!

Conversation

Fedosin commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

dprotaso commented Oct 16, 2025

Uh oh!

codecov bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dprotaso commented Oct 16, 2025

Uh oh!

Fedosin commented Oct 16, 2025

Uh oh!

dprotaso commented Oct 17, 2025

Uh oh!

maschmid commented Oct 17, 2025

Uh oh!

knative-prow bot commented Oct 17, 2025

Uh oh!

maschmid commented Oct 17, 2025

Uh oh!

maschmid commented Oct 17, 2025

Uh oh!

maschmid commented Oct 17, 2025

Uh oh!

Fedosin commented Oct 17, 2025

Uh oh!

Fedosin commented Oct 17, 2025

Uh oh!

dprotaso commented Oct 18, 2025

Uh oh!

knative-prow bot commented Oct 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fedosin commented Oct 16, 2025 •

edited

Loading

codecov bot commented Oct 16, 2025 •

edited

Loading