feat(remote-desktop): browser-based noVNC desktops for Slurm clusters - #6141
feat(remote-desktop): browser-based noVNC desktops for Slurm clusters#6141scott-nag wants to merge 2 commits into
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces comprehensive remote desktop capabilities to the Cluster Toolkit and OFE. By providing a modular architecture for VNC-based desktop sessions, it allows users to access interactive Linux environments directly from their browsers. The implementation includes robust identity verification, secure proxying, and integration with existing Slurm cluster management workflows, while ensuring that cluster teardown processes respect active desktop sessions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
scott-nag
added
release-new-modules
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds browser-based remote desktop support for Slurm clusters using noVNC, introducing new Terraform modules, blueprints, and frontend integration (OFE) to manage per-user desktop sessions. The review feedback identifies several critical and high-severity issues, including a missing grp import in the daemon script, potential permission issues during recursive directory creation, and performance bottlenecks from synchronous network connections and sequential API calls in Django views. Additionally, the feedback recommends addressing potential runtime errors (such as AttributeError and TypeError on null or non-string values), avoiding blocking synchronous disk I/O on the main event loop, and pinning the Terraform version in versions.tf to comply with repository style guidelines.
| """Submit a long-lived desktop allocation job.""" | ||
| outfile = job_dir / "desktop.out" | ||
| errfile = job_dir / "desktop.err" | ||
| group_name = grp.getgrgid(gid).gr_name |
There was a problem hiding this comment.
The grp module is used here to look up the group name from the GID, but it is not imported in this file. This will result in a NameError at runtime when attempting to submit a desktop job. Please import grp at the top of the file or inline.
| group_name = grp.getgrgid(gid).gr_name | |
| import grp | |
| group_name = grp.getgrgid(gid).gr_name |
There was a problem hiding this comment.
This is a false positive - grp is imported on line 19 already.
| with socket.create_connection( | ||
| (service["host"], service["port"]), | ||
| timeout=2, | ||
| ): | ||
| return None | ||
| except OSError: |
There was a problem hiding this comment.
Performing a synchronous socket.create_connection check inside the Django view block is a major performance bottleneck. Because this is called during the rendering of the Cluster Detail page, any network latency, firewall block, or offline node will block the entire page load for up to 2 seconds per desktop target (potentially 4+ seconds if both login and viz desktops are enabled). Consider moving this connectivity check to an asynchronous AJAX endpoint called by the frontend, or caching the status with a short TTL.
There was a problem hiding this comment.
Fair assessment but the premise is wrong as this wouldn't slow down/block the cluster detail page since these aren't used on the cluster detail page directly.
Timeout could certainly be reduced from 2 seconds but if there's a transient networking issue or packet loss of sorts you'd get an error. Therefore 2 seconds is a sensible timeout window in my opinion.
| job_dir = Path(homedir) / "desktop" / str(config["cluster_id"]) | ||
| job_dir.mkdir(parents=True, exist_ok=True) | ||
| os.chown(job_dir, uid, gid) |
There was a problem hiding this comment.
When creating job_dir using mkdir(parents=True), if the intermediate desktop directory does not exist, it will be created by root (since the daemon runs as root) and will not be chowned to the user. Depending on the system umask, this can prevent the user from accessing their own desktop directory. It is safer to create and chown both directories sequentially.
desktop_dir = Path(homedir) / "desktop"
desktop_dir.mkdir(exist_ok=True)
os.chown(desktop_dir, uid, gid)
job_dir = desktop_dir / str(config["cluster_id"])
job_dir.mkdir(exist_ok=True)
os.chown(job_dir, uid, gid)| def _desktop_mount_options(self, mount_options, fs_type): | ||
| if fs_type != "nfs": | ||
| return mount_options | ||
| options = [option.strip() for option in mount_options.split(",") if option.strip()] | ||
| if "_netdev" not in options: | ||
| options.append("_netdev") | ||
| return ",".join(options) |
There was a problem hiding this comment.
If mount_options is None (which can happen if the field is null/empty in the database), calling mount_options.split(",") will raise an AttributeError. Adding a guard for None or empty values will make this method more robust.
| def _desktop_mount_options(self, mount_options, fs_type): | |
| if fs_type != "nfs": | |
| return mount_options | |
| options = [option.strip() for option in mount_options.split(",") if option.strip()] | |
| if "_netdev" not in options: | |
| options.append("_netdev") | |
| return ",".join(options) | |
| def _desktop_mount_options(self, mount_options, fs_type): | |
| if fs_type != "nfs": | |
| return mount_options | |
| if not mount_options: | |
| return "_netdev" | |
| options = [option.strip() for option in mount_options.split(",") if option.strip()] | |
| if "_netdev" not in options: | |
| options.append("_netdev") | |
| return ",".join(options) |
|
|
||
| zones = get_region_zone_info(cloud_provider, credentials).get(region, []) | ||
| supported = [] | ||
| for zone in zones: |
There was a problem hiding this comment.
Querying get_machine_types sequentially in a loop for each zone can introduce significant latency during web requests (e.g., us-central1 has 6 zones, leading to 6 sequential API calls). Consider caching the results of get_machine_types or utilizing GCP's aggregated list API to retrieve machine types across all zones in a single call.
There was a problem hiding this comment.
Runs at blueprint generation time. get_machine_types already has LRU caching too enabled. False positive.
| elif not machine_type: | ||
| pass | ||
| else: |
There was a problem hiding this comment.
If zone is None (e.g., if the user has not selected a zone yet on a new cluster form), calling validate_gcp_reservation will trigger an API call with a null zone, which will fail. It is safer to skip validation if zone is not yet available.
| elif not machine_type: | |
| pass | |
| else: | |
| elif not machine_type or not zone: | |
| pass | |
| else: |
| # limitations under the License. | ||
|
|
||
| terraform { | ||
| required_version = ">= 1.12.2" |
There was a problem hiding this comment.
Pin the Terraform version to a specific value (using '=') in modules to ensure compatibility with the version specifically supported by the repository or toolkit, rather than using a minimum version constraint.
required_version = "= 1.5.0"
References
- Pin the Terraform version to a specific value (using '=') in modules to ensure compatibility with the version specifically supported by the repository or toolkit.
There was a problem hiding this comment.
Repo standard is >= 1.12.2 (80 modules using it).
This suggestion goes against convention and is a lower version than the toolkit itself requires...
That being said the modules are inconsistent in that I've got vnc-runtime set to >= 1.0 so I will bring this in-line to match the rest of the repo.
| while True: | ||
| await asyncio.sleep(300) | ||
| now = int(time.time()) | ||
| for session in self._list_sessions(): |
There was a problem hiding this comment.
Calling _list_sessions() directly on the main event loop blocks the loop with synchronous disk I/O (glob and read_text operations). This can degrade the performance of the broker under load or on slow filesystems. It should be run in the executor using _run_blocking.
| for session in self._list_sessions(): | |
| sessions = await self._run_blocking(self._list_sessions) | |
| for session in sessions: |
There was a problem hiding this comment.
| if "SSH_AUTH_SOCK" in new_env: | ||
| del new_env["SSH_AUTH_SOCK"] | ||
| new_env.update(extra_env) | ||
| terraform_env = {key: os.fspath(value) for key, value in extra_env.items()} |
There was a problem hiding this comment.
Using os.fspath(value) will raise a TypeError if any value in extra_env is not a string, bytes, or path-like object (for example, if an integer port or boolean flag is passed). Since environment variables must ultimately be strings, using str(value) is safer and more robust.
| terraform_env = {key: os.fspath(value) for key, value in extra_env.items()} | |
| terraform_env = {key: str(value) for key, value in extra_env.items()} |
There was a problem hiding this comment.
|
Addressed the issues raised by Gemini now. Note: see my comment replies to Gemini for explanation as to why certain parts weren't changed. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces browser-based Linux remote desktop blueprints and modules (novnc-desktop, novnc-runtime, and vnc-runtime) integrated with the On-Demand Frontend (OFE) to support interactive access. The code review identified critical security vulnerabilities regarding symlink attacks when executing os.chown as root in ghpcfe_c2daemon.py and desktop-broker.py. Additionally, feedback was provided to resolve a double-expansion bug in bootstrap-remote-desktop-gateway.sh, prevent thread-blocking synchronous socket connections in views/desktop.py, and handle potential MultipleObjectsReturned exceptions in adapters.py.
| # Create and chown each level rather than using parents=True: the daemon runs as root | ||
| # so an intermediate directory created that way stays owned by root inside users home | ||
| desktop_dir = Path(homedir) / "desktop" | ||
| desktop_dir.mkdir(exist_ok=True) | ||
| os.chown(desktop_dir, uid, gid) | ||
| job_dir = desktop_dir / str(config["cluster_id"]) | ||
| job_dir.mkdir(exist_ok=True) | ||
| os.chown(job_dir, uid, gid) |
There was a problem hiding this comment.
Operating on user-controlled directories as root is vulnerable to symlink attacks. If a malicious user pre-creates desktop or the cluster directory as a symlink pointing to a system directory (e.g., /etc), os.chown will follow the symlink and grant the user ownership of that system directory, leading to privilege escalation. Pass follow_symlinks=False to os.chown to prevent following symlinks.
| # Create and chown each level rather than using parents=True: the daemon runs as root | |
| # so an intermediate directory created that way stays owned by root inside users home | |
| desktop_dir = Path(homedir) / "desktop" | |
| desktop_dir.mkdir(exist_ok=True) | |
| os.chown(desktop_dir, uid, gid) | |
| job_dir = desktop_dir / str(config["cluster_id"]) | |
| job_dir.mkdir(exist_ok=True) | |
| os.chown(job_dir, uid, gid) | |
| # Create and chown each level rather than using parents=True: the daemon runs as root | |
| # so an intermediate directory created that way stays owned by root inside users home | |
| desktop_dir = Path(homedir) / "desktop" | |
| desktop_dir.mkdir(exist_ok=True) | |
| os.chown(desktop_dir, uid, gid, follow_symlinks=False) | |
| job_dir = desktop_dir / str(config["cluster_id"]) | |
| job_dir.mkdir(exist_ok=True) | |
| os.chown(job_dir, uid, gid, follow_symlinks=False) |
There was a problem hiding this comment.
This is the established pattern used by c2daemon which has been part of OFE for many years.
Nevertheless it is worth updating and has now been updated per the suggested fix.
| def _ensure_home_directory(self, home_dir, uid, gid): | ||
| home_path = Path(home_dir) | ||
| try: | ||
| home_created = not home_path.exists() | ||
| home_path.mkdir(parents=True, exist_ok=True) | ||
| if home_created: | ||
| os.chown(home_path, uid, gid) | ||
| os.chmod(home_path, 0o700) | ||
| vnc_dir = home_path / ".vnc" | ||
| vnc_created = not vnc_dir.exists() | ||
| vnc_dir.mkdir(parents=True, exist_ok=True) | ||
| if vnc_created: | ||
| os.chown(vnc_dir, uid, gid) | ||
| os.chmod(vnc_dir, 0o700) |
There was a problem hiding this comment.
Creating directories and files inside a user-controlled home directory as root is highly vulnerable to symlink attacks. If a user pre-creates ~/.vnc or ~/.vnc/xstartup as symlinks to critical system files, the broker will overwrite them or change their ownership, leading to privilege escalation. Ensure we check if the paths are symlinks before operating on them, and pass follow_symlinks=False to os.chown.
| def _ensure_home_directory(self, home_dir, uid, gid): | |
| home_path = Path(home_dir) | |
| try: | |
| home_created = not home_path.exists() | |
| home_path.mkdir(parents=True, exist_ok=True) | |
| if home_created: | |
| os.chown(home_path, uid, gid) | |
| os.chmod(home_path, 0o700) | |
| vnc_dir = home_path / ".vnc" | |
| vnc_created = not vnc_dir.exists() | |
| vnc_dir.mkdir(parents=True, exist_ok=True) | |
| if vnc_created: | |
| os.chown(vnc_dir, uid, gid) | |
| os.chmod(vnc_dir, 0o700) | |
| def _ensure_home_directory(self, home_dir, uid, gid): | |
| home_path = Path(home_dir) | |
| try: | |
| if home_path.is_symlink(): | |
| raise BrokerError(403, "Home directory cannot be a symlink.") | |
| home_created = not home_path.exists() | |
| home_path.mkdir(parents=True, exist_ok=True) | |
| if home_created: | |
| os.chown(home_path, uid, gid, follow_symlinks=False) | |
| os.chmod(home_path, 0o700) | |
| vnc_dir = home_path / ".vnc" | |
| if vnc_dir.is_symlink(): | |
| raise BrokerError(403, ".vnc directory cannot be a symlink.") | |
| vnc_created = not vnc_dir.exists() | |
| vnc_dir.mkdir(parents=True, exist_ok=True) | |
| if vnc_created: | |
| os.chown(vnc_dir, uid, gid, follow_symlinks=False) | |
| os.chmod(vnc_dir, 0o700) |
There was a problem hiding this comment.
This is the established pattern used by c2daemon which has been part of OFE for many years.
Nevertheless it is worth updating and has now been updated per the suggested fix.
| cat > "${nginx_conf_path}" <<EOF_NGINX | ||
| map \$http_upgrade \$connection_upgrade { | ||
| default upgrade; | ||
| '' close; | ||
| } | ||
|
|
||
| ${redirect_server_block} | ||
|
|
||
| ${ssl_server_block} | ||
| EOF_NGINX |
There was a problem hiding this comment.
Using an unquoted heredoc (<<EOF_NGINX) to write the final configuration file causes the shell to perform a second pass of parameter expansion on the contents of ${redirect_server_block} and ${ssl_server_block}. This prematurely evaluates Nginx variables (like $host and $request_uri) to empty strings, breaking the configuration. Writing the static map block with a quoted heredoc and appending the pre-expanded blocks using printf avoids this double-expansion issue.
| cat > "${nginx_conf_path}" <<EOF_NGINX | |
| map \$http_upgrade \$connection_upgrade { | |
| default upgrade; | |
| '' close; | |
| } | |
| ${redirect_server_block} | |
| ${ssl_server_block} | |
| EOF_NGINX | |
| cat > "${nginx_conf_path}" <<'EOF_NGINX' | |
| map $http_upgrade $connection_upgrade { | |
| default upgrade; | |
| '' close; | |
| } | |
| EOF_NGINX | |
| printf "%s\n\n%s\n" "${redirect_server_block}" "${ssl_server_block}" >> "${nginx_conf_path}" |
There was a problem hiding this comment.
False positive on this one. Shell expansion is a single pass. Result of the variable substitution is never re-scanned.
| try: | ||
| with socket.create_connection( | ||
| (service["host"], service["port"]), | ||
| timeout=2, | ||
| ): | ||
| return None |
There was a problem hiding this comment.
Performing a synchronous socket.create_connection with a 2-second timeout inside the Django request-response cycle can block the request thread and exhaust the worker pool if the desktop host is unreachable. Reduce the timeout to a safer value (e.g., 0.5 seconds) since local VPC connections are extremely fast, or consider caching the status.
| try: | |
| with socket.create_connection( | |
| (service["host"], service["port"]), | |
| timeout=2, | |
| ): | |
| return None | |
| try: | |
| with socket.create_connection( | |
| (service["host"], service["port"]), | |
| timeout=0.5, | |
| ): | |
| return None |
There was a problem hiding this comment.
This is a re-framing of the comment Gemini raised previously. 2s is a safe timeout. A single dropped SYN guarantees a false "not listening" because Linux's initial TCP retransmit is ~1s. Cutting the timeout trades a real correctness bug for a marginal latency win.
| try: | ||
| existing_user = user_model.objects.get(email__iexact=email) | ||
| except user_model.DoesNotExist: | ||
| return |
There was a problem hiding this comment.
If multiple users exist with the same email address, user_model.objects.get will raise a MultipleObjectsReturned exception, causing a 500 Internal Server Error. Catch MultipleObjectsReturned along with DoesNotExist to handle this gracefully.
| try: | |
| existing_user = user_model.objects.get(email__iexact=email) | |
| except user_model.DoesNotExist: | |
| return | |
| try: | |
| existing_user = user_model.objects.get(email__iexact=email) | |
| except (user_model.DoesNotExist, user_model.MultipleObjectsReturned): | |
| return |
There was a problem hiding this comment.
Valid. Updated and fixed.
|
Addressed Gemini's suggestions now and improved displayed messaging in OFE when stopping remote desktop running on cluster partition too. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive support for browser-based remote desktop sessions, including both login-node and visualization-partition desktops. It adds the necessary infrastructure modules, a desktop broker service, and integration with the OFE frontend, including secure identity assertion signing. I have reviewed the changes and identified a few critical and high-priority issues, including a missing import in the C2 daemon, a potential bootstrap failure due to package installation order, and a security consideration regarding social account linking. Please address these issues and ensure the new example is indexed in the examples README as per the style guide.
| import socket | ||
| import subprocess | ||
| import sys | ||
| import tempfile | ||
| import time | ||
| import concurrent.futures | ||
| from datetime import datetime |
There was a problem hiding this comment.
The grp module is used on line 1033 (group_name = grp.getgrgid(gid).gr_name) but it is not imported at the top of the file. This will cause a NameError at runtime when attempting to submit a desktop job. Please add import grp to the imports.
| import socket | |
| import subprocess | |
| import sys | |
| import tempfile | |
| import time | |
| import concurrent.futures | |
| from datetime import datetime | |
| import grp | |
| import socket | |
| import subprocess | |
| import sys | |
| import tempfile | |
| import time | |
| import concurrent.futures | |
| from datetime import datetime |
There was a problem hiding this comment.
false positive (discussed above previously)
| try: | ||
| with socket.create_connection( | ||
| (service["host"], service["port"]), | ||
| timeout=2, | ||
| ): | ||
| return None |
There was a problem hiding this comment.
Performing synchronous network I/O (socket.create_connection) on the main request thread during a page load is a performance and reliability anti-pattern. If the desktop host is down, slow, or firewalled, this call will block the Django worker thread for up to 2 seconds. Under high load or multiple concurrent requests, this can easily exhaust the worker pool and cause a Denial of Service (DoS) for the entire front-end.
Consider caching the connectivity status (e.g., using Django's cache framework for 15 seconds) to avoid blocking the page render on every load.
| try: | |
| with socket.create_connection( | |
| (service["host"], service["port"]), | |
| timeout=2, | |
| ): | |
| return None | |
| cache_key = f"desktop_conn_{cluster.id}_{target}" | |
| is_ready = cache.get(cache_key) | |
| if is_ready is None: | |
| try: | |
| with socket.create_connection( | |
| (service["host"], service["port"]), | |
| timeout=2, | |
| ): | |
| is_ready = True | |
| except OSError: | |
| is_ready = False | |
| cache.set(cache_key, is_ready, 15) | |
| if is_ready: | |
| return None |
There was a problem hiding this comment.
Previously discussed the justification for the 2 second timeout (above)
| access_token="$( | ||
| metadata_get \ | ||
| 'http://metadata.google.internal/computeMetadata/v1/instance/service-accounts/default/token' | | ||
| python3 -c 'import json, sys; print(json.load(sys.stdin)["access_token"])' | ||
| )" |
There was a problem hiding this comment.
The script uses python3 to parse the metadata access token here, but install_packages (which installs python3) is not called until line 205. If the base image does not have python3 pre-installed, the script will fail with a command-not-found error. Please ensure install_packages is called before retrieving the access token.
There was a problem hiding this comment.
Not necessary. Example blueprint only.
| def pre_social_login(self, request, sociallogin): | ||
| if sociallogin.is_existing: | ||
| return | ||
|
|
||
| email = (sociallogin.user.email or "").strip().lower() |
There was a problem hiding this comment.
Automatically linking any social account to an existing local user based solely on the email address can be a security risk if unverified or non-Google social providers are enabled in the future. An attacker could register an account with a victim's email on a provider that does not enforce email verification, and then log in to the victim's OFE account.
Since the log message specifically mentions Google, please explicitly restrict this automatic linking to the 'google' provider.
| def pre_social_login(self, request, sociallogin): | |
| if sociallogin.is_existing: | |
| return | |
| email = (sociallogin.user.email or "").strip().lower() | |
| def pre_social_login(self, request, sociallogin): | |
| if sociallogin.is_existing: | |
| return | |
| if sociallogin.account.provider != "google": | |
| return | |
| email = (sociallogin.user.email or "").strip().lower() |
There was a problem hiding this comment.
Registration isn't possible so not relevant.
| # Remote Desktop Examples | ||
|
|
||
| These blueprints deploy browser-based Linux desktops alongside a Slurm cluster, | ||
| using the [novnc-runtime](../../modules/remote-desktop/novnc-runtime/README.md) | ||
| and [novnc-desktop](../../modules/remote-desktop/novnc-desktop/README.md) |
There was a problem hiding this comment.
According to the Repository Style Guide, if new examples are added, they must be added to the index in examples/README.md. Please ensure this new remote desktop example is indexed there.
References
- If new examples (core or community) are added, ensure they are added to the index in examples/README.md. (link)
|
I believe this PR is in a good position now in terms of addressing Gemini's concerns. Please feel free to get in touch with me if there's any questions or suggestions from a human being too 😄 |
|
I'm going to split this up into multiple PRs next |
|
Please NOTE : We recently upgraded our repository's Go version to 1.26 (#6173). We strongly recommend rebasing your open branches onto the latest develop branch to avoid or resolve any PR test failures. Thank you! |
Thanks for letting me know - will test things out and get it rebased 👍 |
…ters Adds two public modules and the shared runtime they are built on: - novnc-runtime: emits startup-script runners that install the desktop stack onto an existing VM, so a Slurm login node can host desktops and keep its scheduler tooling. - novnc-desktop: the same runtime plus a dedicated VM and firewall rule for standalone visualisation nodes. - internal/desktop-broker: the shared foundation both wrap. A small Python service that maps an authenticated request to a POSIX account via OS Login, starts a per-user Xvnc session, and relays RFB over a websocket to noVNC in the browser. Sessions are isolated per user: each Xvnc listens on a 0700 unix socket with no TCP listener at all, so one user cannot reach another's display even with a local shell on the host. The broker takes identity from request headers and requires a shared secret, so it must sit behind an authenticating proxy. This change reaches desktops over an IAP tunnel with a local nginx header injector, included as an example config. A load-balancer/IAP module that removes the local proxy is left to follow-up work. Both modules accept the shared secret either as a literal or as a Secret Manager secret ID fetched on the instance at boot, which keeps the value out of Terraform state. Includes 57 unit tests for the broker and a daily integration test.
5de12c4 to
dc10d9e
Compare
scott-nag
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a browser-based Linux desktop solution for Slurm clusters using noVNC. It adds a shared internal Python-based desktop session broker (desktop-broker) that handles user authentication via OS Login, session lifecycle, and VNC backends (TigerVNC and TurboVNC with optional VirtualGL GPU acceleration). It also introduces two new community modules, novnc-runtime and novnc-desktop, along with a validation blueprint (hpc-slurm-remote-desktop.yaml), local proxy configuration, comprehensive pytest suites, and daily integration tests. The new example has been correctly indexed in examples/README.md. No review comments were provided for this pull request.
| * [cae-slurm.yaml](#cae-slurmyaml-) ![core-badge] | ||
| * [hpc-build-slurm-image.yaml](#hpc-build-slurm-imageyaml--) ![community-badge] ![experimental-badge] | ||
| * [hpc-slurm-ubuntu2204.yaml](#hpc-slurm-ubuntu2204yaml-) ![community-badge] | ||
| * [hpc-slurm-remote-desktop.yaml](#hpc-slurm-remote-desktopyaml--) ![community-badge] ![experimental-badge] |
There was a problem hiding this comment.
Please add entries for new modules added to the file modules/README.md.
|
|
||
| ```bash | ||
| ./gcluster create community/examples/remote-desktop/BLUEPRINT.yaml -w | ||
| ./gcluster deploy BLUEPRINT |
There was a problem hiding this comment.
Nit: the BLUEPRINT placeholder for the deploy command is incorrect. once you run ./gcluster create <blueprint path>, we need to run ./gcluster deploy <deployment_name>
deployment_name is defined in the blueprint.
|
|
||
| ## Teardown Instructions | ||
|
|
||
| Replace `BLUEPRINT` with the `deployment_name` used in the blueprint vars block. |
There was a problem hiding this comment.
Nit: same here, it is confusing to use BLUEPRINT as placeholder, we need to use deployment_name.
| settings: | ||
| machine_type: n2-standard-4 | ||
| enable_public_ips: false | ||
| service_account_email: $(desktop-service-account.service_account_email) |
There was a problem hiding this comment.
Nit: instead of referencing other module, please add the module name to use clause.
| # openssl rand -base64 48 | gcloud secrets create ghpc-test-desktop-proxy-secret \ | ||
| # --replication-policy automatic --data-file=- | ||
| --- | ||
| test_name: "novnc-{{ os }}" |
There was a problem hiding this comment.
Thank you for adding the Test, will get back to you after validating it.
| services: | ||
| - compute.googleapis.com | ||
| - secretmanager.googleapis.com | ||
| - storage.googleapis.com |
There was a problem hiding this comment.
The README notes that novnc_proxy_secret and novnc_proxy_secret_id are mutually exclusive and exactly one is required.
To fail fast and prevent users from hitting strange Terraform errors later in the deployment process, we can enforce this immediately during the ./gcluster create phase using the toolkit's metadata validation.
Could you add validations block like this to the metadata.yaml for both novnc-desktop and novnc-runtime modules?
validations:
- validator: exclusive
inputs:
vars: [novnc_proxy_secret, novnc_proxy_secret_id]
error_message: "You must provide exactly one of 'novnc_proxy_secret' or 'novnc_proxy_secret_id'."| spec: | ||
| requirements: | ||
| services: | ||
| - secretmanager.googleapis.com |
There was a problem hiding this comment.
Same as above, please add metadata validator where ever applicable to fail fast.
5 participants
What this adds
Browser-based Linux desktops for Cluster Toolkit clusters: two public modules and the shared runtime they are built on.
community/modules/remote-desktop/novnc-runtimecommunity/modules/remote-desktop/novnc-desktopcommunity/modules/internal/desktop-brokerNote on this PR replacing its own history
The earlier revision bundled the Guacamole front end, the OFE integration and an nginx gateway alongside this runtime. It was too large to review sensibly, so it has been split.
Secrets
Both modules take the shared secret either as a literal or as a Secret Manager secret ID fetched on the instance at boot. The
_idform keeps the value out of Terraform state and out of the startup script staged in Cloud Storage, and is what the example and the integration test use.Testing
community/modules/internal/desktop-broker/files/tests/).Mutation-tested to confirm they are load-bearing: removing
-rfbport -1,accepting any secret, making session records world-readable, changing the
security type, and breaking the OS Login index were each caught.
tools/cloud-build/daily-tests/.../novnc-desktop.*).Packaging
TurboVNC, TigerVNC, VirtualGL and noVNC install from pinned upstream releases verified by SHA-256. No third-party apt/yum repositories are added and no GPG keys are imported. The only existing repository file this PR modifies is
examples/README.md, to index the new example.Note for reviewer
The daily test will require a
hpc-test-desktop-proxy-secretproxy secret added to test project's Secret Manager. One can be created withopenssl rand -base64 48 | gcloud secrets create ghpc-desktop-proxy-secret.