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

Add EKS cluster auth token data resource #4904

Closed
wants to merge 1 commit into from

Conversation

@evilmarty
Copy link
Contributor

evilmarty commented Jun 21, 2018

Allow Terraform to authenticate with an EKS cluster via the Kubernetes provider:

resource "aws_eks_cluster" "foo" {
  name = "foo"
}

data "aws_eks_cluster_auth" "foo_auth" {
  name = "foo"
}

provider "kubernetes" {
  host = "${aws_eks_cluster.foo.endpoint}"
  cluster_ca_certificate = "${base64decode(aws_eks_cluster.foo.certificate_authority.0.data)}"
  token = "${data.aws_eks_cluster_auth.foo_auth.token}"
}

The auth logic was extracted from
https://github.com/heptio/aws-iam-authenticator because of lack of
documentation from AWS. Basically, the token is a signed URL for the
GetCallerIdentity action with a custom header. The URL is then base64
encoded and prefixed with vendor string.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSEksClusterAuthDataSource_basic'
 ==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -run=TestAccAWSEksClusterAuthDataSource_basic -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSEksClusterAuthDataSource_basic
--- PASS: TestAccAWSEksClusterAuthDataSource_basic (40.47s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	40.510s
@wotc-baynhas

This comment has been minimized.

Copy link

wotc-baynhas commented Aug 5, 2018

This would be a huge deal for me, and is a major blocker for adoption of the kubernetes provider- not really much of a point to migrate kubectl configs over if I can't have a cluster created and configured with a single apply.

@omeid

This comment has been minimized.

Copy link
Contributor

omeid commented Dec 23, 2018

Is there anything holding this up? this would make using kubernetes provider with eks very seamless!

@omeid

This comment has been minimized.

Copy link
Contributor

omeid commented Jan 6, 2019

@evilmarty Are you still keen to get this landed? I think rebasing should help move things ahead.

@evilmarty evilmarty force-pushed the evilmarty:master branch from 98b8f13 to 917695c Jan 6, 2019
@hashibot hashibot bot added size/M provider tests and removed size/L labels Jan 6, 2019
This allows Terraform to authenticate with an EKS cluster via the
Kubernetes provider:

```hcl
resource "aws_eks_cluster" "foo" {
  name = "foo"
}

data "aws_eks_cluster_auth" "foo_auth" {
  name = "foo"
}

provider "kubernetes" {
  host = "${aws_eks_cluster.foo.endpoint}"
  cluster_ca_certificate = "${base64decode(aws_eks_cluster.foo.certificate_authority.0.data)}"
  token = "${data.aws_eks_cluster_auth.foo_auth.token}"
}
```

The auth logic was extracted from
https://github.com/heptio/aws-iam-authenticator because of lack of
documentation from AWS. Basically, the token is a signed URL for the
GetCallerIdentity action with a custom header. The URL is then base64
encoded and prefixed with vendor string.
@evilmarty evilmarty force-pushed the evilmarty:master branch from 917695c to fea6d9f Jan 6, 2019
@evilmarty

This comment has been minimized.

Copy link
Contributor Author

evilmarty commented Jan 6, 2019

@omeid Sure am. I've rebased with master.

@omeid

This comment has been minimized.

Copy link
Contributor

omeid commented Jan 7, 2019

@apparentlymart Can you look at this please? this is a very small change that makes a massive difference in using EKS and K8S with Terraform.

@yves-vogl

This comment has been minimized.

Copy link

yves-vogl commented Jan 13, 2019

Hi,

we'd be also very happy to see this being merge.

Thanks for this great implementation, @evilmarty!

@ktham

This comment has been minimized.

Copy link

ktham commented Jan 14, 2019

cc @alexsomesan We'd also like to see this merged in being EKS users ourselves. Thanks @evilmarty !

@ktham

This comment has been minimized.

Copy link

ktham commented Jan 14, 2019

cc @bflad also, we can remove the need for terraform-providers/terraform-provider-kubernetes#161 if we get this in, and this would be much cleaner as well.

@ktham

This comment has been minimized.

Copy link

ktham commented Jan 15, 2019

@evilmarty looks like you need to rebase on master again

@bflad

This comment has been minimized.

Copy link
Member

bflad commented Jan 15, 2019

Hi @evilmarty (and everyone chiming in) 👋 Thanks for submitting and showing your interest in this new data source. Apologies for the lengthy delay in an initial review.

Due to the lack of AWS/EKS documentation on the matter, we have been hesitant to accept any implementation due to its potential to be incorrect or outdated. I think we are willing to concede a little on this though because the EKS documentation goes through great lengths about using aws-iam-authenticator and this code is currently based off that, which likely means there is some stability for the interface and implementation.

There are few things holding this pull request up though:

  • Missing Terraform website documentation for the new data source (sidebar link in website/aws.erb and new website/docs/d/eks_cluster_auth.html.markdown page as noted in https://github.com/terraform-providers/terraform-provider-aws/blob/master/.github/CONTRIBUTING.md#new-resource)
  • A lack of inline code documentation surrounding the how's and why's of the implementation (partially AWS/EKS fault on this due to their lack of documentation on the matter, but I would expect to see comments similar to the upstream aws-iam-authenticator ones or at least URL comments pointing to the upstream codebase for context and future maintainability of this code)
  • Only limited testing is being performed in the acceptance testing (its currently checking for a non-empty string, how do we know its a valid token?)
  • Double checking that a configurable duration value is valid, given some of the notes in the upstream code

I think rather than re-implementing this ourselves which may more maintenance overhead than its potentially worth should the interface or implementation changes on AWS' side in the future, this may actually be a good time to consider vendoring the part of aws-iam-authenticator that handles this: https://github.com/kubernetes-sigs/aws-iam-authenticator/blob/master/pkg/token/token.go

The upstream code handles most of my concerns by providing an easy GetWithSTS(clusterID string, stsAPI *sts.STS) (Token, error) interface and a verification method Verify(token string) (*Identity, error) which we can acceptance test against.

If we decide to go that path, I would encourage waiting a few days while we switch the repository over to Go modules in preparation for vendoring Terraform 0.12 and beginning 2.0 provider work so you don't wind up with a vendoring pull request that needs to be redone. 👍 I will try to remember to post back here when that's completed.

@evilmarty

This comment has been minimized.

Copy link
Contributor Author

evilmarty commented Jan 17, 2019

@bflad I agree with you 100%. I was not aware of that project but it a lot of sense to use an existing and supported implementation. I'm happy to make those changes but my time is a little limited right now as I just had a baby girl, which buys me time for Terraform 0.12 to release.

@bflad

This comment has been minimized.

Copy link
Member

bflad commented Feb 6, 2019

Thanks so much to @evilmarty (congratulations on the baby girl 👶 🎉 ) and @mbarrien this new aws_eks_cluster_auth data source has been merged via #7438 and will release with version 1.58.0 of the Terraform AWS provider, likely in the next day or two.

I'm guessing the co-authorship of commit a6ba529 was enough of a difference for GitHub to not automatically close this PR when #7438 was merged but proper attribution should be in the Git history to @evilmarty for the initial work. 👍

@bflad bflad closed this Feb 6, 2019
@bflad

This comment has been minimized.

Copy link
Member

bflad commented Feb 8, 2019

This has been released in version 1.58.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@evilmarty

This comment has been minimized.

Copy link
Contributor Author

evilmarty commented Feb 10, 2019

Thanks everyone for your help and guidance in getting this feature merged. Again, apologies for lack of comms recently. Glad that it did not affect the overall outcome.

@ncrothe

This comment has been minimized.

Copy link

ncrothe commented Mar 1, 2019

This is a great addition, considering the external script approach was clunky. However, our use case with the external script involved assuming a role (aws-iam-authenticator token -i <clustername> -r <some_IAM_role>). Would it be hard to add an optional role parameter?

The use case behind that is group permissions, i.e. without roles I would need to add every user that needs Kubernetes permissions individually to the aws auth config map (as far as I understand). With roles, I can just give sts:assume for a particular role (that doesn't even need to give any intrinsic permissions) to a group policy for everyone I want to be able to work with eks.

@bflad

This comment has been minimized.

Copy link
Member

bflad commented Mar 1, 2019

@ncrothe if I'm thinking about this correctly, would setting up an assume role provider configuration suit your need? e.g.

provider "aws" {
  # ... potentially other configuration ...
  alias = "eks_assume_role"

  assume_role {
    role_arn = "arn:PARTITION:iam::ACCOUNTID:role/ROLENAME"
  }
}

data "aws_eks_cluster_auth" "example" {
  provider = "aws.eks_assume_role"
  name     = "example"
}

If not, I would suggest opening a new GitHub issue for further tracking. 👍

@ncrothe

This comment has been minimized.

Copy link

ncrothe commented Mar 2, 2019

Great idea, will test that out.

And of course, if not working, I'll open an issue.

@ncrothe

This comment has been minimized.

Copy link

ncrothe commented Mar 4, 2019

@bflad Worked like a charm, thank again for the suggestion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.