ci-secret-generator: Sync *all* secrets to GSM
#4809
Conversation
WalkthroughAdds GSM index-secret support: new client method Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
openshift-ci
bot
added
the
approved
|
/cc @danilo-gemoli |
![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)
pkg/gsm-secrets/secrets.go (1)
71-83: Function logic is correct.The
ConstructIndexSecretContentfunction properly:
- Includes the updater service account via
UpdaterSASecretName- Sorts the list for deterministic output
- Formats each entry as "- "
Note: The function modifies the input slice by appending. While this is not currently an issue since callers pass a new slice, consider documenting this behavior or creating a copy internally for safety.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (9)
cmd/ci-secret-generator/main.go(3 hunks)cmd/ci-secret-generator/main_test.go(1 hunks)pkg/gsm-secrets/execution.go(1 hunks)pkg/gsm-secrets/secrets.go(2 hunks)pkg/gsm-secrets/secrets_test.go(1 hunks)pkg/gsm-secrets/types.go(1 hunks)pkg/secrets/client.go(1 hunks)pkg/secrets/gsm.go(2 hunks)pkg/secrets/vault.go(2 hunks)
🔇 Additional comments (12)
pkg/gsm-secrets/types.go (1)
18-18: LGTM! Clear constant definition.The new
UpdaterSASecretNameconstant provides a centralized definition for the updater service account secret name, which is used in index secret content construction. This improves maintainability.pkg/gsm-secrets/execution.go (1)
268-271: Good refactoring to use centralized index content construction.The change from inline formatting to
ConstructIndexSecretContent([]string{})centralizes the index secret payload logic, improving consistency across the codebase.pkg/secrets/client.go (1)
21-21: LGTM! Clean interface extension.The new
UpdateIndexSecretmethod cleanly extends theClientinterface to support index secret updates. All implementations properly satisfy this interface requirement.pkg/secrets/vault.go (2)
47-49: LGTM! Appropriate no-op implementation for dry-run client.The no-op implementation is correct for the dry-run client, which doesn't need to perform actual index secret operations.
137-140: LGTM! Clear no-op with good documentation.The no-op implementation is appropriate since index secrets are GSM-specific and not needed for Vault operations. The comment clearly explains this rationale.
cmd/ci-secret-generator/main_test.go (1)
196-196: LGTM! Test correctly updated for new signature.The test properly passes
falsefor the newGSMsyncEnabledparameter, maintaining the existing test behavior while adapting to the extended function signature.cmd/ci-secret-generator/main.go (4)
214-214: LGTM! Clean signature extension.The addition of the
GSMsyncEnabledparameter cleanly extends the function to support conditional GSM index secret updates without breaking existing behavior.
218-246: Good implementation of index secret tracking.The logic correctly:
- Initializes
secretsListper item (one index per collection)- Only adds secrets that were successfully processed (after
SetFieldOnItemsucceeds)- Normalizes secret names using
gsm.NormalizeSecretName- Skips disabled clusters
248-254: Index secret update logic is correct.The implementation properly:
- Constructs the index payload using the accumulated secrets list
- Uses the correct index secret name via
gsm.GetIndexSecretName- Handles errors by adding them to the error slice
341-341: LGTM! Proper propagation of GSM sync flag.The flag is correctly propagated from the options to the
updateSecretsfunction.pkg/secrets/gsm.go (2)
52-53: Good change to support syncing all secrets.The dynamic secret naming using
itemNamecorrectly removes the hardcoded "cluster-init" pattern, enabling GSM sync for all collections as per the PR objectives.
71-78: LGTM! Index secret update implementation is correct.The
UpdateIndexSecretimplementation properly:
- Uses
gsm.GetIndexSecretNamefor consistent naming- Applies appropriate annotations
- Returns errors directly for proper error propagation
Note: This differs from
SetFieldOnItemwhich logs GSM errors but doesn't fail the Vault write. This is appropriate sinceUpdateIndexSecretis a GSM-only operation without a dual-write concern.
| func TestConstructIndexSecretContent(t *testing.T) { | ||
| testCases := []struct { | ||
| name string | ||
| secretsList []string | ||
| expectedOutput string | ||
| }{ | ||
| { | ||
| name: "empty list", | ||
| secretsList: []string{}, | ||
| expectedOutput: "- updater-service-account", | ||
| }, | ||
| { | ||
| name: "single secret", | ||
| secretsList: []string{"abc"}, | ||
| expectedOutput: "- abc\n- updater-service-account", | ||
| }, | ||
| { | ||
| name: "multiple secrets sorted", | ||
| secretsList: []string{"abc", "second-secret"}, | ||
| expectedOutput: "- abc\n- second-secret\n- updater-service-account", | ||
| }, | ||
| { | ||
| name: "multiple secrets unsorted", | ||
| secretsList: []string{"zebra", "apple", "banana"}, | ||
| expectedOutput: "- apple\n- banana\n- updater-service-account\n- zebra", | ||
| }, | ||
| { | ||
| name: "secrets with hyphens", | ||
| secretsList: []string{"my-secret", "another-secret"}, | ||
| expectedOutput: "- another-secret\n- my-secret\n- updater-service-account", | ||
| }, | ||
| { | ||
| name: "secrets that sort after updater-service-account", | ||
| secretsList: []string{"xyz", "zzz"}, | ||
| expectedOutput: "- updater-service-account\n- xyz\n- zzz", | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range testCases { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| result := ConstructIndexSecretContent(tc.secretsList) | ||
| actualOutput := string(result) | ||
| if actualOutput != tc.expectedOutput { | ||
| t.Errorf("Expected:\n%s\n\nGot:\n%s", tc.expectedOutput, actualOutput) | ||
| } | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Consider additional edge case tests.
The test cases are well-structured and correctly validate the sorting and formatting behavior. However, consider adding test cases for these edge cases:
- Duplicate handling: What happens if the input already contains
"updater-service-account"? Should it be deduplicated or appear twice? - Empty strings: How should empty strings in the input be handled?
- Invalid secret names: Should the function filter out or reject names that fail
ValidateSecretName? - Case sensitivity: How should secrets that differ only in case be sorted (e.g.,
"ABC"vs"abc")?
Add these test cases to improve coverage:
{
name: "input already contains updater-service-account",
secretsList: []string{"abc", "updater-service-account", "xyz"},
expectedOutput: "- abc\n- updater-service-account\n- xyz", // or should it deduplicate?
},
{
name: "input contains empty string",
secretsList: []string{"abc", "", "xyz"},
expectedOutput: "- \n- abc\n- updater-service-account\n- xyz", // or should empty be filtered?
},
{
name: "case sensitivity",
secretsList: []string{"ABC", "abc", "Abc"},
expectedOutput: "- ABC\n- Abc\n- abc\n- updater-service-account", // verify actual sort behavior
},Run this script to verify the implementation's behavior for these edge cases:
🏁 Script executed:
#!/bin/bash
# Description: Check the ConstructIndexSecretContent implementation for edge case handling
# Find and examine the ConstructIndexSecretContent function implementation
ast-grep --pattern 'func ConstructIndexSecretContent($_, $_) $_ {
$$$
}'
# Also check if there's any deduplication logic
rg -nP -A 10 'func ConstructIndexSecretContent' pkg/gsm-secrets/Length of output: 680
🏁 Script executed:
#!/bin/bash
# Find UpdaterSASecretName constant definition
rg -nP 'const.*UpdaterSASecretName|UpdaterSASecretName\s*=' pkg/gsm-secrets/ -B2 -A2Length of output: 56
🏁 Script executed:
#!/bin/bash
# Find UpdaterSASecretName constant definition
rg -nP 'const.*UpdaterSASecretName|var.*UpdaterSASecretName' -A1 -B1
# Also search for its value assignment
rg -nP 'UpdaterSASecretName.*=' -A1Length of output: 347
Add critical edge case tests for duplicate handling, empty strings, and case sensitivity.
The test cases validate basic sorting and formatting correctly, but miss critical edge cases that the current implementation handles without proper safeguards:
-
Duplicate handling: The implementation unconditionally appends
"updater-service-account"without checking if it already exists in the input. If the input contains this secret, it will appear twice in the output. -
Empty strings: Empty strings in the input are not filtered and will appear as
"- "in the output. -
Case sensitivity: Go's
sort.Stringsuses lexicographic ordering where uppercase letters sort before lowercase (e.g.,"ABC"<"abc").
Add these test cases:
{
name: "input already contains updater-service-account",
secretsList: []string{"abc", "updater-service-account", "xyz"},
expectedOutput: "- abc\n- updater-service-account\n- updater-service-account\n- xyz",
},
{
name: "input contains empty string",
secretsList: []string{"abc", "", "xyz"},
expectedOutput: "- \n- abc\n- updater-service-account\n- xyz",
},
{
name: "case sensitivity",
secretsList: []string{"ABC", "abc", "Abc"},
expectedOutput: "- ABC\n- Abc\n- abc\n- updater-service-account",
},🤖 Prompt for AI Agents
In pkg/gsm-secrets/secrets_test.go around lines 168 to 215, the test suite is
missing edge-case coverage for duplicate handling, empty strings, and case
sensitivity; add three new table-driven tests to
TestConstructIndexSecretContent: one named "input already contains
updater-service-account" with secretsList
[]string{"abc","updater-service-account","xyz"} and expectedOutput "- abc\n-
updater-service-account\n- updater-service-account\n- xyz", one named "input
contains empty string" with secretsList []string{"abc","","xyz"} and
expectedOutput "- \n- abc\n- updater-service-account\n- xyz", and one named
"case sensitivity" with secretsList []string{"ABC","abc","Abc"} and
expectedOutput "- ABC\n- Abc\n- abc\n- updater-service-account"; insert them
into the testCases slice so they run with the other cases and assert equality as
existing tests do.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cmd/ci-secret-generator/main_test.go (3)
589-593: Refactor string-based test case identification to use a dedicated field.The string comparison
tc.name == "GSM sync enabled - disabled cluster field excluded from index"is fragile. If the test name changes, this logic silently breaks.Add a dedicated field to the test case struct:
testCases := []struct { name string config secretgenerator.Config GSMsyncEnabled bool + hasDisabledClusters bool expectedIndexCalls int verifyIndexPayload func(t *testing.T, itemName string, payload []byte) }{Then update the test case definition:
{ name: "GSM sync enabled - disabled cluster field excluded from index", config: secretgenerator.Config{{ ItemName: "cluster-test-item", Fields: []secretgenerator.FieldGenerator{ {Name: "field1", Cmd: "printf 'value1'", Cluster: "enabled-cluster"}, {Name: "field2", Cmd: "printf 'value2'", Cluster: "disabled-cluster"}, {Name: "field3", Cmd: "printf 'value3'", Cluster: "enabled-cluster"}, }, }}, GSMsyncEnabled: true, + hasDisabledClusters: true, expectedIndexCalls: 1,And use it in the conditional:
// for the disabled cluster test case, pass the disabled clusters set var disabledClusters sets.Set[string] - if tc.name == "GSM sync enabled - disabled cluster field excluded from index" { + if tc.hasDisabledClusters { disabledClusters = sets.New[string]("disabled-cluster") }
628-639: Clarify comment about index content when all fields fail.The comment states "index contains only updater-service-account" but the code at line 729 passes an empty slice
[]string{}toConstructIndexSecretContent. This suggests thatupdater-service-accountis automatically added by theConstructIndexSecretContentfunction rather than explicitly included in the slice.Consider clarifying the comment to make this explicit:
{ - name: "SetFieldOnItem error - all fields fail, index contains only updater-service-account", + name: "SetFieldOnItem error - all fields fail, index contains only updater-service-account (auto-added by ConstructIndexSecretContent)", config: secretgenerator.Config{{Or update the comment at line 729:
} else { - // When all fields fail, index should only contain updater-service-account + // When all fields fail, ConstructIndexSecretContent adds only updater-service-account expectedPayload = string(gsm.ConstructIndexSecretContent([]string{})) }
744-753: Verify that the actual error count matches the expected count.The test defines
expectedErrorsLenbut never verifies that the actual number of errors matches this expectation, making the test less precise.While parsing and counting individual errors from an aggregated error message can be complex, consider at minimum verifying that the error message contains expected substrings or error indicators. For example:
err := updateSecrets(tc.config, mockClient, nil, true) if err == nil { t.Errorf("expected error but got nil") return } - errStr := err.Error() - if errStr == "" { - t.Errorf("expected non-empty error message") - } + + // Verify error message contains expected failure indicators + errStr := err.Error() + if errStr == "" { + t.Errorf("expected non-empty error message") + } + // Could add more specific validation based on tc.expectedErrorsLen + // e.g., checking for expected error keywords or patterns }) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
cmd/ci-secret-generator/main_test.go(3 hunks)pkg/secrets/client_mock.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
cmd/ci-secret-generator/main_test.gopkg/secrets/client_mock.go
🔇 Additional comments (3)
cmd/ci-secret-generator/main_test.go (2)
10-10: LGTM!The gomock and gsm package imports are appropriately added to support the new GSM index testing functionality.
Also applies to: 16-16
198-198: LGTM!The addition of the
falseparameter correctly maintains backward compatibility for existing tests by disabling GSM sync.pkg/secrets/client_mock.go (1)
1-296: LGTM!This is correctly generated gomock mock code. The mock implementations for
ReadOnlyClient,Client, andSecretUsageComparerinterfaces follow standard gomock patterns. The newUpdateIndexSecretmock method (lines 204-216) properly supports the GSM index functionality tested inmain_test.go.
| } | ||
|
|
||
| func TestUpdateSecretsWithGSMIndex(t *testing.T) { | ||
| t.Parallel() |
There was a problem hiding this comment.
Beware that child tests spawned by a "parallel" parent don't run in parallel each other.
An example is worth more:
func Test1(t *testing.T) {
t.Parallel()
t.Run("subtest1", func(t *testing.T) {
// ...
})
}
func Test2(t *testing.T) {
t.Parallel()
t.Run("subtest2", func(t *testing.T) {
// ...
})
}subtest1 and subtest2 do run in parallel but:
func Test1(t *testing.T) {
t.Parallel()
for i := range 2 {
t.Run(fmt.Sprintf("subtest1.%d", i), func(t *testing.T){
// ...
})
}
}subtest1.1 and subtest1.2 don't. In order to achieve that you have to move t.Parallel() within t.Run(...), see here.
There was a problem hiding this comment.
Ah I see, today I learned 😄 Do you think I should change it? I was following the pattern of other tests in ci-secret-generator/main_test.go
There was a problem hiding this comment.
It depends on what the test is doing. If you won't have any race condition then you can increase the parallelism, otherwise leave it as it is.
There was a problem hiding this comment.
Added the parallelism within the subtests in the newest commit
|
/retest-required |
|
@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. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
2 participants
Extends ci-secret-generator to sync all generated secrets to GSM (previously only cluster-init secrets), and generates index secrets to track collection membership.
Part of the Vault-to-GSM migration (see design doc).
What Changed
{collection}____indexcontaining sorted list of secrets per collectionWhy
build_farmitem (378 keys) → 378 GSM secrets + 1 index secretStructure in GSM
For each collection (e.g., build_farm):
Index content:
Tests created with the help of cursor AI.