invoke credentials builder from model initializer (kubeflow#215)

* invoke credentials builder from model initializer * update test * remove sa annotation * fix initContainer typo * fix status code on configmap not found * format changes * update test to incl. credbuilder * add credentials unit tests
magdalenakuhn17 · Jul 2, 2019 · ad78acd · ad78acd
1 parent d537344
commit ad78acd
Show file tree

Hide file tree

Showing 7 changed files with 391 additions and 19 deletions.
diff --git a/pkg/controller/kfservice/resources/credentials/service_account_credentials.go b/pkg/controller/kfservice/resources/credentials/service_account_credentials.go
@@ -21,7 +21,6 @@ import (
 	"encoding/json"
 	"fmt"
 
-	knservingv1alpha1 "github.com/knative/serving/pkg/apis/serving/v1alpha1"
 	"github.com/kubeflow/kfserving/pkg/controller/kfservice/resources/credentials/gcs"
 	"github.com/kubeflow/kfserving/pkg/controller/kfservice/resources/credentials/s3"
 	v1 "k8s.io/api/core/v1"
@@ -61,7 +60,7 @@ func NewCredentialBulder(client client.Client, config *v1.ConfigMap) *Credential
 }
 
 func (c *CredentialBuilder) CreateSecretVolumeAndEnv(namespace string, serviceAccountName string,
-	configuration *knservingv1alpha1.Configuration) error {
+	container *v1.Container, volumes *[]v1.Volume) error {
 	if serviceAccountName == "" {
 		serviceAccountName = "default"
 	}
@@ -94,15 +93,14 @@ func (c *CredentialBuilder) CreateSecretVolumeAndEnv(namespace string, serviceAc
 		if _, ok := secret.Data[s3SecretAccessKeyName]; ok {
 			log.Info("Setting secret envs for s3", "S3Secret", secret.Name)
 			envs := s3.BuildSecretEnvs(secret, &c.config.S3)
-			configuration.Spec.RevisionTemplate.Spec.Container.Env = append(configuration.Spec.RevisionTemplate.Spec.Container.Env, envs...)
+			container.Env = append(container.Env, envs...)
 		} else if _, ok := secret.Data[gcsCredentialFileName]; ok {
 			log.Info("Setting secret volume for gcs", "GCSSecret", secret.Name)
 			volume, volumeMount := gcs.BuildSecretVolume(secret)
-			configuration.Spec.RevisionTemplate.Spec.Volumes =
-				append(configuration.Spec.RevisionTemplate.Spec.Volumes, volume)
-			configuration.Spec.RevisionTemplate.Spec.Container.VolumeMounts =
-				append(configuration.Spec.RevisionTemplate.Spec.Container.VolumeMounts, volumeMount)
-			configuration.Spec.RevisionTemplate.Spec.Container.Env = append(configuration.Spec.RevisionTemplate.Spec.Container.Env,
+			*volumes = append(*volumes, volume)
+			container.VolumeMounts =
+				append(container.VolumeMounts, volumeMount)
+			container.Env = append(container.Env,
 				v1.EnvVar{
 					Name:  gcs.GCSCredentialEnvKey,
 					Value: gcs.GCSCredentialVolumeMountPath + gcsCredentialFileName,

diff --git a/pkg/controller/kfservice/resources/credentials/service_account_credentials_test.go b/pkg/controller/kfservice/resources/credentials/service_account_credentials_test.go
@@ -138,7 +138,9 @@ func TestS3CredentialBuilder(t *testing.T) {
 		g.Expect(c.Create(context.TODO(), existingS3Secret)).NotTo(gomega.HaveOccurred())
 
 		err := builder.CreateSecretVolumeAndEnv(scenario.serviceAccount.Namespace, scenario.serviceAccount.Name,
-			scenario.inputConfiguration)
+			scenario.inputConfiguration.Spec.RevisionTemplate.Spec.Container,
+			&scenario.inputConfiguration.Spec.RevisionTemplate.Spec.Volumes,
+		)
 		if scenario.shouldFail && err == nil {
 			t.Errorf("Test %q failed: returned success but expected error", name)
 		}
@@ -244,7 +246,9 @@ func TestGCSCredentialBuilder(t *testing.T) {
 		g.Expect(c.Create(context.TODO(), existingGCSSecret)).NotTo(gomega.HaveOccurred())
 
 		err := builder.CreateSecretVolumeAndEnv(scenario.serviceAccount.Namespace, scenario.serviceAccount.Name,
-			scenario.inputConfiguration)
+			scenario.inputConfiguration.Spec.RevisionTemplate.Spec.Container,
+			&scenario.inputConfiguration.Spec.RevisionTemplate.Spec.Volumes,
+		)
 		if scenario.shouldFail && err == nil {
 			t.Errorf("Test %q failed: returned success but expected error", name)
 		}

diff --git a/pkg/controller/kfservice/resources/knative/configuration.go b/pkg/controller/kfservice/resources/knative/configuration.go
@@ -115,7 +115,8 @@ func (c *ConfigurationBuilder) CreateKnativeConfiguration(name string, metadata
 	if err := c.credentialBuilder.CreateSecretVolumeAndEnv(
 		metadata.Namespace,
 		modelSpec.ServiceAccountName,
-		configuration,
+		configuration.Spec.RevisionTemplate.Spec.Container,
+		&configuration.Spec.RevisionTemplate.Spec.Volumes,
 	); err != nil {
 		return nil, err
 	}

diff --git a/pkg/webhook/admission/deployment/model_initializer_injector.go b/pkg/webhook/admission/deployment/model_initializer_injector.go
@@ -18,6 +18,7 @@ import (
 	"strings"
 
 	"github.com/kubeflow/kfserving/pkg/constants"
+	"github.com/kubeflow/kfserving/pkg/controller/kfservice/resources/credentials"
 
 	appsv1 "k8s.io/api/apps/v1"
 	v1 "k8s.io/api/core/v1"
@@ -34,11 +35,15 @@ const (
 	UserContainerName                = "user-container"
 )
 
+type ModelInitializerInjector struct {
+	credentialBuilder *credentials.CredentialBuilder
+}
+
 // InjectModelInitializer injects an init container to provision model data
 // for the serving container in a unified way across storage tech by injecting
 // a provisioning INIT container. This is a work around because KNative does not
 // support INIT containers: https://github.com/knative/serving/issues/4307
-func InjectModelInitializer(deployment *appsv1.Deployment) error {
+func (mi *ModelInitializerInjector) InjectModelInitializer(deployment *appsv1.Deployment) error {
 
 	annotations := deployment.Spec.Template.ObjectMeta.Annotations
 	podSpec := &deployment.Spec.Template.Spec
@@ -114,7 +119,7 @@ func InjectModelInitializer(deployment *appsv1.Deployment) error {
 	modelInitializerMounts = append(modelInitializerMounts, sharedVolumeWriteMount)
 
 	// Add an init container to run provisioning logic to the PodSpec
-	initContianer := v1.Container{
+	initContainer := &v1.Container{
 		Name:  ModelInitializerContainerName,
 		Image: ModelInitializerContainerImage + ":" + ModelInitializerContainerVersion,
 		Args: []string{
@@ -123,7 +128,6 @@ func InjectModelInitializer(deployment *appsv1.Deployment) error {
 		},
 		VolumeMounts: modelInitializerMounts,
 	}
-	podSpec.InitContainers = append(podSpec.InitContainers, initContianer)
 
 	// Add a mount the shared volume on the user-container, update the PodSpec
 	sharedVolumeReadMount := v1.VolumeMount{
@@ -136,6 +140,19 @@ func InjectModelInitializer(deployment *appsv1.Deployment) error {
 	// Add volumes to the PodSpec
 	podSpec.Volumes = append(podSpec.Volumes, podVolumes...)
 
+	// Inject credentials
+	if err := mi.credentialBuilder.CreateSecretVolumeAndEnv(
+		deployment.Namespace,
+		podSpec.ServiceAccountName,
+		initContainer,
+		&podSpec.Volumes,
+	); err != nil {
+		return err
+	}
+
+	// Add init container to the spec
+	podSpec.InitContainers = append(podSpec.InitContainers, *initContainer)
+
 	return nil
 }