controller/security: properly add strictSecurity to initContainers (#…

…1141)

Previously strictSecurity was added to the operator defined initContainer only
if there is user-defined initContainers. This logic is incorrect.

 Operator must apply strict security configuration only to the containers created on its own.
All security related settings for side-car and initContainers must be managed by operator's users.

 This commit fixes incorrect logic and properly apply strict security params to the init containers.
Affected CRDs - VMAgent, VMAuth and VMAlertmanager.

 Added tests to verify this logic.

Related issue:
#1134

Signed-off-by: f41gh7 <[email protected]>

api/operator/v1beta1/vmalertmanager_types.go

@@ -184,12 +184,6 @@ type VMAlertmanagerSpec struct {
 	RollingUpdateStrategy appsv1.StatefulSetUpdateStrategyType `json:"rollingUpdateStrategy,omitempty"`
 	// ClaimTemplates allows adding additional VolumeClaimTemplates for StatefulSet
 	ClaimTemplates []v1.PersistentVolumeClaim `json:"claimTemplates,omitempty"`
-	// UseStrictSecurity enables strict security mode for component
-	// it restricts disk writes access
-	// uses non-root user out of the box
-	// drops not needed security permissions
-	// +optional
-	UseStrictSecurity *bool `json:"useStrictSecurity,omitempty"`
 
 	// WebConfig defines configuration for webserver
 	// https://github.com/prometheus/alertmanager/blob/main/docs/https.md

docs/CHANGELOG.md

@@ -13,6 +13,7 @@ aliases:
 
 ## tip
 
+- [operator](https://docs.victoriametrics.com/operator/): properly apply `useStrictSecurity: true` to the `initContainers` for `VMAuth`, `VMAgent` and `VMAlertmanager`. See [this issue](https://github.com/VictoriaMetrics/operator/issues/1134) for details.
 - [vmauth](https://docs.victoriametrics.com/operator/resources/vmauth): Moved `spec.configSecret` to `spec.externalConfig.secretRef.name` and added `spec.externalConfig.localPath` to be able to provide custom configs via sidecar.
 - [vmcluster](https://docs.victoriametrics.com/operator/resources/vmcluster): adds `requestsLoadBalancer` configuration to the `VMCluster.spec`. See [this issue](https://github.com/VictoriaMetrics/operator/issues/1130) for details.
 - [vmcluster](https://docs.victoriametrics.com/operator/resources/vmcluster): properly configure monitoring for `VMCluster` with enabled `backup`.

internal/controller/operator/factory/alertmanager/statefulset.go

@@ -69,7 +69,6 @@ func newStsForAlertManager(cr *vmv1beta1.VMAlertmanager) (*appsv1.StatefulSet, e
 		Spec: *spec,
 	}
 	build.StatefulSetAddCommonParams(statefulset, ptr.Deref(cr.Spec.UseStrictSecurity, false), &cr.Spec.CommonApplicationDeploymentParams)
-
 	cr.Spec.Storage.IntoSTSVolume(cr.GetVolumeName(), &statefulset.Spec)
 	statefulset.Spec.Template.Spec.Volumes = append(statefulset.Spec.Template.Spec.Volumes, cr.Spec.Volumes...)
 
@@ -379,14 +378,13 @@ func makeStatefulSetSpec(cr *vmv1beta1.VMAlertmanager) (*appsv1.StatefulSetSpec,
 	useStrictSecurity := ptr.Deref(cr.Spec.UseStrictSecurity, false)
 
 	initContainers = append(initContainers, buildInitConfigContainer(cr)...)
-	if len(cr.Spec.InitContainers) > 0 {
-		var err error
-		build.AddStrictSecuritySettingsToContainers(cr.Spec.SecurityContext, initContainers, useStrictSecurity)
-		initContainers, err = k8stools.MergePatchContainers(initContainers, cr.Spec.InitContainers)
-		if err != nil {
-			return nil, fmt.Errorf("cannot apply patch for initContainers: %w", err)
-		}
+	build.AddStrictSecuritySettingsToContainers(cr.Spec.SecurityContext, initContainers, useStrictSecurity)
+
+	ic, err := k8stools.MergePatchContainers(initContainers, cr.Spec.InitContainers)
+	if err != nil {
+		return nil, fmt.Errorf("cannot apply patch for initContainers: %w", err)
 	}
+
 	vmaContainer := corev1.Container{
 		Args:                     amArgs,
 		Name:                     "alertmanager",
@@ -430,7 +428,7 @@ func makeStatefulSetSpec(cr *vmv1beta1.VMAlertmanager) (*appsv1.StatefulSetSpec,
 				Annotations: cr.PodAnnotations(),
 			},
 			Spec: corev1.PodSpec{
-				InitContainers:     initContainers,
+				InitContainers:     ic,
 				Containers:         containers,
 				Volumes:            volumes,
 				ServiceAccountName: cr.GetServiceAccountName(),

internal/controller/operator/factory/vmagent/vmagent.go

@@ -608,17 +608,14 @@ func makeSpecForVMAgent(cr *vmv1beta1.VMAgent, ssCache *scrapesSecretsCache) (*c
 		if !cr.Spec.IngestOnlyMode {
 			ic = append(ic,
 				buildInitConfigContainer(ptr.Deref(cr.Spec.UseVMConfigReloader, false), cr.Spec.ConfigReloaderImageTag, cr.Spec.ConfigReloaderResources, configReloader.Args)...)
-			if len(cr.Spec.InitContainers) > 0 {
-				var err error
-				build.AddStrictSecuritySettingsToContainers(cr.Spec.SecurityContext, ic, useStrictSecurity)
-				ic, err = k8stools.MergePatchContainers(ic, cr.Spec.InitContainers)
-				if err != nil {
-					return nil, fmt.Errorf("cannot apply patch for initContainers: %w", err)
-				}
-			}
-
+			build.AddStrictSecuritySettingsToContainers(cr.Spec.SecurityContext, ic, useStrictSecurity)
 		}
 	}
+	var err error
+	ic, err = k8stools.MergePatchContainers(ic, cr.Spec.InitContainers)
+	if err != nil {
+		return nil, fmt.Errorf("cannot apply patch for initContainers: %w", err)
+	}
 
 	operatorContainers = append(operatorContainers, vmagentContainer)
 

internal/controller/operator/factory/vmauth/vmauth.go

@@ -258,6 +258,11 @@ func makeSpecForVMAuth(cr *vmv1beta1.VMAuth) (*corev1.PodTemplateSpec, error) {
 		operatorContainers = append(operatorContainers, configReloader)
 		initContainers = append(initContainers,
 			buildInitConfigContainer(useCustomConfigReloader, cr.Spec.ConfigReloaderImageTag, cr.Spec.ConfigReloaderResources, configReloader.Args)...)
+		build.AddStrictSecuritySettingsToContainers(cr.Spec.SecurityContext, initContainers, useStrictSecurity)
+	}
+	ic, err := k8stools.MergePatchContainers(initContainers, cr.Spec.InitContainers)
+	if err != nil {
+		return nil, fmt.Errorf("cannot apply patch for initContainers: %w", err)
 	}
 
 	args = build.AddExtraArgsOverrideDefaults(args, cr.Spec.ExtraArgs, "-")
@@ -285,22 +290,14 @@ func makeSpecForVMAuth(cr *vmv1beta1.VMAuth) (*corev1.PodTemplateSpec, error) {
 		return nil, err
 	}
 
-	if len(cr.Spec.InitContainers) > 0 {
-		build.AddStrictSecuritySettingsToContainers(cr.Spec.SecurityContext, initContainers, useStrictSecurity)
-		initContainers, err = k8stools.MergePatchContainers(initContainers, cr.Spec.InitContainers)
-		if err != nil {
-			return nil, fmt.Errorf("cannot apply patch for initContainers: %w", err)
-		}
-	}
-
 	vmAuthSpec := &corev1.PodTemplateSpec{
 		ObjectMeta: metav1.ObjectMeta{
 			Labels:      cr.PodLabels(),
 			Annotations: cr.PodAnnotations(),
 		},
 		Spec: corev1.PodSpec{
 			Volumes:            volumes,
-			InitContainers:     initContainers,
+			InitContainers:     ic,
 			Containers:         containers,
 			ServiceAccountName: cr.GetServiceAccountName(),
 		},

internal/controller/operator/factory/vmcluster/vmcluster.go

@@ -1077,11 +1077,12 @@ func makePodSpecForVMStorage(ctx context.Context, cr *vmv1beta1.VMCluster) (*cor
 	}
 	useStrictSecurity := ptr.Deref(cr.Spec.VMStorage.UseStrictSecurity, false)
 	build.AddStrictSecuritySettingsToContainers(cr.Spec.VMStorage.SecurityContext, initContainers, useStrictSecurity)
-	build.AddStrictSecuritySettingsToContainers(cr.Spec.VMStorage.SecurityContext, operatorContainers, useStrictSecurity)
 	ic, err := k8stools.MergePatchContainers(initContainers, cr.Spec.VMStorage.InitContainers)
 	if err != nil {
 		return nil, fmt.Errorf("cannot patch vmstorage init containers: %w", err)
 	}
+
+	build.AddStrictSecuritySettingsToContainers(cr.Spec.VMStorage.SecurityContext, operatorContainers, useStrictSecurity)
 	containers, err := k8stools.MergePatchContainers(operatorContainers, cr.Spec.VMStorage.Containers)
 	if err != nil {
 		return nil, fmt.Errorf("cannot patch vmstorage containers: %w", err)
@@ -1480,9 +1481,9 @@ func buildVMauthLBDeployment(cr *vmv1beta1.VMCluster) (*appsv1.Deployment, error
 	containers := []corev1.Container{
 		vmauthLBCnt,
 	}
-	build.AddStrictSecuritySettingsToContainers(spec.SecurityContext, containers, ptr.Deref(spec.UseStrictSecurity, false))
-
 	var err error
+
+	build.AddStrictSecuritySettingsToContainers(spec.SecurityContext, containers, ptr.Deref(spec.UseStrictSecurity, false))
 	containers, err = k8stools.MergePatchContainers(containers, spec.Containers)
 	if err != nil {
 		return nil, fmt.Errorf("cannot patch containers: %w", err)

internal/controller/operator/factory/vmsingle/vmsingle.go

@@ -300,7 +300,7 @@ func makeSpecForVMSingle(ctx context.Context, cr *vmv1beta1.VMSingle) (*corev1.P
 	vmsingleContainer = build.Probe(vmsingleContainer, cr)
 
 	operatorContainers := []corev1.Container{vmsingleContainer}
-	initContainers := cr.Spec.InitContainers
+	var initContainers []corev1.Container
 
 	if cr.Spec.VMBackup != nil {
 		vmBackupManagerContainer, err := build.VMBackupManager(ctx, cr.Spec.VMBackup, cr.Spec.Port, storagePath, vmDataVolumeName, cr.Spec.ExtraArgs, false, cr.Spec.License)
@@ -323,8 +323,13 @@ func makeSpecForVMSingle(ctx context.Context, cr *vmv1beta1.VMSingle) (*corev1.P
 		}
 	}
 
-	build.AddStrictSecuritySettingsToContainers(cr.Spec.SecurityContext, operatorContainers, ptr.Deref(cr.Spec.UseStrictSecurity, false))
+	build.AddStrictSecuritySettingsToContainers(cr.Spec.SecurityContext, initContainers, ptr.Deref(cr.Spec.UseStrictSecurity, false))
+	ic, err := k8stools.MergePatchContainers(initContainers, cr.Spec.InitContainers)
+	if err != nil {
+		return nil, fmt.Errorf("cannot apply initContainer patch: %w", err)
+	}
 
+	build.AddStrictSecuritySettingsToContainers(cr.Spec.SecurityContext, operatorContainers, ptr.Deref(cr.Spec.UseStrictSecurity, false))
 	containers, err := k8stools.MergePatchContainers(operatorContainers, cr.Spec.Containers)
 	if err != nil {
 		return nil, err
@@ -337,7 +342,7 @@ func makeSpecForVMSingle(ctx context.Context, cr *vmv1beta1.VMSingle) (*corev1.P
 		},
 		Spec: corev1.PodSpec{
 			Volumes:            volumes,
-			InitContainers:     initContainers,
+			InitContainers:     ic,
 			Containers:         containers,
 			ServiceAccountName: cr.GetServiceAccountName(),
 		},

test/e2e/vmagent_test.go

@@ -236,6 +236,44 @@ var _ = Describe("test  vmagent Controller", func() {
 						}},
 					)).To(Succeed())
 
+				}),
+			Entry("with strict security", "strict-sec",
+				&v1beta1vm.VMAgent{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: namespace,
+						Name:      namespacedName.Name,
+					},
+					Spec: v1beta1vm.VMAgentSpec{
+						CommonDefaultableParams: v1beta1vm.CommonDefaultableParams{
+							UseStrictSecurity: ptr.To(true),
+						},
+						CommonApplicationDeploymentParams: v1beta1vm.CommonApplicationDeploymentParams{
+							ReplicaCount: ptr.To[int32](1),
+						},
+						RemoteWrite: []v1beta1vm.VMAgentRemoteWriteSpec{
+							{URL: "http://localhost:8428"},
+						},
+						SelectAllByDefault: true,
+					},
+				}, nil, func(cr *v1beta1vm.VMAgent) {
+					Eventually(func() string {
+						return expectPodCount(k8sClient, 1, namespace, cr.SelectorLabels())
+					}, eventualDeploymentPodTimeout, 1).Should(BeEmpty())
+					var dep appsv1.Deployment
+					Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cr.PrefixedName(), Namespace: namespace}, &dep)).To(Succeed())
+					Expect(dep.Spec.Template.Spec.SecurityContext).NotTo(BeNil())
+					Expect(dep.Spec.Template.Spec.SecurityContext.RunAsUser).NotTo(BeNil())
+					Expect(dep.Spec.Template.Spec.Containers).To(HaveLen(2))
+					Expect(dep.Spec.Template.Spec.InitContainers).To(HaveLen(1))
+					pc := dep.Spec.Template.Spec.Containers
+					pic := dep.Spec.Template.Spec.InitContainers
+					Expect(pc[0].SecurityContext).NotTo(BeNil())
+					Expect(pc[1].SecurityContext).NotTo(BeNil())
+					Expect(pic[0].SecurityContext).NotTo(BeNil())
+					Expect(pc[0].SecurityContext.AllowPrivilegeEscalation).NotTo(BeNil())
+					Expect(pc[1].SecurityContext.AllowPrivilegeEscalation).NotTo(BeNil())
+					Expect(pic[0].SecurityContext.AllowPrivilegeEscalation).NotTo(BeNil())
+
 				}),
 		)
 		type testStep struct {

test/e2e/vmalertmanager_test.go

@@ -119,6 +119,41 @@ var _ = Describe("test  vmalertmanager Controller", func() {
 					Expect(expectPodCount(k8sClient, 1, namespace, cr.SelectorLabels())).To(BeEmpty())
 				},
 			),
+			Entry("with strict security and vm config reloader", "strict-vmreloader-create",
+				&v1beta1vm.VMAlertmanager{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: namespace,
+					},
+					Spec: v1beta1vm.VMAlertmanagerSpec{
+						CommonConfigReloaderParams: v1beta1vm.CommonConfigReloaderParams{
+							UseVMConfigReloader: ptr.To(true),
+						},
+						CommonDefaultableParams: v1beta1vm.CommonDefaultableParams{
+							UseDefaultResources: ptr.To(false),
+							UseStrictSecurity:   ptr.To(true),
+						},
+						CommonApplicationDeploymentParams: v1beta1vm.CommonApplicationDeploymentParams{
+							ReplicaCount: ptr.To[int32](1),
+						},
+					},
+				},
+				func(cr *v1beta1vm.VMAlertmanager) {
+					Expect(expectPodCount(k8sClient, 1, namespace, cr.SelectorLabels())).To(BeEmpty())
+					var sts appsv1.StatefulSet
+					Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: namespace, Name: cr.PrefixedName()}, &sts)).To(Succeed())
+					ps := sts.Spec.Template.Spec
+					Expect(ps.SecurityContext).NotTo(BeNil())
+					Expect(ps.SecurityContext.RunAsNonRoot).NotTo(BeNil())
+					Expect(ps.Containers).To(HaveLen(2))
+					Expect(ps.InitContainers).To(HaveLen(1))
+					Expect(ps.Containers[0].SecurityContext).NotTo(BeNil())
+					Expect(ps.Containers[1].SecurityContext).NotTo(BeNil())
+					Expect(ps.InitContainers[0].SecurityContext).NotTo(BeNil())
+					Expect(ps.Containers[0].SecurityContext.AllowPrivilegeEscalation).NotTo(BeNil())
+					Expect(ps.Containers[1].SecurityContext.AllowPrivilegeEscalation).NotTo(BeNil())
+					Expect(ps.InitContainers[0].SecurityContext.AllowPrivilegeEscalation).NotTo(BeNil())
+				},
+			),
 		)
 
 		existAlertmanager := &v1beta1vm.VMAlertmanager{

test/e2e/vmauth_test.go

@@ -112,6 +112,20 @@ var _ = Describe("test vmauth Controller", func() {
 					},
 				}, func(cr *v1beta1vm.VMAuth) {
 					Expect(expectPodCount(k8sClient, 1, cr.Namespace, cr.SelectorLabels())).To(BeEmpty())
+					var dep appsv1.Deployment
+					Expect(k8sClient.Get(ctx, types.NamespacedName{Name: cr.PrefixedName(), Namespace: namespace}, &dep)).To(Succeed())
+					ps := dep.Spec.Template.Spec
+					Expect(ps.SecurityContext).NotTo(BeNil())
+					Expect(ps.SecurityContext.RunAsNonRoot).NotTo(BeNil())
+					Expect(ps.Containers).To(HaveLen(2))
+					Expect(ps.InitContainers).To(HaveLen(1))
+					Expect(ps.Containers[0].SecurityContext).NotTo(BeNil())
+					Expect(ps.Containers[1].SecurityContext).NotTo(BeNil())
+					Expect(ps.InitContainers[0].SecurityContext).NotTo(BeNil())
+					Expect(ps.Containers[0].SecurityContext.AllowPrivilegeEscalation).NotTo(BeNil())
+					Expect(ps.Containers[1].SecurityContext.AllowPrivilegeEscalation).NotTo(BeNil())
+					Expect(ps.InitContainers[0].SecurityContext.AllowPrivilegeEscalation).NotTo(BeNil())
+
 				}),
 			)