Skip to content

Conversation

@kferrone
Copy link
Collaborator

@kferrone kferrone commented Mar 10, 2025

User description

Define your entire stack from docker compose file.


PR Type

Enhancement, Tests, Documentation


Description

  • Introduced a Terraform module for parsing .env files.

  • Added a new compose module to handle Docker Compose configurations.

    • Supports services, configs, and secrets with DuploCloud integration.
    • Processes Docker Compose YAML files and extracts relevant components.
  • Enhanced configuration management with new schema definitions and support for AWS SSM parameters.

  • Added comprehensive tests for .env parsing, Docker Compose handling, and AWS SSM configurations.


Changes walkthrough 📝

Relevant files
Enhancement
16 files
main.tf
Example for using the `.env` file parser module                   
+28/-0   
config.tf
Placeholder for Docker Compose configs handling                   
+9/-0     
main.tf
Core logic for processing Docker Compose YAML                       
+45/-0   
outputs.tf
Outputs for processed services, configs, and secrets         
+9/-0     
services.tf
Service module integration for Docker Compose services     
+17/-0   
variables.tf
Variables for Docker Compose module                                           
+17/-0   
aws-ssm.tf
Enhanced AWS SSM parameter handling                                           
+4/-4     
csi-class.tf
Updated CSI class definitions for AWS secrets                       
+2/-2     
main.tf
Refactored configuration logic with schema definitions     
+26/-7   
variables.tf
Added new configuration classes for AWS SSM                           
+2/-1     
main.tf
Logic for parsing `.env` files into key-value pairs           
+31/-0   
outputs.tf
Outputs for parsed `.env` data                                                     
+3/-0     
variables.tf
Variables for `.env` file parser module                                   
+14/-0   
main.tf
Updated tenant variable for Lambda module                               
+1/-1     
variables.tf
Updated and added variables for Lambda module                       
+8/-8     
variables.tf
Added validation and description for micro-service variables
+11/-0   
Dependencies
1 files
.terraform.lock.hcl
Lock file for Terraform dependencies in compose module     
+64/-0   
Tests
6 files
main.tftest.hcl
Tests for Docker Compose module functionality                       
+31/-0   
main.tftest.hcl
Updated tests for configuration module                                     
+6/-1     
ssm.tftest.hcl
New tests for AWS SSM parameter configurations                     
+40/-0   
main.tftest.hcl
Tests for `.env` file parsing module                                         
+17/-0   
docker-compose.yaml
Sample Docker Compose YAML for testing                                     
+72/-0   
.env
Sample `.env` file for testing                                                     
+5/-0     
Configuration changes
2 files
versions.tf
Required providers and versions for compose module             
+13/-0   
versions.tf
Required providers and versions for `.env` file parser     
+6/-0     
Documentation
1 files
README.md
Documentation for `.env` file parser module                           
+3/-0     
Additional files
1 files
README.md [link]   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-code-review
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 Security concerns

    Sensitive information exposure:
    The module processes environment variables and secrets that might contain sensitive information like AWS credentials. In modules/compose/tests/docker-compose.yaml, there are references to AWS_ACCESS_KEY_ID, AWS_SECRET_ACCESS_KEY, and AWS_SESSION_TOKEN. While these appear to be placeholders in the test file, the module should ensure that these values are properly handled as sensitive data and not exposed in logs or outputs.

    ⚡ Recommended focus areas for review

    Port Parsing Bug

    The port extraction from Docker Compose ports is incorrect. It attempts to get the host port by taking the first element after splitting by colon, but Docker Compose format is typically HOST:CONTAINER, so it should be the first element, not the second.

    port = length(each.value.ports) > 0 ? split(":", each.value.ports[0])[0] : 80
    Error Handling

    The code assumes Docker Compose files will always have certain keys like 'services', 'configs', and 'secrets'. If any of these keys are missing, the code might fail with unclear errors.

    compose = {
      for component_type in ["services", "configs", "secrets"] : component_type => {
        for name, service in {
          for name, service in local.docker_compose[component_type] : name => service
          if contains(keys(service), "labels")
        } : name => service
        if contains(keys(service.labels), "duplo.class")
      }
    }
    Regex Limitation

    The regex pattern for parsing environment variables doesn't handle all valid .env file formats, such as variables with spaces around equals sign or multi-line values.

    kv_pattern = "^(\\w+)=['\"]?(.*?)['\"]?$"

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Mar 10, 2025

    CI Feedback 🧐

    (Feedback updated until commit 3df422e)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: Test / Testing Module (compose)

    Failed stage: TF Validate Module [❌]

    Failed test name: terraform fmt

    Failure summary:

    The action failed because the Terraform formatting check (terraform fmt) failed. The log shows
    formatting differences in the variables.tf file (lines 1375-1386) where spacing in the type and
    default attributes needed to be adjusted. The specific error is shown on line 1387: "Terraform
    exited with code 3." Line 1448 confirms this with "FMT_OUTCOME: failure".

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Ubuntu
    ...
    
    952:  �[36;1mMODULE=$(basename $WKDIR)�[0m
    953:  �[36;1mDUPLO_TF_BUCKET=${PREFIX}-${DUPLO_ACCOUNT_ID}�[0m
    954:  �[36;1mARGS=(�[0m
    955:  �[36;1m  -input=false�[0m
    956:  �[36;1m)�[0m
    957:  �[36;1m�[0m
    958:  �[36;1mif [ "$AWS_ENABLED" == "true" ]; then�[0m
    959:  �[36;1m  echo "AWS enabled, setting up backend config for S3 and DynamoDB"�[0m
    960:  �[36;1m  ARGS+=(�[0m
    961:  �[36;1m    -backend-config=dynamodb_table=${DUPLO_TF_BUCKET}-lock�[0m
    962:  �[36;1m    -backend-config=region=$DUPLO_DEFAULT_REGION�[0m
    963:  �[36;1m    -backend-config=bucket=$DUPLO_TF_BUCKET�[0m
    964:  �[36;1m  )�[0m
    965:  �[36;1melif [ "$AZURE_ENABLED" == "true" ]; then�[0m
    966:  �[36;1m  echo "Azure enabled, setting up backend config for Blob Storage"�[0m
    967:  �[36;1m  # if the RESOURCE_GROUP variable is not set, fail with error�[0m
    968:  �[36;1m  if [ -z "$RESOURCE_GROUP" ]; then�[0m
    ...
    
    1372:  secret_key.environment => lookup(
    1373:  var.args, secret_key.environment, null
    1374:  )
    1375:  variables.tf
    1376:  --- old/variables.tf
    1377:  +++ new/variables.tf
    1378:  @@ -12,6 +12,6 @@
    1379:  description = <<EOF
    1380:  These are the environment variable arguments for the compose file itself. For example if the image is something like $IMAGE:$TAG you can pass in IMAGE and TAG values to replace the variables in the compose file.
    1381:  EOF
    1382:  -  type = map(string)
    1383:  -  default = {}
    1384:  +  type        = map(string)
    1385:  +  default     = {}
    1386:  }
    1387:  ##[error]Terraform exited with code 3.
    1388:  ##[error]Process completed with exit code 1.
    1389:  ##[group]Run function outcome() {
    ...
    
    1433:  ROOTDIR: 
    1434:  WORKDIR: 
    1435:  AWS_REGION: us-west-2
    1436:  AWS_ACCESS_KEY_ID: ***
    1437:  AWS_SECRET_ACCESS_KEY: ***
    1438:  AWS_SESSION_TOKEN: ***
    1439:  TF_IN_AUTOMATION: true
    1440:  TF_MODULES: 
    1441:  TFLINT_FILE: /home/runner/work/terraform-duplocloud-components/terraform-duplocloud-components/.tflint.hcl
    1442:  LINT_ENABLED: true
    1443:  TF_VERSION: 1.10.5
    1444:  TERRAFORM_CLI_PATH: /home/runner/work/_temp/75d929da-6278-4fa9-beeb-4f254cd04ede
    1445:  DUPLO_TF_BUCKET: duplo-tfstate-813590939111
    1446:  MODULE_NAME: compose
    1447:  MODULE_DIR: modules/compose
    1448:  FMT_OUTCOME: failure
    1449:  VALIDATE_OUTCOME: success
    

    @qodo-code-review
    Copy link
    Contributor

    qodo-code-review bot commented Mar 10, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix port assignment

    Fix the port assignment logic. Currently it's taking the left side of the colon
    which is the host port, but should take the right side which is the container
    port.

    modules/compose/services.tf [1-17]

     module "services" {
       for_each = {
         for name, service in local.services : name => service
         if service.labels["duplo.class"] == "service"
       }
       source = "../../modules/micro-service"
       tenant = var.tenant
       name   = each.key
       image = {
         uri = each.value.image
       }
    -  # if there is a port use regex to get the right side of the colon of the poirt
    -  port = length(each.value.ports) > 0 ? split(":", each.value.ports[0])[0] : 80
    +  # if there is a port use regex to get the right side of the colon of the port
    +  port = length(each.value.ports) > 0 ? split(":", each.value.ports[0])[1] : 80
       lb = {
         enabled = (each.value.labels["duplo.lb.enabled"] == "true")
       }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: The current code incorrectly uses the host port (left side of colon) instead of the container port (right side). This is a critical bug that would cause services to be configured with incorrect port mappings, potentially breaking connectivity.

    High
    Add null check

    Add null check for component_type in docker_compose to prevent errors when a
    component type is missing from the compose file.

    modules/compose/main.tf [1-11]

     locals {
       docker_compose = yamldecode(templatefile(var.file, var.args))
       compose = {
         for component_type in ["services", "configs", "secrets"] : component_type => {
           for name, service in {
    -        for name, service in local.docker_compose[component_type] : name => service
    +        for name, service in lookup(local.docker_compose, component_type, {}) : name => service
             if contains(keys(service), "labels")
           } : name => service
           if contains(keys(service.labels), "duplo.class")
         }
       }
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: This suggestion addresses a potential runtime error when a component type is missing from the compose file. Using lookup() with a default empty map prevents the code from failing when a section is missing, making the module more robust.

    Medium
    General
    Fix regex pattern

    The regex pattern for parsing environment variables is too restrictive. It only
    allows alphanumeric characters and underscores in variable names, but
    environment variables can contain other characters like dots or hyphens.

    modules/env-file/main.tf [11-12]

     # Pattern to capture key and value with or without quotes
    -kv_pattern = "^(\\w+)=['\"]?(.*?)['\"]?$"
    +kv_pattern = "^([A-Za-z0-9_.-]+)=['\"]?(.*?)['\"]?$"
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The current regex pattern is too restrictive and would fail to parse environment variables with hyphens or dots in their names. This improvement makes the module more compatible with common environment variable naming conventions.

    Medium
    • Update

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants