Custom SecurityContexts for envoy DaemonSet and contour Deployment #398

dmorgan81 · 2021-07-06T18:08:18Z

Allow users to specify a security context for both envoy daemonsets and contour deployments. The default value is unprivileged and is equivalent to the following:

contourSecurityContext:
  runAsUser: 65534
  runAsGroup: 65534
  runAsNonRoot: true

Note that this does change the default behavior for envoy daemonsets. Currently an envoy daemonset has an empty security context, meaning it will run with the user/group specified in the envoy image, which is commonly root. Switching to a non-root user by default will prevent the envoy container from binding to a low port. Existing setups will need to be updated by adding a envoySecurityContext entry with the appropriate settings if they need to bind to a low port.

Fixes #112
Updates #378

codecov · 2021-07-07T15:28:48Z

Codecov Report

Merging #398 (62e48a2) into main (7b041eb) will decrease coverage by 20.47%.
The diff coverage is 100.00%.

❗ Current head 62e48a2 differs from pull request most recent head 263c7c4. Consider uploading reports for the commit 263c7c4 to get more accurate results

@@             Coverage Diff             @@
##             main     #398       +/-   ##
===========================================
- Coverage   79.82%   59.34%   -20.48%     
===========================================
  Files          29       19       -10     
  Lines        2235     2071      -164     
===========================================
- Hits         1784     1229      -555     
- Misses        331      815      +484     
+ Partials      120       27       -93

Impacted Files	Coverage Δ
internal/objects/daemonset/daemonset.go	`83.33% <100.00%> (-12.39%)`	⬇️
internal/objects/deployment/deployment.go	`81.17% <100.00%> (-12.90%)`	⬇️
internal/objects/namespace/namespace.go	`15.00% <0.00%> (-50.00%)`	⬇️
internal/objects/role/role.go	`36.36% <0.00%> (-43.64%)`	⬇️
internal/objects/configmap/configmap.go	`34.14% <0.00%> (-43.07%)`	⬇️
internal/objects/rolebinding/role_binding.go	`41.07% <0.00%> (-39.29%)`	⬇️
...objects/clusterrolebinding/cluster_role_binding.go	`41.50% <0.00%> (-37.74%)`	⬇️
internal/objects/job/job.go	`53.04% <0.00%> (-28.70%)`	⬇️
internal/objects/service/service.go	`60.00% <0.00%> (-28.70%)`	⬇️
pkg/labels/labels.go	`75.00% <0.00%> (-25.00%)`	⬇️
... and 20 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b041eb...263c7c4. Read the comment docs.

dmorgan81 · 2021-07-07T20:25:19Z

I've been trying to run the e2e tests locally but I'm getting inconsistent results, even on the main branch:

TestContourNodePortService often hangs.
TestGateway and TestGatewayOwnership sometimes fail. The contour pods associated with these tests are stuck in a crash loop with the only logging being contour: error: invalid Contour configuration: invalid Gateway parameters specified: controllerName required, try --help

I just added a change to the e2e tests to use an empty PodSecurityContext for the contour pods since port 80 appears to be in the mix. However, I'm unable to get through the entire e2e test suite because of the above two issues.

sunjayBhatia · 2021-07-07T20:28:17Z

I've been trying to run the e2e tests locally but I'm getting inconsistent results, even on the main branch:

TestContourNodePortService often hangs.

TestGateway and TestGatewayOwnership sometimes fail. The contour pods associated with these tests are stuck in a crash loop with the only logging being contour: error: invalid Contour configuration: invalid Gateway parameters specified: controllerName required, try --help

I just added a change to the e2e tests to use an empty PodSecurityContext for the contour pods since port 80 appears to be in the mix. However, I'm unable to get through the entire e2e test suite because of the above two issues.

ah, this is probably not related to your change at all, i think this is related to this change in Contour being merged: projectcontour/contour@9e98a72

We need to fix this up on main and then you should be able to merge main and your change go green

nak3

I also wanted this feature! Thank you 👍

nak3 · 2021-07-09T06:19:32Z

internal/objects/deployment/deployment.go

@@ -304,7 +304,6 @@ func DesiredDeployment(contour *operatorv1alpha1.Contour, image string) *appsv1.
 					ServiceAccountName:            objutil.ContourRbacName,
 					RestartPolicy:                 corev1.RestartPolicyAlways,
 					SchedulerName:                 "default-scheduler",
-					SecurityContext:               objutil.NewUnprivilegedPodSecurity(),


I think we don't need to move this line.

If contour.ContourSecurityContextExists() is true, we just overrides deploy.Spec.Template.Spec.SecurityContext otherwise, use the default set here.

It might be premature optimization but I was hoping to avoid an unnecessary allocation.

nak3 · 2021-07-09T06:31:39Z

api/v1alpha1/contour_types.go

+	// +optional
+	ContourSecurityContext *corev1.PodSecurityContext `json:"contourSecurityContext,omitempty"`
+
+	//EnvoySecurityContext defines a PodSecurityContext for Envoy pods.


Suggested change

//EnvoySecurityContext defines a PodSecurityContext for Envoy pods.

// EnvoySecurityContext defines a PodSecurityContext for Envoy pods.

nit

nak3 · 2021-07-09T06:34:52Z

api/v1alpha1/helpers.go

@@ -95,3 +95,11 @@ func (c *Contour) EnvoyTolerationsExist() bool {

 	return false
 }
+


I think ContourSecurityContextExists and EnvoySecurityContextExists should have a comment line as these are public functions. I wonder why lint check does not produce the error 🤔

sunjayBhatia · 2021-07-22T20:19:35Z

@dmorgan81 I think this PR might be placed on a lower priority to merge at the moment since the Operator/Contour relationship is going through a rework as part of our Gateway API support work. Apologies for that, but we will try to get to this soon!

sunjayBhatia

Added projectcontour/contour#4039 and projectcontour/contour#4040 to track we want to make sure this is captured in the new world where Contour can manage the Envoy daemonset etc.

arjunrn · 2021-09-24T11:45:31Z

@dmorgan81 The jobs created by the operator will also have to use the custom security context. For reference.

Signed-off-by: David Morgan <[email protected]>

Allow setting custom security contexts for both contour deployments and envoy daemonsets. The default value is unpriviledged and is equivalent to the following: contourSecurityContext: runAsUser: 65534 runAsGroup: 65534 runAsNonRoot: true Fixes projectcontour#112 Updates projectcontour#378 Signed-off-by: David Morgan <[email protected]>

Signed-off-by: David Morgan <[email protected]>

skriss · 2022-09-21T21:21:05Z

This is stale, closing out.

dmorgan81 requested a review from a team as a code owner July 6, 2021 18:08

dmorgan81 requested review from stevesloka and sunjayBhatia and removed request for a team July 6, 2021 18:08

dmorgan81 force-pushed the custom-security-context branch from 650cce5 to 06e3b97 Compare July 6, 2021 18:09

dmorgan81 force-pushed the custom-security-context branch 2 times, most recently from d6db195 to dc0e198 Compare July 7, 2021 19:32

nak3 reviewed Jul 9, 2021

View reviewed changes

amdonov mentioned this pull request Jul 9, 2021

Running with UID 101 and GID 101 when using high ports #393

Closed

arjunrn mentioned this pull request Sep 6, 2021

Custom security context openshift/contour-operator#7

Merged

This was referenced Sep 17, 2021

ContourConfiguration CRD should allow custom SecurityContexts for Envoy DaemonSet projectcontour/contour#4039

Closed

ContourDeployment CRD should allow custom SecurityContext for Contour deployment projectcontour/contour#4040

Closed

sunjayBhatia reviewed Sep 17, 2021

View reviewed changes

dmorgan81 force-pushed the custom-security-context branch from d81aadc to 87b76dd Compare September 30, 2021 01:27

David Morgan added 5 commits September 30, 2021 08:09

API changes for custom security contexts

623f0d2

Signed-off-by: David Morgan <[email protected]>

Update manifest

9fa8165

Signed-off-by: David Morgan <[email protected]>

Add empty PodSecurityContext for e2e tests

eca1cb2

Signed-off-by: David Morgan <[email protected]>

Fix up some commenting issues

263c7c4

Signed-off-by: David Morgan <[email protected]>

dmorgan81 force-pushed the custom-security-context branch from 87b76dd to 263c7c4 Compare September 30, 2021 12:10

skriss closed this Sep 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom SecurityContexts for envoy DaemonSet and contour Deployment #398

Custom SecurityContexts for envoy DaemonSet and contour Deployment #398

dmorgan81 commented Jul 6, 2021

codecov bot commented Jul 7, 2021 •

edited

Loading

dmorgan81 commented Jul 7, 2021

sunjayBhatia commented Jul 7, 2021

nak3 left a comment

nak3 Jul 9, 2021 •

edited

Loading

dmorgan81 Jul 9, 2021

nak3 Jul 9, 2021

nak3 Jul 9, 2021 •

edited

Loading

sunjayBhatia commented Jul 22, 2021

sunjayBhatia left a comment

arjunrn commented Sep 24, 2021

skriss commented Sep 21, 2022

	//EnvoySecurityContext defines a PodSecurityContext for Envoy pods.
	// EnvoySecurityContext defines a PodSecurityContext for Envoy pods.

		@@ -95,3 +95,11 @@ func (c *Contour) EnvoyTolerationsExist() bool {

		return false
		}

Custom SecurityContexts for envoy DaemonSet and contour Deployment #398

Custom SecurityContexts for envoy DaemonSet and contour Deployment #398

Conversation

dmorgan81 commented Jul 6, 2021

codecov bot commented Jul 7, 2021 • edited Loading

Codecov Report

dmorgan81 commented Jul 7, 2021

sunjayBhatia commented Jul 7, 2021

nak3 left a comment

Choose a reason for hiding this comment

nak3 Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

dmorgan81 Jul 9, 2021

Choose a reason for hiding this comment

nak3 Jul 9, 2021

Choose a reason for hiding this comment

nak3 Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

sunjayBhatia commented Jul 22, 2021

sunjayBhatia left a comment

Choose a reason for hiding this comment

arjunrn commented Sep 24, 2021

skriss commented Sep 21, 2022

codecov bot commented Jul 7, 2021 •

edited

Loading

nak3 Jul 9, 2021 •

edited

Loading

nak3 Jul 9, 2021 •

edited

Loading