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.

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

feat: disable HTTP/2 ALPN handshake for connections on routes configured with AI-proxy. #13735

Merged
merged 12 commits into from
Nov 4, 2024

Conversation

oowl
Copy link
Member

@oowl oowl commented Oct 9, 2024

Summary

This change will disable HTTP/2 ALPN handshake for connections on routes configured with AI-proxy.

The following are the specific changes

Checklist

  • The Pull Request has tests
  • A changelog file has been created under changelog/unreleased/kong or skip-changelog label added on PR if changelog is unnecessary. README.md
  • There is a user-facing docs PR against https://github.com/Kong/docs.konghq.com - PUT DOCS PR HERE

Issue reference

AG-119

@github-actions github-actions bot added build/bazel cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Oct 9, 2024
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Oct 9, 2024
@oowl oowl changed the base branch from master to disable-h2-alpn-re October 12, 2024 07:00
@oowl oowl changed the title feat: disable http2 traffic when ai-proxy was enabled in the current route feat: disable HTTP/2 ALPN handshakes for requests hitting the route configured with ai-proxy Oct 12, 2024
@oowl oowl changed the title feat: disable HTTP/2 ALPN handshakes for requests hitting the route configured with ai-proxy feat: disable HTTP/2 ALPN handshake for connections on routes configured with AI-proxy. Oct 12, 2024
@oowl oowl force-pushed the disable-h2-alpn-ai-proxy branch 2 times, most recently from 9d538a0 to 9e4bf73 Compare October 12, 2024 11:07
Base automatically changed from disable-h2-alpn-re to master October 21, 2024 08:14
Copy link
Contributor

@fffonion fffonion left a comment

Choose a reason for hiding this comment

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

lgtm except a minor naming comment

kong/llm/proxy/handler.lua Show resolved Hide resolved
@catbro666
Copy link
Contributor

catbro666 commented Nov 1, 2024

The current solution (based on snis on the routes entity) isn't compatible with expressions router, as the mtls-auth plugin already faces. Please see https://konghq.atlassian.net/browse/FTI-6227 for more details. I suggest considering this PR along with FTI-6227, otherwise it may be difficult to deal with in the future.

Comment on lines +129 to +130
local send_ca_dn = plugin.config.send_ca_dn
local ca_ids = plugin.config.ca_certificates
Copy link
Contributor

Choose a reason for hiding this comment

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

This is actually related to the mtls-auth plugin, and we may need to refactor here as well.

Copy link
Member

@windmgc windmgc left a comment

Choose a reason for hiding this comment

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

Approving to unblock the 3.9 planning.
This will be refactored later

@windmgc windmgc merged commit 7d71b6b into master Nov 4, 2024
26 checks passed
@windmgc windmgc deleted the disable-h2-alpn-ai-proxy branch November 4, 2024 06:47
@team-gateway-bot
Copy link
Collaborator

Cherry-pick failed for master, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git remote add upstream https://github.com/kong/kong-ee
git fetch upstream master
git worktree add -d .worktree/cherry-pick-13735-to-master-to-upstream upstream/master
cd .worktree/cherry-pick-13735-to-master-to-upstream
git checkout -b cherry-pick-13735-to-master-to-upstream
ancref=$(git merge-base 6d7bf6819fc4a253fe80b12b5a5f1f6ec8f342dc 30414a664f54b187e9e2e39f65d81b2cff68948c)
git cherry-pick -x $ancref..30414a664f54b187e9e2e39f65d81b2cff68948c

@github-actions github-actions bot added the incomplete-cherry-pick A cherry-pick was incomplete and needs manual intervention label Nov 4, 2024
@oowl
Copy link
Member Author

oowl commented Nov 4, 2024

offline discussion with @ms2008 @catbro666 @dndx
considered https://github.com/Kong/kong-ee/pull/10510 as a temporary solution for the mtls plugin ( ATC route expression route can not scan all the route sni match results, so the change of 10510 can not apply to ee master branch), now we merge the current PR to 3.9 release and then follow the refactor when the 10510 change merge to the master branch. (after 3.9)

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

Successfully merging this pull request may close these issues.

6 participants