Skip to content

Allow serve mode when system-internal-tls is enabled - #16183

Merged
knative-prow[bot] merged 1 commit into
knative:mainfrom
linkvt:allow-serve-mode-with-internal-tls
Nov 28, 2025
Merged

Allow serve mode when system-internal-tls is enabled#16183
knative-prow[bot] merged 1 commit into
knative:mainfrom
linkvt:allow-serve-mode-with-internal-tls

Conversation

@linkvt

@linkvt linkvt commented Oct 21, 2025 •
edited by dprotaso
Loading

Copy link
Copy Markdown
Member

Hi,

Proposed Changes

  • Allow serve mode when system-internal-tls is enabled
    • right now Proxy mode is always forced when using system-internal-tls
    • after a review from Dave the decision is to not force proxy mode anymore.

Quote from Dave:

Meaning I say we don't require the configuration knob and just say implementations must support multiple sans for this feature to work properly.

Going through our implementations

Istio (unsupported since you have mTLS with sidecars/ambient)
Contour (supports multiple-sans)
Kourier (supports multiple-sans)
Gateway API (supports this through extended BackendTLSPolicy features - this is not implemented)

Release Note

Allow activator to be out of the request path when system-internal-tls is enabled

Questions

  1. Should this change result in a docs change, even if I marked it as alpha flag for now?

Thanks for the feedback!

/kind enhancement
/cc @Fedosin

@knative-prow
knative-prow Bot requested a review from Fedosin October 21, 2025 13:42
@knative-prow

knative-prow Bot commented Oct 21, 2025

Copy link
Copy Markdown

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow Bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 21, 2025
@linkvt linkvt mentioned this pull request Oct 21, 2025
Closed
@codecov

codecov Bot commented Oct 21, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.08%. Comparing base (f7aa291) to head (69b4f33).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #16183      +/-   ##
==========================================
+ Coverage   80.04%   80.08%   +0.03%     
==========================================
  Files         215      215              
  Lines       13322    13320       -2     
==========================================
+ Hits        10664    10667       +3     
+ Misses       2299     2294       -5     
  Partials      359      359

☔ 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.

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2025
@Fedosin

Fedosin commented Oct 29, 2025

Copy link
Copy Markdown
Contributor

We'll likely need an e2e test for this scenario. Since this feature is expected to work with net-kourier, we can restrict the test environments to that specific Ingress controller.

@linkvt
linkvt force-pushed the allow-serve-mode-with-internal-tls branch from 98fda9c to 99dbad5 Compare October 31, 2025 13:02
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2025
@linkvt
linkvt force-pushed the allow-serve-mode-with-internal-tls branch from 99dbad5 to 3099645 Compare October 31, 2025 13:10
@linkvt

linkvt commented Oct 31, 2025

Copy link
Copy Markdown
Member Author

I added an e2e test verifying this functionality and verified already locally that it works (both directions).

/hold until is knative/networking#1093 is merged, released and deps are updated
/remove-approve - was added automatically as I'm co-release lead.

@knative-prow knative-prow Bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 31, 2025
@linkvt
linkvt marked this pull request as ready for review October 31, 2025 13:12
@knative-prow knative-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 31, 2025
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2025
@dprotaso

dprotaso commented Nov 27, 2025

Copy link
Copy Markdown
Member

I'd like to simplify the configuration and just simply allow serve mode without a knob.

See my comment here: knative/networking#1093 (comment)

@linkvt
linkvt force-pushed the allow-serve-mode-with-internal-tls branch from 3099645 to 43a438e Compare November 28, 2025 12:33
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 28, 2025
@linkvt linkvt changed the title Allow using serve mode with system-internal-tls Allow serve mode when system-internal-tls is enabled Nov 28, 2025
@linkvt
Allow serve mode when system-internal-tls is enabled
69b4f33 
Previously, when system-internal-tls was enabled, the KPA reconciler
would force all traffic through the activator (proxy mode) regardless
of scaling conditions. This prevented the use of serve mode even when
the standard conditions were met (desired scale > 0 and excess burst
capacity >= 0).

This change removes that restriction, allowing serve mode to be used
with TLS based on the normal scaling conditions, improving performance
in TLS-enabled environments.
@linkvt
linkvt force-pushed the allow-serve-mode-with-internal-tls branch from 43a438e to 69b4f33 Compare November 28, 2025 13:08
@dprotaso

dprotaso commented Nov 28, 2025

Copy link
Copy Markdown
Member

/hold cancel
/lgtm
/approve

thanks for the changes!

@knative-prow knative-prow Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2025
@knative-prow knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Nov 28, 2025
@knative-prow

knative-prow Bot commented Nov 28, 2025

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso

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:

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

@knative-prow knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2025
@knative-prow
knative-prow Bot merged commit abbe514 into knative:main Nov 28, 2025
92 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fedosin Fedosin Awaiting requested review from Fedosin

Assignees

@dprotaso dprotaso

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@linkvt @Fedosin @dprotaso @knative-prow-robot