-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Deprecate Kubernetes client API version v1alpha1 #6476
Deprecate Kubernetes client API version v1alpha1 #6476
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #6476 +/- ##
===========================================
+ Coverage 92.85% 92.87% +0.01%
===========================================
Files 204 204
Lines 16310 16344 +34
===========================================
+ Hits 15145 15179 +34
Misses 1165 1165
Continue to review full report at Codecov.
|
@@ -143,3 +144,105 @@ def test_url_different_partition(self): | |||
expected_endpoint='sts.cn-north-1.amazonaws.com.cn', | |||
expected_signing_region='cn-north-1' | |||
) | |||
|
|||
def test_api_version_discovery_deprecated(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do these tests get run?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're run by pytest. See scripts/ci/run-tests
def test_api_version_discovery_deprecated(self): | ||
os.environ["KUBERNETES_EXEC_INFO"] = '{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1alpha1","spec":{"interactive":true}}' | ||
cmd = 'eks get-token --cluster-name %s' % self.cluster_name | ||
stdout, stderr, _ = self.run_cmd(cmd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this testing with an external kubectl binary? If so, what version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is called just by the test runner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cool if we had an integration test that used client-go in some capacity, even if it was only a manual test initially. Not a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
fixes #6308 |
del os.environ["KUBERNETES_EXEC_INFO"] | ||
|
||
def test_api_version_discovery_malformed(self): | ||
os.environ["KUBERNETES_EXEC_INFO"] = '{{"kind":"ExecCredential","apiVersion":"client.authentication.k8s.io/v1alpha1","spec":{"interactive":true}}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should be using a copy of the environment variables here (and the other tests) instead (i.e., os.environ.copy()
). The tests are run in parallel, and directly modifying envvars (os.environ["KUBERNETES_EXEC_INFO"] = ...
, del os.environ["KUBERNETES_EXEC_INFO"]
) may interfere with other tests running in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've used this in the past for setting env vars: #6476 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine. All of my feedback was related to Python-specifics and codebase usage/organization.
@@ -13,6 +13,8 @@ | |||
import base64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be great if we can get a changelog for this feature? You can use the scripts/new-change
script to generate a changelog entry. For the type field, I'd recommend setting the type to enhancement
and the category to eks get-token
Kubernetes has deprecated v1alpha1, v1beta1 has been available since Kubernetes v1.11 (kubernetes/kubernetes#64482), and EKS currently supports Kubernetes versions v1.16 through v1.21. This is a breaking change for clients running versions v1.10 and older, which haven't been supported by EKS since September 2019. "aws eks get-token" now respects the KUBERNETES_EXEC_INFO environment variable and conservatively falls back to v1alpha1, which is supported by Kubernetes versions 1.10 through 1.22 (released upstream August 2021, to be released by EKS in Q4 2021). It also now supports "v1beta1" and "v1". "aws eks update-kubeconfig" now writes "v1beta1" in the kubeconfig which will be supported by Kubernetes until 1.29 (aproximately December 2023). At or around that date, we can change the default version written to kubeconfigs to "v1" Signed-off-by: Micah Hausler <mhausler@amazon.com>
5ea108a
to
18fdfa3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Just had some more suggestions.
.changes/next-release/enhancement-eksupdatekubeconfig-63017.json
Outdated
Show resolved
Hide resolved
3a2c6d6
to
3124795
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🚢
LGTM |
] | ||
|
||
ERROR_MSG_TPL = ( | ||
"{0} KUBERNETES_EXEC_INFO, defaulting to {1}. This is likely a " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it say "Error reading KUBERNETES_EXEC_INFO" instead of just "KUBERNETES_EXEC_INFO"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will say Error parsing
to start the sentence as the variable is being used as a template. Tests also confirm that too: https://github.com/aws/aws-cli/pull/6476/files#diff-ee0c03e30582b485e61dba3f70f956a6024118bdffe8657c476fee7f9ac655afR194.
@micahhausler I am hitting an issue using kubectl 1.19 where it doesn't set KUBERNETES_EXEC_INFO. So the following flow is broken...
This returns the following error:
kubectl >- 1.20 works as expected. I still have some 1.19 clusters running so i kept kubectl at 1.19, but I can update kubectl, but since eks still supports kubernetes 1.19 I figured it was worth a post. |
@zawachte-c thank you! Take a look at #6940, as this should resolve the <= 1.19 problems |
Kubernetes has deprecated v1alpha1, v1beta1 has been available since Kubernetes
v1.11 (kubernetes/kubernetes#64482), and EKS currently supports Kubernetes
versions v1.16 through v1.21. This is a breaking change for clients running
versions v1.10 and older, which haven't been supported by EKS since September
2019.
"aws eks get-token" now respects the KUBERNETES_EXEC_INFO environment
variable and conservatively falls back to v1alpha1, which is supported
by Kubernetes versions 1.10 through 1.22 (released upstream August 2021, to be
released by EKS in Q4 2021). It also now supports clients requesting credentials
using the "v1beta1" and "v1" versions.
"aws eks update-kubeconfig" now writes "v1beta1" in the kubeconfig which
will be supported by Kubernetes until 1.29 (aproximately December 2023).
At or around that date, we can change the default version written to
kubeconfigs to "v1"
Signed-off-by: Micah Hausler mhausler@amazon.com
Issue #, if available:
Description of changes:
See above
cc: @nckturner @sgundapu
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.