Repository: numaproj/numaflow
Pull request #528 (2023-02-07 00:44:10)

fix: securityContext not applied to container templates
Signed-off-by: Derek Wang <whynowy@gmail.com>

Explain what this PR does.

<!--

Before you push your changes:

* Run `make pre-push -B` to fix codegen and lint problems.

Then, you MUST:

* Sign-off your commit (otherwise the DCO check will fail).
* Use [a conventional commit message](https://www.conventionalcommits.org/en/v1.0.0/) (otherwise the commit message check will fail).

If you did not do this, reset all your commit and replace them with a single commit:

```
git reset HEAD~1 ;# change 1 to how many commits you made
git commit --signoff -m 'feat: my feat. Fixes #1234'
```

When creating your PR: 

* Make sure that "Fixes #" or "Closes #" is in both the PR title (for release notes) and description (to automatically link and close the issue).
* Say how you tested your changes. If you changed the UI, attach screenshots.
* Set your PR as a draft initially.
* Your PR needs to pass the required checks before it can be approved. 
* Once required tests have passed, mark your PR "Ready for review".

If changes were requested, once you've made them, you MUST dismiss the review to get it reviewed again.

-->



Patch:
diff --git a/go.mod b/go.mod
index 93fad1a2..537aa749 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 012fdd6c..7b751e0b 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 fcee0915..154d9b33 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/jetstream_buffer_service.go b/pkg/apis/numaflow/v1alpha1/jetstream_buffer_service.go
index ac9d400c..7965872e 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/vertex_types.go b/pkg/apis/numaflow/v1alpha1/vertex_types.go
index a4f31155..2ad4ae6d 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()


Test patch:
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 00000000..88720cd5
--- /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_test.go b/pkg/apis/numaflow/v1alpha1/jetstream_buffer_service_test.go
index d849bc9a..9a4b75fa 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 fc6b6485..113e6ea8 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/reconciler/pipeline/controller_test.go b/pkg/reconciler/pipeline/controller_test.go
index 4f3d56e7..edafa8eb 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)