spacewander commented on a change in pull request #5874:
URL: https://github.com/apache/apisix/pull/5874#discussion_r774262884
##########
File path: ci/pod/docker-compose.yml
##########
@@ -402,11 +402,14 @@ services:
restart: unless-stopped
ports:
- 8181:8181
- command: run -s /example.rego /data.json
+ command: run -s /example.rego /example2.rego /data.json
Review comment:
Better to give a reason name instead of exampleX
##########
File path: t/plugin/opa2.t
##########
@@ -0,0 +1,217 @@
+#
+# 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();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+ $block->set_value("no_error_log", "[error]");
+ }
+
+ if (!defined $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: setup upstream
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/upstreams/u1',
+ ngx.HTTP_PUT,
+ [[{
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 2: setup 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": "test",
+ "plugins": {
+ "key-auth": {
+ "disable": false,
+ "key": "test-key"
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 3: setup service
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/s1',
+ ngx.HTTP_PUT,
+ [[{
+ "name": "s1",
+ "plugins": {
+ "key-auth": {
+ "disable": false
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 4: setup route with APISIX data
Review comment:
We can merge TEST 1~4 into one case?
##########
File path: t/plugin/opa2.t
##########
@@ -0,0 +1,217 @@
+#
+# 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();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+ $block->set_value("no_error_log", "[error]");
+ }
+
+ if (!defined $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: setup upstream
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/upstreams/u1',
+ ngx.HTTP_PUT,
+ [[{
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 2: setup 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": "test",
+ "plugins": {
+ "key-auth": {
+ "disable": false,
+ "key": "test-key"
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 3: setup service
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/s1',
+ ngx.HTTP_PUT,
+ [[{
+ "name": "s1",
+ "plugins": {
+ "key-auth": {
+ "disable": false
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 4: setup route with APISIX data
+--- 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": {
+ "opa": {
+ "host": "http://127.0.0.1:8181";,
+ "policy": "example2",
+ "with_route": true,
+ "with_upstream": true,
+ "with_consumer": true,
+ "with_service": true
+ }
+ },
+ "upstream_id": "u1",
+ "service_id": "s1",
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 5: hit route (test without apikey)
Review comment:
This one seems is not relative to OPA?
##########
File path: apisix/plugins/opa/helper.lua
##########
@@ -45,16 +52,136 @@ local function build_http_request(conf, ctx)
end
-function _M.build_opa_input(conf, ctx, subsystem)
- local request = build_http_request(conf, ctx)
+local function _build_http_route(conf, ctx)
+ local route_id = ctx.route_id
+ local routes = get_routes()
+
+ for _, route in ipairs(routes) do
+ if route.value.id == route_id then
+ return core.table.deepcopy(route).value
+ end
+ end
+
+ return nil
+end
+
+
+local function build_http_route(conf, ctx, remove_upstream)
+ local route = _build_http_route(conf, ctx)
+
+ if remove_upstream and route and route.upstream then
+ route.upstream = nil
+ end
+
+ return route
+end
+
+
+local function _build_http_upstream(conf, ctx)
Review comment:
As the upstream can be changed by traffic-split, we can omit the
upstream feature now?
##########
File path: apisix/plugins/opa/helper.lua
##########
@@ -45,16 +52,136 @@ local function build_http_request(conf, ctx)
end
-function _M.build_opa_input(conf, ctx, subsystem)
- local request = build_http_request(conf, ctx)
+local function _build_http_route(conf, ctx)
+ local route_id = ctx.route_id
+ local routes = get_routes()
+
+ for _, route in ipairs(routes) do
+ if route.value.id == route_id then
+ return core.table.deepcopy(route).value
+ end
+ end
+
+ return nil
+end
+
+
+local function build_http_route(conf, ctx, remove_upstream)
+ local route = _build_http_route(conf, ctx)
+
+ if remove_upstream and route and route.upstream then
+ route.upstream = nil
+ end
+
+ return route
+end
+
+
+local function _build_http_upstream(conf, ctx)
+ local route = build_http_route(conf, ctx, false)
+
+ if route then
+ if route.upstream then
+ return core.table.deepcopy(route.upstream)
+ else
+ local upstream_id = route.upstream_id
+ local upstreams = get_upstreams()
+
+ for _, upstream in ipairs(upstreams) do
+ if upstream.value.id == upstream_id then
+ return core.table.deepcopy(upstream).value
+ end
+ end
+ end
+ end
+
+ return nil
+end
+
+
+local function build_http_upstream(conf, ctx)
+ local upstream = _build_http_upstream(conf, ctx)
+
+ if upstream and upstream.parent then
+ upstream.parent = nil
+ end
+
+ return upstream
+end
+
+
+local function _build_http_service(conf, ctx)
+ local service_id = ctx.service_id
+
+ -- possible that the route is not bind a service
+ if service_id then
+ local services = get_services()
+ for _, service in ipairs(services) do
+ if service.value.id == service_id then
+ return core.table.deepcopy(service).value
+ end
+ end
+ end
+
+ return nil
+end
+
+
+local function build_http_service(conf, ctx)
+ local service = _build_http_service(conf, ctx)
+
+ if service and service.upstream and service.upstream.parent then
+ service.upstream.parent = nil
+ end
+
+ return service
+end
+
+
+local function build_http_consumer(conf, ctx)
+ local consumer_name = ctx.consumer_name
Review comment:
We can use ctx.consumer
##########
File path: t/plugin/opa2.t
##########
@@ -0,0 +1,217 @@
+#
+# 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();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if ((!defined $block->error_log) && (!defined $block->no_error_log)) {
+ $block->set_value("no_error_log", "[error]");
+ }
+
+ if (!defined $block->request) {
+ $block->set_value("request", "GET /t");
+ }
+});
+
+run_tests();
+
+__DATA__
+
+=== TEST 1: setup upstream
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/upstreams/u1',
+ ngx.HTTP_PUT,
+ [[{
+ "nodes": {
+ "127.0.0.1:1980": 1
+ },
+ "type": "roundrobin"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 2: setup 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": "test",
+ "plugins": {
+ "key-auth": {
+ "disable": false,
+ "key": "test-key"
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 3: setup service
+--- config
+ location /t {
+ content_by_lua_block {
+ local t = require("lib.test_admin").test
+ local code, body = t('/apisix/admin/services/s1',
+ ngx.HTTP_PUT,
+ [[{
+ "name": "s1",
+ "plugins": {
+ "key-auth": {
+ "disable": false
+ }
+ }
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 4: setup route with APISIX data
+--- 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": {
+ "opa": {
+ "host": "http://127.0.0.1:8181";,
+ "policy": "example2",
+ "with_route": true,
+ "with_upstream": true,
+ "with_consumer": true,
+ "with_service": true
+ }
+ },
+ "upstream_id": "u1",
+ "service_id": "s1",
+ "uri": "/hello"
+ }]]
+ )
+
+ if code >= 300 then
+ ngx.status = code
+ end
+ ngx.say(body)
+ }
+ }
+--- response_body
+passed
+
+
+
+=== TEST 5: hit route (test without apikey)
+--- request
+GET /hello
+--- more_headers
+test-header: only-for-test
+--- error_code: 401
+--- response_body eval
+qr/Missing API key found in request/
+
+
+
+=== TEST 6: hit route (test route data)
+--- request
+GET /hello
+--- more_headers
+test-header: only-for-test
+apikey: test-key
+--- error_code: 403
+--- response_body eval
+qr/\"route\":/ and qr/\"id\":\"r1\"/ and qr/\"plugins\":\{\"opa\"/ and
+qr/\"with_route\":true/
+
+
+
+=== TEST 7: hit route (test upstream data)
+--- request
+GET /hello
+--- more_headers
+test-header: only-for-test
+apikey: test-key
+--- error_code: 403
+--- response_body eval
+qr/\"upstream\":/ and qr/\"id\":\"u1\"/ and qr/\"nodes\":\[\{/ and
+qr/\"host\":\"127.0.0.1\"/ and qr/\"port\":1980/ and
+qr/\"weight\":1/ and qr/\"with_upstream\":true/ and
+qr/\"type\":\"roundrobin\"/
+
+
+
+=== TEST 8: hit route (test consumer data)
Review comment:
Let's add test for no consumer/service cases
##########
File path: apisix/plugins/opa/helper.lua
##########
@@ -45,16 +52,136 @@ local function build_http_request(conf, ctx)
end
-function _M.build_opa_input(conf, ctx, subsystem)
- local request = build_http_request(conf, ctx)
+local function _build_http_route(conf, ctx)
+ local route_id = ctx.route_id
+ local routes = get_routes()
+
+ for _, route in ipairs(routes) do
+ if route.value.id == route_id then
+ return core.table.deepcopy(route).value
+ end
+ end
+
+ return nil
+end
+
+
+local function build_http_route(conf, ctx, remove_upstream)
+ local route = _build_http_route(conf, ctx)
+
+ if remove_upstream and route and route.upstream then
+ route.upstream = nil
+ end
+
+ return route
+end
+
+
+local function _build_http_upstream(conf, ctx)
+ local route = build_http_route(conf, ctx, false)
+
+ if route then
+ if route.upstream then
+ return core.table.deepcopy(route.upstream)
+ else
+ local upstream_id = route.upstream_id
+ local upstreams = get_upstreams()
+
+ for _, upstream in ipairs(upstreams) do
+ if upstream.value.id == upstream_id then
+ return core.table.deepcopy(upstream).value
+ end
+ end
+ end
+ end
+
+ return nil
+end
+
+
+local function build_http_upstream(conf, ctx)
+ local upstream = _build_http_upstream(conf, ctx)
+
+ if upstream and upstream.parent then
+ upstream.parent = nil
+ end
+
+ return upstream
+end
+
+
+local function _build_http_service(conf, ctx)
+ local service_id = ctx.service_id
+
+ -- possible that the route is not bind a service
+ if service_id then
+ local services = get_services()
Review comment:
We can use:
https://github.com/apache/apisix/blob/3c0b374808903388f5facc081dbdb2db672c7c69/apisix/init.lua#L415
##########
File path: apisix/plugins/opa/helper.lua
##########
@@ -45,16 +52,136 @@ local function build_http_request(conf, ctx)
end
-function _M.build_opa_input(conf, ctx, subsystem)
- local request = build_http_request(conf, ctx)
+local function _build_http_route(conf, ctx)
+ local route_id = ctx.route_id
+ local routes = get_routes()
Review comment:
We can use
https://github.com/apache/apisix/blob/3c0b374808903388f5facc081dbdb2db672c7c69/apisix/init.lua#L388
##########
File path: apisix/plugins/opa/helper.lua
##########
@@ -45,16 +52,136 @@ local function build_http_request(conf, ctx)
end
-function _M.build_opa_input(conf, ctx, subsystem)
- local request = build_http_request(conf, ctx)
+local function _build_http_route(conf, ctx)
+ local route_id = ctx.route_id
+ local routes = get_routes()
+
+ for _, route in ipairs(routes) do
+ if route.value.id == route_id then
+ return core.table.deepcopy(route).value
Review comment:
Use table.clone is enough?
--
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]
