fix/opv2-fix-dropped-SA #316
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| package reconciler | ||
|
|
||
| import ( | ||
| v2 "github.com/wandb/operator/api/v2" | ||
| "github.com/wandb/operator/pkg/utils" | ||
| ) | ||
|
|
||
| // serviceAccountIssuerUnknownReason marks a CR that can't be reconciled because | ||
| // the cluster's service-account issuer is neither configured nor discoverable. | ||
| const serviceAccountIssuerUnknownReason = "ServiceAccountIssuerUnknown" | ||
|
|
||
| const serviceAccountIssuerUnknownMessage = "could not determine the cluster service-account issuer: " + | ||
| "set spec.wandb.internalServiceAuth.oidcIssuer to the value of " + | ||
| "`kubectl get --raw /.well-known/openid-configuration`, or grant the operator get on that URL " + | ||
| "and restart it" | ||
|
Comment on lines
+8
to
+15
Contributor
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)internal/controller/reconciler/internal_service_auth\.go$|(^|/)suite_test\.go$|(^|/)Makefile$' || true
echo "== file excerpt =="
sed -n '1,160p' internal/controller/reconciler/internal_service_auth.go
echo "== references in tracked files =="
rg -n "serviceAccountIssuerUnknownReason|serviceAccountIssuerUnknownMessage|ServiceAccountIssuerUnknown|internalServiceAuthIssuerUnknown|internalServiceAuthIssuer" . --glob '!vendor/**' --glob '!node_modules/**' || true
echo "== lint config excerpt =="
for f in .golangci.yml .golangci.yaml golangci.yml golangci.yaml; do
if [ -f "$f" ]; then
echo "-- $f --"
sed -n '1,220p' "$f"
fi
done
echo "== golangci linters list if present =="
python3 - <<'PY'
from pathlib import Path
import re, yaml, json, os
for p in [Path(".golangci.yml"), Path(".golangci.yaml"), Path("golangci.yml"), Path("golangci.yaml")]:
if p.exists():
data=yaml.safe_load(p.read_text())
print("golangci.yml linters configuration:", data.get("linters") if data else None)
print("linters.enable:", (data.get("linters") or {}).get("enable"))
print("linters.disble:", (data.get("linters") or {}).get("disable"))
print("linters.defaults.enable:", ((data.get("linters") or {}).get("defaults") or {}).get("enable"))
PYRepository: wandb/operator Length of output: 2998 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)internal/controller/reconciler/internal_service_auth\.go$|(^|/)suite_test\.go$|(^|/)Makefile$' || true
echo "== file excerpt =="
sed -n '1,160p' internal/controller/reconciler/internal_service_auth.go
echo "== references in tracked files =="
rg -n "serviceAccountIssuerUnknownReason|serviceAccountIssuerUnknownMessage|ServiceAccountIssuerUnknown|internalServiceAuthIssuerUnknown|internalServiceAuthIssuer" . --glob '!vendor/**' --glob '!node_modules/**' || true
echo "== lint config excerpt =="
for f in .golangci.yml .golangci.yaml golangci.yml golangci.yaml; do
if [ -f "$f" ]; then
echo "-- $f --"
sed -n '1,220p' "$f"
fi
done
echo "== golangci linters list if present =="
python3 - <<'PY'
from pathlib import Path
import yaml
for p in [Path(".golangci.yml"), Path(".golangci.yaml"), Path("golangci.yml"), Path("golangci.yaml")]:
if p.exists():
data=yaml.safe_load(p.read_text())
print("golangci.yml linters configuration:", data.get("linters") if data else None)
print("linters.enable:", (data.get("linters") or {}).get("enable"))
print("linters.disable:", (data.get("linters") or {}).get("disable"))
print("linters.defaults.enable:", ((data.get("linters") or {}).get("defaults") or {}).get("enable"))
PYRepository: wandb/operator Length of output: 2998 🌐 Web query:
💡 Result: The unused linter is included in the default set of linters enabled by golangci-lint [1][2]. It is responsible for checking Go code for unused constants, variables, functions, and types [1][3]. If you are encountering errors related to "unused declarations," it is likely because this linter is enabled by default [1]. You do not need to explicitly enable it in your configuration file. Key points regarding the unused linter: 1. Enabled by Default: It is part of the standard/default set of linters [1][2]. You can verify this by running the command: golangci-lint help linters [1][4]. 2. Functionality: It replaces older, now-deprecated linters such as varcheck, deadcode, and structcheck, consolidating their functionality into a single, maintained tool [5]. 3. Managing Errors: - If you wish to disable it, you can do so in your.golangci.yml configuration file: linters: disable: - unused - Alternatively, you can use a //nolint:unused comment to suppress the error for a specific line or block of code [6]. For a complete list of all currently enabled linters, you can execute golangci-lint help linters in your terminal [1][4]. Citations:
Remove or use the unused issuer-error constants.
🧰 Tools🪛 golangci-lint (2.12.2)[error] 10-10: const serviceAccountIssuerUnknownReason is unused (unused) [error] 12-12: const serviceAccountIssuerUnknownMessage is unused (unused) 🤖 Prompt for AI AgentsSources: Coding guidelines, Linters/SAST tools |
||
|
|
||
| // internalServiceAuthEnabled reports whether W&B services validate each other's | ||
| // projected ServiceAccount tokens. | ||
| func internalServiceAuthEnabled(wandb *v2.WeightsAndBiases) bool { | ||
| return wandb.Spec.Wandb.InternalServiceAuth.Enabled != nil && | ||
| *wandb.Spec.Wandb.InternalServiceAuth.Enabled | ||
| } | ||
|
|
||
| // resolveInternalServiceAuthIssuer returns the issuer W&B services must validate | ||
| // projected ServiceAccount tokens against: the explicit CR value when set, else | ||
| // the issuer discovered from the cluster at start-up. | ||
| // | ||
| // Empty means unknown, and callers must not substitute a guess. The API server | ||
| // stamps its own --service-account-issuer as the token's `iss`, so any other | ||
| // value fails validation — which surfaces as a 401 and panics the API rather | ||
| // than degrading. | ||
| func resolveInternalServiceAuthIssuer(wandb *v2.WeightsAndBiases) string { | ||
| if wandb.Spec.Wandb.InternalServiceAuth.OIDCIssuer != "" { | ||
| return wandb.Spec.Wandb.InternalServiceAuth.OIDCIssuer | ||
| } | ||
| return utils.ServiceAccountIssuer() | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pair
createwithnameinstead of applying them independently.The loop applies
createandnameas separate fields. A v1 block that setscreate: falseand omitsnametherefore producesCreate=falsewith an emptyServiceAccountName, and the v2 CRD then defaults the name towandb. Pods reference awandbServiceAccount that nothing creates. The function doc comment describes this exact failure mode.The same independence lets
createcome fromappwhilenamecomes fromapi, which combines two subcharts into one identity that neither declared.Reject or ignore an incomplete identity instead. Example: fail conversion when a block sets
create=falsewithout aname, so the operator asks for an explicit v2 value.🐛 Proposed fix: carry create and name from the same block
// First non-empty wins, so earlier subcharts take precedence. sa := &dst.Spec.Wandb.ServiceAccount for _, block := range blocks { + // create=false without a name is unrepresentable in v2: the CRD would + // default the name to wandb and pods would reference a ServiceAccount + // nothing creates. + if block.create != nil && !*block.create && block.name == "" { + return fmt.Errorf( + "spec.values.%s.serviceAccount: create=false without name; "+ + "set spec.wandb.serviceAccount.serviceAccountName explicitly", + block.subchart) + } if block.create != nil && sa.Create == nil { sa.Create = ptr.To(*block.create) }Add a test for
create: falsewith noname, and a test forcreateinappwithnameonly inapi.🤖 Prompt for AI Agents