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(origins) Add ability to override hosts from kong configuration on a per-kong-node basis #3679

Merged
merged 2 commits into from Sep 6, 2018

Conversation

Projects
None yet
3 participants
@james-callahan
Copy link
Contributor

commented Aug 7, 2018

Summary

This allows (via configuration, to facilitate it on a per-node basis instead of cluster-wide) overriding the scheme, host and port before resolution occurs in the balancer phase.

Full changelog

  • feat(utils) normalize ipv6 address when formatting
  • feat(core) add 'origins' configuration property

@james-callahan james-callahan force-pushed the james-callahan:feat/origins branch from 91796d8 to 3d18b95 Aug 7, 2018

@james-callahan james-callahan requested a review from thibaultcha Aug 7, 2018

@james-callahan james-callahan force-pushed the james-callahan:feat/origins branch 2 times, most recently from 686534b to ccf8775 Aug 7, 2018

@james-callahan

This comment was marked as resolved.

Copy link
Contributor Author

commented Aug 21, 2018

Needs tests.

@james-callahan james-callahan force-pushed the james-callahan:feat/origins branch 2 times, most recently from c4b433f to 71eb68b Aug 21, 2018

@@ -752,6 +752,16 @@ end
-- which do not support sending it)
-- @return true on success, nil+error message+status code otherwise
local function execute(target, ctx)
do -- Check for KONG_ORIGINS override
local origin_key = target.scheme .. "://" .. utils.format_host(target)

This comment has been minimized.

Copy link
@Tieske

Tieske Aug 24, 2018

Member

Since this comparison might be expensive maybe cache the results?

Does this cater for matching different formats? casing in names, shortening in ipv6? could use some accompanying tests

This comment has been minimized.

Copy link
@james-callahan

james-callahan Aug 24, 2018

Author Contributor

What do you mean "comparison" here? What would you cache?

This comment has been minimized.

Copy link
@Tieske

Tieske Aug 25, 2018

Member

sorry bad wording. Creating the key and then looking it up (=comparison) in the origins table.

so if origins has

{
  ["http://[::1]:80"] = {
    scheme = "http",
    host = "somewhere.else",
    port = 80,
  }
}

and incoming is http://[0000::1]:80 then it doesn't match, so you need to normalize. That is expensive, and that is something you could cache.

@@ -0,0 +1,121 @@
local helpers = require "spec.helpers"

This comment has been minimized.

Copy link
@Tieske

Tieske Aug 24, 2018

Member

this file could use some more vertical white space

@james-callahan james-callahan force-pushed the james-callahan:feat/origins branch from 2d9f1ed to a073e8e Aug 28, 2018

@@ -0,0 +1,121 @@
local helpers = require "spec.helpers"

describe("origins config option", function()

This comment was marked as resolved.

Copy link
@thibaultcha

thibaultcha Aug 29, 2018

Member

Those tests really belong to the conf_loader_spec suite in the unit tests - I missed this before.


-- set the upstream scheme as balancer selected
local upstream_scheme = balancer_data.scheme
var.upstream_scheme = upstream_scheme

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 29, 2018

Member

note 1: this is a breaking change for any plugin out there that relies on ngx.var.upstream_scheme = "...", they now need to do ngx.ctx.balancer_data.scheme = "...".

note 2: a good way to avoid such breaking changes would be to provide a PDK API for this operation (none exists as of today in the kong.service module); this is outside the scope of this PR of course.

note 3: the var.upstream_scheme = match_t.upstream_scheme assignment in the access.before handler could arguably be removed, since plugins making use of it are already broken.

It is hard to know if such a change would have any impact - I myself am not sure if we offer any plugins making use of this variable (forward-proxy seems to hard-code http://, luckily). I do have a vague memory of it being mentioned from community users though.

This comment has been minimized.

Copy link
@james-callahan

james-callahan Sep 5, 2018

Author Contributor

After some research, the PDK does have a function to modify the scheme. request.set_scheme.

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Sep 5, 2018

Member

Indeed; after our irl this does mean that the patch in its current form breaks the PDK. Options were discussed, for now that is a blocker for this PR.

@@ -752,6 +752,17 @@ end
-- which do not support sending it)
-- @return true on success, nil+error message+status code otherwise
local function execute(target, ctx)
do -- Check for KONG_ORIGINS override
local origin_key =
target.scheme .. "://" .. utils.format_host(utils.normalize_ip(target))

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 29, 2018

Member

Something caught my eye, not sure what I may have missed: utils.normalize_ip() expects a string, but target is a table here, correct?

end

dao:truncate_tables()
assert(db:truncate())

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 29, 2018

Member

Better to let the get_db_utils() utility do this type of work (above line as well) for speed and debug reasons.

})
assert.res_status(502, res)
proxy_client:close()
helpers.stop_kong(nil, nil, true)

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 29, 2018

Member

stop_kong() only accept 2 arguments nowadays, this can be written as helpers.stop_kong() now

-- Check for duplicates
local from_origin = from_scheme .. "://" .. from_authority
if seen_origins[from_origin] then
return nil, "duplicate origin"

This comment has been minimized.

Copy link
@thibaultcha

thibaultcha Aug 29, 2018

Member

This message is a bit unfriendly, isn't it?

  1. It does not specify which origin is duplicated
  2. If more than one origin is duplicated, the user will only find out after several trial and errors. Something that the conf_loader tries hard to avoid. Instead, it always tries to be a good citizen and report all errors at once, to avoid back and forth from the user.

@james-callahan james-callahan force-pushed the james-callahan:feat/origins branch 4 times, most recently from 40fad93 to df5ec6e Sep 6, 2018

@thibaultcha
Copy link
Member

left a comment

Approving but please do not merge - will be done manually.

@thibaultcha thibaultcha force-pushed the james-callahan:feat/origins branch from df5ec6e to 30c2cce Sep 6, 2018

james-callahan added some commits Sep 5, 2018

feat(core) add 'origins' configuration property
This allows (via configuration, to facilitate it on a per-node basis instead
of cluster-wide) overriding the scheme, host and port before resolution
occurs in the balancer phase.

The option does not appear in the configuration file, as it is intended to be
used only via the environment variable `KONG_ORIGINS`.

@james-callahan james-callahan force-pushed the james-callahan:feat/origins branch from 30c2cce to e245c23 Sep 6, 2018

@thibaultcha thibaultcha merged commit 8eb0ba4 into Kong:next Sep 6, 2018

1 check passed

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

@james-callahan james-callahan deleted the james-callahan:feat/origins branch Sep 6, 2018

@@ -786,7 +786,7 @@ _M.format_host = function(p1, p2)
return nil, "cannot format type '" .. t .. "'"
end
if typ == "ipv6" and not find(host, "[", nil, true) then
return "[" .. host .. "]" .. (port and ":" .. port or "")
return "[" .. _M.normalize_ipv6(host) .. "]" .. (port and ":" .. port or "")

This comment has been minimized.

Copy link
@Tieske

Tieske Sep 7, 2018

Member

This is not ok.

There are 3 functions for normalizing/validating, and there is 1 for formatting (this one). With this change the formatter:

  • normalizes/validates IPv6
  • does not normalize/validate IPv4 nor names
  • suddenly becomes a lot more expensive, due to the 1st point above

This comment has been minimized.

Copy link
@james-callahan

james-callahan Sep 7, 2018

Author Contributor

All of our other "normalization" functions don't actually do any normalization. They just do validation.

This comment has been minimized.

Copy link
@Tieske

Tieske Sep 7, 2018

Member

If we change behaviour, it should be consistent. Currently it moved the other direction

This comment has been minimized.

Copy link
@Tieske

Tieske Sep 7, 2018

Member

Also the documentation is no longer correct add the behaviour of the function changed.

assert.are.equal("[::1]", utils.format_host("::1"))
assert.are.equal("[::1]:80", utils.format_host("::1", 80))
assert.are.equal("[0000:0000:0000:0000:0000:0000:0000:0001]", utils.format_host("::1"))
assert.are.equal("[0000:0000:0000:0000:0000:0000:0000:0001]:80", utils.format_host("::1", 80))

This comment has been minimized.

Copy link
@Tieske

Tieske Sep 7, 2018

Member

this should be formatting only, not normalizing

This comment has been minimized.

Copy link
@james-callahan

james-callahan Sep 7, 2018

Author Contributor

Thibault and I discussed yesterday and decided that formatting is allowed to include normalisation.

@@ -0,0 +1,61 @@
local helpers = require "spec.helpers"

describe("origins config option", function()

This comment has been minimized.

Copy link
@Tieske

Tieske Sep 7, 2018

Member

Please add tests for the proper matching of configured (non-normalized) addresses to the normalized ones in origins.

edit: 'incoming' -> 'configured'

This comment has been minimized.

Copy link
@Tieske

Tieske Sep 7, 2018

Member

@james Callahan with my current understanding of the code it will simply fail if you configure a service with an upstream url http://192.168.001.001/somewhere due to a lack of normalizing the ipv4.
Also be aware that plugins might change the upstream properties, so the normalization must be done in each request. Just updating the service entity is not enough.

This comment has been minimized.

Copy link
@Tieske

This comment has been minimized.

Copy link
@james-callahan

james-callahan Sep 7, 2018

Author Contributor

IPv4 addresses are not allowed to contain leading zeros: that should be a syntax error according to RFC 3986. If we don't check that on input then that is where the fix needs to be.

This comment has been minimized.

Copy link
@Tieske

Tieske Sep 7, 2018

Member

That does not matter. Since the utility functions are now inconsistent. And those functions serve a bigger purpose than just this change.

And even then the test cases should be added to show it is validated in the right places.

And finally the function is expensive and should not be on a hot code path imo.

else
-- Validate 'from'
local from_authority, err =
utils.format_host(utils.normalize_ip(from_host_port))

This comment has been minimized.

Copy link
@Tieske

Tieske Sep 7, 2018

Member

This code now normalized an ipv6 address twice. Once before calling format and once inside the format call.

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.