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.

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

feat: automatic TLS certificate reload #826

Merged
merged 1 commit into from
Jul 19, 2024
Merged

Conversation

flavio
Copy link
Member

@flavio flavio commented Jul 16, 2024

When running on Linux, monitor changes done to the TLS certificate and key used by the https server.

When both change, react by updating the TLS configuration.

This allows to rotate the TLS certificate used by a Policy Server without having to restart the process.

Notes:

  • This is not covered by the unit tests right now. I don't know yet how hard it would be to test it. I'll leave a comment here once I know more about the topic
  • This change is part of the upcoming work to handle certificate rotation inside of the whole KW stack
  • Ideally, we can merge this code right now and make it part of the 1.15 release since it won't cause any harm

@flavio flavio requested a review from a team as a code owner July 16, 2024 14:10
@flavio flavio self-assigned this Jul 16, 2024
Copy link

codecov bot commented Jul 16, 2024

Codecov Report

Attention: Patch coverage is 70.45455% with 13 lines in your changes missing coverage. Please review.

Project coverage is 61.44%. Comparing base (1677586) to head (5e94a62).
Report is 3 commits behind head on main.

Files Patch % Lines
src/lib.rs 70.45% 13 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #826      +/-   ##
==========================================
+ Coverage   58.45%   61.44%   +2.99%     
==========================================
  Files          17       17              
  Lines         994     1035      +41     
==========================================
+ Hits          581      636      +55     
+ Misses        413      399      -14     
Flag Coverage Δ
integration-tests 53.86% <70.45%> (+3.31%) ⬆️
unit-tests 33.20% <0.00%> (-1.41%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

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

Looks great! As far as testing goes, we could write an integration test. However, I don't see this as critical and will be fine leaving this part untested.

@flavio
Copy link
Member Author

flavio commented Jul 17, 2024

UPDATE: all done

I've updated the code to have an integration test. The integration test will fail until I do the following changes:

When running on Linux, monitor changes done to the TLS certificate and
key used by the https server.

When both change, react by updating the TLS configuration.

This allows to rotate the TLS certificate used by a Policy Server
without having to restart the process.

Signed-off-by: Flavio Castelli <fcastelli@suse.com>
Copy link
Member

@viccuad viccuad left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@flavio flavio merged commit f9788b0 into kubewarden:main Jul 19, 2024
11 checks passed
@flavio flavio deleted the cert-reload branch July 19, 2024 13:25
@kkaempf kkaempf added this to the 1.15 milestone Jul 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants