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
fix: ensure certificate gets updated on reload #12076
fix: ensure certificate gets updated on reload #12076
Conversation
Fixes argoproj#10707. `GetCertificate` ensures that the most current version of `a.settings.Certificate` is used. It's still a bit of a mystery to me as to why the reloading of the server does not work for this, since it should fulfill the same function. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Codecov ReportBase: 47.29% // Head: 47.42% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #12076 +/- ##
==========================================
+ Coverage 47.29% 47.42% +0.13%
==========================================
Files 245 246 +1
Lines 41670 41826 +156
==========================================
+ Hits 19707 19838 +131
- Misses 19978 19992 +14
- Partials 1985 1996 +11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
I think this is the change that broke it: 9d4c940 I don’t completely understand what causes us to move through the loop. But prior to that change, we’d initialize ArgoCDServer on each iteration. Now we initialize it outside that loop. The certificates are set in NewServer and then never updated. |
With 3553ef8, there's no longer any need to break out of the loop. The webhook reloading logic needs another look (since it likely no longer works), but can be handled in another PR. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
I haven't had time to look into this in depth but this probably means that the webhook reloading doesn't work either. I modified |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
I've tested this locally and indeed it fixes the regression. I've used the testsuite from @vx-github :
Step 1: Get the current certificate
2023-03-02 11:17:13
subject= /C=NL/O=Argo CD Cert Test number 1/CN=localhost
notBefore=Mar 2 10:16:12 2023 GMT
notAfter=Mar 12 10:16:12 2023 GMT
serial=ECD5483932CD2E8C <--- old
Step 2: Update the certificate secret/argocd-server-tls
2023-03-02 11:17:14
subject= /C=NL/O=Argo CD Cert Test number 2/CN=localhost
notBefore=Mar 2 10:17:14 2023 GMT
notAfter=Mar 12 10:17:14 2023 GMT
serial=B850266BA30BFB68
secret/argocd-server-tls configured
Check if the certificate has indeed changed
2023-03-02 11:17:14
subject= /C=NL/O=Argo CD Cert Test number 2/CN=localhost
notBefore=Mar 2 10:17:14 2023 GMT
notAfter=Mar 12 10:17:14 2023 GMT
serial=B850266BA30BFB68 <--- new
Notice the change now it so fast it happens even within the same second.
This is also visible from the logs:
[argocd-server] time="2023-03-02T10:17:14Z" level=info msg="Loading TLS configuration from secret argocd/argocd-server-tls"
[argocd-server] time="2023-03-02T10:17:14Z" level=info msg="Notifying 1 settings subscribers: [0xc00135a960]"
[argocd-server] time="2023-03-02T10:17:14Z" level=info msg="tls certificate modified. reloading certificate"
[dex] time="2023-03-02T10:17:14Z" level=info msg="Loading TLS configuration from secret argocd/argocd-server-tls"
[dex] time="2023-03-02T10:17:14Z" level=info msg="Notifying 1 settings subscribers: [0xc0001866c0]"
[dex] time="2023-03-02T10:17:14Z" level=info msg="dex config unmodified"
[argocd-applicationset-controller] time="2023-03-02T10:17:14Z" level=info msg="Loading TLS configuration from secret argocd/argocd-server-tls"
[argocd-application-controller] time="2023-03-02T10:17:19Z" level=info msg="Loading TLS configuration from secret argocd/argocd-server-tls
In the previous version, the tlsConfig
variable was set with a single certificate using Certificates field.
However, in this PR's updated version, the TlsConfig.GetCertificate
method is set instead, which returns the certificate from the a.settings.Certificate
variable that contains the latest version of certificates. This hook dynamically reloads the certificate as needed, without restarting the server, which could be more efficient and less disruptive to ongoing operations.
/cherry-pick release-2.4 |
/cherry-pick release-2.5 |
/cherry-pick release-2.6 |
* fix: ensure certificate gets updated on reload Fixes #10707. `GetCertificate` ensures that the most current version of `a.settings.Certificate` is used. It's still a bit of a mystery to me as to why the reloading of the server does not work for this, since it should fulfill the same function. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * fix: remove break from cert changes With 3553ef8, there's no longer any need to break out of the loop. The webhook reloading logic needs another look (since it likely no longer works), but can be handled in another PR. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
* fix: ensure certificate gets updated on reload Fixes #10707. `GetCertificate` ensures that the most current version of `a.settings.Certificate` is used. It's still a bit of a mystery to me as to why the reloading of the server does not work for this, since it should fulfill the same function. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * fix: remove break from cert changes With 3553ef8, there's no longer any need to break out of the loop. The webhook reloading logic needs another look (since it likely no longer works), but can be handled in another PR. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
* fix: ensure certificate gets updated on reload Fixes #10707. `GetCertificate` ensures that the most current version of `a.settings.Certificate` is used. It's still a bit of a mystery to me as to why the reloading of the server does not work for this, since it should fulfill the same function. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * fix: remove break from cert changes With 3553ef8, there's no longer any need to break out of the loop. The webhook reloading logic needs another look (since it likely no longer works), but can be handled in another PR. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
* fix: ensure certificate gets updated on reload Fixes #10707. `GetCertificate` ensures that the most current version of `a.settings.Certificate` is used. It's still a bit of a mystery to me as to why the reloading of the server does not work for this, since it should fulfill the same function. * fix: remove break from cert changes With 3553ef8, there's no longer any need to break out of the loop. The webhook reloading logic needs another look (since it likely no longer works), but can be handled in another PR. --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
* fix: ensure certificate gets updated on reload Fixes #10707. `GetCertificate` ensures that the most current version of `a.settings.Certificate` is used. It's still a bit of a mystery to me as to why the reloading of the server does not work for this, since it should fulfill the same function. * fix: remove break from cert changes With 3553ef8, there's no longer any need to break out of the loop. The webhook reloading logic needs another look (since it likely no longer works), but can be handled in another PR. --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
* fix: ensure certificate gets updated on reload Fixes #10707. `GetCertificate` ensures that the most current version of `a.settings.Certificate` is used. It's still a bit of a mystery to me as to why the reloading of the server does not work for this, since it should fulfill the same function. * fix: remove break from cert changes With 3553ef8, there's no longer any need to break out of the loop. The webhook reloading logic needs another look (since it likely no longer works), but can be handled in another PR. --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> Co-authored-by: Blake Pettersson <blake.pettersson@gmail.com>
* fix: ensure certificate gets updated on reload Fixes argoproj#10707. `GetCertificate` ensures that the most current version of `a.settings.Certificate` is used. It's still a bit of a mystery to me as to why the reloading of the server does not work for this, since it should fulfill the same function. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * fix: remove break from cert changes With 3553ef8, there's no longer any need to break out of the loop. The webhook reloading logic needs another look (since it likely no longer works), but can be handled in another PR. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> Signed-off-by: rumstead <37445536+rumstead@users.noreply.github.com>
* fix: ensure certificate gets updated on reload Fixes argoproj#10707. `GetCertificate` ensures that the most current version of `a.settings.Certificate` is used. It's still a bit of a mystery to me as to why the reloading of the server does not work for this, since it should fulfill the same function. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> * fix: remove break from cert changes With 3553ef8, there's no longer any need to break out of the loop. The webhook reloading logic needs another look (since it likely no longer works), but can be handled in another PR. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> --------- Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>
Fixes #10707.
GetCertificate
ensures that the most current version ofa.settings.Certificate
is used. It's still a bit of a mystery to meas to why the reloading of the server does not work for this, since it
should fulfill the same function.
I'm not sure of the best way to test that this works the way as intended,
but I tested this locally with the test suite given in #10707 and this works
fine.
Signed-off-by: Blake Pettersson blake.pettersson@gmail.com
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: