fix: Regenerate non-UTF-8 adopted generated secrets - #300
Open
wnevis-cmyk wants to merge 4 commits into
Open
Conversation
Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
📝 WalkthroughWalkthroughChangesGenerated Secret validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 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}
Contributor
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/generate_secrets_test.go`:
- Around line 19-34: Rewrite the tests in generate_secrets_test.go from
testing/Testify and fake.NewClientBuilder to the repository’s Ginkgo/Gomega spec
style, attaching them to the appropriate suite_test.go and using envtest. Remove
the direct testing, Testify, and fake-client dependencies where no longer
needed, then run make lint and make test and report their results before
merging.
🪄 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: 55169eab-b5da-4cc8-84ac-8281f987cb79
📒 Files selected for processing (2)
internal/controller/reconciler/generate_secrets_test.gointernal/controller/reconciler/reconcile_v2.go
Comment on lines
+19
to
+34
| import ( | ||
| "context" | ||
| "testing" | ||
| "unicode/utf8" | ||
|
|
||
| "github.com/stretchr/testify/require" | ||
| corev1 "k8s.io/api/core/v1" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| ctrlClient "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/controller-runtime/pkg/client/fake" | ||
|
|
||
| apiv2 "github.com/wandb/operator/api/v2" | ||
| serverManifest "github.com/wandb/operator/pkg/wandb/manifest" | ||
| ) |
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 target file and suite files =="
git ls-files | rg '(^|/)internal/controller/reconciler/generate_secrets_test\.go$|(^|/)suite_test\.go$|(^|/)go\.mod$'
echo
echo "== target file imports and test declarations =="
sed -n '1,220p' internal/controller/reconciler/generate_secrets_test.go
echo
echo "== test framework indicators =="
rg -n 'Describe|Context|It|When|Specify|testing\.|func Test|\bdRequire\(|\bgomega\b|Ginkgo|TestingT|ginkgo|suite_test\.go|envtest|testing\.B|func TestMain' -S .
echo
echo "== lint/test make targets and go.mod deps =="
if [ -f Makefile ]; then sed -n '1,220p' Makefile; fi
echo
[ -f go.mod ] && awk '/module |require \(/,/^)/' go.mod | sed -n '1,220p'Repository: wandb/operator
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file imports/declarations =="
sed -n '1,180p' internal/controller/reconciler/generate_secrets_test.go
echo
echo "== focused test framework indicators excluding vendored/crds =="
rg -n --glob '!pkg/vendored/**' --glob '!**/crds/**' 'func Test[A-Za-z0-9_]+|require\..+|\.Expect\(|Describe\(|Context\(|It\(|GinkgoT|Gomega|envtest|suite_test\.go' --glob '*_test.go' . | sed -n '1,240p'
echo
echo "== reconciler suite files =="
git ls-files 'internal/controller/**suite_test.go' 'internal/controller/**/*_test.go'
echo
echo "== Makefile relevant targets =="
[ -f Makefile ] && (sed -n '/^lint:/,/^test:/p' Makefile; sed -n '/^test:/,/^[a-zA-Z_][A-Za-z0-9_-]*:/p' Makefile)
echo
echo "== go.mod relevant deps =="
[ -f go.mod ] && awk '/^module |^require \(/,/^)/' go.mod | rg 'ginkgo|gomega|envtest|testify' || trueRepository: wandb/operator
Length of output: 42962
Use the required test framework and run validation before merge.
internal/controller/reconciler/generate_secrets_test.go uses testing, Testify, and fake.NewClientBuilder. The repository policy requires Ginkgo/Gomega specs attached to suite_test.go files with envtest.
Also run make lint and make test and include their results before merge.
🤖 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/generate_secrets_test.go` around lines 19 -
34, Rewrite the tests in generate_secrets_test.go from testing/Testify and
fake.NewClientBuilder to the repository’s Ginkgo/Gomega spec style, attaching
them to the appropriate suite_test.go and using envtest. Remove the direct
testing, Testify, and fake-client dependencies where no longer needed, then run
make lint and make test and report their results before merging.
Source: Coding guidelines
Contributor
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/reconcile_v2.go`:
- Around line 1465-1481: Update internal/controller/reconciler/reconcile_v2.go
lines 1465-1481 so invalid adopted Secret values are replaced with a newly
generated UTF-8-safe value, persisted to the Secret, and reconciliation
continues successfully instead of emitting an error and returning. Update
internal/controller/reconciler/generate_secrets_test.go lines 78-114 to assert
successful reconciliation and verify the replaced value is a valid generated
token.
🪄 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: 8760ac0a-3006-4b9e-8bc6-7d22561aeee0
📒 Files selected for processing (5)
internal/controller/common/condition.gointernal/controller/reconciler/generate_secrets_test.gointernal/controller/reconciler/reconcile_v2.gointernal/controller/weightsandbiases_controller_networking_test.gointernal/controller/weightsandbiases_controller_test.go
Comment on lines
+1465
to
+1481
| // Secret exists; don't overwrite a valid existing value. | ||
| existing, hasKey := sec.Data[keyName] | ||
| // Non-UTF-8 secretKeyRef env vars break container creation. | ||
| if hasKey && !utf8.Valid(existing) { | ||
| msg := fmt.Sprintf( | ||
| "generated secret %q key %q contains non-UTF-8 bytes; values consumed as container environment variables must be valid UTF-8 — replace it with a UTF-8-safe value", | ||
| secretName, keyName, | ||
| ) | ||
| recorder.Event(wandb, corev1.EventTypeWarning, common.InvalidSecretEncodingReason, msg) | ||
| if err := updateReadyStatus(ctx, client, wandb, statusBefore, false, common.InvalidSecretEncodingReason, msg); err != nil { | ||
| return ctrl.Result{}, err | ||
| } | ||
| // Generate a value only if missing | ||
| return ctrl.Result{}, errors.New(msg) | ||
| } | ||
| if !hasKey && sec.StringData == nil { | ||
| // Secret exists but has no usable key; populate one. | ||
| sec.StringData = map[string]string{} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align the implementation and test with regeneration.
The implementation and test encode rejection. The PR objective requires regeneration of invalid adopted values.
- internal/controller/reconciler/reconcile_v2.go#L1465-L1481: Generate and persist a UTF-8-safe replacement instead of returning before the Secret is updated.
- internal/controller/reconciler/generate_secrets_test.go#L78-L114: Assert successful reconciliation and replacement with a valid token.
📍 Affects 2 files
internal/controller/reconciler/reconcile_v2.go#L1465-L1481(this comment)internal/controller/reconciler/generate_secrets_test.go#L78-L114
🤖 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/reconcile_v2.go` around lines 1465 - 1481,
Update internal/controller/reconciler/reconcile_v2.go lines 1465-1481 so invalid
adopted Secret values are replaced with a newly generated UTF-8-safe value,
persisted to the Secret, and reconciliation continues successfully instead of
emitting an error and returning. Update
internal/controller/reconciler/generate_secrets_test.go lines 78-114 to assert
successful reconciliation and verify the replaced value is a valid generated
token.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of the Operator v2 migration bugfix work from Ajay Pandey's V1 -> V2 migration test.
Problem: the
weave-worker-authgenerated secret usesuseExactName: true, so on migrationgenerateSecretsadopts the pre-existing v1 secret. If v1 stored a raw binary (non-UTF-8) token underkey, it is injected as theWANDB_INTERNAL_SERVICE_TOKENstring env var and kubelet fails container creation withgrpc: error while marshaling: string field contains invalid UTF-8, leaving weave-trace pods inCreateContainerError.Fix:
generateSecretsnow detects an adopted generated secret whosekeyvalue is not valid UTF-8 and regenerates it with a UTF-8-safe token (mirrors the accepted manual fix). Valid existing values are left untouched. Applied generically to all generated secrets since the encoding hazard is generic.internal/controller/reconciler/reconcile_v2.go: UTF-8 guard in the existing-secret branch ofgenerateSecrets.Verification:
golangci-lint(0 issues) + targetedgo testpass.Summary by CodeRabbit