Add SSL Support - completion of PR#5 #36

ElvinEfendi · 2017-01-27T17:42:37Z

This PR is based on the work done at #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

Ssl support

avoid permission requirement by using port > 1024

ElvinEfendi · 2017-02-02T14:48:04Z

ping

charlescng · 2017-02-10T18:48:28Z

@agentzh Can you review?

agentzh · 2017-02-11T21:21:14Z

lib/resty/upstream/healthcheck.lua

+    end
+
+    if ctx.type == "https" then
+        session, err = sock:sslhandshake(ctx.session, name, ctx.ssl_verify)


Is session a global Lua variable? We should declare it as a local.

Better use the lua-releng script of nginx-devel-utils to check your Lua code. I'm getting the following errors by running this tool against your branch:

$ lua-releng lib/resty/upstream/healthcheck.lua: 0.03 (0.03) Checking use of Lua global variables in file lib/resty/upstream/healthcheck.lua... op no. line instruction args ; code 53 [237] SETGLOBAL 11 -14 ; session 54 [238] GETGLOBAL 11 -14 ; session 73 [244] GETGLOBAL 11 -14 ; session Checking line length exceeding 80...

So apparently the session is a misused Lua global variable.

agentzh · 2017-02-11T21:22:45Z

t/sanity.t

+}
+
+lua_shared_dict healthcheck 1m;
+init_worker_by_lua '


We should use init_worker_by_lua_block here for newly added tests.

agentzh · 2017-02-11T21:23:11Z

t/sanity.t

+--- config
+    location = /t {
+        access_log off;
+        content_by_lua '


We should use content_by_lua_block in newly added tests. Please fix other similar places.

agentzh · 2017-02-11T21:24:41Z

Also, please rebase to the latest git master.

agentzh · 2017-02-11T21:26:10Z

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

Comply with lua releng requirements

ElvinEfendi · 2017-02-15T19:27:56Z

@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 · 2017-02-22T18:48:20Z

@agentzh any more comment on this PR?

ElvinEfendi · 2017-02-22T23:15:16Z

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

let users to pass custom server_name to tcpsock:sslhandshake function

add status_table API

solio · 2017-06-30T02:05:53Z

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

agentzh · 2017-07-10T17:51:07Z

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

agentzh · 2017-07-10T17:53:27Z

@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 · 2017-07-11T03:01:04Z

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 69751de which is already in downstream as well Shopify@69751de.

master...Shopify:master also shows "Able to merge":

ElvinEfendi · 2017-07-11T03:11:40Z

I'd also like to highlight that 6731f95 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: #44

agentzh · 2017-07-12T02:55:58Z

@ElvinEfendi Cool, thanks!

berlincount · 2017-07-17T14:04:02Z

hmmm, may I ping on this PR? :)

pavelnemirovsky · 2018-02-20T15:50:01Z

@agentzh when do u plan to merge it? Thx

agentzh · 2018-02-26T00:40:13Z

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

agentzh · 2018-02-26T00:42:45Z

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

ricardosantosalves · 2018-06-12T11:17:29Z

Hi,
Is there any update regarding this PR?

gentle-king · 2019-03-25T10:41:17Z

Dears, any update on this feature?

TWHOI · 2020-05-25T14:00:35Z

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

doujiang24 · 2020-05-25T16:46:35Z

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

leandromoreira · 2021-08-04T20:29:36Z

@agentzh is there any plan to merge this ?

leandromoreira · 2021-08-05T11:52:15Z

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 · 2021-08-06T20:31:01Z

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

xiaocang · 2021-08-09T15:06:06Z

README.markdown

            type = "http",
-
+            -- type = "https",


we do not add such a comment in the sample code, here just keep the options related to https, detailed explanation is placed below

xiaocang · 2021-08-09T15:11:43Z

lib/resty/upstream/healthcheck.lua

-    if typ ~= "http" then
-        return nil, "only \"http\" type is supported right now"
+    if typ ~= "http" and typ ~= "https" then
+        return nil, "no support for this protocol type"


It is better to point out in the error message what the unsupported type is

xiaocang · 2021-08-09T15:17:38Z

t/sanity.t

+
+server {
+    listen 12355;
+    ssl on;


the "ssl" directive is deprecated, use the "listen ... ssl" directive instead

xiaocang · 2021-08-09T15:17:48Z

t/sanity.t

+
+server {
+    listen 12355;
+    ssl on;


xiaocang · 2021-08-09T15:17:57Z

t/sanity.t

+
+server {
+    listen 12355;
+    ssl on;


xiaocang · 2021-08-09T15:18:05Z

t/sanity.t

+
+server {
+    listen 12355;
+    ssl on;


xiaocang · 2021-08-09T15:18:11Z

t/sanity.t

+
+server {
+    listen 12355;
+    ssl on;


xiaocang · 2021-08-09T15:18:18Z

t/sanity.t

+
+server {
+    listen 12355;
+    ssl on;


xiaocang · 2021-08-09T15:18:26Z

t/sanity.t

+
+server {
+    listen 12355;
+    ssl on;


xiaocang · 2021-08-09T15:18:34Z

t/sanity.t

+
+server {
+    listen 12355;
+    ssl on;


sitano and others added 16 commits August 24, 2015 18:09

add ssl support

3f077a0

reuse ctx.type to allow https

e9a373d

Merge branch 'master' into ssl_support

282f5b8

a blank line before the else line

0ee70dd

session reuse

212b25a

initial ssl testing

19a0b13

when ssl_verify, fail if certificate missmatch

20e6ba3

added tests for SSL health check

a63f03f

removed trailing whitespace

6819ea7

assert that ssl session is reused

3034363

make ssl session reuse optional

e568b74

document ssl_reuse_session flag

44fcbef

return test plan back with the adjusted number

9eaa01c

Merge pull request #1 from Shopify/ssl_support

0623e89

Ssl support

avoid permission requirement by using port > 1024

ba4bdf3

Merge pull request #2 from Shopify/use-different-port

62ff3e1

avoid permission requirement by using port > 1024

agentzh requested changes Feb 11, 2017

View reviewed changes

ElvinEfendi added 3 commits February 15, 2017 13:23

make sure session is declared as local variable

683b771

make sure line lengths do not exceed 80

63eaad5

use new *_by_lua_block directive

ec633a8

ElvinEfendi mentioned this pull request Feb 15, 2017

Comply with lua releng requirements Shopify/lua-resty-upstream-healthcheck#3

Merged

Merge pull request #3 from Shopify/comply-with-lua-releng-requirements

109526c

Comply with lua releng requirements

ElvinEfendi added 2 commits February 15, 2017 14:30

bump version

e09b29e

add missing comma

6227b39

let users to pass custom server_name to tcpsock:sslhandshake function

ce88ef0

Merge pull request #5 from Shopify/custom-server-name-option

bb498ff

let users to pass custom server_name to tcpsock:sslhandshake function

ElvinEfendi force-pushed the master branch from 42bde72 to bb498ff Compare February 22, 2017 23:34

ElvinEfendi added 2 commits February 22, 2017 18:35

add status_table API with tests

6731f95

Merge pull request #4 from Shopify/status-table-api

5a26fec

add status_table API

ElvinEfendi mentioned this pull request Jul 11, 2017

SSL support, completion of bb498ffb459b88cc9889d30f68a1e267bec102bb #44

Open

xiaocang requested changes Aug 9, 2021

View reviewed changes

Add SSL Support - completion of PR#5 #36

Are you sure you want to change the base?

Add SSL Support - completion of PR#5 #36

Conversation

ElvinEfendi commented Jan 27, 2017

ElvinEfendi commented Feb 2, 2017

charlescng commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agentzh commented Feb 11, 2017

agentzh commented Feb 11, 2017

ElvinEfendi commented Feb 15, 2017

ElvinEfendi commented Feb 22, 2017

ElvinEfendi commented Feb 22, 2017

solio commented Jun 30, 2017

agentzh commented Jul 10, 2017

agentzh commented Jul 10, 2017

ElvinEfendi commented Jul 11, 2017 • edited Loading

ElvinEfendi commented Jul 11, 2017 • edited Loading

agentzh commented Jul 12, 2017

berlincount commented Jul 17, 2017

pavelnemirovsky commented Feb 20, 2018

agentzh commented Feb 26, 2018

agentzh commented Feb 26, 2018

ricardosantosalves commented Jun 12, 2018

gentle-king commented Mar 25, 2019

TWHOI commented May 25, 2020

doujiang24 commented May 25, 2020

leandromoreira commented Aug 4, 2021

leandromoreira commented Aug 5, 2021

leandromoreira commented Aug 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi commented Jul 11, 2017 •

edited

Loading

ElvinEfendi commented Jul 11, 2017 •

edited

Loading