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

feat: Add add metadata options to managed node groups launch template #1562

Closed

Conversation

xr0n1ck
Copy link
Contributor

@xr0n1ck xr0n1ck commented Sep 2, 2021

PR o'clock

Description

Please explain the changes you made here and link to any relevant issues.

Checklist

@@ -63,6 +63,12 @@ resource "aws_launch_template" "workers" {
])
}

metadata_options {
http_endpoint = lookup(each.value, "metadata_http_endpoint", "enabled")
Copy link
Member

Choose a reason for hiding this comment

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

Let's generate metadata_options using dynamic and create it only if one of 3 inputs are present. Exactly as here - https://github.com/terraform-aws-modules/terraform-aws-autoscaling/blob/master/main.tf#L92-L99

@daroga0002
Copy link
Contributor

looks that it is duplicating #1485

1 similar comment
@daroga0002
Copy link
Contributor

looks that it is duplicating #1485

@xr0n1ck
Copy link
Contributor Author

xr0n1ck commented Sep 2, 2021

@antonbabenko
Should I make changes to #1485
or can I create separate pr with dynamic metadata_options?

@antonbabenko
Copy link
Member

@xr0n1ck If you can verify it and apply the changes to that PR, it would be great.

@xr0n1ck
Copy link
Contributor Author

xr0n1ck commented Sep 2, 2021

@antonbabenko
As far as I have no write permissions to that repo, I can create pr to that branch and wait to approval.
It will be easier and faster to create new one, since dynamic metadata little bit different

@antonbabenko
Copy link
Member

I understand, let's make a PR based on that one into this repository that will close #1485

@maxbrunet
Copy link
Contributor

Hello! I have rebased #1485

@bryantbiggs
Copy link
Member

hi @maxbrunet / @xr0n1ck - whichever PR we go with, can we just copy what we have for the autoscaling group module https://github.com/terraform-aws-modules/terraform-aws-autoscaling/blob/d4b7da041ef781604726953234b5bcd494cc5908/main.tf#L92-L99

you should be able to copy that block, and its variable and paste them in your PRs

@maxbrunet
Copy link
Contributor

@bryantbiggs I copied what was already done in this module, I was thinking to be consistent with that:

metadata_options {
http_endpoint = lookup(
var.worker_groups_launch_template[count.index],
"metadata_http_endpoint",
local.workers_group_defaults["metadata_http_endpoint"],
)
http_tokens = lookup(
var.worker_groups_launch_template[count.index],
"metadata_http_tokens",
local.workers_group_defaults["metadata_http_tokens"],
)
http_put_response_hop_limit = lookup(
var.worker_groups_launch_template[count.index],
"metadata_http_put_response_hop_limit",
local.workers_group_defaults["metadata_http_put_response_hop_limit"],
)
}

metadata_options {
http_endpoint = lookup(
var.worker_groups[count.index],
"metadata_http_endpoint",
local.workers_group_defaults["metadata_http_endpoint"],
)
http_tokens = lookup(
var.worker_groups[count.index],
"metadata_http_tokens",
local.workers_group_defaults["metadata_http_tokens"],
)
http_put_response_hop_limit = lookup(
var.worker_groups[count.index],
"metadata_http_put_response_hop_limit",
local.workers_group_defaults["metadata_http_put_response_hop_limit"],
)
}

Also in the autoscaling module you shared, defaults being null adds to the inconsistency, and this errors if http_endpoint is not specified:

http_endpoint               = lookup(metadata_options.value, "http_endpoint", null)

http_endpoint must be one of enabled or disabled.

@bryantbiggs
Copy link
Member

@maxbrunet
Copy link
Contributor

I apply the defaults here https://github.com/terraform-aws-modules/terraform-aws-eks/pull/1485/files#diff-98193ca5efaaf91be09ba7fc2c847ee1d2df56aa4e9d3691b7fa4e5fffc318edR29-R31

So in code, it is consistent with how the node_groups module applies defaults, and the behaviour is consistent with the self-managed workers

@github-actions
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 Nov 13, 2022
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.

5 participants