sample-app: Fix port of node-server-svc (80 to 8080) #6204
Conversation
✅ Deploy Preview for knative ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Hi, When I reached step 6 of the sample app tutorial I noticed that the event never reached the subscriber (node-server service). If I understand correctly, this is because when the subscriber's destination is a reference to a service, the resolver assumes it listens on port 80, i.e. it doesn't consider the "port" part of the hostname. I lack experience to fully understand how this is supposed to work. Should I open an issue in pkg/ to handle this kind of situation? or, am I going in the wrong direction? Thanks! |
|
cc @Leo6Leo Could you recreate the problem reported here, does this fix it? |
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
@guzalv Yes, I tested locally, and encountered the same issue as you do.
There was a problem hiding this comment.
can we make the service use port 80 and targetPort 8080 ?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I agree with your view on how this should be handled:
would be by having a way to pass the port to the service ref, and I suspect that requires significant changes
Since this could involve a substantial change, how about we leave it as-is for now in this PR? Instead, let’s create a GitHub issue to propose this change and gather community feedback. We can link that issue to this PR so we won’t forget to update the sample app if the proposal moves forward.
How does that sound? Thanks again for catching this issue! @guzalv
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
hey @Leo6Leo does the ref work if it's switched over to port 8080?
apiVersion: v1
kind: Service
name: node-server-svc
There was a problem hiding this comment.
oppps sorry i was busy with exams in the past days... I will take a look today after school.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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:
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?
There was a problem hiding this comment.
From the conversation and the observation, If we don’t specify a port, the resolver assumes the default port (80 for http, 443 for https). If the Service needs another port, we must provide a full URI with the desired port now.
In that way, we might need to update the Kreference to add port field? Then users will be able to use ref with their desired ports
code-samples/eventing/bookstore-sample-app/start/node-server/config/100-deployment.yaml
Show resolved
Hide resolved
Without this, following the instructions that don't require port forwarding (e.g. using "minikube tunnel") fails: the service is exposed on port 80, which is not what the frontend expects. After this commit the setup mimics that of the frontend service, which works both with and without port forwarding. Signed-off-by: Guzman <guzman@guzman.fi>
This is needed because the node-server service listens on port 8080, and the K8s ref resolver doesn't support custom ports [1], and referring to the node-server-svc service yields: http://node-server-svc.default.svc.cluster.local/insert Instead of the correct URL: http://node-server-svc.default.svc.cluster.local:8080/insert Which results in hanging connections and eventually timeouts when adding new comments that pass the trigger's filter. [1] https://github.com/knative/pkg/blob/dcf159339de2fec9dde678a04cb7a354e912d2bf/resolver/addressable_resolver.go#L227 Signed-off-by: Guzman <guzman@guzman.fi>
bbbd4e2 to
74539d6
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: guzalv The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
bump @Leo6Leo |
1 similar comment
|
bump @Leo6Leo |
|
Thanks for your input @Leo6Leo. After checking this again, I doubt adding a Following from there, and looking at And To an outsider like me it looks like one of the use cases of Otherwise, perhaps port-related field(s) could be added to What do you think about merging this PR, so that the sample app works in the meanwhile? I just verified again that following the recommended instructions in knative.dev to setup the environment (use I'd propose to either merge this to benefit users, or to change the instructions to remove references to Minikube tunnel and advise to always use port forwarding. Finally, if havi
A668
ng an absolute URL in the |
Without this, following the instructions that don't require port forwarding (e.g. using "minikube tunnel") fails: the service is exposed on port 80, which is not what the frontend expects.
After t 8000 his commit the setup mimics that of the frontend service, which works both with and without port forwarding.