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(healthchecks) add support for HTTPS in active health checks #3815

Merged
merged 4 commits into from Oct 1, 2018

Conversation

Projects
None yet
2 participants
@hishamhm
Copy link
Member

commented Sep 29, 2018

  • Introduces three new fields to active healthchecks configuration:
    • type - "tcp", "http" or "https", mirrorring lua-resty-healthcheck 0.6.0, bumped dependency
    • https_verify_certificate - boolean, mirrorring lua-resty-healthcheck 0.6.0
    • https_sni - explicitly give an SNI, overriding the hostname configured in the Target
  • Adds tests using HTTPS, based on lua-http (new dependency)
    • lua-http is used for HTTPS tests, the mock server based on LuaSocket remains used for HTTP tests for performance reasons (faster startup on separate threads which are repeatedly spawned for various short-lived tests)
    • This PR also updates the certs in spec/fixtures, which were expired

@hishamhm hishamhm force-pushed the feat/https-healthchecks branch from 81d2a5e to e6c1109 Sep 29, 2018

timeout = 1,
concurrency = 10,
http_path = "/",
https_sni = NO_DEFAULT,

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Oct 1, 2018

Member

https_sni expects a host typedef (which in turn, if of type string), but the default is a table? Doesn't this feel strange?

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Oct 1, 2018

Member

It seems like the host typedef allows for IP addresses (https://github.com/Kong/kong/blob/master/kong/db/schema/typedefs.lua#L10-L19), while as per RFC 6066, an SNI can only contain DNS hostnames:

Currently, the only server names supported are DNS hostnames; [...] Literal IPv4 and IPv6 addresses are not permitted in "HostName".

This probably warrants the creation of a new sni typedef.

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 1, 2018

Author Member

@thibaultcha good catch! updated the code with the new sni typedef, and also applied it to the snis.name attribute while I was at it!

local def
local def, required
if default == NO_DEFAULT then
default = nil

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Oct 1, 2018

Member

This answers the question in my first comment; ignore it :)

@hishamhm hishamhm force-pushed the feat/https-healthchecks branch from e6c1109 to ab2754f Oct 1, 2018

end

return true
end

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Oct 1, 2018

Member

If I were to specify ip:port, I would first receive an error stating that I should not specify a port, and after retrying, I would get an error that I should also not specify an IP. I think the "most not be an IP address" error should be thrown first.

Also, are we lacking tests for this function?

This comment has been minimized.

Copy link
@hishamhm

hishamhm Oct 1, 2018

Author Member

Also, are we lacking tests for this function?

It was being tested indirectly via the upstreams tests (as is the case for some other typedefs).

@thibaultcha

This comment has been minimized.

Copy link
Member

commented Oct 1, 2018

@hishamhm Ping on the failing CI suite - it looks related.

hishamhm added some commits Oct 1, 2018

feat(schema) adds `sni` typedef validating hostnames
Introduces `typedefs.sni` for verifying the validity of hostnames.
chore(deps) bump lua-resty-healthchecks to 0.6.0
Adds support to `https_verify_certificate` field.
feat(healthchecks) add support for HTTPS in active health checks
* Introduces three new fields to `active` healthchecks configuration:
  * `type` ("tcp", "http" or "https", mirrorring lua-resty-healthcheck)
  * `https_verify_certificate` (boolean, mirrorring lua-resty-healthcheck)
  * `https_sni` - explicitly give an SNI, overriding the hostname configured
    in the Target
* Adds tests using HTTPS, based on lua-http
  * lua-http is used for HTTPS tests, the mock server based on
    LuaSocket remains used for HTTP tests for performance reasons
    (faster startup on separate threads which are repeatedly spawned
    for various short-lived tests)
  * This commit also updates the certs in spec/fixtures, which were
    expired

@hishamhm hishamhm force-pushed the feat/https-healthchecks branch from ab2754f to 20011dd Oct 1, 2018

@hishamhm

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

Re-pushed. Apparently snis needs to accept non-hostname name entries otherwise Kong's own certs are not stored.

@thibaultcha
Copy link
Member

left a comment

LGTM!

@hishamhm hishamhm merged commit f3f7e68 into next Oct 1, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hishamhm hishamhm deleted the feat/https-healthchecks branch Oct 1, 2018

hbagdi added a commit to hbagdi/go-kong that referenced this pull request Apr 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.