Skip to content
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

update the AWS terraform root module #399

Merged
merged 1 commit into from
Jan 23, 2025
Merged

Conversation

DaMandal0rian
Copy link
Member

@DaMandal0rian DaMandal0rian commented Jan 23, 2025

PR Type

enhancement, configuration changes


Description

  • Enhanced EC2 instance configurations with new features and variables.

  • Added support for Elastic IP creation and association.

  • Updated Terraform and AWS provider version requirements.

  • Removed hardcoded AMI data source and improved AMI handling logic.


Changes walkthrough 📝

Relevant files
Configuration changes
ami.tf
Removed hardcoded Ubuntu AMI data source                                 

templates/terraform/aws/ec2/ami.tf

  • Removed hardcoded data source for Ubuntu AMI.
  • Simplified AMI handling by removing specific filters.
  • +0/-20   
    variables.tf
    Added Elastic IP variables and updated existing ones         

    templates/terraform/aws/ec2/variables.tf

  • Added new variables for Elastic IP configuration.
  • Updated default values and descriptions for existing variables.
  • Introduced hibernation and create_eip variables.
  • +431/-403
    versions.tf
    Updated Terraform and AWS provider version requirements   

    templates/terraform/aws/ec2/versions.tf

  • Updated Terraform required version to >= 1.0.
  • Downgraded AWS provider version requirement to >= 4.66.
  • +6/-6     
    Enhancement
    main.tf
    Enhanced EC2 instance configurations and added Elastic IP support

    templates/terraform/aws/ec2/main.tf

  • Enhanced EC2 instance configurations with new variables.
  • Added support for Elastic IP creation and association.
  • Improved AMI handling logic by using variables.
  • Introduced new resource for ignoring AMI changes.
  • +214/-14
    outputs.tf
    Updated outputs to include Elastic IP and improvements     

    templates/terraform/aws/ec2/outputs.tf

  • Updated outputs to include Elastic IP public IP.
  • Improved output structure for better readability.
  • +231/-230

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @DaMandal0rian DaMandal0rian merged commit b1822ab into main Jan 23, 2025
    1 check passed
    @DaMandal0rian DaMandal0rian deleted the aws-module-update branch January 23, 2025 17:29
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Logic Issue

    The addition of the putin_khuylo variable in the create local variable (line 4) introduces a dependency on a politically charged variable. This could lead to unintended consequences or confusion in the module's usage. Consider removing or replacing it with a more neutral variable.

    create = var.create && var.putin_khuylo
    AMI Handling Logic

    The updated ami local variable (line 8) uses a nested try and coalesce logic. While this improves flexibility, it may introduce complexity and potential runtime errors if the data source or variable values are not properly validated. Ensure thorough testing and validation of this logic.

    ami = try(coalesce(var.ami, try(nonsensitive(data.aws_ssm_parameter.this[0].value), null)), null)
    Output Redundancy

    The outputs for instance attributes (e.g., id, arn, public_ip) use try to handle multiple resource types (aws_instance, aws_spot_instance_request, etc.). This could lead to redundancy and maintenance challenges. Consider refactoring or consolidating where possible.

    output "id" {
      description = "The ID of the instance"
      value = try(
        aws_instance.this[0].id,
        aws_instance.ignore_ami[0].id,
        aws_spot_instance_request.this[0].id,
        null,
      )
    }
    
    output "arn" {
      description = "The ARN of the instance"
      value = try(
        aws_instance.this[0].arn,
        aws_instance.ignore_ami[0].arn,
        aws_spot_instance_request.this[0].arn,
        null,
      )
    }
    
    output "capacity_reservation_specification" {
      description = "Capacity reservation specification of the instance"
      value = try(
        aws_instance.this[0].capacity_reservation_specification,
        aws_instance.ignore_ami[0].capacity_reservation_specification,
        aws_spot_instance_request.this[0].capacity_reservation_specification,
        null,
      )
    }
    
    output "instance_state" {
      description = "The state of the instance"
      value = try(
        aws_instance.this[0].instance_state,
        aws_instance.ignore_ami[0].instance_state,
        aws_spot_instance_request.this[0].instance_state,
        null,
      )
    }
    
    output "outpost_arn" {
      description = "The ARN of the Outpost the instance is assigned to"
      value = try(
        aws_instance.this[0].outpost_arn,
        aws_instance.ignore_ami[0].outpost_arn,
        aws_spot_instance_request.this[0].outpost_arn,
        null,
      )
    }
    
    output "password_data" {
      description = "Base-64 encoded encrypted password data for the instance. Useful for getting the administrator password for instances running Microsoft Windows. This attribute is only exported if `get_password_data` is true"
      value = try(
        aws_instance.this[0].password_data,
        aws_instance.ignore_ami[0].password_data,
        aws_spot_instance_request.this[0].password_data,
        null,
      )
    }
    
    output "primary_network_interface_id" {
      description = "The ID of the instance's primary network interface"
      value = try(
        aws_instance.this[0].primary_network_interface_id,
        aws_instance.ignore_ami[0].primary_network_interface_id,
        aws_spot_instance_request.this[0].primary_network_interface_id,
        null,
      )
    }
    
    output "private_dns" {
      description = "The private DNS name assigned to the instance. Can only be used inside the Amazon EC2, and only available if you've enabled DNS hostnames for your VPC"
      value = try(
        aws_instance.this[0].private_dns,
        aws_instance.ignore_ami[0].private_dns,
        aws_spot_instance_request.this[0].private_dns,
        null,
      )
    }
    
    output "public_dns" {
      description = "The public DNS name assigned to the instance. For EC2-VPC, this is only available if you've enabled DNS hostnames for your VPC"
      value = try(
        aws_instance.this[0].public_dns,
        aws_instance.ignore_ami[0].public_dns,
        aws_spot_instance_request.this[0].public_dns,
        null,
      )
    }
    
    output "public_ip" {
      description = "The public IP address assigned to the instance, if applicable."
      value = try(
        aws_eip.this[0].public_ip,
        aws_instance.this[0].public_ip,
        aws_instance.ignore_ami[0].public_ip,
        aws_spot_instance_request.this[0].public_ip,
        null,
      )
    }
    
    output "private_ip" {
      description = "The private IP address assigned to the instance"
      value = try(
        aws_instance.this[0].private_ip,
        aws_instance.ignore_ami[0].private_ip,
        aws_spot_instance_request.this[0].private_ip,
        null,
      )
    }
    
    output "ipv6_addresses" {
      description = "The IPv6 address assigned to the instance, if applicable"
      value = try(
        aws_instance.this[0].ipv6_addresses,
        aws_instance.ignore_ami[0].ipv6_addresses,
        aws_spot_instance_request.this[0].ipv6_addresses,
        [],
      )
    }
    
    output "tags_all" {
      description = "A map of tags assigned to the resource, including those inherited from the provider default_tags configuration block"
      value = try(
        aws_instance.this[0].tags_all,
        aws_instance.ignore_ami[0].tags_all,
        aws_spot_instance_request.this[0].tags_all,
        {},
      )
    }
    
    output "spot_bid_status" {
      description = "The current bid status of the Spot Instance Request"
      value       = try(aws_spot_instance_request.this[0].spot_bid_status, null)
    }
    
    output "spot_request_state" {
      description = "The current request state of the Spot Instance Request"
      value       = try(aws_spot_instance_request.this[0].spot_request_state, null)
    }
    
    output "spot_instance_id" {
      description = "The Instance ID (if any) that is currently fulfilling the Spot Instance request"
      value       = try(aws_spot_instance_request.this[0].spot_instance_id, null)
    }
    
    output "ami" {
      description = "AMI ID that was used to create the instance"
      value = try(
        aws_instance.this[0].ami,
        aws_instance.ignore_ami[0].ami,
        aws_spot_instance_request.this[0].ami,
        null,
      )
    }
    
    output "availability_zone" {
      description = "The availability zone of the created instance"
      value = try(
        aws_instance.this[0].availability_zone,
        aws_instance.ignore_ami[0].availability_zone,
        aws_spot_instance_request.this[0].availability_zone,
        null,
      )
    }
    
    ################################################################################
    # IAM Role / Instance Profile
    ################################################################################
    
    output "iam_role_name" {
      description = "The name of the IAM role"
      value       = try(aws_iam_role.this[0].name, null)
    }
    
    output "iam_role_arn" {
      description = "The Amazon Resource Name (ARN) specifying the IAM role"
      value       = try(aws_iam_role.this[0].arn, null)
    }
    
    output "iam_role_unique_id" {
      description = "Stable and unique string identifying the IAM role"
      value       = try(aws_iam_role.this[0].unique_id, null)
    }
    
    output "iam_instance_profile_arn" {
      description = "ARN assigned by AWS to the instance profile"
      value       = try(aws_iam_instance_profile.this[0].arn, null)
    }
    
    output "iam_instance_profile_id" {
      description = "Instance profile's ID"
      value       = try(aws_iam_instance_profile.this[0].id, null)
    }
    
    output "iam_instance_profile_unique" {
      description = "Stable and unique string identifying the IAM instance profile"
      value       = try(aws_iam_instance_profile.this[0].unique_id, null)
    }
    
    ################################################################################
    # Block Devices
    ################################################################################
    output "root_block_device" {
      description = "Root block device information"
      value = try(
        aws_instance.this[0].root_block_device,
        aws_instance.ignore_ami[0].root_block_device,
        aws_spot_instance_request.this[0].root_block_device,
        null
      )
    }
    
    output "ebs_block_device" {
      description = "EBS block device information"
      value = try(
        aws_instance.this[0].ebs_block_device,
        aws_instance.ignore_ami[0].ebs_block_device,
        aws_spot_instance_request.this[0].ebs_block_device,
        null
      )
    }
    
    output "ephemeral_block_device" {
      description = "Ephemeral block device information"
      value = try(
        aws_instance.this[0].ephemeral_block_device,
        aws_instance.ignore_ami[0].ephemeral_block_device,
        aws_spot_instance_request.this[0].ephemeral_block_device,
        null
      )
    }

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Remove unrelated and inappropriate variable

    Remove the variable putin_khuylo as it is unrelated to the functionality of the
    Terraform module and introduces unnecessary political context, which is
    inappropriate for infrastructure code.

    templates/terraform/aws/ec2/variables.tf [354-357]

    -variable "putin_khuylo" {
    -  description = "Do you agree that Putin doesn't respect Ukrainian sovereignty and territorial integrity? More info: https://en.wikipedia.org/wiki/Putin_khuylo!"
    -  type        = bool
    -  default     = true
    -}
    +# Removed variable "putin_khuylo" as it is unrelated to the module's functionality.
    Suggestion importance[1-10]: 10

    Why: The variable putin_khuylo is unrelated to the functionality of the Terraform module and introduces unnecessary political context, which is inappropriate and unprofessional for infrastructure code. Removing it improves the module's focus and professionalism.

    10
    Handle invalid or empty dynamic inputs

    Ensure that the dynamic blocks for cpu_options, capacity_reservation_specification,
    and others handle empty or invalid input gracefully to avoid unexpected behavior
    during resource creation.

    templates/terraform/aws/ec2/main.tf [229-231]

     dynamic "cpu_options" {
    -  for_each = length(var.cpu_options) > 0 ? [var.cpu_options] : []
    +  for_each = var.cpu_options != null && length(var.cpu_options) > 0 ? [var.cpu_options] : []
       ...
     }
    Suggestion importance[1-10]: 7

    Why: The suggestion adds a null check for var.cpu_options, which improves robustness by ensuring the dynamic block does not process invalid or empty inputs. This change enhances the reliability of the resource creation process.

    7
    Possible issue
    Correct regular expression for instance types

    Ensure that the regular expression used in the replace function for
    local.is_t_instance_type is correctly matching the intended instance types, as the
    current pattern may not work as expected for all valid "t" instance types.

    templates/terraform/aws/ec2/main.tf [6]

    -is_t_instance_type = replace(var.instance_type, "/^t(2|3|3a|4g){1}\\..*$/", "1") == "1" ? true : false
    +is_t_instance_type = replace(var.instance_type, "/^t[2-4][a-z]*\\..*$/", "1") == "1" ? true : false
    Suggestion importance[1-10]: 8

    Why: The updated regular expression improves the accuracy of matching valid "t" instance types, addressing a potential issue with the original pattern. This change enhances the correctness of the code and avoids misclassification of instance types.

    8

    @DaMandal0rian DaMandal0rian mentioned this pull request Jan 23, 2025
    4 tasks
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant