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

Terminated instances included in aws_ec2_instance(s) results #983

Open
mwiczynski-cartera opened this issue Nov 28, 2023 · 2 comments
Open

Comments

@mwiczynski-cartera
Copy link

When an ec2 instance is terminated it can still appear in results/response for describing ec2 instances.

Describe the problem

When an ec2 instance is terminated it can still appear in results/response for describing ec2 instances. Eventually AWS removes it. A terminated instance passes the it { should exist }. This is not correct. it can also lead to problems when you have more than one instance with same name. One is terminated and other is non-terminated state(running, stopped, etc). The aws_ec2_instance(name: 'foo') resource will fail because more than once instance matching name is returned. The aws_ec2_instances resource does not allow filter by instance state. The resources should filter out terminated instances by default or give ability to do so. Not sure if anyone would ever want them to be included given how transient the terminated state is.

Possible Solution

I suggest filtering out terminated by default. I suppose could also add instance state as column in FilterTable?

Change fetch_data

to filter out terminated.

 def fetch_data
    instance_rows = []
    filters = [
      {
        name: 'instance-state-name',
        values: [
          'pending',
          'running',
          'shutting-down',
          'stopping',
          'stopped'
        ]
      }]
    options = {filters: filters}

    loop do
      catch_aws_errors do
        @api_response = @aws.compute_client.describe_instances(options)
      end
      return [] if !@api_response || @api_response.empty?
      @api_response.reservations.each do |res|
        res.instances.each do |instance|
          instance_tags = map_tags(instance.tags)
          instance_rows += [{
            instance_id: instance.instance_id,
            vpc_id: instance.vpc_id,
            subnet_id: instance.subnet_id,
            instance_type: instance.instance_type,
            tags: instance_tags,
            name: instance_tags["Name"],
            iam_profile: instance.iam_instance_profile&.arn&.split("/")&.last,
          }]
        end
      end
      break unless @api_response.next_token
      options = { next_token: @api_response.next_token }
    end
    @table = instance_rows
  end

This might not be ideal but I like idea of filtering out terminated instance here

def initialize(opts = {})

  def initialize(opts = {})
    opts = { instance_id: opts } if opts.is_a?(String)
    super(opts)
    validate_parameters(require_any_of: %i(instance_id name))

    state_filter =  {
      name: 'instance-state-name',
      values: [
        'pending',
        'running',
        'shutting-down',
        'stopping',
        'stopped'
      ]
    }

    if opts[:instance_id] && !opts[:instance_id].empty? # Use instance_id, if provided
      if !opts[:instance_id].is_a?(String) || opts[:instance_id] !~ /(^i-[0-9a-f]{8})|(^i-[0-9a-f]{17})$/
        raise ArgumentError, "#{@__resource_name__}: `instance_id` must be a string in the format of 'i-' followed by 8 or 17 hexadecimal characters."
      end
      @display_name = opts[:instance_id]
      instance_arguments = { instance_ids: [opts[:instance_id]], filters: [state_filter] }
    elsif opts[:name] && !opts[:name].empty? # Otherwise use name, if provided
      @display_name = opts[:name]
      instance_arguments = { filters: [{ name: "tag:Name", values: [opts[:name]] }, state_filter] }
    else
      raise ArgumentError, "#{@__resource_name__}: either instance_id or name must be provided."
    end
@aaronlippold
Copy link

If you fork the repo and push a small PR with the update that would be the fastest way to see this improvement moved into the resource pack.

@clintoncwolfe
Copy link
Contributor

I think this is a great suggestion! This would indeed make a great PR.

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

No branches or pull requests

3 participants