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

don't use name_prefix for aws_iam_role name, enabled by the iam_role_use_name_prefix attribute in the self_managed_node_group module. #22

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

clay-moss
Copy link
Contributor

@clay-moss clay-moss commented Dec 8, 2023

an aws_iam_role resource is created by the self_managed_node_group module.

the name_prefix is either set to the iam_role_name or name(+ "-node-group") attributes if the former is not configured.

a name_prefix will only be used for the IAM role name created by the self_managed_node_group module if iam_role_use_name_prefix is true

if a name_prefix is used, then a unique ID is generated and appended to the name_prefix.

the name for an aws_iam_role resource is set here in resourceRoleCreate().

its generated by a call to the Name() function, passing in the name and name_prefix attributes of the aws_iam_role resource.

the Name() function returns the Generate() method called on the results of NewNameGenerator() which is passed the function arguments WithConfiguredName(name) and WithConfiguredPrefix(namePrefix). this sets configuredName and configuredPrefix fields for a nameGenerator object.

the Generate() method which ultimately creates the name that will be used for the aws_iam_role will return the name if configured, otherwise it returns the name_prefix with a unique 26 digit ID appended, created by PrefixedUniqueId().

this restricts the name_prefix length to 38 characters, which is the defined maximum length of a role name set by constant roleNameMaxLen (64) minus the unique ID suffix length (26) i.e. the constant roleNamePrefixLen

this change is intended to resolve the below terraform error commonly encountered by CuTE users:

╷
│ Error: expected length of name_prefix to be in the range (1 - 38), got 123-example-test-egressgw-1-node-group-
│
│   with module.eks.module.main.module.self_managed_node_group["egressgw_1"].aws_iam_role.this[0],
│   on .terraform/modules/eks.main/modules/self-managed-node-group/main.tf line 728, in resource "aws_iam_role" "this":
│  728:   name_prefix = var.iam_role_use_name_prefix ? "${local.iam_role_name}-" : null
│
╵
make: *** [Makefile:8: apply] Error 1

overall this will allow a longer name to be used for clusters and/or node groups in CuTE configurations, about 53 characters total since "-node-group" is always appended to the node group IAM role name.

…role_use_name_prefix` attribute (default `true`) in the `self_managed_node_group` module.

an `aws_iam_role` resource is (created)[https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/modules/self-managed-node-group/main.tf#L735] by the `self_managed_node_group` module.

the `name_prefix` is either (set)[https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/modules/self-managed-node-group/main.tf#L716] to the `iam_role_name` or `name`(+ "-node-group") attributes if the former is not configured.

a `name_prefix` will only be used for the IAM role name created by the `self_managed_node_group` module if `iam_role_use_name_prefix` is (true)[https://github.com/terraform-aws-modules/terraform-aws-eks/blob/master/modules/self-managed-node-group/main.tf#L739]

if a (`name_prefix`)[https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role#name_prefix] is used, then a unique ID is generated and appended to the `name_prefix`.

the name for an `aws_iam_role` resource is set (here)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/service/iam/role.go#L193] in `resourceRoleCreate()`.

its generated by a call to the (`Name()`)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/create/naming.go#L14] function, passing in the `name` and `name_prefix` attributes of the `aws_iam_role` resource.

the `Name()` function (returns)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/create/naming.go#L15] the (`Generate()`)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/create/naming.go#L108] method called on the results of (`NewNameGenerator()`)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/create/naming.go#L97] which is passed the function arguments (`WithConfiguredName(name)`)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/create/naming.go#L65] and (`WithConfiguredPrefix(namePrefix)`)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/create/naming.go#L74]. this sets `configuredName` and `configuredPrefix` fields for a (`nameGenerator`)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/create/naming.go#L52] object.

the `Generate()` method which ultimately creates the name that will be used for the `aws_iam_role` will (return)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/create/naming.go#L109C11-L110] the `name` if configured, otherwise it (returns)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/create/naming.go#L117] the `name_prefix` with a unique 26 digit ID appended by (PrefixedUniqueId())[https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/id/id.go].

this restricts the `name_prefix` length to 38 characters, which is the defined maximum length of a role name set by constant (`roleNameMaxLen`)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/service/iam/role.go#L38] (64) minus the unique ID suffix length (26) i.e. the constant (`roleNamePrefixLen`)[https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/service/iam/role.go#L39]

this change is intended to resolve the below terraform error commonly encountered by CuTE users:

```
╷
│ Error: expected length of name_prefix to be in the range (1 - 38), got 123-example-test-egressgw-1-node-group-
│
│   with module.eks.module.main.module.self_managed_node_group["egressgw_1"].aws_iam_role.this[0],
│   on .terraform/modules/eks.main/modules/self-managed-node-group/main.tf line 728, in resource "aws_iam_role" "this":
│  728:   name_prefix = var.iam_role_use_name_prefix ? "${local.iam_role_name}-" : null
│
╵
make: *** [Makefile:8: apply] Error 1
```

overall this will allow a longer name to be used for clusters and/or node groups in CuTE configurations, about 53 characters total since "-node-group" is always appended to the node group IAM role name.
@clay-moss clay-moss changed the title don't use name_prefix for aws_iam_role name, enabled by the iam_role_use_name_prefix attribute (default true) in the self_managed_node_group module. don't use name_prefix for aws_iam_role name, enabled by the iam_role_use_name_prefix attribute in the self_managed_node_group module. Dec 8, 2023
@clay-moss clay-moss requested review from chancez and f1ko December 8, 2023 03:04
eks.tf Outdated
@@ -130,6 +130,7 @@ module "main" {
pre_bootstrap_user_data = g.pre_bootstrap_user_data // The pre-bootstrap user data to use for worker nodes.
post_bootstrap_user_data = g.post_bootstrap_user_data // The pre-bootstrap user data to use for worker nodes.
iam_role_additional_policies = g.iam_role_additional_policies
iam_role_use_name_prefix = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of universally setting this to false, I suggest having it configurable by the self_managed_node_groups var and you can set a default if it's unset (null).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a commit with suggested changes @chancez

clay-moss and others added 2 commits December 8, 2023 11:47
…le use of [Optional Object Type Attributes](https://developer.hashicorp.com/terraform/language/v1.3.x/expressions/type-constraints#optional-object-type-attributes) with default values

- _variables.tf_: make `iam_role_use_name_prefix` an attribute for the `self_managed_node_groups` object variable with type `bool` and default value `false`
- _eks.tf_: set `iam_role_use_name_prefix` variable for node groups defined by the `self_managed_node_groups` map in eks cluster module equal to the `iam_role_use_name_prefix` attribute value in the `self_managed_node_groups` object variable
@clay-moss clay-moss requested a review from chancez December 8, 2023 18:59
variables.tf Outdated Show resolved Hide resolved
@f1ko f1ko requested a review from chancez December 11, 2023 17:19
@clay-moss clay-moss merged commit 377ba78 into main Dec 14, 2023
@clay-moss clay-moss deleted the pr/cmoss/dont-use-iam-role-name-prefix branch December 14, 2023 21:55
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.

3 participants