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.

Sign up for GitHub

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

Jump to bottom

Drop aws kubelet credential provider and cleanup aws storage e2e tests #116329

Merged
merged 2 commits into from
Mar 8, 2023
Merged

Drop aws kubelet credential provider and cleanup aws storage e2e tests #116329

merged 2 commits into from
Mar 8, 2023

Conversation

dims
Copy link
Member

@dims dims commented Mar 7, 2023 •
edited
Loading

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

To get rid of dependency to aws-sdk library we need additional clean up. Also note:

  • Since ecr-credential-provider has been available for a while and the KEP-2133 for external kubelet credential providers is already GA, folks can use that.
  • Also since we already removed the in-tree cloud provider for AWS, it only makes sense to clean the built-in kubelet credential provider as well.
  • The storage e2e tests had some references as well which do not make sense now since we removed the AWS EBS support already.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Removed AWS kubelet credential provider. Please use the external kubelet credential provider binary named `ecr-credential-provider` instead.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@dims
Drop AWS kubelet credential provider and cleanup AWS storage e2e tests
90d185b 
Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 7, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/dependency Issues or PRs related to dependency changes area/kubelet area/test sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 7, 2023
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 7, 2023
@dims
Copy link
Member Author

dims commented Mar 7, 2023

/assign @xing-yang @andrewsykim @nckturner @torredil @msau42 @SergeyKanzhelev

@dims
Copy link
Member Author

dims commented Mar 7, 2023

/milestone v1.27

@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Mar 7, 2023
@liggitt
Copy link
Member

liggitt commented Mar 7, 2023

weird, I expected dependency count to drop way more...

-Direct Dependencies: 208 
+Direct Dependencies: 206 
 Transitive Dependencies: 235 
 Total Dependencies: 284 
 Max Depth Of Dependencies: 23

regardless, I'm super happy to see the vendored packages drop out

@dims dims changed the title [WIP] Drop aws kubelet credential provider and cleanup aws storage e2e tests Drop aws kubelet credential provider and cleanup aws storage e2e tests Mar 7, 2023
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 7, 2023
@dims
Copy link
Member Author

dims commented Mar 7, 2023

regardless, I'm super happy to see the vendored packages drop out

+100 - am happy with 260k less lines of code in our repo!

SergeyKanzhelev
SergeyKanzhelev approved these changes Mar 7, 2023
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm

from node

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 7, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: b62c4aa017c478f74a4e1a1bb9aacf28dbf84f52

@dims
Copy link
Member Author

dims commented Mar 8, 2023

/hold for additional eyes

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2023
@xing-yang
Copy link
Contributor

lgtm

@dims
Copy link
Member Author

dims commented Mar 8, 2023

lgtm

thanks @xing-yang

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2023
@dims dims mentioned this pull request Mar 8, 2023
Closed
4 tasks
torredil
torredil approved these changes Mar 8, 2023
View reviewed changes
Copy link
Member

@torredil torredil left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, SergeyKanzhelev, torredil

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 03ff890 into kubernetes:master Mar 8, 2023
@sftim sftim mentioned this pull request Mar 8, 2023
Closed
@sftim
Copy link
Contributor

sftim commented Mar 8, 2023

Changelog text suggestion:

-Removes AWS kubelet credential provider. Please use the external kubelet credential provider binary named `ecr-credential-provider` instead.
+Removed AWS kubelet credential provider. Please use the external kubelet credential provider binary named `ecr-credential-provider` instead.

@dims
Copy link
Member Author

dims commented Mar 8, 2023

Applied the suggestion. thanks @sftim

@buckleyGI
Copy link

Also experiencing this? We got word from AWS support that this merge is the cause.
Now waiting on the fix which is a new AMI release I believe. This issue had a big impact so I hope this will give you a straw.

After updating to EKS 1.27 our EKS Managed Windows nodes don't seem to be able to pull from any AWS ECR anymore.
Pulling custom images from our own ECR fails with a 401: Unauthorized and pulling images from managed addons (like aws ebs csi driver) cross-account also fail with 401:Unauthorized.
We don't have this issue on 1.26 though and IAM permissions are all fine.

Failed to pull image REDACTED: rpc error: code = Unknown desc = failed to pull and unpack image REDACTED: failed to resolve reference REDACTED: pulling from host REDACTED failed with status code [manifests v1.19.0]: 401 Unauthorized

@dims
Copy link
Member Author

dims commented Jun 5, 2023

@buckleyGI The support team SHOULD HAVE pointed out that you do need the ecr-credential-provider now with 1.27. This is a no-op from community side, please add the binary mentioned to your image.

@buckleyGI
Copy link

Thanks @dims We haven't gotten that feedback at this point from support. We had a chat with them some hours ago so I suppose we will get guidance.

@buckleyGI buckleyGI mentioned this pull request Jun 6, 2023
Closed
wongma7 pushed a commit to wongma7/kubernetes that referenced this pull request Jul 24, 2023
@zafs23 @wongma7
--EKS-PATCH-- Update aws-sdk-go to include new regions
c15e5bc 
Description:
* Adds support for new AWS regions by updating aws-sdk-go versions

Upstream PR, Issue, KEP, etc. links:
* This patch was initially based on cherry pick of Kubernetes commit af76f3b
(kubernetes@af76f3b), which is part
of PR kubernetes#113084 (kubernetes#113084). But changes have been made to it to update the
aws-sdk-go version to a newer one that the original PR did.

If this patch is based on an upstream commit, how (if at all) do this patch and the upstream source differ?
* As mentioned above, this patch has been changed - and continues to be changed - to update the aws-sdk-go version to a
newer version. While these causes a number of differences in terms of number of lines, the only meaningful change is the
enabling of newer aws-sdk-go versions and the features that come with it.

If this patch's changes have not been added by upstream, why not?
* N/A

Other patches related to this patch:
* EKS-PATCH-Pass-region-to-sts-client.patch

Changes made to this patch after its initial creation and reasons for these changes:
* Previously, multiple patches were used to update version of aws-sdk-go. They were replaced by this one patch.
* This patch is updated whenever a new region needs to be added

Kubernetes version this patch can be dropped:
* Likely 1.27 -- kubernetes#116329

Signed-off-by: Davanum Srinivas <davanum@gmail.com>
@nickperry nickperry mentioned this pull request Aug 14, 2023
Open
@yha-sparklane yha-sparklane mentioned this pull request Mar 20, 2024
Open
@name212 name212 mentioned this pull request Apr 9, 2024
Merged
4 tasks
@deckhouse-BOaTswain deckhouse-BOaTswain mentioned this pull request Apr 11, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsafrane jsafrane jsafrane left review comments

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@torredil torredil torredil approved these changes

@justinsb justinsb Awaiting requested review from justinsb

@logicalhan logicalhan Awaiting requested review from logicalhan

Assignees

@xing-yang xing-yang

@nckturner nckturner

@SergeyKanzhelev SergeyKanzhelev

@andrewsykim andrewsykim

@msau42 msau42

@torredil torredil

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/dependency Issues or PRs related to dependency changes area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
SIG Node PR Triage
Archived in project
[sig-release] Bug Triage
Archived in project
Milestone
v1.27
Development

Successfully merging this pull request may close these issues.