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

kubeadm: mark the phase 'experimental-cert-rotation' as deprecated #124419

Merged
merged 1 commit into from
Apr 26, 2024
Merged

kubeadm: mark the phase 'experimental-cert-rotation' as deprecated #124419

merged 1 commit into from
Apr 26, 2024

Conversation

neolit123
Copy link
Member

@neolit123 neolit123 commented Apr 20, 2024 •
edited

What type of PR is this?

/kind cleanup deprecation

What this PR does / why we need it:

  • Mark the phase 'init kubelet-finilize experimental-cert-rotation' as deprecated and print a warning if its used directly. It should be removed in 1.32.
  • Add a replacement phase 'enable-client-cert-rotation'.

(!!) We should stop using the 'experimental' and 'alpha' words in phases, clI flags and k8s object names as this complicates the graduation and users will use the feature in production regardless.

Having a 'DEPRECATED' or 'EXPERIMENTAL' text in descriptions is sufficient to indicate a status of a feature that should not be used in production.

Which issue(s) this PR fixes:

xref kubernetes/kubeadm#3046

Special notes for your reviewer:

NONE

Does this PR introduce a user-facing change?

kubeadm: mark the sub-phase of 'init kubelet-finilize' called 'experimental-cert-rotation' as deprecated and print a warning if it is used directly; it will be removed in a future release. Add a replacement sub-phase 'enable-client-cert-rotation'.

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. labels Apr 20, 2024
@neolit123
Copy link
Member Author

/triage accepted
/priority backlog

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. triage/accepted Indicates an issue or PR is ready to be actively worked on. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. priority/backlog Higher priority than priority/awaiting-more-evidence. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Apr 20, 2024
@neolit123 neolit123 mentioned this pull request Apr 20, 2024
Open
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: neolit123

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 added approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Apr 20, 2024
@neolit123
Copy link
Member Author

neolit123 commented Apr 20, 2024 •
edited

searching on github i found a couple of shell scripts that use the deprecated phase directly.
we don't mention it in authored docs at k8s.io. it's only listed in generated reference docs.

the plan is to remove it in 1.32.

@pacoxu
Copy link
Member

pacoxu commented Apr 22, 2024

searching on github i found a couple of shell scripts that use the deprecated phase directly.

Out of kubernetes community?

the plan is to remove it in 1.32.

That's OK for deprecating an experimental phase. Or a later release. Either is OK to me.

LGTM

leave to @SataQiu to take a look.

@neolit123
Copy link
Member Author

Out of kubernetes community?

not from k / k-sigs orgs.

That's OK for deprecating an experimental phase. Or a later release. Either is OK to me.

as per the rules we can remove it without deprecation. but if something says alpha or expirimental for a long time and is used in user scripts and production, then it's not alpha / experimental anymore. a great example is kube-proxy's v1alpha1 config.

@sftim
Copy link
Contributor

sftim commented Apr 22, 2024

Changelog suggestion

-kubeadm: mark the sub-phase of 'init kubelet-finilize' called 'experimental-cert-rotation' as deprecated and print a warning if its used directly. It will be removed in a future release. Add a replacement phase 'enable-client-cert-rotation'.
+kubeadm: marked a sub-phase of `kubeadm init kubelet-finalize`, named `experimental-cert-rotation`, as deprecated;
+it will be removed in a future release.
+The kubeadm tool now prints a warning if that deprecated sub-phase is used directly.
+Added a replacement sub-phase: `enable-client-cert-rotation`.

@pacoxu
Copy link
Member

pacoxu commented Apr 23, 2024

leave to @SataQiu to take a look.

/lgtm
/hold
double checked

@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 Apr 23, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 17cafb4d77c8b95bda59c0f7a305dc108f4ba9aa

SataQiu
SataQiu reviewed Apr 23, 2024
View reviewed changes
Copy link
Member

@SataQiu SataQiu left a comment

Choose a reason for hiding this comment

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

SGTM

cmd/kubeadm/app/cmd/phases/init/kubeletfinalize.go Outdated
if enableClientCertRotationRun {
return nil
}
klog.Warning("The phase 'experimental-cert-rotation' is deprecate and will be removed in a future release. " +
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
klog.Warning("The phase 'experimental-cert-rotation' is deprecate and will be removed in a future release. " +
klog.Warning("The phase 'experimental-cert-rotation' is deprecated and will be removed in a future release. " +

Copy link
Member Author

Choose a reason for hiding this comment

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

updated.

@neolit123
kubeadm: mark the phase 'experimental-cert-rotation' as deprecated
6e17cc7 
- Mark the phase 'init kubelet-finilize experimental-cert-rotation'
as deprecated and print a warning if its used directly. It should be
removed in 1.32.
- Add a replacement phase 'enable-client-cert-rotation'.

(!!) We should stop using the 'experimental' and 'alpha' names for
phases, clI flags and k8s object names as this complicates
the graduation and users will use the feature in production regardless.

Having a 'DEPRECATED' or 'EXPERIMENTAL' text in descriptions is
sufficient to indicate a status of a feature that should not be used
in production.
@neolit123 neolit123 force-pushed the 1.31-deprecate-experimental-cert-rotation branch from 518c51a to 6e17cc7 Compare April 23, 2024 10:54
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 23, 2024
@neolit123
Copy link
Member Author

/retest

SataQiu
SataQiu reviewed Apr 26, 2024
View reviewed changes
Copy link
Member

@SataQiu SataQiu 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 Apr 26, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 70636b90e5a2fb9003b1a0644fa4afc8668548d0

@pacoxu
Copy link
Member

pacoxu commented Apr 26, 2024

/unhold

@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 Apr 26, 2024
@k8s-triage-robot
Copy link

The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.

This bot retests PRs for certain kubernetes repos according to the following rules:

  • The PR does have any do-not-merge/* labels
  • The PR does not have the needs-ok-to-test label
  • The PR is mergeable (does not have a needs-rebase label)
  • The PR is approved (has cncf-cla: yes, lgtm, approved labels)
  • The PR is failing tests required for merge

You can:

/retest

@k8s-ci-robot k8s-ci-robot merged commit ba05a8d into kubernetes:master Apr 26, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.31 milestone Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SataQiu SataQiu SataQiu left review comments

@chendave chendave Awaiting requested review from chendave

@pacoxu pacoxu Awaiting requested review from pacoxu

Assignees

@pacoxu pacoxu

@SataQiu SataQiu

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm 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. kind/deprecation Categorizes issue or PR as related to a feature/enhancement marked for deprecation. 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 Denotes a PR that will be considered when it comes time to generate release notes. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. 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
None yet
Milestone
v1.31
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@neolit123 @k8s-ci-robot @pacoxu @sftim @k8s-triage-robot @SataQiu