Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sample-app: Fix port of node-server-svc (80 to 8080) #6204

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

sample-app: Fix port of node-server-svc (80 to 8080) #6204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a4062d0
sample-app: Fix port of node-server-svc (80 to 8080)
guzalv Jan 24, 2025
74539d6
sample-app: Use hardcoded URL as db-insert trigger subscriber
guzalv Jan 25, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion code-samples/eventing/bookstore-sample-app/solution/node-server/config/100-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,6 @@ spec:
app: node-server
ports:
- protocol: TCP
port: 80
port: 8080
targetPort: 8000
type: LoadBalancer
6 changes: 1 addition & 5 deletions code-samples/eventing/bookstore-sample-app/solution/node-server/config/200-broker.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,4 @@ spec:
type: moderated-comment # This is the filter that will be applied to the event, only events with the ce-type moderated-comment will be processed
badwordfilter: good # This is the filter that will be applied to the event, only events with the ce-extension badwordfilter: good will be processed
subscriber:
ref:
apiVersion: v1
kind: Service
name: node-server-svc
uri: /insert # This is the path where the event will be sent to the subscriber, see /insert in node-server code: index.js
uri: http://node-server-svc.default.svc.cluster.local:8080/insert # This is the path where the event will be sent to the subscriber, see /insert in node-server code: index.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we would still prefer using the ref instead of hardcoding the uri, as the uri might get changed based on the environment. The original yaml file without this change should work as expected.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hey @Leo6Leo does the ref work if it's switched over to port 8080?

apiVersion: v1
kind: Service
name: node-server-svc

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oppps sorry i was busy with exams in the past days... I will take a look today after school.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I tested, after switching the port to 8080, the ref doesn't work as expected. @dprotaso do you have any idea on what could go wrong? After changing the ref to uri, things work out. That's very weird...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure - this is an eventing API so I'm not an expert. cc @pierDipi @Cali0707

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know Services are a special case of 'Addressable' - I would assume it would look up a port number or allow you to specify the port in the ref

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking. I believe this is because the resolver, which converts the ref to an URI, doesn’t seem to handle the “port” part of the hostname, see here:

https://github.com/knative/pkg/blob/dcf159339de2fec9dde678a04cb7a354e912d2bf/resolver/addressable_resolver.go#L227

In a way this makes sense, because a service can listen on multiple ports, so which one would the resolver choose?

How could this kind of additional information (which port to use) be passed to the ref, to be able to use that instead of an URI directly?

2 changes: 1 addition & 1 deletion code-samples/eventing/bookstore-sample-app/start/node-server/config/100-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,6 @@ spec:
app: node-server
ports:
- protocol: TCP
port: 80
port: 8080
targetPort: 8000
type: LoadBalancer
2 changes: 1 addition & 1 deletion docs/bookstore/page-0.5/environment-setup.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,7 @@ node-server-svc LoadBalancer 10.101.90.35 <pending> 80:31792/T
If port forwarding is required, open a new terminal and run:

```shell
kubectl port-forward svc/node-server-svc 8080:80
kubectl port-forward svc/node-server-svc 8080:8080
```
You should see the following output:

Expand Down
6 changes: 1 addition & 5 deletions docs/bookstore/page-6/advanced-event-filtering.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,7 @@ Append the following Trigger configuration to the existing `node-server/config/2
type: moderated-comment # This is the filter that will be applied to the event, only events with the ce-type moderated-comment will be processed
badwordfilter: good # This is the filter that will be applied to the event, only events with the ce-extension badwordfilter: good will be processed
subscriber:
ref:
apiVersion: v1
kind: Service
name: node-server-svc
uri: /insert # This is the path where the event will be sent to the subscriber, see /insert in node-server code: index.js
uri: http://node-server-svc.default.svc.cluster.local:8080/insert # This is the path where the event will be sent to the subscriber, see /insert in node-server code: index.js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we would still prefer using the ref instead of hardcoding the uri, as the uri might get changed based on the environment. The original yaml file without this change should work as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree the ref looks much nicer. However this is the only way I could make it work, see this explanation: #6204 (comment)

I just tried it out again just in case and observed the same behavior:
Screenshot 2025-02-04 at 11 43 53

I think the ref is cleaner and the proper way to do this, but it seems to me that this method lacks support to discover the service port. And since it now is 8080 rather than the default 80, the event never reaches the node server.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@guzalv Yes, I tested locally, and encountered the same issue as you do.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we make the service use port 80 and targetPort 8080 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Port 80 and targetPort 8080 is how it currently works, and this PR changes it to port 8080 and targetPort 8080.

The reason is that the frontend expects port 8080 rather than 80, see here, and currently the sample app doesn't work if using "minikube tunnel", i.e. without port-forwarding to 8080:

const ws = new WebSocket("ws://localhost:8080/comments");

That could be changed to port 80 to avoid the issue with the service ref, and then the kubectl port-forward commands would need elevated privileges.

My knative-newcomer's view is that the correct way to handle this would be by having a way to pass the port to the service ref, and I suspect that requires significant changes

So if you agree it's worth to fix this here instead (in the meanwhile?), options include to merge this PR or change the frontend app to hit port 80 on the backend.

What do you think?

```

```shell
Expand Down
Loading