Copilot commented on code in PR #11629: URL: https://github.com/apache/apisix/pull/11629#discussion_r2844262098
########## t/plugin/opa3.t: ########## @@ -0,0 +1,106 @@ +# +# 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->request) { + $block->set_value("request", "GET /t"); + } +}); + +run_tests(); + +__DATA__ + +=== TEST 1: setup route with plugin +--- 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, + [[{ + "methods": ["POST"], + "plugins": { + "opa": { + "host": "http://127.0.0.1:8181";, + "policy": "with_body", + "with_body": true + } + }, + "upstream": { + "nodes": { + "127.0.0.1:1980": 1 + }, + "type": "roundrobin" + }, + "uris": ["/hello", "/test"] + }]] + ) + + if code >= 300 then + ngx.status = code + end + ngx.say(body) + } + } +--- response_body +passed + + + +=== TEST 2: hit route (with empty request) +--- request +POST /hello +--- error_code: 403 + + + +=== TEST 3: hit route (with json request) +--- request +POST /hello +{ + "hello": "world" +} +--- response_body +hello world +--- error_log_matches +"\"request\":{\"body\":{\"hello\":\"world\"}" + + + Review Comment: `error_log_matches` makes the test depend on log output that isn't directly tied to the assertion (the allow/deny behavior already proves the body is being used by OPA). This is likely to be flaky across log-level/config changes. Prefer asserting via response behavior only (e.g., rely on 200 vs 403), or use an `echo`-style policy to return the evaluated input and assert `request.body` from the response body instead of logs. ```suggestion ``` ########## docs/en/latest/plugins/opa.md: ########## @@ -87,6 +89,7 @@ Each of these keys are explained below: - `type` indicates the request type (`http` or `stream`). - `request` is used when the `type` is `http` and contains the basic request information (URL, headers etc). - `var` contains the basic information about the requested connection (IP, port, request timestamp etc). +- `body` contains the http-body of the request Review Comment: Documentation mismatch: the example shows `"body": {}` under `request`, but the text below describes `body` as if it were a top-level key. Also, the implementation can omit `request.body` when empty, and can send a string for non-JSON bodies. Please clarify that this is `request.body` and document the possible types/absence so policy authors know what to expect. ```suggestion - `request.body` contains the HTTP request body. This field may be omitted when the body is empty. When present, it is: - a JSON value (object, array, or primitive) if the request body is parsed as JSON, or - a string for non‑JSON bodies or bodies that cannot be parsed as JSON. ``` ########## apisix/plugins/opa/helper.lua: ########## @@ -45,8 +61,18 @@ local function build_http_request(conf, ctx) headers = core.request.headers(ctx), query = core.request.get_uri_args(ctx), } -end + if conf.with_body then + local body, err = get_body_for_request() + if err then + core.log.error(err) + else + http.body = body + end Review Comment: When `with_body` is enabled and reading the request body fails (e.g., HTTP/2 or HTTP/3 request without `Content-Length`, or temp-file read errors), the code currently just logs and continues without `request.body`. This can cause OPA policies that rely on the body to evaluate against incomplete input (potentially allowing unintended requests). Consider failing closed when `with_body` is true by propagating the error up so the plugin can reject the request (e.g., 400/403/503) instead of silently omitting the body. ```suggestion -- fail closed when with_body is enabled and the request body -- cannot be read, so that OPA does not evaluate against -- incomplete input core.log.error(err) return nil, err end http.body = body ``` ########## ci/pod/docker-compose.plugin.yml: ########## @@ -212,11 +212,14 @@ services: restart: unless-stopped ports: - 8181:8181 - command: run -s /example.rego /echo.rego /data.json /with_route.rego + command: run -s --log-level debug /example.rego /echo.rego /data.json /with_route.rego /with_body.rego Review Comment: `--log-level debug` was added to the OPA container command. This can significantly increase CI log volume/noise and may slow down runs. Unless debug logging is required for tests, consider reverting to the previous default log level (or making it conditional) and keeping the compose file minimal. ```suggestion command: run -s /example.rego /echo.rego /data.json /with_route.rego /with_body.rego ``` ########## docs/zh/latest/plugins/opa.md: ########## @@ -2,8 +2,6 @@ title: opa keywords: - Apache APISIX - - API 网关 - - Plugin - Open Policy Agent - opa Review Comment: The `keywords` front-matter removed the common `API 网关` and `Plugin` entries. Most zh plugin docs keep these keywords (e.g., `docs/zh/latest/plugins/key-auth.md:3-7`). If this removal isn't intentional, please restore them to stay consistent and preserve search/SEO metadata. ```suggestion - opa - API 网关 - Plugin ``` ########## docs/zh/latest/plugins/opa.md: ########## @@ -82,16 +81,17 @@ description: 本篇文档介绍了 Apache APISIX 通过 opa 插件与 Open Polic } ``` -上述代码具体释义如下： +以下是各个键的说明： -- `type` 代表请求类型（如 `http` 或 `stream`）； -- `request` 则需要在 `type` 为 `http` 时使用，包含基本的请求信息（如 URL、头信息等）； -- `var` 包含关于请求连接的基本信息（如 IP、端口、请求时间戳等）； -- `route`、`service` 和 `consumer` 包含的数据与 APISIX 中存储的数据相同，只有当这些对象上配置了 `opa` 插件时才会发送。 +- `type` 表示请求类型（`http` 或 `stream`）. +- `request` 在 `type` 为 `http` 时使用，包含基本请求信息（URL、头信息等）. +- `var` 包含请求连接的基本信息（IP、端口、请求时间戳等）。 +- `body` 包含请求的 HTTP 主体。 Review Comment: 文档表述有歧义：示例里 `body` 位于 `request` 对象下，但后面的说明将 `body` 当作顶层字段。并且实现里 `request.body` 可能在空请求体时缺失，且非 JSON 请求体会以字符串形式发送。建议明确写为 `request.body`，并补充其可能的类型/缺失情况，方便编写策略。 ```suggestion - `request` 在 `type` 为 `http` 时使用，包含基本请求信息（URL、头信息等）。 - `var` 包含请求连接的基本信息（IP、端口、请求时间戳等）。 - `request.body` 包含请求的 HTTP 主体：当请求体为空时该字段可能不存在；当请求体为 JSON 时，该字段为解析后的 JSON 值；当请求体为非 JSON 时，该字段为原始字符串。 ``` ########## apisix/plugins/opa/helper.lua: ########## @@ -34,9 +34,25 @@ local function build_var(conf, ctx) } end +local function get_body_for_request() + local original_body, err = core.request.get_body() + if err then + return nil, "failed to get request body: " .. err + end + if original_body == nil then + return nil + end + -- decode to prevent double encoded json objects + local body, err = core.json.decode(original_body) + if err then + -- if its not json, the body can just be added Review Comment: For non-JSON bodies, `body` is forwarded as a raw Lua string. If the payload contains binary or invalid UTF-8, `core.json.encode` (cjson.safe) can fail to encode the OPA input, which will break the OPA decision call. Consider encoding non-JSON bodies into a JSON-safe representation (e.g., base64 + a marker/type field) or explicitly dropping/denying on non-UTF8 bodies when `with_body` is enabled. ```suggestion local body, decode_err = core.json.decode(original_body) if decode_err then -- if it's not json, ensure the body is JSON-safe (e.g., valid UTF-8) local _, encode_err = core.json.encode(original_body) if encode_err then return nil, "request body is not JSON-encodable: " .. encode_err end ``` -- 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]
