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

manual rotation of CA certificates (#15444 and #19165) #19351

Closed
wants to merge 1 commit into from

Conversation

abhiTamrakar
Copy link
Contributor

@abhiTamrakar abhiTamrakar commented Feb 27, 2020

This PR is meant to address the steps for manual rotation of CA certificates.

Preview link: https://deploy-preview-19351--kubernetes-io-master-staging.netlify.com/docs/tasks/tls/manual-rotation-of-ca-certificates/

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
  • Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Feb 27, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @abhiTamrakar!

It looks like this is your first PR to kubernetes/website 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/website has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 27, 2020
@k8s-ci-robot k8s-ci-robot added language/en Issues or PRs related to English language sig/docs Categorizes an issue or PR as relevant to SIG Docs. labels Feb 27, 2020
@netlify
Copy link

netlify bot commented Feb 27, 2020

Deploy preview for kubernetes-io-master-staging ready!

Built with commit de57586

https://deploy-preview-19351--kubernetes-io-master-staging.netlify.app

@abhiTamrakar abhiTamrakar changed the title manual rotation of CA certificates #15444 and #19165 manual rotation of CA certificates #15444 and #19165 [WIP] Feb 27, 2020
@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 Feb 28, 2020
Copy link
Contributor

@zacharysarah zacharysarah left a comment

Choose a reason for hiding this comment

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

@abhiTamrakar Thanks for this addition, this is great! ✨ Seriously, this is one of the best new pages I've worked on in some time. I don't often get to work directly with page content, so thanks for making this a pleasure.

Thanks especially for following the style guide and adding callouts and capture statements. 👍

Some grammar and formatting fixes, otherwise LGTM from a docs perspective.

Note that Markdown only requires single back ticks for in-line code fencing. No need to add ```three```.

@jimangel
Copy link
Member

This is awesome! Thanks!

I think it would be worth having a quick tech review from @kubernetes/sig-cluster-lifecycle. While not directly related, it would be good to validate any disclaimers / pitfalls for performing manual CA rotation as documented.

@neolit123
Copy link
Member

thanks for sending this PR @abhiTamrakar .

@kubernetes/sig-auth-pr-reviews

we need to establish the best practice for CA rotation, even if quite manual.
currently users outside of managed solutions are left in the dark in terms of how they are supposed to rotate CA - what are the recommendation and what is the sequence of actions.

please advise on the way forward and whether this document is something that we should include.

@k8s-ci-robot k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 28, 2020
@sftim
Copy link
Contributor

sftim commented Feb 29, 2020

To let Prow understand your intentions @abhiTamrakar I am going to update the PR title to match conventions:
/retitle [WIP] manual rotation of CA certificates (#15444 and #19165)

@k8s-ci-robot k8s-ci-robot changed the title manual rotation of CA certificates #15444 and #19165 [WIP] [WIP] manual rotation of CA certificates (#15444 and #19165) Feb 29, 2020
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 29, 2020
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Good work @abhiTamrakar

Here's some feedback, including suggestions you can accept if you're happy to.

@abhiTamrakar
Copy link
Contributor Author

abhiTamrakar commented Feb 29, 2020

@zacharysarah 'a' were not getting parsed properly in the ordered list (https://github.com/kubernetes/website/pull/19351/files#diff-f7024f94c2d9c79b0e5ce9a9c0f8aaa7R111) So i changed it to numbers in the nested lists as well.
Sorry, I am not an expert in markdown, tried few other combinations for using roman numerals 'i' and versioned '1.1' with no better luck on my local machine.

@sftim
Copy link
Contributor

sftim commented Feb 29, 2020

The preview is https://deploy-preview-19351--kubernetes-io-master-staging.netlify.com/docs/tasks/tls/manual-rotation-of-ca-certificates/

(@abhiTamrakar - once I know what the URL of the preview page is for my changes, I typically amend the PR description to include a link)

@k8s-ci-robot k8s-ci-robot removed language/ko Issues or PRs related to Korean language language/pl Issues or PRs related to Polish language language/pt Issues or PRs related to Portuguese language language/ru Issues or PRs related to Russian language language/uk Issues or PRs related to Ukrainian language language/vi Issues or PRs related to Vietnamese language language/zh Issues or PRs related to Chinese language labels May 25, 2020
@abhiTamrakar
Copy link
Contributor Author

abhiTamrakar commented May 25, 2020

@sftim thanks for helping hand, @neolit123 advise worked, I knew about reset hard just forgot about git reflog.
Thanks @neolit123

I removed the labels that got added as part of the rebase with upstream.

@abhiTamrakar
Copy link
Contributor Author

@micahhausler @deads2k review please.
@sftim I squashed earlier, is it looking good now?

@sftim
Copy link
Contributor

sftim commented May 25, 2020

@abhiTamrakar after squashing and pushing you should see that https://github.com/kubernetes/website/pull/19351/commits is just one commit

It does not look as if you have squashed and force-pushed

@sftim
Copy link
Contributor

sftim commented May 25, 2020

/remove-area blog

@k8s-ci-robot k8s-ci-robot removed the area/blog Issues or PRs related to the Kubernetes Blog subproject label May 25, 2020
@abhiTamrakar
Copy link
Contributor Author

@sftim doing now.

Apply suggestions from code review

Co-Authored-By: Zach Corleissen <[email protected]>

fix typo in kubelet client certificate name

use both old and new CA for rotation

include an alternative approach for CA rotation

remove alternative approach

Apply suggestions from code review

Co-authored-by: Micah Hausler <[email protected]>

reorder some tasks to avoid restart all pods again.

reordered the CA rotation steps

nit: fixing typo cause by an extra character.

added reference to pod disruption budget
@sftim
Copy link
Contributor

sftim commented May 25, 2020

That looks squashed to me!

@sftim
Copy link
Contributor

sftim commented May 25, 2020

@kubernetes/sig-auth-pr-reviews please take a look

@abhiTamrakar
Copy link
Contributor Author

@deads2k could you please review, @micahhausler requested a review from you as well.

@neolit123
Copy link
Member

@abhiTamrakar
seem there are some minor conflicts again.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 10, 2020
@k8s-ci-robot
Copy link
Contributor

@abhiTamrakar: PR needs rebase.

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.

@abhiTamrakar
Copy link
Contributor Author

@abhiTamrakar
seem there are some minor conflicts again.

What's the recommendation here? Last time I rebased there were 2k commits introduced.

@neolit123
Copy link
Member

you seem to be using your local master branch which is quite messy.
always create a new branch for new PRs.

i'm willing to take over this PR, rather than explaining standard git workflows.
co-authorship of your commit will be preserved.

would you agree to that?

@neolit123
Copy link
Member

new PR is here:
#21651

would you agree to that?

@abhiTamrakar
if you agree, please close this one.

@abhiTamrakar
Copy link
Contributor Author

you seem to be using your local master branch which is quite messy.
always create a new branch for new PRs.

i'm willing to take over this PR, rather than explaining standard git workflows.
co-authorship of your commit will be preserved.

would you agree to that?

Yeah that's the basic mistake happened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. language/en Issues or PRs related to English language needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants