From f31141e9d3147e615636cadda33a96b9e869cb12 Mon Sep 17 00:00:00 2001 From: rakelkar Date: Mon, 26 Aug 2019 23:28:40 -0700 Subject: [PATCH] Remove default azure secret name (#315) * add readme for azure * fallback if the well known az secret is not found * remove azure config name * remove azure config --- config/default/configmap/kfservice.yaml | 3 - docs/samples/azure/README.md | 2 +- .../credentials/azure/azure_secret.go | 7 +- .../credentials/azure/azure_secret_test.go | 3 +- .../service_account_credentials.go | 15 +- .../service_account_credentials_test.go | 149 +++++++++++++++++- 6 files changed, 155 insertions(+), 24 deletions(-) diff --git a/config/default/configmap/kfservice.yaml b/config/default/configmap/kfservice.yaml index 4077e331373..ecec8e385c7 100644 --- a/config/default/configmap/kfservice.yaml +++ b/config/default/configmap/kfservice.yaml @@ -34,8 +34,5 @@ data: "s3": { "s3AccessKeyIDName": "awsAccessKeyID", "s3SecretAccessKeyName": "awsSecretAccessKey" - }, - "azure": { - "azureSecretName": "azcreds" } } diff --git a/docs/samples/azure/README.md b/docs/samples/azure/README.md index e97dddf71f3..65784956094 100644 --- a/docs/samples/azure/README.md +++ b/docs/samples/azure/README.md @@ -14,7 +14,7 @@ KFServing supports authenticating using an Azure Service Principle. * Details on assigning storage roles [here](https://docs.microsoft.com/en-us/azure/storage/common/storage-auth-aad). ### Create a K8s Secret -Store your Azure SP secrets as a k8s secret named `azcreds`. +Store your Azure SP secrets as a k8s secret. ```yaml apiVersion: v1 diff --git a/pkg/controller/kfservice/resources/credentials/azure/azure_secret.go b/pkg/controller/kfservice/resources/credentials/azure/azure_secret.go index a9de12ac84f..c1d5818533e 100644 --- a/pkg/controller/kfservice/resources/credentials/azure/azure_secret.go +++ b/pkg/controller/kfservice/resources/credentials/azure/azure_secret.go @@ -21,18 +21,13 @@ import ( ) const ( - AzureSecretName = "azcreds" AzureSubscriptionId = "AZ_SUBSCRIPTION_ID" AzureTenantId = "AZ_TENANT_ID" AzureClientId = "AZ_CLIENT_ID" AzureClientSecret = "AZ_CLIENT_SECRET" ) -type AzureConfig struct { - AzureSecretName string `json:"azureSecretName,omitempty"` -} - -func BuildSecretEnvs(secret *v1.Secret, azureConfig *AzureConfig) []v1.EnvVar { +func BuildSecretEnvs(secret *v1.Secret) []v1.EnvVar { envs := []v1.EnvVar{ { Name: AzureSubscriptionId, diff --git a/pkg/controller/kfservice/resources/credentials/azure/azure_secret_test.go b/pkg/controller/kfservice/resources/credentials/azure/azure_secret_test.go index e8a3d829a57..59787bbbe48 100644 --- a/pkg/controller/kfservice/resources/credentials/azure/azure_secret_test.go +++ b/pkg/controller/kfservice/resources/credentials/azure/azure_secret_test.go @@ -26,7 +26,6 @@ import ( func TestAzureSecret(t *testing.T) { scenarios := map[string]struct { - config AzureConfig secret *v1.Secret expected []v1.EnvVar }{ @@ -86,7 +85,7 @@ func TestAzureSecret(t *testing.T) { } for name, scenario := range scenarios { - envs := BuildSecretEnvs(scenario.secret, &scenario.config) + envs := BuildSecretEnvs(scenario.secret) if diff := cmp.Diff(scenario.expected, envs); diff != "" { t.Errorf("Test %q unexpected result (-want +got): %v", name, diff) diff --git a/pkg/controller/kfservice/resources/credentials/service_account_credentials.go b/pkg/controller/kfservice/resources/credentials/service_account_credentials.go index 8f9fee05bb8..49c9add0ef2 100644 --- a/pkg/controller/kfservice/resources/credentials/service_account_credentials.go +++ b/pkg/controller/kfservice/resources/credentials/service_account_credentials.go @@ -35,9 +35,8 @@ const ( ) type CredentialConfig struct { - S3 s3.S3Config `json:"s3,omitempty"` - GCS gcs.GCSConfig `json:"gcs,omitempty"` - AZURE azure.AzureConfig `json:"azure,omitempty"` + S3 s3.S3Config `json:"s3,omitempty"` + GCS gcs.GCSConfig `json:"gcs,omitempty"` } type CredentialBuilder struct { @@ -67,17 +66,12 @@ func (c *CredentialBuilder) CreateSecretVolumeAndEnv(namespace string, serviceAc serviceAccountName = "default" } s3SecretAccessKeyName := s3.AWSSecretAccessKeyName - azureSecretName := azure.AzureSecretName gcsCredentialFileName := gcs.GCSCredentialFileName if c.config.S3.S3SecretAccessKeyName != "" { s3SecretAccessKeyName = c.config.S3.S3SecretAccessKeyName } - if c.config.AZURE.AzureSecretName != "" { - azureSecretName = c.config.AZURE.AzureSecretName - } - if c.config.GCS.GCSCredentialFileName != "" { gcsCredentialFileName = c.config.GCS.GCSCredentialFileName } @@ -89,6 +83,7 @@ func (c *CredentialBuilder) CreateSecretVolumeAndEnv(namespace string, serviceAc log.Error(err, "Failed to find service account", "ServiceAccountName", serviceAccountName) return nil } + for _, secretRef := range serviceAccount.Secrets { log.Info("found secret", "SecretName", secretRef.Name) secret := &v1.Secret{} @@ -113,9 +108,9 @@ func (c *CredentialBuilder) CreateSecretVolumeAndEnv(namespace string, serviceAc Name: gcs.GCSCredentialEnvKey, Value: gcs.GCSCredentialVolumeMountPath + gcsCredentialFileName, }) - } else if secret.Name == azureSecretName { + } else if _, ok := secret.Data[azure.AzureClientSecret]; ok { log.Info("Setting secret envs for azure", "AzureSecret", secret.Name) - envs := azure.BuildSecretEnvs(secret, &c.config.AZURE) + envs := azure.BuildSecretEnvs(secret) container.Env = append(container.Env, envs...) } else { log.V(5).Info("Skipping non gcs/s3/azure secret", "Secret", secret.Name) diff --git a/pkg/controller/kfservice/resources/credentials/service_account_credentials_test.go b/pkg/controller/kfservice/resources/credentials/service_account_credentials_test.go index e54105e5b39..0c1ee750bef 100644 --- a/pkg/controller/kfservice/resources/credentials/service_account_credentials_test.go +++ b/pkg/controller/kfservice/resources/credentials/service_account_credentials_test.go @@ -20,6 +20,8 @@ import ( "context" "testing" + "github.com/kubeflow/kfserving/pkg/controller/kfservice/resources/credentials/azure" + "github.com/google/go-cmp/cmp" "github.com/kubeflow/kfserving/pkg/controller/kfservice/resources/credentials/gcs" "github.com/kubeflow/kfserving/pkg/controller/kfservice/resources/credentials/s3" @@ -37,8 +39,7 @@ var configMap = &v1.ConfigMap{ "s3" : { "s3AccessKeyIDName": "awsAccessKeyID", "s3SecretAccessKeyName": "awsSecretAccessKey" - }, - "azure" : {"azureSecretName": "azcreds"} + } }`, }, } @@ -287,3 +288,147 @@ func TestGCSCredentialBuilder(t *testing.T) { } } + +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{ + "AZ_SUBSCRIPTION_ID": {}, + "AZ_TENANT_ID": {}, + "AZ_CLIENT_ID": {}, + "AZ_CLIENT_SECRET": {}, + }, + } + + scenarios := map[string]struct { + serviceAccount *v1.ServiceAccount + inputConfiguration *v1alpha1.Configuration + expectedConfiguration *v1alpha1.Configuration + shouldFail bool + }{ + "Custom Azure Secret": { + serviceAccount: customOnlyServiceAccount, + inputConfiguration: &v1alpha1.Configuration{ + Spec: v1alpha1.ConfigurationSpec{ + Template: &v1alpha1.RevisionTemplateSpec{ + Spec: v1alpha1.RevisionSpec{ + RevisionSpec: v1beta1.RevisionSpec{ + PodSpec: v1.PodSpec{ + Containers: []v1.Container{ + {}, + }, + }, + }, + }, + }, + }, + }, + expectedConfiguration: &v1alpha1.Configuration{ + Spec: v1alpha1.ConfigurationSpec{ + Template: &v1alpha1.RevisionTemplateSpec{ + Spec: v1alpha1.RevisionSpec{ + RevisionSpec: v1beta1.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()) +}