Skip to content

Conversation

@dprotaso
Copy link
Member

@dprotaso dprotaso commented Nov 30, 2025

Testing - knative/pkg#3298

@knative-prow knative-prow bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 30, 2025
@knative-prow knative-prow bot requested review from dsimansk and skonto November 30, 2025 17:11
@dprotaso dprotaso changed the title update deps to drop h2c lib [wip] update deps to drop h2c lib Nov 30, 2025
@knative-prow
Copy link

knative-prow bot commented Nov 30, 2025

[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

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Nov 30, 2025
@codecov
Copy link

codecov bot commented Nov 30, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.03%. Comparing base (abbe514) to head (f2547c2).

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16280      +/-   ##
==========================================
- Coverage   80.04%   80.03%   -0.01%     
==========================================
  Files         215      215              
  Lines       13320    13320              
==========================================
- Hits        10662    10661       -1     
- Misses       2296     2298       +2     
+ Partials      362      361       -1     

☔ 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
Copy link
Member Author

dprotaso commented Dec 1, 2025

@linkvt do you know anyone that could help debug why the kourier-tls settings are failing?

@linkvt
Copy link
Contributor

linkvt commented Dec 1, 2025

Hi @dprotaso ,
from what I remember when I started this in the other PR I had a lot more pending changes. I don't think its a kourier issue but rather because we only test istio and kourier with TLS and istio does likely some meshmagic? I would assume that gateway-tls and contour-tls would also fail if they would exist.

My test setup was:

  • kourier with knative internal TLS enabled
  • 3 KServices that speak only HTTP1, only HTTP2 or both
  • curl where I forced to use either HTTP1 or HTTP2

I would assume that the current implementation will not work with these 6 test cases.

One example was that when I used kourier with TLS and HTTP2 enabled my naive implementation would always try to use HTTP2 internally despite me using curl --http1.1 -> ingress -> knative -> http1 only service.
When the activator talks to the queue-proxy they would IIRC always agree on using HTTP2 via their TLS ALPN independent from the client requests protocol and independent from upgrade headers or anything else - this happens on the transport level below HTTP which I think is important to understand.
My idea there was to adjust the TLSConfig depending on the proxied request so that TLS ALPN does not allow h2 in the NextProtos depending on the protocol used by the client.
Reason for that is that the clients protocol decision determines (to my understanding) which protocol has to be used in the whole chain through knative).

I learned in the meanwhile that e.g. envoy is able to translate between http1, http2 and http3 requests and that this may be more desired in our case as well, so that the client request protocol does not determine which protocol is used internally. I guess this would be a larger change though.

Feel free to ping me in slack, we can look at this together but I think you will see the issue already when using my test setup described above.

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants