Skip to content

Conversation

@beautifulentropy
Copy link
Member

@beautifulentropy beautifulentropy commented Jan 22, 2026

NewTransactionBuilderFromDatabase treated bucket keys formatted as enum:id (5:example.com) as if they were raw IDs (example.com, 8.8.8.8, 12345, etc.), similar to how they appear in the YAML override files. As a result, every override entry was treated as invalid.

Instead, we now use the bucket key exactly as stored by SA, and skip any additional normalization when loading overrides from the database.

Also dropped rehydration of the comment field. It’s never referenced at runtime and consumes unnecessary memory.

Important: this change makes the overrides table the source of truth.

Fixes #8590

@beautifulentropy beautifulentropy marked this pull request as ready for review January 22, 2026 20:22
@beautifulentropy beautifulentropy requested a review from a team as a code owner January 22, 2026 20:22
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

I think there's a disconnect between the contents of this diff and how its described in the PR description.

This diff removes a call to hydrateOverrideLimit. That function takes a CertificatesPerDomain bucket key that looks like 1.2.3.4 and converts it to 1.2.3.4/32, and it takes a CertificatesPerFQDNSet bucket key that looks like example.com,example.org and converts it to 0xDEADBEEF. Are we really storing CIDR prefixes and hashed fqdnsets in the database, such that we don't have to rehydrate these bucket keys? I thought we'd decided that the database would be storing human-readable bucket keys, just like the yaml files do. (Editor's note: it does indeed look like we're storing unreadable bucket keys, since both the admin tool and the sfe importer use BuildBucketKey, which in turn uses HashIdentifiers. I'm sure I reviewed these PRs at the time, but I am nonetheless surprised right now.)

But that said, the PR description sounds like the problem is the difference between 5:example.com and just example.com, rather than the problem being the difference between example.com and 0xDEADBEEF.

Beyond that, we're also transitively losing calls to limitName.isValid() (which should be unaffected by rehydration, and therefore would be nice to preserve) and to validateIdForName (which I'm guessing is the real culprit here, since 0xDEADBEEF isn't a valid ID for CertificatesPerFQDNSet). But if we were storing the same kinds of bucket keys as were in the yaml files, then those validations could be preserved.

@aarongable aarongable requested a review from jsha January 22, 2026 23:11
Copy link
Contributor

@aarongable aarongable left a comment

Choose a reason for hiding this comment

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

Chatted about this out-of-band. Makes sense now! Based on the PR description I was thinking that the whole NewTransactionBuilderFromDatabase function was assuming keys would arrive in the old yaml-esque format, and was very confused about where and how it was doing the transformation between bare IDs and bucketKeys. But in fact most of this function is totally correct, and it's only the call tp hydrateOverrideLimit that was making incorrect assumptions.

Diff now completely LGTM, I think the PR description should be updated to more specifically call out hydrateOverrideLimit and why it's not necessary when loading from the DB.

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.

wfe/ra cannot hydrate any of the limits from the DB

3 participants