From c0aa7c1e3769a8a9180e0a4b1a0f84facf393046 Mon Sep 17 00:00:00 2001 From: Derek Wang Date: Mon, 6 Feb 2023 17:13:15 -0800 Subject: [PATCH] fix: securityContext not applied to container templates (#528) Signed-off-by: Derek Wang --- go.mod | 2 +- go.sum | 5 +- .../numaflow/v1alpha1/container_template.go | 12 ++- .../v1alpha1/container_template_test.go | 77 +++++++++++++++++++ .../v1alpha1/jetstream_buffer_service.go | 42 +++------- .../v1alpha1/jetstream_buffer_service_test.go | 10 ++- .../numaflow/v1alpha1/pipeline_types_test.go | 6 ++ pkg/apis/numaflow/v1alpha1/vertex_types.go | 1 + pkg/reconciler/pipeline/controller_test.go | 8 ++ 9 files changed, 122 insertions(+), 41 deletions(-) create mode 100644 pkg/apis/numaflow/v1alpha1/container_template_test.go diff --git a/go.mod b/go.mod index 93fad1a22..537aa749c 100644 --- a/go.mod +++ b/go.mod @@ -22,6 +22,7 @@ require ( github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 github.com/grpc-ecosystem/grpc-gateway v1.16.0 github.com/hashicorp/golang-lru v0.5.4 + github.com/imdario/mergo v0.3.13 github.com/montanaflynn/stats v0.0.0-20171201202039-1bf9dbcd8cbe github.com/nats-io/nats-server/v2 v2.7.5-0.20220415000625-a6b62f61a703 github.com/nats-io/nats.go v1.21.0 @@ -102,7 +103,6 @@ require ( github.com/hashicorp/go-uuid v1.0.2 // indirect github.com/hashicorp/hcl v1.0.0 // indirect github.com/huandu/xstrings v1.3.1 // indirect - github.com/imdario/mergo v0.3.12 // indirect github.com/imkira/go-interpol v1.0.0 // indirect github.com/inconshreveable/mousetrap v1.0.0 // indirect github.com/jcmturner/aescts/v2 v2.0.0 // indirect diff --git a/go.sum b/go.sum index 012fdd6c3..7b751e0b3 100644 --- a/go.sum +++ b/go.sum @@ -539,8 +539,8 @@ github.com/ianlancetaylor/demangle v0.0.0-20181102032728-5e5cf60278f6/go.mod h1: github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc= github.com/imdario/mergo v0.3.5/go.mod h1:2EnlNZ0deacrJVfApfmtdGgDfMuh/nq6Ok1EcJh5FfA= github.com/imdario/mergo v0.3.11/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA= -github.com/imdario/mergo v0.3.12 h1:b6R2BslTbIEToALKP7LxUvijTsNI9TAe80pLWN2g/HU= -github.com/imdario/mergo v0.3.12/go.mod h1:jmQim1M+e3UYxmgPu/WyfjB3N3VflVyUjjjwH0dnCYA= +github.com/imdario/mergo v0.3.13 h1:lFzP57bqS/wsqKssCGmtLAb8A0wKjLGrve2q3PPVcBk= +github.com/imdario/mergo v0.3.13/go.mod h1:4lJ1jqUDcsbIECGy0RUJAXNIhg+6ocWgb1ALK2O4oXg= github.com/imkira/go-interpol v1.0.0 h1:HrmLyvOLJyjR0YofMw8QGdCIuYOs4TJUBDNU5sJC09E= github.com/imkira/go-interpol v1.0.0/go.mod h1:z0h2/2T3XF8kyEPpRgJ3kmNv+C43p+I/CoI+jC3w2iA= github.com/inconshreveable/mousetrap v1.0.0 h1:Z8tu5sraLXCXIcARxBp/8cbvlwVa7Z1NHg9XEKhtSvM= @@ -1453,6 +1453,7 @@ gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C gopkg.in/yaml.v3 v3.0.0-20200605160147-a5ece683394c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.0-20200615113413-eeeca48fe776/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.0-20210107192922-496545a6307b/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= +gopkg.in/yaml.v3 v3.0.0/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA= gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM= gotest.tools/v3 v3.0.2/go.mod h1:3SzNCllyD9/Y+b5r9JIKQ474KzkZyqLqEfYqMsX94Bk= diff --git a/pkg/apis/numaflow/v1alpha1/container_template.go b/pkg/apis/numaflow/v1alpha1/container_template.go index fcee09150..154d9b33f 100644 --- a/pkg/apis/numaflow/v1alpha1/container_template.go +++ b/pkg/apis/numaflow/v1alpha1/container_template.go @@ -16,7 +16,10 @@ limitations under the License. package v1alpha1 -import corev1 "k8s.io/api/core/v1" +import ( + "github.com/imdario/mergo" + corev1 "k8s.io/api/core/v1" +) // ContainerTemplate defines customized spec for a container type ContainerTemplate struct { @@ -28,8 +31,11 @@ type ContainerTemplate struct { // ApplyToContainer updates the Container with the values from the ContainerTemplate func (ct *ContainerTemplate) ApplyToContainer(c *corev1.Container) { - // currently only doing resources & env, ignoring imagePullPolicy & securityContext - c.Resources = ct.Resources + _ = mergo.Merge(&c.Resources, ct.Resources, mergo.WithOverride) + c.SecurityContext = ct.SecurityContext + if c.ImagePullPolicy == "" { + c.ImagePullPolicy = ct.ImagePullPolicy + } if len(ct.Env) > 0 { c.Env = append(c.Env, ct.Env...) } diff --git a/pkg/apis/numaflow/v1alpha1/container_template_test.go b/pkg/apis/numaflow/v1alpha1/container_template_test.go new file mode 100644 index 000000000..88720cd5b --- /dev/null +++ b/pkg/apis/numaflow/v1alpha1/container_template_test.go @@ -0,0 +1,77 @@ +/* +Copyright 2022 The Numaproj Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +import ( + "testing" + + "github.com/stretchr/testify/assert" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +var ( + testContainerTemplate = &ContainerTemplate{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + "cpu": resource.MustParse("100m"), + }, + }, + } +) + +func Test_ApplyToContainer(t *testing.T) { + c := &corev1.Container{} + testContainerTemplate.ApplyToContainer(c) + assert.Equal(t, "", string(c.ImagePullPolicy)) + assert.Equal(t, testContainerTemplate.Resources, c.Resources) + c.Resources.Limits = corev1.ResourceList{ + "cpu": resource.MustParse("200m"), + } + testContainerTemplate.ApplyToContainer(c) + assert.Equal(t, resource.MustParse("200m"), *c.Resources.Limits.Cpu()) + c.Resources.Limits = corev1.ResourceList{ + "memory": resource.MustParse("32Mi"), + } + assert.Equal(t, resource.MustParse("32Mi"), *c.Resources.Limits.Memory()) + testContainerTemplate.ImagePullPolicy = corev1.PullAlways + testContainerTemplate.ApplyToContainer(c) + assert.Equal(t, corev1.PullAlways, c.ImagePullPolicy) + c.ImagePullPolicy = corev1.PullIfNotPresent + testContainerTemplate.ApplyToContainer(c) + assert.Equal(t, corev1.PullIfNotPresent, c.ImagePullPolicy) + testContainerTemplate.SecurityContext = &corev1.SecurityContext{} + testContainerTemplate.ApplyToContainer(c) + assert.NotNil(t, c.SecurityContext) + testContainerTemplate.Env = []corev1.EnvVar{{Name: "a", Value: "b"}} + testContainerTemplate.ApplyToContainer(c) + envs := []string{} + for _, e := range c.Env { + envs = append(envs, e.Name) + } + assert.Contains(t, envs, "a") +} + +func Test_ApplyToNumaflowContainers(t *testing.T) { + cs := []corev1.Container{ + {Name: CtrMain}, + {Name: "nono"}, + } + testContainerTemplate.ApplyToNumaflowContainers(cs) + assert.Equal(t, testContainerTemplate.Resources, cs[0].Resources) + assert.NotEqual(t, testContainerTemplate.Resources, cs[1].Resources) +} diff --git a/pkg/apis/numaflow/v1alpha1/jetstream_buffer_service.go b/pkg/apis/numaflow/v1alpha1/jetstream_buffer_service.go index ac9d400cb..7965872e0 100644 --- a/pkg/apis/numaflow/v1alpha1/jetstream_buffer_service.go +++ b/pkg/apis/numaflow/v1alpha1/jetstream_buffer_service.go @@ -111,20 +111,6 @@ func (j JetStreamBufferService) GetStatefulSetSpec(req GetJetStreamStatefulSetSp for k, v := range req.Labels { podTemplateLabels[k] = v } - var jsContainerPullPolicy, reloaderContainerPullPolicy, metricsContainerPullPolicy corev1.PullPolicy - var jsContainerSecurityContext, reloaderContainerSecurityContext, metricsContainerSecurityContext *corev1.SecurityContext - if j.ContainerTemplate != nil { - jsContainerPullPolicy = j.ContainerTemplate.ImagePullPolicy - jsContainerSecurityContext = j.ContainerTemplate.SecurityContext - } - if j.ReloaderContainerTemplate != nil { - reloaderContainerPullPolicy = j.ReloaderContainerTemplate.ImagePullPolicy - reloaderContainerSecurityContext = j.ReloaderContainerTemplate.SecurityContext - } - if j.MetricsContainerTemplate != nil { - metricsContainerPullPolicy = j.MetricsContainerTemplate.ImagePullPolicy - metricsContainerSecurityContext = j.MetricsContainerTemplate.SecurityContext - } projectedSecretKeyToPaths := []corev1.KeyToPath{ { Key: JetStreamServerSecretAuthKey, @@ -196,9 +182,8 @@ func (j JetStreamBufferService) GetStatefulSetSpec(req GetJetStreamStatefulSetSp }, Containers: []corev1.Container{ { - Name: "main", - Image: req.NatsImage, - ImagePullPolicy: jsContainerPullPolicy, + Name: "main", + Image: req.NatsImage, Ports: []corev1.ContainerPort{ {Name: "client", ContainerPort: req.ClientPort}, {Name: "cluster", ContainerPort: req.ClusterPort}, @@ -218,7 +203,6 @@ func (j JetStreamBufferService) GetStatefulSetSpec(req GetJetStreamStatefulSetSp {Name: "config-volume", MountPath: "/etc/nats-config"}, {Name: "pid", MountPath: "/var/run/nats"}, }, - SecurityContext: jsContainerSecurityContext, StartupProbe: &corev1.Probe{ ProbeHandler: corev1.ProbeHandler{ HTTPGet: &corev1.HTTPGetAction{ @@ -250,25 +234,21 @@ func (j JetStreamBufferService) GetStatefulSetSpec(req GetJetStreamStatefulSetSp }, }, { - Name: "reloader", - Image: req.ConfigReloaderImage, - ImagePullPolicy: reloaderContainerPullPolicy, - SecurityContext: reloaderContainerSecurityContext, - Command: []string{"nats-server-config-reloader", "-pid", "/var/run/nats/nats.pid", "-config", "/etc/nats-config/nats-js.conf"}, + Name: "reloader", + Image: req.ConfigReloaderImage, + Command: []string{"nats-server-config-reloader", "-pid", "/var/run/nats/nats.pid", "-config", "/etc/nats-config/nats-js.conf"}, VolumeMounts: []corev1.VolumeMount{ {Name: "config-volume", MountPath: "/etc/nats-config"}, {Name: "pid", MountPath: "/var/run/nats"}, }, }, { - Name: "metrics", - Image: req.MetricsExporterImage, - ImagePullPolicy: metricsContainerPullPolicy, + Name: "metrics", + Image: req.MetricsExporterImage, Ports: []corev1.ContainerPort{ {Name: "metrics", ContainerPort: req.MetricsPort}, }, - Args: []string{"-connz", "-routez", "-subz", "-varz", "-prefix=nats", "-use_internal_server_id", "-jsz=all", fmt.Sprintf("http://localhost:%s", strconv.Itoa(int(req.MonitorPort)))}, - SecurityContext: metricsContainerSecurityContext, + Args: []string{"-connz", "-routez", "-subz", "-varz", "-prefix=nats", "-use_internal_server_id", "-jsz=all", fmt.Sprintf("http://localhost:%s", strconv.Itoa(int(req.MonitorPort)))}, }, }, } @@ -291,13 +271,13 @@ func (j JetStreamBufferService) GetStatefulSetSpec(req GetJetStreamStatefulSetSp spec.Template.SetAnnotations(j.Metadata.Annotations) } if j.ContainerTemplate != nil { - spec.Template.Spec.Containers[0].Resources = j.ContainerTemplate.Resources + j.ContainerTemplate.ApplyToContainer(&spec.Template.Spec.Containers[0]) } if j.ReloaderContainerTemplate != nil { - spec.Template.Spec.Containers[1].Resources = j.ReloaderContainerTemplate.Resources + j.ReloaderContainerTemplate.ApplyToContainer(&spec.Template.Spec.Containers[1]) } if j.MetricsContainerTemplate != nil { - spec.Template.Spec.Containers[2].Resources = j.MetricsContainerTemplate.Resources + j.MetricsContainerTemplate.ApplyToContainer(&spec.Template.Spec.Containers[2]) } if j.Persistence != nil { spec.VolumeClaimTemplates = []corev1.PersistentVolumeClaim{ diff --git a/pkg/apis/numaflow/v1alpha1/jetstream_buffer_service_test.go b/pkg/apis/numaflow/v1alpha1/jetstream_buffer_service_test.go index d849bc9ae..9a4b75fad 100644 --- a/pkg/apis/numaflow/v1alpha1/jetstream_buffer_service_test.go +++ b/pkg/apis/numaflow/v1alpha1/jetstream_buffer_service_test.go @@ -86,20 +86,22 @@ func TestJetStreamGetStatefulSetSpec(t *testing.T) { assert.Equal(t, 7, len(spec.Template.Spec.Volumes[1].VolumeSource.Projected.Sources[1].Secret.Items)) }) - t.Run("with container resources", func(t *testing.T) { + t.Run("with customized containers", func(t *testing.T) { r := corev1.ResourceRequirements{ Limits: corev1.ResourceList{ corev1.ResourceMemory: resource.MustParse("100Mi"), }, } s := &JetStreamBufferService{ - ContainerTemplate: &ContainerTemplate{Resources: r}, - ReloaderContainerTemplate: &ContainerTemplate{Resources: r}, - MetricsContainerTemplate: &ContainerTemplate{Resources: r}, + ContainerTemplate: &ContainerTemplate{Resources: r, SecurityContext: &corev1.SecurityContext{}, ImagePullPolicy: corev1.PullNever}, + ReloaderContainerTemplate: &ContainerTemplate{Resources: r, SecurityContext: &corev1.SecurityContext{}, ImagePullPolicy: corev1.PullNever}, + MetricsContainerTemplate: &ContainerTemplate{Resources: r, SecurityContext: &corev1.SecurityContext{}, ImagePullPolicy: corev1.PullNever}, } spec := s.GetStatefulSetSpec(req) for _, c := range spec.Template.Spec.Containers { assert.Equal(t, c.Resources, r) + assert.NotNil(t, c.SecurityContext) + assert.Equal(t, corev1.PullNever, c.ImagePullPolicy) } }) } diff --git a/pkg/apis/numaflow/v1alpha1/pipeline_types_test.go b/pkg/apis/numaflow/v1alpha1/pipeline_types_test.go index fc6b6485c..113e6ea85 100644 --- a/pkg/apis/numaflow/v1alpha1/pipeline_types_test.go +++ b/pkg/apis/numaflow/v1alpha1/pipeline_types_test.go @@ -186,6 +186,9 @@ func TestGetDaemonDeploy(t *testing.T) { ContainerTemplate: &ContainerTemplate{ Resources: testResources, Env: []corev1.EnvVar{env}, + SecurityContext: &corev1.SecurityContext{ + Privileged: pointer.Bool(false), + }, }, InitContainerTemplate: &ContainerTemplate{ Resources: testResources, @@ -210,6 +213,9 @@ func TestGetDaemonDeploy(t *testing.T) { assert.Equal(t, 1, len(s.Spec.Template.Spec.Containers)) assert.Greater(t, len(s.Spec.Template.Spec.Containers[0].Env), 1) assert.Contains(t, s.Spec.Template.Spec.Containers[0].Env, env) + assert.NotNil(t, s.Spec.Template.Spec.Containers[0].SecurityContext) + assert.NotNil(t, s.Spec.Template.Spec.Containers[0].SecurityContext.Privileged) + assert.False(t, *s.Spec.Template.Spec.Containers[0].SecurityContext.Privileged) assert.Equal(t, 1, len(s.Spec.Template.Spec.InitContainers)) assert.Equal(t, s.Spec.Template.Spec.InitContainers[0].Resources, testResources) assert.Contains(t, s.Spec.Template.Spec.InitContainers[0].Env, initEnv) diff --git a/pkg/apis/numaflow/v1alpha1/vertex_types.go b/pkg/apis/numaflow/v1alpha1/vertex_types.go index a4f311553..2ad4ae6df 100644 --- a/pkg/apis/numaflow/v1alpha1/vertex_types.go +++ b/pkg/apis/numaflow/v1alpha1/vertex_types.go @@ -266,6 +266,7 @@ func (v Vertex) getInitContainers(req GetVertexPodSpecReq) []corev1.Container { } return append(initContainers, v.Spec.InitContainers...) } + func (vs VertexSpec) WithOutReplicas() VertexSpec { zero := int32(0) x := *vs.DeepCopy() diff --git a/pkg/reconciler/pipeline/controller_test.go b/pkg/reconciler/pipeline/controller_test.go index 4f3d56e7f..edafa8eb9 100644 --- a/pkg/reconciler/pipeline/controller_test.go +++ b/pkg/reconciler/pipeline/controller_test.go @@ -32,6 +32,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/client-go/kubernetes/scheme" + "k8s.io/utils/pointer" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" @@ -228,6 +229,7 @@ func Test_buildISBBatchJob(t *testing.T) { assert.Contains(t, envNames, dfv1.EnvISBSvcRedisUser) assert.Contains(t, envNames, dfv1.EnvISBSvcRedisURL) }) + t.Run("test build ISB batch job with pipeline overrides", func(t *testing.T) { resources := corev1.ResourceRequirements{ Limits: corev1.ResourceList{ @@ -255,6 +257,9 @@ func Test_buildISBBatchJob(t *testing.T) { ContainerTemplate: &dfv1.ContainerTemplate{ Resources: resources, Env: []corev1.EnvVar{env}, + SecurityContext: &corev1.SecurityContext{ + Privileged: pointer.Bool(false), + }, }, AbstractPodTemplate: dfv1.AbstractPodTemplate{ Metadata: &dfv1.Metadata{ @@ -273,6 +278,9 @@ func Test_buildISBBatchJob(t *testing.T) { assert.Equal(t, j.Spec.Template.Spec.Containers[0].Resources, resources) assert.Greater(t, len(j.Spec.Template.Spec.Containers[0].Env), 1) assert.Contains(t, j.Spec.Template.Spec.Containers[0].Env, env) + assert.NotNil(t, j.Spec.Template.Spec.Containers[0].SecurityContext) + assert.NotNil(t, j.Spec.Template.Spec.Containers[0].SecurityContext.Privileged) + assert.False(t, *j.Spec.Template.Spec.Containers[0].SecurityContext.Privileged) assert.Equal(t, j.Spec.Template.Labels["my-label-name"], podLabels["my-label-name"]) assert.Equal(t, j.Spec.Template.Annotations["my-annotation-name"], podAnnotations["my-annotation-name"]) assert.NotNil(t, j.Spec.TTLSecondsAfterFinished)