Skip to content

ci-secret-generator: Sync *all* secrets to GSM #4809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
psalajova wants to merge 5 commits into
base: main
Choose a base branch
from
Open

ci-secret-generator: Sync *all* secrets to GSM #4809

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
30156fd
Add func to construct index secret payload
psalajova Nov 5, 2025
72a2538
Edit client so that *all* generated secrets are synced to GSM
psalajova Nov 5, 2025
8556f71
Add method to update index secret
psalajova Nov 5, 2025
9b67316
Add tests
psalajova Nov 10, 2025
435ae02
Add parallel testing also within child tests
psalajova Nov 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions cmd/ci-secret-generator/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,11 @@ func fmtExecCmdErr(action, cmd string, wrappedErr error, stdout, stderr []byte,
stdout, stderrPreamble, stderr)
}

func updateSecrets(config secretgenerator.Config, client secrets.Client, disabledClusters sets.Set[string]) error {
func updateSecrets(config secretgenerator.Config, client secrets.Client, disabledClusters sets.Set[string], GSMsyncEnabled bool) error {
var errs []error
for _, item := range config {
logger := logrus.WithField("item", item.ItemName)
var secretsList []string
for _, field := range item.Fields {
logger = logger.WithFields(logrus.Fields{
"field": field.Name,
Expand All @@ -239,6 +240,17 @@ func updateSecrets(config secretgenerator.Config, client secrets.Client, disable
errs = append(errs, errors.New(msg))
continue
}
if GSMsyncEnabled {
secretsList = append(secretsList, gsm.NormalizeSecretName(field.Name))
}
}

if GSMsyncEnabled {
indexPayload := gsm.ConstructIndexSecretContent(secretsList)
err := client.UpdateIndexSecret(gsm.GetIndexSecretName(item.ItemName), indexPayload)
if err != nil {
errs = append(errs, err)
}
}

// Adding the notes not empty check here since we dont want to overwrite any notes that might already be present
Expand Down Expand Up @@ -326,7 +338,7 @@ func generateSecrets(o options, censor *secrets.DynamicCensor) (errs []error) {
}
}

if err := updateSecrets(o.config, client, o.disabledClusters); err != nil {
if err := updateSecrets(o.config, client, o.disabledClusters, o.enableGsmSync); err != nil {
errs = append(errs, fmt.Errorf("failed to update secrets: %w", err))
}

Expand Down
321 changes: 320 additions & 1 deletion cmd/ci-secret-generator/main_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
"go.uber.org/mock/gomock"

"k8s.io/apimachinery/pkg/util/sets"

"github.com/openshift/ci-tools/pkg/api/secretbootstrap"
"github.com/openshift/ci-tools/pkg/api/secretgenerator"
gsm "github.com/openshift/ci-tools/pkg/gsm-secrets"
"github.com/openshift/ci-tools/pkg/secrets"
"github.com/openshift/ci-tools/pkg/testhelper"
"github.com/openshift/ci-tools/pkg/vaultclient"
Expand Down Expand Up @@ -193,7 +195,7 @@ func TestVault(t *testing.T) {
}
}
}()
if err := updateSecrets(tc.config, client, tc.disabledClusters); err != nil {
if err := updateSecrets(tc.config, client, tc.disabledClusters, false); err != nil {
t.Errorf("failed to update secrets: %v", err)
}
list, err := vault.ListKV("secret")
Expand Down Expand Up @@ -435,3 +437,320 @@ func TestValidateConfig(t *testing.T) {
})
}
}

func TestUpdateSecretsWithGSMIndex(t *testing.T) {
t.Parallel()
Copy link
Contributor

@danilo-gemoli danilo-gemoli Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@psalajova psalajova Nov 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@danilo-gemoli danilo-gemoli Nov 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@psalajova psalajova Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added the parallelism within the subtests in the newest commit


testCases := []struct {
name string
config secretgenerator.Config
GSMsyncEnabled bool
expectedIndexCalls int
verifyIndexPayload func(t *testing.T, itemName string, payload []byte)
}{
{
name: "GSM sync enabled - single field creates index secret",
config: secretgenerator.Config{{
ItemName: "test-item",
Fields: []secretgenerator.FieldGenerator{
{Name: "field1", Cmd: "printf 'value1'"},
},
}},
GSMsyncEnabled: true,
expectedIndexCalls: 1,
verifyIndexPayload: func(t *testing.T, itemName string, payload []byte) {
expectedItemName := gsm.GetIndexSecretName("test-item")
if itemName != expectedItemName {
t.Errorf("expected item name %q, got %q", expectedItemName, itemName)
}
expectedPayload := string(gsm.ConstructIndexSecretContent([]string{"field1"}))
if string(payload) != expectedPayload {
t.Errorf("expected payload %q, got %q", expectedPayload, string(payload))
}
},
},
{
name: "GSM sync enabled - multiple fields create index with all fields",
config: secretgenerator.Config{{
ItemName: "multi-field-item",
Fields: []secretgenerator.FieldGenerator{
{Name: "field1", Cmd: "printf 'value1'"},
{Name: "field2", Cmd: "printf 'value2'"},
{Name: "field3", Cmd: "printf 'value3'"},
},
}},
GSMsyncEnabled: true,
expectedIndexCalls: 1,
verifyIndexPayload: func(t *testing.T, itemName string, payload []byte) {
expectedItemName := gsm.GetIndexSecretName("multi-field-item")
if itemName != expectedItemName {
t.Errorf("expected item name %q, got %q", expectedItemName, itemName)
}
expectedPayload := string(gsm.ConstructIndexSecretContent([]string{"field1", "field2", "field3"}))
if string(payload) != expectedPayload {
t.Errorf("expected payload %q, got %q", expectedPayload, string(payload))
}
},
},
{
name: "GSM sync enabled - multiple items create multiple index secrets",
config: secretgenerator.Config{
{
ItemName: "item1",
Fields: []secretgenerator.FieldGenerator{
{Name: "field1", Cmd: "printf 'value1'"},
},
},
{
ItemName: "item2",
Fields: []secretgenerator.FieldGenerator{
{Name: "fieldA", Cmd: "printf 'valueA'"},
{Name: "fieldB", Cmd: "printf 'valueB'"},
},
},
},
GSMsyncEnabled: true,
expectedIndexCalls: 2,
verifyIndexPayload: func(t *testing.T, itemName string, payload []byte) {
// This will be called twice, once for each item
if itemName == gsm.GetIndexSecretName("item1") {
expectedPayload := string(gsm.ConstructIndexSecretContent([]string{"field1"}))
if string(payload) != expectedPayload {
t.Errorf("expected payload for item1 %q, got %q", expectedPayload, string(payload))
}
} else if itemName == gsm.GetIndexSecretName("item2") {
expectedPayload := string(gsm.ConstructIndexSecretContent([]string{"fieldA", "fieldB"}))
if string(payload) != expectedPayload {
t.Errorf("expected payload for item2 %q, got %q", expectedPayload, string(payload))
}
} else {
t.Errorf("unexpected item name: %q", itemName)
}
},
},
{
name: "GSM sync disabled - no index secret created",
config: secretgenerator.Config{{
ItemName: "test-item",
Fields: []secretgenerator.FieldGenerator{
{Name: "field1", Cmd: "printf 'value1'"},
{Name: "field2", Cmd: "printf 'value2'"},
},
}},
GSMsyncEnabled: false,
expectedIndexCalls: 0,
},
{
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,
expectedIndexCalls: 1,
verifyIndexPayload: func(t *testing.T, itemName string, payload []byte) {
// Only field1 and field3 should be in the index (field2 is from disabled cluster)
expectedPayload := string(gsm.ConstructIndexSecretContent([]string{"field1", "field3"}))
if string(payload) != expectedPayload {
t.Errorf("expected payload %q, got %q", expectedPayload, string(payload))
}
},
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

mockClient := secrets.NewMockClient(mockCtrl)
mockClient.EXPECT().
SetFieldOnItem(gomock.Any(), gomock.Any(), gomock.Any()).
AnyTimes().
Return(nil)

// set expectations for UpdateIndexSecret based on test case
if tc.expectedIndexCalls > 0 {
mockClient.EXPECT().
UpdateIndexSecret(gomock.Any(), gomock.Any()).
Times(tc.expectedIndexCalls).
DoAndReturn(func(itemName string, payload []byte) error {
if tc.verifyIndexPayload != nil {
tc.verifyIndexPayload(t, itemName, payload)
}
return nil
})
}

// 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)
if err != nil {
t.Errorf("unexpected error: %v", err)
}
})
}
}

func TestUpdateSecretsWithGSMIndexErrors(t *testing.T) {
t.Parallel()

testCases := []struct {
name string
config secretgenerator.Config
setFieldError error
updateIndexError error
expectedErrorsLen int
partialFailure bool
partialFailureConfig map[string]error // field name -> error (nil = success)
expectedIndexFields []string // fields that should appear in index for partial failure
}{
{
name: "UpdateIndexSecret error is aggregated",
config: secretgenerator.Config{{
ItemName: "test-item",
Fields: []secretgenerator.FieldGenerator{
{Name: "field1", Cmd: "printf 'value1'"},
},
}},
updateIndexError: errors.New("GSM update failed"),
expectedErrorsLen: 1,
},
{
name: "SetFieldOnItem error - all fields fail, index contains only updater-service-account",
config: secretgenerator.Config{{
ItemName: "test-item",
Fields: []secretgenerator.FieldGenerator{
{Name: "field1", Cmd: "printf 'value1'"},
{Name: "field2", Cmd: "printf 'value2'"},
},
}},
setFieldError: errors.New("field upload failed"),
updateIndexError: nil, // Index update should still be called
expectedErrorsLen: 2, // Both field errors
},
{
name: "SetFieldOnItem partial failure - only successful fields in index",
config: secretgenerator.Config{{
ItemName: "test-item",
Fields: []secretgenerator.FieldGenerator{
{Name: "field1", Cmd: "printf 'value1'"},
{Name: "field2", Cmd: "printf 'value2'"},
{Name: "field3", Cmd: "printf 'value3'"},
},
}},
partialFailure: true,
partialFailureConfig: map[string]error{
"field1": nil, // succeeds
"field2": errors.New("field2 upload failed"), // fails
"field3": nil, // succeeds
},
expectedIndexFields: []string{"field1", "field3"}, // only successful fields
expectedErrorsLen: 1, // only field2 error
},
{
name: "Multiple items - index errors are aggregated",
config: secretgenerator.Config{
{
ItemName: "item1",
Fields: []secretgenerator.FieldGenerator{
{Name: "field1", Cmd: "printf 'value1'"},
},
},
{
ItemName: "item2",
Fields: []secretgenerator.FieldGenerator{
{Name: "field2", Cmd: "printf 'value2'"},
},
},
},
updateIndexError: errors.New("GSM update failed"),
expectedErrorsLen: 2, // One error for each item's index update
},
}

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
mockCtrl := gomock.NewController(t)
defer mockCtrl.Finish()

mockClient := secrets.NewMockClient(mockCtrl)

if tc.partialFailure {
// For partial failure, return different errors based on field name
mockClient.EXPECT().
SetFieldOnItem(gomock.Any(), gomock.Any(), gomock.Any()).
AnyTimes().
DoAndReturn(func(itemName, fieldName string, fieldValue []byte) error {
if err, exists := tc.partialFailureConfig[fieldName]; exists {
return err
}
return nil
})
} else if tc.setFieldError != nil {
mockClient.EXPECT().
SetFieldOnItem(gomock.Any(), gomock.Any(), gomock.Any()).
AnyTimes().
Return(tc.setFieldError)
} else {
mockClient.EXPECT().
SetFieldOnItem(gomock.Any(), gomock.Any(), gomock.Any()).
AnyTimes().
Return(nil)
}

// Set expectations for UpdateIndexSecret - it should always be called in these tests
if tc.updateIndexError != nil {
mockClient.EXPECT().
UpdateIndexSecret(gomock.Any(), gomock.Any()).
AnyTimes().
Return(tc.updateIndexError)
} else {
// Verify that when SetFieldOnItem fails, the index only contains successful fields
if tc.setFieldError != nil || tc.partialFailure {
mockClient.EXPECT().
UpdateIndexSecret(gomock.Any(), gomock.Any()).
AnyTimes().
DoAndReturn(func(itemName string, payload []byte) error {
var expectedPayload string
if tc.partialFailure {
// For partial failure, index should contain only successful fields
expectedPayload = string(gsm.ConstructIndexSecretContent(tc.expectedIndexFields))
} else {
// When all fields fail, index should only contain updater-service-account
expectedPayload = string(gsm.ConstructIndexSecretContent([]string{}))
}
if string(payload) != expectedPayload {
t.Errorf("expected index payload %q, got %q", expectedPayload, string(payload))
}
return nil
})
} else {
mockClient.EXPECT().
UpdateIndexSecret(gomock.Any(), gomock.Any()).
AnyTimes().
Return(nil)
}
}

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")
}
})
}
}
2 changes: 1 addition & 1 deletion pkg/gsm-secrets/execution.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ func (a *Actions) CreateSecrets(ctx context.Context, secretsClient SecretManager
}

if s.Type == SecretTypeIndex {
s.Payload = fmt.Appendf(nil, "- updater-service-account")
s.Payload = ConstructIndexSecretContent([]string{})
a.SecretsToCreate[name] = s
}

Expand Down
Loading