-
Notifications
You must be signed in to change notification settings - Fork 5
HYPERFLEET-439 - feat: add external API access via LoadBalancer #6
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
base: main
Are you sure you want to change the base?
HYPERFLEET-439 - feat: add external API access via LoadBalancer #6
Conversation
WalkthroughAdds an optional external API access feature to the Terraform GKE deployment. Introduces variable Sequence Diagram(s)sequenceDiagram
participant User as Terraform user
participant Terraform as Terraform
participant GCP as GCP APIs
participant GKE as GKE Node Pool
participant FW as Compute Firewall
User->>Terraform: run apply (with enable_external_api = true)
Terraform->>GKE: create/update node pool with tag gke-${cluster_name}
Terraform->>GCP: create google_compute_firewall.allow_lb_health_checks
GCP->>FW: provision firewall rule (allow TCP:8000, source_ranges = GCP health checks)
FW->>GKE: permit health-check traffic to nodes with tag
Terraform->>User: outputs external_api_enabled & external_api_note
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
9d9e51d to
a38b8ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @terraform/main.tf:
- Around line 74-93: The firewall resource
google_compute_firewall.allow_lb_health_checks uses target_tags
"gke-${local.cluster_name}" but GKE nodes lack that tag; fix by adding tags =
["gke-${local.cluster_name}"] to the node_config in the GKE node pool module
(terraform/modules/cluster/gke/main.tf) so nodes receive the tag and the
firewall applies, or alternatively update
google_compute_firewall.allow_lb_health_checks.target_tags to match actual
GKE-generated node tags (not recommended due to fragility).
🧹 Nitpick comments (2)
terraform/outputs.tf (1)
70-73: Consider using thekubernetes_namespacevariable instead of hardcoding.The Helm command hardcodes
-n hyperfleet-system, but this file has access tovar.kubernetes_namespace(default:"hyperfleet-system") andlocal.kubernetes_namespaceis used elsewhere. If users customize the namespace, this instruction could be misleading.Also, splitting the long conditional would improve readability.
♻️ Suggested refactor
output "external_api_note" { description = "Instructions for external API access" - value = var.enable_external_api ? "External API access is ENABLED. Deploy with: helm install hyperfleet charts/hyperfleet-gcp --set base.hyperfleet-api.service.type=LoadBalancer -n hyperfleet-system" : "External API access is DISABLED. Set enable_external_api=true to enable." + value = var.enable_external_api ? ( + "External API access is ENABLED. Deploy with: helm install hyperfleet charts/hyperfleet-gcp --set base.hyperfleet-api.service.type=LoadBalancer -n ${var.kubernetes_namespace}" + ) : ( + "External API access is DISABLED. Set enable_external_api=true to enable." + ) }terraform/main.tf (1)
71-73: Consider adding a warning about production use.Per PR objectives, this is intended for development deployments. Consider adding a comment or updating the description to warn against enabling this in production without additional security controls (e.g., IP allowlisting, authentication requirements).
📝 Suggested documentation
# ============================================================================= -# External API Access (optional firewall rule for LoadBalancer health checks) +# External API Access (optional firewall rule for LoadBalancer health checks) +# WARNING: This exposes the API externally. Intended for development only. +# For production, consider additional security controls (WAF, IP restrictions). # =============================================================================
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
terraform/main.tfterraform/outputs.tfterraform/variables.tf
🔇 Additional comments (3)
terraform/variables.tf (1)
165-173: LGTM!The variable is well-defined with a sensible default of
false(opt-in security posture). The type and description are clear, following the existing patterns in this file.terraform/outputs.tf (1)
65-68: LGTM!Clean passthrough of the variable value, providing visibility into the enabled state for downstream automation or users.
terraform/main.tf (1)
85-87: Source ranges are correct for GCP health checks.The CIDR ranges
35.191.0.0/16and130.211.0.0/22match the official GCP documentation for health check probe sources. Good documentation reference in the comment.
Add optional LoadBalancer support for exposing HyperFleet API externally. Changes: - Add enable_external_api variable (default: false) - Add firewall rule for GCP health check IPs (35.191.0.0/16, 130.211.0.0/22) - Add external_api_enabled and external_api_note outputs The firewall rule targets port 8000 and is only created when enable_external_api=true and cloud_provider=gke. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The LoadBalancer health check firewall rule targets nodes with tag
gke-${cluster_name}, but nodes didn't have that tag applied.
Added tags = ["gke-${var.cluster_name}"] to node_config so the
firewall rule correctly targets the GKE nodes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
a38b8ea to
e29bca7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
terraform/outputs.tf (1)
80-83: Consider breaking the long line for readability.The output logic is correct. For improved readability, consider using a heredoc or local for the long instruction string. Also, the hardcoded namespace
-n hyperfleet-systemcould diverge fromvar.kubernetes_namespaceif changed.♻️ Optional refactor for readability
output "external_api_note" { description = "Instructions for external API access" - value = var.enable_external_api ? "External API access is ENABLED. Deploy with: helm install hyperfleet charts/hyperfleet-gcp --set base.hyperfleet-api.service.type=LoadBalancer -n hyperfleet-system" : "External API access is DISABLED. Set enable_external_api=true to enable." + value = var.enable_external_api ? join(" ", [ + "External API access is ENABLED.", + "Deploy with: helm install hyperfleet charts/hyperfleet-gcp", + "--set base.hyperfleet-api.service.type=LoadBalancer", + "-n ${var.kubernetes_namespace}" + ]) : "External API access is DISABLED. Set enable_external_api=true to enable." }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
terraform/main.tfterraform/modules/cluster/gke/main.tfterraform/outputs.tfterraform/variables.tf
🚧 Files skipped from review as they are similar to previous changes (1)
- terraform/main.tf
🔇 Additional comments (4)
terraform/variables.tf (1)
165-173: LGTM!The variable declaration follows the established pattern in this file. The default of
falseis a sensible security-conscious choice, requiring explicit opt-in for external API exposure.terraform/outputs.tf (1)
75-78: LGTM!Simple and useful output for automation and scripting purposes.
terraform/modules/cluster/gke/main.tf (2)
40-43: LGTM!Formatting alignment changes with no semantic impact.
45-46: Tags are correctly consistent between node pool and firewall rule.The node pool tag
["gke-${var.cluster_name}"]and firewall rule target tag["gke-${local.cluster_name}"]both resolve to the same value ("gke-hyperfleet-dev-${var.developer_name}"), ensuring the firewall rule correctly targets the nodes created by this module. The implementation is sound.
Summary
Add optional LoadBalancer support for exposing the HyperFleet API externally for dev deployments.
enable_external_apivariable (default: false)external_api_enabledandexternal_api_noteoutputsChanges
terraform/variables.tf- Addedenable_external_apibool variableterraform/main.tf- Added conditional firewall rule for health checks (35.191.0.0/16, 130.211.0.0/22)terraform/outputs.tf- Added outputs for external API statusUsage
Then deploy helm with
service.type: LoadBalancer(see hyperfleet-chart PR).Test plan
enable_external_api=truecreates firewall rule🤖 Generated with Claude Code
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.