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

Support TLS certificate updates #442

Merged
merged 3 commits into from
Mar 11, 2021
Merged

Support TLS certificate updates #442

merged 3 commits into from
Mar 11, 2021

Conversation

marquiz
Copy link
Contributor

@marquiz marquiz commented Feb 18, 2021

Leveraging some of the work done on worker config file watcher lately, implement filesystem watcher of TLS certificate files for nfd-master and nfd-worker.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 18, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: marquiz

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:

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 k8s-ci-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. labels Feb 18, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Feb 18, 2021

My testing is still fairly limited
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 18, 2021
@marquiz marquiz mentioned this pull request Feb 18, 2021
Merged
3 tasks
@marquiz marquiz force-pushed the devel/cert-watcher branch 2 times, most recently from ee2e92b to b64c233 Compare February 18, 2021 21:22
@mythi
Copy link
Contributor

mythi commented Feb 22, 2021

How about re-using certWatcher by first moving it to k8s.io/utils. I had already started that conversation: kubernetes-sigs/controller-runtime#1370

@marquiz
Copy link
Contributor Author

marquiz commented Feb 22, 2021

How about re-using certWatcher by first moving it to k8s.io/utils.

Thanks for the heads-up. We could try to move it there. But, I'm not sure if we want to block this by waiting for that to happen. WDYT?

@marquiz marquiz force-pushed the devel/cert-watcher branch 3 times, most recently from de151f8 to efb41c5 Compare February 25, 2021 10:02
@marquiz
Copy link
Contributor Author

marquiz commented Feb 25, 2021 •
edited

Generalized the code a bit more.

How about re-using certWatcher by first moving it to k8s.io/utils.

Thanks for the heads-up. We could try to move it there. But, I'm not sure if we want to block this by waiting for that to happen. WDYT?

Took a quick look and the code looks incomplete, flaky and buggy. Thus, I don't think I want to spend time on that atm. We could try to upstream our code, though 🤔 But that's a separate issue nevertheless

@marquiz
Copy link
Contributor Author

marquiz commented Feb 25, 2021

My testing is still somewhat limited but seems to work for me
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Feb 25, 2021

Rebased. Change pkg/utils/fswatcher.go to use klog

@mythi
Copy link
Contributor

mythi commented Feb 25, 2021

Took a quick look and the code looks incomplete, flaky and buggy.

OK, I did not try certWatcher myself. What issues did you find with it?

@marquiz
Copy link
Contributor Author

marquiz commented Feb 25, 2021

If I read it correctly, fails on multiple corner cases, e.g. renames. Does not watch for ca cert.

@marquiz
Copy link
Contributor Author

marquiz commented Feb 25, 2021

Let's put this on hold until someone® verifies this with #379
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2021
@marquiz
Copy link
Contributor Author

marquiz commented Mar 9, 2021

#379 seems to be fine with this one
/unhold

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 9, 2021
zvonkok
zvonkok reviewed Mar 9, 2021
View reviewed changes
pkg/nfd-master/tls.go Outdated Show resolved Hide resolved
@marquiz
pkg/utils: generalize file watcher
dfc2596 
Add the capability to watch multiple files. Move it to a separate
package in order to make it reusable.
@marquiz
nfd-master: support certificate rotation
e771a35 
Add a helper/wrapper in pkg/utils to handle gRPC server-side certificate
rotation.
@marquiz
nfd-worker: support certificate rotation
2d20a2f 
Watch for changes in TLS files and re-connect to nfd-master in the event
of changes.
@marquiz
Copy link
Contributor Author

marquiz commented Mar 9, 2021

Rebased.

Moved pkg/nfd-master/tls.go -> pkg/utils/tls.go

@zvonkok
Copy link
Contributor

zvonkok commented Mar 11, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 11, 2021
@k8s-ci-robot k8s-ci-robot merged commit 8d1a647 into kubernetes-sigs:master Mar 11, 2021
@marquiz marquiz deleted the devel/cert-watcher branch March 11, 2021 16:31
@marquiz marquiz mentioned this pull request Mar 16, 2021
Closed
32 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zvonkok zvonkok zvonkok left review comments

@kad kad Awaiting requested review from kad

Assignees

@zvonkok zvonkok

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@marquiz @k8s-ci-robot @mythi @zvonkok