fix(config) error when specifying port for cassandra #2263
Conversation
kong/conf_loader.lua
Outdated
| errors[#errors+1] = "can't specify port in cassandra_contact_point, ".. | ||
| "use cassandra_port for that" | ||
| end | ||
| end |
There was a problem hiding this comment.
I think we should use something like:
local host, port = utils.normalize_ip(contact_point)
if host and port then
host = nil
port = "no port allowed"
end
if not host then
errors[#errors+1] = "bad Cassandra contact point '" . .contact_point . ."': " .. port
endThere was a problem hiding this comment.
also some tests are missing in the validation section here: https://github.com/Mashape/kong/blob/f9716acfe4f1c1417c747bad02aa01909ebb3208/spec/01-unit/02-conf_loader_spec.lua#L179
Tieske
added
the
pr/changes requested
Vermeille
force-pushed
the
patch-2
branch
2 times, most recently
from
6701cf5 to
1cbe588
Compare
|
You're right. Checking for hostname sanity was out of scope since it was not already there and is far less likely to be the root of an hard to spot mistake, but given how cheap and easy it is to implement, expect if in few minutes! |
spec/01-unit/02-conf_loader_spec.lua
Outdated
| local conf, err = conf_loader(nil, { | ||
| cassandra_contact_points = "some/really\\bad/host\name,addr2" | ||
| }) | ||
| assert.is_nil(conf) |
There was a problem hiding this comment.
the second \ also needs escaping, as well as testing for a proper error message, as I think it currently errors with something like "bad name" without any 'contact_points' context
There was a problem hiding this comment.
Better off using [[]] here to avoid escaping those \
There was a problem hiding this comment.
Also need to assert what the value of err is here
kong/conf_loader.lua
Outdated
| @@ -183,6 +183,16 @@ local function check_and_infer(conf) | |||
| "DCAwareRoundRobin policy is in use" | |||
| end | |||
|
|
|||
| for _, contact_point in pairs(conf.cassandra_contact_points) do | |||
There was a problem hiding this comment.
This value is a list, and as such, should be iterated over with ipairs.
kong/conf_loader.lua
Outdated
| errors[#errors+1] = err | ||
| elseif endpoint.port then | ||
| errors[#errors+1] = "bad Cassandra contact point '" .. contact_point .. | ||
| "': port must be specified in cassandra_port" |
There was a problem hiding this comment.
the alignment of the " character is off by 1 column here
kong/conf_loader.lua
Outdated
| local endpoint, err = utils.normalize_ip(contact_point) | ||
| if not endpoint then | ||
| errors[#errors+1] = err | ||
| elseif endpoint.port then |
There was a problem hiding this comment.
missing a blank line before the elseif statement
kong/conf_loader.lua
Outdated
| for _, contact_point in pairs(conf.cassandra_contact_points) do | ||
| local endpoint, err = utils.normalize_ip(contact_point) | ||
| if not endpoint then | ||
| errors[#errors+1] = err |
There was a problem hiding this comment.
We probably want to give more context to this error, like:
errors[#errors + 1] = "bad cassandra contact point '" .. contact_point .. "': " .. err
spec/01-unit/02-conf_loader_spec.lua
Outdated
| local conf, err = conf_loader(nil, { | ||
| cassandra_contact_points = "some/really\\bad/host\name,addr2" | ||
| }) | ||
| assert.is_nil(conf) |
There was a problem hiding this comment.
Better off using [[]] here to avoid escaping those \
spec/01-unit/02-conf_loader_spec.lua
Outdated
| local conf, err = conf_loader(nil, { | ||
| cassandra_contact_points = "some/really\\bad/host\name,addr2" | ||
| }) | ||
| assert.is_nil(conf) |
There was a problem hiding this comment.
Also need to assert what the value of err is here
|
Let me know if you think the error messages are not good enough :) |
spec/01-unit/02-conf_loader_spec.lua
Outdated
| }) | ||
| assert.equal("bad Cassandra contact point 'addr1:9042': port must be specified in cassandra_port", err) | ||
| assert.is_nil(conf) | ||
| end) |
There was a problem hiding this comment.
@Vermeille sorry, but hadn't noticed before you use an indentation of 4 where the rest of the code uses 2. Mind fixing that?
all well otherwise imo.
| for _, contact_point in ipairs(conf.cassandra_contact_points) do | ||
| local endpoint, err = utils.normalize_ip(contact_point) | ||
| if not endpoint then | ||
| errors[#errors+1] = "bad cassandra contact point '" .. contact_point .. |
There was a problem hiding this comment.
The two error messages inconsistently capitalize "cassandra". Ideally, both should be lowercase.
cassandra_contact_point does not tolerate port specification. Issue an error when the user specifies it by mistake.
|
@Vermeille Thx! |
No description provided.