introduce authentication/authorization at the EventListenerTrigger le…

[gabemontero]

…vel (with default back to existing EventListener level)

Changes

While this does not fully address #77, this change intends to provide more more granular authorization, besides just the EventListener service account,
and sets the stage for "multi-tenant themed" authorization around creating tekton resources
from triggers and the EventListener sink.

As discussed in recent instances of the weekly project sync call, the actual employing of SubjectAccessReview checks (and the k8s admission related resources those would require) belongs in the https://github.com/tektoncd/pipelines project, where the identity and permissions
around the existing SA of the EventListener or the new SA for the EventListenerTrigger introduced here would be used for any SAR calls in the core pipeline controller.

As such, we could ultimately gate merging of this, if we reached consensus on this, on that tektoncd/pipeline work. However, this change does not require that work. And it provides an opt in
to finer grained control over which if the tekton CRD types can be created from a given trigger.

And certainly, while reviewing this, if finer grained authorization is deemed desirable at all, one possible outcome is that we want even finer grained control, and incorporate service account on objects with say the EventListenerTrigger

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

See the contribution guide for more details.

Release Notes

The specification of a service account on the EventListenerTrigger is now available as an override of the service account of the EventListener to facilitate permissions changes wrt creating Tekton objects as a result of the sink receiving an event.

[tekton-robot]

Hi @gabemontero. 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.

[gabemontero]

@vdemeester @pmorie @chmouel FYI

[gabemontero]

I'll defer on assigning reviewers myself until either one of the project admins does it, or after we've discussed this PR on the sync call and decided who should get assigned.

[gabemontero]

/work-in-progress cancel

[gabemontero]

/assign @wlynch

[gabemontero]

/assign @wlynch
/assign @khrm

[vdemeester]

/ok-to-test

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/resources/create.go	85.7%	81.8%	-3.9
test/builder/eventlistener.go	95.2%	92.2%	-3.0

[gabemontero]

ok at least fixed the gofmt 's

need to look into my new integration test

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/resources/create.go	85.7%	81.8%	-3.9
test/builder/eventlistener.go	95.2%	92.2%	-3.0

[gabemontero]

eventlistener_test.go:540: Error creating ClusterTriggerBinding: clustertriggerbindings.tekton.dev "my-clustertriggerbinding" already exists

my new test's reuse needs some work

/work-in-progress

[gabemontero]

quick update: I have my new auth test passing locally when I temporary disable the existing test, in that the trigger SA without perms is getting forbidden on the create resources attempt.

When I next get to this I'll either:

• figure out how to use the event listener's el-port option from the integration tests so concurrent tests listen on different ports
• or simply merge the tests to run in succession off of the same EL instance

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/resources/create.go	85.7%	81.8%	-3.9
test/builder/eventlistener.go	95.2%	92.2%	-3.0
test/controller.go	75.2%	73.0%	-2.3

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/resources/create.go	85.7%	81.8%	-3.9
test/builder/eventlistener.go	95.2%	92.2%	-3.0
test/controller.go	75.2%	73.0%	-2.3

[gabemontero]

OK another rebase based on recent changes causing conflicts pushed

@dibyom @ncskier - with a scenario documented in #456 what are the final steps needed for possibly tagging this PR for merge (assuming my merges for conflicts pass all the tests)?

thanks

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/resources/create.go	85.7%	81.8%	-3.9
test/builder/eventlistener.go	95.2%	92.2%	-3.0
test/controller.go	75.2%	73.0%	-2.3

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/resources/create.go	85.7%	81.8%	-3.9
test/builder/eventlistener.go	95.2%	92.2%	-3.0
test/controller.go	75.2%	73.0%	-2.3

[gabemontero]

OK another rebase based on recent changes causing conflicts pushed

@dibyom @ncskier - with a scenario documented in #456 what are the final steps needed for possibly tagging this PR for merge (assuming my merges for conflicts pass all the tests)?

OK tests all green again

thanks

[gabemontero]

Need another rebase ...

[tekton-robot]

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

File	Old Coverage	New Coverage	Delta
pkg/resources/create.go	85.7%	81.8%	-3.9
test/builder/eventlistener.go	95.2%	92.2%	-3.0
test/controller.go	75.2%	73.0%	-2.3

[gabemontero]

Perhaps another "scenario", I also have some usage experience from other CI systems which I believe have bearing here.

I certainly don't think it makes sense to reference it in the doc updates in any way. And I thought I mentioned it on prior calls, but if need be, I can further detail / reiterate in either forum.

Perhaps a discussion point for today's call.

[gabemontero]

latest rebase up / tests clean

[tekton-robot]

[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

[dibyom]

/lgtm

[gabemontero]

level=error msg="Timeout exceeded: try increasing it by passing --timeout option"

/retest

[dibyom]

Thanks for adding this @gabemontero Appreciate your patience with this PR!
One thing that would be really cool to add would be a full example of an EventListener using two different service accounts (e.g. like the Gitlab one)

[gabemontero]

Thanks for adding this @gabemontero Appreciate your patience with this PR!

My pleasure @dibyom and likewise appreciate the patience and openness wrt the PR. Authorization related matters are certainly not to be taken lightly.

One thing that would be really cool to add would be a full example of an EventListener using two different service accounts (e.g. like the Gitlab one)

I would agree. Poking around the examples, it seems like we would add the new SA and a more precise/specific set of roles under https://github.com/tektoncd/triggers/tree/master/examples/role-resources , and then provide an alternative to the example you noted accordingly, agree ?

I'm thinking I'm going to circle back with @khrm @siamaksade and @vdemeester on my end and see if we want to do a short term example independent of @khrm 's upcoming work, or if that might land reasonably soon, and use that instead to drive the example.

One of us will report back once we've had a chance to confer.

thanks again

[dibyom]

I would agree. Poking around the examples, it seems like we would add the new SA and a more precise/specific set of roles under https://github.com/tektoncd/triggers/tree/master/examples/role-resources , and then provide an alternative to the example you noted accordingly, agree ?

Yeah, we'd need a couple of more specific roles/SAs. They could either live with the example itself or in the role-resources folder...the role resources folder is meant for more of the "common" resources that are shared between multiple examples.

I'm thinking I'm going to circle back with @khrm @siamaksade and @vdemeester on my end and see if we want to do a short term example independent of @khrm 's upcoming work, or if that might land reasonably soon, and use that instead to drive the example.

Sounds good to me! Thank you!