remove interceptor cross namespace secret references; adjust controller permission accordingly

[gabemontero]

Changes

Per recent discussions in the WG, the ability for interceptors to access secrets in other namespaces
appears to be an unintended feature, where we know of no use of it at this time.

Aside from the security implications with the controller accessing secrets from any namespace, eliminating this ability aids the multi-tenant eventlistener migration, as that design scopes the interceptor along with the trigger to within a namespace only.

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: any interceptors that attempt to reference a Secret outside of the EventListener's namespace will have to change to have those Secrets reside in the EventListener's namespace.

[tekton-robot]

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/interceptors/cel/triggers.go	88.3%	88.8%	0.5
pkg/interceptors/interceptors.go	80.0%	82.4%	2.4

[tekton-robot]

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/interceptors/cel/triggers.go	88.3%	88.6%	0.3
pkg/interceptors/interceptors.go	80.0%	82.4%	2.4

[gabemontero]

OK @dibyom @wlynch @khrm the tests are clean and I have the release note ready.

I did not find any doc references to the SecretRef Namespace field. But double check my greps, etc. with any recollections you all have.

Two questions:

1. How if any do we want to further surface this for any potential feedback? associated github issue? tekton-dev group notification? triggers slack channel notification? all of the above? none of the above / this PR and the release note when the new release goes out suffices?

2. who should be assigned the PR?

thanks

@skaegi FYI ... access to secrets is reduced to the pipeline namespace with this PR

[dibyom]

How if any do we want to further surface this for any potential feedback? associated github issue? tekton-dev group notification? triggers slack channel notification? all of the above? none of the above / this PR and the release note when the new release goes out suffices?

All of those sound like good ideas. I think a slack message in #general or #triggers and maybe a mail to the tekton-dev group? Maybe we can also point it out in the next Tekton WG meeting.

who should be assigned the PR?

I'm happy to take a look at it.

[gabemontero]

How if any do we want to further surface this for any potential feedback? associated github issue? tekton-dev group notification? triggers slack channel notification? all of the above? none of the above / this PR and the release note when the new release goes out suffices?

All of those sound like good ideas. I think a slack message in #general or #triggers and maybe a mail to the tekton-dev group? Maybe we can also point it out in the next Tekton WG meeting.

who should be assigned the PR?

I'm happy to take a look at it.

/assign @dibyom

thanks :-)

[tekton-robot]

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/interceptors/cel/triggers.go	88.3%	88.6%	0.3
pkg/interceptors/interceptors.go	80.0%	82.4%	2.4

[dibyom]

🎉

[dibyom]

Hmmm, so why does the EL reconciler need access to secrets in the first place 🤔 ? As far as I know, only the EL sink's SA should need access to secrets.

[gabemontero]

Oooh ... interesting @dibyom

I think you are right. Ultimately it is the interceptors called within the sink that are getting the secret token for the git*/bitbucket/etc. verification that we have de-scoped to be local namespace only.

And I see no other use of the k8s secret client.

So that would imply not even creating the new 200-role.yaml and 201-rolebinding.yaml files I have currently in the PR, and instead updating the event listener / sink related yaml under the examples folder, right?

On the plus side, being this much of a newbie path makes me feel young :-)

I'll work on some changes tomorrow / Friday and we'll see how the PR tests fair.

thanks

[tekton-robot]

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/interceptors/cel/triggers.go	88.3%	88.6%	0.3
pkg/interceptors/interceptors.go	80.0%	82.4%	2.4

[gabemontero]

integrations tests still pass, but I'm not sure they run the examples

will look into it Friday and run manually at least if needed

[gabemontero]

OK I see the https://github.com/tektoncd/triggers/blob/master/test/e2e-tests-yaml.sh that is analogous to what I've seen in tektoncd/pipelines, but I can't find evidence in the integration-tests run or build-tests run where it is utilized

Does that sound right to your @dibyom ?

In any event, I ran through the instructions at https://github.com/tektoncd/triggers/tree/master/examples manually with this branch deployed and it functioned correctly.

Let me know on about the testing and rest of the code changes @dibyom
Once we are good there, I'll squash the commits and go through the announcement protocol established in #748 (comment)

thanks

[dibyom]

Thanks @gabemontero
This looks good to me!
/lgtm

[gabemontero]

How if any do we want to further surface this for any potential feedback? associated github issue? tekton-dev group notification? triggers slack channel notification? all of the above? none of the above / this PR and the release note when the new release goes out suffices?

All of those sound like good ideas. I think a slack message in #general or #triggers and maybe a mail to the tekton-dev group?

Messages have been posted to the two slack channels and the tekton users group.

[gabemontero]

OK, back from PTO. Looks like I have to rebase this PR, but beforehand, @dibyom - your thoughts on the feedback from the public announcements. If I recall correctly, there was only a minor exchange with you and @afrittoli

I do not think anything concerning stemmed from it, but I'll admit I do not fully recall.

Thoughts gentlemen ?

[dibyom]

Hope you had a nice break @gabemontero 🏖️

I've heard any concerning feedback, so I'm ok merging this once it is rebased!

[gabemontero]

Hope you had a nice break @gabemontero beach_umbrella

thanks @dibyom !

I've heard any concerning feedback, so I'm ok merging this once it is rebased!

rebase pushed

[tekton-robot]

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/interceptors/cel/triggers.go	88.3%	88.6%	0.3
pkg/interceptors/interceptors.go	80.0%	82.4%	2.4

[dibyom]

/approve
/lgtm

[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