Improve dynamic cert file change detection #104102

tnqn · 2021-08-03T14:04:07Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

DynamicFileCAContent and DynamicCertKeyPairContent used periodical job to check whether the file content has changed, leading to 1 minute of delay in worst case. This patch improves it by leveraging fsnotify watcher. The content change will be reflected immediately.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

This is also a TODO @deads2k left in the code.

Does this PR introduce a user-facing change?

CA, certificate and key bundles for the generic-apiserver based servers will be reloaded immediately after the files are changed.

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

caesarxuchao · 2021-08-03T20:11:54Z

/assign @deads2k

could you help triage? Thanks.

/triage accepted

deads2k · 2021-08-03T20:16:45Z

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.go

-
-	// TODO this can be wired to an fsnotifier as well.
+	// start the loop that watches the CA file until stopCh is closed.
+	go wait.NonSlidingUntil(func() { c.watchCAFile(stopCh) }, time.Second, stopCh)


Since watchCAFile only returns on error, let's make the retry every minute instead of every second to avoid warm loops.

Why non-sliding? I think we want the timer to interval to start when the function ends, not when it starts.

Thanks for the review. watchCAFile also returns on file removal. The 1 second interval and non-sliding is for the mounted volume case: if a secret includes the CA bundle is mounted in a Pod, updating the secret leads to a file replacement, not content update. fsnotify watcher will receive a fsnotify.Remove event of the old file. If we use one minute as the interval and slidingUntil, it will restart the watch after one minute even the new file is ready immediately.
Maybe I could handle file removal in watchCAFile and start a new watch immediately without relying on the external loop. Then the external loop handles error case only and can wait longer interval.

I have made the change, PTAL.

deads2k · 2021-08-04T20:19:30Z

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.go

+				if err := w.Add(c.filename); err != nil {
+					return fmt.Errorf("error adding watch for file %s: %v", c.filename, err)
+				}
+				c.queue.Add(workItemKey)


Do this before you remove the file and add it back. Doing it here means you could return an error and not trigger the workqueue for another minute.

ditto, a trigger after starting watch is required to prevent the same race condition above. There is also a trigger in L188, which will trigger reload as long as the current file has any update. The two triggers can be merged by using defer but the routine will need to be wrapped in a function then. Please let me know which way you would prefer.

I updated this a little to guarantee a resync will be triggered after any event is received and it will be executed after starting new watch if there is.

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.go

deads2k · 2021-08-04T20:22:01Z

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_serving_content.go

+				if err := w.Add(e.Name); err != nil {
+					return fmt.Errorf("error adding watch for file %s: %v", e.Name, err)
+				}
+				c.queue.Add(workItemKey)


remind me how cert/key mismatches are handled. A code reference comment right here about how it will handled when one is updated before the other would be helpful. Previously, at a one minute polling cycle it was pretty unlikely, but I'm hoping past-me did something clever.

You definitely handled this well: the certificate and key are validated before they are used. If either is missing or they don't match, an error will be returned and workqueue will reschedule it later with rate limiter. I will add comment to explain it.

kubernetes/staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_serving_content.go

Lines 79 to 96 in f1c8176

func (c *DynamicCertKeyPairContent) loadCertKeyPair() error {

cert, err := ioutil.ReadFile(c.certFile)

if err != nil {

return err

}

key, err := ioutil.ReadFile(c.keyFile)

if err != nil {

return err

}

if len(cert) == 0 || len(key) == 0 {

return fmt.Errorf("missing content for serving cert %q", c.Name())

}

// Ensure that the key matches the cert and both are valid

_, err = tls.X509KeyPair(cert, key)

if err != nil {

return err

}

test/integration/apiserver/certreload/certreload_test.go

deads2k · 2021-08-05T13:52:53Z

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.go

+func (c *DynamicFileCAContent) handleWatchEvent(e fsnotify.Event, w *fsnotify.Watcher) error {
+	// This should be executed after restarting the watch (if applicable) to ensure no file event will be missing.
+	defer c.queue.Add(workItemKey)
+	if e.Op&(fsnotify.Remove|e.Op&fsnotify.Rename) > 0 {


since you'll be pushing an update anyway, the project standard is to invert this check and return nil, so i think that's

if condition <=0{ return nil } w.remove w.add

done, thanks for the suggestion.

DynamicFileCAContent and DynamicCertKeyPairContent used periodical job to check whether the file content has changed, leading to 1 minute of delay in worst case. This patch improves it by leveraging fsnotify watcher. The content change will be reflected immediately.

deads2k · 2021-08-05T20:04:28Z

/lgtm
/approve

k8s-ci-robot · 2021-08-05T20:04:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, tnqn

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:

~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k]
~~test/integration/apiserver/OWNERS~~ [deads2k]

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 requested a review from enj August 3, 2021 14:04

k8s-ci-robot added the area/apiserver label Aug 3, 2021

k8s-ci-robot requested review from SataQiu and a team August 3, 2021 14:04

k8s-ci-robot assigned deads2k Aug 3, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 3, 2021

deads2k reviewed Aug 3, 2021

View reviewed changes

tnqn force-pushed the dynamic-file branch 2 times, most recently from bb24290 to 5eeae97 Compare August 4, 2021 06:07

deads2k reviewed Aug 4, 2021

View reviewed changes

staging/src/k8s.io/apiserver/pkg/server/dynamiccertificates/dynamic_cafile_content.go Show resolved Hide resolved

deads2k reviewed Aug 4, 2021

View reviewed changes

test/integration/apiserver/certreload/certreload_test.go Show resolved Hide resolved

tnqn force-pushed the dynamic-file branch from 5eeae97 to a93e1eb Compare August 5, 2021 12:05

dims removed the area/dependency Issues or PRs related to dependency changes label Aug 5, 2021

deads2k reviewed Aug 5, 2021

View reviewed changes

tnqn force-pushed the dynamic-file branch from a93e1eb to 675fc97 Compare August 5, 2021 17:20

k8s-ci-robot added the area/dependency Issues or PRs related to dependency changes label Aug 5, 2021

tnqn force-pushed the dynamic-file branch from 675fc97 to 3cfe3d0 Compare August 5, 2021 17:47

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2021

k8s-ci-robot merged commit 98bd200 into kubernetes:master Aug 5, 2021

k8s-ci-robot added this to the v1.23 milestone Aug 5, 2021

tnqn deleted the dynamic-file branch August 6, 2021 00:59

enj mentioned this pull request Oct 18, 2022

Implements hot reload of the KMS EncryptionConfiguration #112050

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve dynamic cert file change detection #104102

Improve dynamic cert file change detection #104102

tnqn commented Aug 3, 2021

caesarxuchao commented Aug 3, 2021

deads2k Aug 3, 2021

deads2k Aug 3, 2021

tnqn Aug 4, 2021

tnqn Aug 4, 2021

deads2k Aug 4, 2021

tnqn Aug 5, 2021

tnqn Aug 5, 2021

deads2k Aug 4, 2021

tnqn Aug 5, 2021

deads2k Aug 5, 2021

tnqn Aug 5, 2021

deads2k commented Aug 5, 2021

k8s-ci-robot commented Aug 5, 2021

	func (c *DynamicCertKeyPairContent) loadCertKeyPair() error {
	cert, err := ioutil.ReadFile(c.certFile)
	if err != nil {
	return err
	}
	key, err := ioutil.ReadFile(c.keyFile)
	if err != nil {
	return err
	}
	if len(cert) == 0 \|\| len(key) == 0 {
	return fmt.Errorf("missing content for serving cert %q", c.Name())
	}

	// Ensure that the key matches the cert and both are valid
	_, err = tls.X509KeyPair(cert, key)
	if err != nil {
	return err
	}

Improve dynamic cert file change detection #104102

Improve dynamic cert file change detection #104102

Conversation

tnqn commented Aug 3, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

caesarxuchao commented Aug 3, 2021

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

deads2k commented Aug 5, 2021

k8s-ci-robot commented Aug 5, 2021