From c5dcced1cf90e2ffa5b73583784fc9bd34706aac Mon Sep 17 00:00:00 2001 From: YuanSheng Wang Date: Sun, 4 Oct 2020 13:12:28 +0800 Subject: [PATCH] bugfix: when service works with upstream that contains a domain name, the merged configuration should not always changing. (#2322) --- apisix/init.lua | 102 ++++----- apisix/upstream.lua | 25 +-- t/plugin/key-auth-upstream-domain-node.t | 258 +++++++++++++++++++++++ 3 files changed, 307 insertions(+), 78 deletions(-) create mode 100644 t/plugin/key-auth-upstream-domain-node.t diff --git a/apisix/init.lua b/apisix/init.lua index 9f8b38ba..780757f2 100644 --- a/apisix/init.lua +++ b/apisix/init.lua @@ -240,7 +240,7 @@ local function compare_upstream_node(old_t, new_t) end -local function parse_domain_in_up(up, api_ctx) +local function parse_domain_in_up(up) local nodes = up.value.nodes local new_nodes, err = parse_domain_for_nodes(nodes) if not new_nodes then @@ -253,20 +253,17 @@ local function parse_domain_in_up(up, api_ctx) return up end - if not up.modifiedIndex_org then - up.modifiedIndex_org = up.modifiedIndex - end - up.modifiedIndex = up.modifiedIndex_org .. "#" .. ngx_now() - - up.dns_value = core.table.clone(up.value) - up.dns_value.nodes = new_nodes + local up_new = core.table.clone(up) + up_new.modifiedIndex = up.modifiedIndex .. "#" .. ngx_now() + up_new.dns_value = core.table.clone(up.value) + up_new.dns_value.nodes = new_nodes core.log.info("resolve upstream which contain domain: ", - core.json.delay_encode(up)) - return up + core.json.delay_encode(up_new)) + return up_new end -local function parse_domain_in_route(route, api_ctx) +local function parse_domain_in_route(route) local nodes = route.value.upstream.nodes local new_nodes, err = parse_domain_for_nodes(nodes) if not new_nodes then @@ -279,22 +276,14 @@ local function parse_domain_in_route(route, api_ctx) return route end - if not route.modifiedIndex_org then - route.modifiedIndex_org = route.modifiedIndex - end - route.modifiedIndex = route.modifiedIndex_org .. "#" .. ngx_now() - api_ctx.conf_version = route.modifiedIndex + local route_new = core.table.clone(route) + route_new.modifiedIndex = route.modifiedIndex .. "#" .. ngx_now() - route.dns_value = core.table.deepcopy(route.value) - route.dns_value.upstream.nodes = new_nodes + route_new.dns_value = core.table.deepcopy(route.value) + route_new.dns_value.upstream.nodes = new_nodes core.log.info("parse route which contain domain: ", core.json.delay_encode(route)) - return route -end - - -local function return_direct(...) - return ... + return route_new end @@ -428,25 +417,15 @@ function _M.http_access_phase() -- the `api_ctx.conf_version` is different after we called -- `parse_domain_in_up`, need to recreate the cache by new -- `api_ctx.conf_version` - local parsed_upstream, err = lru_resolved_domain(upstream, - upstream.modifiedIndex, return_direct, nil) + local err + upstream, err = lru_resolved_domain(upstream, + upstream.modifiedIndex, + parse_domain_in_up, + upstream) if err then core.log.error("failed to get resolved upstream: ", err) return core.response.exit(500) end - - if not parsed_upstream then - parsed_upstream, err = parse_domain_in_up(upstream) - if err then - core.log.error("failed to reolve domain in upstream: ", - err) - return core.response.exit(500) - end - - lru_resolved_domain(upstream, upstream.modifiedIndex, - return_direct, parsed_upstream) - end - end if upstream.value.enable_websocket then @@ -457,37 +436,37 @@ function _M.http_access_phase() api_ctx.pass_host = upstream.value.pass_host api_ctx.upstream_host = upstream.value.upstream_host end + + core.log.info("parsed upstream: ", core.json.delay_encode(upstream)) + api_ctx.matched_upstream = upstream.dns_value or upstream.value end else if route.has_domain then - local parsed_route, err = lru_resolved_domain(route, api_ctx.conf_version, - return_direct, nil) + local err + route, err = lru_resolved_domain(route, api_ctx.conf_version, + parse_domain_in_route, route) if err then core.log.error("failed to get resolved route: ", err) return core.response.exit(500) end - if not parsed_route then - route, err = parse_domain_in_route(route, api_ctx) - if err then - core.log.error("failed to reolve domain in route: ", err) - return core.response.exit(500) - end - - lru_resolved_domain(route, api_ctx.conf_version, - return_direct, route) - end + api_ctx.matched_route = route end - if route.value.upstream and route.value.upstream.enable_websocket then + local route_val = route.value + if route_val.upstream and route_val.upstream.enable_websocket then enable_websocket = true end - if route.value.upstream and route.value.upstream.pass_host then - api_ctx.pass_host = route.value.upstream.pass_host - api_ctx.upstream_host = route.value.upstream.upstream_host + if route_val.upstream and route_val.upstream.pass_host then + api_ctx.pass_host = route_val.upstream.pass_host + api_ctx.upstream_host = route_val.upstream.upstream_host end + + api_ctx.matched_upstream = (route.dns_value and + route.dns_value.upstream) + or route_val.upstream end if enable_websocket then @@ -580,6 +559,12 @@ function _M.grpc_access_phase() api_ctx.conf_id = route.value.id end + -- todo: support upstream id + + api_ctx.matched_upstream = (route.dns_value and + route.dns_value.upstream) + or route.value.upstream + local plugins = core.tablepool.fetch("plugins", 32, 0) api_ctx.plugins = plugin.filter(route, plugins) @@ -816,13 +801,18 @@ function _M.stream_preread_phase() api_ctx.plugins = plugin.stream_filter(matched_route, plugins) -- core.log.info("valid plugins: ", core.json.delay_encode(plugins, true)) + api_ctx.matched_upstream = matched_route.value.upstream api_ctx.conf_type = "stream/route" api_ctx.conf_version = matched_route.modifiedIndex api_ctx.conf_id = matched_route.value.id run_plugin("preread", plugins, api_ctx) - set_upstream(matched_route, api_ctx) + local ok, err = set_upstream(matched_route, api_ctx) + if not ok then + core.log.error("failed to set upstream: ", err) + return ngx_exit(1) + end end diff --git a/apisix/upstream.lua b/apisix/upstream.lua index dabb303e..0ce1f2b1 100644 --- a/apisix/upstream.lua +++ b/apisix/upstream.lua @@ -59,32 +59,13 @@ function _M.set_by_route(route, api_ctx) return true end - local up_id = route.value.upstream_id - if up_id then - if not upstreams then - return false, "need to create a etcd instance for fetching " - .. "upstream information" - end - - local up_obj = upstreams:get(tostring(up_id)) - if not up_obj then - return false, "failed to find upstream by id: " .. up_id - end - core.log.info("upstream: ", core.json.delay_encode(up_obj)) - - local up_conf = up_obj.dns_value or up_obj.value - set_directly(api_ctx, up_conf.type .. "#upstream_" .. up_id, - up_obj.modifiedIndex, up_conf, up_obj) - return true - end - - local up_conf = (route.dns_value and route.dns_value.upstream) - or route.value.upstream + local up_conf = api_ctx.matched_upstream if not up_conf then return false, "missing upstream configuration in Route or Service" end + -- core.log.info("up_conf: ", core.json.delay_encode(up_conf, true)) - set_directly(api_ctx, up_conf.type .. "#route_" .. route.value.id, + set_directly(api_ctx, up_conf.type .. "#upstream_" .. tostring(up_conf), api_ctx.conf_version, up_conf, route) return true end diff --git a/t/plugin/key-auth-upstream-domain-node.t b/t/plugin/key-auth-upstream-domain-node.t new file mode 100644 index 00000000..f2f15897 --- /dev/null +++ b/t/plugin/key-auth-upstream-domain-node.t @@ -0,0 +1,258 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# +use t::APISIX 'no_plan'; + +repeat_each(1); +no_long_string(); +no_root_location(); +no_shuffle(); + +run_tests; + +__DATA__ + +=== TEST 1: create consumer +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/consumers', + ngx.HTTP_PUT, + [[{ + "username": "jack", + "plugins": { + "key-auth": { + "key": "auth-one" + } + } + }]] + ) + + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 2: set service and enabled plugin `key-auth` +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/services/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "key-auth": {} + }, + "desc": "new service" + }]] + ) + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 3: create route with plugin `limit-req`(upstream node contains domain) +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "limit-req": { + "rate": 1, + "burst": 0, + "rejected_code": 503, + "key": "remote_addr" + } + }, + "upstream": { + "nodes": { + "www.apple.com:80": 1 + }, + "pass_host": "node", + "type": "roundrobin" + }, + "service_id": 1, + "uri": "/index.html" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 4: hit route 3 times +--- config +location /t { + content_by_lua_block { + local core = require("apisix.core") + local t = require("lib.test_admin") + local headers = { + ["User-Agent"] = "curl/7.68.0", + ["apikey"] = "auth-one", + } + + for i = 1, 3 do + local code, body = t.test('/index.html', + ngx.HTTP_GET, + "", + nil, + headers + ) + ngx.say("return: ", code) + end + } +} +--- request +GET /t +--- response_body +return: 301 +return: 503 +return: 503 +--- no_error_log +[error] +--- timeout: 5 + + + +=== TEST 5: set upstream +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/upstreams/1', + ngx.HTTP_PUT, + [[{ + "nodes": { + "www.apple.com:80": 1 + }, + "pass_host": "node", + "type": "roundrobin" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 6: create route with plugin `limit-req`, and bind upstream via id +--- config + location /t { + content_by_lua_block { + local t = require("lib.test_admin").test + local code, body = t('/apisix/admin/routes/1', + ngx.HTTP_PUT, + [[{ + "plugins": { + "limit-req": { + "rate": 1, + "burst": 0, + "rejected_code": 503, + "key": "remote_addr" + } + }, + "upstream_id": 1, + "service_id": 1, + "uri": "/index.html" + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- request +GET /t +--- response_body +passed +--- no_error_log +[error] + + + +=== TEST 7: hit route 3 times +--- config +location /t { + content_by_lua_block { + local core = require("apisix.core") + local t = require("lib.test_admin") + local headers = { + ["User-Agent"] = "curl/7.68.0", + ["apikey"] = "auth-one", + } + + for i = 1, 3 do + local code, body = t.test('/index.html', + ngx.HTTP_GET, + "", + nil, + headers + ) + ngx.say("return: ", code) + end + } +} +--- request +GET /t +--- response_body +return: 301 +return: 503 +return: 503 +--- no_error_log +[error] +--- timeout: 5