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: #957 Use aws eks get-token instead of aws-iam-authenticator #1235

Closed
wants to merge 2 commits into from

Conversation

schollii
Copy link

@schollii schollii commented Feb 11, 2021

PR o'clock

Description

Updated local.tf to use aws eks get-token, and updated the variables.tf default/docs for the two vars used

Checklist

  • CI tests are passing: the code formatting is correct in my test using terraform 0.14.6, maybe version issue? the tflint does not seem to be in a file I changed
  • README.md has been updated after any changes to variables and outputs: the docs folder does not mention authenticator.

@schollii schollii changed the title Use aws eks get-token instead of aws-iam-authenticator fix: #957 Use aws eks get-token instead of aws-iam-authenticator Feb 11, 2021
@nathanblair
Copy link

Is there any feedback from maintainers on this? The corresponding issue has been closed as "stale".

Would the maintainers rather the branch be updated so it passes CI? I am willing to help with that so this external aws-iam-authenticator dependency can be removed.

@Yasumoto
Copy link

Yasumoto commented Jul 2, 2021

I'm also happy to pitch in to get this over the finish line if that's helpful! I know y'all maintainers are the ones that'll need to do most of the work and are surely busy!

It'd be awesome to only depend on the aws CLI tools 🙏🏼 Thanks for all the hard work on this, folks!

@Yasumoto
Copy link

Yasumoto commented Jul 6, 2021

I merged the main branch against this PR at https://github.com/schollii/terraform-aws-eks/pull/1

@schollii if you have time, any chance you could take a look and refresh this PR to make it easier to land this? 🙏🏼

@schollii
Copy link
Author

schollii commented Jul 6, 2021

@Yasumoto I fixed the conflict, if that's what you mean.

@Yasumoto
Copy link

Yasumoto commented Jul 6, 2021

Exactly, thanks so much @schollii !

Copy link
Contributor

@lisfo4ka lisfo4ka left a comment

Choose a reason for hiding this comment

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

@schollii @Yasumoto folks, thanks a lot for this really useful change!
I've tested it and it works absolutely great. Please, provide minor fixes according to the comments below and I assume we can merge it.

README.md Outdated
@@ -232,7 +232,7 @@ Apache 2 Licensed. See [LICENSE](https://github.com/terraform-aws-modules/terraf
| <a name="input_cluster_endpoint_private_access_cidrs"></a> [cluster\_endpoint\_private\_access\_cidrs](#input\_cluster\_endpoint\_private\_access\_cidrs) | List of CIDR blocks which can access the Amazon EKS private API server endpoint. To use this `cluster_endpoint_private_access` and `cluster_create_endpoint_private_access_sg_rule` must be set to `true`. | `list(string)` | `null` | no |
| <a name="input_cluster_endpoint_private_access_sg"></a> [cluster\_endpoint\_private\_access\_sg](#input\_cluster\_endpoint\_private\_access\_sg) | List of security group IDs which can access the Amazon EKS private API server endpoint. To use this `cluster_endpoint_private_access` and `cluster_create_endpoint_private_access_sg_rule` must be set to `true`. | `list(string)` | `null` | no |
| <a name="input_cluster_endpoint_public_access"></a> [cluster\_endpoint\_public\_access](#input\_cluster\_endpoint\_public\_access) | Indicates whether or not the Amazon EKS public API server endpoint is enabled. When it's set to `false` ensure to have a proper private access with `cluster_endpoint_private_access = true`. | `bool` | `true` | no |
| <a name="input_cluster_endpoint_public_access_cidrs"></a> [cluster\_endpoint\_public\_access\_cidrs](#input\_cluster\_endpoint\_public\_access\_cidrs) | List of CIDR blocks which can access the Amazon EKS public API server endpoint. | `list(string)` | <pre>[<br> "0.0.0.0/0"<br>]</pre> | no |
| <a name="input_cluster_endpoint_public_access_cidrs"></a> [cluster\_endpoint\_public\_access\_cidrs](#input\_cluster\_endpoint\_public\_access\_cidrs) | List of CIDR blocks which can access the Amazon EKS public API server endpoint. You likely need to set `cluster_endpoint_private_access` to true so the nodes can join the cluster (see _API server endpoint access options_ table in [EKS User Guide](https://docs.aws.amazon.com/eks/latest/userguide/cluster-endpoint.html) for details). | `list(string)` | <pre>[<br> "0.0.0.0/0"<br>]</pre> | no |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this comment makes sense in case cluster_endpoint_public_access=true, is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

it doesnt, you can use all 3 scenarios which have behaviour as in screen:
image

Copy link
Author

@schollii schollii Sep 17, 2021

Choose a reason for hiding this comment

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

@lisfo4ka @daroga0002 My change is correct: If you set public CIDR but private is false, then worker nodes have no choice but to access API server through the public access point, ie internet, but their IP will be prevented (since you set the IP filter and you don't have a way to determine WAN IP of nodes). So they will never join the cluster. If you set IP filter, you HAVE to have private = true so they can directly access the API server from within the VPC.

Copy link
Contributor

Choose a reason for hiding this comment

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

what ip filter you are talking?

local.tf Outdated
@@ -1,3 +1,6 @@
data "aws_region" "this" {
Copy link
Contributor

Choose a reason for hiding this comment

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

let's put this in a single line
data "aws_region" "current" {}

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

local.tf Outdated
"get-token",
"--cluster-name",
var.cluster_name
]
kubeconfig = var.create_eks ? templatefile("${path.module}/templates/kubeconfig.tpl", {
Copy link
Contributor

Choose a reason for hiding this comment

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

please, add empty line

Copy link
Author

Choose a reason for hiding this comment

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

Fixed

@schollii
Copy link
Author

schollii commented Sep 17, 2021

@lisfo4ka fixes made, and one fix ignored because I gave explanation of why it would be wrong to remove that comment; but if you prefer, I can create a separate PR just for that one, since it has nothing to do with the primary issue of this PR, it ended up on the same PR kind of by mistake

@daroga0002
Copy link
Contributor

I dont understand what IP filter you writing. In general if you set public only then it also works (especially as this is default behaviour).

Please also rebase branch to current master

@schollii
Copy link
Author

schollii commented Sep 17, 2021

The rebase was a bit of a headache because the local.tf was renamed meanwhile to locals.tf. The latest is code has more changes that what I did originally. I deleted this fork and recreated it and create PR #1590, was way easier given the small number of mods.

I create PR #1592 re public API endpoint IP filtering, it ended up in here by mistake.

@schollii schollii closed this Sep 17, 2021
@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 12, 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