-
Notifications
You must be signed in to change notification settings - Fork 8
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
[DPE-5932] Remove cert-available check for CA rotation #502
Conversation
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.
Hey Pedro, I think this change makes sense. We had a couple of changes on the whole process in the last weeks, therefore the condition if not self.ca_rotation_complete_in_cluster()
in _on_certificate_available
is now probably obsolete. The original need for it was to make sure nothing gets updated during the rollout of the new CA: no certs, no secrets, no ca-bundle.
Your assumption of a race condition seems plausible, at least the integration tests look good after the change. Important tough: Make sure the condition if not self.ca_rotation_complete_in_cluster()
stays in store_new_tls_resources()
, otherwise the certs might get updated during the rollout of the new CA.
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.
Hey @phvalguima the unit tests look fine now. The only thing that needs to be adjusted is the comments in line 1581 and 1690 of test_opensearch_tls.py
saying In case any stage of a CA cert rotation is being processed, further 'certificate-available' events are deferred.
Other than that, the change can be merged from my point of view.
This PR removes one of the filters that was causing a race condition at CA rotation + large deployments. It fixes the CA rotation for large deployments in our CI.
Closes #500