Add https-listener service port name #887

MarcelMue · 2021-01-05T18:23:22Z

Changes

This is a change following discussions in #881 - it introduces a second port name when TLS is used and changes the port behaviour to keep container ports static and only change service port when configured.

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

Add https-listener service port name if TLS is used.

tekton-robot · 2021-01-05T18:23:40Z

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

dibyom · 2021-01-05T18:37:21Z

/ok-to-test

tekton-robot · 2021-01-05T18:38:29Z

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/reconciler/v1alpha1/eventlistener/eventlistener.go	82.5%	80.3%	-2.2

savitaashture

Overall looks good to me 👍

few questions around default values

savitaashture · 2021-01-06T05:46:19Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

+	// eventListenerServiceTLSPortName defines service TLS port name for EventListener Service
+	eventListenerServiceTLSPortName = "https-listener"
+	// eventListenerContainerPort defines the port exposed by the EventListener Container
+	eventListenerContainerPort = 8000


any reason to change default port value from 8080 to 8000 ?

savitaashture · 2021-01-06T05:53:52Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

@@ -543,7 +538,7 @@ func getContainer(el *v1alpha1.EventListener, c Config) corev1.Container {
 		Args: []string{
 			"--el-name=" + el.Name,
 			"--el-namespace=" + el.Namespace,
-			"--port=" + strconv.Itoa(port),
+			"--port=" + strconv.Itoa(eventListenerContainerPort),


Is this change to make EventListener pod to run on 8000 always ?

if thats the case we need to document about this and also we need to document when this config/controller.yaml used and update the flag description here

The idea was together with @dibyom :
Changing the port for the deployment / container to be static while only making the service configurable. This causes two effects.

Less work templating because we only need to change the port in the service instead of everywhere.

Logic becomes easier to understand because configuration is only applied to the service and not to the deployment / container and so on.

I chose port 8000 because noone from outside should consume it - they should interact with the service instead (where it remains 8080 / 8443 by default). Additionally it makes clear (to me at least) that it could be either protocol (8080 implies HTTP).

savitaashture · 2021-01-06T05:57:14Z

pkg/reconciler/v1alpha1/eventlistener/eventlistener.go

 	return corev1.Container{
 		Name:  "event-listener",
 		Image: *c.Image,
 		Ports: []corev1.ContainerPort{{
-			ContainerPort: int32(port),
+			ContainerPort: int32(eventListenerContainerPort),


This changes indicates that for https connection also it uses 8000 port for container I mean the target port is always constant for (http and https) connection right?

khrm

/test pull-tekton-triggers-integration-tests

tekton-robot · 2021-01-06T10:57: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/reconciler/v1alpha1/eventlistener/eventlistener.go	82.5%	80.3%	-2.2

dibyom · 2021-01-06T16:01:42Z

Ok so I think I found one place we were using the deployment's port -- our E2E test 😬

I think the value is being set here: https://github.com/tektoncd/triggers/blob/master/test/eventlistener_test.go#L381

(For this PR I think its fine to just switch that port to the new one, I'm working on updating some of the E2E tests -- I can take a stab at changing it to portforward to the service instead of the pod then)

tekton-robot · 2021-01-07T13:51:29Z

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/reconciler/v1alpha1/eventlistener/eventlistener.go	82.5%	80.3%	-2.2

tekton-robot · 2021-01-07T14:17: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/reconciler/v1alpha1/eventlistener/eventlistener.go	82.5%	80.3%	-2.2

tekton-robot · 2021-01-07T20:04:43Z

[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

savitaashture

/lgtm

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 5, 2021

tekton-robot requested review from bobcatfish and khrm January 5, 2021 18:23

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 5, 2021

tekton-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 5, 2021

tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 5, 2021

savitaashture reviewed Jan 6, 2021

View reviewed changes

khrm reviewed Jan 6, 2021

View reviewed changes

MarcelMue force-pushed the introduce-port-names branch from 4e9cc04 to 947b7c2 Compare January 7, 2021 14:09

tekton-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2021

Add https-listener service port name

441f651

MarcelMue force-pushed the introduce-port-names branch from 947b7c2 to 441f651 Compare January 7, 2021 14:15

tekton-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 7, 2021

dibyom approved these changes Jan 7, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 7, 2021

savitaashture reviewed Jan 8, 2021

View reviewed changes

tekton-robot assigned savitaashture Jan 8, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2021

tekton-robot merged commit d74327a into tektoncd:master Jan 8, 2021

MarcelMue deleted the introduce-port-names branch January 8, 2021 10:52

jmcshane mentioned this pull request May 6, 2021

Update examples readme to use valid port 8000 #1071

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add https-listener service port name #887

Add https-listener service port name #887

MarcelMue commented Jan 5, 2021 •

edited

Loading

tekton-robot commented Jan 5, 2021

dibyom commented Jan 5, 2021

tekton-robot commented Jan 5, 2021

savitaashture left a comment

savitaashture Jan 6, 2021

savitaashture Jan 6, 2021

MarcelMue Jan 6, 2021

savitaashture Jan 6, 2021

khrm left a comment

tekton-robot commented Jan 6, 2021

dibyom commented Jan 6, 2021 •

edited

Loading

tekton-robot commented Jan 7, 2021

tekton-robot commented Jan 7, 2021

tekton-robot commented Jan 7, 2021

savitaashture left a comment

Add https-listener service port name #887

Add https-listener service port name #887

Conversation

MarcelMue commented Jan 5, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Jan 5, 2021

dibyom commented Jan 5, 2021

tekton-robot commented Jan 5, 2021

savitaashture left a comment

Choose a reason for hiding this comment

savitaashture Jan 6, 2021

Choose a reason for hiding this comment

savitaashture Jan 6, 2021

Choose a reason for hiding this comment

MarcelMue Jan 6, 2021

Choose a reason for hiding this comment

savitaashture Jan 6, 2021

Choose a reason for hiding this comment

khrm left a comment

Choose a reason for hiding this comment

tekton-robot commented Jan 6, 2021

dibyom commented Jan 6, 2021 • edited Loading

tekton-robot commented Jan 7, 2021

tekton-robot commented Jan 7, 2021

tekton-robot commented Jan 7, 2021

savitaashture left a comment

Choose a reason for hiding this comment

MarcelMue commented Jan 5, 2021 •

edited

Loading

dibyom commented Jan 6, 2021 •

edited

Loading