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: TLS certs auto-reload #2910

Closed
wants to merge 1 commit into from
Closed

Conversation

StarAurryon
Copy link
Contributor

@StarAurryon StarAurryon commented Dec 30, 2021

Kubernetes is able to auto-renew certificates with cert-manager and update files in the container. I would be cool if Ory Hydra could support auto certificate update on file change.

Related issue(s)

#2568

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

Adds dependencies to fsnotify to support Mac/Windows/Linux.

This is an example of a working PoC. I need feedback ;-)

@codecov
Copy link

codecov bot commented Dec 30, 2021

Codecov Report

Merging #2910 (45a4254) into master (ff10e17) will decrease coverage by 0.79%.
The diff coverage is 8.98%.

❗ Current head 45a4254 differs from pull request most recent head 4019d10. Consider uploading reports for the commit 4019d10 to get more accurate results

@@            Coverage Diff             @@
##           master    #2910      +/-   ##
==========================================
- Coverage   79.40%   78.60%   -0.80%     
==========================================
  Files         112      112              
  Lines        7889     7971      +82     
==========================================
+ Hits         6264     6266       +2     
- Misses       1223     1302      +79     
- Partials      402      403       +1     
Impacted Files Coverage Δ
cmd/server/helper_cert.go 15.78% <4.81%> (-32.79%) ⬇️
driver/config/tls.go 91.66% <60.00%> (-8.34%) ⬇️
cmd/server/handler.go 63.76% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00100a1...4019d10. Read the comment docs.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Cool stuff! This makes a ton of sense!

I would like for this PR though to wait for @aarmam's PR #2625 to be merged as it might introduce a few breaking changes again. But in general, this looks very good!

Alternatively, you could also branch off of #2625 :)

@StarAurryon
Copy link
Contributor Author

Cool. I will wait to see things that have changed. In the meantime, if you have any recommendation on the coding style, please let me know. ;-)

@StarAurryon
Copy link
Contributor Author

May I propose sth similar for kratos?

@aeneasr
Copy link
Member

aeneasr commented Jan 5, 2022

Sure! :) You can also work on kratos first and then port this here

@aeneasr
Copy link
Member

aeneasr commented Jan 11, 2022

In the meanwhile (while this is work in progress), I'll mark this as draft!

@aeneasr aeneasr marked this pull request as draft January 11, 2022 13:23
@StarAurryon
Copy link
Contributor Author

Hi @aeneasr ,

Github seems quite buggy today. A lot of 500 are occurring, maybe because of Ukraine IDK. I have push the update synced with the latest master commit.

I also added a small 2 second wait until all changes are made to avoid spam reload in the logs.

As PR #2625 is merged and if it is ok for you we could avoid considering this as a Draft.

;-)

@StarAurryon StarAurryon force-pushed the tls-autoreload branch 2 times, most recently from 5052f53 to 57c5a04 Compare March 17, 2022 18:14
@StarAurryon StarAurryon changed the title wip: TLS Cert auto-reload proposal feat: TLS Cert auto-reload proposal Mar 28, 2022
@StarAurryon StarAurryon changed the title feat: TLS Cert auto-reload proposal feat: TLS Cert auto-reload Mar 28, 2022
@StarAurryon StarAurryon changed the title feat: TLS Cert auto-reload feat: TLS certs auto-reload Mar 28, 2022
@aeneasr
Copy link
Member

aeneasr commented Apr 11, 2022

@StarAurryon is this good for review now? :)

@StarAurryon
Copy link
Contributor Author

StarAurryon commented Apr 12, 2022

@aeneasr Yes that's ok. But as you mention part of the code is shared with the Kratos PR too. Merging it to the right ory/x could be better.

I am waiting for more instructions.

@aeneasr
Copy link
Member

aeneasr commented Apr 17, 2022

Merging it to the right ory/x could be better.

I think that's the way to go forward :)

@StarAurryon
Copy link
Contributor Author

#3265 was merged. There is no more need for this PR

@StarAurryon StarAurryon closed this Oct 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants