search

openresty / lua-resty-upstream-healthcheck

Health Checker for Nginx Upstream Servers in Pure Lua
521 stars 134 forks source link

Add SSL Support - completion of PR#5 #36

Open ElvinEfendi opened 7 years ago

ElvinEfendi commented 7 years ago

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 commented 7 years ago

ping

charlescng commented 7 years ago

@agentzh Can you review?

agentzh commented 7 years ago

Also, please rebase to the latest git master.

agentzh commented 7 years ago

@ElvinEfendi Thank you for your patch! Please take care of my comments above. Thanks!

ElvinEfendi commented 7 years ago

@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?

ElvinEfendi commented 7 years ago

@agentzh any more comment on this PR?

ElvinEfendi commented 7 years ago

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

solio commented 7 years ago

why this pr has not bean merged？ 这个ssl support什么时候能合到主干上？

agentzh commented 7 years ago

@solio This is still in my review queue. Sorry for the delay on my side.

agentzh commented 7 years ago

@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.

ElvinEfendi commented 7 years ago

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":

screen shot 2017-07-10 at 11 05 48 pm
ElvinEfendi commented 7 years ago

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

agentzh commented 7 years ago

@ElvinEfendi Cool, thanks!

berlincount commented 7 years ago

hmmm, may I ping on this PR? :)

pavelnemirovsky commented 6 years ago

@agentzh when do u plan to merge it? Thx

agentzh commented 6 years ago

Sorry, we haven't had the time to look at this. Been busy with other things with higher priority.

agentzh commented 6 years ago

@dndx @spacewander Please help review this PR once you have a chance. Thanks!

ricardosantosalves commented 6 years ago

Hi, Is there any update regarding this PR?

gentle-king commented 5 years ago

Dears, any update on this feature?

TWHOI commented 4 years ago

+1 year :-/ Would be nice to have this merged.

doujiang24 commented 4 years ago

@rainingmaster do you have time to take a look at this PR?

leandromoreira commented 3 years ago

@agentzh is there any plan to merge this ?

leandromoreira commented 3 years ago

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?

leandromoreira commented 3 years ago

I copied this PR and released it as a rock https://github.com/globocom/lua-resty-upstream-healthcheck