Repository navigation
feat(iap): add external app load balancer, backend service, and managed certificate modules- #202 - #6632
feat(iap): add external app load balancer, backend service, and managed certificate modules- #202#6632scott-nag wants to merge 6 commits into
Conversation
Pin IAP assertions to ES256, rate-limit key refetches, allow 60s clock skew, refuse cross-site websocket upgrades, require TLS 1.2 in ext-app-lb, pin cryptography (quoting pip specifiers so the pin installs), split the labels locals, and correct the docs on port reachability and the backend service lookup. Validate the OAuth client pair, and take the example's firewall port from the desktop runtime.
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 a secure, browser-based access method for remote desktops within a Slurm cluster environment. By leveraging Google Cloud's Identity-Aware Proxy (IAP) and an external Application Load Balancer, users can now access their desktops via HTTPS hostnames. The changes include new infrastructure modules for managing backend services, load balancers, and managed certificates, as well as significant updates to the desktop broker to handle IAP assertion verification and enhanced security checks for websocket connections. 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
|
|
/gemini review |
![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 introduces a new blueprint and several supporting modules ("ext-app-lb", "iap-backend-service", and "managed-certificate") to enable browser-based Linux desktops on a Slurm cluster via an HTTPS load balancer behind Identity-Aware Proxy (IAP). It also updates the internal "desktop-broker" to support a TCP listener and verify signed IAP assertions. The review feedback suggests enhancing error handling in "iap.py" when parsing the service account token from the metadata server, and refactoring the "google_compute_network_endpoint" resource in "iap-backend-service/main.tf" to use "for_each" instead of "count" to prevent resource disruption during list reordering.
There was a problem hiding this comment.
Code Review
This pull request introduces a new blueprint (hpc-slurm-remote-desktop-iap.yaml) and three supporting network modules (ext-app-lb, iap-backend-service, and managed-certificate) to enable browser-based Linux desktops on a Slurm cluster reached via an HTTPS load balancer behind Identity-Aware Proxy (IAP). It also updates the desktop-broker and novnc modules to support TCP listening, verify signed IAP assertions, map Google accounts to OS Login accounts, and enforce cross-site websocket origin checks. The review feedback highlights opportunities to improve the robustness of the Python broker code, specifically by wrapping metadata token retrieval in a try...except block to handle parsing errors gracefully, and catching AttributeError when parsing JWKs. Additionally, a potential Terraform plan-time crash is addressed by suggesting the use of try() when accessing the computed dns_resource_record list in the managed certificate outputs.
There was a problem hiding this comment.
Code Review
This pull request introduces support for accessing remote desktops on a Slurm cluster via an HTTPS load balancer behind Identity-Aware Proxy (IAP). It adds new modules for a global external Application Load Balancer (ext-app-lb), an IAP backend service (iap-backend-service), and a Google-managed certificate (managed-certificate), along with a new example blueprint. The desktop-broker is updated to support a TCP listener, verify signed IAP assertions, map Google accounts to OS Login accounts, and enforce same-site websocket origin checks. The review feedback recommends improving robustness by rate-limiting metadata server refetches for unknown OS Login subjects to prevent potential DoS, and wrapping metadata token retrieval in a try-except block to handle parsing errors gracefully.
# Conflicts: # llms-full.txt # llms.txt # skills/cluster-blueprint-assistant/references/catalog.yaml
Remote desktops through an HTTPS load balancer behind IAP
This lets users reach the noVNC remote desktops (#6141) in a browser at an HTTPS hostname, signed in through Identity-Aware Proxy. No SSH tunnel is needed, and the existing SSH access still works alongside it.
Changes
desktop-broker,novnc-runtime,novnc-desktop:iap_backend_serviceandiap_listen_port. When they're set, the broker also listens on TCP for the load balancer.audandiss. The broker maps it to the user's OS Login account, and OS Login decides access, as it does over SSH.network/iap-backend-service: a backend service behind IAP, with a zonal NEG of the desktop hosts, a health check, and IAP members.network/ext-app-lb: the front end of a global external Application Load Balancer: address, certificate, URL map, and host routes to backend services defined elsewhere. TLS 1.2 is the minimum by default. It's the external counterpart ofint-app-lb.network/managed-certificate: a Certificate Manager certificate authorised by DNS (wildcards supported), with a certificate map forext-app-lb.community/examples/remote-desktop/hpc-slurm-remote-desktop-iap.yaml, a Slurm cluster with a login-node desktop and a visualization VM desktop, each at its own hostname.Testing
managed-certificate. Both desktops were reachable through IAP, and SSH access kept working.make tests:TestInitUserConfig_ExistingIsGoogler(pkg/config) fails, but it also fails on an unmodifieddevelopdeployment.Known limitation:
ext-app-lbcan't switch an existing load balancer from Compute Engine certificates to acertificate_mapin place. The Google provider clears the proxy's certificates before setting the map, which the API rejects, so the load balancer has to be recreated. This is documented in the module README.Cloud-Docs: none (adds community modules and one community blueprint; no public docs page changes required)