lua-resty-upstream-healthcheck
lua-resty-upstream-healthcheck copied to clipboard
openresty
Add SSL Support - completion of PR#5
This PR is based on the work done at https://github.com/openresty/lua-resty-upstream-healthcheck/pull/5.
It adds SSL support and addresses all the comments(tests and session reuse) made in the original PR.
One significant modification to the original PR is that now, the health check will fail if ssl_verify = true and the certificate verification fails.
/r @agentzh
@ElvinEfendi Thank you for your patch! Please take care of my comments above. Thanks!
@agentzh thanks for reviewing, I addressed your comments.
Also, please rebase to the latest git master.
I don't see any conflict with the upstream master(there were some initially but those have been sycned to this branch already). And all the commits from it are already included in this branch. Why do you think this branch is not in sync with the upstream master?
By default peer.name(an entry defined inside upstream block as <host>:<port>) is being passed to tcpsock:sslhandshake function as server_name which is used to validate the server name specified in the server certificate sent from the remote when ssl_verify is true. Refer to https://github.com/openresty/lua-nginx-module#tcpsocksslhandshake for more information. This is a constraint when we want to share a single server certificate to do SSL health check between multiple peers. Therefore I added TEST 20 and 21 to make ssl_verify option's behaviour more clear and added a new functionality that lets users to override peer.name with a custom name set during the initialization. TEST 21 covers the new functionality. These changes are added in commit: https://github.com/openresty/lua-resty-upstream-healthcheck/pull/36/commits/ce88ef0ffc8e2230882bbb7bd83247a35e9061c6
@ElvinEfendi Will you please rebase to the latest git master? I'm seeing merge conflicts while rebasing it myself:
$ git rebase master
First, rewinding head to replay your work on top of it...
Applying: add ssl support
Using index info to reconstruct a base tree...
M README.markdown
M lib/resty/upstream/healthcheck.lua
Falling back to patching base and 3-way merge...
Auto-merging lib/resty/upstream/healthcheck.lua
CONFLICT (content): Merge conflict in lib/resty/upstream/healthcheck.lua
Auto-merging README.markdown
error: Failed to merge in the changes.
Patch failed at 0001 add ssl support
The copy of the patch that failed is found in: .git/rebase-apply/patch
Thanks! And sorry for the delay on my side.
Will you please rebase to the latest git master? I'm seeing merge conflicts while rebasing it myself:
Hmm I don't know why does it show conflict. There's no commit in the upstream after https://github.com/openresty/lua-resty-upstream-healthcheck/commit/69751de70d5ac6ab84947e8e074b7a14715fdb02 which is already in downstream as well https://github.com/Shopify/lua-resty-upstream-healthcheck/commit/69751de70d5ac6ab84947e8e074b7a14715fdb02.
https://github.com/openresty/lua-resty-upstream-healthcheck/compare/master...Shopify:master also shows "Able to merge":
I'd also like to highlight that https://github.com/openresty/lua-resty-upstream-healthcheck/pull/36/commits/6731f959d146264e8ba1b5adbd41968deb0ae92e is not related to this PR, it is just included here because the downstream is Shopify fork's master.
If you don't want to merge that last unrelated commit, here is the PR with only SSL support: https://github.com/openresty/lua-resty-upstream-healthcheck/pull/44
Sorry, we haven't had the time to look at this. Been busy with other things with higher priority.
@dndx @spacewander Please help review this PR once you have a chance. Thanks!
Hi @ElvinEfendi did you publish a rock with these changes? If you didn't, I'm thinking to keep a lua-resty-upstream-healthcheck-ssl repo patching your changes to the main branch of this repo, what do you think?
I copied this PR and released it as a rock https://github.com/globocom/lua-resty-upstream-healthcheck