Add flag to disable security context on EL Deployment #865

MarcelMue · 2020-12-16T11:49:13Z

Changes

This is a follow up to #864 - it introduces a flag which removes the security context from all created deployments. This enables triggers to work correctly on OpenShift.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Add a flag to disable SecurityContext on the reconciled eventlistener Deployment

tekton-robot · 2020-12-16T11:49:28Z

Hi @MarcelMue. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

MarcelMue · 2020-12-16T11:51:01Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

@@ -61,12 +61,15 @@ const (
 )

 var (
-	// The container that we use to run in the EventListener Pods
-	elImage = flag.String("el-image", "override-with-el:latest",
+	// ELImage defines the container that we use to run in the EventListener Pods


I renamed this flag to capitalize it like the other flags. Mainly for consistency.

MarcelMue · 2020-12-16T11:51:55Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

 		"The container image for the EventListener Pod.")
 	// ElPort defines the port for the EventListener to listen on
 	ElPort = flag.Int("el-port", 8080,
 		"The container port for the EventListener to listen on.")
+	// ELSetSecurityContext defines if the security context is set.
+	ELSetSecurityContext = flag.Bool("el-security-context", true,


I would like to unit test this but have no idea how. Are there any examples where it's easy to tell how flags can be invoked for unit tests in this project?

Current idea for me is to re-factor the flag setup in this controller to allow unit testing different values.

One way might be to refactor one of the functions e.g. reconcileDeployment (or getDeployment) to take in a parameter called isSecurityContexEnabled and then we write unit tests for those functions (we lack those smaller unit tests today but maybe we should add them)

Another way might be to add these as fields in the Reconciler struct and we can set the right values in the test Reconcilers.

Another way might be to add these as fields in the Reconciler struct and we can set the right values in the test Reconcilers.

This is what I want to go for currently. I would like to do this as a follow up to this PR to prevent this from getting too big.

sounds good to me!

dibyom · 2020-12-16T15:55:05Z

/ok-to-test

tekton-robot · 2020-12-16T15:56:38Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	78.6%	78.8%	0.2

tekton-robot · 2020-12-16T16:27:56Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	78.6%	78.8%	0.2

savitaashture · 2020-12-16T16:41:56Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

 		"The container image for the EventListener Pod.")
 	// ElPort defines the port for the EventListener to listen on
 	ElPort = flag.Int("el-port", 8080,
 		"The container port for the EventListener to listen on.")
+	// ELSetSecurityContext defines if the security context is set.
+	ELSetSecurityContext = flag.Bool("el-security-context", true,


Forgot to mention this during WG call
I think If we keep default value as true then for openshift platform we need to update the controller deployment to set the flag to false
I believe which is extra step
so considering this shall we make default as false
@dibyom @bigkevmcd @gabemontero WDYT?

Yeah, my understanding is that openshift already generates/modifies some of the YAML so it should be able to change/add the flag here: https://github.com/tektoncd/triggers/blob/master/config/controller.yaml#L62

Would that work?

Yeah @savitaashture I think upstream needs to default to being able to function securely out of the box, which means being able to function if PSP is set to more restrictive default policies like what @MarcelMue and Jose experienced.

Which means default to true, no?

And then if openshift has to tweak more yaml like we do already when dealing with SCC vs. PSP, that that is what we have to do.

@vdemeester @piyush-garg FYI

Agreed, default to true is ok for me 👍🏼

We will need to modify release.yaml for openshift because controller and webhook also have hardcoded UUID. So we can do for this flag also. We already do this for pipelines controller and webhook.

The only downside of the flag approach (compared to a configmap approach) is that a user that wants to run upstream triggers on OpenShift needs to edit the deployment, where, otherwise only a configmap edition is required (which is.. simpler..). But I am definitely fine with the flag 😉

yeah in general, the number of flags are increasing...so it'd be nice to move to a configMap at some point.

MarcelMue · 2020-12-17T11:03:38Z

I think this should be ready to merge now. Unit tests currently don't exist and will be added in a follow up. I can not find any place where we document these flags - correct me if I am wrong.

tekton-robot · 2020-12-17T11:03:54Z

The following is the coverage report on the affected files.
Say /test pull-tekton-triggers-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	78.6%	78.8%	0.2

dibyom · 2020-12-17T16:10:54Z

/approve
/lgtm

tekton-robot · 2020-12-17T16:10:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dibyom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 16, 2020

tekton-robot requested review from ncskier and wlynch December 16, 2020 11:49

tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 16, 2020

MarcelMue commented Dec 16, 2020

View reviewed changes

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Dec 16, 2020

savitaashture reviewed Dec 16, 2020

View reviewed changes

Add flag to disable security context on EL Deployment

1033d70

MarcelMue force-pushed the add-security-context-flag branch from 76d5460 to 1033d70 Compare December 17, 2020 11:02

tekton-robot assigned dibyom Dec 17, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2020

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 17, 2020

tekton-robot merged commit 9edbdd9 into tektoncd:master Dec 17, 2020

MarcelMue deleted the add-security-context-flag branch December 17, 2020 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag to disable security context on EL Deployment #865

Add flag to disable security context on EL Deployment #865

MarcelMue commented Dec 16, 2020 •

edited

Loading

tekton-robot commented Dec 16, 2020

MarcelMue Dec 16, 2020

MarcelMue Dec 16, 2020

MarcelMue Dec 16, 2020

dibyom Dec 16, 2020

MarcelMue Dec 17, 2020

dibyom Dec 17, 2020

dibyom commented Dec 16, 2020

tekton-robot commented Dec 16, 2020

tekton-robot commented Dec 16, 2020

savitaashture Dec 16, 2020 •

edited

Loading

dibyom Dec 16, 2020

gabemontero Dec 16, 2020

savitaashture Dec 17, 2020

vdemeester Dec 17, 2020

khrm Dec 17, 2020 •

edited

Loading

vdemeester Dec 17, 2020

dibyom Dec 17, 2020

MarcelMue commented Dec 17, 2020

tekton-robot commented Dec 17, 2020

dibyom commented Dec 17, 2020

tekton-robot commented Dec 17, 2020

Add flag to disable security context on EL Deployment #865

Add flag to disable security context on EL Deployment #865

Conversation

MarcelMue commented Dec 16, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Dec 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dibyom commented Dec 16, 2020

tekton-robot commented Dec 16, 2020

tekton-robot commented Dec 16, 2020

savitaashture Dec 16, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khrm Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarcelMue commented Dec 17, 2020

tekton-robot commented Dec 17, 2020

dibyom commented Dec 17, 2020

tekton-robot commented Dec 17, 2020

MarcelMue commented Dec 16, 2020 •

edited

Loading

savitaashture Dec 16, 2020 •

edited

Loading

khrm Dec 17, 2020 •

edited

Loading