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.

Sign up for GitHub

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
Merged

feat(healthchecks) add support for HTTPS in active health checks #3815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
925e402
feat(schema) adds `sni` typedef validating hostnames
hishamhm Oct 1, 2018
b8d2608
chore(deps) add lua-http dependency
hishamhm Sep 28, 2018
ea8568a
chore(deps) bump lua-resty-healthchecks to 0.6.0
hishamhm Sep 28, 2018
20011dd
feat(healthchecks) add support for HTTPS in active health checks
hishamhm Sep 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion kong-0.14.1-0.rockspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ dependencies = {
"lua-cassandra == 1.3.2",
"pgmoon == 1.8.0",
"luatz == 0.3",
"http == 0.2",
"lua_system_constants == 0.1.2",
"lua-resty-iputils == 0.3.0",
"luaossl == 20180708",
Expand All @@ -31,7 +32,7 @@ dependencies = {
"lua-resty-dns-client == 2.2.0",
"lua-resty-worker-events == 0.3.3",
"lua-resty-mediador == 0.1.2",
"lua-resty-healthcheck == 0.5.0",
"lua-resty-healthcheck == 0.6.0",
"lua-resty-cookie == 0.1.0",
"lua-resty-mlcache == 2.2.0",
-- external Kong plugins
Expand Down
44 changes: 38 additions & 6 deletions kong/db/schema/entities/upstreams.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,30 @@ local header_name = Schema.define {
}


local healthchecks_defaults = {
local check_type = Schema.define {
type = "string",
one_of = { "tcp", "http", "https" },
default = "http",
}


local check_verify_certificate = Schema.define {
type = "boolean",
default = true,
}


local NO_DEFAULT = {}


local healthchecks_config = {
active = {
type = "http",
timeout = 1,
concurrency = 10,
http_path = "/",
https_sni = NO_DEFAULT,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@hishamhm hishamhm Oct 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

https_verify_certificate = true,
healthy = {
interval = 0, -- 0 = probing disabled by default
http_statuses = { 200, 302 },
Expand All @@ -75,6 +94,7 @@ local healthchecks_defaults = {
},
},
passive = {
type = "http",
healthy = {
http_statuses = { 200, 201, 202, 203, 204, 205, 206, 207, 208, 226,
300, 301, 302, 303, 304, 305, 306, 307, 308 },
Expand All @@ -91,6 +111,7 @@ local healthchecks_defaults = {


local types = {
type = check_type,
timeout = seconds,
concurrency = positive_int,
interval = seconds,
Expand All @@ -100,26 +121,37 @@ local types = {
http_failures = positive_int_or_zero,
http_path = typedefs.path,
http_statuses = http_statuses,
https_sni = typedefs.sni,
https_verify_certificate = check_verify_certificate,
}


local function gen_fields(tbl)
local fields = {}
local count = 0
for name, default in pairs(tbl) do
local typ = types[name]
local def
local def, required
if default == NO_DEFAULT then
default = nil
Copy link
Member

Choose a reason for hiding this comment

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

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

required = false
tbl[name] = nil
end
if typ then
def = typ{ default = default }
def = typ{ default = default, required = required }
else
def = { type = "record", fields = gen_fields(default), default = default }
end
count = count + 1
fields[count] = { [name] = def }
end
return fields
return fields, tbl
end


local healthchecks_fields, healthchecks_defaults = gen_fields(healthchecks_config)


local r = {
name = "upstreams",
primary_key = { "id" },
Expand All @@ -137,9 +169,9 @@ local r = {
{ slots = { type = "integer", default = 10000, between = { 10, 2^16 }, }, },
{ healthchecks = { type = "record",
default = healthchecks_defaults,
fields = gen_fields(healthchecks_defaults),
fields = healthchecks_fields,
}, },
},
},
entity_checks = {
-- hash_on_header must be present when hashing on header
{ conditional = {
Expand Down
24 changes: 24 additions & 0 deletions kong/db/schema/typedefs.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,24 @@ local function validate_name(name)
end


local function validate_sni(host)
local res, err_or_port = utils.normalize_ip(host)
if type(err_or_port) == "string" and err_or_port ~= "invalid port number" then
return nil, "invalid value: " .. host
end

if res.type ~= "name" then
return nil, "must not be an IP"
end

if err_or_port == "invalid port number" or type(res.port) == "number" then
return nil, "must not have a port"
end

return true
end
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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



local typedefs = {}


Expand Down Expand Up @@ -161,4 +179,10 @@ typedefs.name = Schema.define {
}


typedefs.sni = Schema.define {
type = "string",
custom_validator = validate_sni,
}


return typedefs
19 changes: 16 additions & 3 deletions spec/01-unit/000-new-dao/01-schema/03-typedefs_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,20 @@ local typedefs = require("kong.db.schema.typedefs")
describe("typedefs", function()
local a_valid_uuid = "cbb297c0-a956-486d-ad1d-f9b42df9465a"
local a_blank_uuid = "00000000-0000-0000-0000-000000000000"


it("features sni typedef", function()
local Test = Schema.new({
fields = {
{ f = typedefs.sni }
}
})
assert.truthy(Test:validate({ f = "example.com" }))
assert.truthy(Test:validate({ f = "9foo.te-st.bar.test" }))
assert.falsy(Test:validate({ f = "127.0.0.1" }))
assert.falsy(Test:validate({ f = "example.com:80" }))
assert.falsy(Test:validate({ f = "[::1]" }))
end)

it("features port typedef", function()
local Test = Schema.new({
fields = {
Expand All @@ -19,7 +32,7 @@ describe("typedefs", function()
assert.falsy(Test:validate({ f = 65536 }))
assert.falsy(Test:validate({ f = 65536.1 }))
end)

it("features protocol typedef", function()
local Test = Schema.new({
fields = {
Expand Down Expand Up @@ -77,7 +90,7 @@ describe("typedefs", function()
local data = Test:process_auto_fields({})
assert.truthy(Test:validate(data))
assert.same(data.f, 120)

data = Test:process_auto_fields({ f = 900 })
assert.truthy(Test:validate(data))
assert.same(data.f, 900)
Expand Down
25 changes: 24 additions & 1 deletion spec/01-unit/000-new-dao/01-schema/08-upstreams_spec.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,9 +152,11 @@ describe("load upstreams", function()
assert.same(u.slots, 10000)
assert.same(u.healthchecks, {
active = {
type = "http",
timeout = 1,
concurrency = 10,
http_path = "/",
https_verify_certificate = true,
healthy = {
interval = 0,
http_statuses = { 200, 302 },
Expand All @@ -170,6 +172,7 @@ describe("load upstreams", function()
},
},
passive = {
type = "http",
healthy = {
http_statuses = { 200, 201, 202, 203, 204, 205, 206, 207, 208, 226,
300, 301, 302, 303, 304, 305, 306, 307, 308 },
Expand Down Expand Up @@ -221,6 +224,10 @@ describe("load upstreams", function()
local zero_integer = "value should be between 0 and 2147483648"
local status_code = "value should be between 100 and 999"
local integer = "expected an integer"
local boolean = "expected a boolean"
local invalid_host = "invalid value: "
local invalid_host_port = "must not have a port"
local invalid_ip = "must not be an IP"
local tests = {
{{ active = { timeout = -1 }}, seconds },
{{ active = { timeout = 1e+42 }}, seconds },
Expand All @@ -229,6 +236,20 @@ describe("load upstreams", function()
{{ active = { concurrency = -10 }}, pos_integer },
{{ active = { http_path = "" }}, "length must be at least 1" },
{{ active = { http_path = "ovo" }}, "should start with: /" },
{{ active = { https_sni = "127.0.0.1", }}, invalid_ip },
{{ active = { https_sni = "127.0.0.1:8080", }}, invalid_ip },
{{ active = { https_sni = "/example", }}, invalid_host },
{{ active = { https_sni = ".example", }}, invalid_host },
{{ active = { https_sni = "example.", }}, invalid_host },
{{ active = { https_sni = "example:", }}, invalid_host },
{{ active = { https_sni = "mock;bin", }}, invalid_host },
{{ active = { https_sni = "example.com/org", }}, invalid_host },
{{ active = { https_sni = "example-.org", }}, invalid_host },
{{ active = { https_sni = "example.org-", }}, invalid_host },
{{ active = { https_sni = "hello..example.com", }}, invalid_host },
{{ active = { https_sni = "hello-.example.com", }}, invalid_host },
{{ active = { https_sni = "example.com:1234", }}, invalid_host_port },
{{ active = { https_verify_certificate = "ovo", }}, boolean },
{{ active = { healthy = { interval = -1 }}}, seconds },
{{ active = { healthy = { interval = 1e+42 }}}, seconds },
{{ active = { healthy = { http_statuses = 404 }}}, "expected an array" },
Expand Down Expand Up @@ -292,7 +313,7 @@ describe("load upstreams", function()
repeat
leaf = leaf[next(leaf)]
until type(leaf) ~= "table" or type(next(leaf)) ~= "string"
assert.equal(test[2], leaf, inspect(err))
assert.match(test[2], leaf, 1, true, inspect(err))
end
end)

Expand All @@ -304,6 +325,8 @@ describe("load upstreams", function()
{ active = { concurrency = 2 }},
{ active = { http_path = "/" }},
{ active = { http_path = "/test" }},
{ active = { https_sni = "example.com" }},
{ active = { https_verify_certificate = false }},
{ active = { healthy = { interval = 0 }}},
{ active = { healthy = { http_statuses = { 200, 300 } }}},
{ active = { healthy = { successes = 2 }}},
Expand Down
Loading