bug fix: Don't drop secrets from index that don't come from ci-secret-generator config#5311
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
approved
📝 WalkthroughWalkthroughChangesGSM index preservation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 17✅ Passed checks (17 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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)
cmd/ci-secret-generator/main.go (1)
287-289: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated GSM key-normalization logic.
The same
NormalizeName(...) + CollectionSecretDelimiter + NormalizeName(...)construction appears in bothconfigManagedIndexEntriesand the main loop. Extracting a shared helper avoids future drift between the "managed" set and the "generated" set (which would silently break the merge logic).♻️ Proposed helper extraction
+func gsmIndexKey(itemName, fieldName string) string { + return fmt.Sprintf("%s%s%s", gsmvalidation.NormalizeName(itemName), gsmvalidation.CollectionSecretDelimiter, gsmvalidation.NormalizeName(fieldName)) +} + func configManagedIndexEntries(config secretgenerator.Config) sets.Set[string] { managed := sets.New[string]() for _, item := range config { for _, field := range item.Fields { - normalizedGroup := gsmvalidation.NormalizeName(item.ItemName) - normalizedField := gsmvalidation.NormalizeName(field.Name) - managed.Insert(fmt.Sprintf("%s%s%s", normalizedGroup, gsmvalidation.CollectionSecretDelimiter, normalizedField)) + managed.Insert(gsmIndexKey(item.ItemName, field.Name)) } } return managed }and reuse
gsmIndexKey(item.ItemName, field.Name)in place of Lines 336-338.Also applies to: 336-338
🤖 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 `@cmd/ci-secret-generator/main.go` around lines 287 - 289, Extract the duplicated normalized GSM key construction into a shared gsmIndexKey helper that normalizes the group and field names and joins them with CollectionSecretDelimiter. Update both configManagedIndexEntries and the main loop to call gsmIndexKey(item.ItemName, field.Name), replacing their inline construction while preserving the existing managed/generated set 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 `@cmd/ci-secret-generator/main.go`:
- Around line 283-293: Update configManagedIndexEntries to accept
disabledClusters and exclude fields belonging to disabled clusters, matching the
skip behavior in buildSecretsToUpdate. Propagate the new argument from its
callers so mergeIndexEntries preserves existing index entries for disabled
clusters, and add a test combining existingIndexValues with a disabled cluster.
---
Nitpick comments:
In `@cmd/ci-secret-generator/main.go`:
- Around line 287-289: Extract the duplicated normalized GSM key construction
into a shared gsmIndexKey helper that normalizes the group and field names and
joins them with CollectionSecretDelimiter. Update both configManagedIndexEntries
and the main loop to call gsmIndexKey(item.ItemName, field.Name), replacing
their inline construction while preserving the existing managed/generated set
behavior.
🪄 Autofix (Beta)
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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2a48fe7f-9117-4d1c-9dd0-56ff81c02157
📒 Files selected for processing (2)
cmd/ci-secret-generator/main.gocmd/ci-secret-generator/main_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
openshift/release(manual)openshift/ci-docs(manual)openshift/release-controller(manual)openshift/ci-chat-bot(manual)
psalajova
force-pushed
the
fix-ci-secret-generator-preserve-manual-index-entries
branch
from
705c194 to
aca8969
Compare
|
/override ci/prow/images |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Prucek, psalajova The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@Prucek: Overrode contexts on behalf of Prucek: ci/prow/images DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Scheduling tests matching the |
|
the e2e test ran for 3h without even starting.... |
|
/test e2e |
|
@psalajova: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
ci-secret-generatorruns every 12h and rebuilds the collection index from_config.yaml. Secrets created manually (e.g.secret-manager create cherrypick-signing-key/id_ed25519using the CLI tool) were dropped from the index each run — GSM kept the secret but the index didn't list it, sosecret-manager listmissed it andcreate/updatecould fail.Root cause: The generator read the previous index but only used it to rescue failed command executions — it never merged in entries not derived from config.
Fix: After building index entries from config, merge in previous index entries whose names don't appear in the config-managed set. Index is now sorted for determinism.
Example: Some GSM secrets (including
cherrypick-signing-key__id_ed25519) were missing from the index while present in GSM. After this fix they persist across generator runs.Trade-off: Secrets removed from config will linger in the index (indistinguishable from manual secrets). No GSM-side deletion occurs — cleanup is manual.
Fix
ci-secret-generatordropping manually created GSM secrets during its periodic (12-hour) index rebuilds.In the
cmd/ci-secret-generatorcomponent, newly generated index entries from_config.yamlare now merged with the previous index contents for any entries that are not config-managed (including entries for disabled clusters). The resulting index is then sorted deterministically sosecret-managercan reliably discover the full set of GSM secrets, not just those coming from config. If a secret is removed from_config.yaml, it is kept in the index and requires manual cleanup; the generator does not delete anything in GSM.Unit tests were updated to verify preservation of manually created entries across rebuilds and to assert the expected deterministic ordering of index fields/items.