diff --git a/pkg/webhook/admission/pod/metrics_aggregate_injector.go b/pkg/webhook/admission/pod/metrics_aggregate_injector.go index 3a5d6fa3569..223a930f042 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" ) diff --git a/pkg/webhook/admission/pod/mutator.go b/pkg/webhook/admission/pod/mutator.go index c320967f3c1..f5b2e266bb2 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) @@ -118,7 +125,9 @@ func (mutator *Mutator) mutate(pod *v1.Pod, configMap *v1.ConfigMap) error { mutators := []func(pod *v1.Pod) error{ InjectGKEAcceleratorSelector, - storageInitializer.InjectStorageInitializer, + func(pod *v1.Pod) error { + return storageInitializer.InjectStorageInitializer(pod, targetNs) + }, agentInjector.InjectAgent, metricsAggregator.InjectMetricsAggregator, } diff --git a/pkg/webhook/admission/pod/storage_initializer_injector.go b/pkg/webhook/admission/pod/storage_initializer_injector.go index fb7aa032ab2..39bb93eb7f1 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,37 @@ 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. + /* + 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. + + 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 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 { + 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 80456344032..b4f83f61cb8 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" @@ -62,6 +63,17 @@ var ( v1.ResourceMemory: resource.MustParse(StorageInitializerDefaultMemoryRequest), }, } + + targetNS = &v1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "my-ns", + Annotations: map[string]string{ + OpenShiftUidRangeAnnotationKey: "1000740000/10000", + }, + }, + } + + expectedInitContainerUid = ptr.Int64(1000740001) ) func TestStorageInitializerInjector(t *testing.T) { @@ -182,6 +194,9 @@ func TestStorageInitializerInjector(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -252,6 +267,9 @@ func TestStorageInitializerInjector(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -332,6 +350,9 @@ func TestStorageInitializerInjector(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -354,7 +375,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 +415,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) } @@ -404,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 @@ -493,7 +653,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) } @@ -597,6 +757,9 @@ func TestCredentialInjection(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, Env: []v1.EnvVar{ { Name: s3.AWSAccessKeyId, @@ -702,6 +865,9 @@ func TestCredentialInjection(t *testing.T) { MountPath: gcs.GCSCredentialVolumeMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, Env: []v1.EnvVar{ { Name: gcs.GCSCredentialEnvKey, @@ -790,6 +956,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, @@ -885,6 +1054,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, @@ -944,7 +1116,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 != "" { @@ -1008,6 +1180,9 @@ func TestStorageInitializerConfigmap(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -1037,7 +1212,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 +1447,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 != "" { @@ -1385,6 +1560,9 @@ func TestTransformerCollocation(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -1563,6 +1741,9 @@ func TestTransformerCollocation(t *testing.T) { MountPath: constants.DefaultModelLocalMountPath, }, }, + SecurityContext: &v1.SecurityContext{ + RunAsUser: expectedInitContainerUid, + }, }, }, Volumes: []v1.Volume{ @@ -1594,7 +1775,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 != "" {