Copilot commented on code in PR #117: URL: https://github.com/apache/dubbo-go-pixiu-samples/pull/117#discussion_r2641684680
########## plugins/opa/server-mode/test/pixiu_test.go: ########## @@ -0,0 +1,88 @@ +/* + * 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. + */ + +package test + +import ( + "io" + "net" + "net/http" + "strings" + "testing" + "time" +) + +import ( + "github.com/stretchr/testify/assert" +) + +const serverModeGatewayBaseURL = "http://localhost:8888"; + +func doServerModeRequest(t *testing.T, path string, headerVal *string) (int, string) { + t.Helper() + + client := &http.Client{Timeout: 5 * time.Second} + req, err := http.NewRequest("GET", serverModeGatewayBaseURL+path, nil) + if err != nil { + t.Fatalf("failed to build request: %v", err) + } + + if headerVal != nil { + req.Header.Set("Test_header", *headerVal) + } + + resp, err := client.Do(req) + if err != nil { + if ne, ok := err.(net.Error); ok && ne.Timeout() { + t.Fatalf("request timeout: ensure server-mode Pixiu, OPA server, and backend are running: %v", err) + } + t.Fatalf("request failed: ensure server-mode Pixiu, OPA server, and backend are running: %v", err) + } + if resp == nil { + t.Fatalf("response is nil") + } + defer resp.Body.Close() + + body, _ := io.ReadAll(resp.Body) Review Comment: Error handling is incomplete. The error from io.ReadAll is being silently ignored with a blank identifier. While this might be acceptable in a test helper, it would be better to handle the error appropriately or at least log it for debugging purposes. ```suggestion body, err := io.ReadAll(resp.Body) if err != nil { t.Fatalf("failed to read response body: %v", err) } ``` ########## plugins/opa/README.md: ########## @@ -1,128 +1,122 @@ -# This sample for OPA filter. - -English | [中文](README_CN.md) - -The OPA filter can provide out-of-the-box authorization capability to ensure service security and stability. - -> The filter is based on [Open Policy Agent](https://www.openpolicyagent.org/), see more here [Documentation](https://www.openpolicyagent.org/docs/latest/). - -### Create the API: - -- Create a simple Http API, Reference the [Create a simple Http API](../../dubbogo/http/README.md), then we got a path. - -- just test it: `curl http://localhost:8888/UserService -H "Test_header: 1"` - -### Define Policy - -- The first step, Define the policy. The OPA filter requires a Rego policy provided inline. Example: - -```yaml - policy: | - package http.authz - - default allow = false - - allow { - input.method == "GET" - input.path == "/status" - } -``` - -### Define Entrypoint - -- The second step, Define entrypoint.The entrypoint must match the package - -```yaml - entrypoint: "data.http.authz.allow" -``` - - - -- The third step, Ensure filter order. **The OPA filter must be placed before the HTTP proxy filter** in `http_filters`: - -```yaml -filters: - - name: dgp.filter.httpconnectionmanager - config: - route_config: - # ... your routes - http_filters: - - name: dgp.filter.http.opa - config: - policy: | - package http.authz - - default allow = false - - allow { - input.method == "GET" - input.path == "/status" - } - entrypoint: "data.http.authz.allow" - # HTTP proxy filter should be after OPA filter - - name: dgp.filter.http.proxy - config: -``` - - - -### Test: - -```cmd -go run /path_to/dubbo-go-pixiu/cmd/pixiu/*.go gateway start -c /path_to/dubbo-go-pixiu-samples/plugins/opa/pixiu/conf.yaml -``` - -``` -go run /path_to/dubbo-go-pixiu-samples/plugins/opa/server/app/* -``` - - - -##### Go Test - -``` -go test -v /path_to/dubbo-go-pixiu-samples/plugins/opa/test -``` - - - -``` -=== RUN TestUserServiceAllow ---- PASS: TestUserServiceAllow (0.02s) -=== RUN TestUserServiceDeny ---- PASS: TestUserServiceDeny (0.00s) -=== RUN TestOtherServiceDeny ---- PASS: TestOtherServiceDeny (0.00s) -PASS -``` - - - -###### Use Curl to Test - -- Denied request: - -```bash -curl -s http://127.0.0.1:8888/OtherService -``` - - Expected result: - -``` -null -``` - - - -- Allowed request: - -``` -curl -s http://127.0.0.1:8888/UserService -H "Test_header: 1" -``` - - Expected result: - -``` -{"message":"UserService","result":"pass"} -``` - +# This sample for OPA filter. Review Comment: The title is incomplete and grammatically incorrect. It should be "Sample for OPA filter" or "OPA filter sample" instead of "This sample for OPA filter." ```suggestion # Sample for OPA filter ``` ########## plugins/opa/README.md: ########## @@ -1,128 +1,122 @@ -# This sample for OPA filter. - -English | [中文](README_CN.md) - -The OPA filter can provide out-of-the-box authorization capability to ensure service security and stability. - -> The filter is based on [Open Policy Agent](https://www.openpolicyagent.org/), see more here [Documentation](https://www.openpolicyagent.org/docs/latest/). - -### Create the API: - -- Create a simple Http API, Reference the [Create a simple Http API](../../dubbogo/http/README.md), then we got a path. - -- just test it: `curl http://localhost:8888/UserService -H "Test_header: 1"` - -### Define Policy - -- The first step, Define the policy. The OPA filter requires a Rego policy provided inline. Example: - -```yaml - policy: | - package http.authz - - default allow = false - - allow { - input.method == "GET" - input.path == "/status" - } -``` - -### Define Entrypoint - -- The second step, Define entrypoint.The entrypoint must match the package - -```yaml - entrypoint: "data.http.authz.allow" -``` - - - -- The third step, Ensure filter order. **The OPA filter must be placed before the HTTP proxy filter** in `http_filters`: - -```yaml -filters: - - name: dgp.filter.httpconnectionmanager - config: - route_config: - # ... your routes - http_filters: - - name: dgp.filter.http.opa - config: - policy: | - package http.authz - - default allow = false - - allow { - input.method == "GET" - input.path == "/status" - } - entrypoint: "data.http.authz.allow" - # HTTP proxy filter should be after OPA filter - - name: dgp.filter.http.proxy - config: -``` - - - -### Test: - -```cmd -go run /path_to/dubbo-go-pixiu/cmd/pixiu/*.go gateway start -c /path_to/dubbo-go-pixiu-samples/plugins/opa/pixiu/conf.yaml -``` - -``` -go run /path_to/dubbo-go-pixiu-samples/plugins/opa/server/app/* -``` - - - -##### Go Test - -``` -go test -v /path_to/dubbo-go-pixiu-samples/plugins/opa/test -``` - - - -``` -=== RUN TestUserServiceAllow ---- PASS: TestUserServiceAllow (0.02s) -=== RUN TestUserServiceDeny ---- PASS: TestUserServiceDeny (0.00s) -=== RUN TestOtherServiceDeny ---- PASS: TestOtherServiceDeny (0.00s) -PASS -``` - - - -###### Use Curl to Test - -- Denied request: - -```bash -curl -s http://127.0.0.1:8888/OtherService -``` - - Expected result: - -``` -null -``` - - - -- Allowed request: - -``` -curl -s http://127.0.0.1:8888/UserService -H "Test_header: 1" -``` - - Expected result: - -``` -{"message":"UserService","result":"pass"} -``` - +# This sample for OPA filter. + +English | [中文](README_CN.md) + +The OPA filter can provide out-of-the-box authorization capability to ensure service security and stability. + +> The filter is based on [Open Policy Agent](https://www.openpolicyagent.org/), see more here [Documentation](https://www.openpolicyagent.org/docs/latest/). + +**Recommendation:** Use server mode (remote OPA) for production; embedded mode is for compatibility and simple demos. + +### Entrypoint reminder +- `entrypoint` must match your policy package/rule path (e.g., `data.pixiu.authz.allow`). +- Filter order: OPA must be before the HTTP proxy filter. + +### Embedded mode Review Comment: Inconsistent capitalization in section headers. "Embedded mode" should be "Embedded Mode" to match standard title case formatting used in markdown headers. ```suggestion ### Embedded Mode ``` ########## plugins/opa/README.md: ########## @@ -1,128 +1,122 @@ -# This sample for OPA filter. - -English | [中文](README_CN.md) - -The OPA filter can provide out-of-the-box authorization capability to ensure service security and stability. - -> The filter is based on [Open Policy Agent](https://www.openpolicyagent.org/), see more here [Documentation](https://www.openpolicyagent.org/docs/latest/). - -### Create the API: - -- Create a simple Http API, Reference the [Create a simple Http API](../../dubbogo/http/README.md), then we got a path. - -- just test it: `curl http://localhost:8888/UserService -H "Test_header: 1"` - -### Define Policy - -- The first step, Define the policy. The OPA filter requires a Rego policy provided inline. Example: - -```yaml - policy: | - package http.authz - - default allow = false - - allow { - input.method == "GET" - input.path == "/status" - } -``` - -### Define Entrypoint - -- The second step, Define entrypoint.The entrypoint must match the package - -```yaml - entrypoint: "data.http.authz.allow" -``` - - - -- The third step, Ensure filter order. **The OPA filter must be placed before the HTTP proxy filter** in `http_filters`: - -```yaml -filters: - - name: dgp.filter.httpconnectionmanager - config: - route_config: - # ... your routes - http_filters: - - name: dgp.filter.http.opa - config: - policy: | - package http.authz - - default allow = false - - allow { - input.method == "GET" - input.path == "/status" - } - entrypoint: "data.http.authz.allow" - # HTTP proxy filter should be after OPA filter - - name: dgp.filter.http.proxy - config: -``` - - - -### Test: - -```cmd -go run /path_to/dubbo-go-pixiu/cmd/pixiu/*.go gateway start -c /path_to/dubbo-go-pixiu-samples/plugins/opa/pixiu/conf.yaml -``` - -``` -go run /path_to/dubbo-go-pixiu-samples/plugins/opa/server/app/* -``` - - - -##### Go Test - -``` -go test -v /path_to/dubbo-go-pixiu-samples/plugins/opa/test -``` - - - -``` -=== RUN TestUserServiceAllow ---- PASS: TestUserServiceAllow (0.02s) -=== RUN TestUserServiceDeny ---- PASS: TestUserServiceDeny (0.00s) -=== RUN TestOtherServiceDeny ---- PASS: TestOtherServiceDeny (0.00s) -PASS -``` - - - -###### Use Curl to Test - -- Denied request: - -```bash -curl -s http://127.0.0.1:8888/OtherService -``` - - Expected result: - -``` -null -``` - - - -- Allowed request: - -``` -curl -s http://127.0.0.1:8888/UserService -H "Test_header: 1" -``` - - Expected result: - -``` -{"message":"UserService","result":"pass"} -``` - +# This sample for OPA filter. + +English | [中文](README_CN.md) + +The OPA filter can provide out-of-the-box authorization capability to ensure service security and stability. + +> The filter is based on [Open Policy Agent](https://www.openpolicyagent.org/), see more here [Documentation](https://www.openpolicyagent.org/docs/latest/). + +**Recommendation:** Use server mode (remote OPA) for production; embedded mode is for compatibility and simple demos. + +### Entrypoint reminder +- `entrypoint` must match your policy package/rule path (e.g., `data.pixiu.authz.allow`). +- Filter order: OPA must be before the HTTP proxy filter. + +### Embedded mode + +- Backend: `go run /path_to/dubbo-go-pixiu-samples/plugins/opa/embedded/server/app/server.go` +- Pixiu: `go run /path_to/dubbo-go-pixiu/cmd/pixiu/*.go gateway start -c /path_to/dubbo-go-pixiu-samples/plugins/opa/embedded/pixiu/conf.yaml` +- Pixiu key config (inline policy): + ```yaml + http_filters: + - name: dgp.filter.http.opa + config: + policy: | + package pixiu + default allow := false + allow { + input.path == "/UserService" + input.headers["Test_header"][0] == "1" + } + entrypoint: data.pixiu.allow + - name: dgp.filter.http.httpproxy + config: {} + clusters: + - name: "user" + endpoints: + - socket_address: + address: 127.0.0.1 + port: 1314 + ``` +- Go test: + ```bash + cd /path_to/dubbo-go-pixiu-samples/plugins/opa/embedded/test + go test -v . + # Expected: + # === RUN TestEmbeddedUserServiceAllow + # --- PASS: TestEmbeddedUserServiceAllow + # === RUN TestEmbeddedUserServiceDeny + # --- PASS: TestEmbeddedUserServiceDeny + # === RUN TestEmbeddedOtherServiceDeny + # --- PASS: TestEmbeddedOtherServiceDeny + # PASS + ``` +- Curl (through Pixiu): + - Deny: `curl -s http://127.0.0.1:8888/UserService` / `curl -s http://127.0.0.1:8888/OtherService` + - Allow: `curl -s http://127.0.0.1:8888/UserService -H "Test_header: 1"` + +### Server mode Review Comment: Inconsistent capitalization in section headers. "Server mode" should be "Server Mode" to match standard title case formatting used in markdown headers. ```suggestion ### Server Mode ``` ########## plugins/opa/embedded/test/pixiu_test.go: ########## @@ -0,0 +1,89 @@ +/* + * 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. + */ + +package test + +import ( + "io" + "net" + "net/http" + "strings" + "testing" + "time" +) + +import ( + "github.com/stretchr/testify/assert" +) + +const embeddedGatewayBaseURL = "http://localhost:8888"; + +func doEmbeddedRequest(t *testing.T, path string, headerVal *string) (int, string) { + t.Helper() + + client := &http.Client{Timeout: 5 * time.Second} + req, err := http.NewRequest("GET", embeddedGatewayBaseURL+path, nil) + if err != nil { + t.Fatalf("failed to build request: %v", err) + } + + if headerVal != nil { + req.Header.Set("Test_header", *headerVal) + } + + resp, err := client.Do(req) + if err != nil { + // Surface clearer guidance when gateway is not running + if ne, ok := err.(net.Error); ok && ne.Timeout() { + t.Fatalf("request timeout: ensure embedded Pixiu gateway and backend are running: %v", err) + } + t.Fatalf("request failed: ensure embedded Pixiu gateway and backend are running: %v", err) + } + if resp == nil { + t.Fatalf("response is nil") + } + defer resp.Body.Close() + + body, _ := io.ReadAll(resp.Body) Review Comment: Error handling is incomplete. The error from io.ReadAll is being silently ignored with a blank identifier. While this might be acceptable in a test helper, it would be better to handle the error appropriately or at least log it for debugging purposes. ```suggestion body, err := io.ReadAll(resp.Body) if err != nil { t.Fatalf("failed to read response body: %v", err) } ``` ########## plugins/opa/README.md: ########## @@ -1,128 +1,122 @@ -# This sample for OPA filter. - -English | [中文](README_CN.md) - -The OPA filter can provide out-of-the-box authorization capability to ensure service security and stability. - -> The filter is based on [Open Policy Agent](https://www.openpolicyagent.org/), see more here [Documentation](https://www.openpolicyagent.org/docs/latest/). - -### Create the API: - -- Create a simple Http API, Reference the [Create a simple Http API](../../dubbogo/http/README.md), then we got a path. - -- just test it: `curl http://localhost:8888/UserService -H "Test_header: 1"` - -### Define Policy - -- The first step, Define the policy. The OPA filter requires a Rego policy provided inline. Example: - -```yaml - policy: | - package http.authz - - default allow = false - - allow { - input.method == "GET" - input.path == "/status" - } -``` - -### Define Entrypoint - -- The second step, Define entrypoint.The entrypoint must match the package - -```yaml - entrypoint: "data.http.authz.allow" -``` - - - -- The third step, Ensure filter order. **The OPA filter must be placed before the HTTP proxy filter** in `http_filters`: - -```yaml -filters: - - name: dgp.filter.httpconnectionmanager - config: - route_config: - # ... your routes - http_filters: - - name: dgp.filter.http.opa - config: - policy: | - package http.authz - - default allow = false - - allow { - input.method == "GET" - input.path == "/status" - } - entrypoint: "data.http.authz.allow" - # HTTP proxy filter should be after OPA filter - - name: dgp.filter.http.proxy - config: -``` - - - -### Test: - -```cmd -go run /path_to/dubbo-go-pixiu/cmd/pixiu/*.go gateway start -c /path_to/dubbo-go-pixiu-samples/plugins/opa/pixiu/conf.yaml -``` - -``` -go run /path_to/dubbo-go-pixiu-samples/plugins/opa/server/app/* -``` - - - -##### Go Test - -``` -go test -v /path_to/dubbo-go-pixiu-samples/plugins/opa/test -``` - - - -``` -=== RUN TestUserServiceAllow ---- PASS: TestUserServiceAllow (0.02s) -=== RUN TestUserServiceDeny ---- PASS: TestUserServiceDeny (0.00s) -=== RUN TestOtherServiceDeny ---- PASS: TestOtherServiceDeny (0.00s) -PASS -``` - - - -###### Use Curl to Test - -- Denied request: - -```bash -curl -s http://127.0.0.1:8888/OtherService -``` - - Expected result: - -``` -null -``` - - - -- Allowed request: - -``` -curl -s http://127.0.0.1:8888/UserService -H "Test_header: 1" -``` - - Expected result: - -``` -{"message":"UserService","result":"pass"} -``` - +# This sample for OPA filter. + +English | [中文](README_CN.md) + +The OPA filter can provide out-of-the-box authorization capability to ensure service security and stability. + +> The filter is based on [Open Policy Agent](https://www.openpolicyagent.org/), see more here [Documentation](https://www.openpolicyagent.org/docs/latest/). + +**Recommendation:** Use server mode (remote OPA) for production; embedded mode is for compatibility and simple demos. + +### Entrypoint reminder Review Comment: Inconsistent capitalization in section headers. "Entrypoint reminder" should be "Entrypoint Reminder" to match standard title case formatting used in other markdown headers. ```suggestion ### Entrypoint Reminder ``` ########## plugins/opa/server-mode/remote/policy.rego: ########## @@ -0,0 +1,25 @@ +# 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. + +package pixiu.authz + +default allow := false + +allow if{ Review Comment: The spacing in the rule definition is inconsistent. According to Rego best practices, there should be a space after "if" and the opening brace should be on the same line. The current syntax "allow if{" should be "allow if {" with a space before the brace. ```suggestion allow if { ``` -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
