terraform-linters / tflint-ruleset-aws

TFLint ruleset for terraform-provider-aws
Mozilla Public License 2.0
336 stars 72 forks source link

unnecessary check aws_vpc_ipam_pool_invalid_aws_service causing unnecessary errors #357

Closed drewmullen closed 2 years ago

drewmullen commented 2 years ago

My module is getting an error saying:

Failed to check ruleset; Failed to check aws_vpc_ipam_pool_invalid_aws_service rule: failed to eval an expression in modules/sub_pool/main.tf:24; Unsupported attribute: This object does not have an attribute named "aws_service".

Hoping someone can clarify what is going on with the error... we do provide the argument value as either user provider or null

That being said, this check is unnecessary since the service values are validated during terraform plan (no API interaction required)

wata727 commented 2 years ago

This seems to be an error that the var.pool_config.aws_service failed to evaluate because the aws_service attribute does not exist in var.pool_config. Check the value of var.pool_config.

drewmullen commented 2 years ago

its right here: https://github.com/aws-ia/terraform-aws-ipam/blob/4617b236e4ef2accf2616595e55652139dcbdc59/modules/sub_pool/variables.tf#L12

when using the way this works is it defaults the value to null if not provided by the user. not sure if the null value is creating the issue

Maybe im missing something but is the check even helpful since the validation is performed by the provider code itself?

wata727 commented 2 years ago

If the value is null, you got the "Attempt to get attribute from null value" error. You're probably passing a value of a different object type.

TFLint is different from validation by the provider. I believe there are many situations where TFLint is useful, but if you feel you don't need it, you can disable that rule.

drewmullen commented 2 years ago

I believe there are many situations where TFLint is useful

agreed! i use tflint for a lot of things such as ensuring version pinning and we block out several resource types that we dont want people to use

If i can just push a little bit more to try and understand... what is the use case for this check? the test only validates that youre passing an approved string value and it uses a static set.

To me this check in tflint is the equivalent of writing a variable validation for something like that... eg:

variable "aws_service" {
  validation {
    error_message = "Must only pass valid values. Valid values: \"ec2\"."
    condition = var.aws_service == "ec2" || var.aws_service == null
  }
}

We would never do that because

  1. its already covered by the provider
  2. its manually expressed in my codebase now

if youre going to keep the check in, i recommend you reference the allowed values enum function from the SDK instead of hardcoding the approved values in. These are planned to be expanded overtime.

Heres the function for the aws-sdk-go ec2 api.og file

// IpamPoolAwsService_Values returns all elements of the IpamPoolAwsService enum
func IpamPoolAwsService_Values() []string {
    return []string{
        IpamPoolAwsServiceEc2,
    }
}

Back to the error:

I think i figured out what is giving tflint heartburn and it may be a broader problem, not specific to aws_service. I do not pass the value in 1 of the module references. Terraform then views that value as null because because var.pool_config in the module has aws_service as optional() aka null

If i update that module reference to include pool_config = { ... aws_service = null} explicitly then tflint no longer errors.

I believe tflint is missing the implicit null from the optional() setting - thoughts?

wata727 commented 2 years ago

Some rules in TFLint are automatically generated from the AWS OpenAPI specification, and this rule is also a part of this. This is the same mechanism as the automatic generation of aws-sdk-go, and if there is an update, it will be updated automatically.

I believe tflint is missing the implicit null from the optional() setting - thoughts?

Umm, you're right. I have confirmed that using optional object attributes gives different results than Terraform.

terraform {
  experiments = [module_variable_optional_attrs]
}

variable "foo" {
  type = object({
    a = optional(string)
    b = optional(string)
  })
}

resource "aws_instance" "foo" {
  instance_type = var.foo.b
}
% TF_VAR_foo={a=1} tflint
Failed to check ruleset; Failed to check `aws_instance_previous_type` rule: failed to eval an expression in main.tf:13; Unsupported attribute: This object does not have an attribute named "b".
% TF_VAR_foo={a=1} terraform console
╷
│ Warning: Experimental feature "module_variable_optional_attrs" is active
│
│   on main.tf line 2, in terraform:
│    2:   experiments = [module_variable_optional_attrs]
│
│ Experimental features are subject to breaking changes in future minor or patch releases, based on feedback.
│
│ If you have feedback on the design of this feature, please open a GitHub issue to discuss it.
╵

> var.foo.b
tostring(null)
wata727 commented 2 years ago

https://github.com/terraform-linters/tflint/issues/1436

drewmullen commented 2 years ago

it will be updated automatically.

Good to know. The check itself is not really a problem now that I know it's a broader bug. We can close this out. Thanks for helping investigated

drewmullen commented 2 years ago

tflint v39.1 + 0.15 aws-ruleset solved this issue for me. thank you!