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

csr: add resync to csr approver #49788

Merged
merged 1 commit into from
Aug 10, 2017
Merged

csr: add resync to csr approver #49788

merged 1 commit into from
Aug 10, 2017
+14 −2

Conversation

mikedanese
Copy link
Member

@mikedanese mikedanese commented Jul 28, 2017 •
edited
Loading

fixes #49787

Fix an issue where if a CSR is not approved initially by the SAR approver is not retried.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 28, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed labels Jul 28, 2017
liggitt
liggitt reviewed Jul 28, 2017
View reviewed changes
pkg/controller/certificates/approver/sarapprove.go Outdated
@@ -33,6 +34,10 @@ import (
"k8s.io/kubernetes/pkg/features"
)

// Changes outside of the kubernetes universe could cause the approval decision
// to change. We get around this be frequently resyncing.
Copy link
Member

Choose a reason for hiding this comment

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

I definitely wouldn't expect resyncs this frequently

Copy link
Member Author

Choose a reason for hiding this comment

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

Picked 15 seconds for initial testing but I was hoping to discuss what would be reasonable in this review. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Our use case is sometimes nodes post a CSR before the RBAC role bindings are initialize during cluster start up. In that particular case, 15s would be great. In the more general case, a few minutes seems appropriate.

If there are a lot of CSRs in the system, they will all be reprocessed with each resync, right? Approved/Denied ones will be quickly processed, but they will all go through this queue each cycle, right?

@k8s-github-robot k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jul 28, 2017
jcbsmpsn
jcbsmpsn reviewed Jul 28, 2017
View reviewed changes
pkg/controller/certificates/approver/sarapprove.go Outdated
@@ -33,6 +34,10 @@ import (
"k8s.io/kubernetes/pkg/features"
)

// Changes outside of the kubernetes universe could cause the approval decision
Copy link
Contributor

Choose a reason for hiding this comment

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

More like outside the state of this controller. Adding a new RBAC role binding is not exactly outside the Kubernetes universe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not just rbac. An IAM change could require resyncing the controller.

@mikedanese
Copy link
Member Author

@liggitt @jcbsmpsn I pushed an alternative approach that relies on the controller default backoff which is:

// DefaultControllerRateLimiter is a no-arg constructor for a default rate limiter for a workqueue.  It has
// both overall and per-item rate limitting.  The overall is a token bucket and the per-item is exponential
func DefaultControllerRateLimiter() RateLimiter {
	return NewMaxOfRateLimiter(
		NewItemExponentialFailureRateLimiter(5*time.Millisecond, 1000*time.Second),
		// 10 qps, 100 bucket size.  This is only for retry speed and its only the overall factor (not per item)
		&BucketRateLimiter{Bucket: ratelimit.NewBucketWithRate(float64(10), int64(100))},
	)
}

Basically recognized certificates will be continuously with a backoff to ~15 minutes.

@mikedanese mikedanese force-pushed the resync-csr branch 2 times, most recently from d26f7f3 to dd6c32a Compare August 2, 2017 00:33
@kubernetes kubernetes deleted a comment from k8s-ci-robot Aug 3, 2017
@jcbsmpsn
Copy link
Contributor

jcbsmpsn commented Aug 9, 2017

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 49787

The full list of commands accepted by this bot can be found here.

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

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2017
@mikedanese mikedanese added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 9, 2017
@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 49615, 49321, 49982, 49788, 50355)

@k8s-github-robot k8s-github-robot merged commit 319bef2 into kubernetes:master Aug 10, 2017
@mikedanese mikedanese deleted the resync-csr branch August 10, 2017 15:56
@mikedanese mikedanese mentioned this pull request Aug 14, 2017
Merged
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@wojtek-t wojtek-t added this to the v1.7 milestone Aug 14, 2017
@wojtek-t wojtek-t added cherrypick-candidate cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. labels Aug 14, 2017
k8s-github-robot pushed a commit that referenced this pull request Aug 16, 2017
Merge pull request #50623 from mikedanese/automated-cherry-pick-of-#4…
4f113c8 
…9788-release-1.7

Automatic merge from submit-queue

Automated cherry pick of #49788 release 1.7

Automated cherry pick of #49788 release 1.7

Makes cluster create without ABAC less flaky

Fixes #49787
@k8s-cherrypick-bot
Copy link

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jcbsmpsn jcbsmpsn jcbsmpsn left review comments

@liggitt liggitt liggitt left review comments

Assignees

@liggitt liggitt

@deads2k deads2k

@jcbsmpsn jcbsmpsn

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

Allow configuring resync period for CSR approver
10 participants
@mikedanese @jcbsmpsn @k8s-github-robot @fejta-bot @k8s-cherrypick-bot @liggitt @caesarxuchao @deads2k @wojtek-t @k8s-ci-robot