ci-secret-generator: Sync *all* secrets to GSM
#4809
Conversation
WalkthroughThis change introduces GSM index secret synchronization capabilities. New methods enable setting fields and updating index secrets in Google Secret Manager. A function normalizes secret listings into index content. Updates propagate through the client interface and implementations, with no-op Vault variants maintaining backward compatibility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (2)cmd/ci-secret-generator/main_test.go (2)
pkg/gsm-secrets/secrets.go (1)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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.
pkg/gsm-secrets/secrets_test.go
Outdated
| 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 |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
psalajova
force-pushed
the
sync-to-gsm
branch
from
435ae02 to
b94f96a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @pkg/secrets/gsm.go:
- Around line 82-97: The doc comment for UpdateIndexSecret is incorrect: payload
is a YAML-formatted newline list (from ConstructIndexSecretContent), not a JSON
array; update the comment to say "payload: YAML newline-separated list (e.g., '-
secret1\n- secret2')" and clarify the created secret name description remains
the same; additionally, if the omission of labels is intentional, add a brief
inline comment in UpdateIndexSecret noting that labels are intentionally nil
(unlike SetFieldOnItem which sets "jira-project": "dptp") or modify the call to
pass labels if it was a mistake so the behavior is explicit.
🧹 Nitpick comments (3)
pkg/secrets/vault.go (1)
47-49: Consider logging index secret in dry-run mode for visibility.Other
dryRunClientmethods (e.g.,SetFieldOnItem,UpdateNotesOnItem) write to the output file for debugging and validation. This method silently returns nil, which could make dry-run output incomplete when GSM sync is enabled.♻️ Optional: Log index secret in dry-run mode
func (d dryRunClient) UpdateIndexSecret(itemName string, payload []byte) error { + _, err := fmt.Fprintf(d.file, "IndexSecret: %s\n\tPayload: %s\n", itemName, string(payload)) + return err - return nil }pkg/gsm-secrets/secrets.go (1)
87-89: Input slice mutation could cause unexpected behavior.The function modifies the input slice by appending and sorting in place. If a caller passes a slice with spare capacity, their original slice's underlying array will be modified. Consider copying the slice first to avoid side effects.
♻️ Proposed fix to avoid mutating input
func ConstructIndexSecretContent(secretsList []string) []byte { - secretsList = append(secretsList, UpdaterSASecretName) + // Copy to avoid mutating the caller's slice + result := make([]string, len(secretsList)+1) + copy(result, secretsList) + result[len(secretsList)] = UpdaterSASecretName + sort.Strings(result) - sort.Strings(secretsList) var formattedSecrets []string - for _, secret := range secretsList { + for _, secret := range result { formattedSecrets = append(formattedSecrets, fmt.Sprintf("- %s", secret)) } return []byte(strings.Join(formattedSecrets, "\n")) }cmd/ci-secret-generator/main_test.go (1)
444-450: AdddisabledClustersto the test struct instead of using name-based conditional.The current approach of checking
tc.name == "GSM sync enabled - disabled cluster field excluded from index"to set disabled clusters is fragile. If the test name changes, the test logic silently breaks.♻️ Suggested refactor
testCases := []struct { name string config secretgenerator.Config GSMsyncEnabled bool expectedIndexCalls int verifyIndexPayload func(t *testing.T, itemName string, payload []byte) + disabledClusters sets.Set[string] }{Then update the test case at line 536:
{ name: "GSM sync enabled - disabled cluster field excluded from index", config: secretgenerator.Config{{ ... }}, GSMsyncEnabled: true, expectedIndexCalls: 1, + disabledClusters: sets.New[string]("disabled-cluster"), verifyIndexPayload: func(t *testing.T, itemName string, payload []byte) {And simplify the setup at line 582:
- // 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" { - disabledClusters = sets.New[string]("disabled-cluster") - } - - err := updateSecrets(tc.config, mockClient, disabledClusters, tc.GSMsyncEnabled) + err := updateSecrets(tc.config, mockClient, tc.disabledClusters, tc.GSMsyncEnabled)Also applies to: 582-585
📜 Review details
Configuration used: Organization 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.gocmd/ci-secret-generator/main_test.gopkg/gsm-secrets/execution.gopkg/gsm-secrets/secrets.gopkg/gsm-secrets/types.gopkg/secrets/client.gopkg/secrets/client_mock.gopkg/secrets/gsm.gopkg/secrets/vault.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/secrets/client_mock.go
- pkg/gsm-secrets/types.go
🧰 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:
pkg/gsm-secrets/execution.gopkg/secrets/client.gocmd/ci-secret-generator/main.gopkg/gsm-secrets/secrets.gopkg/secrets/vault.gopkg/secrets/gsm.gocmd/ci-secret-generator/main_test.go
🧬 Code graph analysis (5)
pkg/gsm-secrets/execution.go (1)
pkg/gsm-secrets/secrets.go (1)
ConstructIndexSecretContent(87-97)
cmd/ci-secret-generator/main.go (3)
pkg/secrets/client.go (1)
Client(17-22)pkg/gsm-validation/gsm.go (1)
NormalizeName(91-97)pkg/gsm-secrets/secrets.go (1)
ConstructIndexSecretContent(87-97)
pkg/gsm-secrets/secrets.go (1)
pkg/gsm-secrets/types.go (1)
UpdaterSASecretName(20-20)
pkg/secrets/gsm.go (4)
pkg/secrets/client.go (1)
Client(17-22)pkg/gsm-validation/gsm.go (1)
NormalizeName(91-97)pkg/gsm-secrets/types.go (2)
GetGSMSecretName(219-228)GetIndexSecretName(204-206)pkg/gsm-secrets/execution.go (1)
CreateOrUpdateSecret(31-64)
cmd/ci-secret-generator/main_test.go (3)
pkg/gsm-secrets/secrets.go (1)
ConstructIndexSecretContent(87-97)pkg/api/types.go (1)
Cluster(749-749)pkg/secrets/client_mock.go (1)
NewMockClient(119-123)
🔇 Additional comments (11)
pkg/gsm-secrets/execution.go (1)
257-260: LGTM!The refactoring to use
ConstructIndexSecretContentis correct. For newly created index secrets (where the secrets list is empty), this produces the expected content with just the updater service account entry.pkg/secrets/client.go (1)
17-22: LGTM!The new
UpdateIndexSecretmethod is a clean extension to theClientinterface. The signature aligns with existing methods and clearly expresses its purpose for GSM index secret synchronization.pkg/secrets/vault.go (1)
137-140: LGTM!The no-op implementation with the explanatory comment is appropriate. Index secrets are GSM-specific, and the actual work is delegated to the GSM sync decorator when enabled.
cmd/ci-secret-generator/main.go (3)
244-248: LGTM!The normalization and accumulation logic correctly builds GSM-compatible secret identifiers. The format
{normalizedGroup}__{normalizedField}aligns with the GSM naming convention.
215-217: LGTM!The function signature extension with
GSMsyncEnabledfollows a clean feature-flag pattern, and the localallSecretsListaccumulator is appropriately scoped.
266-271: No actionable issues found. The hardcodedsecrets.TestPlatformCollectionis the correct, intended design—all ci-secret-generator secrets belong to the test-platform-infra collection by design, with individual secrets differentiated by group and field names. The index secret correctly aggregates all generated secrets into a single unified index under this collection. Tests validate this behavior.cmd/ci-secret-generator/main_test.go (3)
10-16: LGTM!The new imports for gomock and GSM secrets package are appropriate for the new test functionality.
198-198: LGTM!The existing test correctly passes
falsefor the newgsmSyncEnabledparameter, preserving the original test behavior.
595-748: LGTM!The error test cases comprehensively cover:
- Index update failures being aggregated
- Field upload failures with proper index content verification
- Partial failures where only successful fields appear in the index
- Multiple items with aggregated errors
The test structure with
partialFailureConfigandexpectedIndexFieldsprovides good granularity for verifying partial failure behavior.pkg/secrets/gsm.go (2)
15-17: LGTM!The constant provides a single source of truth for the collection name.
47-77: LGTM!The documentation clearly explains the 3-level GSM hierarchy with a concrete example. Using
Debuglevel for per-secret success messages is appropriate given the ~460 secrets being synced.
| // UpdateIndexSecret creates or updates the index secret for a GSM collection. | ||
| // The index secret tracks all field names within a collection. | ||
| // | ||
| // Parameters: | ||
| // - itemName: The GSM collection name (e.g., "test-platform-infra") | ||
| // - payload: JSON array of all field names in the collection | ||
| // | ||
| // Creates GSM secret named: {itemName}____index | ||
| func (g *gsmSyncDecorator) UpdateIndexSecret(itemName string, payload []byte) error { | ||
| annotations := make(map[string]string) | ||
| annotations["request-information"] = "Created by periodic-ci-secret-generator." | ||
| if err := gsm.CreateOrUpdateSecret(g.ctx, g.gsmClient, g.config.ProjectIdNumber, gsm.GetIndexSecretName(itemName), payload, nil, annotations); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Fix documentation: payload is YAML-formatted list, not JSON.
The comment on line 87 states "payload: JSON array" but ConstructIndexSecretContent (from pkg/gsm-secrets/secrets.go) returns a newline-separated YAML list format (- secret1\n- secret2), not JSON.
📝 Suggested fix
// UpdateIndexSecret creates or updates the index secret for a GSM collection.
// The index secret tracks all field names within a collection.
//
// Parameters:
// - itemName: The GSM collection name (e.g., "test-platform-infra")
-// - payload: JSON array of all field names in the collection
+// - payload: YAML-formatted list of all field names in the collection (e.g., "- secret1\n- secret2")
//
// Creates GSM secret named: {itemName}____indexAlso consider: SetFieldOnItem sets the "jira-project": "dptp" label, but UpdateIndexSecret passes nil for labels. If this is intentional, a brief comment explaining the difference would help future maintainers.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // UpdateIndexSecret creates or updates the index secret for a GSM collection. | |
| // The index secret tracks all field names within a collection. | |
| // | |
| // Parameters: | |
| // - itemName: The GSM collection name (e.g., "test-platform-infra") | |
| // - payload: JSON array of all field names in the collection | |
| // | |
| // Creates GSM secret named: {itemName}____index | |
| func (g *gsmSyncDecorator) UpdateIndexSecret(itemName string, payload []byte) error { | |
| annotations := make(map[string]string) | |
| annotations["request-information"] = "Created by periodic-ci-secret-generator." | |
| if err := gsm.CreateOrUpdateSecret(g.ctx, g.gsmClient, g.config.ProjectIdNumber, gsm.GetIndexSecretName(itemName), payload, nil, annotations); err != nil { | |
| return err | |
| } | |
| return nil | |
| } | |
| // UpdateIndexSecret creates or updates the index secret for a GSM collection. | |
| // The index secret tracks all field names within a collection. | |
| // | |
| // Parameters: | |
| // - itemName: The GSM collection name (e.g., "test-platform-infra") | |
| // - payload: YAML-formatted list of all field names in the collection (e.g., "- secret1\n- secret2") | |
| // | |
| // Creates GSM secret named: {itemName}____index | |
| func (g *gsmSyncDecorator) UpdateIndexSecret(itemName string, payload []byte) error { | |
| annotations := make(map[string]string) | |
| annotations["request-information"] = "Created by periodic-ci-secret-generator." | |
| if err := gsm.CreateOrUpdateSecret(g.ctx, g.gsmClient, g.config.ProjectIdNumber, gsm.GetIndexSecretName(itemName), payload, nil, annotations); err != nil { | |
| return err | |
| } | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In @pkg/secrets/gsm.go around lines 82 - 97, The doc comment for
UpdateIndexSecret is incorrect: payload is a YAML-formatted newline list (from
ConstructIndexSecretContent), not a JSON array; update the comment to say
"payload: YAML newline-separated list (e.g., '- secret1\n- secret2')" and
clarify the created secret name description remains the same; additionally, if
the omission of labels is intentional, add a brief inline comment in
UpdateIndexSecret noting that labels are intentionally nil (unlike
SetFieldOnItem which sets "jira-project": "dptp") or modify the call to pass
labels if it was a mistake so the behavior is explicit.
b94f96a to
4b51cdc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
cmd/ci-secret-generator/main_test.go (2)
582-585: Consider using a field in the test case struct instead of string matching.The disabled clusters set is conditionally applied based on test name string comparison, which is fragile if the test name changes.
♻️ Suggested improvement
Add a
disabledClustersfield to the test case struct:testCases := []struct { name string config secretgenerator.Config GSMsyncEnabled bool expectedIndexCalls int verifyIndexPayload func(t *testing.T, itemName string, payload []byte) + disabledClusters sets.Set[string] }{Then use it directly:
- var disabledClusters sets.Set[string] - if tc.name == "GSM sync enabled - disabled cluster field excluded from index" { - disabledClusters = sets.New[string]("disabled-cluster") - } - - err := updateSecrets(tc.config, mockClient, disabledClusters, tc.GSMsyncEnabled) + err := updateSecrets(tc.config, mockClient, tc.disabledClusters, tc.GSMsyncEnabled)
603-606:expectedErrorsLenfield is defined but not verified.The test cases define
expectedErrorsLenbut the assertion only checks that an error exists, not the number of aggregated errors.♻️ Suggested verification
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 count if using aggregate errors + if agg, ok := err.(interface{ Errors() []error }); ok { + if len(agg.Errors()) != tc.expectedErrorsLen { + t.Errorf("expected %d errors, got %d", tc.expectedErrorsLen, len(agg.Errors())) + } }Also applies to: 738-745
pkg/secrets/gsm.go (2)
72-77: GSM sync failures are logged but don't propagate errors.The design intentionally prevents GSM failures from blocking Vault writes. However, at scale (~460 secrets), silent GSM failures could accumulate unnoticed. Consider whether metrics or alerting should be added for GSM sync failure rates.
90-97: Consider adding thejira-projectlabel for consistency.
SetFieldOnItemsetslabels["jira-project"] = "dptp"butUpdateIndexSecretpassesnilfor labels. For consistency in GSM secret management and discoverability, consider adding the same label.♻️ Suggested fix
func (g *gsmSyncDecorator) UpdateIndexSecret(itemName string, payload []byte) error { + labels := make(map[string]string) + labels["jira-project"] = "dptp" + annotations := make(map[string]string) annotations["request-information"] = "Created by periodic-ci-secret-generator." - if err := gsm.CreateOrUpdateSecret(g.ctx, g.gsmClient, g.config.ProjectIdNumber, gsm.GetIndexSecretName(itemName), payload, nil, annotations); err != nil { + if err := gsm.CreateOrUpdateSecret(g.ctx, g.gsmClient, g.config.ProjectIdNumber, gsm.GetIndexSecretName(itemName), payload, labels, annotations); err != nil { return err } return nil }
📜 Review details
Configuration used: Organization 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 (3)
cmd/ci-secret-generator/main.gocmd/ci-secret-generator/main_test.gopkg/secrets/gsm.go
🧰 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/gsm.gocmd/ci-secret-generator/main.go
🧬 Code graph analysis (3)
cmd/ci-secret-generator/main_test.go (4)
pkg/secrets/gsm.go (1)
TestPlatformCollection(16-16)pkg/gsm-secrets/secrets.go (1)
ConstructIndexSecretContent(87-97)pkg/api/types.go (1)
Cluster(749-749)pkg/secrets/client_mock.go (1)
NewMockClient(119-123)
pkg/secrets/gsm.go (4)
pkg/secrets/client.go (1)
Client(17-22)pkg/gsm-validation/gsm.go (1)
NormalizeName(91-97)pkg/gsm-secrets/types.go (2)
GetGSMSecretName(219-228)GetIndexSecretName(204-206)pkg/gsm-secrets/execution.go (1)
CreateOrUpdateSecret(31-64)
cmd/ci-secret-generator/main.go (4)
pkg/secrets/client.go (1)
Client(17-22)pkg/gsm-validation/gsm.go (1)
NormalizeName(91-97)pkg/secrets/gsm.go (1)
TestPlatformCollection(16-16)pkg/gsm-secrets/secrets.go (1)
ConstructIndexSecretContent(87-97)
🔇 Additional comments (11)
cmd/ci-secret-generator/main.go (5)
22-22: LGTM!Import addition for
gsmvalidationaligns with the normalization requirements for GSM secret naming.
215-217: LGTM!Function signature extension and index list initialization are correctly implemented.
244-248: LGTM!Secret names are correctly accumulated only after successful field uploads, ensuring the index reflects only successfully synced secrets.
266-271: LGTM!The index secret update is correctly placed after all field processing, with proper error aggregation.
343-345: LGTM!Call site correctly passes the GSM sync flag to
updateSecrets.cmd/ci-secret-generator/main_test.go (4)
10-10: LGTM!Import additions for gomock and gsm package are appropriate for the new GSM index tests.
Also applies to: 16-16
198-200: LGTM!Existing test correctly updated to pass
falsefor the new GSM sync parameter, preserving original test behavior.
441-593: Good test coverage for GSM index functionality.The test cases comprehensively cover single/multiple fields, multiple items, GSM disabled path, and disabled cluster exclusion scenarios.
595-748: Good error scenario coverage.The tests appropriately cover UpdateIndexSecret failures, SetFieldOnItem failures, and partial failure scenarios where only successful fields should appear in the index.
pkg/secrets/gsm.go (2)
15-17: LGTM!Exported constant for the collection name provides a single source of truth for the GSM hierarchy root.
82-97: LGTM!The
UpdateIndexSecretimplementation correctly creates/updates the index secret and appropriately propagates errors (unlike individual field syncs which are non-blocking).
|
/test integration |
|
Scheduling required tests: Scheduling tests matching the |
|
@psalajova: The following tests 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. |
|
/test e2e |
|
@droslean could you pls check out the latest changes? Ty! |
4 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.