bzp2010 commented on code in PR #12668:
URL: https://github.com/apache/apisix/pull/12668#discussion_r2428130574
##########
apisix/ssl/router/radixtree_sni.lua:
##########
@@ -55,24 +55,30 @@ local function create_router(ssl_items)
if type(ssl.value.snis) == "table" and #ssl.value.snis > 0 then
sni = core.table.new(0, #ssl.value.snis)
for _, s in ipairs(ssl.value.snis) do
- j = j + 1
- sni[j] = s:reverse()
+ if s ~= "*" then
+ j = j + 1
+ sni[j] = s:reverse()
+ end
end
else
- sni = ssl.value.sni:reverse()
+ if ssl.value.sni ~= "*" then
+ sni = ssl.value.sni:reverse()
+ end
Review Comment:
ditto
##########
apisix/ssl/router/radixtree_sni.lua:
##########
@@ -55,24 +55,30 @@ local function create_router(ssl_items)
if type(ssl.value.snis) == "table" and #ssl.value.snis > 0 then
sni = core.table.new(0, #ssl.value.snis)
for _, s in ipairs(ssl.value.snis) do
- j = j + 1
- sni[j] = s:reverse()
+ if s ~= "*" then
+ j = j + 1
+ sni[j] = s:reverse()
+ end
end
else
- sni = ssl.value.sni:reverse()
+ if ssl.value.sni ~= "*" then
+ sni = ssl.value.sni:reverse()
+ end
end
- idx = idx + 1
- route_items[idx] = {
- paths = sni,
- handler = function (api_ctx)
- if not api_ctx then
- return
+ if sni and (type(sni) == "table" and #sni > 0 or type(sni) ==
"string") then
Review Comment:
This seems to cover all existing scenarios. Is this necessary?
##########
apisix/ssl/router/radixtree_sni.lua:
##########
@@ -171,6 +176,33 @@ function _M.match_and_set(api_ctx, match_only, alt_sni)
local sni_rev = sni:reverse()
local ok = radixtree_router:dispatch(sni_rev, nil, api_ctx)
+
+ -- if no SSL matched, try to find a wildcard SSL
Review Comment:
Can't radix trees perform wildcard matching directly? Why do wildcard
matches work fine in our HTTP routes (`/test/*`) and SSL SNI (`*.example.com`)?
In my view, there's no fundamental difference between `*.example.com` and
`*`. If the former works correctly after reverse resolution, the latter should
also function out of the box. We shouldn't need to add any special logic for
this.
##########
apisix/ssl/router/radixtree_sni.lua:
##########
@@ -55,24 +55,30 @@ local function create_router(ssl_items)
if type(ssl.value.snis) == "table" and #ssl.value.snis > 0 then
sni = core.table.new(0, #ssl.value.snis)
for _, s in ipairs(ssl.value.snis) do
- j = j + 1
- sni[j] = s:reverse()
+ if s ~= "*" then
+ j = j + 1
+ sni[j] = s:reverse()
+ end
Review Comment:
<img width="327" height="53" alt="Image"
src="https://github.com/user-attachments/assets/629e7741-aa9e-4143-ae7e-e2e49ae5485c";
/>
It is reasonable not to perform additional processing, as it does not cause
any issues.
This is not necessary, as `("*"):reverse()` functions correctly. Even if
this extra check were added, it would only apply in extremely rare cases and
would require explanation for maintainability. I do not believe it differs from
normal sni case.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
