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

Do not register Kubelet In tree credential provider if external provider is enabled #102951

Closed
wants to merge 1 commit into from
Closed

Do not register Kubelet In tree credential provider if external provider is enabled #102951

wants to merge 1 commit into from

Conversation

adisky
Copy link
Contributor

@adisky adisky commented Jun 17, 2021 •
edited

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Kubelet registers credential provider while startup, This PR proposes to not register in-tree credential providers if feature flag for external credential provider is enabled.
Secondly if both external and intree registered then on any error from external providers it falls back to in tree which makes it difficult to catch any errors from external providers.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

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

@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. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. 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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 17, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: adisky
To complete the pull request process, please assign derekwaynecarr, liggitt after the PR has been reviewed.
You can assign the PR to them by writing /assign @derekwaynecarr @liggitt in a comment when ready.

The full list of commands accepted by this bot can be found 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

@adisky adisky force-pushed the disable-intree-credential-provider branch 2 times, most recently from 1a1ee15 to c549568 Compare June 17, 2021 14:08
@adisky adisky changed the title [WIP] Testing disable Intree credential providers if external providers are enabled Do not register Kubelet In tree credential provider if external provider is enabled Jun 17, 2021
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jun 17, 2021
@ehashman ehashman added this to Triage in SIG Node PR Triage Jun 21, 2021
ehashman
ehashman reviewed Jun 21, 2021
View reviewed changes
Copy link
Member

@ehashman ehashman left a comment

Choose a reason for hiding this comment

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

/kind cleanup
/priority backlog
/triage accepted
/skip

cmd/kubelet/app/options/globalflags.go Outdated Show resolved Hide resolved
pkg/credentialprovider/aws/aws_credentials.go
@@ -48,7 +48,7 @@ var (

// init registers a credential provider for each registryURLTemplate and creates
// an ECR token getter factory with a new cache to store token getters
func init() {
func Register() {
Copy link
Member

Choose a reason for hiding this comment

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

Comments need to be updated... is Register part of an interface somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, No it is not part of any interface. Introduced this to have collective initialisation at
pkg/credentialprovider/register/register.go

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/backlog Higher priority than priority/awaiting-more-evidence. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. 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. labels Jun 21, 2021
@ehashman ehashman moved this from Triage to Waiting on Author in SIG Node PR Triage Jun 21, 2021
@adisky
Disable Intree credential providers
7753587 
disable intree credential providers if fetaure flag for external
credential provider is enabled

Signed-off-by: Aditi Sharma <adi.sky17@gmail.com>
@adisky adisky force-pushed the disable-intree-credential-provider branch from c549568 to 7753587 Compare June 22, 2021 12:53
ehashman
ehashman reviewed Jun 22, 2021
View reviewed changes
Copy link
Member

@ehashman ehashman 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 k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2021
@ehashman
Copy link
Member

/retest

@ehashman ehashman moved this from Waiting on Author to Needs Approver in SIG Node PR Triage Jun 22, 2021
@adisky
Copy link
Contributor Author

adisky commented Jun 25, 2021

cc @andrewsykim @cheftako @DangerOnTheRanger

@andrewsykim
Copy link
Member

@adisky what are your thoughts on this PR? #102507

I think they're trying to introduce similar behaviors

@DangerOnTheRanger
Copy link
Contributor

This is pretty similar to #102507 as @andrewsykim already noted - the other PR is an implementation of an already-approved KEP as well.

@adisky
Copy link
Contributor Author

adisky commented Jun 28, 2021 •
edited

@andrewsykim @DangerOnTheRanger It is pretty similar I agree, they are introducing same behaviour

IMO intree providers should be disabled in both the cases

@adisky
Copy link
Contributor Author

adisky commented Jun 28, 2021

closing this in favour of #102507

@adisky
Copy link
Contributor Author

adisky commented Jun 28, 2021

/close

SIG Node PR Triage automation moved this from Needs Approver to Done Jun 28, 2021
@k8s-ci-robot
Copy link
Contributor

@adisky: Closed this PR.

In response to this:

/close

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.

@adisky adisky deleted the disable-intree-credential-provider branch April 4, 2022 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ehashman ehashman ehashman left review comments

@andyzhangx andyzhangx Awaiting requested review from andyzhangx

@dims dims Awaiting requested review from dims

Assignees

@ehashman ehashman

Labels
area/kubelet 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. priority/backlog Higher priority than priority/awaiting-more-evidence. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Node PR Triage
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@adisky @k8s-ci-robot @ehashman @andrewsykim @DangerOnTheRanger