feature: add service port spec for eventlistener #1272

waveywaves · 2021-11-25T07:45:42Z

Changes

add the ability to specify the service port for a eventlistener
service. the TLS port will default to 443 instead of 8443 in
the case where the service port is set.
this has been done to circumvent the logic of 8443 only being set
when the default port is set with TLS enabled.

fixes #1270

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

- API changes
  Added ServicePort specification to kubernetesResource to allow users to modify on which port their eventListener service is exposed on (defaults to 8080).

tekton-robot · 2021-11-25T07:48: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/reconciler/eventlistener/resources/service.go	100.0%	93.3%	-6.7

waveywaves · 2021-11-25T08:46:33Z

#1270

tekton-robot · 2021-11-25T08:52: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/eventlistener/resources/service.go	100.0%	93.3%	-6.7

tekton-robot · 2021-11-26T04:50:13Z

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/eventlistener/resources/service.go	100.0%	93.3%	-6.7

waveywaves · 2021-11-26T14:12:36Z

/test pull-tekton-triggers-go-coverage

savitaashture · 2021-11-29T08:55:17Z

pkg/reconciler/eventlistener/resources/service.go

 			// We return port 8443 if TLS is enabled and the default HTTP port is set.
 			// This effectively makes 8443 the default HTTPS port unless a user explicitly sets a different port.
 			servicePortPort = 8443
+		} else {
+			// Return port 443 if TLS is enabled and the HTTP port is not the default one.
+			// This will allow traffic to be directed to the default HTTPS port.
+			servicePortPort = 443
 		}


@waveywaves How about to remove

if *c.Port == DefaultPort {

and just override servicePortPort to 8443 instead of confusing with 8443 and 443

WDYT?

Could you explain why we need 443 as the default TLS port?

After giving it some thought I realized that this may not be a great solution to the TLS port issue. Allowing port override should allow the user to configure the port as 443 if they want to. Having it as a default port is not a great idea fosho.

docs/eventlisteners.md

go.sum

pkg/apis/triggers/v1beta1/event_listener_types.go

dibyom · 2021-11-30T16:51:57Z

pkg/reconciler/eventlistener/resources/service.go

 			// We return port 8443 if TLS is enabled and the default HTTP port is set.
 			// This effectively makes 8443 the default HTTPS port unless a user explicitly sets a different port.
 			servicePortPort = 8443
+		} else {
+			// Return port 443 if TLS is enabled and the HTTP port is not the default one.
+			// This will allow traffic to be directed to the default HTTPS port.
+			servicePortPort = 443
 		}


Could you explain why we need 443 as the default TLS port?

waveywaves · 2021-12-10T08:11:42Z

Could you explain why we need 443 as the default TLS port?

After giving it some thought I realized that this may not be a great solution to the TLS port issue. Allowing port override should allow the user to configure the port as 443 if they want to. Having it as a default port is not a great idea fosho.

dibyom · 2022-01-12T16:45:30Z

@waveywaves as I mentioned in the WG today, we might want to look into passing the servicePort into the EventListenerStatus.SetAddress func so that the status.address.URL that is set on the EL contains the right port.

UPDATE: It looks like it might already do the right thing - calls to SetAddress uses resources.ListenerHostname which relies on the ServicePort function that contains the changes.

waveywaves · 2022-01-14T15:13:03Z

@waveywaves as I mentioned in the WG today, we might want to look into passing the servicePort into the EventListenerStatus.SetAddress func so that the status.address.URL that is set on the EL contains the right port.

UPDATE: It looks like it might already do the right thing - calls to SetAddress uses resources.ListenerHostname which relies on the ServicePort function that contains the changes.

@dibyom In that case, are there any other changes you think I should make ?

tekton-robot · 2022-01-19T15:34:12Z

[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 🎉

dibyom · 2022-02-04T23:57:46Z

/lgtm

dibyom · 2022-02-07T15:14:27Z

@waveywaves Please run hack/update-codegen.sh to generate updated api docs

add the ability to specify the service port for a eventlistener service. Default TLS Port 8443 is not set when service port is specified by the user. Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>

waveywaves · 2022-02-08T12:16:32Z

@dibyom I just updated this. Do take a look. Thanks for the ping.

dibyom · 2022-02-08T16:09:40Z

/lgtm

tekton-robot requested review from bobcatfish and khrm November 25, 2021 07:45

tekton-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 25, 2021

waveywaves force-pushed the eventlistener/service-port branch from bbb622b to 7b9a4b9 Compare November 25, 2021 08:50

waveywaves force-pushed the eventlistener/service-port branch from 7b9a4b9 to 9a4f8ec Compare November 26, 2021 04:48

waveywaves force-pushed the eventlistener/service-port branch from 9a4f8ec to 624f331 Compare November 26, 2021 05:06

savitaashture reviewed Nov 29, 2021

View reviewed changes

dibyom reviewed Nov 30, 2021

View reviewed changes

waveywaves force-pushed the eventlistener/service-port branch 10 times, most recently from 054abf5 to cbd349f Compare December 10, 2021 10:34

khrm added this to the Triggers v0.19 milestone Jan 12, 2022

dibyom approved these changes Jan 19, 2022

View reviewed changes

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

savitaashture reviewed Feb 4, 2022

View reviewed changes

tekton-robot assigned dibyom Feb 4, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2022

waveywaves force-pushed the eventlistener/service-port branch from cbd349f to f498414 Compare February 8, 2022 11:28

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 8, 2022

feature: add service port spec for el

6fb5407

add the ability to specify the service port for a eventlistener service. Default TLS Port 8443 is not set when service port is specified by the user. Signed-off-by: Vibhav Bobade <vibhav.bobde@gmail.com>

waveywaves force-pushed the eventlistener/service-port branch from f498414 to 6fb5407 Compare February 8, 2022 11:40

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

tekton-robot merged commit a55e2f3 into tektoncd:main Feb 8, 2022

khrm added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: add service port spec for eventlistener #1272

feature: add service port spec for eventlistener #1272

waveywaves commented Nov 25, 2021 •

edited

Loading

tekton-robot commented Nov 25, 2021

waveywaves commented Nov 25, 2021

tekton-robot commented Nov 25, 2021

tekton-robot commented Nov 26, 2021

waveywaves commented Nov 26, 2021

savitaashture Nov 29, 2021

dibyom Nov 30, 2021

waveywaves Dec 10, 2021

dibyom Nov 30, 2021

waveywaves commented Dec 10, 2021 •

edited

Loading

dibyom commented Jan 12, 2022 •

edited

Loading

waveywaves commented Jan 14, 2022

tekton-robot commented Jan 19, 2022

savitaashture left a comment

dibyom commented Feb 4, 2022

dibyom commented Feb 7, 2022

waveywaves commented Feb 8, 2022 •

edited

Loading

dibyom commented Feb 8, 2022

feature: add service port spec for eventlistener #1272

feature: add service port spec for eventlistener #1272

Conversation

waveywaves commented Nov 25, 2021 • edited Loading

Changes

Submitter Checklist

Release Notes

tekton-robot commented Nov 25, 2021

waveywaves commented Nov 25, 2021

tekton-robot commented Nov 25, 2021

tekton-robot commented Nov 26, 2021

waveywaves commented Nov 26, 2021

savitaashture Nov 29, 2021

Choose a reason for hiding this comment

dibyom Nov 30, 2021

Choose a reason for hiding this comment

waveywaves Dec 10, 2021

Choose a reason for hiding this comment

dibyom Nov 30, 2021

Choose a reason for hiding this comment

waveywaves commented Dec 10, 2021 • edited Loading

dibyom commented Jan 12, 2022 • edited Loading

waveywaves commented Jan 14, 2022

tekton-robot commented Jan 19, 2022

savitaashture left a comment

Choose a reason for hiding this comment

dibyom commented Feb 4, 2022

dibyom commented Feb 7, 2022

waveywaves commented Feb 8, 2022 • edited Loading

dibyom commented Feb 8, 2022

waveywaves commented Nov 25, 2021 •

edited

Loading

waveywaves commented Dec 10, 2021 •

edited

Loading

dibyom commented Jan 12, 2022 •

edited

Loading

waveywaves commented Feb 8, 2022 •

edited

Loading