OADP-6652: Fix unnecessary secret updates and logging in STS flow

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")