if eventlistenertrigger sa not set, assume user means default sa #836
Conversation
tekton-robot
added
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
size/XS
|
The following is the coverage report on the affected files.
|
gabemontero
force-pushed
the
explicit-default-sa-default
branch
from
00fe122
to
5269c86
Compare
|
fyi even some unit tests failed ... I left them as is to help asses the total scope of change that would be required for changing behavior in this way |
|
The following is the coverage report on the affected files.
|
|
So aside from the unit tests, only the one e2e I was sure would fail (since that is where I had to add the granular auth via impersonation stuff), though iirc and read the test output correctly, I do no think the integration run cycles through the examples at https://github.com/tektoncd/triggers/tree/master/examples I would suspect failures in that set as well with this change, where set up would need to be changes to add requisite role bindings to the default SA in the associated namespace(s). But I'll leave it at that and see what everyone's reactions are. Note, I'll be on PTO all of Thanksgiving week next week, but should be able to check out comments afterward, as well as attend subsequent WG meetings. |
gabemontero
force-pushed
the
explicit-default-sa-default
branch
from
5269c86
to
7e3d842
Compare
tekton-robot
added
size/S
|
The following is the coverage report on the affected files.
|
|
I lied ... one more consideration ;-) The same approach could be applied at the trigger SA as well. Essentially force it to the default SA instead of using the Sink's dynamic client (i.e. the SA associated with the sink). separate commit pushed. I now think the "defaulting" makes more sense at the trigger level than the eventlistenertrigger level, as that is where the "multi-tenant" dividing line will fall, if I recall the TEP correctly, but we can sort that out either in discussion here on in the next WG I can attend. |
|
@gabemontero: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
1 similar comment
|
@gabemontero: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| @@ -173,6 +173,12 @@ func (r Sink) HandleEvent(response http.ResponseWriter, request *http.Request) { | |||
| func (r Sink) merge(et []triggersv1.EventListenerTrigger, trItems []*triggersv1.Trigger) ([]*triggersv1.Trigger, error) { | |||
| triggers := trItems | |||
| for _, t := range et { | |||
| // if ELT serviceAccountName not set, take that as assuming the user if fine with using the default SA; | |||
| // but no longer allow the EventListener SA and its permissions to influence what can be created | |||
| serviceAccountName := "default" | |||
There was a problem hiding this comment.
Curious - alternatively, instead of explicitly setting default SA, do you think it would also work to conditionally set TriggerSpec.serviceaccount based on presence of t.ServiceAccountName after 201, so we could leave it blank when omitted? Would that would achieve the same result, since the SA would default to default?
There was a problem hiding this comment.
not exactly @iancoffey .... the intent on always setting it explicitly here is so that the event listener sink's use of k8s impersonation kicks in, and as a result, any subsequent creation of objects are from the start done using permissions OTHER than the permissions of the event listener sink.
|
closing this experiment out for now ... may revisit as other features reach more maturation ... see #77 (comment) |
Changes
/hold
A POC experiment related to #77 and mult-tenant event listeners
@dibyom @khrm @vdemeester FYI
First, I'm curious what breaks and what does not today when we take the "optional" nature of the ELT SA ref toward
always relevant, in that if they don't set it, we do the same thing k8s does with Pods, and assigns the
defaultSA to the Pod.Today, we end up using the EL's SA and its permissions when executing the trigger if this ELT SA is not set.
When multi-tenant event listeners become a full reality, the permission ramifications if any increase, as the multi tenant EL, presumably with enough permissions to operator on triggers with any of the namespaces it manages, come into play and only increases the potential enhanced and escalated permission flaw.
And thus, the need for making the ELT SA minimally explicit (i.e. we use default if not set) or required (they user has to put a valid sa ref) is more vital in the multi-tenant event listener case.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Release Notes