Skip to content

Add AlreadyRevoked handling to revokedCertificates table inserts - #8408

Draft
aarongable wants to merge 6 commits into
mainfrom
already-revoked
Draft

Add AlreadyRevoked handling to revokedCertificates table inserts#8408
aarongable wants to merge 6 commits into
mainfrom
already-revoked

Conversation

@aarongable

@aarongable aarongable commented Sep 24, 2025

Copy link
Copy Markdown
Contributor

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
Do not merge before #8884 has been reverted (and that revert deployed)

@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
Comment thread sa/sa.go
jprenken
jprenken previously approved these changes Oct 3, 2025
@aarongable
aarongable marked this pull request as ready for review October 7, 2025 21:44

@jsha jsha left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread sa/sa.go Outdated
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

aarongable commented Oct 22, 2025

Copy link
Copy Markdown
Contributor Author

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
Comment thread sa/sa.go Outdated
@pgporada

pgporada commented Jul 29, 2026

Copy link
Copy Markdown
Member

@aarongable IN-11835 sat lingering forever in my backlog, but has been deployed to staging and prod successfully.

@danjeffery It was replication safe on mixed binlog replication meaning that replication wasn't unexpectedly stopped and no replicas needed to be recovered. However, you will have a replication pause as the table copy without partitioning is blasted across the network unless you do some migration tricks like dropping a node out of the cluster, doing dropping the partition, rinse repeat. Now our prod table as ~2GB and ~12M rows so that happened fast ~2 minutes. If you aren't using partition then don't even worry about that. The inplace index add took ~30 seconds.

# Remove the partitioning
ALTER TABLE revokedCertificates REMOVE PARTITIONING;

# Add the new index
ALTER TABLE revokedCertificates ADD UNIQUE INDEX `serial` (`serial`), ALGORITHM=INPLACE, LOCK=NONE;

Tl;dr consider your error budget

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