diff --git a/internal/controller/bsl_test.go b/internal/controller/bsl_test.go index 14fa0774ef..dc8b4a39ac 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/internal/controller/cloudstorage_controller.go b/internal/controller/cloudstorage_controller.go index 1c5fa32f64..d29fc67009 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/bucket/azure.go b/pkg/bucket/azure.go index 587649d89d..4010dba04d 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 0554e19039..88ef754320 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 a3c63f16b7..2bf426223a 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 @@ -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") diff --git a/pkg/credentials/stsflow/stsflow_test.go b/pkg/credentials/stsflow/stsflow_test.go index 599567813e..51b37a361b 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]) } }) } @@ -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")