Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
aarongable
left a comment
aarongable
left a comment
There was a problem hiding this comment.
I have a few minor comments about the code, but nothing big.
My major concern is that this simply doesn't feel like a prober. It's doing so much work, and has so many opportunities for failure, all of which get wrapped up in big accumulated errors. In my mind, the ideal prober is short, simple, only emits metrics (not meaningful logs), and we have alerts for every single metric it exposes. I understand that it is useful for this to live within boulder-observer, so we don't have to deploy and monitor Yet Another Component, but I'm registering my moderate discomfort with the complexity here.
cmd/boulder-observer/main.go
Outdated
| cmd.FailOnError(err, "config failed validation") | ||
| } | ||
|
|
||
| defer cmd.AuditPanic() |
There was a problem hiding this comment.
Shouldn't this be at the very top of main(), as it documents that it should be "called in a defer statement as early as possible"?
There was a problem hiding this comment.
Was this file accidentally included? It's not a valid PEM file, and I don't see any references to it in the Go source.
There was a problem hiding this comment.
Oops, yes. But, fun note, PEM files can have stuff between the blocks! Will delete anyhow. :D
observer/probers/ccadb/ccadb.go
Outdated
| return err | ||
| } | ||
|
|
||
| var crls, entries, bytes int |
There was a problem hiding this comment.
These accumulator variables are written to, but never read from -- I think they can be removed. Also, bytes shadows the stdlib package of the same name.
observer/probers/ccadb/ccadb.go
Outdated
| return err | ||
| } | ||
|
|
||
| crlURLs, err := c.getCRLURLs(ctx, c.allCertificatesCSVURL, c.caOwner, issuers) |
There was a problem hiding this comment.
getCRLURLs is a method on the CCADBProber, so it doesn't need c.allCertificatesCSVURL and c.caOwner as arguments. It feels like it should either drop those arguments and read their values from the receiver, or it should become a helper function instead of method.
observer/probers/ccadb/ccadb.go
Outdated
| crls++ | ||
| issuer := issuers[skid] | ||
| if issuer == nil { | ||
| return fmt.Errorf("no issuer found for skid %x", skid) |
There was a problem hiding this comment.
Should this also be an append(errs, ...); continue instead of bailing out?
There was a problem hiding this comment.
Good point. Moved it up a level at the same time so it won't trigger N times for every affected CRL.
observer/probers/ccadb/ccadb.go
Outdated
| if otherCRL, ok := serials[serialByteString]; ok { | ||
| otherCRLURL, err := getIDP(otherCRL) | ||
| if err != nil { | ||
| return err |
There was a problem hiding this comment.
Same comment as above about appending err and continuing?
2 participants
Prior work: letsencrypt/crl-monitor#88
Fixes #8618