fix/opv2-fix-dropped-SA - #316
Conversation
📝 WalkthroughWalkthroughThe v1-to-v2 conversion now maps app and api service-account settings, validates conflicts, preserves annotations, and handles boolean parsing. New service-authentication helpers resolve the OIDC issuer and report token validation enablement. ChangesService-account conversion
Service authentication
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant AppSubchart
participant ApiSubchart
participant mapServiceAccount
participant V2ApplicationServiceAccount
AppSubchart->>mapServiceAccount: provide service-account settings
ApiSubchart->>mapServiceAccount: provide fallback settings
mapServiceAccount->>V2ApplicationServiceAccount: write validated name, create, and annotations
sequenceDiagram
participant ClusterDiscovery
participant ServiceAccountIssuer
participant ServiceAuthHelpers
ClusterDiscovery->>ServiceAccountIssuer: set discovered issuer
ServiceAuthHelpers->>ServiceAccountIssuer: read discovered issuer
ServiceAuthHelpers->>ServiceAuthHelpers: prefer configured issuer when present
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
api/v1/weightsandbiases_conversion_mapping.go (1)
224-252: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider validating annotation disagreement as well.
assertServiceAccountAgreementchecksnameandcreateonly. Ifappandapideclare different annotation maps,mapServiceAccountkeeps theappmap and drops theapimap without any signal. Annotations bind the cloud identity, so a silent drop has the same effect as a wrong name.Either compare the annotation maps and return the same style of conflict error, or state in the doc comment why annotations use last-write-loses precedence instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1/weightsandbiases_conversion_mapping.go` around lines 224 - 252, Extend assertServiceAccountAgreement to track each non-nil service-account annotation map and detect disagreements between subcharts, alongside the existing name and create checks. Return a conflict error identifying both subcharts and their annotation values before mapServiceAccount can silently discard one; otherwise document and intentionally preserve the existing precedence behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1/weightsandbiases_conversion_mapping.go`:
- Around line 206-216: Update the block-processing logic in the conversion
function so create and name are treated as one identity from the same block,
rather than merged independently across blocks. Reject or ignore incomplete
identities, especially create=false without a name, so conversion requires an
explicit v2 value instead of producing a default ServiceAccount name; preserve
valid paired values from a single block. Add coverage for create=false without
name and for create in app with name only in api.
---
Nitpick comments:
In `@api/v1/weightsandbiases_conversion_mapping.go`:
- Around line 224-252: Extend assertServiceAccountAgreement to track each
non-nil service-account annotation map and detect disagreements between
subcharts, alongside the existing name and create checks. Return a conflict
error identifying both subcharts and their annotation values before
mapServiceAccount can silently discard one; otherwise document and intentionally
preserve the existing precedence behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d2d36ce3-6368-414d-89ef-f41ab78fcd71
📒 Files selected for processing (2)
api/v1/weightsandbiases_conversion_mapping.goapi/v1/weightsandbiases_conversion_test.go
| for _, block := range blocks { | ||
| if block.create != nil && sa.Create == nil { | ||
| sa.Create = ptr.To(*block.create) | ||
| } | ||
| if block.name != "" && sa.ServiceAccountName == "" { | ||
| sa.ServiceAccountName = block.name | ||
| } | ||
| if len(block.annotations) > 0 && len(sa.Annotations) == 0 { | ||
| sa.Annotations = block.annotations | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pair create with name instead of applying them independently.
The loop applies create and name as separate fields. A v1 block that sets create: false and omits name therefore produces Create=false with an empty ServiceAccountName, and the v2 CRD then defaults the name to wandb. Pods reference a wandb ServiceAccount that nothing creates. The function doc comment describes this exact failure mode.
The same independence lets create come from app while name comes from api, 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=false without a name, 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: false with no name, and a test for create in app with name only in api.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/v1/weightsandbiases_conversion_mapping.go` around lines 206 - 216, Update
the block-processing logic in the conversion function so create and name are
treated as one identity from the same block, rather than merged independently
across blocks. Reject or ignore incomplete identities, especially create=false
without a name, so conversion requires an explicit v2 value instead of producing
a default ServiceAccount name; preserve valid paired values from a single block.
Add coverage for create=false without name and for create in app with name only
in api.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/reconciler/internal_service_auth.go`:
- Around line 8-15: Remove the unused serviceAccountIssuerUnknownReason and
serviceAccountIssuerUnknownMessage declarations, or wire both into the
reconciliation error path that handles an undiscoverable service-account issuer;
ensure no unused constants remain and preserve the existing issuer error
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 996c876a-6b8a-4e0d-9ec4-1fce07ee40e4
📒 Files selected for processing (3)
internal/controller/reconciler/internal_service_auth.gointernal/controller/reconciler/internal_service_auth_test.gopkg/utils/serviceaccount_issuer.go
| // 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" |
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:
golangci-lint unused declarations error default linters unused
💡 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:
- 1: https://golangci-lint.run/docs/welcome/quick-start/
- 2: https://github.com/golangci/golangci-lint/blob/main/.golangci.reference.yml
- 3: https://golangci-lint.run/docs/linters/
- 4: https://github.com/golangci/golangci-lint/blob/master/docs/src/docs/usage/linters.mdx
- 5: feat: deprecate varcheck, deadcode, and structcheck golangci/golangci-lint#3125
- 6: https://golangci-lint.run/docs/linters/false-positives/
Remove or use the unused issuer-error constants.
serviceAccountIssuerUnknownReason and serviceAccountIssuerUnknownMessage are only declared. golangci-lint flags unused declarations by default, so make lint fails unless they are wired into the reconciliation error path or removed.
🧰 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 Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/controller/reconciler/internal_service_auth.go` around lines 8 - 15,
Remove the unused serviceAccountIssuerUnknownReason and
serviceAccountIssuerUnknownMessage declarations, or wire both into the
reconciliation error path that handles an undiscoverable service-account issuer;
ensure no unused constants remain and preserve the existing issuer error
behavior.
Sources: Coding guidelines, Linters/SAST tools
1 participant
Conversion never writes create or serviceAccountName. mapServiceAccountAnnotations copies annotations and nothing else. v1's serviceAccount.create: false and serviceAccount.name are dropped.
v1 sets SA to create: false + name to use existing SA and tell v2 not to create the default one
Conversion reads neither field, the CRD schema default asserts create: true / serviceAccountName: wandb, and the reconciler does exactly what it's told:
get/create/update/delete and namespaces get/list
Summary by CodeRabbit
Bug Fixes
New Features