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

Talos k8s with AWS CCM support #173

Open
wants to merge 38 commits into
base: main
Choose a base branch
from
Open

Conversation

erikvveen
Copy link

No description provided.

Copy link
Collaborator

@PhilipSchmid PhilipSchmid left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! I commented on a few things to address. Once done, please also run terraform fmt, make docs, and squash all commits into a single one with a meaningful message.

},
controllerManager = {
extraArgs = {
allocate-node-cidrs = var.allocate_node_cidrs
cloud-provider = "external"
leader-elect = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
leader-elect = true

This should already be the default, according to https://kubernetes.io/docs/reference/command-line-tools-reference/kube-controller-manager/.

module.elb_k8s_elb.elb_dns_name
]
extraArgs = {
cloud-provider = "external"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
cloud-provider = "external"

kube-apiserver doesn't have a cloud-provider extraArg, according to https://kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/.

},
certSANs = [
module.elb_k8s_elb.elb_dns_name,
]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please fix the formatting

}
}
}
}

# Used to configure Cilium Kube-Proxy replacement

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change




config_patches_common = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

Restore indentation

@@ -32,7 +34,9 @@ module "talos_worker_group" {
subnet_id = element(data.aws_subnets.public.ids, tonumber(trimprefix(each.key, "${each.value.name}.")))
associate_public_ip_address = true
tags = merge(each.value.tags, var.tags, local.cluster_required_tags)

metadata_options = var.metadata_options
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this change, as it's already covered in your other PR: #171

@@ -45,18 +49,32 @@ module "talos_worker_group" {
resource "talos_machine_secrets" "this" {}

data "talos_machine_configuration" "controlplane" {
count = var.controlplane_count
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we iterate via the same method (for_each) as for the worker_group talos_machine_configuration?

}

resource "talos_machine_configuration_apply" "controlplane" {
count = var.controlplane_count

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove spaces from all empty lines

@@ -106,6 +137,7 @@ data "talos_client_configuration" "this" {
cluster_name = var.cluster_name
client_configuration = talos_machine_secrets.this.client_configuration
endpoints = module.talos_control_plane_nodes.*.public_ip
nodes = module.talos_control_plane_nodes.*.private_ip
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this come from? I don't see it in https://registry.terraform.io/providers/siderolabs/talos/0.7.0/docs/resources/machine_bootstrap. Same for endpoints (which was already there).

]
extraArgs = {
cloud-provider = "external"
enable-admission-plugins = "MutatingAdmissionWebhook,ValidatingAdmissionWebhook, ServiceAccount"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
enable-admission-plugins = "MutatingAdmissionWebhook,ValidatingAdmissionWebhook, ServiceAccount"
enable-admission-plugins = "MutatingAdmissionWebhook,ValidatingAdmissionWebhook,ServiceAccount"

& please make that configurable.

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.

2 participants