From 9725289e4ec30a8159dc1ea9603eaef9a5e09164 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Wed, 3 Sep 2025 08:30:01 -0500 Subject: [PATCH 1/2] Fix unnecessary secret updates and logging in STS flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The operator was repeatedly logging "Secret already exists, updating" and "Following standardized STS workflow, secret created successfully" even when the secret content hadn't changed. This was happening because the CloudStorage controller calls STSStandardizedFlow() on every reconciliation, which always attempted to create the secret first, then caught the AlreadyExists error and performed an update. Changed the approach to: - First check if the secret exists - Compare existing data with desired data - Only update when there are actual differences - Skip updates and avoid logging when content is identical - Changed CloudStorage controller to use Debug level and more accurate message when STS secret is available (not necessarily created) This eliminates unnecessary API calls to the Kubernetes cluster and reduces noise in the operator logs. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- .../controller/cloudstorage_controller.go | 4 +- pkg/credentials/stsflow/stsflow.go | 94 +++++++++++++------ pkg/credentials/stsflow/stsflow_test.go | 64 ++++++++++--- 3 files changed, 120 insertions(+), 42 deletions(-) diff --git a/internal/controller/cloudstorage_controller.go b/internal/controller/cloudstorage_controller.go index 1c5fa32f643..d29fc670092 100644 --- a/internal/controller/cloudstorage_controller.go +++ b/internal/controller/cloudstorage_controller.go @@ -132,8 +132,8 @@ func (b CloudStorageReconciler) Reconcile(ctx context.Context, req ctrl.Request) return ctrl.Result{RequeueAfter: 30 * time.Second}, nil } if secretName != "" { - // Secret was created successfully by STSStandardizedFlow - logger.Info(fmt.Sprintf("Following standardized STS workflow, secret %s created successfully", secretName)) + // Secret exists after STSStandardizedFlow (may have been created, updated, or unchanged) + logger.V(1).Info(fmt.Sprintf("Following standardized STS workflow, secret %s is available", secretName)) } // Now continue with bucket creation as secret exists and we are good to go !!! if ok, err = clnt.Exists(); !ok && err == nil { diff --git a/pkg/credentials/stsflow/stsflow.go b/pkg/credentials/stsflow/stsflow.go index a3c63f16b7d..eba519ebbb6 100644 --- a/pkg/credentials/stsflow/stsflow.go +++ b/pkg/credentials/stsflow/stsflow.go @@ -216,7 +216,7 @@ func CreateOrUpdateSTSSecretWithClients(setupLog logr.Logger, secretName string, func CreateOrUpdateSTSSecretWithClientsAndWait(setupLog logr.Logger, secretName string, credStringData map[string]string, secretNS string, clientInstance client.Client, clientset kubernetes.Interface, waitForSecret bool) error { // Create a secret with the appropriate credentials format for STS/WIF authentication // Secret format follows standard patterns used by cloud providers - secret := corev1.Secret{ + desiredSecret := corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: secretName, Namespace: secretNS, @@ -226,48 +226,88 @@ func CreateOrUpdateSTSSecretWithClientsAndWait(setupLog logr.Logger, secretName }, StringData: credStringData, } + + // First, try to get the existing secret + existingSecret := corev1.Secret{} + err := clientInstance.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: secretNS}, &existingSecret) + verb := "created" - if err := clientInstance.Create(context.Background(), &secret); err != nil { - if errors.IsAlreadyExists(err) { - verb = "updated" - setupLog.Info("Secret already exists, updating") - fromCluster := corev1.Secret{} - err = clientInstance.Get(context.Background(), types.NamespacedName{Name: secret.Name, Namespace: secret.Namespace}, &fromCluster) - if err != nil { - setupLog.Error(err, "unable to get existing secret resource") + if err != nil { + if errors.IsNotFound(err) { + // Secret doesn't exist, create it + if err := clientInstance.Create(context.Background(), &desiredSecret); err != nil { + setupLog.Error(err, "unable to create secret resource") return err } - // update StringData - preserve existing Data that's not being replaced - // This is safe because STS credentials are only updated during install/reconfiguration, - // and any BSL-specific patches (like region) should be preserved - updatedFromCluster := fromCluster.DeepCopy() + } else { + // Some other error occurred while getting the secret + setupLog.Error(err, "unable to get secret resource") + return err + } + } else { + // Secret exists, check if update is needed + needsUpdate := false + + // Check if labels need updating + if existingSecret.Labels == nil || existingSecret.Labels["oadp.openshift.io/secret-type"] != "sts-credentials" { + needsUpdate = true + } + + // Check if data needs updating + // Convert existing Data to string for comparison + existingData := make(map[string]string) + for key, value := range existingSecret.Data { + existingData[key] = string(value) + } + + // Compare each key in credStringData + for key, desiredValue := range credStringData { + if existingValue, exists := existingData[key]; !exists || existingValue != desiredValue { + needsUpdate = true + break + } + } + + if needsUpdate { + verb = "updated" + setupLog.Info("Secret content differs, updating") + + // Update the secret + updatedSecret := existingSecret.DeepCopy() + // Initialize StringData if not present - if updatedFromCluster.StringData == nil { - updatedFromCluster.StringData = make(map[string]string) + if updatedSecret.StringData == nil { + updatedSecret.StringData = make(map[string]string) } + // Update only the new StringData fields, preserving existing Data - for key, value := range secret.StringData { - updatedFromCluster.StringData[key] = value + for key, value := range credStringData { + updatedSecret.StringData[key] = value } + // Ensure labels are set - if updatedFromCluster.Labels == nil { - updatedFromCluster.Labels = make(map[string]string) + if updatedSecret.Labels == nil { + updatedSecret.Labels = make(map[string]string) } - updatedFromCluster.Labels["oadp.openshift.io/secret-type"] = "sts-credentials" - if err := clientInstance.Patch(context.Background(), updatedFromCluster, client.MergeFrom(&fromCluster)); err != nil { + updatedSecret.Labels["oadp.openshift.io/secret-type"] = "sts-credentials" + + if err := clientInstance.Patch(context.Background(), updatedSecret, client.MergeFrom(&existingSecret)); err != nil { setupLog.Error(err, fmt.Sprintf("unable to update secret resource: %v", err)) return err } } else { - setupLog.Error(err, "unable to create secret resource") - return err + // No update needed + verb = "unchanged" } } - setupLog.Info("Secret " + secret.Name + " " + verb + " successfully") - if waitForSecret { - // Wait for the Secret to be available - setupLog.Info(fmt.Sprintf("Waiting for %s Secret to be available", secret.Name)) + if verb != "unchanged" { + setupLog.Info("Secret " + desiredSecret.Name + " " + verb + " successfully") + } + + if waitForSecret && verb == "created" { + // Wait for the Secret to be available (only needed for newly created secrets) + setupLog.Info(fmt.Sprintf("Waiting for %s Secret to be available", desiredSecret.Name)) _, err := WaitForSecret(clientset, secretNS, secretName) if err != nil { setupLog.Error(err, "error waiting for credentials Secret") diff --git a/pkg/credentials/stsflow/stsflow_test.go b/pkg/credentials/stsflow/stsflow_test.go index 599567813e3..50f4f5f99ec 100644 --- a/pkg/credentials/stsflow/stsflow_test.go +++ b/pkg/credentials/stsflow/stsflow_test.go @@ -284,18 +284,11 @@ func TestCreateOrUpdateSTSSecret_ErrorScenarios(t *testing.T) { testSecretName := "test-secret" testLogger := zap.New(zap.UseDevMode(true)) - t.Run("Get error during update", func(t *testing.T) { - // Create a client that returns an error on Get - fakeClient := &mockErrorClient{ - Client: fake.NewClientBuilder(). - WithRuntimeObjects(&corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: testSecretName, - Namespace: testNamespace, - }, - }). - Build(), - getError: true, + t.Run("Get error (non-NotFound) during initial check", func(t *testing.T) { + // Create a client that returns a non-NotFound error on Get + // This simulates a real error (not just secret not existing) + fakeClient := &mockErrorClientGenericGetError{ + Client: fake.NewClientBuilder().Build(), } fakeClientset := k8sfake.NewSimpleClientset() @@ -304,7 +297,7 @@ func TestCreateOrUpdateSTSSecret_ErrorScenarios(t *testing.T) { }, testNamespace, fakeClient, fakeClientset, false) assert.Error(t, err) - assert.Contains(t, err.Error(), "not found") + assert.Contains(t, err.Error(), "unable to get secret resource") }) t.Run("Patch error during update", func(t *testing.T) { @@ -329,6 +322,42 @@ func TestCreateOrUpdateSTSSecret_ErrorScenarios(t *testing.T) { assert.Error(t, err) assert.Contains(t, err.Error(), "patch error") }) + + t.Run("No update when content is identical", func(t *testing.T) { + // Create a secret with the same data we'll try to update with + existingSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testSecretName, + Namespace: testNamespace, + Labels: map[string]string{ + "oadp.openshift.io/secret-type": "sts-credentials", + }, + }, + Data: map[string][]byte{ + "key": []byte("value"), + }, + } + fakeClient := fake.NewClientBuilder(). + WithRuntimeObjects(existingSecret). + Build() + fakeClientset := k8sfake.NewSimpleClientset() + + // Try to update with the same content + err := CreateOrUpdateSTSSecretWithClientsAndWait(testLogger, testSecretName, map[string]string{ + "key": "value", + }, testNamespace, fakeClient, fakeClientset, false) + + assert.NoError(t, err) + // Verify the secret wasn't modified + secretResult := &corev1.Secret{} + err = fakeClient.Get(context.Background(), client.ObjectKey{ + Name: testSecretName, + Namespace: testNamespace, + }, secretResult) + assert.NoError(t, err) + // The Data should remain unchanged (no StringData should be set) + assert.Equal(t, []byte("value"), secretResult.Data["key"]) + }) } // Mock client that can simulate errors @@ -357,6 +386,15 @@ func (m *mockErrorClient) Patch(ctx context.Context, obj client.Object, patch cl return m.Client.Patch(ctx, obj, patch, opts...) } +// New mock client that returns a generic error on Get (not NotFound) +type mockErrorClientGenericGetError struct { + client.Client +} + +func (m *mockErrorClientGenericGetError) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + return errors.NewServiceUnavailable("unable to get secret resource") +} + func TestSTSStandardizedFlow(t *testing.T) { // Save original env values originalWatchNS := os.Getenv("WATCH_NAMESPACE") From 03d6a9c17901a7a27151e507c8f079bc572194c2 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Fri, 5 Sep 2025 13:28:36 -0500 Subject: [PATCH 2/2] refactor: Use constants for STS secret labels and error messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace hardcoded strings with constants from stsflow package: - Add constants for secret operation verbs (created, updated, unchanged) - Add constants for STS secret label key/value - Add constants for error messages - Update all files using "oadp.openshift.io/secret-type" to use STSSecretLabelKey - Update test files to use the new constants This improves maintainability and reduces risk of typos in label names and error messages across the codebase. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude --- internal/controller/bsl_test.go | 15 ++++++----- pkg/bucket/azure.go | 3 ++- pkg/bucket/azure_test.go | 5 ++-- pkg/credentials/stsflow/stsflow.go | 36 +++++++++++++++++-------- pkg/credentials/stsflow/stsflow_test.go | 4 +-- 5 files changed, 40 insertions(+), 23 deletions(-) diff --git a/internal/controller/bsl_test.go b/internal/controller/bsl_test.go index 14fa0774ef5..dc8b4a39ac4 100644 --- a/internal/controller/bsl_test.go +++ b/internal/controller/bsl_test.go @@ -23,6 +23,7 @@ import ( oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" "github.com/openshift/oadp-operator/pkg/common" + "github.com/openshift/oadp-operator/pkg/credentials/stsflow" ) // A bucket that region can be automatically discovered @@ -3457,7 +3458,7 @@ func TestPatchSecretsForBSL(t *testing.T) { Name: "aws-secret", Namespace: "test-ns", Labels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + stsflow.STSSecretLabelKey: stsflow.STSSecretLabelValue, }, }, Data: map[string][]byte{ @@ -3498,7 +3499,7 @@ web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token`), Name: "aws-secret", Namespace: "test-ns", Labels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + stsflow.STSSecretLabelKey: stsflow.STSSecretLabelValue, }, }, Data: map[string][]byte{ @@ -3539,7 +3540,7 @@ web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token`), Name: "aws-secret", Namespace: "test-ns", Labels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + stsflow.STSSecretLabelKey: stsflow.STSSecretLabelValue, }, }, Data: map[string][]byte{ @@ -3579,7 +3580,7 @@ web_identity_token_file = /var/run/secrets/openshift/serviceaccount/token`), Name: "azure-secret", Namespace: "test-ns", Labels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + stsflow.STSSecretLabelKey: stsflow.STSSecretLabelValue, }, }, Data: map[string][]byte{ @@ -3662,7 +3663,7 @@ AZURE_TENANT_ID=test-tenant`), Name: "aws-secret", Namespace: "test-ns", Labels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + stsflow.STSSecretLabelKey: stsflow.STSSecretLabelValue, }, }, Data: map[string][]byte{ @@ -3777,7 +3778,7 @@ aws_secret_access_key=test-secret`), Name: "aws-sts-secret", Namespace: "test-ns", Labels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + stsflow.STSSecretLabelKey: stsflow.STSSecretLabelValue, }, }, Data: map[string][]byte{ @@ -3860,7 +3861,7 @@ aws_secret_access_key=test-secret`), Name: "azure-sts-secret", Namespace: "test-ns", Labels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + stsflow.STSSecretLabelKey: stsflow.STSSecretLabelValue, }, }, Data: map[string][]byte{ diff --git a/pkg/bucket/azure.go b/pkg/bucket/azure.go index 587649d89d4..4010dba04d3 100644 --- a/pkg/bucket/azure.go +++ b/pkg/bucket/azure.go @@ -20,6 +20,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/oadp-operator/api/v1alpha1" + "github.com/openshift/oadp-operator/pkg/credentials/stsflow" ) // azureServiceClient abstracts the Azure blob service client for testing @@ -352,7 +353,7 @@ func (a *azureBucketClient) createAzureClient() (azureServiceClient, error) { // hasWorkloadIdentityCredentials checks if the secret contains workload identity credentials func (a *azureBucketClient) hasWorkloadIdentityCredentials(secret *corev1.Secret) bool { // Check if this is an STS-type secret created by OADP operator - if labels, ok := secret.Labels["oadp.openshift.io/secret-type"]; ok && labels == "sts-credentials" { + if labels, ok := secret.Labels[stsflow.STSSecretLabelKey]; ok && labels == stsflow.STSSecretLabelValue { // For Azure STS secrets, check if it has the azurekey field if azureKey, ok := secret.Data["azurekey"]; ok && len(azureKey) > 0 { // Parse the azurekey to ensure it has the required fields diff --git a/pkg/bucket/azure_test.go b/pkg/bucket/azure_test.go index 0554e19039a..88ef7543209 100644 --- a/pkg/bucket/azure_test.go +++ b/pkg/bucket/azure_test.go @@ -15,6 +15,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" "github.com/openshift/oadp-operator/api/v1alpha1" + "github.com/openshift/oadp-operator/pkg/credentials/stsflow" ) func TestValidateContainerName(t *testing.T) { @@ -295,7 +296,7 @@ AZURE_CLOUD_NAME=AzurePublicCloud `), }, secretLabels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + stsflow.STSSecretLabelKey: stsflow.STSSecretLabelValue, }, expected: true, }, @@ -308,7 +309,7 @@ AZURE_CLOUD_NAME=AzurePublicCloud `), }, secretLabels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + stsflow.STSSecretLabelKey: stsflow.STSSecretLabelValue, }, expected: false, }, diff --git a/pkg/credentials/stsflow/stsflow.go b/pkg/credentials/stsflow/stsflow.go index eba519ebbb6..2bf426223a4 100644 --- a/pkg/credentials/stsflow/stsflow.go +++ b/pkg/credentials/stsflow/stsflow.go @@ -59,6 +59,20 @@ const ( VeleroAWSSecretName = "cloud-credentials" VeleroAzureSecretName = "cloud-credentials-azure" VeleroGCPSecretName = "cloud-credentials-gcp" + + // Secret operation verbs + SecretVerbCreated = "created" + SecretVerbUpdated = "updated" + SecretVerbUnchanged = "unchanged" + + // Label keys and values + STSSecretLabelKey = "oadp.openshift.io/secret-type" + STSSecretLabelValue = "sts-credentials" + + // Error messages + ErrMsgCreateSecret = "unable to create secret resource" + ErrMsgGetSecret = "unable to get secret resource" + ErrMsgUpdateSecret = "unable to update secret resource: %v" ) // STSStandardizedFlow creates secrets for Short Term Service Account Tokens from environment variables for @@ -221,7 +235,7 @@ func CreateOrUpdateSTSSecretWithClientsAndWait(setupLog logr.Logger, secretName Name: secretName, Namespace: secretNS, Labels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + STSSecretLabelKey: STSSecretLabelValue, }, }, StringData: credStringData, @@ -231,17 +245,17 @@ func CreateOrUpdateSTSSecretWithClientsAndWait(setupLog logr.Logger, secretName existingSecret := corev1.Secret{} err := clientInstance.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: secretNS}, &existingSecret) - verb := "created" + verb := SecretVerbCreated if err != nil { if errors.IsNotFound(err) { // Secret doesn't exist, create it if err := clientInstance.Create(context.Background(), &desiredSecret); err != nil { - setupLog.Error(err, "unable to create secret resource") + setupLog.Error(err, ErrMsgCreateSecret) return err } } else { // Some other error occurred while getting the secret - setupLog.Error(err, "unable to get secret resource") + setupLog.Error(err, ErrMsgGetSecret) return err } } else { @@ -249,7 +263,7 @@ func CreateOrUpdateSTSSecretWithClientsAndWait(setupLog logr.Logger, secretName needsUpdate := false // Check if labels need updating - if existingSecret.Labels == nil || existingSecret.Labels["oadp.openshift.io/secret-type"] != "sts-credentials" { + if existingSecret.Labels == nil || existingSecret.Labels[STSSecretLabelKey] != STSSecretLabelValue { needsUpdate = true } @@ -269,7 +283,7 @@ func CreateOrUpdateSTSSecretWithClientsAndWait(setupLog logr.Logger, secretName } if needsUpdate { - verb = "updated" + verb = SecretVerbUpdated setupLog.Info("Secret content differs, updating") // Update the secret @@ -289,23 +303,23 @@ func CreateOrUpdateSTSSecretWithClientsAndWait(setupLog logr.Logger, secretName if updatedSecret.Labels == nil { updatedSecret.Labels = make(map[string]string) } - updatedSecret.Labels["oadp.openshift.io/secret-type"] = "sts-credentials" + updatedSecret.Labels[STSSecretLabelKey] = STSSecretLabelValue if err := clientInstance.Patch(context.Background(), updatedSecret, client.MergeFrom(&existingSecret)); err != nil { - setupLog.Error(err, fmt.Sprintf("unable to update secret resource: %v", err)) + setupLog.Error(err, fmt.Sprintf(ErrMsgUpdateSecret, err)) return err } } else { // No update needed - verb = "unchanged" + verb = SecretVerbUnchanged } } - if verb != "unchanged" { + if verb != SecretVerbUnchanged { setupLog.Info("Secret " + desiredSecret.Name + " " + verb + " successfully") } - if waitForSecret && verb == "created" { + if waitForSecret && verb == SecretVerbCreated { // Wait for the Secret to be available (only needed for newly created secrets) setupLog.Info(fmt.Sprintf("Waiting for %s Secret to be available", desiredSecret.Name)) _, err := WaitForSecret(clientset, secretNS, secretName) diff --git a/pkg/credentials/stsflow/stsflow_test.go b/pkg/credentials/stsflow/stsflow_test.go index 50f4f5f99ec..51b37a361b4 100644 --- a/pkg/credentials/stsflow/stsflow_test.go +++ b/pkg/credentials/stsflow/stsflow_test.go @@ -158,7 +158,7 @@ func TestCreateOrUpdateSTSSecret(t *testing.T) { // Verify the label is set assert.NotNil(t, secret.Labels) - assert.Equal(t, "sts-credentials", secret.Labels["oadp.openshift.io/secret-type"]) + assert.Equal(t, STSSecretLabelValue, secret.Labels[STSSecretLabelKey]) } }) } @@ -330,7 +330,7 @@ func TestCreateOrUpdateSTSSecret_ErrorScenarios(t *testing.T) { Name: testSecretName, Namespace: testNamespace, Labels: map[string]string{ - "oadp.openshift.io/secret-type": "sts-credentials", + STSSecretLabelKey: STSSecretLabelValue, }, }, Data: map[string][]byte{