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

[WIP]When kubelet enable rotate-certificates and bootstrap, request CSR in foreground #112240

Closed

Conversation

chenk008
Copy link
Contributor

@chenk008 chenk008 commented Sep 5, 2022

What type of PR is this?

/kind feature

What this PR does / why we need it:

When kubelet enable rotate-certificates and bootstrap, request CSR in foreground. It can help to reduce the uncertainty of node register time.

Which issue(s) this PR fixes:

Fixes #112170

Special notes for your reviewer:

Now when kubelet enable rotate-certificates and bootstrap-kubeconfig, it will request CSR in background. With the delayed CSR approval, kubelet use the bootstrap token to register, but the bootstrap token has no permission. It will need wait 10s to change client transport tls config.

Requesting CSR in foreground, even the CSR delayed a seconds, the kubelet will register successfully Immediately after the CSR approved. It don't need to wait 10s for the certificate check.

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. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/feature Categorizes issue or PR as related to a new feature. labels Sep 5, 2022
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 5, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: chenk008 (2b193d2663491a3949c5f749460fcdce6c6458d9)

@k8s-ci-robot k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 5, 2022
@k8s-ci-robot
Copy link
Contributor

@chenk008: 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-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 5, 2022
@k8s-ci-robot
Copy link
Contributor

Hi @chenk008. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Sep 5, 2022
@chenk008 chenk008 changed the title [WIP]Request CRS in foreground [WIP]When kubelet enable rotate-certificates and bootstrap, request CSR in foreground Sep 5, 2022
@k8s-ci-robot k8s-ci-robot added area/kubelet sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. 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 Sep 5, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chenk008
Once this PR has been reviewed and has the lgtm label, please assign mikedanese for approval by writing /assign @mikedanese in a comment. For more information see:The Kubernetes Code Review Process.

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 5, 2022
@leilajal
Copy link
Contributor

leilajal commented Sep 6, 2022

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Sep 6, 2022
@enj enj added this to Needs Triage in SIG Auth Old Sep 12, 2022
@liggitt
Copy link
Member

liggitt commented Oct 17, 2022

Requesting CSR in foreground, even the CSR delayed a seconds, the kubelet will register successfully Immediately after the CSR approved. It don't need to wait 10s for the certificate check.

It's unclear to me what the implications of blocking on this are for running static pods. I know putting this in the background was explicitly done so that static pods would not be impacted by CSR requests. xref #69890

/cc @smarterclayton

/hold

@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 Oct 17, 2022
@chenk008
Copy link
Contributor Author

chenk008 commented Nov 28, 2022

@liggitt
Sorry, I just learned the background are for running static pods.

In my scenario:

  1. My cluster enableS node autoscale, we hope the node can be ready in 10 seconds after the kubelet process started.
  2. The components (e.g. CNI, kube-proxy) are deployed as daemonset,
  3. The node ready condition depends on CNI pod.
  4. We want the kubelet finish registration as soon as possible, so that these components pods can be created. We expect the CNI pod to be created in 3 seconds.

For now, it will request CSR in background, and check certificates every 10 seconds. https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/certificate/transport.go#L57. Once the CSR delayed, it has to wait for the next cycle, it is too long in our scenario.

How about the following options?

  1. Adjust the certificates check period to 1 second. In most cases, the CSR will be approved in 1 second.
  2. Add other mechanisms to watch the certificates change (i.e. file inotify)

WDYT?

@liggitt
Copy link
Member

liggitt commented Dec 8, 2022

is the issue that this line allows successfully establishing connections with no certificate while waiting for the initial certificate, and those connections persist up to 10 seconds once the initial CSR is approved/signed/issued?

if cert == nil {
return &tls.Certificate{Certificate: nil}, nil
}

It seems reasonable to check more frequently until we get non-null initial certificate... opened #114367 as a possible example of that

@dims
Copy link
Member

dims commented Dec 12, 2022

This PR has the label work-in-progress, please revisit to see if you still need this, please close if not

@bart0sh bart0sh moved this from Triage to Waiting on Author in SIG Node PR Triage Jan 2, 2023
@bart0sh bart0sh moved this from Waiting on Author to WIP in SIG Node PR Triage Jan 27, 2023
@enj
Copy link
Member

enj commented Jan 30, 2023

/close
In favor of #114367

@k8s-ci-robot
Copy link
Contributor

@enj: Closed this PR.

In response to this:

/close
In favor of #114367

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.

SIG Node PR Triage automation moved this from WIP to Done Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/feature Categorizes issue or PR as related to a new feature. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. 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-none Denotes a PR that doesn't merit a release note. sig/auth Categorizes an issue or PR as relevant to SIG Auth. 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.
Projects
Archived in project
SIG Auth Old
Needs Triage
Development

Successfully merging this pull request may close these issues.

When kubelet enable rotate-certificates and bootstrap, request CSR in foreground
6 participants