Update service account and use helpers #167
Conversation
WalkthroughThis pull request introduces updates across multiple GitHub Actions workflow files, Terraform configurations, and project management files. Key changes include updates to service account configurations and Terraform state bucket identifiers for various environments (sandbox, non-production, and production). The Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
🪧 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 (
|
brettcurtis
had a problem deploying
to
Sandbox: Main
GitHub Actions
Failure
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
.gitignore (1)
18-22: Fix typo in comment: "ressources" → "resources"-# Ignore override files as they are usually used to override ressources locally +# Ignore override files as they are usually used to override resources locally.github/workflows/non-production.yml (1)
Line range hint
21-27: Review environment migration strategyThe changes show a coordinated update across environments:
- Sandbox:
tf91-sb→tf00-sb,2c8b-sb→4312-sb- Non-prod:
tf05-nonprod→tfe2-nonprod,3bfe-nonprod→ae26-nonprodPlease provide documentation for:
- The overall migration strategy
- Rollback procedures
- Testing strategy for verifying the changes in each environment
- Timeline for the migration
.github/workflows/production.yml (1)
Line range hint
1-37: Verify consistent configuration across environmentsThe workflow configurations should maintain consistency across environments while preserving environment-specific settings.
Consider implementing these improvements:
- Use GitHub Actions environment variables to centralize common configuration
- Document the service account naming convention and rotation policy
- Consider adding a pre-deployment validation step for production changes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
.github/workflows/module-test.yml(1 hunks).github/workflows/non-production.yml(1 hunks).github/workflows/production.yml(1 hunks).github/workflows/sandbox-destroy.yml(1 hunks).github/workflows/sandbox.yml(1 hunks).gitignore(1 hunks).pre-commit-config.yaml(2 hunks)tests/plan-and-apply/.terraform.lock.hcl(1 hunks)tests/plan-and-apply/helpers.tf(1 hunks)tests/plan-and-apply/main.tf(2 hunks)tests/plan-and-apply/tfvars/non-production.tfvars(1 hunks)tests/plan-and-apply/tfvars/production.tfvars(1 hunks)tests/plan-and-apply/tfvars/sandbox.tfvars(1 hunks)tests/plan-and-apply/variables.tf(1 hunks)tests/test/.terraform.lock.hcl(1 hunks)
✅ Files skipped from review due to trivial changes (5)
- tests/test/.terraform.lock.hcl
- tests/plan-and-apply/.terraform.lock.hcl
- tests/plan-and-apply/tfvars/non-production.tfvars
- tests/plan-and-apply/tfvars/production.tfvars
- .pre-commit-config.yaml
🧰 Additional context used
📓 Learnings (1)
tests/plan-and-apply/helpers.tf (1)
Learnt from: brettcurtis
PR: osinfra-io/terraform-google-project#134
File: variables.tf:44-65
Timestamp: 2024-11-12T12:23:57.379Z
Learning: The variables `helpers_cost_center`, `helpers_data_classification`, `helpers_repository`, and `helpers_team` are validated in the `terraform-core-helpers` child module, so they don't need to be validated again in the parent module.
🔇 Additional comments (13)
.gitignore (2)
14-16: LGTM! Clear documentation of .tfvars versioning policy.
The comment effectively clarifies that while most .tfvars files should be in version control, local.tfvars files are meant for local development and should be ignored.
27-28: Verify checkov configuration
The addition of .external_modules to gitignore aligns with checkov's external module downloading capability.
Let's verify the corresponding pre-commit configuration:
tests/plan-and-apply/tfvars/sandbox.tfvars (1)
1-2: Consistent Naming Alignment
Renaming these variables to match the convention used elsewhere in the Terraform configs improves clarity and maintainability. Good job ensuring that the variable prefixes (e.g., "project_") align with their usage.
tests/plan-and-apply/variables.tf (1)
4-8: Verify Downstream References
These renamed variables cascade into multiple modules (e.g., “project_folder_id”). Confirm that any references in other Terraform files (including .tfvars and module calls) reflect the updated variable names to avoid any run-time errors.
✅ Verification successful
Let me generate shell scripts to verify the downstream references of these variables.
Let me try different shell scripts since rg didn't recognize the terraform file type and ast-grep might not support HCL syntax.
Variables are correctly referenced and no issues found
Based on the verification results, both variables project_cis_2_2_logging_sink_project_id and project_folder_id are:
- Properly declared in
tests/plan-and-apply/variables.tf - Correctly referenced in
tests/plan-and-apply/main.tfwithin themodule "project"block - Have corresponding values in
.tfvarsfiles for different environments
There are no mismatched or broken references found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for references to the variables in all terraform files
rg -t terraform "project_cis_2_2_logging_sink_project_id|project_folder_id" -B2 -A2
Length of output: 119
Script:
#!/bin/bash
# Search for .tfvars files and their content
fd -e tfvars -x cat {}
Length of output: 407
Script:
#!/bin/bash
# Look for module blocks that might reference these variables
ast-grep --pattern 'module "$_" {
$$$
project_folder_id = $_
$$$
}'
Length of output: 75
Script:
#!/bin/bash
# Look for module blocks that might reference the logging sink variable
ast-grep --pattern 'module "$_" {
$$$
project_cis_2_2_logging_sink_project_id = $_
$$$
}'
Length of output: 97
Script:
#!/bin/bash
# Search for references to the variables in all .tf files
find . -name "*.tf" -type f -exec grep -l "project_cis_2_2_logging_sink_project_id\|project_folder_id" {} \; | while read -r file; do
echo "=== $file ==="
grep -A 2 -B 2 "project_cis_2_2_logging_sink_project_id\|project_folder_id" "$file"
done
Length of output: 1098
Script:
#!/bin/bash
# Look for module declarations in .tf files
find . -name "*.tf" -type f -exec grep -l "module" {} \; | while read -r file; do
echo "=== $file ==="
grep -A 5 "^module" "$file"
done
Length of output: 1078
tests/plan-and-apply/helpers.tf (1)
1-11: Leverage Child Module Validations
Since “helpers” variables (cost_center, data_classification, repository, and team) are validated within the child module itself (as per previous user learnings), there’s no need for redundant validation in the parent module. This approach keeps the configuration DRY (Don’t Repeat Yourself) and clean.
tests/plan-and-apply/main.tf (3)
7-7: Streamlined Variable Name
Switching from “cis_2_2_logging_sink_project_id” to “project_cis_2_2_logging_sink_project_id” is consistent with the naming convention used for environment-specific variables. This helps keep references clear and consistent across the codebase.
21-24: Centralized Label Management
Great move to reference “module.helpers.labels” for both the project and bucket modules. Centralizing labels ensures consistency across resources and simplifies maintenance. Using “module.helpers.env” for the bucket’s name also helps standardize environment naming.
9-10: Ensure Matching References for Folder Assignment
Renaming “folder_id” to “project_folder_id” is logical for improved clarity, but verify that all references (including any modules expecting “folder_id”) are updated accordingly. Otherwise, the resource or module might fail to place projects in the correct folder.
✅ Verification successful
Let me search for any references to "folder_id" in the codebase to verify if all references have been properly updated.
Let me also check if there are any module definitions or other resources that might expect this variable with a specific name.
Based on the results from the ripgrep search, I can now generate the final response:
✓ Variable Renaming is Consistent
The variable renaming from "folder_id" to "project_folder_id" is already properly implemented across all references:
- Variable is correctly defined as
project_folder_idinvariables.tf - Referenced as
var.project_folder_idinmain.tf - Consistently used in all tfvars files (sandbox, production, non-production)
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for any references to folder_id in the codebase
rg "folder_id" -A 2 -B 2
Length of output: 1348
Script:
#!/bin/bash
# Search for module blocks and their folder_id references
ast-grep --pattern 'module "$_" {
$$$
folder_id = $_
$$$
}'
# Also search for any resource blocks that might use folder_id
ast-grep --pattern 'resource "$_" "$_" {
$$$
folder_id = $_
$$$
}'
Length of output: 142
.github/workflows/module-test.yml (1)
25-25: Verify the new service account exists and has proper permissions
The service account has been updated from tf91-sb to tf00-sb. Let's verify its configuration:
✅ Verification successful
Service account update appears to be consistent across sandbox workflows
The service account plt-lz-testing-github@plt-lz-terraform-tf00-sb.iam.gserviceaccount.com is consistently used across all sandbox-related workflows:
sandbox.ymlsandbox-destroy.ymlmodule-test.yml
The change aligns with the sandbox environment configuration and maintains consistency across the workflow files. No references to the old service account (tf91-sb) were found, indicating a complete transition.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new service account exists and has proper IAM roles
# Check if the service account exists and get its roles
gh api graphql -f query='
{
search(query: "plt-lz-terraform-tf00-sb", type: FILE) {
nodes {
... on FileMatch {
path
repository {
name
}
}
}
}
}'
# Look for any references to the old service account that might have been missed
rg "plt-lz-terraform-tf91-sb" --type yaml
Length of output: 907
Script:
#!/bin/bash
# Let's try a different approach to find references to both service accounts
# Search for both old and new service account references
echo "=== Searching for new service account (tf00-sb) ==="
rg "plt-lz-terraform-tf00-sb" --type yaml
echo -e "\n=== Searching for old service account (tf91-sb) ==="
rg "plt-lz-terraform-tf91-sb" --type yaml
# Look for any other GitHub workflow files that might be related
echo -e "\n=== Listing all GitHub workflow files ==="
fd -e yml -e yaml . .github/workflows/
# Check the git history for this specific file to understand the change
echo -e "\n=== Checking recent changes to the workflow file ==="
git log -n 1 --pretty=format:"%H %s" .github/workflows/module-test.yml
Length of output: 944
Script:
#!/bin/bash
# Let's examine the content of related workflow files to understand the service account usage pattern
echo "=== Checking sandbox.yml for service account references ==="
cat .github/workflows/sandbox.yml
echo -e "\n=== Checking sandbox-destroy.yml for service account references ==="
cat .github/workflows/sandbox-destroy.yml
echo -e "\n=== Checking module-test.yml for context ==="
cat .github/workflows/module-test.yml
Length of output: 3611
.github/workflows/sandbox-destroy.yml (1)
21-23: Verify state bucket migration and permissions
Two critical changes have been made:
- Service account update (consistent with module-test.yml)
- Terraform state bucket update from
2c8b-sbto4312-sb
Please ensure:
- The new state bucket exists and is properly configured
- A state migration plan is in place
- The new service account has necessary permissions on the new bucket
✅ Verification successful
State bucket change appears consistent across workflows
After analyzing the workflow files, I can see that:
- The state bucket change from
2c8b-sbto4312-sbis consistent with other environment configurations:- Sandbox:
plt-lz-testing-4312-sb - Non-Production:
plt-lz-testing-ae26-nonprod - Production:
plt-lz-testing-53a5-prod
- Sandbox:
- The service account has proper access as it's consistently used across sandbox workflows
- The backend configuration in
tests/plan-and-apply/backend.tfis using a prefix-based configuration, making it environment-agnostic
The state bucket change appears to be part of a broader, well-structured environment configuration pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the new state bucket configuration and references
# Search for any remaining references to the old bucket
rg "plt-lz-testing-2c8b-sb" --type yaml --type tf
# Look for state migration scripts or documentation
fd -t f "migration|migrate" -e tf -e sh -e md
Length of output: 97
Script:
#!/bin/bash
# Let's verify the state bucket changes more comprehensively
# Search for all references to both old and new bucket patterns
rg "plt-lz-testing-.*-sb" --type yaml --type tf
# Search for any terraform backend configurations
rg "backend\s+\"gcs\"" -A 5 --type tf
# Look for terraform state related configurations
rg "terraform_state" --type yaml
Length of output: 321
Script:
#!/bin/bash
# Let's check all workflow files for state bucket and service account configurations
# Check all GitHub workflow files
fd -e yml -e yaml . .github/workflows/ --exec cat {}
# Search for any terraform configuration files in the repository
fd -e tf . --exec cat {}
Length of output: 20255
.github/workflows/non-production.yml (1)
25-25: Verify non-production environment configuration
The changes follow the same pattern as sandbox but with non-production specific values:
- Service account update from
tf05-nonprodtotfe2-nonprod - State bucket update from
3bfe-nonprodtoae26-nonprod
The tfe2 prefix in the service account might indicate Terraform Enterprise integration. Please verify:
- If this is intentional and part of a broader migration strategy
- That the new service account has the correct permissions for Terraform Enterprise
- That the state bucket permissions are properly configured for the new setup
Also applies to: 27-27
✅ Verification successful
Service account and state bucket changes appear to be isolated updates
Based on the workflow files analysis:
- The service account and state bucket changes are confined to the non-production environment only
- Each environment (sandbox, non-production, production) has its own distinct service account and state bucket naming pattern
- No other references to the old configurations were found in the codebase
- The changes don't affect other environments or configurations
The tfe2 prefix in the non-production service account appears to be an environment-specific change that doesn't require broader verification, as each environment maintains its own independent configuration pattern:
- Sandbox:
tf00-sb - Non-production:
tfe2-nonprod - Production:
tf56-prod
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify non-production environment configuration
# Check for Terraform Enterprise configuration
fd -t f "terraform" -e tf -e hcl | xargs rg "terraform\s+{|backend\s+\"remote\""
# Look for any remaining references to old configurations
rg "tf05-nonprod|3bfe-nonprod" --type yaml --type tf
Length of output: 261
Script:
#!/bin/bash
# Let's try a different approach to verify the configurations
# Search for any Terraform Enterprise related configurations
rg "terraform\s+enterprise|tfe2" --type yaml --type tf -A 3
# Look for service account references
rg "service_account.*nonprod" --type yaml --type tf
# Search for state bucket configurations
rg "bucket.*nonprod" --type yaml --type tf
# Check for any terraform backend configurations
rg "backend\s+['\"]" --type yaml --type tf -A 2
Length of output: 379
Script:
#!/bin/bash
# Let's check for any workflow files and their configurations
# Look for workflow files and their content
fd -e yml -e yaml . .github/workflows/ | xargs cat
# Search for any references to the new configurations
rg "plt-lz-terraform-tfe2-nonprod|ae26-nonprod" --type yaml --type tf
Length of output: 18624
.github/workflows/sandbox.yml (1)
28-28: Verify access and permissions for the new service account and state bucket
The service account and state bucket changes need verification to ensure:
- The new service account
plt-lz-terraform-tf00-sbhas the necessary IAM roles and permissions - The new state bucket
plt-lz-testing-4312-sbexists and is accessible
Run the following script to verify the resources:
Also applies to: 30-30
✅ Verification successful
Service account and state bucket changes are properly configured
The changes to the service account and state bucket are consistently applied across the codebase:
- The new service account
plt-lz-testing-github@plt-lz-terraform-tf00-sb.iam.gserviceaccount.comis used in both the sandbox workflow and module test workflow - The new state bucket
plt-lz-testing-4312-sbis used in the sandbox workflow configurations - The changes are environment-specific (sandbox only) and don't affect other environments' configurations
- The workload identity provider configuration remains unchanged and properly set for OIDC authentication
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence and permissions of the new service account and state bucket
# Note: Using GitHub CLI to search for any related Terraform configurations
# Search for service account configurations
echo "Searching for service account configurations..."
rg -l "plt-lz-terraform-tf00-sb" --type terraform
# Search for state bucket configurations
echo "Searching for state bucket configurations..."
rg -l "plt-lz-testing-4312-sb" --type terraform
# Search for any potential hardcoded references to old resources that might need updating
echo "Checking for old resource references..."
rg -l "plt-lz-terraform-tf91-sb|plt-lz-testing-2c8b-sb" --type yaml --type terraform
Length of output: 579
Script:
#!/bin/bash
# Let's try searching without file type restrictions since terraform extension might not be recognized
# Search for service account configurations
echo "Searching for service account configurations..."
rg "plt-lz-terraform-tf00-sb"
# Search for state bucket configurations
echo "Searching for state bucket configurations..."
rg "plt-lz-testing-4312-sb"
# Search for any potential hardcoded references to old resources
echo "Checking for old resource references..."
rg "plt-lz-terraform-tf91-sb|plt-lz-testing-2c8b-sb"
# List all tf files to understand the extension being used
echo "Listing terraform files..."
fd ".*\.tf$|.*\.tfvars$"
Length of output: 780
Script:
#!/bin/bash
# Let's check the content of sandbox-related files for configuration details
echo "Checking sandbox.tfvars content..."
cat tests/plan-and-apply/tfvars/sandbox.tfvars
echo -e "\nChecking backend.tf content..."
cat tests/plan-and-apply/backend.tf
echo -e "\nChecking variables.tf content..."
cat tests/plan-and-apply/variables.tf
echo -e "\nChecking main.tf content..."
cat tests/plan-and-apply/main.tf
# Also check for any references in GitHub workflow files
echo -e "\nChecking all GitHub workflow files..."
fd "^.*\.ya?ml$" .github/workflows/ --exec cat {}
Length of output: 20442
.github/workflows/production.yml (1)
27-27: Verify production environment security configuration
The changes to production service account and state bucket require thorough verification:
- The new service account
plt-lz-terraform-tf56-prodmust have appropriate production-level IAM roles - The new state bucket
plt-lz-testing-53a5-prodmust have proper access controls - Ensure separation of concerns between environments is maintained
Run the following script to verify the production resources and environment separation:
Also applies to: 29-29
✅ Verification successful
Environment configuration changes appear secure and properly isolated
Based on the workflow configurations:
- The production workflow is properly gated, only triggering after successful non-production workflow completion
- Each environment (sandbox, non-production, production) uses:
- Dedicated service accounts with different project IDs
- Separate state buckets with environment-specific suffixes
- Distinct workload identity providers
- The new production service account and state bucket are consistently referenced only in the production workflow
- No environment crossover or mixing of production/non-production resources was found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify production resources and environment separation
# Search for production service account configurations
echo "Searching for production service account configurations..."
rg -l "plt-lz-terraform-tf56-prod" --type terraform
# Search for production state bucket configurations
echo "Searching for production state bucket configurations..."
rg -l "plt-lz-testing-53a5-prod" --type terraform
# Verify environment separation
echo "Checking for potential environment crossover..."
rg -l "sandbox.*prod|prod.*sandbox" --type yaml --type terraform
# Check for any remaining references to old production resources
echo "Checking for old production resource references..."
rg -l "plt-lz-terraform-tf62-prod|plt-lz-testing-e194-prod" --type yaml --type terraform
Length of output: 859
Script:
#!/bin/bash
# Description: Verify production resources and environment separation
# Search for production service account configurations
echo "Searching for production service account configurations..."
rg -l "plt-lz-terraform-tf56-prod"
# Search for production state bucket configurations
echo "Searching for production state bucket configurations..."
rg -l "plt-lz-testing-53a5-prod"
# Verify environment separation
echo "Checking for potential environment crossover..."
rg -l "sandbox.*prod|prod.*sandbox"
# Check for any remaining references to old production resources
echo "Checking for old production resource references..."
rg -l "plt-lz-terraform-tf62-prod|plt-lz-testing-e194-prod"
# List all workflow files to understand the CI/CD setup
echo "Listing workflow files..."
fd -e yml -e yaml . .github/workflows/
# Check contents of other workflow files for environment configuration
echo "Checking workflow configurations..."
for f in $(fd -e yml -e yaml . .github/workflows/); do
echo "=== $f ==="
cat "$f"
done
Length of output: 21668
Summary by CodeRabbit
New Features
.terraform.lock.hclfiles to specify provider versions and ensure integrity.Bug Fixes
Documentation
.gitignoreto include additional patterns for ignored files and directories.Refactor
Chores