admin: Support creating incidents and adding serials#8740
admin: Support creating incidents and adding serials#8740beautifulentropy wants to merge 1 commit intomainfrom
Conversation
beautifulentropy
force-pushed
the
admin-tool-sa-incidents
branch
7 times, most recently
from
da7c02c to
5392654
Compare
beautifulentropy
force-pushed
the
admin-tool-sa-incidents
branch
from
5392654 to
24c16e5
Compare
|
@beautifulentropy, 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. |
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
I haven't done a detailed review of admin/incident_test.go or saa_test.go, but I've done a pass on everything else.
| var _ saAdminClient = (*dryRunSAAdmin)(nil) | ||
|
|
||
| func (d dryRunSAAdmin) CreateIncident(_ context.Context, req *sapb.CreateIncidentRequest, _ ...grpc.CallOption) (*sapb.Incident, error) { | ||
| d.log.Infof("dry-run: Create incident %q (url=%q, renewBy=%s)", req.SerialTable, req.Url, req.RenewBy.AsTime()) |
There was a problem hiding this comment.
Just a heads up that the blog PR totally overhauls how the dry-run clients above log, so keep an eye on that depending on whether this lands before or after the blog PR.
| // incidents row identified by serialTable. An empty url, nil renewBy, and | ||
| // nil enabled all mean "leave that field alone"; at least one of them must | ||
| // be set. | ||
| message UpdateIncidentRequest { |
There was a problem hiding this comment.
If you wanted to be really fancy, you could use a FieldMask to control which fields get updated, rather than relying on zero-values.
But as cool as FieldMasks are, we don't use them anywhere else, and this probably isn't the time to start. If we want to use them, we should make a concerted effort to use them everywhere. Anyway, just sharing for fun.
| return errors.New("-parallelism must be > 0") | ||
| } | ||
|
|
||
| file, err := os.Open(s.serialsFile) |
There was a problem hiding this comment.
Idea: if s.serialsFile is -, then open stdin. Alternatively, add documentation noting that a user could pass /dev/stdin as the file to read from if they want to stream serials on stdin.
| // StorageAuthorityAdmin exposes those SA methods exclusive to the admin tool. | ||
| service StorageAuthorityAdmin { | ||
| rpc CreateIncident(CreateIncidentRequest) returns (Incident) {} | ||
| rpc UpdateIncident(UpdateIncidentRequest) returns (google.protobuf.Empty) {} |
There was a problem hiding this comment.
Should this not return Incident, reflecting the new state of the world, just like Create?
|
|
||
| var incidentTable string | ||
| var buf []string | ||
|
|
There was a problem hiding this comment.
Might be worth adding an explicit check that the table exists, so we can provide a better error message if it doesn't? Totally optional, feel free to ignore.
aarongable
requested review from
a team and
jsha
and removed request for
a team
2 participants
Add a StorageAuthorityAdmin gRPC service and four admin subcommands that move incident management into the
admintool. Register the service only for the admin client and write through a newincidents_sa_adminMySQL user; the existingincidents_sauser stays SELECT-only.Incidents can impact tens to hundreds of millions of certificates, and operators may load serials from multiple overlapping time spans during the affected window.
admin load-incident-serialsreads serials from a file, fans them out across N workers (default 10), and streams them to the SA in batches of 10,000. The server re-batches at the same threshold and usesINSERT IGNOREon insertion, so overlapping bulk loads and within-batch duplicates are idempotent.The remaining subcommands cover CRUD on the incidents table:
admin create-incidentadds a row and creates the per-incident serials table.admin list-incidentsprints each incident's name, enabled state, renewBy, and URL.admin update-incidentchanges any subset of url, renewBy, and enabled on an incident by name.Fixes #6943