Allow secure connection to eventlistener pod #819

savitaashture · 2020-10-31T15:12:13Z

Changes

Issue: #650

As part of this PR Triggers eventlistener now support end to end secure connection to eventlistener pod.
For more info about the feature refer TEP-0027 .

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

HTTPS connection to eventlistener can be configured by tweaking eventlistener configuration

   apiVersion: triggers.tekton.dev/v1alpha1
   kind: EventListener
   metadata:
     name: github-listener-interceptor
   spec:
     ...
     resources:
       kubernetesResource:
         spec:
           template:
             spec:
               serviceAccountName: tekton-triggers-github-sa
               containers:
               - env:
                 - name: TLS_CERT
                   valueFrom:
                     secretKeyRef:
                       name: tls-key-secret
                       key: tls.crt
                 - name: TLS_KEY
                   valueFrom:
                     secretKeyRef:
                       name: tls-key-secret
                       key: tls.key

Note:
TLS_CERT, TLS_KEY are RESERVED env

dibyom

Overall looks good. Just some minor comments. Also, how feasible would it be to add an e2e test for this?

(Also noticed some questions on tektoncd/community#244 that we should resolve before merging this)

cmd/eventlistenersink/main.go

docs/eventlisteners.md

pkg/apis/triggers/v1alpha1/event_listener_validation_test.go

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

pkg/reconciler/v1alpha1/eventlistener/eventlistener_test.go

pkg/sink/initialization.go

dibyom · 2020-11-17T17:15:34Z

/lgtm

Code looks fine. Pending design discussion on using valueFrom/secretRef instead of value: in tektoncd/community#244

tekton-robot · 2020-11-18T15:33:57Z

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/apis/triggers/v1alpha1/event_listener_validation.go	100.0%	98.2%	-1.8
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	78.2%	79.1%	0.9

savitaashture · 2020-11-18T15:39:10Z

pkg/apis/triggers/v1alpha1/event_listener_validation.go

+func envVarSourceMask(in *corev1.EnvVarSource) *corev1.EnvVarSource {
+	if in == nil {
+		return nil
+	}
+	out := new(corev1.EnvVarSource)
+	// Allowed fields
+	out.SecretKeyRef = in.SecretKeyRef
+
+	// Disallowed fields
+	out.ConfigMapKeyRef = nil
+	out.FieldRef = nil
+	out.ResourceFieldRef = nil
+
+	return out
+}


I own to refactor changes related to masking fields in a follow up PR by creating separate file so that _validation.go will have changes related to validation

tekton-robot · 2020-11-18T16:09: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/apis/triggers/v1alpha1/event_listener_validation.go	100.0%	98.2%	-1.8
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	78.2%	79.1%	0.9

dibyom

One small nit about docs otherwise LGTM!

/approve

dibyom · 2020-11-19T21:38:56Z

docs/eventlisteners.md

+
+## EventListener Secure Connection
+
+Triggers now support both `HTTP` and `HTTPS` connection by adding few configurations to eventlistener.


Could we add a little bit more detail here? e.g. :

even a like like -- To setup TLS, add set two environment variables TLS_CERT, and TLS_KEY with values set to a Secret. See the full example(LINK) for details`

tekton-robot · 2020-11-20T05:19:30Z

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/apis/triggers/v1alpha1/event_listener_validation.go	100.0%	98.2%	-1.8
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	78.2%	79.1%	0.9

khrm · 2020-11-20T10:56:28Z

examples/eventlistener-tls-connection/README.md

+   ```
+
+#### Configuring EventListener for TLS connection
+```yaml


Nit: Why not put this in yaml file like other examples? It make it faster to test out examples. Otherwise, lgtm.

The dependency is on secret which is given by user and which contains cert and key files so just added sample example in README so that user can refer

We can probably assume tls-key-secret as secretKeyRef.

if we assume then we should create that before hand and for that we also need to create cert n keys

It's OK to not create key beforehand because those command will do it anyway.

tekton-robot · 2020-11-20T13:11: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/apis/triggers/v1alpha1/event_listener_validation.go	98.8%	97.3%	-1.5
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	78.5%	79.4%	0.9

tekton-robot · 2020-11-20T13:33: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/apis/triggers/v1alpha1/event_listener_validation.go	98.8%	97.3%	-1.5
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	78.5%	79.4%	0.9

tekton-robot · 2020-11-20T13:36: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/apis/triggers/v1alpha1/event_listener_validation.go	98.8%	97.3%	-1.5
pkg/reconciler/v1alpha1/eventlistener/eventlistener.go	78.5%	79.4%	0.9

khrm

/lgtm

tekton-robot · 2020-11-20T13:40:42Z

[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

…ented TEP-0027 was implemented in tektoncd/triggers#819. This change updates the TEP state from `implementable` to `implemented`.

tekton-robot requested review from dibyom and wlynch October 31, 2020 15:12

tekton-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 31, 2020

savitaashture force-pushed the secure-connection-el branch 3 times, most recently from c81be10 to 6264fb2 Compare October 31, 2020 15:34

savitaashture mentioned this pull request Nov 1, 2020

Change status of TEP-0027 to Implementable tektoncd/community#244

Merged

savitaashture changed the title ~~Allow secure connection to eventlistener pod~~ [WIP]Allow secure connection to eventlistener pod Nov 2, 2020

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 2, 2020

savitaashture force-pushed the secure-connection-el branch 2 times, most recently from 3d73905 to 5668819 Compare November 2, 2020 18:56

savitaashture changed the title ~~[WIP]Allow secure connection to eventlistener pod~~ Allow secure connection to eventlistener pod Nov 3, 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 Nov 3, 2020

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 10, 2020

dibyom reviewed Nov 10, 2020

View reviewed changes

dibyom mentioned this pull request Nov 11, 2020

EventListeners stops receiving events randomly #824

Closed

savitaashture force-pushed the secure-connection-el branch from 5668819 to 604c8d1 Compare November 13, 2020 13:15

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2020

savitaashture force-pushed the secure-connection-el branch from 604c8d1 to 309185c Compare November 15, 2020 08:39

tekton-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 15, 2020

savitaashture force-pushed the secure-connection-el branch 2 times, most recently from 9af1b9a to 94c22a9 Compare November 15, 2020 08:47

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Nov 15, 2020

savitaashture force-pushed the secure-connection-el branch 2 times, most recently from 0740e24 to bbd8248 Compare November 15, 2020 09:32

tekton-robot assigned dibyom Nov 17, 2020

tekton-robot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 17, 2020

savitaashture force-pushed the secure-connection-el branch from bbd8248 to f057378 Compare November 18, 2020 15:32

tekton-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 18, 2020

savitaashture commented Nov 18, 2020

View reviewed changes

savitaashture force-pushed the secure-connection-el branch from f057378 to 7e97b68 Compare November 18, 2020 16:08

dibyom reviewed Nov 19, 2020

View reviewed changes

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

savitaashture force-pushed the secure-connection-el branch from 7e97b68 to 978df6f Compare November 20, 2020 05:18

khrm reviewed Nov 20, 2020

View reviewed changes

savitaashture force-pushed the secure-connection-el branch from 978df6f to 7ae961f Compare November 20, 2020 13:10

savitaashture force-pushed the secure-connection-el branch from 7ae961f to 6633356 Compare November 20, 2020 13:32

Allow secure connection to eventlistener pod

dfdd879

savitaashture force-pushed the secure-connection-el branch from 6633356 to dfdd879 Compare November 20, 2020 13:35

khrm approved these changes Nov 20, 2020

View reviewed changes

tekton-robot assigned khrm Nov 20, 2020

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

khrm approved these changes Nov 20, 2020

View reviewed changes

tekton-robot merged commit e1e7d18 into tektoncd:master Nov 20, 2020

jerop mentioned this pull request Mar 21, 2023

TEP-0027: HTTPS Connection To Triggers EventListener - Mark as Implemented tektoncd/community#986

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow secure connection to eventlistener pod #819

Allow secure connection to eventlistener pod #819

savitaashture commented Oct 31, 2020 •

edited by khrm

Loading

dibyom left a comment

dibyom commented Nov 17, 2020

tekton-robot commented Nov 18, 2020

savitaashture Nov 18, 2020

tekton-robot commented Nov 18, 2020

dibyom left a comment

dibyom Nov 19, 2020

savitaashture Nov 20, 2020

tekton-robot commented Nov 20, 2020

khrm Nov 20, 2020

savitaashture Nov 20, 2020 •

edited

Loading

khrm Nov 20, 2020

savitaashture Nov 20, 2020

khrm Nov 20, 2020

tekton-robot commented Nov 20, 2020

tekton-robot commented Nov 20, 2020

tekton-robot commented Nov 20, 2020

khrm left a comment

tekton-robot commented Nov 20, 2020


		## EventListener Secure Connection

		Triggers now support both `HTTP` and `HTTPS` connection by adding few configurations to eventlistener.

Allow secure connection to eventlistener pod #819

Allow secure connection to eventlistener pod #819

Conversation

savitaashture commented Oct 31, 2020 • edited by khrm Loading

Changes

Submitter Checklist

Release Notes

dibyom left a comment

Choose a reason for hiding this comment

dibyom commented Nov 17, 2020

tekton-robot commented Nov 18, 2020

savitaashture Nov 18, 2020

Choose a reason for hiding this comment

tekton-robot commented Nov 18, 2020

dibyom left a comment

Choose a reason for hiding this comment

dibyom Nov 19, 2020

Choose a reason for hiding this comment

savitaashture Nov 20, 2020

Choose a reason for hiding this comment

tekton-robot commented Nov 20, 2020

khrm Nov 20, 2020

Choose a reason for hiding this comment

savitaashture Nov 20, 2020 • edited Loading

Choose a reason for hiding this comment

khrm Nov 20, 2020

Choose a reason for hiding this comment

savitaashture Nov 20, 2020

Choose a reason for hiding this comment

khrm Nov 20, 2020

Choose a reason for hiding this comment

tekton-robot commented Nov 20, 2020

tekton-robot commented Nov 20, 2020

tekton-robot commented Nov 20, 2020

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Nov 20, 2020

savitaashture commented Oct 31, 2020 •

edited by khrm

Loading

savitaashture Nov 20, 2020 •

edited

Loading