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

Allow to set api_version for kube_exec_auth #123

Closed
wants to merge 1 commit into from
Closed

Allow to set api_version for kube_exec_auth #123

wants to merge 1 commit into from

Conversation

z0rc
Copy link
Contributor

@z0rc z0rc commented Jul 29, 2021

what

Allow to set api_version for kube_exec_auth. Maintain backwards compatibility by using older API version.

why

Latest AWS CLI switched API version from client.authentication.k8s.io/v1alpha1 to client.authentication.k8s.io/v1beta1. As there is no easy way to autodetect this version, provide variable that allows to set API version to expected value.

references

Latest AWS CLI switched API version from
'client.authentication.k8s.io/v1alpha1' to
'client.authentication.k8s.io/v1beta1'. As there is no easy way to
autodetect this version, provide variable that allows to set API version
to expected value.

Maintain backwards compatibility by using older API version.
@z0rc z0rc requested review from a team as code owners July 29, 2021 09:55
@z0rc z0rc requested review from dotCipher and brcnblc July 29, 2021 09:55
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

@z0rc Thank you for this PR. When I say "this is awful" I do not mean your work, I mean this situation.

I want to do a lot more investigation before I accept that there is no way to automatically configure the correct version or force the aws CLI to return a consistent version.

I think I would rather run aws --version and parse the results if necessary, because it is going to be too hard for users to configure this across environments.

@Nuru Nuru self-assigned this Jul 30, 2021
@Nuru Nuru added the do not merge Do not merge this PR, doing so would cause problems label Jul 30, 2021
@z0rc
Copy link
Contributor Author

z0rc commented Jul 30, 2021

@Nuru your proposed approach has it's own set of challenges and resulting code will be a pain to maintain. Here are some of them:

  • Terraform doesn't provide a sane way to capture stdout of exec into variable. null_resource with local-exec doesn't capture stdout, data external requires specific json structure in stdout, which aws-cli doesn't implement (maybe doable with pipe to jq).
  • Assuming previous point is solved, terraform doesn't provide functions to compare semver. There will be some hard to read string splitting and array comparison.
  • There are two maintained aws-cli versions v1 and v2, which translates in even more complex logic for semver matching.
  • On top of that, there are optional module variables that have to be taken in account and complicate already complex locals used for version extraction and comparison even further.

I honestly tried to code some of those challenges, but it doesn't look pretty or even solvable at this moment. The potential amount of esoteric terraform code to just set single string is horrifying me.

I'm happy if you find a way to solve this, but personally I already found a solution that satisfies me, this PR.

@max-lobur
Copy link
Contributor

max-lobur commented Jul 30, 2021

instead it should match API version returned by aws eks get-token

You might try:

  1. Do the bash script that does this (pass cluster name as a parameter).
  2. Then parse out the field you need using jq or yq
  3. Return the formated value out of the script, so that you don't need to do anything additional in tf

bash

#!/usr/bin/env bash

set -eo pipefail

CLUSTER_NAME="${1}"
aws eks get-token --cluster-name ${CLUSTER_NAME} | jq ".apiVersion | [. ]"

tf

data "external" "kube_exec_auth_api_version" {
  program = [
    "bash",
    "${path.module}/scripts/kube_exec_auth_api_version.sh",
   aws_eks_cluster.default.name,
  ]
}

...
api_version = data.external.kube_exec_auth_api_version.result[0]
...

I did not test, but copied from the other internal working example which does similar thing for AWS MSK

@z0rc
Copy link
Contributor Author

z0rc commented Jul 30, 2021

@max-lobur Proposed approach introduces bash and jq dependencies, which doesn't sit well with me for running terraform. I'd probably migrate away from this module, if something like this is implemented.

@max-lobur
Copy link
Contributor

best example I have ¯_(ツ)_/¯

@Nuru
Copy link
Contributor

Nuru commented Jul 31, 2021

@z0rc I would recommend that rather than migrate away from this module because exec_auth (which introduces a dependency on the aws CLI being installed and AWS credentials being available in the environment and now has a version matching problem, see #124) does not work, you use the default, supported, data_auth option.

We are not committed to supporting exec_auth long-term. It is a hack to work around issues with the Kubernetes provider and the way Terraform handles provider initialization (as is dummy_kubeapi_server). I support providing a fix for this problem, since it affects the primary use case for exec_auth, but we are not committed to making exec_auth fully portable or supported on all platforms. We only fully support data_auth.

@z0rc
Copy link
Contributor Author

z0rc commented Aug 1, 2021

@Nuru Well, thing is exec_auth was the option I tested when upgrading 1.20 clusters to 1.21 recently, and to my finding this was the first time terraform plan and apply succeeded without manual intervention. I'm not sure whether it's related to switching auth method or by introduction of dummy_kubeapi_server. I'll check out data_auth option. Either way, introducing new option and stating it isn't supported sends confusing signal to community.

Anyway, there is aws/aws-cli#6308 now. The breaking change is reverted and this PR isn't needed anymore. Something like this might be needed in future, depending how api version handling develops on aws-cli side.

I'll wait a couple of days to see how story develops here and on aws-cli side. But most likely this PR will be closed without merge.

@Nuru
Copy link
Contributor

Nuru commented Aug 2, 2021

No longer needed (See #122)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge Do not merge this PR, doing so would cause problems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate kube_exec_auth to client.authentication.k8s.io/v1beta1 api version
3 participants