Support fsnotify based reloading of certificate, key and CA certs

[charlieegan3]

We currently have a command line parameter tls-cert-refresh-period to control how often certificates and keys (tls-cert-file, tls-private-key-file) are reloaded from disk.

We can see this here:

opa/cmd/run.go

Line 196 in 107cace

runCommand.Flags().DurationVar(&cmdParams.tlsCertRefresh, "tls-cert-refresh-period", 0, "set certificate refresh period")

This triggers a reloading loop but only if the refresh interval is set

opa/server/server.go

Line 562 in 107cace

if s.certRefresh > 0 {

It would be nice to make use of fsnotify which we already depend on for the watch run flag.

There are other functionalities that could benefit from this too #1719 (comment). Some refactoring out our use of fsnotify into something more generic might be a pre-requisite in both cases.

[anderseknert]

Sounds good! We likely want to keep the manual option too, as the file watcher has been known not to work with things like nfs/efs, and similar.

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days.

[charlieegan3]

I think this might be made easier once some of the work in #5812 is merged.

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

[tjons]

@anderseknert would this be a good-second-issue?

[anderseknert]

I don't see why not! 😃 I know @ashutosh-narkar recently worked with some fsnotify related stuff in #5950 so maybe he could provide some pointers. But I'd say go for it!

[ashutosh-narkar]

@tjons go for it! We've moved the watcher utilities in this package to enable easy sharing. opa test and the runtime package are good examples of how to use the watcher functionality. Let us know if you have any questions and thanks for looking into this.

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

[charlieegan3]

I've just had a report that from a user who's been burned by this. Their certs had been rotated, but the OPA instance didn't reload it as they'd not set tls-cert-refresh-period.

[charlieegan3]

@tjons if you're interested, a proof-of-concept implementation is here: #6118. I'm going to be out on leave until the 8th so feel free to carry on where I left off. There's a test case that's passing but the implementation is pretty scruffy and there are some other TODOs.

[tjons]

@charlieegan3 I've been moving but plan to pick this up this weekend! I'll keep you posted

[charlieegan3]

Hey that's no problem 🙂 I've been OOO too - tis the season!

If you're still keen, it'd be nice to try and get something for this in the next release (in around 2 weeks time).

[tjons]

@charlieegan3 yep, still keen :D. Started in over the weekend and should have something ready by this next Monday.

[charlieegan3]

Sounds great, keep us posted!

[tjons]

@charlieegan3 I've still been busy.. might be able to get something up end of the week but if this is really time-sensitive I'm happy to bow out and pick up another issue when I have bandwidth - lmk.

[charlieegan3]

Hey, thanks for keeping us in the loop! I'm going out on leave again on Friday for a week. Keen to let you have a go 🙂, but if you'd rather someone else did it, I can take a look when I get back.

[tjons]

Cool, I'll keep it on my todo list

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

[charlieegan3]

Hey @tjons, I have some time to work on this issue this week. Do you have something WIP, or should I pick this up?

[yogisinha]

Hi @charlieegan3 , can I look at this ? I can start after couple of days. have to learn abt fsnotify also.

[tjons]

Hey @charlieegan3 - I do have something WIP, was hoping to wrap by end of week. Sorry for the delays here, I understand if you want to just handle it. I can pick up another issue instead.

[charlieegan3]

👍 still keen for you to do it if you have time and something WIP @tjons 🙂.

(@yogisinha, lmk if you need help selecting a good issue in a few days time!)

[yogisinha]

yes @charlieegan3 lmk what I can work on.

[charlieegan3]

Have you seen the backlog list here: https://github.com/open-policy-agent/opa/projects/5

(if you'd like to chat more, can you drop me a message in the OPA slack to keep this issue on-task. Thanks!)

[charlieegan3]

Hey @tjons, @yogisinha was interested in picking this one up, does that sound ok?

[tjons]

@charlieegan3 sure yes. I'll find something else to do when my schedule frees up. Have been quite busy with my regular day job.

[charlieegan3]

Thanks @tjons! No worries, @yogisinha it's all yours 🙂

[stale]

This issue has been automatically marked as inactive because it has not had any activity in the last 30 days. Although currently inactive, the issue could still be considered and actively worked on in the future. More details about the use-case this issue attempts to address, the value provided by completing it or possible solutions to resolve it would help to prioritize the issue.

[charlieegan3]

I've opened a PR for this here: #6415, any comments welcome and I'll get back to them tomorrow.