AlexStocks commented on code in PR #1061: URL: https://github.com/apache/dubbo-go-pixiu/pull/1061#discussion_r4172867357
########## admin/core/server_test.go: ########## @@ -0,0 +1,33 @@ +/* + * 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 core + +import ( + "testing" + + "github.com/stretchr/testify/require" Review Comment: [P1] 新增测试文件的 import 分组不符合仓库格式化约定，导致 CI 门禁失败并跳过全部测试 `admin/core/server_test.go` 把标准库和第三方库写在同一个 `import (...)` 块里、仅用空行分隔，而本仓库的 `imports-formatter` 要求每组各占一个 `import (...)` 块（同目录 `admin/controller/auth/auth_test.go` 已是该写法）。CI 的 `License Check and Import Format` job 在 `Check Import Formatting` 步骤执行 `go tool imports-formatter` + `git diff --exit-code` 后以 exit code 1 失败： - 失败 job: https://github.com/apache/dubbo-go-pixiu/actions/runs/37096623888/job/111185777030 - 该 job 失败后，`Golang Lint`、`Unit Test`、`Integration Test` 因 `needs: license-check-and-import-format` 全部为 SKIPPED，本 PR 目前在 CI 上没有任何测试证据。 - 本地独立复现（隔离副本，未改动被审代码）：`go tool imports-formatter` (1.0.10) 执行后本文件 sha256 由 `106b519e6c94b2094299b7926ff1ba13ebbeb31d04e88cfdf18967c85e8fc569` 变为 `0ba706105aa30c81a843b3893f1e3636de7cef31a1a59481cab773821d338dab`；全仓 1000+ 个 .go 文件哈希对比后，受影响文件仅此一个。 建议改成： ```go import ( "testing" ) import ( "github.com/stretchr/testify/require" ) ``` 修好后请重跑 CI，确认 `Unit Test` 真正执行而不是继续被跳过。 ########## README.md: ########## @@ -114,12 +114,14 @@ docker run --name pixiu-gateway -p 8888:8888 -d \ ## Pixiu Admin – Visual Control Plane Manage traffic, routing via `pixiu-admin`. -Start instantly with Docker Compose: +For a single-instance development deployment, start Docker Compose without a key: ```bash -docker-compose up -d +docker compose up -d ``` +When `DUBBOGO_PIXIU_JWT_SIGN_KEY` is empty, the admin generates a random key in memory for that process. It is never written to a file: restarting the backend invalidates its admin JWTs, and separate replicas cannot verify each other's tokens. For production or multiple replicas, configure the same unique random key of at least 32 bytes in every instance, for example through a secret manager. For Compose, you can put it in a private `.env` file (generate one with `openssl rand -hex 32`); do not commit that file. The former default `dubbo-go-pixiu` and short configured keys are rejected. Review Comment: [P2] 密钥行为说明只落在根 README，Pixiu-Admin 部署指南未同步，同一条启动命令也出现两套写法 本 PR 把根 README 的启动命令改为 `docker compose up -d` 并在此处新增了 `DUBBOGO_PIXIU_JWT_SIGN_KEY` 的行为说明，但面向 Pixiu-Admin 操作者的 `admin/README.md`（"Pixiu-Admin Deployment and Configuration Guide"）与 `admin/README_CN.md` 未同步： - 两者仍写 `docker-compose up -d`（`admin/README.md:16`、`admin/README_CN.md:17`），与根 README 形成同一条命令的两套写法。 - 两份文件全文没有任何 `DUBBOGO_PIXIU_JWT_SIGN_KEY` 说明。按这份部署指南操作的人不会知道：密钥缺失时用的是进程内随机密钥（重启即失效）、显式配置短密钥或旧默认值会让 admin 直接启动失败。 影响是具体的：升级后遇到 admin 启动失败或重启后 token 失效时，操作者按组件自身的部署指南排查会找不到原因。建议在这两份文件（含其中的 compose 命令）同步本处说明。 ########## admin/core/server_test.go: ########## @@ -0,0 +1,33 @@ +/* + * 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 core + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestRunServerRejectsUnsafeConfiguredJWTKeyBeforeInitialization(t *testing.T) { + for _, key := range []string{"dubbo-go-pixiu", "short-key"} { + t.Run(key, func(t *testing.T) { + t.Setenv("DUBBOGO_PIXIU_JWT_SIGN_KEY", key) + require.Error(t, RunServer()) Review Comment: [P2] 断言过弱，既未验证测试名声称的 "BeforeInitialization"，也未锁定错误来源 `require.Error(t, RunServer())` 只断言"返回了某个错误"。测试名声称密钥校验发生在初始化之前，但没有任何断言能证明这一点：如果后续有人把 `global.VP = Viper()` / `config.InitEtcdClient()` 挪到密钥校验之前，或 `RunServer()` 因其他原因返回错误，这个测试仍会通过，而本 PR 想保证的"先拒绝非法密钥、再初始化"已经失效。 建议锁定错误来源，例如： ```go err := RunServer() require.ErrorContains(t, err, "DUBBOGO_PIXIU_JWT_SIGN_KEY") ``` `admin/controller/auth` 侧的 `TestConfiguredUnsafeSigningKeyIsRejected` 已覆盖 `NewJWT()` 对旧默认值和短密钥的拒绝，因此这里不必重复断言错误类型。 -- 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]
