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.

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

[ports-plugin] don't prompt user to redirect endpoints with exposure: none #301

Merged
merged 3 commits into from
Nov 20, 2023

Conversation

amisevsk
Copy link
Contributor

What does this PR do?

Updates how endpoints are handled in the ports plugin:

  • If a process starts listening internally (i.e. on localhost) to a port that matches an endpoint with exposure: none, do not prompt the user to redirect this port
  • If a process starts listening internally (i.e. on localhost) to a port that matches an endpoint with exposure: public, change the redirect prompt to mention this specifically, as it may indicate unexpected behavior in the editor (redirecting a process rather than serving it over the defined endpoint in the devfile)

This PR also removes some old JWT handling code, since we don't use JWT anymore.

What issues does this PR fix?

Closes eclipse-che/che#22655

How to test this PR?

The easiest way to test this PR is via this devfile https://gist.github.com/amisevsk/2f11f0e282b93dc6413b0dd360437a7b and the following simple script (test.js):

var http = require('http');
http.createServer(function (req, res) {}).listen(1110); // public endpoint, listening on all addresses
http.createServer(function (req, res) {}).listen(1111); // internal endpoint, listening on all addresses
http.createServer(function (req, res) {}).listen(1112); // none endpoint, listening on all addresses

http.createServer(function (req, res) {}).listen(2220, "localhost"); // public endpoint, listening on localhost
http.createServer(function (req, res) {}).listen(2221, "localhost"); // internal endpoint, listening on localhost
http.createServer(function (req, res) {}).listen(2222, "localhost"); // none endpoint, listening on localhost

The devfile has six endpoints:

  • 1110, 1111, and 1112 are public, internal, and none endpoints (in that order) for testing "listen on 0.0.0.0"
  • 2220, 2221, and 2222 are public, internal, and none endpoints (in that order) for testing "listen on localhost"

If you run the test.js script, you should see:

  • Two prompts -- one for the public endpoint and one warning/prompt for the public-internal endpoint
    2023-11-14-13:34:11

  • Four logs in the ports plugins output pane:

    Endpoint test-none on port 1112 is defined as exposure: none. Do not prompt to open it.
    Endpoint test-internal on port 1111 is defined as exposure: internal. Do not prompt to open it.
    Endpoint test-local-internal on port 2221 is defined as exposure: internal. Do not prompt to open it.
    Endpoint test-local-none on port 2222 is defined as exposure: none. Do not prompt to open it.
    

The JWT proxy was removed as of Che 7.42 and is no longer used.

See Che issue eclipse-che/che#21275 for details.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
…n ports-plugin

If an endpoint exists in the devfile and has exposure: none, we
shouldn't prompt the user to expose that endpoint if a process starts
listening on localhost.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
If a process starts that is listening to localhost (and is thus
inaccessible from outside the pod), we show a prompt to the user to add
it to Code's redirect routes. However, if the process is listening on a
port that matches a public endpoint in the devfile, we should prompt
differently, noting the potential error, since the user intends to use
the endpoint route instead of a redirect route.

Signed-off-by: Angel Misevski <amisevsk@redhat.com>
Copy link

github-actions bot commented Nov 14, 2023

Click here to review and test in web IDE: Contribute

@amisevsk
Copy link
Contributor Author

The smoke test failure is due to a current quay.io outage

Copy link
Member

@azatsarynnyy azatsarynnyy left a comment

Choose a reason for hiding this comment

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

I tested the patch with my test project and I can confirm that it works as described above.
I just had to modify the last line in the test.js script to listen on 2223 instead of 2222 according to the edpoints declared in the provided test Devfile.
Thanks @amisevsk for your PR and for removing the obsolete code.

@azatsarynnyy
Copy link
Member

I rerun the smoke tests.

@amisevsk
Copy link
Contributor Author

test.js script to listen on 2223 instead of 2222 according to the edpoints declared in the provided test Devfile.

Ah yeah, I fixed that in my tests but I fixed it in the DevWorkspace and forgot to update the devfile. I will update the sample devfile 👍

@vitaliy-guliy
Copy link
Contributor

vitaliy-guliy commented Nov 15, 2023

I confused a bit. Why do we have the first prompt if we have the same definition for 2220 and 1110?

        - exposure: public
          name: test-public
          secure: true
          protocol: http
          targetPort: 1110
          
        - exposure: public
          name: test-local-public
          secure: true
          protocol: http
          targetPort: 2220

Anyway, I haven't played yet with http server bind to a specific ip/host.
But at the first glance it's strange.

Copy link
Contributor

@vitaliy-guliy vitaliy-guliy left a comment

Choose a reason for hiding this comment

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

Tested using @azatsarynnyy sample, works as described.

@amisevsk
Copy link
Contributor Author

Why do we have the first prompt if we have the same definition for 2220 and 1110?

The first prompt is because the server that's listening on port 2220 is only listening on localhost (instead of 0.0.0.0), and so would be inaccessible from outside the container. To connect to this server, we would need an in-container proxy (which is what Code will do if you accept the prompt) -- the route/ingress created for port 2220 will not be able to connect.

Previously, the user would see the same prompt as for other non-exposed endpoints (e.g. if you listen on port 9999), where Code is asking if you want it to proxy connections to 9999 within the container. I changed the prompt to point out the potential issue, since the public endpoint suggests the user wants to use that endpoint's URL to connect instead of a redirect route/ingress.

code-port-plugin-flow

@amisevsk
Copy link
Contributor Author

For reference, this is what the prompts look like in the current main branch:

2023-11-15-14:57:24

The first one in this screenshot is due to eclipse-che/che#22655. The second one is the generic "your process is listening on localhost instead of 0.0.0.0", which is changed in this PR to reflect that an endpoint exists and isn't being used.

@amisevsk amisevsk merged commit 44799de into che-incubator:main Nov 20, 2023
3 checks passed
@amisevsk amisevsk deleted the internal-endpoint-exposure branch November 20, 2023 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't ask to add a redirect for endpoints with non public exposure
3 participants