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

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

@ghost ghost added the size/L Managed by automation to categorize the size of a PR. label Jun 21, 2018
@radeksimko radeksimko added new-data-source Introduces a new data source. service/eks Issues and PRs that pertain to the eks service. labels Jun 21, 2018
@wotc-baynhas
Copy link

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
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
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.

@ghost ghost added size/M Managed by automation to categorize the size of a PR. provider Pertains to the provider itself, rather than any interaction with AWS. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/L Managed by automation to categorize the size of a PR. 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
Copy link
Contributor Author

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

@omeid
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
Copy link

Hi,

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

Thanks for this great implementation, @evilmarty!

@ktham
Copy link
Contributor

ktham commented Jan 14, 2019

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

@ktham
Copy link
Contributor

ktham commented Jan 14, 2019

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

@ktham
Copy link
Contributor

ktham commented Jan 15, 2019

@evilmarty looks like you need to rebase on master again

@bflad
Copy link
Contributor

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
Copy link
Contributor Author

@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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

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
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
Copy link
Contributor

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
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
Copy link

ncrothe commented Mar 4, 2019

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

bflad added a commit that referenced this pull request Jan 31, 2020
…kg/token to internal implementation

Reference: #11697
Reference: #8453
Reference: #7438
Reference: #4904

Including the Kubernetes ecosystem dependency rather than hard copying the implementation was originally for a few concerns as noted in #4904 (comment). Since its introduction, the upstream implementation has remained stable with respects to the GetWithSTS token generator implementation we use.

However, changes to the surrounding upstream package code and its broad transitive dependencies have prevented a clear upgrade path since github.com/kubernetes-sigs/aws-iam-authenticator@v0.4.0 (now re-verified with v0.5.0), where Terraform AWS Provider builds cannot succeed on solaris/amd64:

```console
$ gox -os='linux darwin windows freebsd openbsd solaris' -arch='386 amd64 arm' -osarch='!darwin/arm !darwin/386' -ldflags '-s -w -X aws/version.ProviderVersion=99.99.99 -X aws/version.ProtocolVersion=4' -output 'results/{{.OS}}_{{.Arch}}/terraform-provider-aws_v99.99.99_x4' .
...
1 errors occurred:
--> solaris/amd64 error: exit status 2
Stderr: # github.com/gofrs/flock
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:28:22: undefined: syscall.LOCK_EX
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:39:22: undefined: syscall.LOCK_SH
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:56:12: undefined: syscall.Flock
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:66:12: undefined: syscall.Flock
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:96:12: undefined: syscall.Flock
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:96:42: undefined: syscall.LOCK_UN
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:118:21: undefined: syscall.LOCK_EX
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:130:21: undefined: syscall.LOCK_SH
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:149:9: undefined: syscall.Flock
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:149:44: undefined: syscall.LOCK_NB
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:149:44: too many errors
```

This issue is non-obvious to contributors and maintainers as we do not perform cross-compilation build testing in CI during pull requests since it is very time prohibitive.

Rather than leave this single data source's dependency in an unstable state, instead we opt to hard copy the relevant upstream Go package and prune that package to only the code we use, removing many unnecessary dependencies.

Updated via:

```console
$ go mod tidy
$ go mod vendor
```

Output from acceptance testing:

```
--- PASS: TestAccAWSEksClusterAuthDataSource_basic (15.00s)
```
bflad added a commit that referenced this pull request Feb 4, 2020
…kg/token to internal implementation (#11822)

* deps: Migrate from github.com/kubernetes-sigs/aws-iam-authenticator/pkg/token to internal implementation

Reference: #11697
Reference: #8453
Reference: #7438
Reference: #4904

Including the Kubernetes ecosystem dependency rather than hard copying the implementation was originally for a few concerns as noted in #4904 (comment). Since its introduction, the upstream implementation has remained stable with respects to the GetWithSTS token generator implementation we use.

However, changes to the surrounding upstream package code and its broad transitive dependencies have prevented a clear upgrade path since github.com/kubernetes-sigs/aws-iam-authenticator@v0.4.0 (now re-verified with v0.5.0), where Terraform AWS Provider builds cannot succeed on solaris/amd64:

```console
$ gox -os='linux darwin windows freebsd openbsd solaris' -arch='386 amd64 arm' -osarch='!darwin/arm !darwin/386' -ldflags '-s -w -X aws/version.ProviderVersion=99.99.99 -X aws/version.ProtocolVersion=4' -output 'results/{{.OS}}_{{.Arch}}/terraform-provider-aws_v99.99.99_x4' .
...
1 errors occurred:
--> solaris/amd64 error: exit status 2
Stderr: # github.com/gofrs/flock
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:28:22: undefined: syscall.LOCK_EX
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:39:22: undefined: syscall.LOCK_SH
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:56:12: undefined: syscall.Flock
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:66:12: undefined: syscall.Flock
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:96:12: undefined: syscall.Flock
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:96:42: undefined: syscall.LOCK_UN
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:118:21: undefined: syscall.LOCK_EX
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:130:21: undefined: syscall.LOCK_SH
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:149:9: undefined: syscall.Flock
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:149:44: undefined: syscall.LOCK_NB
../../../../go/pkg/mod/github.com/gofrs/flock@v0.7.0/flock_unix.go:149:44: too many errors
```

This issue is non-obvious to contributors and maintainers as we do not perform cross-compilation build testing in CI during pull requests since it is very time prohibitive.

Rather than leave this single data source's dependency in an unstable state, instead we opt to hard copy the relevant upstream Go package and prune that package to only the code we use, removing many unnecessary dependencies.

Updated via:

```console
$ go mod tidy
$ go mod vendor
```

Output from acceptance testing:

```
--- PASS: TestAccAWSEksClusterAuthDataSource_basic (15.00s)
```

* internal/service/eks/token: Fix linting issues from upstream code

Previously:

```
aws/internal/service/eks/token/token.go:74:8: `conjuction` is a misspelling of `conjunction` (misspell)
	// in conjuction with CloudTrail to determine the identity of the individual
	      ^
aws/internal/service/eks/token/token_test.go:144:20: S1019: should use make([]byte, maxTokenLenBytes + 1) instead (gosimple)
	b := make([]byte, maxTokenLenBytes+1, maxTokenLenBytes+1)
	                  ^
```
@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
new-data-source Introduces a new data source. provider Pertains to the provider itself, rather than any interaction with AWS. service/eks Issues and PRs that pertain to the eks service. size/M Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants