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 ?
| 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?
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 |
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.