diff --git a/internal/controller/nodeagent.go b/internal/controller/nodeagent.go index 3c4fa643f2..004459c6c1 100644 --- a/internal/controller/nodeagent.go +++ b/internal/controller/nodeagent.go @@ -555,8 +555,7 @@ func (r *DataProtectionApplicationReconciler) customizeNodeAgentDaemonset(ds *ap nodeAgentContainer.Env = common.AppendUniqueEnvVars(nodeAgentContainer.Env, proxy.ReadProxyVarsFromEnv()) // Add Azure workload identity environment variables if configured - azureClientID := os.Getenv(stsflow.ClientIDEnvKey) - if azureClientID != "" && os.Getenv(stsflow.TenantIDEnvKey) != "" && os.Getenv(stsflow.SubscriptionIDEnvKey) != "" { + if stsflow.AzureIsWorkloadIdentity() { // Use envFrom to reference the secret containing Azure workload identity env vars if nodeAgentContainer.EnvFrom == nil { nodeAgentContainer.EnvFrom = []corev1.EnvFromSource{} diff --git a/internal/controller/registry.go b/internal/controller/registry.go index 75658a1356..49de6d907f 100644 --- a/internal/controller/registry.go +++ b/internal/controller/registry.go @@ -22,6 +22,7 @@ import ( oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" "github.com/openshift/oadp-operator/pkg/common" "github.com/openshift/oadp-operator/pkg/credentials" + "github.com/openshift/oadp-operator/pkg/credentials/stsflow" ) // Registry Env var keys @@ -36,13 +37,13 @@ const ( RegistryStorageS3RootdirectoryEnvVarKey = "REGISTRY_STORAGE_S3_ROOTDIRECTORY" RegistryStorageS3SkipverifyEnvVarKey = "REGISTRY_STORAGE_S3_SKIPVERIFY" // Azure registry env vars - RegistryStorageAzureContainerEnvVarKey = "REGISTRY_STORAGE_AZURE_CONTAINER" - RegistryStorageAzureAccountnameEnvVarKey = "REGISTRY_STORAGE_AZURE_ACCOUNTNAME" - RegistryStorageAzureAccountkeyEnvVarKey = "REGISTRY_STORAGE_AZURE_ACCOUNTKEY" - RegistryStorageAzureSPNClientIDEnvVarKey = "REGISTRY_STORAGE_AZURE_SPN_CLIENT_ID" - RegistryStorageAzureSPNClientSecretEnvVarKey = "REGISTRY_STORAGE_AZURE_SPN_CLIENT_SECRET" - RegistryStorageAzureSPNTenantIDEnvVarKey = "REGISTRY_STORAGE_AZURE_SPN_TENANT_ID" - RegistryStorageAzureAADEndpointEnvVarKey = "REGISTRY_STORAGE_AZURE_AAD_ENDPOINT" + RegistryStorageAzureContainerEnvVarKey = "REGISTRY_STORAGE_AZURE_CONTAINER" + RegistryStorageAzureAccountnameEnvVarKey = "REGISTRY_STORAGE_AZURE_ACCOUNTNAME" + RegistryStorageAzureAccountkeyEnvVarKey = "REGISTRY_STORAGE_AZURE_ACCOUNTKEY" + RegistryStorageAzureCredentialsTypeEnvVarKey = "REGISTRY_STORAGE_AZURE_CREDENTIALS_TYPE" + RegistryStorageAzureCredentialsClientIDEnvVarKey = "REGISTRY_STORAGE_AZURE_CREDENTIALS_CLIENTID" + RegistryStorageAzureCredentialsSecretEnvVarKey = "REGISTRY_STORAGE_AZURE_CREDENTIALS_SECRET" + RegistryStorageAzureCredentialsTenantIDEnvVarKey = "REGISTRY_STORAGE_AZURE_CREDENTIALS_TENANTID" // GCP registry env vars RegistryStorageGCSBucket = "REGISTRY_STORAGE_GCS_BUCKET" RegistryStorageGCSKeyfile = "REGISTRY_STORAGE_GCS_KEYFILE" @@ -117,19 +118,19 @@ var cloudProviderEnvVarMap = map[string][]corev1.EnvVar{ Value: "", }, { - Name: RegistryStorageAzureAADEndpointEnvVarKey, - Value: "", + Name: RegistryStorageAzureCredentialsTypeEnvVarKey, + Value: "", // Will be set dynamically based on auth method }, { - Name: RegistryStorageAzureSPNClientIDEnvVarKey, + Name: RegistryStorageAzureCredentialsClientIDEnvVarKey, Value: "", }, { - Name: RegistryStorageAzureSPNClientSecretEnvVarKey, + Name: RegistryStorageAzureCredentialsSecretEnvVarKey, Value: "", }, { - Name: RegistryStorageAzureSPNTenantIDEnvVarKey, + Name: RegistryStorageAzureCredentialsTenantIDEnvVarKey, Value: "", }, }, @@ -784,6 +785,16 @@ func (r *DataProtectionApplicationReconciler) populateAWSRegistrySecret(bsl *vel } func (r *DataProtectionApplicationReconciler) populateAzureRegistrySecret(bsl *velerov1.BackupStorageLocation, registrySecret *corev1.Secret) error { + // Check if Azure workload identity is configured + isWorkloadIdentity := stsflow.AzureIsWorkloadIdentity() + + // Determine authentication type + var credentialsType string + if isWorkloadIdentity { + credentialsType = "default_credentials" + r.Log.Info("Azure workload identity detected, using default_credentials for registry") + } + // Check for secret name secretName, secretKey, _ := r.getSecretNameAndKey(bsl.Spec.Config, bsl.Spec.Credential, oadpv1alpha1.DefaultPluginMicrosoftAzure) @@ -794,25 +805,31 @@ func (r *DataProtectionApplicationReconciler) populateAzureRegistrySecret(bsl *v return err } - // parse the secret and get azure storage account key + // parse the secret and get azure credentials azcreds, err := r.parseAzureSecret(secret, secretKey) if err != nil { r.Log.Info(fmt.Sprintf("Error parsing provider secret %s for backupstoragelocation %s/%s", secretName, bsl.Namespace, bsl.Name)) return err } - if len(bsl.Spec.Config["storageAccountKeyEnvVar"]) != 0 { - if azcreds.strorageAccountKey == "" { - r.Log.Info("Expecting storageAccountKeyEnvVar value set present in the credentials") - return errors.New("no strorageAccountKey value present in credentials file") - } - } else { - if len(azcreds.subscriptionID) == 0 && - len(azcreds.tenantID) == 0 && - len(azcreds.clientID) == 0 && - len(azcreds.clientSecret) == 0 && - len(azcreds.resourceGroup) == 0 { - return errors.New("error finding service principal parameters for the supplied Azure credential") + // Determine credentials type based on available credentials + if credentialsType == "" { + if len(bsl.Spec.Config["storageAccountKeyEnvVar"]) != 0 { + if azcreds.strorageAccountKey == "" { + r.Log.Info("Expecting storageAccountKeyEnvVar value set present in the credentials") + return errors.New("no strorageAccountKey value present in credentials file") + } + credentialsType = "shared_key" + } else if azcreds.clientID != "" && azcreds.clientSecret != "" && azcreds.tenantID != "" { + credentialsType = "client_secret" + } else { + if len(azcreds.subscriptionID) == 0 && + len(azcreds.tenantID) == 0 && + len(azcreds.clientID) == 0 && + len(azcreds.clientSecret) == 0 && + len(azcreds.resourceGroup) == 0 { + return errors.New("error finding service principal parameters for the supplied Azure credential") + } } } @@ -823,6 +840,7 @@ func (r *DataProtectionApplicationReconciler) populateAzureRegistrySecret(bsl *v "client_id_key": []byte(azcreds.clientID), "client_secret_key": []byte(azcreds.clientSecret), "resource_group_key": []byte(azcreds.resourceGroup), + "credentials_type": []byte(credentialsType), } return nil diff --git a/internal/controller/registry_test.go b/internal/controller/registry_test.go index e5636df655..409a4fa4e0 100644 --- a/internal/controller/registry_test.go +++ b/internal/controller/registry_test.go @@ -15,6 +15,7 @@ import ( "k8s.io/client-go/tools/record" oadpv1alpha1 "github.com/openshift/oadp-operator/api/v1alpha1" + "github.com/openshift/oadp-operator/pkg/credentials/stsflow" ) func getSchemeForFakeClientForRegistry() (*runtime.Scheme, error) { @@ -121,7 +122,6 @@ var ( } secretAzureServicePrincipalData = map[string][]byte{ "cloud": []byte("[default]" + "\n" + - "AZURE_STORAGE_ACCOUNT_ACCESS_KEY=" + testStoragekey + "\n" + "AZURE_CLOUD_NAME=" + testCloudName + "\n" + "AZURE_SUBSCRIPTION_ID=" + testSubscriptionID + "\n" + "AZURE_TENANT_ID=" + testTenantID + "\n" + @@ -140,6 +140,7 @@ var ( "storage_account_key": []byte(testStoragekey), "subscription_id_key": []byte(""), "tenant_id_key": []byte(""), + "credentials_type": []byte("shared_key"), } azureRegistrySPSecretData = map[string][]byte{ "client_id_key": []byte(testClientID), @@ -440,9 +441,11 @@ func TestDPAReconciler_populateAzureRegistrySecret(t *testing.T) { azureSecret *corev1.Secret dpa *oadpv1alpha1.DataProtectionApplication wantErr bool + wantSecretData map[string][]byte + envVars map[string]string }{ { - name: "Given Velero CR and bsl instance, appropriate registry secret is updated for azure case", + name: "Given Velero CR and bsl instance with storage account key, appropriate registry secret is updated", wantErr: false, bsl: &velerov1.BackupStorageLocation{ ObjectMeta: metav1.ObjectMeta{ @@ -483,10 +486,175 @@ func TestDPAReconciler_populateAzureRegistrySecret(t *testing.T) { Namespace: "test-ns", }, }, + wantSecretData: azureRegistrySecretData, + }, + { + name: "Azure workload identity detected - uses default_credentials", + wantErr: false, + envVars: map[string]string{ + stsflow.ClientIDEnvKey: "test-client-id", + stsflow.TenantIDEnvKey: "test-tenant-id", + stsflow.SubscriptionIDEnvKey: "test-subscription-id", + }, + bsl: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-bsl-workload", + Namespace: "test-ns", + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: AzureProvider, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "azure-bucket", + }, + }, + Config: map[string]string{ + StorageAccount: "velero-azure-account", + ResourceGroup: testResourceGroup, + }, + }, + }, + dpa: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Velero-test-CR", + Namespace: "test-ns", + }, + }, + azureSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cloud-credentials-azure", + Namespace: "test-ns", + }, + Data: secretAzureServicePrincipalData, + }, + registrySecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "oadp-test-bsl-workload-azure-registry-secret", + Namespace: "test-ns", + }, + }, + wantSecretData: map[string][]byte{ + "client_id_key": []byte(testClientID), + "client_secret_key": []byte(testClientSecret), + "resource_group_key": []byte(testResourceGroup), + "storage_account_key": []byte(""), + "subscription_id_key": []byte(testSubscriptionID), + "tenant_id_key": []byte(testTenantID), + "credentials_type": []byte("default_credentials"), + }, + }, + { + name: "Service principal credentials - uses client_secret", + wantErr: false, + bsl: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-bsl-sp", + Namespace: "test-ns", + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: AzureProvider, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "azure-bucket", + }, + }, + Config: map[string]string{ + StorageAccount: "velero-azure-account", + ResourceGroup: testResourceGroup, + }, + }, + }, + dpa: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Velero-test-CR", + Namespace: "test-ns", + }, + }, + azureSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cloud-credentials-azure", + Namespace: "test-ns", + }, + Data: secretAzureServicePrincipalData, + }, + registrySecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "oadp-test-bsl-sp-azure-registry-secret", + Namespace: "test-ns", + }, + }, + wantSecretData: map[string][]byte{ + "client_id_key": []byte(testClientID), + "client_secret_key": []byte(testClientSecret), + "resource_group_key": []byte(testResourceGroup), + "storage_account_key": []byte(""), + "subscription_id_key": []byte(testSubscriptionID), + "tenant_id_key": []byte(testTenantID), + "credentials_type": []byte("client_secret"), + }, + }, + { + name: "Partial workload identity env vars - falls back to service principal", + wantErr: false, + envVars: map[string]string{ + stsflow.ClientIDEnvKey: "test-client-id", + stsflow.TenantIDEnvKey: "test-tenant-id", + // Missing SUBSCRIPTIONID + }, + bsl: &velerov1.BackupStorageLocation{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-bsl-partial", + Namespace: "test-ns", + }, + Spec: velerov1.BackupStorageLocationSpec{ + Provider: AzureProvider, + StorageType: velerov1.StorageType{ + ObjectStorage: &velerov1.ObjectStorageLocation{ + Bucket: "azure-bucket", + }, + }, + Config: map[string]string{ + StorageAccount: "velero-azure-account", + ResourceGroup: testResourceGroup, + }, + }, + }, + dpa: &oadpv1alpha1.DataProtectionApplication{ + ObjectMeta: metav1.ObjectMeta{ + Name: "Velero-test-CR", + Namespace: "test-ns", + }, + }, + azureSecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cloud-credentials-azure", + Namespace: "test-ns", + }, + Data: secretAzureServicePrincipalData, + }, + registrySecret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "oadp-test-bsl-partial-azure-registry-secret", + Namespace: "test-ns", + }, + }, + wantSecretData: map[string][]byte{ + "client_id_key": []byte(testClientID), + "client_secret_key": []byte(testClientSecret), + "resource_group_key": []byte(testResourceGroup), + "storage_account_key": []byte(""), + "subscription_id_key": []byte(testSubscriptionID), + "tenant_id_key": []byte(testTenantID), + "credentials_type": []byte("client_secret"), + }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + // Set environment variables for test + for key, value := range tt.envVars { + t.Setenv(key, value) + } fakeClient, err := getFakeClientFromObjects(tt.azureSecret, tt.dpa) if err != nil { t.Errorf("error in creating fake client, likely programmer error") @@ -510,13 +678,13 @@ func TestDPAReconciler_populateAzureRegistrySecret(t *testing.T) { oadpv1alpha1.OadpOperatorLabel: "True", }, }, - Data: azureRegistrySecretData, + Data: tt.wantSecretData, } if err := r.populateAzureRegistrySecret(tt.bsl, tt.registrySecret); (err != nil) != tt.wantErr { t.Errorf("populateAzureRegistrySecret() error = %v, wantErr %v", err, tt.wantErr) } - if !reflect.DeepEqual(tt.registrySecret.Data, wantRegistrySecret.Data) { - t.Errorf("expected azure registry secret to be %#v, got %#v", tt.registrySecret, wantRegistrySecret.Data) + if !tt.wantErr && !reflect.DeepEqual(tt.registrySecret.Data, wantRegistrySecret.Data) { + t.Errorf("expected azure registry secret to be %#v, got %#v", wantRegistrySecret.Data, tt.registrySecret.Data) } }) } @@ -683,7 +851,7 @@ func TestDPAReconciler_parseAzureSecret(t *testing.T) { }, secretKey: "cloud", wantCreds: azureCredentials{ - strorageAccountKey: testStoragekey, + strorageAccountKey: "", subscriptionID: testSubscriptionID, tenantID: testTenantID, clientID: testClientID, diff --git a/internal/controller/stsflow.go b/internal/controller/stsflow.go index 5c8223dcb6..eb7c080074 100644 --- a/internal/controller/stsflow.go +++ b/internal/controller/stsflow.go @@ -30,15 +30,16 @@ import ( // eliminating the need for temporary credential files as used by AWS/GCP providers. func (r *DataProtectionApplicationReconciler) ReconcileAzureWorkloadIdentitySecret(log logr.Logger) (bool, error) { dpa := r.dpa - azureClientID := os.Getenv(stsflow.ClientIDEnvKey) // Only create secret if Azure workload identity environment variables are present - azureTenantID := os.Getenv(stsflow.TenantIDEnvKey) - if azureClientID == "" || azureTenantID == "" || os.Getenv(stsflow.SubscriptionIDEnvKey) == "" { + if !stsflow.AzureIsWorkloadIdentity() { // No Azure workload identity configured, nothing to do return true, nil } + azureClientID := os.Getenv(stsflow.ClientIDEnvKey) + azureTenantID := os.Getenv(stsflow.TenantIDEnvKey) + secret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: stsflow.AzureWorkloadIdentitySecretName, diff --git a/internal/controller/velero.go b/internal/controller/velero.go index 3d99f319c6..a0febf7283 100644 --- a/internal/controller/velero.go +++ b/internal/controller/velero.go @@ -639,8 +639,7 @@ func (r *DataProtectionApplicationReconciler) customizeVeleroContainer(veleroCon } // Add Azure workload identity environment variables if using Azure STS - azureClientID := os.Getenv(stsflow.ClientIDEnvKey) - if azureClientID != "" && os.Getenv(stsflow.TenantIDEnvKey) != "" && os.Getenv(stsflow.SubscriptionIDEnvKey) != "" { + if stsflow.AzureIsWorkloadIdentity() { // Use envFrom to reference the secret containing Azure workload identity env vars if veleroContainer.EnvFrom == nil { veleroContainer.EnvFrom = []corev1.EnvFromSource{} diff --git a/pkg/credentials/stsflow/stsflow.go b/pkg/credentials/stsflow/stsflow.go index 2bf426223a..27f47a5c7b 100644 --- a/pkg/credentials/stsflow/stsflow.go +++ b/pkg/credentials/stsflow/stsflow.go @@ -75,6 +75,17 @@ const ( ErrMsgUpdateSecret = "unable to update secret resource: %v" ) +// AzureIsWorkloadIdentity checks if Azure workload identity environment variables are configured. +// Returns true if all required Azure workload identity environment variables are present: +// - CLIENTID (Azure client ID) +// - TENANTID (Azure tenant ID) +// - SUBSCRIPTIONID (Azure subscription ID) +func AzureIsWorkloadIdentity() bool { + return os.Getenv(ClientIDEnvKey) != "" && + os.Getenv(TenantIDEnvKey) != "" && + os.Getenv(SubscriptionIDEnvKey) != "" +} + // STSStandardizedFlow creates secrets for Short Term Service Account Tokens from environment variables for // AWS STS, GCP WIF, and Azure following the standardized authentication workflow (https://github.com/openshift/enhancements/pull/1800). // Users provide these values during web console installation, and they are set as environment