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

add ssl support #5

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

add ssl support #5

wants to merge 2 commits into from

Conversation

sitano
Copy link

@sitano sitano commented Aug 24, 2015

I have added ssl handshake if requested to support https during health checking. It also able to skip cert checks which is fine for my use case. Session caching could be added in the future.

    local ok, err = hc.spawn_checker{
        shm = "healthcheck",  -- defined by "lua_shared_dict"
        upstream = "foo.com", -- defined by "upstream"
        type = "http",

        -- if you put this Lua snippet in separate .lua file,
        -- then you should write this instead: http_req = "GET /status HTTP/1.0\r\nHost: foo.com\r\n\r\n",
        http_req = "GET /status HTTP/1.0\\r\\nHost: foo.com\\r\\n\\r\\n",
                -- raw HTTP request for checking

        ssl = true, -- use https
        ssl_verify = false, -- verify SSL certs, see https://github.com/openresty/lua-nginx-module/pull/290

        interval = 2000,  -- run the check cycle every 2 sec
        timeout = 1000,   -- 1 sec is the timeout for network operations
        fall = 3,  -- # of successive failures before turning a peer down
        rise = 2,  -- # of successive successes before turning a peer up
        valid_statuses = {200, 302},  -- a list valid HTTP status code
        concurrency = 10,  -- concurrency level for test requests
    }

@sitano
Copy link
Author

sitano commented Aug 25, 2015

We can reuse ["type"] field instead of introducing separate ["ssl"] field. Missed it while developing a variant.

@sitano
Copy link
Author

sitano commented Aug 25, 2015

Now its

    local ok, err = hc.spawn_checker{
        shm = "healthcheck",
        upstream = "foo.com",

        type = "https",
        ssl_verify = false,

        http_req = "GET /status HTTP/1.0\\r\\nHost: foo.com\\r\\n\\r\\n",
    }

type = "http",


type = "https",
Copy link
Member

Choose a reason for hiding this comment

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

Because type = "http" is a more common case, we should at least put it in a comment here?

@agentzh
Copy link
Member

agentzh commented Aug 27, 2015

@sitano Thank you for the patch! Will you please add some corresponding test cases to the existing test suite (under t/) also? Thanks!

@sitano
Copy link
Author

sitano commented Aug 27, 2015

hi @agentzh . thanks for the comments . i will do all the changes you described . tests too . i was going to provide ssl session caching a bit later, but i will look into it .

@agentzh
Copy link
Member

agentzh commented Aug 27, 2015

@sitano Great! Thanks!

@pasikarkkainen
Copy link

Any news about getting the ssl support merged?

@agentzh
Copy link
Member

agentzh commented Jun 12, 2016

@pasikarkkainen Yeah, this is long overdue. Sorry about that. I'll look into this soon.

@pasikarkkainen
Copy link

Ping? :)

@binnn6
Copy link

binnn6 commented Oct 17, 2016

hi @agentzh, why not just change socket to resty.http? And we do't have to bind this module with ngx.upstream, so we can use this module with balancer by lua ?

@bompus
Copy link

bompus commented Oct 26, 2016

@agentzh Any update on this?

@agentzh
Copy link
Member

agentzh commented Oct 26, 2016

@doujiang24 Please review this PR when you have a chance. Thanks!

@agentzh
Copy link
Member

agentzh commented Oct 26, 2016

@bompus It seems that @sitano has not updated his branch according to the comments of my last round of review.

@agentzh
Copy link
Member

agentzh commented Oct 26, 2016

Also, there's conflicts between this branch and master that must be solved before this PR can be merged.

@sitano
Copy link
Author

sitano commented Oct 27, 2016

@agentzh yes, sorry. no still.

@pasikarkkainen
Copy link

@sitano is it likely you'll be able to fix/resolve the merge conflict, or should someone else pick up this work? Thanks.

@sitano
Copy link
Author

sitano commented Dec 9, 2016 via email

@ElvinEfendi
Copy link

This work is being completed at: Shopify#1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants