-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Support for Google Identity-Aware Proxy Provider #5
Conversation
WalkthroughThis pull request introduces comprehensive changes to integrate Google Identity-Aware Proxy (IAP) authentication into the Backstage application. The modifications span multiple configuration files across the project, including updates to authentication mechanisms, Terraform deployments, and application configurations. The changes aim to enable secure authentication through Google's IAP, replacing previous authentication methods with a more robust, proxy-based approach. Changes
Assessment against linked issues
The pull request comprehensively addresses the requirements for supporting Google Identity-Aware Proxy authentication in Backstage, implementing changes across configuration, authentication, and deployment layers. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…bles in deployment configurations
…erence to http-backend
…roxy in Backstage Helm configuration
…ge Helm configuration
…lhost in Backstage Helm configuration
…e Helm chart with organization details and TechDocs configuration
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: 3
🧹 Nitpick comments (11)
app/packages/backend/src/index.ts (1)
24-24
: Validate GCP IAP provider initialization
The addition of@backstage/plugin-auth-backend-module-gcp-iap-provider
is appropriate for enabling Identity-Aware Proxy support. Ensure the plugin usage is fully configured (e.g., environment variables, credentials, and IAP brand/client) and that no steps have been overlooked in the backstage configuration.deployments/variables.tf (1)
19-22
: Enhance the variable description for clarity.The description could be more detailed to help users understand:
- The expected format (service account email)
- The purpose (GCP Workload Identity integration)
- The relationship with IAP authentication
variable "k8s_workload_identity_service_account" { - description = "The service account to use for the workload identity" + description = "The service account email ([email protected]) used for GCP Workload Identity. This enables secure authentication between Kubernetes workloads and GCP services, including IAP." type = string }deployments/regional/variables.tf (2)
48-51
: LGTM! Consider adding validation.The variable replacement from
cloud_sql_host_project_id
tonetworking_project_id
better reflects its broader scope for shared VPC configuration.Consider adding validation to ensure the project ID follows GCP naming conventions:
variable "networking_project_id" { description = "The project ID for the shared VPC" type = string + validation { + condition = can(regex("^[a-z][a-z0-9-]{4,28}[a-z0-9]$", var.networking_project_id)) + error_message = "Project ID must be between 6 and 30 characters, start with a letter, and contain only lowercase letters, numbers, and hyphens." + } }
53-56
: Add validation for GCS bucket name format.The remote bucket variable is correctly defined, but adding validation would help prevent configuration errors.
variable "remote_bucket" { description = "The remote bucket the `terraform_remote_state` data source retrieves the state from" type = string + validation { + condition = can(regex("^[a-z0-9][a-z0-9-_.]{1,61}[a-z0-9]$", var.remote_bucket)) + error_message = "Bucket names must be between 3 and 63 characters, start and end with a number or letter, and can contain dots, hyphens, and underscores." + } }deployments/regional/helm/backstage.yml (1)
94-102
: Review permission configuration.The admin permissions configuration uses a simple group-based rule. While functional, consider:
- Adding more granular permissions for different user roles
- Documenting the group membership requirements
Consider expanding the permissions model:
permissions: rules: - name: backstage-admin-rule resourceType: all policy: allow conditions: - type: group group: admins + - name: backstage-developer-rule + resourceType: catalog-entity + policy: allow + conditions: + - type: group + group: developersdeployments/main.tf (1)
64-67
: Ensure secure handling of IAP client credentialsThe IAP client will generate OAuth credentials that need to be handled securely. Make sure:
- The credentials are stored securely in your secrets management system
- The credentials are only accessible to necessary services
- There's a rotation policy in place
Consider documenting the credential management process in your operational runbooks.
deployments/README.md (1)
45-48
: Add descriptions for the IAP client outputsThe outputs
backstage_iap_client_id
andbackstage_iap_client_secret
are missing descriptions. Consider adding meaningful descriptions to help users understand their purpose and usage.| Name | Description | |------|-------------| -| <a name="output_backstage_iap_client_id"></a> [backstage\_iap\_client\_id](#output\_backstage\_iap\_client\_id) | n/a | -| <a name="output_backstage_iap_client_secret"></a> [backstage\_iap\_client\_secret](#output\_backstage\_iap\_client\_secret) | n/a | +| <a name="output_backstage_iap_client_id"></a> [backstage\_iap\_client\_id](#output\_backstage\_iap\_client\_id) | The OAuth client ID for IAP authentication | +| <a name="output_backstage_iap_client_secret"></a> [backstage\_iap\_client\_secret](#output\_backstage\_iap\_client\_secret) | The OAuth client secret for IAP authentication |README.md (1)
43-55
: Enhance testing instructions for IAP configurationWhile the basic local development setup is documented, consider adding:
- Instructions for testing with IAP authentication locally
- Steps to verify IAP configuration in different environments
- Troubleshooting guide for common IAP-related issues
This will help developers validate the IAP integration properly.
deployments/regional/locals.tf (1)
43-46
: LGTM! The local values support IAP implementation.The new local values properly configure:
- Environment-specific hostnames for IAP endpoints
- DNS managed zones for domain management
- Remote state integration for infrastructure dependencies
However, consider documenting the expected outputs from the remote state for better maintainability.
Add comments explaining the expected outputs from
data.terraform_remote_state.main.outputs
:+ # Outputs from main remote state: + # - iap_oauth_client_id: The OAuth client ID for IAP + # - iap_oauth_client_secret: The OAuth client secret for IAP main = data.terraform_remote_state.main.outputsdeployments/regional/main.tf (2)
86-97
: Consider adding explicit dependency on Kubernetes ingress.The DNS record correctly uses the ingress IP, but consider adding an explicit
depends_on
block to ensure proper resource creation order.resource "google_dns_record_set" "backstage_a_record" { project = var.networking_project_id name = "${local.hostname}." # Trailing dot is required managed_zone = local.managed_zone type = "A" ttl = 300 rrdatas = [kubernetes_ingress_v1.backstage.status.0.load_balancer.0.ingress.0.ip] + + depends_on = [ + kubernetes_ingress_v1.backstage + ] }
147-217
: LGTM! Consider additional security headers.The ingress and IAP configuration is well-structured with proper HTTPS enforcement. Consider adding security-related annotations for enhanced protection.
metadata { name = "backstage" namespace = "backstage" annotations = { "kubernetes.io/ingress.allow-http" = "false" "networking.gke.io/managed-certificates" = kubernetes_manifest.backstage_tls.manifest.metadata.name + "nginx.ingress.kubernetes.io/force-ssl-redirect" = "true" + "nginx.ingress.kubernetes.io/ssl-redirect" = "true" + "nginx.ingress.kubernetes.io/hsts-max-age" = "31536000" + "nginx.ingress.kubernetes.io/hsts-include-subdomains" = "true" } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
app/packages/backend/package.json
is excluded by!**/*.json
app/yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
,!**/*.lock
📒 Files selected for processing (21)
.pre-commit-config.yaml
(1 hunks)README.md
(1 hunks)app/app-config.production.yaml
(0 hunks)app/app-config.sandbox.yaml
(0 hunks)app/app-config.yaml
(1 hunks)app/examples/admin.yaml
(1 hunks)app/packages/app/src/App.tsx
(2 hunks)app/packages/backend/src/index.ts
(1 hunks)deployments/README.md
(3 hunks)deployments/main.tf
(1 hunks)deployments/outputs.tf
(1 hunks)deployments/regional/README.md
(3 hunks)deployments/regional/helm/backstage.yml
(2 hunks)deployments/regional/locals.tf
(1 hunks)deployments/regional/main.tf
(3 hunks)deployments/regional/tfvars/us-east1-b-production.tfvars
(1 hunks)deployments/regional/tfvars/us-east1-b-sandbox.tfvars
(1 hunks)deployments/regional/variables.tf
(1 hunks)deployments/shared/helpers.tf
(1 hunks)deployments/tfvars/sandbox.tfvars
(1 hunks)deployments/variables.tf
(1 hunks)
💤 Files with no reviewable changes (2)
- app/app-config.production.yaml
- app/app-config.sandbox.yaml
✅ Files skipped from review due to trivial changes (2)
- .pre-commit-config.yaml
- deployments/tfvars/sandbox.tfvars
🔇 Additional comments (18)
deployments/regional/tfvars/us-east1-b-sandbox.tfvars (1)
1-2
: Validate references to removed variable
Thenetworking_project_id
andremote_bucket
variables have been introduced, presumably replacing the removedcloud_sql_host_project_id
. Ensure any references tocloud_sql_host_project_id
are fully removed throughout the Terraform codebase and that the new variables are properly passed to modules that rely on them.✅ Verification successful
The previous search didn't find any references to
cloud_sql_host_project_id
, but let's verify:
- If these variables are properly defined in Terraform files
- How they are being used across the codebase
- If there are any remnants of the old variable in tfvars files
This will help us confirm if the transition from
cloud_sql_host_project_id
to the new variables is complete and properly implemented.
Let's verify how these new variables are being used in the actual Terraform configurations to ensure they are properly integrated and there are no leftover references to the old variable.
Transition from
cloud_sql_host_project_id
to new variables is complete and correctThe verification shows that:
- No references to the old
cloud_sql_host_project_id
variable exist in the codebase- The new variables are properly defined in
deployments/regional/variables.tf
- The variables are correctly used:
networking_project_id
is used for host project ID in Cloud SQL and DNS record configurationsremote_bucket
is used for backend configurationThe transition appears to be complete and properly implemented with no remnants of the old variable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search the repository for references to `cloud_sql_host_project_id` rg -A 3 -B 3 "cloud_sql_host_project_id"Length of output: 40
Script:
#!/bin/bash # Let's check for any Terraform variable definitions and their usage fd -e tf -x cat {} | grep -A 3 -B 3 "variable.*\(networking_project_id\|remote_bucket\|cloud_sql_host_project_id\)" # Also check tfvars files for these variables fd -e tfvars -x cat {} | grep -A 3 -B 3 "\(networking_project_id\|remote_bucket\|cloud_sql_host_project_id\)"Length of output: 1093
Script:
#!/bin/bash # Let's check how these variables are being used in the Terraform configurations rg -A 3 -B 3 "\b(networking_project_id|remote_bucket)\b" --type tfLength of output: 2153
deployments/regional/tfvars/us-east1-b-production.tfvars (1)
1-2
: Consistency check across environments
Introducingnetworking_project_id
andremote_bucket
in production is aligned with the sandbox environment changes. Verify that all staging/other environment.tfvars
or relevant variables have been consistently updated to avoid mismatched configurations between environments.✅ Verification successful
Variables are missing in some environment configurations
The verification reveals that
networking_project_id
andremote_bucket
variables are only defined in two environment files, likely sandbox and production. If there are staging or other environments, they might be missing these required variables, which could lead to deployment issues.Let's verify if there are other environment files that should include these variables:
Let's check the content of the other tfvars files to understand if these variables should be present there:
Variables are correctly configured across environments
Based on the verification, the
networking_project_id
andremote_bucket
variables are appropriately configured in the regional tfvars files (deployments/regional/tfvars/
) for both sandbox and production environments. The other tfvars files (deployments/tfvars/
) contain different sets of variables for different purposes and don't require these networking-related variables.The configuration is consistent across environments where these variables are needed:
us-east1-b-production.tfvars
: Contains both variables with prod valuesus-east1-b-sandbox.tfvars
: Contains both variables with sandbox values🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Validate usage of new variables across environment .tfvars files fd -e tfvars --exec rg "(networking_project_id|remote_bucket)"Length of output: 268
Script:
#!/bin/bash # List all tfvars files to check for potential missing environment configurations fd -e tfvarsLength of output: 196
Script:
#!/bin/bash # Check content of the other tfvars files cat deployments/tfvars/production.tfvars deployments/tfvars/sandbox.tfvarsLength of output: 486
app/packages/app/src/App.tsx (2)
64-68
: Split environment-specific sign-in flows
This approach introduces a separate GCP IAP sign-in path for non-development environments. This is a solid pattern. Ensure that any local development toggles or environment variables are well-documented so new contributors understand why the fallback logic uses the guest provider locally.
32-32
: Confirm new imports are available
TheProxiedSignInPage
import from@backstage/core-components
is a new addition. Verify that your project dependencies align with this version of@backstage/core-components
, ensuring no version conflicts exist.✅ Verification successful
Dependencies are properly aligned with Backstage versioning
The verification shows that
@backstage/core-components
is correctly listed as a dependency with the version specifierbackstage:^
, which follows the same pattern as other Backstage dependencies in the project. This versioning scheme ensures compatibility across the Backstage ecosystem.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the version of @backstage/core-components rg "dependencies|core-components" package.json -A 2Length of output: 106
Script:
#!/bin/bash # Search for package.json files in the repository and check for @backstage/core-components dependency fd package.json --exec rg "dependencies|core-components" {} -A 2Length of output: 466
deployments/outputs.tf (1)
4-11
: LGTM! Properly configured IAP client outputs.The outputs are well-structured and follow Terraform best practices:
- Appropriate naming convention
- Sensitive flag set for the secret
- Documentation link provided
Let's verify the IAP client resource exists:
✅ Verification successful
IAP client outputs are correctly configured and resources exist
The verification confirms that both required resources are properly defined in the codebase:
google_iap_client.this
resource exists and is properly configured with the brand referencegoogle_iap_brand.this
resource exists and is properly configured with application title and project IDThe outputs in
outputs.tf
correctly reference these existing resources, making the configuration complete and valid.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the google_iap_client resource is properly defined rg "resource\s+\"google_iap_client\"\s+\"this\"" -A 5Length of output: 315
Script:
#!/bin/bash # Let's verify the IAP brand resource is also properly defined rg "resource\s+\"google_iap_brand\"\s+\"this\"" -A 5Length of output: 546
deployments/shared/helpers.tf (1)
9-10
: LGTM! Team and repository values updated correctly.The changes appropriately reflect the Backstage-specific configuration.
Let's verify the latest available version of the helpers module:
✅ Verification successful
Version v0.1.2 of terraform-core-helpers is the latest available
The code is using the most recent version (v0.1.2) of the terraform-core-helpers module, which is confirmed by checking the latest release tag.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check latest release version of terraform-core-helpers gh api repos/osinfra-io/terraform-core-helpers/releases/latest --jq .tag_nameLength of output: 86
app/examples/admin.yaml (2)
5-11
: Consider security implications of exposed admin details.While the user configuration is valid, consider:
- Limiting exposed personal information in version control
- Using environment variables or secrets management for sensitive details
- Adding a comment explaining the purpose of this admin account
15-20
: Verify group membership configuration.The group configuration looks good, but ensure:
- The empty children array is intentional
- Group permissions and roles are properly configured elsewhere
Let's check for related group configurations:
✅ Verification successful
Let me gather more information about the group configurations and their relationships.
Empty children array is intentional, permissions are properly configured
Based on the verification:
- The empty children array in the
admins
group is intentional as it follows the same pattern as theguests
group- Group permissions are properly configured:
- Permissions framework is enabled in
app/app-config.yaml
- Admin permissions are set in
deployments/regional/helm/backstage.yml
with appropriate rules- The
admin
user is correctly associated with theadmins
group throughmemberOf
- The configuration follows Backstage's documented structure (as referenced in the comments)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for additional group configurations rg "kind:\s+Group" --type yaml # Search for group permission configurations rg "permission" --type yaml -A 5Length of output: 1164
Script:
#!/bin/bash # Check content of both group files cat app/examples/org.yaml app/examples/admin.yaml # Look for any references to these groups in other YAML files rg "admins|org" --type yaml -A 3 # Check for role bindings or role configurations rg "role|binding" --type yaml -A 3Length of output: 2490
deployments/regional/helm/backstage.yml (2)
6-7
: Resource configurations look appropriate.The cloud-sql-proxy configuration and resource limits are well-defined and follow best practices:
- Private IP connectivity is enabled
- Resource limits are set appropriately for both memory and CPU
- Requests are set to reasonable values for initial scaling
Also applies to: 12-15
71-78
: Review database connection security.The database configuration uses environment variables for credentials, which is good. However, there are some security considerations:
- The connection is using localhost, implying the cloud-sql-proxy sidecar
- No SSL configuration is specified
Consider adding SSL configuration for enhanced security:
database: client: pg connection: host: localhost port: 5432 user: ${POSTGRES_USER} password: ${POSTGRES_PASSWORD} + ssl: + rejectUnauthorized: true + ca: ${POSTGRES_SSL_CERT}deployments/main.tf (3)
36-36
: LGTM: IAP service enabled correctlyThe addition of "iap.googleapis.com" is necessary for enabling Identity-Aware Proxy functionality.
72-76
: Verify workload identity configurationThe Cloud SQL client role assignment looks correct, but please verify:
- The workload identity service account variable is properly set in all environments
- The minimal required permissions are granted
✅ Verification successful
Cloud SQL IAM configuration is properly set with minimal permissions
The verification shows that:
- The workload identity service account variable is properly defined and set in the tfvars files
- Only the necessary
roles/cloudsql.client
role is granted, which is the minimal required permission for Cloud SQL access- No other excessive Cloud SQL-related permissions are assigned to this service account
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for the workload identity variable in tfvars files rg "k8s_workload_identity_service_account" --type tf # List all IAM roles assigned to this service account ast-grep --pattern 'resource "google_project_iam_member" $_$ { member = "serviceAccount:${var.k8s_workload_identity_service_account}" $$$ }'Length of output: 369
Script:
#!/bin/bash # Check the variable definition and any default values cat deployments/variables.tf | grep -A 5 "k8s_workload_identity_service_account" # Check for tfvars files that might set this variable fd ".tfvars$" # Look for other IAM roles assigned to service accounts in the project rg 'resource "google_project_iam_member"' -A 5 --type tfLength of output: 821
Script:
#!/bin/bash # Check the content of tfvars files for the workload identity variable cat deployments/regional/tfvars/us-east1-b-production.tfvars deployments/regional/tfvars/us-east1-b-sandbox.tfvars deployments/tfvars/production.tfvars deployments/tfvars/sandbox.tfvars | grep "k8s_workload_identity_service_account" # Check for any other IAM-related configurations that might grant additional permissions rg "roles/cloudsql" --type tfLength of output: 433
50-59
: Verify the support email configurationThe support email ([email protected]) needs to be verified as:
- A valid email address that your team controls
- Either a user email or a Google Groups alias (as noted in comments)
- Accessible for IAP-related support requests
This is especially important as the brand cannot be easily modified after creation.
deployments/regional/README.md (2)
31-47
: LGTM! Comprehensive documentation of IAP infrastructure.The new resources and data sources properly document the IAP implementation:
- DNS configuration for IAP endpoints
- Kubernetes resources for IAP integration
- Backend configuration for IAP authentication
- TLS setup for secure communication
61-62
: LGTM! Clear documentation of new input variables.The new input variables properly support:
- Shared VPC networking for IAP
- Remote state management for infrastructure dependencies
deployments/regional/main.tf (3)
1-13
: LGTM! Well-structured remote state configuration.The remote state configuration follows Terraform best practices with clear workspace naming and proper GCS backend setup.
22-22
: Verify networking_project_id variable definition.The change to use
networking_project_id
aligns with the project structure, but let's ensure this variable is properly defined.✅ Verification successful
Variable
networking_project_id
is properly definedThe variable
networking_project_id
is correctly defined indeployments/regional/variables.tf
with appropriate description and type declaration, making the change to use this variable inmain.tf
valid and consistent.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if networking_project_id variable is defined rg -A 5 'variable.*networking_project_id' --type tfLength of output: 397
222-234
: Verify IAP credentials in remote state and consider encryption.The IAP secret configuration looks good, but let's verify:
- The existence of IAP client outputs in the remote state
- Consider using a secret management solution like Google Secret Manager for the IAP credentials
Consider migrating sensitive credentials to Google Secret Manager for enhanced security:
data "google_secret_manager_secret_version" "iap_client_id" { secret = "backstage-iap-client-id" project = var.project_id } data "google_secret_manager_secret_version" "iap_client_secret" { secret = "backstage-iap-client-secret" project = var.project_id }✅ Verification successful
IAP credentials configuration is properly set up, but could be enhanced
The verification confirms that:
- IAP client credentials are properly managed through the
google_iap_client
resource- The outputs are correctly defined with the client secret marked as sensitive
- The Kubernetes secret is properly configured to use these outputs
While the current setup is functional, here's a security enhancement suggestion:
- Consider migrating sensitive credentials to Google Secret Manager for additional security controls and audit capabilities
- This can be implemented using the
google_secret_manager_secret_version
data source as shown in the original architecture advice🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for IAP client outputs in Terraform files rg -A 5 'output.*backstage_iap_client' --type tfLength of output: 419
Script:
#!/bin/bash # Check for the IAP client resource definition rg -A 10 'resource.*google_iap_client.*this' --type tfLength of output: 708
Fixes #2
Summary by CodeRabbit
New Features
Documentation
Configuration Changes
Authentication