feat(network): add internal application load balancer module - #6133
feat(network): add internal application load balancer module#6133mangal390 wants to merge 1 commit 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 adds a new community module for deploying an Internal Application Load Balancer (L7) on Google Cloud. The primary goal is to provide a highly available, static entry point for internal services, specifically addressing the need for a stable endpoint for the Slurm REST API in high-availability controller configurations. By utilizing TCP health checks and static IP reservation, this module ensures reliable service discovery and traffic routing for internal applications. 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-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 community module for an Internal Application Load Balancer (int-app-lb) along with an example blueprint demonstrating its integration with a High-Availability Slurm cluster. The feedback suggests utilizing the use block for module dependencies in both the blueprint and README example to leverage automatic variable propagation. Additionally, it is recommended to add a validation rule for the health_check_type variable to prevent silent failures, and to use the .address attribute instead of .id when referencing the static IP in the forwarding rule.
c45fa21 to
b40e239
Compare
|
this PR has been inactive for 7 days and has no unresolved comments. @GoogleCloudPlatform/hpc-toolkit, please review. |
|
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! |
b40e239 to
c226925
Compare
|
@aslam-quad I have rebased my branch as mentioned. Thank you! |
docs(examples): add slurm HA load balancer blueprint
c226925 to
e04f201
Compare
|
this PR has been inactive for 7 days and has no unresolved comments. @GoogleCloudPlatform/hpc-toolkit, please review. |
|
@arpit974 @shivam222 @shubpal07 - Requesting review on this PR |
|
/gcbrun |
|
this PR has been inactive for 6 days and has no unresolved comments. @arpit974, please review. |
| type = string | ||
| default = "internal-app-lb" | ||
| validation { | ||
| condition = can(regex("^[a-z]([-a-z0-9]*[a-z0-9])?$", var.lb_name)) && length(var.lb_name) <= 63 |
There was a problem hiding this comment.
The current validation allows length(var.lb_name) <= 63. However, the module appends resource suffixes such as -forwarding-rule (16 characters) and -backend-${each.key} (9+ characters).
In GCP Compute Engine, resource names have a strict 63-character limit (RFC 1035). If a user provides an lb_name between 48 and 63 characters, Terraform will pass variable validation but fail during terraform apply when creating the forwarding rule or backend services.
Please adjust the maximum length to <= 47 to account for these suffixes:
| condition = can(regex("^[a-z]([-a-z0-9]*[a-z0-9])?$", var.lb_name)) && length(var.lb_name) <= 63 | |
| condition = can(regex("^[a-z]([-a-z0-9]*[a-z0-9])?$", var.lb_name)) && length(var.lb_name) <= 47 | |
| error_message = "The lb_name must be a valid resource name: 1-47 lowercase alphanumeric characters or hyphens, starting with a letter and ending with an alphanumeric character." |
| subnetwork = var.subnetwork_self_link | ||
| ip_address = google_compute_address.default.address | ||
| load_balancing_scheme = "INTERNAL_MANAGED" | ||
| port_range = local.is_https ? "443" : "80" |
There was a problem hiding this comment.
port_range is currently hardcoded to local.is_https ? "443" : "80".
In the blueprint example, port: 6842 is specified under endpoints.slurm-restapi, but that only sets the backend port and health check probe port. The Load Balancer VIP (10.80.0.100) actually only listens on port 80. If an API client queries http://10.80.0.100:6842/..., the connection is refused.
Could we expose an optional var.port_range variable (defaulting to null) so users can optionally have the VIP listen on port 6842 or custom ports?
In variables.tf:
variable "port_range" {
description = "The port or port range that the forwarding rule listens on. If null, defaults to 443 for HTTPS or 80 for HTTP."
type = string
default = null
}In main.tf:
| port_range = local.is_https ? "443" : "80" | |
| port_range = coalesce(var.port_range, local.is_https ? "443" : "80") |
| name = "allpaths" | ||
| default_service = google_compute_region_backend_service.default[local.default_endpoint_name].id | ||
|
|
||
| dynamic "path_rule" { |
There was a problem hiding this comment.
In the Terraform Google provider, path_rule.paths accepts a list of strings (list(string)), allowing all paths routed to a given backend service to be grouped into a single path_rule block. Flattening every path into individual single-element blocks creates redundant resource definitions in the URL map.
We can simplify this and guard against empty path lists by iterating directly over var.endpoints:
| dynamic "path_rule" { | |
| dynamic "path_rule" { | |
| for_each = { for name, config in var.endpoints : name => config if length(config.paths) > 0 } | |
| content { | |
| paths = path_rule.value.paths | |
| service = google_compute_region_backend_service.default[path_rule.key].id | |
| } | |
| } |
| default = 10 | ||
| } | ||
|
|
||
| variable "health_check_timeout_sec" { |
There was a problem hiding this comment.
Google Cloud Health Checks require that check_interval_sec >= timeout_sec. If a user increases health_check_timeout_sec beyond health_check_interval_sec (default 10), the GCP API will reject the creation during apply.
Consider adding a validation rule to catch this during terraform plan / gcluster validate:
| variable "health_check_timeout_sec" { | |
| variable "health_check_timeout_sec" { | |
| description = "How long (in seconds) to wait before claiming failure." | |
| type = number | |
| default = 5 | |
| validation { | |
| condition = var.health_check_timeout_sec <= var.health_check_interval_sec | |
| error_message = "The health_check_timeout_sec must be less than or equal to health_check_interval_sec." | |
| } | |
| } |
| It provides advanced traffic management, support for regional backends (like Managed Instance Groups), and can be integrated with Identity-Aware Proxy (IAP) or custom SSL certificates. | ||
|
|
||
| This module is primarily intended for exposing the Slurm REST API (`slurmrestd`) in High-Availability setups, but is generic enough to route traffic for any internal L7 application. | ||
|
|
There was a problem hiding this comment.
Please add an explicit note in the README.md highlighting the Proxy-Only Subnet prerequisite for Regional Internal Application Load Balancers (INTERNAL_MANAGED).
While the example blueprint in community/examples/hpc-slurm-ha-lb.yaml properly provisions one, users copying the module snippet from this README into their own cluster networks will encounter deployment errors if their VPC lacks an active proxy-only subnet in that region.
Suggested addition:
> [!IMPORTANT]
> **Proxy-Only Subnet Prerequisite**: Google Cloud Regional Internal Application Load Balancers require an active proxy-only subnet in the deployment region (`purpose = "REGIONAL_MANAGED_PROXY"`, `role = "ACTIVE"`). Ensure your VPC configuration defines one prior to deploying this module.3 participants
Description
This PR introduces a
int-app-lb(Internal Application L7 Load Balancer) community module. It is primarily designed to expose internal services, for example, Slurm REST API (slurmrestd) across High-Availability controller instances.Currently, the toolkit supports deploying the Slurm controller in HA mode, but interacting with the REST API across multiple controllers requires external client-side failover logic. This module solves that by providing a single, highly available static IP for API clients to communicate with. Because
slurmrestdrequires JWT authentication, standard HTTP health checks return a401 Unauthorizedand fail. This module gracefully handles this by utilizing TCP health checks to verify service availability.Changes made:
community/modules/network/int-app-lb: A new Terraform module encapsulating Google Cloud L7 Internal Application Load Balancer resources (Backend Services, URL Maps, Target Proxies, and Forwarding Rules).slurmrestdavailability without requiring JWT tokens for the health check probes.google_compute_addressto ensure the LB receives a static internal IP (rather than an ephemeral one), guaranteeing stability for API clients.community/examples/hpc-slurm-ha-lb.yaml: A new end-to-end blueprint demonstrating how to deploy an HA Slurm cluster and securely wire the controller Managed Instance Group directly into the new Load Balancer backend.Tests:
Screenshots attached below demonstrating successful
openapi/v3API queries routing through the Load Balancer IP from a login node