switch trigger sa based auth to impersonate; change secret ref to string and by extension same namespace as EventListener #705

gabemontero · 2020-08-10T19:53:15Z

Changes

As the efforts to progress support for multi-tenant event listeners has evolved, most notably around the notion of breaking out EventListenerTrigger into a Trigger CRD, or an analogous resource like a PipelineRecipe defined in the pipelines project, some evolution of the finer grained auth originally introduced in #454 seemed appropriate.

This PR currently has 2 commits with that goal in mind:

switch the secret reference originally introduced in introduce authentication/authorization at the EventListenerTrigger le… #454 from a ObjectReference to a LocalObjectReference, hence scoping the SA's available for use to the SA's present in the namespace of the trigger object itself; we originally scoped it as ObjectReference because the interceptors had a a multi-namespace scope for secrets per https://github.com/tektoncd/triggers/blob/master/pkg/apis/triggers/v1alpha1/event_listener_types.go#L97-L104 however as the various TEPs/proposals have evolved that no longer seems like a valid motivator for having this SA reference be multi namespace
as a result, the implementation for creating tekton objects under the permission of the referenced SA has been switched to use impersonation vs. construction of an actual client; this allowed for some positive changes in the roles required, but also should fit in well with how the upcoming TEPs / proposals will get implemented

And as a result this

Fixes #679

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

BREAKING CHANGE: the optional EventListenerTrigger based level of authentication for creating Tekton object has had its ServiceAccount reference changed from an ObjectReference to a string ServiceAccountName, effectively enforcing that the ServiceAccount be in the same namespace as the EventListenerTrigger

tekton-robot · 2020-08-10T19:55:31Z

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/sink/auth_override.go	43.1%	36.2%	-6.9
pkg/sink/sink.go	75.2%	76.3%	1.0

tekton-robot · 2020-08-10T20:35:28Z

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/sink/auth_override.go	43.1%	36.2%	-6.9
pkg/sink/sink.go	75.2%	76.3%	1.0

tekton-robot · 2020-08-10T20:53:03Z

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/sink/auth_override.go	43.1%	38.2%	-4.9
pkg/sink/sink.go	75.2%	76.3%	1.0

tekton-robot · 2020-08-10T21:08:18Z

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/sink/auth_override.go	43.1%	38.2%	-4.9
pkg/sink/sink.go	75.2%	76.3%	1.0

tekton-robot · 2020-08-10T21:28:40Z

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/sink/auth_override.go	43.1%	38.2%	-4.9
pkg/sink/sink.go	75.2%	76.3%	1.0

tekton-robot · 2020-08-10T21:43:36Z

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/sink/auth_override.go	43.1%	38.2%	-4.9
pkg/sink/sink.go	75.2%	76.3%	1.0

tekton-robot · 2020-08-11T00:26:17Z

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/sink/auth_override.go	43.1%	38.2%	-4.9
pkg/sink/sink.go	75.2%	76.3%	1.0

gabemontero · 2020-08-11T13:08:10Z

/assign @dibyom
/assign @khrm

details in the description gentlemen

similar in spirit to #704 but a more comprehensive change as it evolves the support I added back in Feb / Mar of this year.

dibyom · 2020-08-11T14:23:40Z

Thanks @gabemontero I think this should address #679 too?

gabemontero · 2020-08-11T14:39:44Z

Thanks @gabemontero I think this should address #679 too?

yes it does @dibyom

gabemontero · 2020-08-11T14:49:19Z

forgot to

/hold cancel

khrm

/lgtm

I think we should move to ServiceAccountName string instead of taking it as ServiceAccount LocalObjectReference. Maybe a pr later on which deprecate ServiceAccount in favour of ServiceAccountName.

Then, this will have the same format as we do in other Tekton resources which requires ServiceAccount.

gabemontero · 2020-08-13T13:02:13Z

/lgtm

I think we should move to ServiceAccountName string instead of taking it as ServiceAccount LocalObjectReference. Maybe a pr later on which deprecate ServiceAccount in favour of ServiceAccountName.

Then, this will have the same format as we do in other Tekton resources which requires ServiceAccount.

I'm fine with switching to ServiceAccountName with this PR. I'll do that momentarily, and then between the two of you @khrm @dibyom unless anything else comes to mind, feel free to both approve and lgtm. Of course if @dibyom has issue with the move off of LocalObjectReference we can discuss further.

thanks

tekton-robot · 2020-08-13T13:57:37Z

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/sink/auth_override.go	43.1%	0.0%	-43.1
pkg/sink/sink.go	75.2%	76.3%	1.0

tekton-robot · 2020-08-13T13:58:33Z

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/sink/auth_override.go	43.1%	0.0%	-43.1
pkg/sink/sink.go	75.2%	76.3%	1.0

gabemontero · 2020-08-13T13:58:55Z

switch from LocalObjectReference to string pushed

adjusted PR title

Also added Fixes #679 note in description

@dibyom @khrm PTAL

gabemontero · 2020-08-13T13:59:56Z

oh and I removed more unneeded code from auth_override.go, hence the delta change

we are now down to one method which we have to use the "fake" version of in the unit tests to bypass actual client access to a cluster

the function is covered by the e2e that validates trigger level SA

tekton-robot · 2020-08-13T14:03:25Z

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/sink/auth_override.go	43.1%	0.0%	-43.1
pkg/sink/sink.go	75.2%	76.3%	1.0

tekton-robot · 2020-08-13T14:53:01Z

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/sink/auth_override.go	43.1%	0.0%	-43.1
pkg/sink/sink.go	75.2%	76.3%	1.0

tekton-robot · 2020-08-13T15:56: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/sink/auth_override.go	43.1%	0.0%	-43.1
pkg/sink/sink.go	75.2%	76.3%	1.0

…mespace as EventListener

tekton-robot · 2020-08-13T16:30:27Z

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/sink/auth_override.go	43.1%	0.0%	-43.1
pkg/sink/sink.go	75.2%	76.3%	1.0

gabemontero · 2020-08-13T20:15:42Z

tests back to green after the switch to serviceAccountName string

PTAL @khrm @dibyom

dibyom · 2020-08-13T21:28:38Z

/approve

dibyom · 2020-08-13T21:33:27Z

oh and I removed more unneeded code from auth_override.go, hence the delta change

we are now down to one method which we have to use the "fake" version of in the unit tests to bypass actual client access to a cluster

the function is covered by the e2e that validates trigger level SA

Thanks for the clarification!

khrm

/lgtm

tekton-robot · 2020-08-17T05:22:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dibyom, khrm

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,khrm]

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Aug 10, 2020

tekton-robot requested review from dlorenc and wlynch August 10, 2020 19:53

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 10, 2020

gabemontero force-pushed the trigger-sa-impersonate branch from ff655ae to 16fa689 Compare August 10, 2020 20:32

gabemontero force-pushed the trigger-sa-impersonate branch from 16fa689 to da10d43 Compare August 10, 2020 20:50

gabemontero force-pushed the trigger-sa-impersonate branch from da10d43 to c895aff Compare August 10, 2020 21:07

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 10, 2020

gabemontero force-pushed the trigger-sa-impersonate branch from c895aff to d5851cc Compare August 10, 2020 21:26

gabemontero force-pushed the trigger-sa-impersonate branch from d5851cc to 859472a Compare August 10, 2020 21:41

gabemontero force-pushed the trigger-sa-impersonate branch from 19f19e0 to 66f42f1 Compare August 11, 2020 00:24

gabemontero changed the title ~~WIP: switch trigger sa based auth to impersonate~~ switch trigger sa based auth to impersonate; change secret ref to LocalObjectReference Aug 11, 2020

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2020

tekton-robot assigned dibyom and khrm Aug 11, 2020

tekton-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2020

switch trigger sa based auth to impersonate

87709b2

khrm reviewed Aug 13, 2020

View reviewed changes

gabemontero force-pushed the trigger-sa-impersonate branch from 3835cfc to 2ba7ea1 Compare August 13, 2020 13:55

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 13, 2020

gabemontero force-pushed the trigger-sa-impersonate branch from 2ba7ea1 to 690cfe6 Compare August 13, 2020 13:56

gabemontero changed the title ~~switch trigger sa based auth to impersonate; change secret ref to LocalObjectReference~~ switch trigger sa based auth to impersonate; change secret ref to string and by extension same namespace as EventListener Aug 13, 2020

gabemontero force-pushed the trigger-sa-impersonate branch from 690cfe6 to 5e53cc7 Compare August 13, 2020 14:01

gabemontero force-pushed the trigger-sa-impersonate branch from 5e53cc7 to 2ce2246 Compare August 13, 2020 14:51

gabemontero force-pushed the trigger-sa-impersonate branch from 2ce2246 to d774140 Compare August 13, 2020 15:54

switch EventListenerTrigger sa ref to string and by extension same na…

f6264b9

…mespace as EventListener

gabemontero force-pushed the trigger-sa-impersonate branch from d774140 to f6264b9 Compare August 13, 2020 16:28

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

dibyom mentioned this pull request Aug 14, 2020

EventListener ServiceAccountName should default to... default #682

Closed

gabemontero mentioned this pull request Aug 16, 2020

Users should only be able to create TriggerTemplates for resources they are permissioned to use #77

Closed

khrm approved these changes Aug 17, 2020

View reviewed changes

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

tekton-robot merged commit 79884ef into tektoncd:master Aug 17, 2020

gabemontero deleted the trigger-sa-impersonate branch August 17, 2020 12:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

switch trigger sa based auth to impersonate; change secret ref to string and by extension same namespace as EventListener #705

switch trigger sa based auth to impersonate; change secret ref to string and by extension same namespace as EventListener #705

gabemontero commented Aug 10, 2020 •

edited

Loading

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 11, 2020

gabemontero commented Aug 11, 2020 •

edited

Loading

dibyom commented Aug 11, 2020

gabemontero commented Aug 11, 2020

gabemontero commented Aug 11, 2020

khrm left a comment

gabemontero commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

gabemontero commented Aug 13, 2020

gabemontero commented Aug 13, 2020 •

edited

Loading

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

gabemontero commented Aug 13, 2020

dibyom commented Aug 13, 2020

dibyom commented Aug 13, 2020

khrm left a comment

tekton-robot commented Aug 17, 2020

switch trigger sa based auth to impersonate; change secret ref to string and by extension same namespace as EventListener #705

switch trigger sa based auth to impersonate; change secret ref to string and by extension same namespace as EventListener #705

Conversation

gabemontero commented Aug 10, 2020 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 10, 2020

tekton-robot commented Aug 11, 2020

gabemontero commented Aug 11, 2020 • edited Loading

dibyom commented Aug 11, 2020

gabemontero commented Aug 11, 2020

gabemontero commented Aug 11, 2020

khrm left a comment

Choose a reason for hiding this comment

gabemontero commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

gabemontero commented Aug 13, 2020

gabemontero commented Aug 13, 2020 • edited Loading

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

tekton-robot commented Aug 13, 2020

gabemontero commented Aug 13, 2020

dibyom commented Aug 13, 2020

dibyom commented Aug 13, 2020

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Aug 17, 2020

gabemontero commented Aug 10, 2020 •

edited

Loading

gabemontero commented Aug 11, 2020 •

edited

Loading

gabemontero commented Aug 13, 2020 •

edited

Loading