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

Added write_aws_auth_config option #228

Conversation

yutachaos
Copy link
Contributor

@yutachaos yutachaos commented Dec 27, 2018

PR o'clock

Description

Added an option not to create aws_auth configmap.
I made it possible to apply kubectl apply without generating a file to correspond.

Refer issues
#169
#170

Checklist

  • terraform fmt and terraform validate both work from the root and examples/eks_test_fixture directories (look in CI for an example)
  • Tests for the changes have been added and passing (for bug fixes/features)
  • Test results are pasted in this PR (in lieu of CI)
  • I've added my change to CHANGELOG.md
  • Any breaking changes are highlighted above

@yutachaos yutachaos force-pushed the feature/fixed_aws_auth_configmap_update_when_no_manage_aws_auth branch from 8878814 to 0779391 Compare December 27, 2018 05:52
aws_auth.tf Outdated
working_dir = "${path.module}"

command = <<EOS
mkfifo aws_auth_configmap kube_config & \
Copy link
Contributor

Choose a reason for hiding this comment

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

mkfifo won't work on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your comment.
For compatibility with Windows, it changed to a method not using mkfifo command.

@max-rocket-internet
Copy link
Contributor

How about simply if a user sets write_kubeconfig=false and manage_aws_auth=true then when TF runs kubectl to apply the aws-auth configmap, it just removes the --kubeconfig option and then it is up to the user solve the authentication themselves?

@yutachaos yutachaos force-pushed the feature/fixed_aws_auth_configmap_update_when_no_manage_aws_auth branch from 902f855 to d190055 Compare January 4, 2019 05:34
@max-rocket-internet
Copy link
Contributor

I'm still not convinced this is a good approach. Think about these scenarios:

  • What if the user already has a file called aws_auth_configmap.yaml or kube_config.yaml in CWD? It will be overwritten and then deleted.
  • What if the user has 2 clusters? TF runs many things in parallel and this could cause either the same config being applied twice or to the wrong cluster.
  • What the user runs CTRL-C during run? It could leave the files there.

I think you can solve these by writing the file to "${file("${path.module}/..} instead of CWD. Can you test that?

@yutachaos
Copy link
Contributor Author

I am using the local-exec working_dir option to run under module.

FYI
https://www.terraform.io/docs/provisioners/local-exec.html#working_dir

@yutachaos
Copy link
Contributor Author

yutachaos commented Jan 5, 2019

How about simply if a user sets write_kubeconfig=false and manage_aws_auth=true then when TF runs kubectl to apply the aws-auth configmap, it just removes the --kubeconfig option and then it is up to the user solve the authentication themselves?

I am assuming to use the aws eks update-kubeconfig command.

example

  1. TF runs with write_kubeconfig=false and manage_aws_auth=true options
  2. aws eks update-kubeconfig --name {create-cluster-name}
  3. kubectl get nodes

@max-rocket-internet
Copy link
Contributor

I am using the local-exec working_dir option to run under module.

You are right! My mistake, sorry!

I am assuming to use the aws eks update-kubeconfig command.

You mean the AWS CLI? But in this PR, you are not assuming that. If you were assuming that, then we wouldn't need to write kube_config in update_config_map_aws_auth because this would be the responsibility of the user. Right? Or am I misunderstanding?

@max-rocket-internet
Copy link
Contributor

Since @philwinder opened the original issue, I've asked them to test this PR 🙂

@yutachaos
Copy link
Contributor Author

You mean the AWS CLI? But in this PR, you are not assuming that. If you were assuming that, then we wouldn't need to write kube_config in update_config_map_aws_auth because this would be the responsibility of the user. Right? Or am I misunderstanding?

Updating using kubeconfig in update_config_map_aws_auth is only used to join eks node to eks cluster.

see
https://docs.aws.amazon.com/eks/latest/userguide/add-user-role.html

@yutachaos yutachaos force-pushed the feature/fixed_aws_auth_configmap_update_when_no_manage_aws_auth branch from d190055 to a38a290 Compare January 11, 2019 03:36
@yutachaos yutachaos force-pushed the feature/fixed_aws_auth_configmap_update_when_no_manage_aws_auth branch 2 times, most recently from c25c08a to b4005eb Compare January 29, 2019 01:16
@yutachaos
Copy link
Contributor Author

@max-rocket-internet
How about this PR?
If further explanation is necessary, I will add it.
Thank you.

@max-rocket-internet
Copy link
Contributor

Hi @yutachaos
I had a look a couple days ago but there were conflicts. And now this PR is undoing the changes in https://github.com/terraform-aws-modules/terraform-aws-eks/pull/245/files for ZSH compatibility 🙂

Fix that and we can merge.

aws_auth.tf Outdated
working_dir = "${path.module}"

command = <<EOS
for i in {1..5}; do \
Copy link
Contributor

Choose a reason for hiding this comment

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

This was changed in #245 for ZSH

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you fixed it.

@yutachaos yutachaos force-pushed the feature/fixed_aws_auth_configmap_update_when_no_manage_aws_auth branch from b4005eb to 6c0d103 Compare January 31, 2019 05:20
@yutachaos yutachaos force-pushed the feature/fixed_aws_auth_configmap_update_when_no_manage_aws_auth branch 3 times, most recently from b6b65f9 to 883f1b0 Compare January 31, 2019 05:42
@yutachaos yutachaos force-pushed the feature/fixed_aws_auth_configmap_update_when_no_manage_aws_auth branch from 883f1b0 to 64a5785 Compare January 31, 2019 05:45
@max-rocket-internet
Copy link
Contributor

fixes #169
fixes #170

@a7i
Copy link

a7i commented Feb 7, 2019

@max-rocket-internet Hi there, thank you for adding this. Are there any plans to create a Release Tag?

@max-rocket-internet
Copy link
Contributor

Just for you @a7i: https://github.com/terraform-aws-modules/terraform-aws-eks/releases/tag/v2.2.0

tekn0ir pushed a commit to tekn0ir/terraform-aws-eks that referenced this pull request Feb 13, 2019
* master:
  Release v2.2.0 (terraform-aws-modules#270)
  Add optional permissions_boundary (terraform-aws-modules#265)
  Updating example IAM docs to include Launch Template actions (terraform-aws-modules#268)
  Added write_aws_auth_config option (terraform-aws-modules#228)
  Worker group tags (terraform-aws-modules#252)
  Adding the g3s.xlarge instance type ebs optimized mapping (terraform-aws-modules#258)
  Add enabled_metrics attributes to autoscaling_group (terraform-aws-modules#256)
  Use launch template defaults for launch template userdata (terraform-aws-modules#255)
  Enable create_before_destroy for ASG and enable force_delete to be configured (terraform-aws-modules#250)
@yutachaos yutachaos deleted the feature/fixed_aws_auth_configmap_update_when_no_manage_aws_auth branch October 25, 2019 04:39
@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 20, 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.

3 participants