Skip to content

Conversation

@aarongable
Copy link
Contributor

@aarongable aarongable commented Sep 24, 2025

Within sa.addRevokedCertificate, detect if the insert failed due to a duplicate primary key, and return berrors.AlreadyRevoked in that case. This matches the behavior of sa.RevokeCertificate when an update to the certificateStatus table modifies no rows.

This change has no immediate impact, because addRevokedCertificate is only called after RevokeCertificate's update to the certificateStatus table has succeeded: if the first operation returns AlreadyRevoked, this code is never reached. However, we plan to remove the certificateStatus table in the future, so this code will need to return AlreadyRevoked when appropriate.

Part of #8322


Warning

Do not merge before #8427
Do not merge before IN-11835 is complete

@aarongable aarongable marked this pull request as ready for review September 24, 2025 22:25
@aarongable aarongable requested a review from a team as a code owner September 24, 2025 22:25
@aarongable aarongable requested a review from jprenken September 24, 2025 22:25
jprenken
jprenken previously approved these changes Sep 24, 2025
@jprenken jprenken requested review from a team and jsha and removed request for a team September 24, 2025 23:02
jprenken
jprenken previously approved these changes Oct 3, 2025
@aarongable aarongable marked this pull request as ready for review October 7, 2025 21:44
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. I had a pending comment in here I thought I'd already sent.

jprenken
jprenken previously approved these changes Oct 20, 2025
@aarongable aarongable marked this pull request as ready for review October 22, 2025 01:06
@aarongable
Copy link
Contributor Author

aarongable commented Oct 22, 2025

I've updated this PR to make the serials index unique, and then rely on detecting duplicate inserts, like the first draft. I've updated IN-11835 to indicate that the new index should be unique. I think that including the index update in this PR, rather than as a predecessor PR that must be deployed first, is fine because the inserts into this table are still actually protected by the updates to the certificateStatus table. PTAnotherL?

jprenken
jprenken previously approved these changes Oct 22, 2025
@aarongable aarongable marked this pull request as draft October 28, 2025 22:13
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.

4 participants