feat: Supports image pull secrets (opendatahub-io#276)

#### Motivation fix opendatahub-io#214 #### Modifications 1. Update servingRuntime CRDs to include imagePullSecrets 2. pass imagePullSecrets from Configuration and ServingRuntimePodSpec to deployment.Spec.Template and add tests 3. add docs in Configuration #### Result The user is able to pass imagePullSecrets from configuration and servingRuntime to runtime pod and uses images from the private image registry. Signed-off-by: Lize Cai <lize.cai@sap.com>
HumairAK · Nov 30, 2022 · e84df69 · e84df69
1 parent e3d662c
commit e84df69
Show file tree

Hide file tree

Showing 12 changed files with 157 additions and 4 deletions.
diff --git a/config/crd/bases/serving.kserve.io_clusterservingruntimes.yaml b/config/crd/bases/serving.kserve.io_clusterservingruntimes.yaml
@@ -1,4 +1,4 @@
-# Copied from https://github.com/kserve/kserve/blob/532989a2ccef63ff912008359540b0937596f55c/config/crd/serving.kserve.io_clusterservingruntimes.yaml
+# Copied from https://github.com/kserve/kserve/blob/335dfbcc461a3b2127354aeb7df2414fb11ddfe3/config/crd/serving.kserve.io_clusterservingruntimes.yaml
 #
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
@@ -1065,6 +1065,13 @@ spec:
                   type: string
                 httpDataEndpoint:
                   type: string
+                imagePullSecrets:
+                  items:
+                    properties:
+                      name:
+                        type: string
+                    type: object
+                  type: array
                 labels:
                   additionalProperties:
                     type: string

diff --git a/config/crd/bases/serving.kserve.io_servingruntimes.yaml b/config/crd/bases/serving.kserve.io_servingruntimes.yaml
@@ -1,4 +1,4 @@
-# Copied from https://github.com/kserve/kserve/blob/532989a2ccef63ff912008359540b0937596f55c/config/crd/serving.kserve.io_servingruntimes.yaml
+# Copied from https://github.com/kserve/kserve/blob/335dfbcc461a3b2127354aeb7df2414fb11ddfe3/config/crd/serving.kserve.io_servingruntimes.yaml
 #
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
@@ -1065,6 +1065,13 @@ spec:
                   type: string
                 httpDataEndpoint:
                   type: string
+                imagePullSecrets:
+                  items:
+                    properties:
+                      name:
+                        type: string
+                    type: object
+                  type: array
                 labels:
                   additionalProperties:
                     type: string

diff --git a/controllers/modelmesh/modelmesh.go b/controllers/modelmesh/modelmesh.go
@@ -34,7 +34,7 @@ const logYaml = true
 
 const ModelMeshEtcdPrefix = "mm"
 
-//Models a deployment
+// Models a deployment
 type Deployment struct {
 	ServiceName        string
 	ServicePort        uint16
@@ -74,6 +74,7 @@ type Deployment struct {
 	AnnotationConfigMap *corev1.ConfigMap
 	AnnotationsMap      map[string]string
 	LabelsMap           map[string]string
+	ImagePullSecrets    []corev1.LocalObjectReference
 	EnableAccessLogging bool
 	Client              client.Client
 }
@@ -128,6 +129,7 @@ func (m *Deployment) Apply(ctx context.Context) error {
 				m.configureRuntimePodSpecAnnotations,
 				m.configureRuntimePodSpecLabels,
 				m.ensureMMContainerIsLast,
+				m.configureRuntimePodSpecImagePullSecrets,
 			); tErr != nil {
 				return tErr
 			}

diff --git a/controllers/modelmesh/runtime.go b/controllers/modelmesh/runtime.go
@@ -372,3 +372,14 @@ func (m *Deployment) configureRuntimePodSpecLabels(deployment *appsv1.Deployment
 	deployment.Spec.Template.Labels = Union(deployment.Spec.Template.Labels, m.LabelsMap, m.SRSpec.ServingRuntimePodSpec.Labels)
 	return nil
 }
+
+func (m *Deployment) configureRuntimePodSpecImagePullSecrets(deployment *appsv1.Deployment) error {
+	// merging the image pull secrets from deploymentTemplate (if any), configMap and ServingRuntimePodSpec
+	deployment.Spec.Template.Spec.ImagePullSecrets = mergeImagePullSecrets(
+		deployment.Spec.Template.Spec.ImagePullSecrets,
+		m.ImagePullSecrets,
+		m.SRSpec.ServingRuntimePodSpec.ImagePullSecrets,
+	)
+
+	return nil
+}
diff --git a/controllers/modelmesh/runtime_test.go b/controllers/modelmesh/runtime_test.go
@@ -504,3 +504,82 @@ func TestConfigureRuntimeLabels(t *testing.T) {
 		assert.Equal(t, deploy.Spec.Template.Labels["cp4s-internet"], "allow")
 	})
 }
+
+func TestConfigureRuntimeImagePullSecrets(t *testing.T) {
+	t.Run("success-set-image-pull-secrets", func(t *testing.T) {
+		deploy := &appsv1.Deployment{}
+		sr := &kserveapi.ServingRuntime{}
+		imagePullSecretsData := []v1.LocalObjectReference{
+			{Name: "config-image-pull-secret-1"},
+			{Name: "config-image-pull-secret-2"},
+		}
+
+		m := Deployment{Owner: sr, ImagePullSecrets: imagePullSecretsData, SRSpec: &sr.Spec}
+
+		err := m.configureRuntimePodSpecImagePullSecrets(deploy)
+		assert.Nil(t, err)
+		assert.Equal(t, deploy.Spec.Template.Spec.ImagePullSecrets[0].Name, "config-image-pull-secret-1")
+		assert.Equal(t, deploy.Spec.Template.Spec.ImagePullSecrets[1].Name, "config-image-pull-secret-2")
+	})
+
+	t.Run("success-no-image-pull-secrets", func(t *testing.T) {
+		deploy := &appsv1.Deployment{}
+		sr := &kserveapi.ServingRuntime{}
+		m := Deployment{Owner: sr, SRSpec: &sr.Spec}
+
+		err := m.configureRuntimePodSpecImagePullSecrets(deploy)
+		assert.Nil(t, err)
+		assert.Empty(t, deploy.Spec.Template.Spec.ImagePullSecrets)
+	})
+
+	t.Run("success-set-image-pull-secrets-from-servingruntime-spec", func(t *testing.T) {
+		deploy := &appsv1.Deployment{}
+		sr := &kserveapi.ServingRuntime{
+			Spec: kserveapi.ServingRuntimeSpec{
+				ServingRuntimePodSpec: kserveapi.ServingRuntimePodSpec{
+					ImagePullSecrets: []v1.LocalObjectReference{
+						{Name: "sr-image-pull-secret-1"},
+						{Name: "sr-image-pull-secret-2"},
+					},
+				},
+			},
+		}
+
+		m := Deployment{Owner: sr, SRSpec: &sr.Spec}
+
+		err := m.configureRuntimePodSpecImagePullSecrets(deploy)
+		assert.Nil(t, err)
+		assert.Equal(t, deploy.Spec.Template.Spec.ImagePullSecrets[0].Name, "sr-image-pull-secret-1")
+		assert.Equal(t, deploy.Spec.Template.Spec.ImagePullSecrets[1].Name, "sr-image-pull-secret-2")
+	})
+
+	t.Run("success-set-image-pull-secrets-from-config-and-servingruntime-spec", func(t *testing.T) {
+		deploy := &appsv1.Deployment{}
+		imagePullSecretsData := []v1.LocalObjectReference{
+			{Name: "config-image-pull-secret-1"},
+			{Name: "config-image-pull-secret-2"},
+			{Name: "overlap-image-pull-secret"},
+		}
+		sr := &kserveapi.ServingRuntime{
+			Spec: kserveapi.ServingRuntimeSpec{
+				ServingRuntimePodSpec: kserveapi.ServingRuntimePodSpec{
+					ImagePullSecrets: []v1.LocalObjectReference{
+						{Name: "overlap-image-pull-secret"},
+						{Name: "sr-image-pull-secret-1"},
+						{Name: "sr-image-pull-secret-2"},
+					},
+				},
+			},
+		}
+
+		m := Deployment{Owner: sr, ImagePullSecrets: imagePullSecretsData, SRSpec: &sr.Spec}
+
+		err := m.configureRuntimePodSpecImagePullSecrets(deploy)
+		assert.Nil(t, err)
+		assert.Equal(t, deploy.Spec.Template.Spec.ImagePullSecrets[0].Name, "config-image-pull-secret-1")
+		assert.Equal(t, deploy.Spec.Template.Spec.ImagePullSecrets[1].Name, "config-image-pull-secret-2")
+		assert.Equal(t, deploy.Spec.Template.Spec.ImagePullSecrets[2].Name, "overlap-image-pull-secret")
+		assert.Equal(t, deploy.Spec.Template.Spec.ImagePullSecrets[3].Name, "sr-image-pull-secret-1")
+		assert.Equal(t, deploy.Spec.Template.Spec.ImagePullSecrets[4].Name, "sr-image-pull-secret-2")
+	})
+}
diff --git a/controllers/modelmesh/util.go b/controllers/modelmesh/util.go
@@ -135,3 +135,26 @@ func mountPoint(rts *kserveapi.ServingRuntimeSpec) (bool, string, error) {
 
 	return false, "", nil
 }
+
+// mergeImagePullSecrets merge image pull secret lists and remove duplicates
+func mergeImagePullSecrets(secrets ...[]corev1.LocalObjectReference) []corev1.LocalObjectReference {
+	imagePullSecrets := []corev1.LocalObjectReference{}
+
+	// remove the duplicated secrets and keep the order of the secret in the list
+	for _, secret := range secrets {
+		for _, v := range secret {
+			exist := false
+			for _, ips := range imagePullSecrets {
+				if ips.Name == v.Name {
+					exist = true
+					break
+				}
+			}
+			if !exist {
+				imagePullSecrets = append(imagePullSecrets, corev1.LocalObjectReference{Name: v.Name})
+			}
+		}
+	}
+
+	return imagePullSecrets
+}
diff --git a/controllers/servingruntime_controller.go b/controllers/servingruntime_controller.go
@@ -258,6 +258,7 @@ func (r *ServingRuntimeReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		Client:              r.Client,
 		AnnotationsMap:      cfg.RuntimePodAnnotations,
 		LabelsMap:           cfg.RuntimePodLabels,
+		ImagePullSecrets:    cfg.ImagePullSecrets,
 	}
 	// if the runtime is disabled, delete the deployment
 	if spec.IsDisabled() || !spec.IsMultiModelRuntime() || !mmEnabled {

diff --git a/docs/configuration/README.md b/docs/configuration/README.md
@@ -52,6 +52,7 @@ The following parameters are currently supported. _Note_ the keys are expressed
 | `restProxy.port`                           | Port on which the REST proxy to serve REST requests                                                   | `8008`                                     |
 | `runtimePodLabels`                         | `metadata.labels` to be added to all `ServingRuntime` pods                                            | (\*\*\*\*\*) See default labels below      |
 | `runtimePodAnnotations`                    | `metadata.annotations` to be added to all `ServingRuntime` pods                                       | (\*\*\*\*\*) See default annotations below |
+| `imagePullSecrets`                         | The image pull secrets to use for runtime Pods                                                        |                                            |
 
 (\*) Currently requires a controller restart to take effect
 

diff --git a/go.mod b/go.mod
@@ -7,7 +7,7 @@ require (
 	github.com/go-logr/logr v1.2.3
 	github.com/golang/protobuf v1.5.2
 	github.com/google/go-cmp v0.5.8
-	github.com/kserve/kserve v0.9.1-0.20221002134029-532989a2ccef
+	github.com/kserve/kserve v0.9.1-0.20221013012311-335dfbcc461a
 	github.com/manifestival/controller-runtime-client v0.4.0
 	github.com/manifestival/manifestival v0.7.1
 	github.com/moverest/mnist v0.0.0-20160628192128-ec5d9d203b59

diff --git a/go.sum b/go.sum
@@ -503,6 +503,8 @@ github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
 github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
 github.com/kserve/kserve v0.9.1-0.20221002134029-532989a2ccef h1:K3KVPI2w6D4L2VZVDIV+8ArYD7Bo4G1bzUXieQhHX+I=
 github.com/kserve/kserve v0.9.1-0.20221002134029-532989a2ccef/go.mod h1:OgRx84UYmcNeM4Q6uuxHHypQRc8J5v5caGsuyAsbAWQ=
+github.com/kserve/kserve v0.9.1-0.20221013012311-335dfbcc461a h1:gJwJj2hiZ+D0+C4FE9bMwHof8WdWYBjZw5H6F9ACLbY=
+github.com/kserve/kserve v0.9.1-0.20221013012311-335dfbcc461a/go.mod h1:vC6wtbGo6xh9a445wVTTmiZbbF3TbXutIjgSeeH19u4=
 github.com/logrusorgru/aurora/v3 v3.0.0 h1:R6zcoZZbvVcGMvDCKo45A9U/lzYyzl5NfYIvznmDfE4=
 github.com/logrusorgru/aurora/v3 v3.0.0/go.mod h1:vsR12bk5grlLvLXAYrBsb5Oc/N+LxAlxggSjiwMnCUc=
 github.com/magiconair/properties v1.8.0/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ=

diff --git a/pkg/config/config.go b/pkg/config/config.go
@@ -84,6 +84,8 @@ type Config struct {
 	RuntimePodLabels      map[string]string
 	RuntimePodAnnotations map[string]string
 
+	ImagePullSecrets []corev1.LocalObjectReference
+
 	// For internal use only
 	InternalModelMeshEnvVars EnvVarList
 }

diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go
@@ -293,3 +293,21 @@ internalModelMeshEnvVars:
 		t.Fatalf("Expected InternalModelMeshEnvVars to have env var with value [%s], but got [%s]", expectedValue, envvar.Value)
 	}
 }
+
+func TestImagePullSecrets(t *testing.T) {
+	yaml := `
+imagePullSecrets:
+  - name: "config-image-pull-secret"
+`
+	expectedSecretName := "config-image-pull-secret"
+
+	conf, err := NewMergedConfigFromString(yaml)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	secret := conf.ImagePullSecrets[0]
+	if secret.Name != expectedSecretName {
+		t.Fatalf("Expected ImagePullSecrets to have secret with name [%s], but got [%s]", expectedSecretName, secret.Name)
+	}
+}