Conversation
Summary of ChangesHello @fpittelo, 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 refactors the Terraform infrastructure configuration to centralize environment-specific variables. It transitions from using separate Highlights
Ignored Files
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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the Terraform configuration to use a centralized variable map for different environments, which is a great improvement for managing multiple deployments. The deletion of separate .tfvars files and the consolidation into variables.tf simplifies the structure. My review focuses on a critical security issue regarding hardcoded secrets in the new variable map and a medium-severity inconsistency in how resources reference the resource group. Addressing these points will enhance the security and maintainability of the infrastructure code.
| appinsights_instrumentation_key = "your-dev-appinsights-key" | ||
| rbac_enabled = true | ||
| kind = "OpenAI" | ||
| sku_name_cog_acct = "S0" | ||
| auth_enabled = true | ||
| google_client_id = "your-google-client-id" | ||
| google_client_secret = "your-google-client-secret" | ||
| microsoft_client_id = "your-microsoft-client-id" | ||
| microsoft_client_secret = "your-microsoft-client-secret" |
There was a problem hiding this comment.
Hardcoding default values for secrets and keys, even placeholder ones, is a critical security risk as they can be accidentally committed and exposed. These should be provided through a secure mechanism (like environment variables or a secrets manager) and not have default values in version control. I suggest replacing them with empty strings, similar to the qa and main environments, and providing the actual values for dev through a separate, non-versioned .tfvars file.
appinsights_instrumentation_key = ""
rbac_enabled = true
kind = "OpenAI"
sku_name_cog_acct = "S0"
auth_enabled = true
google_client_id = ""
google_client_secret = ""
microsoft_client_id = ""
microsoft_client_secret = ""
| az_location = local.environment_vars.az_location | ||
| az_rg_name = local.environment_vars.az_rg_name |
There was a problem hiding this comment.
For consistency and to create a clear dependency, it's better to reference the azurerm_resource_group.rg resource's attributes for location and name, just as you've done for the key_vault module. This ensures all resources are deployed to the same resource group and location defined by the rg resource.
This comment also applies to the app_service_plan, linux_web_app, cosmos_db modules, and the azurerm_application_insights resource.
az_location = azurerm_resource_group.rg.location
az_rg_name = azurerm_resource_group.rg.name
Dev to qa