diff --git a/docs/brainstorms/2026-05-18-violet-external-binary-brainstorm.md b/docs/brainstorms/2026-05-18-violet-external-binary-brainstorm.md index 7a94bed..4edf0be 100644 --- a/docs/brainstorms/2026-05-18-violet-external-binary-brainstorm.md +++ b/docs/brainstorms/2026-05-18-violet-external-binary-brainstorm.md @@ -3,7 +3,7 @@ - **日期**：2026-05-18 - **范围**：仅讨论"上架"这一步（Artifact / ArtifactVersion 创建）；OLM Subscription / InstallPlan / CSV 不在本次重构内 - **现状代码锚点**： - - `pkg/operator/operatorhub/artifact_versiong.go:15-89`（`InstallArtifactVersion` + `createArtifactVersion` 硬编码 unstructured） + - `pkg/operator/operatorhub/artifact_version.go:15-89`（`InstallArtifactVersion` + `createArtifactVersion` 硬编码 unstructured） - `pkg/exec/exec.go:43-80`（已有的子进程执行机制，testCommand 走这条） - `pkg/config/config.go:29-52`（`OperatorConfig`） @@ -51,7 +51,7 @@ ### 1. violet 接管范围：**只接管"创建 Artifact/ArtifactVersion"** -- 替换：`createArtifactVersion`（`artifact_versiong.go:51-90`） +- 替换：`createArtifactVersion`（`artifact_version.go:51-90`） - 保留：`waitArtifactVersionPresent`、`waitPackageManifest`、`InstallSubscription`、InstallPlan 手动审批 - 原因：violet 物理上不管 OLM 资源；保留 Go 控制 OLM 流程 = 升级时序仍然由 upgrade CLI 把控 @@ -118,7 +118,7 @@ operatorConfig: ## 实施轮廓（不细化，留给 plan） - 新建 `pkg/operator/operatorhub/violet.go`（拼 URL + 下载 + exec violet push） -- 替换 `artifact_versiong.go` 里 `createArtifactVersion` 的调用点 +- 替换 `artifact_version.go` 里 `createArtifactVersion` 的调用点 - `pkg/config/config.go` 增字段 + 默认值 - 文档：更新 README 和 CLAUDE.md "新加 Operator 类型时" 段落（提及 violet 路径） diff --git a/docs/plans/2026-05-18-refactor-externalize-violet-binary-plan.md b/docs/plans/2026-05-18-refactor-externalize-violet-binary-plan.md index d3028cc..400828f 100644 --- a/docs/plans/2026-05-18-refactor-externalize-violet-binary-plan.md +++ b/docs/plans/2026-05-18-refactor-externalize-violet-binary-plan.md @@ -18,7 +18,7 @@ origin：`docs/brainstorms/2026-05-18-violet-external-binary-brainstorm.md`。 ## Problem Statement / Motivation -当前 `pkg/operator/operatorhub/artifact_versiong.go:51-90` 把 `Artifact`/`ArtifactVersion` 的 CRD 字段（apiVersion / labels / annotations / spec.present / spec.tag / OwnerReferences 等）**硬编码**在 Go 里。 +当前 `pkg/operator/operatorhub/artifact_version.go:51-90` 把 `Artifact`/`ArtifactVersion` 的 CRD 字段（apiVersion / labels / annotations / spec.present / spec.tag / OwnerReferences 等）**硬编码**在 Go 里。 问题： 1. **schema 漂移风险** — Alauda CRD 演进时 upgrade CLI 必须跟着改、重发版 @@ -29,7 +29,7 @@ origin：`docs/brainstorms/2026-05-18-violet-external-binary-brainstorm.md`。 ## Proposed Solution -**新 `InstallArtifactVersion` 流程**（替换 `pkg/operator/operatorhub/artifact_versiong.go:15-49`）： +**新 `InstallArtifactVersion` 流程**（替换 `pkg/operator/operatorhub/artifact_version.go:15-49`）： ``` 1. resolve URL : 按 ///.latest.ALL..tgz 拼 @@ -319,7 +319,7 @@ violet 自身打印的 `--password=xxx`（如有 debug 模式）会进 CLI 日 - `pkg/operator/operatorhub/violet.go`： - 新增 `(o *Operator) installViaViolet(ctx, version Version) (*unstructured.Unstructured, error)` - 实现 download HTTP GET → 临时目录 → sha256 校验（若 ExpectedSha256 非空）→ ensure clean AV（Get + Delete + Poll NotFound）→ exec violet（用 PR 1 的 EnvAllowlist）→ 复用 `waitArtifactVersionPresent` / `waitPackageManifest` -- `pkg/operator/operatorhub/artifact_versiong.go`： +- `pkg/operator/operatorhub/artifact_version.go`： - `InstallArtifactVersion` 改走 `o.installViaViolet` - **保留** `createArtifactVersion` 函数为 dead code（不删；便于 PR 2 出问题时本地 diff 对比） - `README.md`：新增章节 @@ -356,7 +356,7 @@ violet 自身打印的 `--password=xxx`（如有 debug 模式）会进 CLI 日 **Branch**：`chore/remove-deprecated-create-av`（基于 PR 2 分支） **改动范围**： -- `pkg/operator/operatorhub/artifact_versiong.go`： +- `pkg/operator/operatorhub/artifact_version.go`： - 删除 `createArtifactVersion` 函数 - 删除仅它在用的 imports（`metav1` OwnerReference 等若不再需要） - 检查 `artifactGVR` 是否还有引用 —— 仍要保留（`InstallArtifactVersion` 的 Get + `installViaViolet` 的 Delete 仍用） @@ -401,10 +401,10 @@ PR 1 (基础设施) ─┬─→ PR 2 (切换 + dead code 保留) ─→ PR 3 ( ### Internal references -- 现有 unstructured 创建逻辑：`pkg/operator/operatorhub/artifact_versiong.go:51-90` +- 现有 unstructured 创建逻辑：`pkg/operator/operatorhub/artifact_version.go:51-90` - 子进程 exec：`pkg/exec/exec.go:43-80`（env 透传现状 line 48） - 配置默认值入口：`pkg/config/config.go:91-112` -- knative logging 模式：`pkg/operator/operatorhub/subscription.go:23`、`pkg/operator/operatorhub/artifact_versiong.go:16` +- knative logging 模式：`pkg/operator/operatorhub/subscription.go:23`、`pkg/operator/operatorhub/artifact_version.go:16` - factory：`pkg/operator/factory.go` - Tekton Pipeline `devops/upgrade-test`（integration-test 集群）：在线读取于 2026-05-18 diff --git a/pkg/operator/operatorhub/artifact_versiong.go b/pkg/operator/operatorhub/artifact_version.go similarity index 100% rename from pkg/operator/operatorhub/artifact_versiong.go rename to pkg/operator/operatorhub/artifact_version.go diff --git a/todos/001-pending-p2-http-download-no-timeout.md b/todos/001-pending-p2-http-download-no-timeout.md deleted file mode 100644 index 9c271b6..0000000 --- a/todos/001-pending-p2-http-download-no-timeout.md +++ /dev/null @@ -1,73 +0,0 @@ ---- -name: http-download-no-timeout -description: downloadToTemp 使用 http.DefaultClient，无 Client.Timeout，且上游 ctx 也没有 deadline，stalled MinIO 可挂数小时 -status: pending -priority: p2 -issue_id: "001" -tags: [code-review, performance, security, reliability, p2] -dependencies: [] ---- - -# Problem Statement - -`downloadToTemp` 在 `pkg/operator/operatorhub/violet.go:234` 用 `http.DefaultClient.Do(req)` 下载 .tgz 包。`http.DefaultClient.Timeout == 0`（无超时），而上游 `cmd/upgrade_command.go` 传进来的 ctx 也没有 `context.WithTimeout` 包裹（`o.timeout` 只在 `wait.PollUntilContextTimeout` 内被消费，不影响下载阶段）。 - -结果：若 MinIO 半开 TCP / 反向代理挂起 / 一边写一边断流，HTTP 请求会阻塞到 OS 级 TCP keepalive（Linux 默认 ~2 小时）才返回。`TestDownloadToTemp_ContextCancel` 只证明了 ctx-cancel 通路工作，没有强制 deadline。 - -# Findings - -- **security-sentinel** S8（P3）+ **architecture-strategist** A6（P3）+ **performance-oracle** P1（P2） 同时命中。三方独立观察。 -- 文件路径：`pkg/operator/operatorhub/violet.go:234` -- 调用源：`cmd/upgrade_command.go:198` 传入 bare ctx（无 deadline） -- 影响：单次升级阶段挂死数小时，CI runner 阻塞，可用性事故 - -# Proposed Solutions - -**Option A（推荐）**：在 `downloadToTemp` 内部用 `context.WithTimeout(ctx, o.timeout)` 包裹。复用已有 `OperatorConfig.Timeout` 旋钮。 - -```go -func (o *Operator) downloadToTemp(ctx context.Context, rawURL string) (string, func(), error) { - dlCtx, cancel := context.WithTimeout(ctx, o.timeout) - defer cancel() - req, err := http.NewRequestWithContext(dlCtx, http.MethodGet, rawURL, nil) - // ... -} -``` - -- 优点：复用已有配置项，5 行改动，语义清晰 -- 缺点：`downloadToTemp` 需要变成 `*Operator` 方法（或者取一个 timeout 参数） -- effort: Small -- risk: Low - -**Option B**：构造专用 `&http.Client{Timeout: o.timeout, Transport: ...}`，加 `DialContext` / `ResponseHeaderTimeout` / `IdleConnTimeout`。 - -- 优点：粒度更细，能区分 dial / header / read 阶段超时 -- 缺点：相比 Option A 多写 ~15 行 Transport 配置，本 CLI 单次调用收益有限 -- effort: Medium -- risk: Low - -# Recommended Action - -(待 triage 后填写) - -# Technical Details - -- 影响文件：`pkg/operator/operatorhub/violet.go` -- 单元测试：可在现有 `TestDownloadToTemp_ContextCancel` 旁加 `TestDownloadToTemp_ParentTimeoutEnforced`，验证即使上游 ctx 没 deadline，downloadToTemp 自己也会在 `o.timeout` 后超时 -- 不影响 K8s 资源 - -# Acceptance Criteria - -- [ ] downloadToTemp 在 `o.timeout` 内必返回（即使上游 ctx 没 deadline） -- [ ] 新增测试覆盖该路径 -- [ ] `go test ./pkg/operator/operatorhub/...` 全绿 -- [ ] 现有 `TestDownloadToTemp_ContextCancel` 保持通过 - -# Work Log - -- 2026-05-18: code review 发现 by 3 个 agent（security/architecture/performance） - -# Resources - -- PR: https://github.com/AlaudaDevops/upgrade-test/pull/14 -- 相关：Release It! "remote calls must assume they will fail" diff --git a/todos/004-pending-p2-package-manifest-substring-csv-match.md b/todos/004-pending-p2-package-manifest-substring-csv-match.md deleted file mode 100644 index 857d94a..0000000 --- a/todos/004-pending-p2-package-manifest-substring-csv-match.md +++ /dev/null @@ -1,78 +0,0 @@ ---- -name: package-manifest-substring-csv-match -description: waitPackageManifest 用 strings.Contains 匹配 CSV 名，v0.65.0 会被 v0.65.0-rc1 / v0.65.0.bak 错误命中，导致 Subscription 绑到错误版本 -status: pending -priority: p2 -issue_id: "004" -tags: [code-review, quality, correctness, p2] -dependencies: [] ---- - -# Problem Statement - -`waitPackageManifest` 在 `pkg/operator/operatorhub/artifact_versiong.go:120-122` 用 substring 匹配 CSV 名： - -```go -csvName, _, _ := unstructured.NestedString(entryMap, "name") -if strings.Contains(csvName, csv) { - return true, nil -} -``` - -`csv` 来自 `av.status.version`，典型值如 `tektoncd-operator.v0.65.0`。如果 PackageManifest 同时存在 `tektoncd-operator.v0.65.0-rc1`、`tektoncd-operator.v0.65.0.bak`、`tektoncd-operator.v0.65.0-canary`，`Contains` 全部命中——首个出现的就会被当成等待目标返回 OK。后续 `InstallSubscription` 拿到 `csv=v0.65.0` 也会被 OLM 正确解析，但等待信号已不可靠。 - -# Findings - -- **silent-failure-hunter** F2（HIGH）独立命中 -- 文件：`pkg/operator/operatorhub/artifact_versiong.go:106-122` -- 三处 `_, _, _` 同时丢弃了 NestedSlice / NestedString 的错误返回，schema drift 也被静默吞掉 - -# Proposed Solutions - -**Option A（推荐）**：用精确匹配（或带 `.` 边界的 prefix-match），并把 NestedString 的 err 传出来。 - -```go -csvName, found, err := unstructured.NestedString(entryMap, "name") -if err != nil { - return false, fmt.Errorf("entry .name has wrong type in PackageManifest %s: %w", o.name, err) -} -if found && csvName == csv { - return true, nil -} -``` - -- 优点：精确 + 错误显式 -- 缺点：如果 OLM 在 CSV 名上加版本后缀（极少见），需要重新评估匹配策略 -- effort: Small -- risk: Low - -**Option B**：保留 substring 但加 `.` 边界检查：`strings.HasPrefix(csvName, csv+".")` 兜底，避免 v0.65.0 命中 v0.65.0-rc1。 - -- 优点：兼容历史命名怪癖 -- 缺点：仍然不精确 -- effort: Small -- risk: Low-Medium - -# Recommended Action - -(待 triage) - -# Technical Details - -- 影响文件：`pkg/operator/operatorhub/artifact_versiong.go` -- 测试：添加单元测试覆盖 (a) 精确匹配命中 (b) `v0.65.0` 不命中 `v0.65.0-rc1` (c) NestedString 类型错误返回 - -# Acceptance Criteria - -- [ ] CSV 名匹配从 substring 改为精确（或带边界的 prefix-match） -- [ ] NestedString 错误不再被静默丢弃 -- [ ] 新增单元测试覆盖三种场景 -- [ ] 现有测试通过 - -# Work Log - -- 2026-05-18: code review 发现 by silent-failure-hunter - -# Resources - -- PR: https://github.com/AlaudaDevops/upgrade-test/pull/14 diff --git a/todos/005-pending-p2-wait-av-no-retry-on-transient-errors.md b/todos/005-pending-p2-wait-av-no-retry-on-transient-errors.md deleted file mode 100644 index b43ca8b..0000000 --- a/todos/005-pending-p2-wait-av-no-retry-on-transient-errors.md +++ /dev/null @@ -1,84 +0,0 @@ ---- -name: wait-av-no-retry-on-transient-errors -description: waitArtifactVersionPresent 把任何 Get 错误（包括 NotFound 与 5xx/429/timeout）直接终结轮询，apiserver 短暂抖动就杀掉整个升级 -status: pending -priority: p2 -issue_id: "005" -tags: [code-review, reliability, correctness, p2] -dependencies: [] ---- - -# Problem Statement - -`pkg/operator/operatorhub/artifact_versiong.go:75-78`： - -```go -obj, err := o.client.Resource(artifactVersionGVR).Namespace(systemNamespace).Get(ctx, name, metav1.GetOptions{}) -if err != nil { - return false, err -} -``` - -两个问题： -1. **NotFound**：violet push 完成到 ArtifactVersion 在 etcd 可见有个最终一致性窗口；此处 NotFound 不应该终止轮询，应当继续等。 -2. **transient errors**：apiserver leader election、webhook timeout、429（throttle）、5xx、连接 reset——都会立即把 `wait.PollUntilContextTimeout` 杀掉，触发"莫名其妙的升级失败"，需要手动重跑。 - -# Findings - -- **silent-failure-hunter** F3（HIGH）独立命中 -- 文件：`pkg/operator/operatorhub/artifact_versiong.go:72-93` -- 影响：CI 升级假阴性、可靠性下降、对工具的信心受损 - -# Proposed Solutions - -**Option A（推荐）**：分类错误，NotFound 与 transient 继续轮询。 - -```go -obj, err := o.client.Resource(...).Get(ctx, name, metav1.GetOptions{}) -switch { -case errors.IsNotFound(err): - return false, nil -case errors.IsServerTimeout(err) || errors.IsTooManyRequests(err) || errors.IsServiceUnavailable(err): - log.Warnw("transient API error while polling AV, will retry", "err", err) - return false, nil -case err != nil: - return false, fmt.Errorf("get AV %s: %w", name, err) -} -``` - -- 优点：贴近 K8s 客户端语义；保留 timeout 作为最终 backstop -- 缺点：需要补测试覆盖每个分支 -- effort: Small -- risk: Low - -**Option B**：所有 Get 错误统一吞为 `(false, nil)`，由 `o.timeout` 兜底。 - -- 优点：极简 -- 缺点：掩盖了真正的非 transient 错误（如 RBAC 拒绝），timeout 后 user 拿不到根因 -- effort: Negligible -- risk: Medium - -# Recommended Action - -(待 triage) - -# Technical Details - -- 影响文件：`pkg/operator/operatorhub/artifact_versiong.go:72-93` -- 同样的模式可顺便审视：`waitPackageManifest` 已经对 NotFound 单独处理（line 98），但其他 transient 没有，建议一并对齐 - -# Acceptance Criteria - -- [ ] NotFound 视为继续轮询 -- [ ] ServerTimeout / TooManyRequests / ServiceUnavailable 视为继续轮询并打 WARN log -- [ ] RBAC denied 等永久错误立即返回 -- [ ] 新增单元测试用 reactor 注入各类错误验证 - -# Work Log - -- 2026-05-18: code review 发现 by silent-failure-hunter - -# Resources - -- PR: https://github.com/AlaudaDevops/upgrade-test/pull/14 -- K8s client-go errors package diff --git a/todos/006-pending-p2-download-close-error-swallowed.md b/todos/006-pending-p2-download-close-error-swallowed.md deleted file mode 100644 index 3d60e34..0000000 --- a/todos/006-pending-p2-download-close-error-swallowed.md +++ /dev/null @@ -1,88 +0,0 @@ ---- -name: download-close-error-swallowed -description: downloadToTemp 的 defer f.Close() 吞掉 Close 返回的 flush 错误，磁盘满或 NFS 重置时静默写出截断的 .tgz -status: pending -priority: p2 -issue_id: "006" -tags: [code-review, quality, correctness, p2] -dependencies: [] ---- - -# Problem Statement - -`pkg/operator/operatorhub/violet.go:246-256`： - -```go -f, err := os.Create(filePath) -// ... -defer f.Close() - -if _, err := io.Copy(f, resp.Body); err != nil { - cleanup() - return "", nil, fmt.Errorf("copy body to %s: %w", filePath, err) -} - -return filePath, cleanup, nil -``` - -许多文件系统在 `Close()` 时才真正 flush write buffer（不是 `Write()`）。如果磁盘恰好在最后一段 flush 时撑爆（ENOSPC）/ NFS 服务器重置 / quota exceeded，`io.Copy` 返回 success，函数返回 "success" 但落地的是截断/损坏的 .tgz。 - -如果 `ExpectedSha256` 未设置（todo 003 默认场景），违法包直接进入 violet push，产生一个完全指错方向的 "violet push: ..." 错误（实际根因是磁盘）。 - -# Findings - -- **silent-failure-hunter** F4（HIGH）独立命中 -- 文件：`pkg/operator/operatorhub/violet.go:246-256` -- 与 todo 003（SHA 强制）相互独立但相互补强 - -# Proposed Solutions - -**Option A（推荐）**：显式 close-and-check 替代 defer。 - -```go -if _, err := io.Copy(f, resp.Body); err != nil { - f.Close() - cleanup() - return "", nil, fmt.Errorf("copy body to %s: %w", filePath, err) -} -if err := f.Close(); err != nil { - cleanup() - return "", nil, fmt.Errorf("close %s after download: %w", filePath, err) -} -``` - -- 优点：直击根因；与 io.Copy 错误路径并列，可读性好 -- 缺点：失去 defer 的 panic-safe 兜底（但本函数无 panic 风险点） -- effort: Small -- risk: Low - -**Option B**：用 `defer func() { if cerr := f.Close(); cerr != nil && err == nil { err = cerr } }()` 模式，让命名返回参数捕获 Close 错误。 - -- 优点：保持 defer 的对称性 -- 缺点：需要把返回签名改为命名返回，可读性略降 -- effort: Small -- risk: Low - -# Recommended Action - -(待 triage) - -# Technical Details - -- 影响文件：`pkg/operator/operatorhub/violet.go:246-256` -- 测试：mock `os.File` 不容易；可用 `httptest` 配合 `t.TempDir()` 做正向测试。负向测试（Close 失败）可暂缓——主要靠 code review 保证不退化 - -# Acceptance Criteria - -- [ ] Close 返回的错误会被传播为函数返回值 -- [ ] 错误信息明确指向 `close %s after download` 阶段 -- [ ] 现有 `TestDownloadToTemp_Success` / `_404` / `_5xx` / `_DefaultFilename` 保持通过 - -# Work Log - -- 2026-05-18: code review 发现 by silent-failure-hunter - -# Resources - -- PR: https://github.com/AlaudaDevops/upgrade-test/pull/14 -- 关联 todo: 003（SHA 强制可在配置层兜底，但 Close 修复仍是独立必要） diff --git a/todos/007-pending-p2-empty-csv-fed-to-subscription.md b/todos/007-pending-p2-empty-csv-fed-to-subscription.md deleted file mode 100644 index 6b3a3ec..0000000 --- a/todos/007-pending-p2-empty-csv-fed-to-subscription.md +++ /dev/null @@ -1,91 +0,0 @@ ---- -name: empty-csv-fed-to-subscription -description: operator.go:117 的 `csv, _, _ := NestedString(av.Object, "status", "version")` 完全不检查 csv 是否为空就传给 InstallSubscription，造成下游莫名失败 -status: pending -priority: p2 -issue_id: "007" -tags: [code-review, quality, correctness, p2] -dependencies: [] ---- - -# Problem Statement - -`pkg/operator/operatorhub/operator.go:117-122`： - -```go -csv, _, _ := unstructured.NestedString(av.Object, "status", "version") -channel := version.Channel -if channel == "" { - channel = "stable" // default fallback -} -if err := o.InstallSubscription(ctx, csv, channel); err != nil { -``` - -`csv` 取值返回三元组 `(value, found, err)` 全部被丢弃；如果 AV 的 `status.version` 类型错（schema drift / controller bug 写了 null）或字段不存在，`csv` 就是空字符串，被原样喂给 `InstallSubscription`，后者远远地以一个"no matching CSV"风格的错误失败，根因离失败点很远。 - -`installViaViolet` 内部对 `csv` 已经有 `!found || csv == ""` 的强检查（violet.go:192-195），返回了带 AV 名字的明确错误。但 **`UpgradeOperator` 的第二次提取没复用前面的强校验**，相当于在同一条调用链上做了两次相同操作，第二次完全裸奔。 - -# Findings - -- **silent-failure-hunter** F5（MEDIUM）独立命中 -- 文件：`pkg/operator/operatorhub/operator.go:117` -- 同一字段在 `violet.go:192` 已经强校验过 - -# Proposed Solutions - -**Option A（推荐）**：让 `InstallArtifactVersion` 把 `csv` 也作为返回值传出来，避免 caller 二次提取。 - -```go -func (o *Operator) InstallArtifactVersion(ctx context.Context, version config.Version) (*unstructured.Unstructured, string, error) { - // ... - return av, csv, nil -} - -// UpgradeOperator: -av, csv, err := o.InstallArtifactVersion(ctx, version) -``` - -- 优点：单一来源，杜绝二次提取漂移 -- 缺点：签名变更，影响接口兼容性 -- effort: Small -- risk: Low - -**Option B**：在 `UpgradeOperator` 内复用 NestedString 但走强校验。 - -```go -csv, found, err := unstructured.NestedString(av.Object, "status", "version") -if err != nil { - return fmt.Errorf("AV %s status.version has wrong type: %w", av.GetName(), err) -} -if !found || csv == "" { - return fmt.Errorf("AV %s status.version is empty", av.GetName()) -} -``` - -- 优点：签名不变 -- 缺点：与 `installViaViolet` 内部的同样代码重复 -- effort: Negligible -- risk: Low - -# Recommended Action - -(待 triage) - -# Technical Details - -- 影响文件：`pkg/operator/operatorhub/operator.go:109-127`、`pkg/operator/operatorhub/artifact_versiong.go:18` -- 测试：现有 unit 覆盖偏少；可加 fake-client 注入 status.version=null 的 AV，断言 UpgradeOperator 在 AV 阶段就报错 - -# Acceptance Criteria - -- [ ] 空字符串 / 类型错的 `status.version` 在 `UpgradeOperator` 入口就报错（带 AV 名字） -- [ ] 不再有"InstallSubscription with empty csv"的可能路径 -- [ ] 单元测试覆盖 - -# Work Log - -- 2026-05-18: code review 发现 by silent-failure-hunter - -# Resources - -- PR: https://github.com/AlaudaDevops/upgrade-test/pull/14 diff --git a/todos/008-pending-p2-channel-default-inconsistency.md b/todos/008-pending-p2-channel-default-inconsistency.md deleted file mode 100644 index dffc964..0000000 --- a/todos/008-pending-p2-channel-default-inconsistency.md +++ /dev/null @@ -1,80 +0,0 @@ ---- -name: channel-default-inconsistency -description: BuildPackageURL 对空 channel fail-fast，但 UpgradeOperator 紧接着的 InstallSubscription 把空 channel 静默回退成 "stable"，同一调用链两套策略 -status: pending -priority: p2 -issue_id: "008" -tags: [code-review, quality, correctness, p2] -dependencies: [] ---- - -# Problem Statement - -同一字段 `version.Channel` 在同一次升级流程中被两个 caller 用截然相反的策略处理： - -1. `violet.go:55-56` `BuildPackageURL` ——空 channel **直接报错**：`"channel is empty (Version.Channel is required when using violet)"` -2. `operator.go:118-121` `UpgradeOperator` ——空 channel **静默回退到 "stable"**： - ```go - channel := version.Channel - if channel == "" { - channel = "stable" // default fallback - } - ``` - -当前因为 `installViaViolet` 跑在前面，空 channel 早就被拦在 BuildPackageURL 阶段，所以 (2) 是 dead branch。但只要未来有任何非 violet 的 install 路径（或 violet 路径分支早返回），(2) 立刻生效——用户在 YAML 里手抖打成 `chanel:` 或者忘填，Subscription 静默走 stable channel，**升级在错误的 channel 上"成功"**，后果是装错 bundle。 - -# Findings - -- **silent-failure-hunter** F7（MEDIUM）+ **type-design-analyzer** T5（关于 Channel 字段语义）双重命中 -- 文件：`pkg/operator/operatorhub/operator.go:118-121`、`pkg/operator/operatorhub/violet.go:55-56` -- 类别：同一字段、同一调用链上的策略不一致 - -# Proposed Solutions - -**Option A（推荐）**：移除 operator.go 的 silent fallback；与 BuildPackageURL 对齐到 "channel 必填，空即报错"。 - -```go -if version.Channel == "" { - return fmt.Errorf("version.channel is required") -} -if err := o.InstallSubscription(ctx, csv, version.Channel); err != nil { ... } -``` - -或者更进一步，在 `Config.Validate()`（参见 todo 010）阶段提前拦下。 - -- 优点：单一策略；fail-fast；与 todo 010 天然合流 -- 缺点：现有 config 如果省略 channel 会立即失败（实际上必然已经在 BuildPackageURL 失败了，所以是 no-op 用户感知） -- effort: Negligible -- risk: Low - -**Option B**：反向对齐——`BuildPackageURL` 也回退到 stable。 - -- 优点：宽容 -- 缺点：把 BuildPackageURL 的 fail-fast 设计意图打掉；rejected -- effort: Negligible -- risk: Medium（弱化错误检测） - -# Recommended Action - -(待 triage) - -# Technical Details - -- 影响文件：`pkg/operator/operatorhub/operator.go:118-121` -- 配合 todo 010（VioletConfig.Validate）一并实现，把 `channel != ""` 提到 config 加载阶段 - -# Acceptance Criteria - -- [ ] 移除 operator.go:118-121 的 silent "stable" fallback -- [ ] 空 channel 在 `Config.Validate()` 或 `UpgradeOperator` 入口报错 -- [ ] 错误信息明确指向 bundleVersion + Channel 字段 -- [ ] 现有测试不退化 - -# Work Log - -- 2026-05-18: code review 发现 by silent-failure-hunter + type-design-analyzer - -# Resources - -- PR: https://github.com/AlaudaDevops/upgrade-test/pull/14 -- 关联 todo: 010（VioletConfig.Validate 总入口） diff --git a/todos/010-pending-p2-violet-config-validate-method.md b/todos/010-pending-p2-violet-config-validate-method.md index 53ee34f..21453e5 100644 --- a/todos/010-pending-p2-violet-config-validate-method.md +++ b/todos/010-pending-p2-violet-config-validate-method.md @@ -15,7 +15,7 @@ VioletConfig 的校验目前分布在三个站点： - `BuildPackageURL`（violet.go:49-62）—— PackagePrefix / Channel / BundleVersion 空字符串检查 - `validateVioletBin`（violet.go:340-355）—— Bin 路径形态 -`OperatorConfig.Type == "operatorhub"` 且 `Violet == nil` 的非法组合，则要拖到 `InstallArtifactVersion`（artifact_versiong.go:19-21）才报错——用户跑了一半升级才发现 config 没写完。 +`OperatorConfig.Type == "operatorhub"` 且 `Violet == nil` 的非法组合，则要拖到 `InstallArtifactVersion`（artifact_version.go:19-21）才报错——用户跑了一半升级才发现 config 没写完。 # Findings