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

fix: Add map license_specifications into list #2799

Closed
wants to merge 4 commits into from

Conversation

palpaga
Copy link
Contributor

@palpaga palpaga commented Oct 31, 2023

Description

Following those PRs #2798 #2796

Motivation and Context

In the first PR, #2796, changing the type to map(string) of the variable was omitted, causing this issue #2797.

However, defining license_specifications as a type list doesn't seem to be the best approach. That's why, in order to keep it as a map, the dynamic block should be declared as follows:

  dynamic "license_specification" {
    for_each = length(var.license_specifications) > 0 ? [var.license_specifications] : []
    content {
      license_configuration_arn = license_specification.value.license_configuration_arn
    }
  }

Because currently, this error is returned while using 19.17.4 module

│ Error: Unsupported attribute
│ 
│   on .terraform/modules/self_managed_node_group/modules/self-managed-node-group/main.tf line 288, in resource "aws_launch_template" "this":
│  288:       license_configuration_arn = license_specification.value.license_configuration_arn
│     ├────────────────
│     │ license_specification.value is "arn:aws:license-manager:us-east-1:XXXXXX:license-configuration:lic-XXXXXXXXXX"
│ 
│ Can't access attributes on a primitive-typed value (string).

Breaking Changes

I do not see any potential breaking changes.

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

This modification solves the issue of using license_specifications while maintaining the map type into a list from dynamic block. I have tested by declaring a module from scratch with and without a license_specifications, and this time I don't have any issues in the case where license_specifications is used.

I didn't reinvent anything. I based the changes on the https://github.com/terraform-aws-modules/terraform-aws-autoscaling/blob/master/main.tf module, which doesn't have any issues with the license_specifications.

@palpaga palpaga changed the title fix: Add map license_specification into list fix: Add map license_specifications into list Oct 31, 2023
@bryantbiggs
Copy link
Member

can you provide a reproduction - what are you trying to pass and how is it being passed

@palpaga
Copy link
Contributor Author

palpaga commented Nov 2, 2023

By declaring a module as follows:

module "self_managed_node_group" {
  source = "terraform-aws-modules/eks/aws//modules/self-managed-node-group"
  version = "19.17.X"

  name = local.eks_name
  cluster_name = module.eks.cluster_name
  cluster_version = var.eks_version
  cluster_endpoint = module.eks.cluster_endpoint
  cluster_auth_base64 = module.eks.cluster_certificate_authority_data

  ....
  placement = {
    tenancy                 = "host"
    host_resource_group_arn = "arn:aws:resource-groups:us-east-1:XXXXXX:XXXXXXXXX/XXXXXXXXX"
  }

  license_specifications = {
    license_configuration_arn = "arn:aws:license-manager:us-east-1:XXXXXX:license-configuration:lic-XXXXXXXXX"
  }
}

I'm getting well:

 # module.self_managed_node_group.aws_launch_template.this[0] will be updated in-place
 ~ resource "aws_launch_template" "this" {
     ~ default_version         = 3 -> (known after apply)
       id                      = "lt-XXXXXXX"
     ~ latest_version          = 3 -> (known after apply)
       name                    = "XXXXXXXXX"
       tags                    = {}
       # (11 unchanged attributes hidden)

      + license_specification {
          + license_configuration_arn = "arn:aws:license-manager:us-east-1:XXXXXXXXX:license-configuration:lic-XXXXXXXXX"
        }

      ~ placement {
          + host_resource_group_arn = "arn:aws:resource-groups:us-east-1:XXXXXXXXX:group/XXXXXXXXX"
          ~ tenancy                 = "default" -> "host"
            # (1 unchanged attribute hidden)
        }

       # (8 unchanged blocks hidden)
   }

If license_specifications is unset, you won't encounter the "Inconsistent conditional result types" error like #2797.
Also, I want to emphasize that I used the https://github.com/terraform-aws-modules/terraform-aws-autoscaling/blob/master/main.tf#L246C2-L246C2 module as a reference, and it doesn't have any problems with license_specifications.

@bryantbiggs
Copy link
Member

@Lamaspanzer can you try the following syntax:

module "self_managed_node_group" {
  source = "terraform-aws-modules/eks/aws//modules/self-managed-node-group"
  version = "19.17.X"

  name = local.eks_name
  cluster_name = module.eks.cluster_name
  cluster_version = var.eks_version
  cluster_endpoint = module.eks.cluster_endpoint
  cluster_auth_base64 = module.eks.cluster_certificate_authority_data

  ....
  placement = {
    tenancy                 = "host"
    host_resource_group_arn = "arn:aws:resource-groups:us-east-1:XXXXXX:XXXXXXXXX/XXXXXXXXX"
  }

  license_specifications = {
    one = { # Wrap in another map
      license_configuration_arn = "arn:aws:license-manager:us-east-1:XXXXXX:license-configuration:lic-XXXXXXXXX"
    }
  }
}

The license_specification supports n-number of license configuration assocaitions https://github.com/hashicorp/terraform-provider-aws/blob/39579f095ed0a3a6ffbd4099b9ef0737b5950270/internal/service/ec2/ec2_launch_template.go#L635-L647 - if we go with the changes here, we are restricting it to only one license configuration can be attached.

@palpaga
Copy link
Contributor Author

palpaga commented Nov 27, 2023

Yes, you right! So no change needed finally. Thanks you, I close the PR

@palpaga palpaga closed this Nov 27, 2023
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants