Store authzIDs directly in order table #8460
Conversation
|
@jsha, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values. |
|
@jsha, this PR adds one or more new feature flags: StoreAuthzsInTheOrder. As such, this PR must be accompanied by a review of the Let's Encrypt CP/CPS to ensure that our behavior both before and after this flag is flipped is compliant with that document. Please conduct such a review, then add your findings to the PR description in a paragraph beginning with "CPS Compliance Review:". |
aarongable
left a comment
There was a problem hiding this comment.
Sorry I didn't review this immediately! It got completely lost in the shuffle.
I think this whole PR can be simplified: specifically, I don't think there's a need for a whole new orderModelWithAuthzs type. We can see a similar example in #8056, where we added a new Replaces DEFAULT NULL column, but were able to just integrate it directly into the existing type.
The key is using the feature flag to determine whether we call borp's SetTransient method, which tells it not to specifically list that column when generating column names for SELECT statements. This means that the tx.Get in saro.go doesn't have to supply different types depending on whether the feature flag is on or off; borp will handle eliding the column name when the flag is off automatically. And it means that the tx.Insert in sa.go can simply condition whether it populates the Authzs field on the feature flag, instead of having to use two whole different types. And of course it means that modelToOrder doesn't need a top-level branch.
| // Used internally for storage in the DB, not for RPCs. | ||
| message Authzs { | ||
| repeated int64 authzIDs = 1; | ||
| } |
There was a problem hiding this comment.
Personally I'd break this out into a separate file, but maybe that plays havoc with our generate.sh.
| // StoreAuthzsInTheOrder causes the SA to write to the `authzs` | ||
| // column in NewOrder and read from it in GetOrder. It should be enabled | ||
| // after the migration to add that column has been run. | ||
| StoreAuthzsInTheOrder bool |
There was a problem hiding this comment.
Hyper-nit: the name of this flag feels incongruous next to StoreReplacesInOrders, which is semantically indicating the exact same type of change, but uses "Orders" instead of "TheOrder".
The
orderstable gets a new columnauthzs, containing a protobuf-encoded list of authorization IDs as int64s. Storing this data directly in theorderstable will allow us to get rid of theorderToAuthz2table, which is large (#8451).There's a new feature flag,
StoreAuthzsInTheOrder, which controls whether the SA expects the new column to exist. The SA gracefully handles the case where the new column is NULL, by querying theorderToAuthz2table. Eventually all valid orders will have a non-NULLauthzscolumn and we can remove the fallback.