From f8c069950d99e01d92c2f6fe533140151493b3d6 Mon Sep 17 00:00:00 2001 From: Zhongcheng Lao Date: Fri, 21 Oct 2022 21:00:27 +0800 Subject: [PATCH] Supports more authentication approaches on Azure in Storage Initializer (#2014) * Use DefaultAzureCredential to support multiple authentication Signed-off-by: Zhongcheng Lao * Use environment variables that matches Azure Identity SDK Signed-off-by: Zhongcheng Lao * Fix test case failure Signed-off-by: Zhongcheng Lao * Add documentation to Azure Managed Identity Signed-off-by: Zhongcheng Lao * Use new key for SA secret entries Signed-off-by: Zhongcheng Lao * Correct Kubernetes manifest in Azure storage sample Signed-off-by: Zhongcheng Lao Signed-off-by: Zhongcheng Lao --- docs/samples/storage/azure/README.md | 51 ++++-- pkg/credentials/azure/azure_secret.go | 75 ++++----- .../service_account_credentials.go | 6 +- .../service_account_credentials_test.go | 150 +++++++++++++++++- python/kserve/kserve/api/creds_utils.py | 8 +- python/kserve/kserve/storage.py | 18 ++- test/e2e/credentials/test_set_creds.py | 8 +- 7 files changed, 243 insertions(+), 73 deletions(-) diff --git a/docs/samples/storage/azure/README.md b/docs/samples/storage/azure/README.md index 3e5637ffdaa..2b886f62048 100644 --- a/docs/samples/storage/azure/README.md +++ b/docs/samples/storage/azure/README.md @@ -1,16 +1,17 @@ # Predict on a InferenceService with saved model on Azure ## Using Public Azure Blobs -By default, KFServing uses anonymous client to download artifacts. To point to an Azure Blob, specify StorageUri to point to an Azure Blob Storage with the format: +By default, KServe uses anonymous client to download artifacts. To point to an Azure Blob, specify StorageUri to point to an Azure Blob Storage with the format: ```https://{$STORAGE_ACCOUNT_NAME}.blob.core.windows.net/{$CONTAINER}/{$PATH}``` -e.g. https://kfserving.blob.core.windows.net/triton/simple_string/ +e.g. https://kserve.blob.core.windows.net/triton/simple_string/ ## Using Private Blobs -KFServing supports authenticating using an Azure Service Principle. +KServe supports authenticating using an Azure Service Principle. ### Create an authorized Azure Service Principle * To create an Azure Service Principle follow the steps [here](https://docs.microsoft.com/en-us/cli/azure/create-an-azure-service-principal-azure-cli?view=azure-cli-latest). -* Assign the SP the `Storage Blob Data Owner` role on your blob (KFServing needs this permission as it needs to list contents at the blob path to filter items to download). +* You may also use Azure Managed Identity follow the steps [here](https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/how-manage-user-assigned-managed-identities?pivots=identity-mi-methods-azcli) +* Assign the SP or MI the `Storage Blob Data Owner` role on your blob (KServe needs this permission as it needs to list contents at the blob path to filter items to download). * Details on assigning storage roles [here](https://docs.microsoft.com/en-us/azure/storage/common/storage-auth-aad). ### Create a K8s Secret @@ -23,16 +24,18 @@ metadata: name: azcreds type: Opaque data: - AZ_CLIENT_ID: xxxxx - AZ_CLIENT_SECRET: xxxxx - AZ_SUBSCRIPTION_ID: xxxxx - AZ_TENANT_ID: xxxxx + AZURE_CLIENT_ID: xxxxx + AZURE_CLIENT_SECRET: xxxxx + AZURE_SUBSCRIPTION_ID: xxxxx + AZURE_TENANT_ID: xxxxx ``` -Note: The azure secret KFServing looks for can be configured by running `kubectl edit -n kfserving-system inferenceservice-config` +Note: +* The azure secret KServe looks for can be configured by running `kubectl edit -n kserving-system inferenceservice-config` +* `AZURE_CLIENT_SECRET` is not required when using Managed Identity ### Attach to Service Account -`KFServing` gets the secrets from your service account, you need to add the above created or existing secret to your service account's secret list. -By default `KFServing` uses `default` service account, user can use own service account and overwrite on `InferenceService` CRD. +`KServe` gets the secrets from your service account, you need to add the above created or existing secret to your service account's secret list. +By default `KServe` uses `default` service account, user can use own service account and overwrite on `InferenceService` CRD. ```yaml apiVersion: v1 @@ -51,4 +54,28 @@ kubectl apply -f azcreds.yaml Note: To use your model binary you must reference the folder where it's located with an ending ```/``` to denote it`s a folder. ```bash https://accountname.blob.core.windows.net/container/models/iris/v1.1/ -``` \ No newline at end of file +``` + +### Use Pod Identity + +You may assign Managed Identity to `InferenceService` resource using Pod Identity. + +* Set up Pod Identity follow the steps [here](https://azure.github.io/aad-pod-identity/docs/demo/standard_walkthrough/) +* Add the `aadpodidbinding` label to your service + +```yaml +--- +apiVersion: serving.kserve.io/v1beta1 +kind: InferenceService +metadata: + name: kserve-simple-string + labels: + aadpodidbinding: piselector +spec: + template: + metadata: + predictor: + serviceAccountName: sa + tensorflow: + storageUri: "https://kserve.blob.core.windows.net/triton/simple_string/" +``` diff --git a/pkg/credentials/azure/azure_secret.go b/pkg/credentials/azure/azure_secret.go index 202d97531aa..3c0f2969ccf 100644 --- a/pkg/credentials/azure/azure_secret.go +++ b/pkg/credentials/azure/azure_secret.go @@ -21,59 +21,50 @@ import ( ) const ( - AzureSubscriptionId = "AZ_SUBSCRIPTION_ID" - AzureTenantId = "AZ_TENANT_ID" - AzureClientId = "AZ_CLIENT_ID" - AzureClientSecret = "AZ_CLIENT_SECRET" AzureStorageAccessKey = "AZURE_STORAGE_ACCESS_KEY" + // Legacy keys for backward compatibility + LegacyAzureSubscriptionId = "AZ_SUBSCRIPTION_ID" + LegacyAzureTenantId = "AZ_TENANT_ID" + LegacyAzureClientId = "AZ_CLIENT_ID" + LegacyAzureClientSecret = "AZ_CLIENT_SECRET" + + // Conforms to Azure constants + AzureSubscriptionId = "AZURE_SUBSCRIPTION_ID" + AzureTenantId = "AZURE_TENANT_ID" + AzureClientId = "AZURE_CLIENT_ID" + AzureClientSecret = "AZURE_CLIENT_SECRET" +) + +var ( + LegacyAzureEnvKeys = []string{LegacyAzureSubscriptionId, LegacyAzureTenantId, LegacyAzureClientId, LegacyAzureClientSecret} + AzureEnvKeys = []string{AzureSubscriptionId, AzureTenantId, AzureClientId, AzureClientSecret} + legacyAzureEnvKeyMappings = map[string]string{ + AzureSubscriptionId: LegacyAzureSubscriptionId, + AzureTenantId: LegacyAzureTenantId, + AzureClientId: LegacyAzureClientId, + AzureClientSecret: LegacyAzureClientSecret, + } ) func BuildSecretEnvs(secret *v1.Secret) []v1.EnvVar { - envs := []v1.EnvVar{ - { - Name: AzureSubscriptionId, - ValueFrom: &v1.EnvVarSource{ - SecretKeyRef: &v1.SecretKeySelector{ - LocalObjectReference: v1.LocalObjectReference{ - Name: secret.Name, - }, - Key: AzureSubscriptionId, - }, - }, - }, - { - Name: AzureTenantId, - ValueFrom: &v1.EnvVarSource{ - SecretKeyRef: &v1.SecretKeySelector{ - LocalObjectReference: v1.LocalObjectReference{ - Name: secret.Name, - }, - Key: AzureTenantId, - }, - }, - }, - { - Name: AzureClientId, + var envs []v1.EnvVar + for _, k := range AzureEnvKeys { + dataKey := k + legacyDataKey := legacyAzureEnvKeyMappings[k] + if _, ok := secret.Data[legacyDataKey]; ok { + dataKey = legacyDataKey + } + envs = append(envs, v1.EnvVar{ + Name: k, ValueFrom: &v1.EnvVarSource{ SecretKeyRef: &v1.SecretKeySelector{ LocalObjectReference: v1.LocalObjectReference{ Name: secret.Name, }, - Key: AzureClientId, + Key: dataKey, }, }, - }, - { - Name: AzureClientSecret, - ValueFrom: &v1.EnvVarSource{ - SecretKeyRef: &v1.SecretKeySelector{ - LocalObjectReference: v1.LocalObjectReference{ - Name: secret.Name, - }, - Key: AzureClientSecret, - }, - }, - }, + }) } return envs diff --git a/pkg/credentials/service_account_credentials.go b/pkg/credentials/service_account_credentials.go index b3499ca3953..e0286c86ffe 100644 --- a/pkg/credentials/service_account_credentials.go +++ b/pkg/credentials/service_account_credentials.go @@ -228,7 +228,11 @@ func (c *CredentialBuilder) CreateSecretVolumeAndEnv(namespace string, serviceAc Name: gcs.GCSCredentialEnvKey, Value: gcs.GCSCredentialVolumeMountPath + gcsCredentialFileName, }) - } else if _, ok := secret.Data[azure.AzureClientSecret]; ok { + } else if _, ok := secret.Data[azure.LegacyAzureClientId]; ok { + log.Info("Setting secret envs for azure", "AzureSecret", secret.Name) + envs := azure.BuildSecretEnvs(secret) + container.Env = append(container.Env, envs...) + } else if _, ok := secret.Data[azure.AzureClientId]; ok { log.Info("Setting secret envs for azure", "AzureSecret", secret.Name) envs := azure.BuildSecretEnvs(secret) container.Env = append(container.Env, envs...) diff --git a/pkg/credentials/service_account_credentials_test.go b/pkg/credentials/service_account_credentials_test.go index f36d4c1298c..5cf3148555d 100644 --- a/pkg/credentials/service_account_credentials_test.go +++ b/pkg/credentials/service_account_credentials_test.go @@ -400,7 +400,7 @@ func TestGCSCredentialBuilder(t *testing.T) { } } -func TestAzureCredentialBuilder(t *testing.T) { +func TestLegacyAzureCredentialBuilder(t *testing.T) { g := gomega.NewGomegaWithT(t) customOnlyServiceAccount := &v1.ServiceAccount{ ObjectMeta: metav1.ObjectMeta{ @@ -463,7 +463,7 @@ func TestAzureCredentialBuilder(t *testing.T) { LocalObjectReference: v1.LocalObjectReference{ Name: "az-custom-secret", }, - Key: azure.AzureSubscriptionId, + Key: azure.LegacyAzureSubscriptionId, }, }, }, @@ -474,7 +474,7 @@ func TestAzureCredentialBuilder(t *testing.T) { LocalObjectReference: v1.LocalObjectReference{ Name: "az-custom-secret", }, - Key: azure.AzureTenantId, + Key: azure.LegacyAzureTenantId, }, }, }, @@ -485,7 +485,7 @@ func TestAzureCredentialBuilder(t *testing.T) { LocalObjectReference: v1.LocalObjectReference{ Name: "az-custom-secret", }, - Key: azure.AzureClientId, + Key: azure.LegacyAzureClientId, }, }, }, @@ -496,7 +496,7 @@ func TestAzureCredentialBuilder(t *testing.T) { LocalObjectReference: v1.LocalObjectReference{ Name: "az-custom-secret", }, - Key: azure.AzureClientSecret, + Key: azure.LegacyAzureClientSecret, }, }, }, @@ -651,6 +651,146 @@ func TestHdfsCredentialBuilder(t *testing.T) { g.Expect(c.Delete(context.TODO(), customOnlyServiceAccount)).NotTo(gomega.HaveOccurred()) } +func TestAzureCredentialBuilder(t *testing.T) { + g := gomega.NewGomegaWithT(t) + customOnlyServiceAccount := &v1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: "custom-sa", + Namespace: "default", + }, + Secrets: []v1.ObjectReference{ + { + Name: "az-custom-secret", + Namespace: "default", + }, + }, + } + customAzureSecret := &v1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "az-custom-secret", + Namespace: "default", + }, + Data: map[string][]byte{ + "AZURE_SUBSCRIPTION_ID": {}, + "AZURE_TENANT_ID": {}, + "AZURE_CLIENT_ID": {}, + "AZURE_CLIENT_SECRET": {}, + }, + } + + scenarios := map[string]struct { + serviceAccount *v1.ServiceAccount + inputConfiguration *knservingv1.Configuration + expectedConfiguration *knservingv1.Configuration + shouldFail bool + }{ + "Custom Azure Secret": { + serviceAccount: customOnlyServiceAccount, + inputConfiguration: &knservingv1.Configuration{ + Spec: knservingv1.ConfigurationSpec{ + Template: knservingv1.RevisionTemplateSpec{ + Spec: knservingv1.RevisionSpec{ + PodSpec: v1.PodSpec{ + Containers: []v1.Container{ + {}, + }, + }, + }, + }, + }, + }, + expectedConfiguration: &knservingv1.Configuration{ + Spec: knservingv1.ConfigurationSpec{ + Template: knservingv1.RevisionTemplateSpec{ + Spec: knservingv1.RevisionSpec{ + PodSpec: v1.PodSpec{ + Containers: []v1.Container{ + { + Env: []v1.EnvVar{ + { + Name: azure.AzureSubscriptionId, + ValueFrom: &v1.EnvVarSource{ + SecretKeyRef: &v1.SecretKeySelector{ + LocalObjectReference: v1.LocalObjectReference{ + Name: "az-custom-secret", + }, + Key: azure.AzureSubscriptionId, + }, + }, + }, + { + Name: azure.AzureTenantId, + ValueFrom: &v1.EnvVarSource{ + SecretKeyRef: &v1.SecretKeySelector{ + LocalObjectReference: v1.LocalObjectReference{ + Name: "az-custom-secret", + }, + Key: azure.AzureTenantId, + }, + }, + }, + { + Name: azure.AzureClientId, + ValueFrom: &v1.EnvVarSource{ + SecretKeyRef: &v1.SecretKeySelector{ + LocalObjectReference: v1.LocalObjectReference{ + Name: "az-custom-secret", + }, + Key: azure.AzureClientId, + }, + }, + }, + { + Name: azure.AzureClientSecret, + ValueFrom: &v1.EnvVarSource{ + SecretKeyRef: &v1.SecretKeySelector{ + LocalObjectReference: v1.LocalObjectReference{ + Name: "az-custom-secret", + }, + Key: azure.AzureClientSecret, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + }, + shouldFail: false, + }, + } + + g.Expect(c.Create(context.TODO(), customAzureSecret)).NotTo(gomega.HaveOccurred()) + g.Expect(c.Create(context.TODO(), customOnlyServiceAccount)).NotTo(gomega.HaveOccurred()) + + builder := NewCredentialBulder(c, configMap) + for name, scenario := range scenarios { + + err := builder.CreateSecretVolumeAndEnv(scenario.serviceAccount.Namespace, scenario.serviceAccount.Name, + &scenario.inputConfiguration.Spec.Template.Spec.Containers[0], + &scenario.inputConfiguration.Spec.Template.Spec.Volumes, + ) + if scenario.shouldFail && err == nil { + t.Errorf("Test %q failed: returned success but expected error", name) + } + // Validate + if !scenario.shouldFail { + if err != nil { + t.Errorf("Test %q failed: returned error: %v", name, err) + } + if diff := cmp.Diff(scenario.expectedConfiguration, scenario.inputConfiguration); diff != "" { + t.Errorf("Test %q unexpected configuration spec (-want +got): %v", name, diff) + } + } + } + + g.Expect(c.Delete(context.TODO(), customAzureSecret)).NotTo(gomega.HaveOccurred()) + g.Expect(c.Delete(context.TODO(), customOnlyServiceAccount)).NotTo(gomega.HaveOccurred()) +} + func TestAzureStorageAccessKeyCredentialBuilder(t *testing.T) { g := gomega.NewGomegaWithT(t) customOnlyServiceAccount := &v1.ServiceAccount{ diff --git a/python/kserve/kserve/api/creds_utils.py b/python/kserve/kserve/api/creds_utils.py index 5bd47551b3d..9f95af49447 100644 --- a/python/kserve/kserve/api/creds_utils.py +++ b/python/kserve/kserve/api/creds_utils.py @@ -132,10 +132,10 @@ def set_azure_credentials(namespace, credentials_file, service_account): azure_creds = json.load(azure_creds_file) data = { - 'AZ_CLIENT_ID': azure_creds['clientId'], - 'AZ_CLIENT_SECRET': azure_creds['clientSecret'], - 'AZ_SUBSCRIPTION_ID': azure_creds['subscriptionId'], - 'AZ_TENANT_ID': azure_creds['tenantId'], + 'AZURE_CLIENT_ID': azure_creds['clientId'], + 'AZURE_CLIENT_SECRET': azure_creds['clientSecret'], + 'AZURE_SUBSCRIPTION_ID': azure_creds['subscriptionId'], + 'AZURE_TENANT_ID': azure_creds['tenantId'], } secret_name = create_secret( diff --git a/python/kserve/kserve/storage.py b/python/kserve/kserve/storage.py index e1d51d79b00..b11848a4dba 100644 --- a/python/kserve/kserve/storage.py +++ b/python/kserve/kserve/storage.py @@ -451,15 +451,23 @@ def _get_azure_storage_token(): tenant_id = os.getenv("AZ_TENANT_ID", "") client_id = os.getenv("AZ_CLIENT_ID", "") client_secret = os.getenv("AZ_CLIENT_SECRET", "") - subscription_id = os.getenv("AZ_SUBSCRIPTION_ID", "") - if tenant_id == "" or client_id == "" or client_secret == "" or subscription_id == "": + # convert old environment variable to conform Azure defaults + # see azure/identity/_constants.py + if tenant_id: + os.environ["AZURE_TENANT_ID"] = tenant_id + if client_id: + os.environ["AZURE_CLIENT_ID"] = client_id + if client_secret: + os.environ["AZURE_CLIENT_SECRET"] = client_secret + + client_id = os.getenv("AZURE_CLIENT_ID", "") + if not client_id: return None # note the SP must have "Storage Blob Data Owner" perms for this to work - from azure.identity import ClientSecretCredential - token_credential = ClientSecretCredential(tenant_id, - client_id, client_secret) + from azure.identity import DefaultAzureCredential + token_credential = DefaultAzureCredential() logging.info("Retrieved SP token credential for client_id: %s", client_id) diff --git a/test/e2e/credentials/test_set_creds.py b/test/e2e/credentials/test_set_creds.py index b2380d9638d..ca1766b8758 100644 --- a/test/e2e/credentials/test_set_creds.py +++ b/test/e2e/credentials/test_set_creds.py @@ -123,7 +123,7 @@ def test_azure_credentials(): created_sa = get_created_sa(sa_name) created_secret_name = created_sa.secrets[0].name created_secret = get_created_secret(created_secret_name) - assert created_secret.data['AZ_CLIENT_ID'] == 'dXNlcgo=' - assert created_secret.data['AZ_CLIENT_SECRET'] == 'cGFzc3dvcmQ=' - assert created_secret.data['AZ_SUBSCRIPTION_ID'] == 'MzMzMzMzMzMtMzMzMy0zMzMzLTMzMzMtMzMzMzMz' - assert created_secret.data['AZ_TENANT_ID'] == 'MTIzNAo=' + assert created_secret.data['AZURE_CLIENT_ID'] == 'dXNlcgo=' + assert created_secret.data['AZURE_CLIENT_SECRET'] == 'cGFzc3dvcmQ=' + assert created_secret.data['AZURE_SUBSCRIPTION_ID'] == 'MzMzMzMzMzMtMzMzMy0zMzMzLTMzMzMtMzMzMzMz' + assert created_secret.data['AZURE_TENANT_ID'] == 'MTIzNAo='