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.

Sign up for GitHub

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: MNG cluster datasource errors #1639

Merged
merged 1 commit into from
Oct 14, 2021
Merged

fix: MNG cluster datasource errors #1639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion modules/node_groups/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,13 +84,14 @@ No modules.
|------|------|
| [aws_eks_node_group.workers](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/eks_node_group) | resource |
| [aws_launch_template.workers](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/launch_template) | resource |
| [aws_eks_cluster.default](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/eks_cluster) | data source |
| [cloudinit_config.workers_userdata](https://registry.terraform.io/providers/hashicorp/cloudinit/latest/docs/data-sources/config) | data source |

## Inputs

| Name | Description | Type | Default | Required |
|------|-------------|------|---------|:--------:|
| <a name="input_cluster_auth_base64"></a> [cluster\_auth\_base64](#input\_cluster\_auth\_base64) | Base64 encoded CA of parent cluster | `string` | `""` | no |
| <a name="input_cluster_endpoint"></a> [cluster\_endpoint](#input\_cluster\_endpoint) | Endpoint of parent cluster | `string` | `""` | no |
| <a name="input_cluster_name"></a> [cluster\_name](#input\_cluster\_name) | Name of parent cluster | `string` | `""` | no |
| <a name="input_create_eks"></a> [create\_eks](#input\_create\_eks) | Controls if EKS resources should be created (it affects almost all resources) | `bool` | `true` | no |
| <a name="input_default_iam_role_arn"></a> [default\_iam\_role\_arn](#input\_default\_iam\_role\_arn) | ARN of the default IAM worker role to use if one is not specified in `var.node_groups` or `var.node_groups_defaults` | `string` | `""` | no |
Expand Down
4 changes: 2 additions & 2 deletions modules/node_groups/launch_template.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ data "cloudinit_config" "workers_userdata" {
ami_id = lookup(each.value, "ami_id", "")
ami_is_eks_optimized = each.value["ami_is_eks_optimized"]
cluster_name = var.cluster_name
cluster_endpoint = data.aws_eks_cluster.default[0].endpoint
cluster_ca = data.aws_eks_cluster.default[0].certificate_authority[0].data
cluster_endpoint = var.cluster_endpoint
cluster_auth_base64 = var.cluster_auth_base64
capacity_type = lookup(each.value, "capacity_type", "ON_DEMAND")
append_labels = length(lookup(each.value, "k8s_labels", {})) > 0 ? ",${join(",", [for k, v in lookup(each.value, "k8s_labels", {}) : "${k}=${v}"])}" : ""
}
Expand Down
6 changes: 0 additions & 6 deletions modules/node_groups/locals.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,3 @@
data "aws_eks_cluster" "default" {
count = var.create_eks ? 1 : 0

name = var.cluster_name
}

locals {
# Merge defaults and per-group values to make code cleaner
node_groups_expanded = { for k, v in var.node_groups : k => merge(
Expand Down
2 changes: 1 addition & 1 deletion modules/node_groups/templates/userdata.sh.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ sed -i '/^KUBELET_EXTRA_ARGS=/a KUBELET_EXTRA_ARGS+=" ${kubelet_extra_args}"' /e

# Set variables for custom AMI
API_SERVER_URL=${cluster_endpoint}
B64_CLUSTER_CA=${cluster_ca}
B64_CLUSTER_CA=${cluster_auth_base64}
KUBELET_EXTRA_ARGS='--node-labels=eks.amazonaws.com/nodegroup-image=${ami_id},eks.amazonaws.com/capacityType=${capacity_type}${append_labels} ${kubelet_extra_args}'
%{endif ~}

Copy link
Contributor

Choose a reason for hiding this comment

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

if we using a EKS optimized AMI then we can just rely on bootstrap.sh key and dont require to pass

--apiserver-endpoint "$${API_SERVER_URL}" --b64-cluster-ca "$${B64_CLUSTER_CA}"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is designed to set the variables once for all non-merging userdata. It also replicates the output of the merged userdata for the --apiserver-endpoint & --b64-cluster-ca arguments.

Expand Down
12 changes: 12 additions & 0 deletions modules/node_groups/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,18 @@ variable "cluster_name" {
default = ""
}

variable "cluster_endpoint" {
description = "Endpoint of parent cluster"
type = string
default = ""
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we cannot leave this empty as there can be risk that we will pass empty values to template what in general will cause that bootstrap script wil lbe crashing

Copy link
Contributor

@daroga0002 daroga0002 Oct 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

probably this should be somehow conditional input which is required only when we dont use eks optimized AMI (as bootstrap.sh can download it by own).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As above, the code is correctly optimised for the 80+% case where this will be set, if it's not set bootstrap.sh will fall back to fetching it.

Copy link
Contributor

Choose a reason for hiding this comment

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

checked bootstrap.sh and seems it is able to cover empty variable scenario


variable "cluster_auth_base64" {
description = "Base64 encoded CA of parent cluster"
type = string
default = ""
Copy link
Contributor

Choose a reason for hiding this comment

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

we cannot leave this empty as there can be risk that we will pass empty values to template what in general will cause that bootstrap script wil lbe crashing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will only ever be empty if the sub module is called directly, is this supported? The code is fine even if it's empty as bootstrap.sh will fall back to fetching this. If we set it then there is no cost at node start (this can add up on large clusters), but if it's not set every time anode starts it needs to query this information.

Copy link
Contributor

Choose a reason for hiding this comment

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

nope, it is not supported (as of now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the cluster_auth_base64 & cluster_endpoint variables will always be set unless create_eks = false, in which case they're not going to be used. This is the same logic as is used for cluster_name which is also optional (and unlike these variables would break bootstrap.sh if not set).

}

variable "default_iam_role_arn" {
description = "ARN of the default IAM worker role to use if one is not specified in `var.node_groups` or `var.node_groups_defaults`"
type = string
Expand Down
5 changes: 4 additions & 1 deletion node_groups.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ module "node_groups" {

create_eks = var.create_eks

cluster_name = local.cluster_name
cluster_name = local.cluster_name
cluster_endpoint = local.cluster_endpoint
cluster_auth_base64 = local.cluster_auth_base64

default_iam_role_arn = coalescelist(aws_iam_role.workers[*].arn, [""])[0]
ebs_optimized_not_supported = local.ebs_optimized_not_supported
workers_group_defaults = local.workers_group_defaults
Expand Down