🐛 fix issue when webhook server refreshing cert #260

mengqiy · 2018-12-18T21:22:33Z

fixes #191
fixes #192

Tests are on the way.

mengqiy · 2018-12-18T21:23:08Z

k8s-ci-robot · 2018-12-18T21:23:09Z

@mengqiy: GitHub didn't allow me to request PR reviews from the following users: anfernee.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @anfernee

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.

anfernee · 2018-12-19T02:06:15Z

/lgtm

it doesn't fix #191 tho.

k8s-ci-robot · 2018-12-19T02:06:23Z

@anfernee: changing LGTM is restricted to assignees, and only kubernetes-sigs/controller-runtime repo collaborators may be assigned issues.

In response to this:

/lgtm

it doesn't fix #191 tho.

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.

droot

The change looks good. will wait for the tests before lgtm.

mengqiy · 2018-12-19T21:28:49Z

/hold
hold it until the tests are ready.

mengqiy · 2018-12-19T21:54:44Z

Made some additional changes in pkg/webhook/server.go and added tests.

mengqiy · 2018-12-19T22:02:45Z

it doesn't fix #191 tho.

@anfernee Explained in #191 :)

@anfernee @droot PTAL

anfernee · 2018-12-19T22:08:30Z

pkg/webhook/server_test.go

@@ -0,0 +1,148 @@
+/*


can you add a test case to test the case where the server exceeds cert refresh interval? you can change defaultCertRefreshInterval to a smaller interview. I am worried that if you start a server on the same port immediately after shutdown, you will have port conflict.

IIUC
https://github.com/kubernetes-sigs/controller-runtime/pull/260/files#diff-8b0412ec4fd52af1419bd0fcd0f2e101R121 is doing what you ask. The server restarts multiple times and rotates the certs.

anfernee · 2018-12-19T22:09:23Z

pkg/webhook/server.go

 			go serveFn()
 		case <-stop:
 			return nil
 		case e := <-errCh:
-			return e


ideally if you start server here, it should just work.

IMHO if there is an unexpected error, we should surface the error instead of keeping retrying.

anfernee · 2018-12-19T22:11:21Z

pkg/webhook/server.go

 				continue
 			}
 			log.Info("server is shutting down to reload the certificates.")
-			err = srv.Shutdown(context.Background())
+			shutdownHappend = true
+			err = s.httpServer.Shutdown(context.Background())


My understanding is Shutdown is async, meaning return of shutdown doesn't necessarily mean the server is already done. But return of ListenAndServeTLS guarantees it.

Shutdown gracefully shuts down the server without interrupting any active connections. Shutdown works by first closing all open listeners, then closing all idle connections, and then waiting indefinitely for connections to return to idle and then shut down

Per https://golang.org/pkg/net/http/#Server.Shutdown, it is graceful shutdown and synchronized call. Did I miss anything?

I am confused of those 2 methods, you are right. pls ignore it.

anfernee · 2018-12-19T22:11:46Z

pkg/webhook/server.go

 			if err != nil {
 				log.Error(err, "encountering error when shutting down")
 				return err
 			}
+			timer = time.Tick(wait.Jitter(defaultCertRefreshInterval, 0.1))
 			go serveFn()


this might have port conflict.

I think the question here is that shutting down the server will unbind the port, but if it will complete in time before the server tries to bind the same port next time.
I have been searching online for this question for quite a while. But I'm still not sure what the 100% correct thing to do here :/
It seems when the Listener is closed, it should have unbinded the port.
Some tests I added in this PR rotates the cert and reloads the server for multiple times. I haven't seen any issue about port conflict.

I'm open to suggestions if you have a better solution. If not, I will probably merge it as is :)

droot · 2019-01-11T19:57:22Z

pkg/webhook/server_test.go

+			}
+
+			cg := &generator.SelfSignedCertGenerator{}
+			s.CertDir, err = ioutil.TempDir("/tmp", "controller-runtime-")


Are we doing cleanup of this temp dir ?

Good catch.
Added code to cleanup the temp dir.
https://github.com/kubernetes-sigs/controller-runtime/pull/260/files#diff-8b0412ec4fd52af1419bd0fcd0f2e101R159

mengqiy · 2019-01-15T23:29:37Z

PTAL

anfernee · 2019-01-16T01:16:35Z

/lgtm

k8s-ci-robot · 2019-01-16T01:16:42Z

@anfernee: changing LGTM is restricted to assignees, and only kubernetes-sigs/controller-runtime repo collaborators may be assigned issues.

In response to this:

/lgtm

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 · 2019-01-16T23:00:13Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: mengqiy

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:

OWNERS

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 added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 18, 2018

k8s-ci-robot requested review from droot and pwittrock December 18, 2018 21:22

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Dec 18, 2018

mengqiy changed the title ~~✨ fix issue when webhook server refreshing cert~~ [WIP] ✨ fix issue when webhook server refreshing cert Dec 18, 2018

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 Dec 18, 2018

mengqiy force-pushed the fixwebhookserver branch from 5b51c4f to e05117f Compare December 18, 2018 22:10

droot reviewed Dec 19, 2018

View reviewed changes

mengqiy force-pushed the fixwebhookserver branch from e05117f to 5c464d1 Compare December 19, 2018 21:16

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 19, 2018

mengqiy mentioned this pull request Dec 19, 2018

RefreshCert should be based on the cert expire time, not after 180 days #191

Closed

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 19, 2018

mengqiy force-pushed the fixwebhookserver branch from 5c464d1 to 6c8993d Compare December 19, 2018 21:48

mengqiy changed the title ~~[WIP] ✨ fix issue when webhook server refreshing cert~~ 🐛 fix issue when webhook server refreshing cert Dec 19, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 19, 2018

anfernee reviewed Dec 19, 2018

View reviewed changes

mengqiy force-pushed the fixwebhookserver branch 2 times, most recently from 43a3f31 to acd82f6 Compare January 9, 2019 21:36

droot reviewed Jan 11, 2019

View reviewed changes

🐛 fix issue when webhook server refreshing cert

09998a3

mengqiy force-pushed the fixwebhookserver branch from acd82f6 to 09998a3 Compare January 12, 2019 00:04

droot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 16, 2019

droot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 16, 2019

k8s-ci-robot merged commit d561f55 into kubernetes-sigs:master Jan 16, 2019

mengqiy deleted the fixwebhookserver branch February 19, 2019 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 fix issue when webhook server refreshing cert #260

🐛 fix issue when webhook server refreshing cert #260

mengqiy commented Dec 18, 2018 •

edited

Loading

mengqiy commented Dec 18, 2018

k8s-ci-robot commented Dec 18, 2018

anfernee commented Dec 19, 2018

k8s-ci-robot commented Dec 19, 2018

droot left a comment

mengqiy commented Dec 19, 2018

mengqiy commented Dec 19, 2018

mengqiy commented Dec 19, 2018

anfernee Dec 19, 2018

mengqiy Dec 19, 2018

anfernee Dec 19, 2018

mengqiy Dec 19, 2018

anfernee Dec 19, 2018

mengqiy Jan 8, 2019

anfernee Jan 16, 2019

anfernee Dec 19, 2018

mengqiy Jan 9, 2019

droot Jan 11, 2019

mengqiy Jan 12, 2019

mengqiy commented Jan 15, 2019

anfernee commented Jan 16, 2019

k8s-ci-robot commented Jan 16, 2019

k8s-ci-robot commented Jan 16, 2019

🐛 fix issue when webhook server refreshing cert #260

🐛 fix issue when webhook server refreshing cert #260

Conversation

mengqiy commented Dec 18, 2018 • edited Loading

mengqiy commented Dec 18, 2018

k8s-ci-robot commented Dec 18, 2018

anfernee commented Dec 19, 2018

k8s-ci-robot commented Dec 19, 2018

droot left a comment

Choose a reason for hiding this comment

mengqiy commented Dec 19, 2018

mengqiy commented Dec 19, 2018

mengqiy commented Dec 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mengqiy commented Jan 15, 2019

anfernee commented Jan 16, 2019

k8s-ci-robot commented Jan 16, 2019

k8s-ci-robot commented Jan 16, 2019

mengqiy commented Dec 18, 2018 •

edited

Loading