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

Default to beta API for eks get-token #6940

Merged
merged 1 commit into from
May 12, 2022

Conversation

micahhausler
Copy link
Member

@micahhausler micahhausler commented May 11, 2022

  • Default to beta API for eks get-token
  • Stop logging a warning on an empty KUBERNETES_EXEC_INFO
  • Respond with correct deprecated version in warning message

Signed-off-by: Micah Hausler mhausler@amazon.com

Issue #, if available:

Resolves #6935
Resolves aws/containers-roadmap#1736

Description of changes:

From Kubernetes 1.11 trough 1.19, client-go (so kubectl, kubelet, etc) accepted client.authentication.k8s.io API version v1alpha1 or v1beta1, but only provided KUBERNETES_EXEC_INFO if the v1alpha1 API version was specified in a kubeconfig (fixed in kubernetes/kubernetes#95489). This leaves client binaries (such as aws-iam-authenticator or aws eks get-token) having to guess what version of the API to respond with. If they guess wrong (which the AWS CLI currently does by falling back to v1alpha1), the user gets an error (observed in comment on #6476).

For 1.11-1.19 clients, if their kubeconfig specifies:

  • v1alpha (the old default) - Everything works. v1alpha1 is specified in KUBERNETES_EXEC_INFO, the CLI will correctly provide the right response, and they'll get a warning to upgrade and run aws eks update-kubeconfig
  • v1beta1 (the new default) - Everything works. The CLI will now correctly guess that an empty KUBERNETES_EXEC_INFO means that client-go handled a v1beta1 kubeconfig, but isn't telling the CLI about it.
  • v1 (they would have to manually set this) - They will get a failure before ever executing the AWS CLI.

For 1.20-1.23 clients (EKS only supports up to 1.22 today), if their kubeconfig specifies:

  • v1alpha (the old default) - Everything works. v1alpha1 is specified in KUBERNETES_EXEC_INFO, the CLI will correctly provide the right response, and they'll get a warning to upgrade and run aws eks update-kubeconfig
  • v1beta1 (the new default) - Everything works. v1beta1 is specified in KUBERNETES_EXEC_INFO, the CLI will correctly provide the right response

For clients <= 1.21, if their kubeconfig specifies v1, they will get a failure before ever executing the AWS CLI.

For 1.24+ clients (if someone installs kubectl with homebrew), if their kubeconfig specifies:

Kubectl version Supports Alpha API can accept beta API Provides EXEC_INFO for non-alpha kubeconfigs Supports v1 API Available for new EKS clusters
v1.17 yes yes no no no
v1.18 yes yes no no no
v1.19 yes yes no no yes
v1.20 yes yes yes no yes
v1.21 yes yes yes no yes
v1.22 yes yes yes yes yes
v1.23 yes yes yes yes no
v1.24 no yes yes yes no

Once EKS no longer supports 1.19 (or previous) clusters, we can add back a warning about the empty KUBERNETES_EXEC_INFO

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Test code used to validate the above
#!/usr/bin/env bash
set -euo pipefail

# gleaned from https://kubernetes.io/releases/patch-releases/#support-period
versions=(
  v1.17.17
  v1.18.20
  v1.19.16
  v1.20.15
  v1.21.12
  v1.22.9
  v1.23.6
  v1.24.0
)


# get_client downloads kubectl
get_client() {
    local version="$1"
    local download="kubernetes-client-darwin-amd64.tar.gz"
    local minor=$(echo $version | cut -c 2-5 | tr '.' '-')
    if [ -f "./kubectl-$minor" ]; then
        return
    fi
    URL=https://dl.k8s.io/${version}/${download}
    wget $URL
    tar xvf $download
    mv kubernetes/client/bin/kubectl ./kubectl-${minor}
    rm -rf ./kubernetes ./$download
}

for version in "${versions[@]}"; do
    get_client $version
done

try_version() {
    local version="$1"
    local minor=$(echo $version | cut -c 2-5 | tr '.' '-')
    local kcli="kubectl-${minor}"

    # copy kubeconfig
    local kubeconfig="kubeconfig-${minor}"
    kubectl config view --minify --raw | sed -e 's,client\.authentication\.k8s\.io/.*,client.authentication.k8s.io/v1alpha1,g' > $kubeconfig

    echo "Testing kubectl version $version with alpha"
    set +e
    KUBECONFIG=${kubeconfig} ./$kcli version
    set -e
    echo

    echo "Testing kubectl version $version with beta"
    kubectl config view --minify --raw | sed -e 's,client\.authentication\.k8s\.io/.*,client.authentication.k8s.io/v1beta1,g' > $kubeconfig
    set +e
    KUBECONFIG=${kubeconfig} ./$kcli version
    set -e
    echo

    echo "Testing kubectl version $version with v1"
    kubectl config view --minify --raw | sed -e 's,client\.authentication\.k8s\.io/.*,client.authentication.k8s.io/v1,g' > $kubeconfig
    set +e
    KUBECONFIG=${kubeconfig} ./$kcli version
    set -e
    echo
}

for version in "${versions[@]}"; do
    try_version $version
done

@codecov-commenter
Copy link

codecov-commenter commented May 11, 2022

Codecov Report

Merging #6940 (99d0fa1) into develop (d693aed) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #6940      +/-   ##
===========================================
- Coverage    92.87%   92.87%   -0.01%     
===========================================
  Files          204      204              
  Lines        16347    16345       -2     
===========================================
- Hits         15182    15180       -2     
  Misses        1165     1165              
Impacted Files Coverage Δ
awscli/customizations/eks/get_token.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d693aed...99d0fa1. Read the comment docs.

@micahhausler micahhausler force-pushed the get-token-version-default branch 4 times, most recently from 1b745fd to 45d13d4 Compare May 11, 2022 21:40
@@ -0,0 +1,5 @@
{
"type": "bugfix",
"category": "eks get-token",
Copy link
Member

Choose a reason for hiding this comment

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

nit; Can we make the category ``eks`` to be consistent with other eks changes? We can mention get-token in the description if needed.

Copy link
Member Author

Choose a reason for hiding this comment

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

This was based on @kyleknap 's comment #6476 (comment)

I can change it to just "eks"

Copy link
Member

Choose a reason for hiding this comment

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

I'll defer to Kyle then if that's the category he wants.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added another as a feature one that is also eks get-token. I'm fine with that as the category.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think eks get-token make sense especially because it is a custom command. One super nitpick thing that I missed in the last PR that I reviewed was that we should encompass the category with double backticks so it shows up as a code literal in the change log (e.g. "``eks get-token``" instead of just "eks get-token"). We should make sure to do that for both change logs entries we have now.

@stealthycoin stealthycoin force-pushed the get-token-version-default branch from 45d13d4 to 835393f Compare May 12, 2022 16:14
* Stop logging a warning on an empty KUBERNETES_EXEC_INFO
* Respond with correct deprecated version in warning message

Signed-off-by: Micah Hausler <mhausler@amazon.com>
@stealthycoin stealthycoin force-pushed the get-token-version-default branch from 835393f to 99d0fa1 Compare May 12, 2022 17:16
Copy link
Contributor

@kyleknap kyleknap left a comment

Choose a reason for hiding this comment

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

Thanks! 🚢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants