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

chore(patches): add patches from upstream openresty #13640

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

Conversation

catbro666
Copy link
Contributor

@catbro666 catbro666 commented Sep 10, 2024

Summary

Fixing the issue where a connection can't be setkeepalive successfully when using TLSv1.3. The issue was originally found in the tcp-log plugin.

ngx_lua patches

ngx_stream_lua patches

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

https://konghq.atlassian.net/browse/FTI-6190

@github-actions github-actions bot added build/bazel cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee labels Sep 10, 2024
Copy link
Member

@bungle bungle left a comment

Choose a reason for hiding this comment

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

Doesn't apply cleanly, please fix these:

patching file bundle/ngx_lua-0.10.26/src/ngx_http_lua_socket_tcp.c
Hunk #1 succeeded at 5731 (offset 6 lines).
Hunk #2 succeeded at 5752 (offset 6 lines).
patching file bundle/ngx_stream_lua-0.0.14/src/ngx_stream_lua_socket_tcp.c
Hunk #1 succeeded at 5595 (offset 7 lines).
Hunk #2 succeeded at 5617 (offset 7 lines).

@bungle
Copy link
Member

bungle commented Sep 11, 2024

Also what is difference here compared to:
#13219 (which was not given approval to merge)

Aka is this fixing some real issue we want to fix. The LuaJIT was a real issue, but hard to reproduce. Just wanting to know how to react on these. Are we always good to merge them or is there some other logic and why the LuaJIT patch was not part of that logic.

@catbro666 catbro666 force-pushed the fti-6190-tcp-setkeepalive-tls13 branch 2 times, most recently from 9d99d5e to abef72a Compare September 11, 2024 09:23
@catbro666 catbro666 force-pushed the fti-6190-tcp-setkeepalive-tls13 branch from abef72a to afba66b Compare September 14, 2024 08:27
@team-gateway-bot team-gateway-bot added the author/community PRs from the open-source community (not Kong Inc) label Sep 16, 2024
@github-actions github-actions bot added the chore Not part of the core functionality of kong, but still needed label Oct 14, 2024
@catbro666 catbro666 force-pushed the fti-6190-tcp-setkeepalive-tls13 branch from 09ec65b to 06ac071 Compare October 14, 2024 08:39
@catbro666
Copy link
Contributor Author

Doesn't apply cleanly, please fix these:

Fixed.

Aka is this fixing some real issue we want to fix.

@bungle Yes, this patch is fixing a real issue discovered by https://konghq.atlassian.net/browse/FTI-6190

@catbro666 catbro666 requested a review from bungle October 14, 2024 09:05
if C.SSL_shutdown(self.ctx) ~= 1 then
return nil, format_error("SSL_shutdown")
end
self.sock:close()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To reviewers: SSL_shutdown only does a half-close, so the connection can still be used after being closed here. This causes some tests in tcp-log_spec.lua and reports_spec.lua to fail because the connection established by a previous test case is reused by the next test case.

@catbro666 catbro666 added this to the 3.9.0 milestone Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author/community PRs from the open-source community (not Kong Inc) build/bazel cherry-pick kong-ee schedule this PR for cherry-picking to kong/kong-ee chore Not part of the core functionality of kong, but still needed size/M skip-changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants