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(tcp-log) TLS handshake support for TCP logs #3091

Merged
merged 1 commit into from
Dec 14, 2017
Merged

Conversation

p0pr0ck5
Copy link
Contributor

Summary

Support TLS connections for remote TCP servers in the tcp-log plugin.

Full changelog

  • Add two new config options to tcp-log: tls, a boolean indicating whether to perform a TLS handshake against the remote server, and tls_sni, and optional string defining the SNI hostname to send in the handshake.
  • Attempt to perform a TLS handshake when tls is true.
  • Add support for TLS connections in the mock tcp_server helpers construct.

Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Yep, this sounds like a job well done 👍

Mind simply updating the 2 style issues before pressing "Merge"? Trust you!

ok, err = sock:sslhandshake(true, conf.tls_sni, false)
if not ok then
ngx.log(ngx.ERR, "[tcp-log] failed to perform TLS handshake to ",
host, ":", port, ": ", err)
Copy link
Member

Choose a reason for hiding this comment

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

style: we typically apply an exception to ngx.log and align subsequent arguments with the 2nd one ("...) 😅

@@ -0,0 +1,21 @@
local plugin_config_iterator = require("kong.dao.migrations.helpers").plugin_config_iterator

return {
Copy link
Member

Choose a reason for hiding this comment

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

style: new file, could we respect the 2 lines jump rule? Thanks!

@thibaultcha thibaultcha added pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done. and removed pr/status/please review labels Dec 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/changes requested Changes were requested to this PR by a maintainer. Please address them and ping back once done.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants