From faf4dfd3e74f050b4e50df95861e95682abe9e3a Mon Sep 17 00:00:00 2001 From: Reto Lehmann Date: Tue, 11 Jul 2023 08:09:36 +0200 Subject: [PATCH 1/5] add storage-initializer uid handling for OpenShift with istio-cni --- .../admission/pod/accelerator_injector.go | 2 +- .../pod/accelerator_injector_test.go | 2 +- pkg/webhook/admission/pod/agent_injector.go | 2 +- .../admission/pod/agent_injector_test.go | 2 +- .../pod/metrics_aggregate_injector.go | 3 +- .../pod/metrics_aggregate_injector_test.go | 2 +- pkg/webhook/admission/pod/mutator.go | 15 +++++++--- .../pod/storage_initializer_injector.go | 29 ++++++++++++++----- .../pod/storage_initializer_injector_test.go | 23 ++++++++++----- 9 files changed, 56 insertions(+), 24 deletions(-) diff --git a/pkg/webhook/admission/pod/accelerator_injector.go b/pkg/webhook/admission/pod/accelerator_injector.go index 205c1edd12f..14bdfc635cc 100644 --- a/pkg/webhook/admission/pod/accelerator_injector.go +++ b/pkg/webhook/admission/pod/accelerator_injector.go @@ -28,7 +28,7 @@ const ( NvidiaGPUTaintValue = "present" ) -func InjectGKEAcceleratorSelector(pod *v1.Pod) error { +func InjectGKEAcceleratorSelector(pod *v1.Pod, _ *v1.Namespace) error { gpuEnabled := false for _, container := range pod.Spec.Containers { if _, ok := container.Resources.Limits[constants.NvidiaGPUResourceType]; ok { diff --git a/pkg/webhook/admission/pod/accelerator_injector_test.go b/pkg/webhook/admission/pod/accelerator_injector_test.go index 7db7aeb1bc8..31558a80e13 100644 --- a/pkg/webhook/admission/pod/accelerator_injector_test.go +++ b/pkg/webhook/admission/pod/accelerator_injector_test.go @@ -94,7 +94,7 @@ func TestAcceleratorInjector(t *testing.T) { } for name, scenario := range scenarios { - InjectGKEAcceleratorSelector(scenario.original) + InjectGKEAcceleratorSelector(scenario.original, nil) // cmd.Diff complains on ResourceList when Nvidia is key. Objects are explicitly compared if diff := cmp.Diff( scenario.expected.Spec.NodeSelector, diff --git a/pkg/webhook/admission/pod/agent_injector.go b/pkg/webhook/admission/pod/agent_injector.go index c783ec0feb7..430d4148098 100644 --- a/pkg/webhook/admission/pod/agent_injector.go +++ b/pkg/webhook/admission/pod/agent_injector.go @@ -116,7 +116,7 @@ func getLoggerConfigs(configMap *v1.ConfigMap) (*LoggerConfig, error) { return loggerConfig, nil } -func (ag *AgentInjector) InjectAgent(pod *v1.Pod) error { +func (ag *AgentInjector) InjectAgent(pod *v1.Pod, _ *v1.Namespace) error { // Only inject the model agent sidecar if the required annotations are set _, injectLogger := pod.ObjectMeta.Annotations[constants.LoggerInternalAnnotationKey] _, injectPuller := pod.ObjectMeta.Annotations[constants.AgentShouldInjectAnnotationKey] diff --git a/pkg/webhook/admission/pod/agent_injector_test.go b/pkg/webhook/admission/pod/agent_injector_test.go index 0d196343052..a028c5e7916 100644 --- a/pkg/webhook/admission/pod/agent_injector_test.go +++ b/pkg/webhook/admission/pod/agent_injector_test.go @@ -1112,7 +1112,7 @@ func TestAgentInjector(t *testing.T) { loggerConfig, batcherTestConfig, } - injector.InjectAgent(scenario.original) + injector.InjectAgent(scenario.original, nil) if diff, _ := kmp.SafeDiff(scenario.expected.Spec, scenario.original.Spec); diff != "" { t.Errorf("Test %q unexpected result (-want +got): %v", name, diff) } diff --git a/pkg/webhook/admission/pod/metrics_aggregate_injector.go b/pkg/webhook/admission/pod/metrics_aggregate_injector.go index 3a5d6fa3569..c881c800874 100644 --- a/pkg/webhook/admission/pod/metrics_aggregate_injector.go +++ b/pkg/webhook/admission/pod/metrics_aggregate_injector.go @@ -19,6 +19,7 @@ package pod import ( "encoding/json" "fmt" + "github.com/kserve/kserve/pkg/constants" v1 "k8s.io/api/core/v1" ) @@ -75,7 +76,7 @@ func setMetricAggregationEnvVars(pod *v1.Pod) { // InjectMetricsAggregator looks for the annotations to enable aggregate kserve-container and queue-proxy metrics and // if specified, sets port-related EnvVars in queue-proxy and the aggregate prometheus annotation. -func (ma *MetricsAggregator) InjectMetricsAggregator(pod *v1.Pod) error { +func (ma *MetricsAggregator) InjectMetricsAggregator(pod *v1.Pod, _ *v1.Namespace) error { //Only set metric configs if the required annotations are set enableMetricAggregation, ok := pod.ObjectMeta.Annotations[constants.EnableMetricAggregation] if !ok { diff --git a/pkg/webhook/admission/pod/metrics_aggregate_injector_test.go b/pkg/webhook/admission/pod/metrics_aggregate_injector_test.go index e1a0a3b99f0..47572690288 100644 --- a/pkg/webhook/admission/pod/metrics_aggregate_injector_test.go +++ b/pkg/webhook/admission/pod/metrics_aggregate_injector_test.go @@ -290,7 +290,7 @@ func TestInjectMetricsAggregator(t *testing.T) { } for name, scenario := range scenarios { - ma.InjectMetricsAggregator(scenario.original) + ma.InjectMetricsAggregator(scenario.original, nil) if diff, _ := kmp.SafeDiff(scenario.expected.Spec, scenario.original.Spec); diff != "" { t.Errorf("Test %q unexpected result (-want +got): %v", name, diff) } diff --git a/pkg/webhook/admission/pod/mutator.go b/pkg/webhook/admission/pod/mutator.go index c320967f3c1..e4d98852fe7 100644 --- a/pkg/webhook/admission/pod/mutator.go +++ b/pkg/webhook/admission/pod/mutator.go @@ -62,7 +62,14 @@ func (mutator *Mutator) Handle(ctx context.Context, req admission.Request) admis // For some reason pod namespace is always empty when coming to pod mutator, need to set from admission request pod.Namespace = req.AdmissionRequest.Namespace - if err := mutator.mutate(pod, configMap); err != nil { + targetNs := &v1.Namespace{} + err = mutator.Client.Get(context.TODO(), k8types.NamespacedName{Name: pod.Namespace, Namespace: pod.Namespace}, targetNs) + if err != nil { + log.Error(err, "Failed to get the target namespace", "name", pod.Namespace) + return admission.Errored(http.StatusInternalServerError, err) + } + + if err := mutator.mutate(pod, configMap, targetNs); err != nil { log.Error(err, "Failed to mutate pod", "name", pod.Labels[constants.InferenceServicePodLabelKey]) return admission.Errored(http.StatusInternalServerError, err) } @@ -76,7 +83,7 @@ func (mutator *Mutator) Handle(ctx context.Context, req admission.Request) admis return admission.PatchResponseFromRaw(req.AdmissionRequest.Object.Raw, patch) } -func (mutator *Mutator) mutate(pod *v1.Pod, configMap *v1.ConfigMap) error { +func (mutator *Mutator) mutate(pod *v1.Pod, configMap *v1.ConfigMap, targetNs *v1.Namespace) error { credentialBuilder := credentials.NewCredentialBulder(mutator.Client, configMap) storageInitializerConfig, err := getStorageInitializerConfigs(configMap) @@ -116,7 +123,7 @@ func (mutator *Mutator) mutate(pod *v1.Pod, configMap *v1.ConfigMap) error { return err } - mutators := []func(pod *v1.Pod) error{ + mutators := []func(pod *v1.Pod, targetNs *v1.Namespace) error{ InjectGKEAcceleratorSelector, storageInitializer.InjectStorageInitializer, agentInjector.InjectAgent, @@ -124,7 +131,7 @@ func (mutator *Mutator) mutate(pod *v1.Pod, configMap *v1.ConfigMap) error { } for _, mutator := range mutators { - if err := mutator(pod); err != nil { + if err := mutator(pod, targetNs); err != nil { return err } } diff --git a/pkg/webhook/admission/pod/storage_initializer_injector.go b/pkg/webhook/admission/pod/storage_initializer_injector.go index fb7aa032ab2..f6495341399 100644 --- a/pkg/webhook/admission/pod/storage_initializer_injector.go +++ b/pkg/webhook/admission/pod/storage_initializer_injector.go @@ -41,6 +41,7 @@ const ( PvcURIPrefix = "pvc://" PvcSourceMountName = "kserve-pvc-source" PvcSourceMountPath = "/mnt/pvc" + OpenShiftUidRangeAnnotationKey = "openshift.io/sa.scc.uid-range" ) type StorageInitializerConfig struct { @@ -85,7 +86,7 @@ func getStorageInitializerConfigs(configMap *v1.ConfigMap) (*StorageInitializerC // 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 (mi *StorageInitializerInjector) InjectStorageInitializer(pod *v1.Pod) error { +func (mi *StorageInitializerInjector) InjectStorageInitializer(pod *v1.Pod, targetNs *v1.Namespace) error { // Only inject if the required annotations are set srcURI, ok := pod.ObjectMeta.Annotations[constants.StorageInitializerSourceUriInternalAnnotationKey] if !ok { @@ -330,12 +331,26 @@ func (mi *StorageInitializerInjector) InjectStorageInitializer(pod *v1.Pod) erro } } - // Allow to override the uid for the case where ISTIO CNI with DNS proxy is enabled - // See for more: https://istio.io/latest/docs/setup/additional-setup/cni/#compatibility-with-application-init-containers. - if value, ok := pod.GetAnnotations()[constants.IstioSidecarUIDAnnotationKey]; ok { - if uid, err := strconv.ParseInt(value, 10, 64); err == nil { - initContainer.SecurityContext.RunAsUser = ptr.Int64(uid) - } + /* + OpenShift uses istio-cni which causes an issue with init-containers when calling external services + like S3 or similar. Setting the `uid` for the `storage-initializer` to the same `uid` as the + `uid` of the `istio-proxy` resolves the issue. In OpenShift the `istio-proxy` always gets assigned + the first `uid` from the namespaces `uid` range + 1 (The range is defined in an annotation on the namespace). + */ + if initContainer.SecurityContext == nil { + initContainer.SecurityContext = &v1.SecurityContext{} + } + uidStr := targetNs.Annotations[OpenShiftUidRangeAnnotationKey] + if uidStr == "" { + return fmt.Errorf("could not find annotation %s on namespace: %s", OpenShiftUidRangeAnnotationKey, targetNs.Name) + } + uidStrParts := strings.Split(uidStr, "/") + if uid, err := strconv.ParseInt(uidStrParts[0], 10, 64); err == nil { + // Set the uid to the first uid in the namespaces range + 1 + uid++ + initContainer.SecurityContext.RunAsUser = ptr.Int64(uid) + } else { + return fmt.Errorf("could not parse value %s in annotation %s on namespace: %s", uidStr, OpenShiftUidRangeAnnotationKey, targetNs.Name) } // Add init container to the spec diff --git a/pkg/webhook/admission/pod/storage_initializer_injector_test.go b/pkg/webhook/admission/pod/storage_initializer_injector_test.go index 80456344032..8bba589ed34 100644 --- a/pkg/webhook/admission/pod/storage_initializer_injector_test.go +++ b/pkg/webhook/admission/pod/storage_initializer_injector_test.go @@ -62,6 +62,15 @@ var ( v1.ResourceMemory: resource.MustParse(StorageInitializerDefaultMemoryRequest), }, } + + targetNS = &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "my-ns", + Annotations: map[string]string{ + OpenShiftUidRangeAnnotationKey: "1000740000/10000", + }, + }, + } ) func TestStorageInitializerInjector(t *testing.T) { @@ -354,7 +363,7 @@ func TestStorageInitializerInjector(t *testing.T) { }), config: storageInitializerConfig, } - if err := injector.InjectStorageInitializer(scenario.original); err != nil { + if err := injector.InjectStorageInitializer(scenario.original, targetNS); err != nil { t.Errorf("Test %q unexpected result: %s", name, err) } if diff, _ := kmp.SafeDiff(scenario.expected.Spec, scenario.original.Spec); diff != "" { @@ -394,7 +403,7 @@ func TestStorageInitializerFailureCases(t *testing.T) { }), config: storageInitializerConfig, } - if err := injector.InjectStorageInitializer(scenario.original); err != nil { + if err := injector.InjectStorageInitializer(scenario.original, targetNS); err != nil { if !strings.HasPrefix(err.Error(), scenario.expectedErrorPrefix) { t.Errorf("Test %q unexpected failure [%s], expected: %s", name, err.Error(), scenario.expectedErrorPrefix) } @@ -493,7 +502,7 @@ func TestCustomSpecStorageUriInjection(t *testing.T) { }), config: storageInitializerConfig, } - if err := injector.InjectStorageInitializer(scenario.original); err != nil { + if err := injector.InjectStorageInitializer(scenario.original, targetNS); err != nil { t.Errorf("Test %q unexpected result: %s", name, err) } @@ -944,7 +953,7 @@ func TestCredentialInjection(t *testing.T) { credentialBuilder: builder, config: storageInitializerConfig, } - if err := injector.InjectStorageInitializer(scenario.original); err != nil { + if err := injector.InjectStorageInitializer(scenario.original, targetNS); err != nil { t.Errorf("Test %q unexpected failure [%s]", name, err.Error()) } if diff, _ := kmp.SafeDiff(scenario.expected.Spec, scenario.original.Spec); diff != "" { @@ -1037,7 +1046,7 @@ func TestStorageInitializerConfigmap(t *testing.T) { StorageSpecSecretName: StorageInitializerDefaultStorageSpecSecretName, }, } - if err := injector.InjectStorageInitializer(scenario.original); err != nil { + if err := injector.InjectStorageInitializer(scenario.original, targetNS); err != nil { t.Errorf("Test %q unexpected result: %s", name, err) } if diff, _ := kmp.SafeDiff(scenario.expected.Spec, scenario.original.Spec); diff != "" { @@ -1272,7 +1281,7 @@ func TestDirectVolumeMountForPvc(t *testing.T) { EnableDirectPvcVolumeMount: true, // enable direct volume mount for PVC }, } - if err := injector.InjectStorageInitializer(scenario.original); err != nil { + if err := injector.InjectStorageInitializer(scenario.original, targetNS); err != nil { t.Errorf("Test %q unexpected result: %s", name, err) } if diff, _ := kmp.SafeDiff(scenario.expected.Spec, scenario.original.Spec); diff != "" { @@ -1594,7 +1603,7 @@ func TestTransformerCollocation(t *testing.T) { }), config: scenario.storageConfig, } - if err := injector.InjectStorageInitializer(scenario.original); err != nil { + if err := injector.InjectStorageInitializer(scenario.original, targetNS); err != nil { t.Errorf("Test %q unexpected result: %s", name, err) } if diff, _ := kmp.SafeDiff(scenario.expected.Spec, scenario.original.Spec); diff != "" { From dff4697d9ef2fe89ef21885cb0d9fe2ccbf962d7 Mon Sep 17 00:00:00 2001 From: Reto Lehmann Date: Tue, 11 Jul 2023 10:11:49 +0200 Subject: [PATCH 2/5] update storage_initializer_injector tests --- .../pod/storage_initializer_injector_test.go | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/pkg/webhook/admission/pod/storage_initializer_injector_test.go b/pkg/webhook/admission/pod/storage_initializer_injector_test.go index 8bba589ed34..cac50b9e654 100644 --- a/pkg/webhook/admission/pod/storage_initializer_injector_test.go +++ b/pkg/webhook/admission/pod/storage_initializer_injector_test.go @@ -22,6 +22,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" "knative.dev/pkg/kmp" + "knative.dev/pkg/ptr" "github.com/kserve/kserve/pkg/constants" "github.com/kserve/kserve/pkg/credentials" @@ -71,6 +72,8 @@ var ( }, }, } + + expectedInitContainerUid = ptr.Int64(1000740001) ) func TestStorageInitializerInjector(t *testing.T) { @@ -191,6 +194,9 @@ func TestStorageInitializerInjector(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -261,6 +267,9 @@ func TestStorageInitializerInjector(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -341,6 +350,9 @@ func TestStorageInitializerInjector(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -606,6 +618,9 @@ func TestCredentialInjection(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, Env: []v1.EnvVar{ { Name: s3.AWSAccessKeyId, @@ -711,6 +726,9 @@ func TestCredentialInjection(t *testing.T) { MountPath: gcs.GCSCredentialVolumeMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, Env: []v1.EnvVar{ { Name: gcs.GCSCredentialEnvKey, @@ -799,6 +817,9 @@ func TestCredentialInjection(t *testing.T) { Name: "storage-initializer", Image: StorageInitializerContainerImage + ":" + StorageInitializerContainerImageVersion, Args: []string{"s3://my-bucket/foo/bar", constants.DefaultModelLocalMountPath}, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, Env: []v1.EnvVar{ { Name: credentials.StorageConfigEnvKey, @@ -894,6 +915,9 @@ func TestCredentialInjection(t *testing.T) { Name: "storage-initializer", Image: StorageInitializerContainerImage + ":" + StorageInitializerContainerImageVersion, Args: []string{"s3://my-bucket/foo/bar", constants.DefaultModelLocalMountPath}, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, Env: []v1.EnvVar{ { Name: credentials.StorageConfigEnvKey, @@ -1017,6 +1041,9 @@ func TestStorageInitializerConfigmap(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -1394,6 +1421,9 @@ func TestTransformerCollocation(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -1572,6 +1602,9 @@ func TestTransformerCollocation(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ From a6c1c563b91c019353078faca8b2ba139f0fadbd Mon Sep 17 00:00:00 2001 From: Reto Lehmann Date: Tue, 11 Jul 2023 10:18:14 +0200 Subject: [PATCH 3/5] Add more context to error message --- pkg/webhook/admission/pod/storage_initializer_injector.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/webhook/admission/pod/storage_initializer_injector.go b/pkg/webhook/admission/pod/storage_initializer_injector.go index f6495341399..0b90fbe5674 100644 --- a/pkg/webhook/admission/pod/storage_initializer_injector.go +++ b/pkg/webhook/admission/pod/storage_initializer_injector.go @@ -342,7 +342,7 @@ func (mi *StorageInitializerInjector) InjectStorageInitializer(pod *v1.Pod, targ } uidStr := targetNs.Annotations[OpenShiftUidRangeAnnotationKey] if uidStr == "" { - return fmt.Errorf("could not find annotation %s on namespace: %s", OpenShiftUidRangeAnnotationKey, targetNs.Name) + return fmt.Errorf("could not find OpenShift internal annotation %s on namespace: %s", OpenShiftUidRangeAnnotationKey, targetNs.Name) } uidStrParts := strings.Split(uidStr, "/") if uid, err := strconv.ParseInt(uidStrParts[0], 10, 64); err == nil { From 60bcc0fc325feb2451f8eec1da405a749937b97a Mon Sep 17 00:00:00 2001 From: Reto Lehmann Date: Tue, 11 Jul 2023 11:39:41 +0200 Subject: [PATCH 4/5] update mutator and error message --- pkg/webhook/admission/pod/accelerator_injector.go | 2 +- pkg/webhook/admission/pod/accelerator_injector_test.go | 2 +- pkg/webhook/admission/pod/agent_injector.go | 2 +- pkg/webhook/admission/pod/agent_injector_test.go | 2 +- pkg/webhook/admission/pod/metrics_aggregate_injector.go | 2 +- .../admission/pod/metrics_aggregate_injector_test.go | 2 +- pkg/webhook/admission/pod/mutator.go | 8 +++++--- pkg/webhook/admission/pod/storage_initializer_injector.go | 4 ++-- 8 files changed, 13 insertions(+), 11 deletions(-) diff --git a/pkg/webhook/admission/pod/accelerator_injector.go b/pkg/webhook/admission/pod/accelerator_injector.go index 14bdfc635cc..205c1edd12f 100644 --- a/pkg/webhook/admission/pod/accelerator_injector.go +++ b/pkg/webhook/admission/pod/accelerator_injector.go @@ -28,7 +28,7 @@ const ( NvidiaGPUTaintValue = "present" ) -func InjectGKEAcceleratorSelector(pod *v1.Pod, _ *v1.Namespace) error { +func InjectGKEAcceleratorSelector(pod *v1.Pod) error { gpuEnabled := false for _, container := range pod.Spec.Containers { if _, ok := container.Resources.Limits[constants.NvidiaGPUResourceType]; ok { diff --git a/pkg/webhook/admission/pod/accelerator_injector_test.go b/pkg/webhook/admission/pod/accelerator_injector_test.go index 31558a80e13..7db7aeb1bc8 100644 --- a/pkg/webhook/admission/pod/accelerator_injector_test.go +++ b/pkg/webhook/admission/pod/accelerator_injector_test.go @@ -94,7 +94,7 @@ func TestAcceleratorInjector(t *testing.T) { } for name, scenario := range scenarios { - InjectGKEAcceleratorSelector(scenario.original, nil) + InjectGKEAcceleratorSelector(scenario.original) // cmd.Diff complains on ResourceList when Nvidia is key. Objects are explicitly compared if diff := cmp.Diff( scenario.expected.Spec.NodeSelector, diff --git a/pkg/webhook/admission/pod/agent_injector.go b/pkg/webhook/admission/pod/agent_injector.go index 430d4148098..c783ec0feb7 100644 --- a/pkg/webhook/admission/pod/agent_injector.go +++ b/pkg/webhook/admission/pod/agent_injector.go @@ -116,7 +116,7 @@ func getLoggerConfigs(configMap *v1.ConfigMap) (*LoggerConfig, error) { return loggerConfig, nil } -func (ag *AgentInjector) InjectAgent(pod *v1.Pod, _ *v1.Namespace) error { +func (ag *AgentInjector) InjectAgent(pod *v1.Pod) error { // Only inject the model agent sidecar if the required annotations are set _, injectLogger := pod.ObjectMeta.Annotations[constants.LoggerInternalAnnotationKey] _, injectPuller := pod.ObjectMeta.Annotations[constants.AgentShouldInjectAnnotationKey] diff --git a/pkg/webhook/admission/pod/agent_injector_test.go b/pkg/webhook/admission/pod/agent_injector_test.go index a028c5e7916..0d196343052 100644 --- a/pkg/webhook/admission/pod/agent_injector_test.go +++ b/pkg/webhook/admission/pod/agent_injector_test.go @@ -1112,7 +1112,7 @@ func TestAgentInjector(t *testing.T) { loggerConfig, batcherTestConfig, } - injector.InjectAgent(scenario.original, nil) + injector.InjectAgent(scenario.original) if diff, _ := kmp.SafeDiff(scenario.expected.Spec, scenario.original.Spec); diff != "" { t.Errorf("Test %q unexpected result (-want +got): %v", name, diff) } diff --git a/pkg/webhook/admission/pod/metrics_aggregate_injector.go b/pkg/webhook/admission/pod/metrics_aggregate_injector.go index c881c800874..223a930f042 100644 --- a/pkg/webhook/admission/pod/metrics_aggregate_injector.go +++ b/pkg/webhook/admission/pod/metrics_aggregate_injector.go @@ -76,7 +76,7 @@ func setMetricAggregationEnvVars(pod *v1.Pod) { // InjectMetricsAggregator looks for the annotations to enable aggregate kserve-container and queue-proxy metrics and // if specified, sets port-related EnvVars in queue-proxy and the aggregate prometheus annotation. -func (ma *MetricsAggregator) InjectMetricsAggregator(pod *v1.Pod, _ *v1.Namespace) error { +func (ma *MetricsAggregator) InjectMetricsAggregator(pod *v1.Pod) error { //Only set metric configs if the required annotations are set enableMetricAggregation, ok := pod.ObjectMeta.Annotations[constants.EnableMetricAggregation] if !ok { diff --git a/pkg/webhook/admission/pod/metrics_aggregate_injector_test.go b/pkg/webhook/admission/pod/metrics_aggregate_injector_test.go index 47572690288..e1a0a3b99f0 100644 --- a/pkg/webhook/admission/pod/metrics_aggregate_injector_test.go +++ b/pkg/webhook/admission/pod/metrics_aggregate_injector_test.go @@ -290,7 +290,7 @@ func TestInjectMetricsAggregator(t *testing.T) { } for name, scenario := range scenarios { - ma.InjectMetricsAggregator(scenario.original, nil) + ma.InjectMetricsAggregator(scenario.original) if diff, _ := kmp.SafeDiff(scenario.expected.Spec, scenario.original.Spec); diff != "" { t.Errorf("Test %q unexpected result (-want +got): %v", name, diff) } diff --git a/pkg/webhook/admission/pod/mutator.go b/pkg/webhook/admission/pod/mutator.go index e4d98852fe7..f5b2e266bb2 100644 --- a/pkg/webhook/admission/pod/mutator.go +++ b/pkg/webhook/admission/pod/mutator.go @@ -123,15 +123,17 @@ func (mutator *Mutator) mutate(pod *v1.Pod, configMap *v1.ConfigMap, targetNs *v return err } - mutators := []func(pod *v1.Pod, targetNs *v1.Namespace) error{ + mutators := []func(pod *v1.Pod) error{ InjectGKEAcceleratorSelector, - storageInitializer.InjectStorageInitializer, + func(pod *v1.Pod) error { + return storageInitializer.InjectStorageInitializer(pod, targetNs) + }, agentInjector.InjectAgent, metricsAggregator.InjectMetricsAggregator, } for _, mutator := range mutators { - if err := mutator(pod, targetNs); err != nil { + if err := mutator(pod); err != nil { return err } } diff --git a/pkg/webhook/admission/pod/storage_initializer_injector.go b/pkg/webhook/admission/pod/storage_initializer_injector.go index 0b90fbe5674..6c7c652042b 100644 --- a/pkg/webhook/admission/pod/storage_initializer_injector.go +++ b/pkg/webhook/admission/pod/storage_initializer_injector.go @@ -342,7 +342,7 @@ func (mi *StorageInitializerInjector) InjectStorageInitializer(pod *v1.Pod, targ } uidStr := targetNs.Annotations[OpenShiftUidRangeAnnotationKey] if uidStr == "" { - return fmt.Errorf("could not find OpenShift internal annotation %s on namespace: %s", OpenShiftUidRangeAnnotationKey, targetNs.Name) + return fmt.Errorf("could not find OpenShift internal annotation %s for calculating the process UID (minRange + 1) on namespace: %s", OpenShiftUidRangeAnnotationKey, targetNs.Name) } uidStrParts := strings.Split(uidStr, "/") if uid, err := strconv.ParseInt(uidStrParts[0], 10, 64); err == nil { @@ -350,7 +350,7 @@ func (mi *StorageInitializerInjector) InjectStorageInitializer(pod *v1.Pod, targ uid++ initContainer.SecurityContext.RunAsUser = ptr.Int64(uid) } else { - return fmt.Errorf("could not parse value %s in annotation %s on namespace: %s", uidStr, OpenShiftUidRangeAnnotationKey, targetNs.Name) + return fmt.Errorf("could not parse value %s in annotation %s as an int64 value on namespace: %s", uidStr, OpenShiftUidRangeAnnotationKey, targetNs.Name) } // Add init container to the spec From 9eac84bd0642ee638bc42558811fb88ef626b171 Mon Sep 17 00:00:00 2001 From: Reto Lehmann Date: Wed, 12 Jul 2023 07:32:19 +0200 Subject: [PATCH 5/5] Also use annotation on pod to override uid --- .../pod/storage_initializer_injector.go | 41 ++++-- .../pod/storage_initializer_injector_test.go | 139 ++++++++++++++++++ 2 files changed, 165 insertions(+), 15 deletions(-) diff --git a/pkg/webhook/admission/pod/storage_initializer_injector.go b/pkg/webhook/admission/pod/storage_initializer_injector.go index 6c7c652042b..39bb93eb7f1 100644 --- a/pkg/webhook/admission/pod/storage_initializer_injector.go +++ b/pkg/webhook/admission/pod/storage_initializer_injector.go @@ -334,23 +334,34 @@ func (mi *StorageInitializerInjector) InjectStorageInitializer(pod *v1.Pod, targ /* OpenShift uses istio-cni which causes an issue with init-containers when calling external services like S3 or similar. Setting the `uid` for the `storage-initializer` to the same `uid` as the - `uid` of the `istio-proxy` resolves the issue. In OpenShift the `istio-proxy` always gets assigned - the first `uid` from the namespaces `uid` range + 1 (The range is defined in an annotation on the namespace). + `uid` of the `istio-proxy` resolves the issue. + + With upstream istio the user has the option to set the uid to 1337 described in https://istio.io/latest/docs/setup/additional-setup/cni/#compatibility-with-application-init-containers + using the annotation IstioSidecarUIDAnnotationKey. + + In OpenShift the `istio-proxy` always gets assigned the first `uid` from the namespaces + `uid` range + 1 (The range is defined in an annotation on the namespace). */ - if initContainer.SecurityContext == nil { - initContainer.SecurityContext = &v1.SecurityContext{} - } - uidStr := targetNs.Annotations[OpenShiftUidRangeAnnotationKey] - if uidStr == "" { - return fmt.Errorf("could not find OpenShift internal annotation %s for calculating the process UID (minRange + 1) on namespace: %s", OpenShiftUidRangeAnnotationKey, targetNs.Name) - } - uidStrParts := strings.Split(uidStr, "/") - if uid, err := strconv.ParseInt(uidStrParts[0], 10, 64); err == nil { - // Set the uid to the first uid in the namespaces range + 1 - uid++ - initContainer.SecurityContext.RunAsUser = ptr.Int64(uid) + if value, ok := pod.GetAnnotations()[constants.IstioSidecarUIDAnnotationKey]; ok { + if uid, err := strconv.ParseInt(value, 10, 64); err == nil { + if initContainer.SecurityContext == nil { + initContainer.SecurityContext = &v1.SecurityContext{} + } + initContainer.SecurityContext.RunAsUser = ptr.Int64(uid) + } } else { - return fmt.Errorf("could not parse value %s in annotation %s as an int64 value on namespace: %s", uidStr, OpenShiftUidRangeAnnotationKey, targetNs.Name) + uidStr := targetNs.Annotations[OpenShiftUidRangeAnnotationKey] + if uidStr != "" { + uidStrParts := strings.Split(uidStr, "/") + if uid, err := strconv.ParseInt(uidStrParts[0], 10, 64); err == nil { + // Set the uid to the first uid in the namespaces range + 1 + uid++ + if initContainer.SecurityContext == nil { + initContainer.SecurityContext = &v1.SecurityContext{} + } + initContainer.SecurityContext.RunAsUser = ptr.Int64(uid) + } + } } // Add init container to the spec diff --git a/pkg/webhook/admission/pod/storage_initializer_injector_test.go b/pkg/webhook/admission/pod/storage_initializer_injector_test.go index cac50b9e654..b4f83f61cb8 100644 --- a/pkg/webhook/admission/pod/storage_initializer_injector_test.go +++ b/pkg/webhook/admission/pod/storage_initializer_injector_test.go @@ -425,6 +425,145 @@ func TestStorageInitializerFailureCases(t *testing.T) { } } +func TestStorageInitializerInjectorUIDHandling(t *testing.T) { + scenarios := map[string]struct { + namespace *v1.Namespace + original *v1.Pod + expected *v1.Pod + }{ + "NoAnnotationNoUid": { + namespace: &v1.Namespace{}, + original: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + constants.StorageInitializerSourceUriInternalAnnotationKey: "gs://foo", + }, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: constants.InferenceServiceContainerName, + }, + }, + }, + }, + expected: &v1.Pod{ + Spec: v1.PodSpec{ + InitContainers: []v1.Container{ + { + Name: "storage-initializer", + Image: StorageInitializerContainerImage + ":" + StorageInitializerContainerImageVersion, + Args: []string{"gs://foo", constants.DefaultModelLocalMountPath}, + Resources: resourceRequirement, + TerminationMessagePolicy: "FallbackToLogsOnError", + VolumeMounts: []v1.VolumeMount{ + { + Name: "kserve-provision-location", + MountPath: constants.DefaultModelLocalMountPath, + }, + }, + }, + }, + }, + }, + }, + "UidFromOpenShiftNamespaceAnnotation": { + namespace: targetNS, + original: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + constants.StorageInitializerSourceUriInternalAnnotationKey: "gs://foo", + }, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: constants.InferenceServiceContainerName, + }, + }, + }, + }, + expected: &v1.Pod{ + Spec: v1.PodSpec{ + InitContainers: []v1.Container{ + { + Name: "storage-initializer", + Image: StorageInitializerContainerImage + ":" + StorageInitializerContainerImageVersion, + Args: []string{"gs://foo", constants.DefaultModelLocalMountPath}, + Resources: resourceRequirement, + TerminationMessagePolicy: "FallbackToLogsOnError", + VolumeMounts: []v1.VolumeMount{ + { + Name: "kserve-provision-location", + MountPath: constants.DefaultModelLocalMountPath, + }, + }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, + }, + }, + }, + }, + }, + "UidFromPodAnnotation": { + namespace: targetNS, + original: &v1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + constants.StorageInitializerSourceUriInternalAnnotationKey: "gs://foo", + constants.IstioSidecarUIDAnnotationKey: "1337", + }, + }, + Spec: v1.PodSpec{ + Containers: []v1.Container{ + { + Name: constants.InferenceServiceContainerName, + }, + }, + }, + }, + expected: &v1.Pod{ + Spec: v1.PodSpec{ + InitContainers: []v1.Container{ + { + Name: "storage-initializer", + Image: StorageInitializerContainerImage + ":" + StorageInitializerContainerImageVersion, + Args: []string{"gs://foo", constants.DefaultModelLocalMountPath}, + Resources: resourceRequirement, + TerminationMessagePolicy: "FallbackToLogsOnError", + VolumeMounts: []v1.VolumeMount{ + { + Name: "kserve-provision-location", + MountPath: constants.DefaultModelLocalMountPath, + }, + }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: ptr.Int64(1337), + }, + }, + }, + }, + }, + }, + } + + for name, scenario := range scenarios { + injector := &StorageInitializerInjector{ + credentialBuilder: credentials.NewCredentialBulder(c, &v1.ConfigMap{ + Data: map[string]string{}, + }), + config: storageInitializerConfig, + } + if err := injector.InjectStorageInitializer(scenario.original, scenario.namespace); err != nil { + t.Errorf("Test %q unexpected result: %s", name, err) + } + if diff, _ := kmp.SafeDiff(scenario.expected.Spec.InitContainers, scenario.original.Spec.InitContainers); diff != "" { + t.Errorf("Test %q unexpected result (-want +got): %v", name, diff) + } + } +} + func TestCustomSpecStorageUriInjection(t *testing.T) { scenarios := map[string]struct { original *v1.Pod