common-fate / granted

The easiest way to access your cloud.
https://granted.dev
MIT License
956 stars 90 forks source link

v0.20.6: eks get-token regression #591

Closed nocnokneo closed 5 months ago

nocnokneo commented 5 months ago

With v0.20.6 I get the following error when attempting to use the kubectl with the recommended authentication configuration in ~/.kube/config:

E0108 09:50:23.809791  817786 memcache.go:265] couldn't get current server API group list: Get "https://<REDACTED>.gr7.us-east-2.eks.amazonaws.com/api?timeout=32s": getting credentials: decoding stdout: couldn't get version/kind; json parse error: json: cannot unmarshal string into Go value of type struct { APIVersion string "json:\"apiVersion,omitempty\""; Kind string "json:\"kind,omitempty\"" }

When running GRANTED_QUIET=true FORCE_NO_ALIAS=true assumego test --exec 'aws --region us-east-2 eks get-token --cluster-name default --output json' this is the JSON output I get with v0.20.6:

GrantedExec <REDACTED> None us-east-2 None false None None None None aws --region us-east-2 eks get-token --cluster-name default --output json

(clearly not json...)

Reverting to v0.20.3 works. This is what I get when running the same assumego command with v0.20.6:

{
    "kind": "ExecCredential",
    "apiVersion": "client.authentication.k8s.io/v1beta1",
    "spec": {},
    "status": {
        "expirationTimestamp": "2024-01-08T15:12:09Z",
        "token": "k8s-aws-v1.<REDACTED>"
    }
}

Is there not a unit test to catch basic stuff like this?

jhuntwork commented 5 months ago

What happens when you use assume --exec -- instead of assumego --exec? I raised a similar issue in #573 but I think these are the result of a fundamental shift in usage brought on by #549

jhuntwork commented 5 months ago

Also it seems the published docs are now broken, since they recommend calling assumego directly: https://docs.commonfate.io/granted/recipes/eks/ So from that standpoint, this is definitely a regression. At the very least the docs should change. cc @JoshuaWilkes

nocnokneo commented 5 months ago

Yes, updating my ~/.kube/config with assume in place of assumego works. At this point the sudden breaking change is already done so maybe just update the docs? It would be nice to have a deprecated transition period for things like this in the future.

jhuntwork commented 5 months ago

It would be nice to have a deprecated transition period for things like this in the future.

Agreed. At the very least I think this change warranted an updated minor version instead of just patch-level.

shwethaumashanker commented 5 months ago

@nocnokneo and @jhuntwork, thank you for reporting this issue! We apologize for not updating the docs sooner; there was an oversight on our part. The necessary updates have been made to the community channel, our documentation, and we have taken internal note of this. Sorry again!