Move ocsp_response_cache:delete after certificate_data:set

[wenzong]

Move ocsp_response_cache:delete after certificate_data:set

What this PR does / why we need it:

There is a potential concurrency problem.

So in /configurations/servers:

• after ocsp_response_cache:delete(uid)
• before certificate_data:set(uid)

Since the two steps are not atomic operation, there is a small chance that some nginx worker missed the cache and read the old certificate, then fetch and cache the old ocsp_response again.

This PR change the sequence of the two operations.

However this PR is not perfect. There might be some requests still served by mismatch certificate and OCSP Response(between certificate_data:set and ocsp_response_cache:delete), but the impact is much more smaller than origin implementation.

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I've read the CONTRIBUTION guide
• I have added tests to cover my changes.
• All new and existing tests passed.

[k8s-ci-robot]

Hi @wenzong. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[wenzong]

/assign @ElvinEfendi

[aledbf]

/ok-to-test

[ElvinEfendi]

However this PR is not perfect. There might be some requests still served by mismatch certificate and OCSP Response(between certificate_data:set and ocsp_response_cache:delete), but the impact is much more smaller than origin implementation.

I think this is significantly better though. There will potentially be mismatch, but we will not be caching OCSP response for an outdated certificate at least. In other words if we hit the race with this PR the worst is few requests will have incorrect OCSP response, whereas with the original fix if the bug was hit incorrect OCSP response would be served until NGINX is restarted or cache is expired after 3 days.

[wenzong]

However this PR is not perfect. There might be some requests still served by mismatch certificate and OCSP Response(between certificate_data:set and ocsp_response_cache:delete), but the impact is much more smaller than origin implementation.

I think this is significantly better though. There will potentially be mismatch, but we will not be caching OCSP response for an outdated certificate at least. In other words if we hit the race with this PR the worst is few requests will have incorrect OCSP response, whereas with the original fix if the bug was hit incorrect OCSP response would be served until NGINX is restarted or cache is expired after 3 days.

I also considered using some kind of lock to make the operations atomically, there is two reasons prevent me from doing so.

The first consideration is the performance could be affected by using lock. Another problem is I am not familiar with the openresty and Lua ecosystem, therefore I don't know whether there is any mature lock implementation.

So I just change the operations order to avoid severe issue ;)

---

Code refactored. Please review again.

[ElvinEfendi]

/lgtm

FWIW lua-resty-lock is mature enough locking library for Openresty.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ElvinEfendi, wenzong

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [ElvinEfendi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment