OADP-6652: Fix unnecessary secret updates and logging in STS flow (#1936)

* Fix unnecessary secret updates and logging in STS flow

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 <[email protected]>

* refactor: Use constants for STS secret labels and error messages

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 <[email protected]>

---------

Co-authored-by: Claude <[email protected]>

Author: kaovilai

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{

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 {

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

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,
 		},

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
@@ -216,58 +230,98 @@ 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,
 			Labels: map[string]string{
-				"oadp.openshift.io/secret-type": "sts-credentials",
+				STSSecretLabelKey: STSSecretLabelValue,
 			},
 		},
 		StringData: credStringData,
 	}
-	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")
+
+	// First, try to get the existing secret
+	existingSecret := corev1.Secret{}
+	err := clientInstance.Get(context.Background(), types.NamespacedName{Name: secretName, Namespace: secretNS}, &existingSecret)
+
+	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, ErrMsgCreateSecret)
 				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, ErrMsgGetSecret)
+			return err
+		}
+	} else {
+		// Secret exists, check if update is needed
+		needsUpdate := false
+
+		// Check if labels need updating
+		if existingSecret.Labels == nil || existingSecret.Labels[STSSecretLabelKey] != STSSecretLabelValue {
+			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 = SecretVerbUpdated
+			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 {
-				setupLog.Error(err, fmt.Sprintf("unable to update secret resource: %v", err))
+			updatedSecret.Labels[STSSecretLabelKey] = STSSecretLabelValue
+
+			if err := clientInstance.Patch(context.Background(), updatedSecret, client.MergeFrom(&existingSecret)); err != nil {
+				setupLog.Error(err, fmt.Sprintf(ErrMsgUpdateSecret, err))
 				return err
 			}
 		} else {
-			setupLog.Error(err, "unable to create secret resource")
-			return err
+			// No update needed
+			verb = SecretVerbUnchanged
 		}
 	}
-	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 != SecretVerbUnchanged {
+		setupLog.Info("Secret " + desiredSecret.Name + " " + verb + " successfully")
+	}
+
+	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)
 		if err != nil {
 			setupLog.Error(err, "error waiting for credentials Secret")

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])
 			}
 		})
 	}
@@ -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{
+					STSSecretLabelKey: STSSecretLabelValue,
+				},
+			},
+			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")