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

refactor certificate controller to break it into two parts #45514

Merged
merged 1 commit into from
May 24, 2017
Merged

refactor certificate controller to break it into two parts #45514

merged 1 commit into from
May 24, 2017

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented May 8, 2017 •
edited
Loading

Break pkg/controller/certificates into:

  • pkg/controller/certificates/approver: containing the group approver
  • pkg/controller/certificates/signer: containing the local signer
  • pkg/controller/certificates: containing shared infrastructure
Break the 'certificatesigningrequests' controller into a 'csrapprover' controller and 'csrsigner' controller.

@mikedanese mikedanese self-assigned this May 8, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 8, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels May 8, 2017
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 8, 2017
@mikedanese mikedanese force-pushed the cert-refactor branch 2 times, most recently from 219be60 to 887d43b Compare May 8, 2017 23:28
@mikedanese mikedanese added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 8, 2017
@luxas
Copy link
Member

luxas commented May 9, 2017

Fixes: #45302 I suppose?

Does this help #45030 as well?

@pipejakob
Copy link
Contributor

Looks good so far. Will you continue the pattern from the GKESigner of adding events to the CSR on these different failure conditions? Would be very useful to trace/diagnose cluster issues.

Also, is the end goal to pull these out into completely different processes, or just separate goroutines in the controller-manager?

@mikedanese
Copy link
Member Author

@pipejakob that's a good idea. I'm going to encorporate it in a follow up.

@mikedanese
Copy link
Member Author

#46130

@mikedanese mikedanese changed the title WIP: refactor certificate controller refactor certificate controller to break it into two parts May 19, 2017
@pipejakob
Copy link
Contributor

Cool. Are you looking for a stronger LGTM before restoring the tests, or are you set for now?

@mikedanese
Copy link
Member Author

Not yet. Sorry.

@mikedanese
Copy link
Member Author

Will ping explicitly.

@mikedanese
Copy link
Member Author

@pipejakob this is ready for final review. I partially restored controller_test.go but a lot of what it is testing is duplicated in cfssl_signer_test.go, so I didn't reinclude that. PTAL.

@mikedanese mikedanese mentioned this pull request May 23, 2017
Merged
@pipejakob
Copy link
Contributor

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@pipejakob
Copy link
Contributor

Federation failure looks to be tracked in #45978.

pipejakob
pipejakob reviewed May 23, 2017
View reviewed changes
pkg/controller/certificates/doc.go
*/

// Package certificates contains logic for watching and synchronizing
// CertificateSigningRequests.
Copy link
Contributor

Choose a reason for hiding this comment

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

With this file gone, and a lot of the code being moved into two new packages, they could each use their own package-level godoc (either inline in an existing file, or a doc.go file).

Copy link
Member Author

Choose a reason for hiding this comment

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

Added godoc to the packages.

pipejakob
pipejakob reviewed May 23, 2017
View reviewed changes
pkg/controller/certificates/approver/groupapprove_test.go

import (
"testing"

certificates "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
api "k8s.io/kubernetes/pkg/apis/certificates/v1beta1"
)

func TestHasKubeletUsages(t *testing.T) {
Copy link
Contributor

@pipejakob pipejakob May 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The only tests I can see here are for the hasExactUsages() helper, but not for the main groupapprover code: handle() and autoApprove(). I would have expected more tests to break with this refactoring; is code coverage just really low here? Can you punch it up as part of this refactoring?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a bunch of test in #45619 for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I hadn't even seen #45619. I'll take a look.

pipejakob
pipejakob reviewed May 23, 2017
View reviewed changes
Copy link
Contributor

@pipejakob pipejakob left a comment

Choose a reason for hiding this comment

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

Mostly LGTM, just a few minor comments.

@k8s-github-robot k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 23, 2017
@mikedanese
Copy link
Member Author

@pipejakob federation is not a blocking build. I think I addressed all comments.

@pipejakob
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 23, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese, pipejakob

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 23, 2017
@pipejakob
Copy link
Contributor

Oh actually, @liggitt had a good comment in #45619. You renamed the controllers (and made the names shorter, as he recommended), but I don't see a release note about breaking backwards compatibility with the CLI.

@mikedanese mikedanese added release-note Denotes a PR that will be considered when it comes time to generate release notes. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-none Denotes a PR that doesn't merit a release note. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels May 23, 2017
@mikedanese
Copy link
Member Author

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 45514, 45635)

@k8s-github-robot k8s-github-robot merged commit 5be7a6a into kubernetes:master May 24, 2017
@k8s-ci-robot
Copy link
Contributor

@mikedanese: The following test(s) failed:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-etcd3 f04ce3c link @k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@liggitt liggitt mentioned this pull request May 26, 2017
Closed
k8s-github-robot pushed a commit that referenced this pull request May 31, 2017
Merge pull request #45619 from mikedanese/cert-sar
4e531f6 
Automatic merge from submit-queue (batch tested with PRs 46635, 45619, 46637, 45059, 46415)

migrate group approver to use subject access reviews

WIP, needs test and changes to kubeadm

depends on #45514
@mikedanese mikedanese deleted the cert-refactor branch November 16, 2018 23:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pipejakob pipejakob pipejakob left review comments

Assignees

@pipejakob pipejakob

@mikedanese mikedanese

@jcbsmpsn jcbsmpsn

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mikedanese @k8s-reviewable @luxas @pipejakob @k8s-github-robot @k8s-ci-robot @jcbsmpsn