-
Notifications
You must be signed in to change notification settings - Fork 42
GCP Infrastructure manager terraform for credentials json #3835
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?
Conversation
|
This pull request does not have a backport label. Could you fix it @amirbenun? 🙏
|
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.
Pull request overview
This PR introduces a new deployment method for GCP service account credentials using GCP Infrastructure Manager (Terraform-based) as an alternative to the existing Deployment Manager approach. The key security improvement is that service account keys are now stored in Secret Manager rather than being exposed in deployment outputs.
Key Changes:
- New Terraform configuration for deploying service accounts with cloudasset.viewer and browser roles
- Bash scripts for setup and deployment using Infrastructure Manager
- Support for both project-level and organization-level IAM bindings
- Secret Manager integration for secure credential storage
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| deploy/infrastructure-manager/gcp-credentials-json/main.tf | Terraform configuration defining service account, IAM bindings, and Secret Manager resources |
| deploy/infrastructure-manager/gcp-credentials-json/variables.tf | Terraform input variables with validation for scope and parent_id |
| deploy/infrastructure-manager/gcp-credentials-json/outputs.tf | Terraform outputs for service account email and secret name |
| deploy/infrastructure-manager/gcp-credentials-json/setup.sh | Setup script to enable APIs and configure Infrastructure Manager service account |
| deploy/infrastructure-manager/gcp-credentials-json/deploy_service_account.sh | Main deployment script that orchestrates the Infrastructure Manager deployment |
| deploy/infrastructure-manager/gcp-credentials-json/README.md | Documentation with usage instructions and troubleshooting guidance |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # 4. Outputs instructions for using the key in Elastic Agent GCP integration | ||
|
|
||
| # Configure GCP project | ||
| PROJECT_ID=$(gcloud config get-value core/project) |
Copilot
AI
Jan 8, 2026
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.
The deployment script doesn't check if the PROJECT_ID was successfully retrieved from gcloud config. If the gcloud default project is not set, PROJECT_ID will be empty, leading to cryptic errors later. Add validation to ensure PROJECT_ID is not empty before proceeding.
| PROJECT_ID=$(gcloud config get-value core/project) | |
| PROJECT_ID=$(gcloud config get-value core/project) | |
| if [ -z "${PROJECT_ID}" ] || [ "${PROJECT_ID}" = "(unset)" ]; then | |
| echo "Error: GCP project is not configured. Please set a default project, for example:" >&2 | |
| echo " gcloud config set project YOUR_PROJECT_ID" >&2 | |
| exit 1 | |
| fi |
|
|
||
| # Accept parameters | ||
| PROJECT_ID="$1" | ||
| SERVICE_ACCOUNT="$2" |
Copilot
AI
Jan 8, 2026
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.
The setup script doesn't verify that the PROJECT_ID parameter is provided. If the first parameter is empty, the script will proceed with an empty project ID, which will cause cryptic errors later. Add validation at the beginning of the script to ensure both required parameters are provided.
| SERVICE_ACCOUNT="$2" | |
| SERVICE_ACCOUNT="$2" | |
| if [ -z "$PROJECT_ID" ] || [ -z "$SERVICE_ACCOUNT" ]; then | |
| echo "Usage: $0 PROJECT_ID SERVICE_ACCOUNT_NAME" | |
| exit 1 | |
| fi |
| echo "Setting up GCP Infrastructure Manager prerequisites..." | ||
|
|
||
| # Enable APIs | ||
| gcloud services enable "${REQUIRED_APIS[@]}" --quiet |
Copilot
AI
Jan 8, 2026
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.
Missing explicit project specification in the gcloud services enable command. While the PROJECT_ID is assigned, it's safer to explicitly pass --project="${PROJECT_ID}" to avoid potential issues if the gcloud default project is not set or is different.
| gcloud services enable "${REQUIRED_APIS[@]}" --quiet | |
| gcloud services enable "${REQUIRED_APIS[@]}" --project="${PROJECT_ID}" --quiet |
| # Secret Manager secret to store the service account key securely | ||
| resource "google_secret_manager_secret" "sa_key" { | ||
| secret_id = "elastic-agent-sa-key-${local.resource_suffix}" | ||
| project = var.project_id | ||
|
|
||
| replication { | ||
| auto {} | ||
| } | ||
| } | ||
|
|
||
| # Store the service account key in Secret Manager | ||
| resource "google_secret_manager_secret_version" "sa_key" { | ||
| secret = google_secret_manager_secret.sa_key.id | ||
| secret_data = google_service_account_key.elastic_agent_key.private_key | ||
| } |
Copilot
AI
Jan 8, 2026
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.
The service account key is stored in Secret Manager, but there's no configuration to grant necessary permissions to access the secret. The deploying user needs secretmanager.versions.access permission on the secret to retrieve it in deploy_service_account.sh. Consider adding a google_secret_manager_secret_iam_member resource to grant the necessary access, or document this requirement clearly.
|
|
||
| #### Option 1: Cloud Shell (Recommended) | ||
|
|
||
| [](https://shell.cloud.google.com/cloudshell/editor?cloudshell_git_repo=https://github.com/elastic/cloudbeat.git&cloudshell_git_branch=main&cloudshell_workspace=deploy/infrastructure-manager/gcp-credentials-json&show=terminal&ephemeral=true) |
Copilot
AI
Jan 8, 2026
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.
The README references a Cloud Shell button that opens the repository with cloudshell_git_branch=main, but this PR is presumably on a feature branch. When this is merged to main, the link will be correct, but during development or review, users clicking this button won't get the latest changes from the PR branch. Consider whether this should point to a different branch during development.
| # Organization-level IAM bindings | ||
| resource "google_organization_iam_member" "cloudasset_viewer_org" { | ||
| count = var.scope == "organizations" ? 1 : 0 | ||
| org_id = var.parent_id | ||
| role = "roles/cloudasset.viewer" | ||
| member = "serviceAccount:${google_service_account.elastic_agent.email}" | ||
| } | ||
|
|
||
| resource "google_organization_iam_member" "browser_org" { | ||
| count = var.scope == "organizations" ? 1 : 0 | ||
| org_id = var.parent_id | ||
| role = "roles/browser" | ||
| member = "serviceAccount:${google_service_account.elastic_agent.email}" | ||
| } |
Copilot
AI
Jan 8, 2026
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.
The org_id parameter in google_organization_iam_member resources expects a numeric organization ID, but the variable validation and documentation don't enforce or clarify this format requirement. If a user passes a full organization resource name (e.g., 'organizations/123456') instead of just the ID ('123456'), the resource creation will fail. Consider adding clearer documentation or validation for the expected format.
| REQUIRED_ROLES=( | ||
| roles/iam.serviceAccountAdmin | ||
| roles/iam.serviceAccountKeyAdmin | ||
| roles/resourcemanager.projectIamAdmin | ||
| roles/config.admin | ||
| roles/storage.admin | ||
| roles/secretmanager.admin |
Copilot
AI
Jan 8, 2026
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.
The setup script grants roles/secretmanager.admin to the Infrastructure Manager service account, but this is overly permissive. The service account only needs to create secrets and add versions (roles/secretmanager.secretAccessor or a custom role with secretmanager.secrets.create and secretmanager.versions.add permissions would be more appropriate). Following the principle of least privilege would improve security.
| roles/iam.serviceAccountKeyAdmin | ||
| roles/resourcemanager.projectIamAdmin | ||
| roles/config.admin | ||
| roles/storage.admin |
Copilot
AI
Jan 8, 2026
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.
The setup script grants roles/storage.admin to the Infrastructure Manager service account, but it's not clear why this broad storage permission is needed. Infrastructure Manager typically uses its own managed storage for state. If this is required for a specific purpose, it should be documented. Otherwise, consider removing this overly permissive role or replacing it with a more specific, limited role.
| roles/storage.admin |
| @@ -0,0 +1,77 @@ | |||
| terraform { | |||
| required_version = ">= 1.0" | |||
Copilot
AI
Jan 8, 2026
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.
The Terraform version constraint '>= 1.0' is quite permissive and doesn't specify an upper bound. While this provides flexibility, it could lead to compatibility issues if future Terraform versions introduce breaking changes. Consider using a more specific version constraint like '>= 1.0, < 2.0' to prevent potential issues with major version updates.
| required_version = ">= 1.0" | |
| required_version = ">= 1.0, < 2.0" |
| # This script: | ||
| # 1. Enables necessary APIs for Elastic Agent GCP integration |
Copilot
AI
Jan 8, 2026
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.
The comment on line 4 mentions that the script 'Enables necessary APIs for Elastic Agent GCP integration' but the comment should be more specific about what the script does. It should mention that it's a setup script for Infrastructure Manager prerequisites, not just API enablement. The actual purpose is clearer from line 26 output message than from this initial comment.
| # This script: | |
| # 1. Enables necessary APIs for Elastic Agent GCP integration | |
| # This script sets up prerequisites and deploys Infrastructure Manager resources for Elastic Agent GCP integration: | |
| # 1. Runs setup.sh to configure GCP Infrastructure Manager prerequisites (APIs, IAM roles, permissions) |
Summary of your changes
Adds a new method for deploying GCP service account credentials using GCP Infrastructure Manager (Terraform-based) as an alternative to the existing Deployment Manager approach in deploy/deployment-manager/. The key improvement is that service account keys are now stored securely in Secret Manager rather than being exposed in deployment outputs. The script creates a service account with cloudasset.viewer and browser roles, stores the JSON key in Secret Manager, and retrieves it locally to KEY_FILE.json for use in the Elastic Agent GCP integration. Supports both project-level and organization-level deployments via the ORG_ID environment variable.
Screenshot/Data
Related Issues
Checklist
Introducing a new rule?