Skip to content

Commit

Permalink
chore(router) deprecate setting path handling v1 when router_flavor i…
Browse files Browse the repository at this point in the history 
…s traditional_compatible (#9290)

* chore(router) deny setting path handling v1 when router_flavor is traditional_compatible


Co-authored-by: Datong Sun <dndx@idndx.com>
  • Loading branch information
@windmgc @dndx
windmgc and dndx authored Aug 25, 2022
1 parent e2abf64 commit 14296db
Show file tree
Hide file tree
Showing 6 changed files with 76 additions and 46 deletions.
9 changes: 8 additions & 1 deletion autodoc/admin-api/data/admin-api.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -971,7 +971,14 @@ return {
#### Path handling algorithms
`"v0"` is the behavior used in Kong 0.x and 2.x. It treats `service.path`, `route.path` and request path as
{:.note}
> **Note**: Path handling algorithms v1 was deprecated in Kong 3.0. From Kong 3.0, when `router_flavor`
> is set to `expressions`, `route.path_handling` will be unconfigurable and the path handling behavior
> will be `"v0"`; when `router_flavor` is set to `traditional_compatible`, the path handling behavior
> will be `"v0"` regardless of the value of `route.path_handling`. Only `router_flavor` = `traditional`
> will support path_handling `"v1'` behavior.
`"v0"` is the behavior used in Kong 0.x, 2.x and 3.x. It treats `service.path`, `route.path` and request path as
*segments* of a URL. It will always join them via slashes. Given a service path `/s`, route path `/r`
and request path `/re`, the concatenated path will be `/s/re`. If the resulting path is a single slash,
no further transformation is done to it. If it's longer, then the trailing slash is removed.
Expand Down
24 changes: 22 additions & 2 deletions kong/db/schema/entities/routes.lua
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
local typedefs = require("kong.db.schema.typedefs")
local atc = require("kong.router.atc")
local router = require("resty.router.router")
local deprecation = require("kong.deprecation")

local kong_router_flavor = kong and kong.configuration and kong.configuration.router_flavor

if kong and kong.configuration and kong.configuration.router_flavor == "expressions" then
if kong_router_flavor == "expressions" then
return {
name = "routes",
primary_key = { "id" },
Expand Down Expand Up @@ -59,6 +61,7 @@ if kong and kong.configuration and kong.configuration.router_flavor == "expressi
},
}

-- router_flavor in ('traditional_compatible', 'traditional')
else
return {
name = "routes",
Expand Down Expand Up @@ -122,6 +125,23 @@ else
then_match = { len_eq = 0 },
then_err = "'snis' can only be set when 'protocols' is 'grpcs', 'https', 'tls' or 'tls_passthrough'",
}},
},
{ custom_entity_check = {
field_sources = { "path_handling" },
fn = function(entity)
if entity.path_handling == "v1" then
if kong_router_flavor == "traditional" then
deprecation("path_handling='v1' is deprecated and will be removed in future version, " ..
"please use path_handling='v0' instead", { after = "3.0", })

elseif kong_router_flavor == "traditional_compatible" then
deprecation("path_handling='v1' is deprecated and will not work under traditional_compatible " ..
"router_flavor, please use path_handling='v0' instead", { after = "3.0", })
end
end

return true
end,
}},
},
}
end
41 changes: 21 additions & 20 deletions spec/01-unit/01-db/01-schema/11-declarative_config/03-flatten_spec.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,11 +167,12 @@ describe("declarative config: flatten", function()
end)

it("accepts field names with the same name as entities", function()
-- "snis" is also an entity
local config = assert(lyaml.load([[
_format_version: "1.1"
routes:
- name: foo
path_handling: v1
path_handling: v0
protocols: ["tls"]
snis:
- "example.com"
Expand All @@ -197,7 +198,7 @@ describe("declarative config: flatten", function()
snis = { "example.com" },
sources = null,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
updated_at = 1234567890,
request_buffering = true,
response_buffering = true,
Expand Down Expand Up @@ -346,7 +347,7 @@ describe("declarative config: flatten", function()
host: example.com
routes:
- name: r1
path_handling: v1
path_handling: v0
paths: [/]
service: svc1
consumers:
Expand Down Expand Up @@ -443,7 +444,7 @@ describe("declarative config: flatten", function()
snis = null,
sources = null,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
updated_at = 1234567890,
request_buffering = true,
response_buffering = true,
Expand Down Expand Up @@ -712,7 +713,7 @@ describe("declarative config: flatten", function()
host: example.com
protocol: https
routes:
- path_handling: v1
- path_handling: v0
paths:
- /path
]]))
Expand All @@ -737,7 +738,7 @@ describe("declarative config: flatten", function()
snis = null,
sources = null,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
tags = null,
updated_at = 1234567890,
request_buffering = true,
Expand Down Expand Up @@ -774,22 +775,22 @@ describe("declarative config: flatten", function()
host: example.com
protocol: https
routes:
- path_handling: v1
- path_handling: v0
paths:
- /path
name: r1
- path_handling: v1
- path_handling: v0
hosts:
- example.com
name: r2
- path_handling: v1
- path_handling: v0
methods: ["GET", "POST"]
name: r3
- name: bar
host: example.test
port: 3000
routes:
- path_handling: v1
- path_handling: v0
paths:
- /path
hosts:
Expand Down Expand Up @@ -818,7 +819,7 @@ describe("declarative config: flatten", function()
snis = null,
sources = null,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
tags = null,
updated_at = 1234567890,
request_buffering = true,
Expand All @@ -842,7 +843,7 @@ describe("declarative config: flatten", function()
snis = null,
sources = null,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
tags = null,
updated_at = 1234567890,
request_buffering = true,
Expand All @@ -866,7 +867,7 @@ describe("declarative config: flatten", function()
snis = null,
sources = null,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
tags = null,
updated_at = 1234567890,
request_buffering = true,
Expand All @@ -890,7 +891,7 @@ describe("declarative config: flatten", function()
snis = null,
sources = null,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
tags = null,
updated_at = 1234567890,
request_buffering = true,
Expand Down Expand Up @@ -949,7 +950,7 @@ describe("declarative config: flatten", function()
protocol: https
routes:
- name: foo
path_handling: v1
path_handling: v0
methods: ["GET"]
plugins:
]]))
Expand All @@ -976,7 +977,7 @@ describe("declarative config: flatten", function()
snis = null,
sources = null,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
updated_at = 1234567890,
request_buffering = true,
response_buffering = true,
Expand Down Expand Up @@ -1016,7 +1017,7 @@ describe("declarative config: flatten", function()
protocol: https
routes:
- name: foo
path_handling: v1
path_handling: v0
methods: ["GET"]
plugins:
- name: key-auth
Expand All @@ -1028,7 +1029,7 @@ describe("declarative config: flatten", function()
port: 3000
routes:
- name: bar
path_handling: v1
path_handling: v0
paths:
- /
plugins:
Expand Down Expand Up @@ -1142,7 +1143,7 @@ describe("declarative config: flatten", function()
snis = null,
sources = null,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
tags = null,
updated_at = 1234567890,
request_buffering = true,
Expand All @@ -1166,7 +1167,7 @@ describe("declarative config: flatten", function()
snis = null,
sources = null,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
tags = null,
updated_at = 1234567890,
request_buffering = true,
Expand Down
2 changes: 1 addition & 1 deletion spec/01-unit/01-db/01-schema/11-declarative_config/04-on-the-fly-migration_spec.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ describe("declarative config: on the fly migration", function()
tags: [hello, world]
routes:
- name: foo
path_handling: v1
path_handling: v0
protocols: ["https"]
paths: ["/regex.+", "/prefix" ]
snis:
Expand Down
26 changes: 13 additions & 13 deletions spec/02-integration/03-db/02-db_core_entities_spec.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -520,7 +520,7 @@ for _, strategy in helpers.each_strategy() do
protocols = { "http" },
hosts = { "example.com" },
service = assert(db.services:insert({ host = "service.com" })),
path_handling = "v1",
path_handling = "v0",
}, { nulls = true, workspace = "8a139c70-49a1-4ba2-98a6-bb36f534269d", })
assert.is_nil(route)
assert.is_string(err)
Expand Down Expand Up @@ -685,7 +685,7 @@ for _, strategy in helpers.each_strategy() do
protocols = { "http" },
hosts = { "example.com" },
service = assert(db.services:insert({ host = "service.com" })),
path_handling = "v1",
path_handling = "v0",
}, { nulls = true })
assert.is_nil(err_t)
assert.is_nil(err)
Expand All @@ -711,7 +711,7 @@ for _, strategy in helpers.each_strategy() do
regex_priority = 0,
preserve_host = false,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
tags = ngx.null,
service = route.service,
https_redirect_status_code = 426,
Expand All @@ -728,7 +728,7 @@ for _, strategy in helpers.each_strategy() do
paths = { "/example" },
regex_priority = 3,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
service = bp.services:insert(),
}, { nulls = true })
assert.is_nil(err_t)
Expand All @@ -754,7 +754,7 @@ for _, strategy in helpers.each_strategy() do
destinations = ngx.null,
regex_priority = 3,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
tags = ngx.null,
preserve_host = false,
service = route.service,
Expand All @@ -771,7 +771,7 @@ for _, strategy in helpers.each_strategy() do
paths = { "/example" },
regex_priority = 3,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
}, { nulls = true })
assert.is_nil(err_t)
assert.is_nil(err)
Expand All @@ -797,7 +797,7 @@ for _, strategy in helpers.each_strategy() do
tags = ngx.null,
regex_priority = 3,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
preserve_host = false,
service = ngx.null,
https_redirect_status_code = 426,
Expand Down Expand Up @@ -1102,7 +1102,7 @@ for _, strategy in helpers.each_strategy() do
protocols = { "https" },
hosts = { "example.com" },
regex_priority = 5,
path_handling = "v1"
path_handling = "v0"
})
assert.is_nil(err_t)
assert.is_nil(err)
Expand All @@ -1116,7 +1116,7 @@ for _, strategy in helpers.each_strategy() do
paths = route.paths,
regex_priority = 5,
strip_path = route.strip_path,
path_handling = "v1",
path_handling = "v0",
preserve_host = route.preserve_host,
tags = route.tags,
service = route.service,
Expand All @@ -1135,7 +1135,7 @@ for _, strategy in helpers.each_strategy() do
local route = bp.routes:insert({
hosts = { "example.com" },
methods = { "GET" },
path_handling = "v1",
path_handling = "v0",
})

local new_route, err, err_t = db.routes:update({ id = route.id }, {
Expand All @@ -1151,7 +1151,7 @@ for _, strategy in helpers.each_strategy() do
hosts = route.hosts,
regex_priority = route.regex_priority,
strip_path = route.strip_path,
path_handling = "v1",
path_handling = "v0",
preserve_host = route.preserve_host,
tags = route.tags,
service = route.service,
Expand Down Expand Up @@ -1978,7 +1978,7 @@ for _, strategy in helpers.each_strategy() do
protocols = { "http" },
hosts = { "example.com" },
service = service,
path_handling = "v1",
path_handling = "v0",
}, { nulls = true })
assert.is_nil(err_t)
assert.is_nil(err)
Expand All @@ -1997,7 +1997,7 @@ for _, strategy in helpers.each_strategy() do
destinations = ngx.null,
regex_priority = 0,
strip_path = true,
path_handling = "v1",
path_handling = "v0",
preserve_host = false,
tags = ngx.null,
service = {
Expand Down
20 changes: 11 additions & 9 deletions spec/02-integration/05-proxy/02-router_spec.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2095,16 +2095,18 @@ for _, strategy in helpers.each_strategy() do

for i, line in ipairs(path_handling_tests) do
for j, test in ipairs(line:expand()) do
routes[#routes + 1] = {
strip_path = test.strip_path,
path_handling = test.path_handling,
paths = test.route_path and { test.route_path } or nil,
hosts = { "localbin-" .. i .. "-" .. j .. ".com" },
service = {
name = "plain_" .. i .. "-" .. j,
path = test.service_path,
if flavor == "traditional" or test.path_handling == "v0" then
routes[#routes + 1] = {
strip_path = test.strip_path,
path_handling = test.path_handling,
paths = test.route_path and { test.route_path } or nil,
hosts = { "localbin-" .. i .. "-" .. j .. ".com" },
service = {
name = "plain_" .. i .. "-" .. j,
path = test.service_path,
}
}
}
end
end
end

Expand Down

0 comments on commit 14296db

Please sign in to comment.