nhurel / terraspec

Terraform unit test framework
Mozilla Public License 2.0
79 stars 7 forks source link

Add the ability to mock return values of resources such as kms keys. #4

Closed tomelliot16 closed 4 years ago

tomelliot16 commented 4 years ago

After playing around a bit somethings I have found that are missing. such as values returned from the provider once resource is created should probably be able to be mocked. Ex:

resource "aws_s3_bucket" "b" {
  bucket = "my-tf-test-bucket"
}

output "private_ip" {
  value = aws_s3_bucket.b.id
}

spec could be something like

assert "aws_s3_bucket" "c" {
  bucket = "my-tf-test-bucket"

  return {
    id = "arn:aws:s3::0000000000:my-tf-test-bucket"
  }
}

assert "output" "private_ip" {
    value = "arn:aws:s3::0000000000:my-tf-test-bucket"
}
nhurel commented 4 years ago

I understand the need of being able to test with specific resource attributes values but I don't think those values should be in the assert block. The assert block should only describe the expected result of applying the terraform config under certain circumstances (the input values and the mocked data source).

Do you have other example in mind where you'd like more control on the resource attributes ? That would help to design this feature

tomelliot16 commented 4 years ago

A real case I'm dealing with which came up with the thought is

resource "aws_kms_key" "key" {
  description = "KMS key 1"
}

resource "aws_iam_role_policy" "policy" {
  role = aws_iam_role.role.id
  policy = <<-EOF
{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Action": [
                "kms:Decrypt",
                "kms:DescribeKey",
                "kms:Encrypt",
                "kms:GenerateDataKey*",
                "kms:ReEncrypt*"
            ],
            "Resource": [
                "${aws_kms_key.key.id}"
            ]
        }
  EOF
}

resource "aws_iam_role" "role" {
  name = "role"

  assume_role_policy = <<-EOF
    {
      "Version": "2012-10-17",
      "Statement": [
        {
          "Effect": "Allow",
          "Principal": {
            "AWS": "arn:aws:iam::${data.aws_caller_identity.current.account_id}:root"
          },
          "Action": "sts:AssumeRole",
          "Condition": {
            "Bool": {
              "aws:MultiFactorAuthPresent": "true"
            }
          }
        }
      ]
    }
    EOF
}

Where I want to assert each one of the above however when ids are not set they are only set as nil

aws_iam_role_policy.policy.role : <nil> != at-least-something-unique-for-role-arn

I also was thinking of asserting policies.

tomelliot16 commented 4 years ago

Also I tend to disagree that is doesn't belong in the assert block because it seems the natural place when comparing it with rspec or other mocking systems expect(thing).to receive(:call_name).with(args).and_return(something) basically we are expecting the provider to fill in some data that will be used for other things. I'm not sure I understand why you don't want to add the return to the assert block.

tomelliot16 commented 4 years ago

perhaps this would be fine

assert "aws_s3_bucket" "c" {
  bucket = "my-tf-test-bucket"
}
mock_resource "aws_s3_bucket" "c" {
  return {
    id = "arn:aws:s3::0000000000:my-tf-test-bucket"
  }
}
tomelliot16 commented 4 years ago

if its language you could change it to

expect "aws_s3_bucket" "c" {
  bucket = "my-tf-test-bucket"
  return {
    id = "arn:aws:s3::0000000000:my-tf-test-bucket"
  }
}
nhurel commented 4 years ago

indeed, that's better with expect rather than assert. And it's easier to read (and write) with a single expect block rather than splitting it betwen an assert block and a mock one :+1:

nhurel commented 4 years ago

Hello @tomelliot16 I've started implemented support for mocking return values or resources in the mock-return branch. It's still work in progress but you can start testing it if you want

tomelliot16 commented 4 years ago

@nhurel I've gotten my team to work on this from Acquia https://github.com/acquia/terraspec/pull/1 I will open a PR to this repo

tomelliot16 commented 4 years ago

https://github.com/nhurel/terraspec/pull/10/files