Copilot commented on code in PR #877:
URL: https://github.com/apache/dubbo-go-pixiu/pull/877#discussion_r2773333303
##########
docker-compose.yml:
##########
@@ -34,7 +34,19 @@ services:
- "2380:2380"
networks:
- app_network
-
+ # OPA service
+ opa:
+ image: openpolicyagent/opa:latest
Review Comment:
`openpolicyagent/opa:latest` is not a reproducible build target and can
change behavior unexpectedly over time. Pin the OPA image to a specific version
tag (and/or digest) to make local/dev environments deterministic.
```suggestion
image: openpolicyagent/opa:0.64.1
```
##########
admin/web/src/api/menu-config.js:
##########
@@ -1,4 +1,4 @@
-/*
+/*
Review Comment:
This file now begins with a UTF-8 BOM (`\ufeff`) before the `/*` comment.
BOMs in JS sources can cause unexpected issues with tooling/linting and create
noisy diffs—please remove it so the file starts with a plain `/*`.
```suggestion
/*
```
##########
admin/logic/logic.go:
##########
@@ -51,13 +57,19 @@ const (
Plugin = "plugin"
Filter = "filter"
Ratelimit = "ratelimit"
+ OPA = "opa"
Clusters = "clusters"
Listeners = "listeners"
Unpublished = "unpublished"
ErrID = -1
)
+// Use a shared client to enable HTTP keep-alive and prevent connection
exhaustion
+var opaHTTPClient = &http.Client{
+ Timeout: adminconfig.DefaultOPAPolicyTimeout,
+}
+
Review Comment:
The shared `opaHTTPClient` timeout is hard-coded to
`DefaultOPAPolicyTimeout`, but the bootstrap config adds `opa.request_timeout`
(and `AdminBootstrap.OPA.RequestTimeout`) which is currently ignored. Consider
initializing the client timeout from `adminconfig.Bootstrap.OPA.RequestTimeout`
when set, falling back to the default only when unset.
```suggestion
Timeout: getOPATimeout(),
}
func getOPATimeout() interface{} {
if adminconfig.Bootstrap.OPA.RequestTimeout > 0 {
return adminconfig.Bootstrap.OPA.RequestTimeout
}
return adminconfig.DefaultOPAPolicyTimeout
}
```
##########
admin/doc/swagger.json:
##########
@@ -789,7 +789,129 @@
}
}
},
- "/config/api/resource/publish": {
+ "/config/api/opa/policy": {
+ "get": {
+ "produces": [
+ "application/json"
+ ],
+ "tags": [
+ "Config"
+ ],
+ "summary": "get OPA policy (server mode)",
+ "parameters": [
+ {
+ "type": "string",
+ "description": "OPA server url",
+ "name": "server_url",
+ "in": "query"
+ },
+ {
+ "type": "string",
+ "description": "Policy ID",
+ "name": "policy_id",
+ "in": "query"
+ },
+ {
+ "type": "string",
+ "description": "Bearer token",
+ "name": "bearer_token",
+ "in": "header"
Review Comment:
Swagger declares `bearer_token` as coming from the request header for GET
(`"in": "header"`), but the implementation binds it from query params
(`form:"bearer_token"` + `ShouldBindQuery`). Either update the swagger spec to
use `in: query`, or update the handler to read the token from headers (e.g.,
`Authorization`) to match the spec.
```suggestion
"in": "query"
```
##########
admin/controller/opa/opa.go:
##########
@@ -0,0 +1,122 @@
+/*
+ * 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 opa
+
+import (
+ "net/http"
+ "strings"
+)
+
+import (
+ "github.com/gin-gonic/gin"
+
+ perrors "github.com/pkg/errors"
+)
+
+import (
+ adminconfig "github.com/apache/dubbo-go-pixiu/admin/config"
+ "github.com/apache/dubbo-go-pixiu/admin/logic"
+)
+
+// @Tags Config
+// @Summary upload OPA policy (server mode)
+// @Router /config/api/opa/policy [put]
+func PutOPAPolicy(c *gin.Context) {
+ // For PUT, we typically expect Form Data
+ serverURL := resolveOPAServerURL(c.PostForm("server_url"))
+ policyID := resolveOPAPolicyID(c.PostForm("policy_id"))
+ bearerToken := c.PostForm("bearer_token")
+ content := c.PostForm("content")
+
+ if content == "" {
+ c.JSON(http.StatusOK, adminconfig.WithError(perrors.New("rego
content is required")))
+ return
+ }
+
+ if err := logic.BizPutOPAPolicy(serverURL, policyID, bearerToken,
content); err != nil {
+ c.JSON(http.StatusOK, adminconfig.WithError(err))
+ return
+ }
+ c.JSON(http.StatusOK, adminconfig.WithRet("Update Success"))
+}
+
+// @Tags Config
+// @Summary get OPA policy (server mode)
+// @Router /config/api/opa/policy [get]
+func GetOPAPolicy(c *gin.Context) {
+ var query adminconfig.OPAQuery
+ if err := c.ShouldBindQuery(&query); err != nil {
+ c.JSON(http.StatusOK, adminconfig.WithError(err))
+ return
+ }
+
+ serverURL := resolveOPAServerURL(query.ServerURL)
+ policyID := resolveOPAPolicyID(query.PolicyID)
+
+ result, err := logic.BizGetOPAPolicy(serverURL, policyID,
query.BearerToken)
+ if err != nil {
+ c.JSON(http.StatusOK, adminconfig.WithError(err))
+ return
+ }
+ c.JSON(http.StatusOK, adminconfig.WithRet(result))
+}
+
+// @Tags Config
+// @Summary delete OPA policy (server mode)
+// @Router /config/api/opa/policy [delete]
+func DeleteOPAPolicy(c *gin.Context) {
+ var query adminconfig.OPAQuery
+ if err := c.ShouldBindQuery(&query); err != nil {
+ c.JSON(http.StatusOK, adminconfig.WithError(err))
+ return
+ }
+
+ serverURL := resolveOPAServerURL(query.ServerURL)
+ policyID := resolveOPAPolicyID(query.PolicyID)
+
+ if err := logic.BizDeleteOPAPolicy(serverURL, policyID,
query.BearerToken); err != nil {
+ c.JSON(http.StatusOK, adminconfig.WithError(err))
+ return
+ }
+ c.JSON(http.StatusOK, adminconfig.WithRet("Delete Success"))
+}
+
+func resolveOPAServerURL(serverURL string) string {
+ serverURL = strings.TrimSpace(serverURL)
+ if serverURL != "" {
+ if adminconfig.Bootstrap != nil {
+ if trimmed :=
strings.TrimSpace(adminconfig.Bootstrap.OPA.PolicyID); trimmed != "" {
+ return trimmed
+ }
+ }
+ }
Review Comment:
`resolveOPAServerURL` is returning `adminconfig.Bootstrap.OPA.PolicyID` (a
policy id) instead of an OPA server URL, and the condition is inverted: when
`server_url` is provided it should return that value (or the bootstrap
`OPA.ServerURL` when omitted). As-is, `server_url` overrides will never work
and the URL may be replaced by a policy id, breaking all proxy calls.
```suggestion
// If an explicit server URL is provided, use it directly.
return serverURL
}
// Otherwise, fall back to the bootstrap-configured OPA server URL, if
available.
if adminconfig.Bootstrap != nil {
if trimmed :=
strings.TrimSpace(adminconfig.Bootstrap.OPA.ServerURL); trimmed != "" {
return trimmed
}
}
// Finally, use the default OPA server URL.
```
##########
admin/doc/swagger.json:
##########
@@ -1,4 +1,4 @@
-{
+{
Review Comment:
The JSON file starts with a UTF-8 BOM (`\ufeff`). Some JSON parsers/tools
will reject BOM-prefixed JSON; it also makes diffs noisy. Please remove the BOM
so the file begins with `{`.
```suggestion
{
```
##########
admin/doc/swagger.yaml:
##########
@@ -1,4 +1,4 @@
-info:
+info:
Review Comment:
The file starts with a UTF-8 BOM (`\ufeff`), which can break some YAML
tooling (and makes diffs noisy). Please remove the BOM so the first key is
plain `info:`.
```suggestion
info:
```
##########
admin/web/src/views/dashboard/manage/OPA.vue:
##########
@@ -0,0 +1,319 @@
+<!--
+ * 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.
+ -->
+<template>
+ <CustomLayout>
+ <div class="custom-body">
+ <div>
+ <CommonTitle title="OPA 策略配置"></CommonTitle>
+ </div>
+ <div class="custom-tools">
+ <div class="table-head">
+ <div class="custom-tools__info">OPA 策略</div>
+ <div>
+ <el-button size="mini" @click="handleSync">同步</el-button>
+ <el-button type="primary" size="mini"
@click="handleSave">保存</el-button>
+ <el-popconfirm
+ title="确定要删除该策略吗？"
+ @confirm="handleDelete">
+ <el-button slot="reference" type="danger"
size="mini">删除</el-button>
+ </el-popconfirm>
+ </div>
+ </div>
+ <div class="custom-tools__content">
+ <el-form :model="form"
+ :inline="true"
+ @submit.native.prevent=""
+ class="table-form bg-gray"
+ label-width="130px">
+ <el-row>
+ <el-form-item label="policy_id">
+ <el-input v-model="form.policy_id"
+ clearable
+ placeholder="pixiu-authz"></el-input>
+ </el-form-item>
+
+ </el-row>
+ <el-row>
+ <div style="clear: both; height: 300px;width: 100%;"
id="policyEditor" ref="policyEditor"/>
+ </el-row>
+ </el-form>
+ </div>
+ </div>
+ </div>
+ </CustomLayout>
+</template>
+
+<script>
+import CommonTitle from '@/components/common/CommonTitle'
+import CustomLayout from '@/components/common/CustomLayout.vue'
+import * as monaco from 'monaco-editor/esm/vs/editor/editor.main.js'
+import
'monaco-editor/esm/vs/basic-languages/javascript/javascript.contribution'
+import { getLocalStorage, setLocalStorage } from '@/utils/auth'
+
+const POLICY_CACHE_KEY = 'opaPolicyLast'
+const DEFAULT_POLICY_ID = 'pixiu-authz'
+
+const DEFAULT_POLICY = `package pixiu.authz
+
+default allow := false
+
+allow if {
+ # write your logic here
+}
+`
+const CONTROL_CHAR_REGEX =
/[\u0000-\u0008\u000B\u000C\u000E-\u001F\u007F\u2028\u2029\uFEFF]/g
+let regoRegistered = false
+
+function normalizePolicyText(value) {
+ if (!value) {
+ return ''
+ }
+ return value
+ .replace(/\r\n/g, '\n')
+ .replace(/\r/g, '\n')
+ .replace(CONTROL_CHAR_REGEX, '')
+}
+
+function registerRegoLanguage(monaco) {
+ if (regoRegistered) {
+ return
+ }
+ regoRegistered = true
+ monaco.languages.register({ id: 'rego' })
+ monaco.languages.setMonarchTokensProvider('rego', {
+ keywords: [
+ 'package', 'default', 'import', 'as', 'with', 'not', 'some',
+ 'else', 'if', 'in', 'true', 'false', 'null'
+ ],
+ operators: [
+ '=', ':=', '==', '!=', '<', '>', '<=', '>=', '+', '-', '*', '/',
+ '%', 'and', 'or'
+ ],
+ tokenizer: {
+ root: [
+ [/[a-zA-Z_][\w\-]*/, {
+ cases: {
+ '@keywords': 'keyword',
+ '@default': 'identifier'
+ }
+ }],
+ [/[{}()[\]]/, '@brackets'],
+ [/[=><!]=?/, 'operator'],
+ [/\"([^\"\\]|\\.)*$/, 'string.invalid'],
+ [/\"/, { token: 'string.quote', bracket: '@open', next: '@string' }],
+ [/#[^\r\n]*/, 'comment'],
+ [/\d+(\.\d+)?/, 'number'],
+ [/[;,.]/, 'delimiter']
+ ],
+ string: [
+ [/[^\\"]+/, 'string'],
+ [/\\./, 'string.escape'],
+ [/\"/, { token: 'string.quote', bracket: '@close', next: '@pop' }]
+ ]
+ }
+ })
+}
+
+export default {
+ name: 'OPAPolicyConfig',
+ components: {
+ CommonTitle,
+ CustomLayout
+ },
+ data () {
+ return {
+ form: {
+ policy_id: DEFAULT_POLICY_ID
+ },
+ monacoEditor: null
+ }
+ },
+ mounted () {
+ this.$nextTick(() => {
+ this.initPolicyEditor()
+ this.handleSync(false)
+ window.addEventListener('resize', this.handleEditorResize)
+ })
+ },
+ methods: {
+ handleEditorResize() {
+ if (this.monacoEditor) {
+ this.monacoEditor.layout()
+ }
+ },
+ initPolicyEditor() {
+ registerRegoLanguage(monaco)
+ let cached = getLocalStorage(POLICY_CACHE_KEY)
+ let value = cached && cached !== '' ? cached : DEFAULT_POLICY
+ this.monacoEditor =
monaco.editor.create(document.getElementById('policyEditor'), {
+ value,
+ language: 'rego',
+ codeLens: true,
+ selectOnLineNumbers: true,
+ roundedSelection: false,
+ readOnly: false,
+ lineNumbers: 'on',
+ theme: 'vs-dark',
+ wordWrapColumn: 120,
+ folding: false,
+ showFoldingControls: 'always',
+ wordWrap: 'wordWrapColumn',
+ cursorStyle: 'line',
+ automaticLayout: true
+ })
+ const model = this.monacoEditor.getModel()
+ if (model) {
+ model.setEOL(monaco.editor.EndOfLineSequence.LF)
+ }
+ monaco.editor.remeasureFonts()
+ this.monacoEditor.layout()
+ },
+ handleSync(showMessage = true) {
+ this.$get('/config/api/opa/policy', {
+ policy_id: this.form.policy_id || DEFAULT_POLICY_ID
+ })
+ .then((res) => {
+ if (res) {
+ let content = ''
+ if (res && typeof res === 'object') {
Review Comment:
This use of variable 'res' always evaluates to true.
```suggestion
if (typeof res === 'object') {
```
--
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]
