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

Fix #622: Make noVNC can be accessed via URLs like https://example.com/novnc #623

Merged
merged 6 commits into from
Nov 15, 2024

Conversation

hzxie
Copy link
Contributor

@hzxie hzxie commented Nov 14, 2024

Please refer to #622 for details.

After merging this PR, then the embedded noVNC can be accessed via URLs like https://example.com/novnc with the following NGINX config.

        location /novnc/ {
                proxy_pass                      http://localhost:8006/;
                proxy_http_version              1.1;
                proxy_set_header                Upgrade         $http_upgrade;
                proxy_set_header                Connection      "upgrade";
        }

@hzxie
Copy link
Contributor Author

hzxie commented Nov 14, 2024

It is verified in HTTPS connections with subpath.

image
image

@kroese
Copy link
Collaborator

kroese commented Nov 14, 2024

Thanks!! Is this ready to merge or are you still working on something?

@hzxie
Copy link
Contributor Author

hzxie commented Nov 15, 2024

I think it's ready to merge.

@kroese
Copy link
Collaborator

kroese commented Nov 15, 2024

Everything looks good to me, except the patching of the NoVNC source code in the Dockerfile. Those kinds of patches are very likely to break in the future. And more important: this whole issue should be solved by them, not just by us.

So I was investigating why they never fixed this issue, and I came across: novnc/noVNC#1058 So it seems like they are aware of the problem, but they dont want to accept your patch because it introduces other problems.

For example when using /vnc_lite.html, /index.html or alternative page files it generates the wrong websocket url.

If you are 100% sure your method does not break anything, why not submit that patch to NoVNC instead? Then much more people will profit from it then only the users of this container.

@hzxie
Copy link
Contributor Author

hzxie commented Nov 15, 2024

It’s clear there are multiple issues related to this specific problem, as highlighted in discussions like issue #1339 and pull request #1058 on the NoVNC GitHub repository. While it’s uncertain why NoVNC developers haven’t merged this PR, I’ve decided to create a patch in this repository to address the issue independently.

If future updates to NoVNC break this feature, we can update the patch as needed. After all, no API remains static indefinitely. It’s similar to adding parameters like ?resize=scale&reconnect=true&autoconnect=true; in an Nginx configuration—if NoVNC changes those parameters, we’d have to adjust accordingly.

@kroese
Copy link
Collaborator

kroese commented Nov 15, 2024

It seems this issue is already fixed since August: novnc/noVNC@96c76f7

But we are using v1.5.0 and the above commit is newer, so it will be included in 1.6.0

I will merge this now and we will remove that patch when 1.6.0 is released. Thanks for your work!

@kroese kroese merged commit a37db1b into qemus:master Nov 15, 2024
1 check passed
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.

2 participants