Running an older version (v0.0.1-2020.1.4) of `staticcheck` in
whole-program mode (`staticcheck --unused.whole-program=true -- ./...`)
finds various instances of unused code which don't normally show up
as CI issues. I've used this to find and remove a large chunk of the
unused code, to pave the way for additional large deletions accompanying
the WFE1 removal.
Part of #5681
Currently, `cmd.FailOnError` both audit-logs the message and error, and
prints it directly to stderr. It does both because originally it only
printed to stderr, and the audit logging capability was added later.
Since audit logs are printed to stderr anyway, and since printing to
stderr without going through the logger produces lines of output that
violate the log-validator's expected checksums, remove the now-redundant
print.
Fixes#5790
We had in place some filtering for grpc errors that we consider
spurious, but that filtering was broken. This change ensures the
filtering gets called regardless of which of the various error/warning
methods grpc calls. This removes a lot of unnecessary red from our
integration test output.
Instead of using the default `json.Unmarshal`, explicitly
construct and use a `json.Decoder` so that we can set the
`DisallowUnknownFields` flag on the decoder. This causes
any unrecognized config keys to result in errors at boulder
startup time.
Fixes#5643
Make the `NonCFSSLSigner` code path the only code path through the CA.
Remove all code related to the old, CFSSL-based code path. Update tests
to supply (or mock) issuers of the new kind. Remove or simplify a few
tests that were testing for behavior only exhibited by the old code
path, such as incrementing certain metrics. Remove code from `//cmd/`
for initializing the CFSSL library. Finally, mark the `NonCFSSLSigner`
feature flag itself as deprecated.
Delete the portions of the vendored CFSSL code which were only used
by these deleted code paths. This does not remove the CFSSL library
entirely, the rest of the cleanup will follow shortly.
Part of #5115
Having both of these very similar methods sitting around
only serves to increase confusion. This removes the last
few places which use `cmd.LoadCert` and replaces them
with `core.LoadCert`, and deletes the method itself.
Fixes#5163
This builds on #4665 and #4781. The problem we had previously was that
we were relying on a goroutine to consume bytes from a pipe in a
non-blocking manner, which meant that log.Fatal would cause us to exit
before writing out the data.
This version implements an io.Writer so we can make sure the log line
gets written in a blocking manner.
The problem with this approach is that there is no way to guarantee the output
is copied to syslog / stdout before shutdown. This is particularly evident when
`log.Fatal` is used, because that calls `os.Exit` immediately after `l.Output`,
creating a race condition where the log line might or might not get printed
before the program exits.
Reverting this change means that in case some component does call `log.Fatal`
we'll still get the output from stdout.
This also changes one instance in cmd/shell.go where we call `log.Fatal` to use
`logger.Errf`.
Since Boulder's log system adds checksums to lines, but log-validator
processes entries on a per-line basis, including newlines in log
messages can cause a validation failure.
Some components, particularly net/http, occasionally output log lines
via log.Print. We'd like to capture these and send them to rsyslog so
all our log data goes to the same place, and so that we can attach log
line checksums to them.
This uses log.SetOutput to change the log output to an io.Pipe,
then consumes that buffer line-by-line in a goroutine and sends it to
our rsyslog logger.
This seems to tickle an unrelated race condition in test/ocsp/helper.go,
so I fixed that too.
Also filters out a noisy and unimportant error from the grpcLog handler.
Fixes#4664Fixes#4628
In a handful of places I've nuked old stats which are not used in any alerts or dashboards as they either duplicate other stats or don't provide much insight/have never actually been used. If we feel like we need them again in the future it's trivial to add them back.
There aren't many dashboards that rely on old statsd style metrics, but a few will need to be updated when this change is deployed. There are also a few cases where prometheus labels have been changed from camel to snake case, dashboards that use these will also need to be updated. As far as I can tell no alerts are impacted by this change.
Fixes#4591.
* cmd: update prometheus.NewProcessCollector args.
There's a new struct `prometheus.ProcessCollectorOpts` that is expected
to be used as the sole argument to `prometheus.NewProcessCollector`. We
don't need to specify `os.Getpid` as the `PidFn` of the struct because
the default is to assume `os.Getpid`. Similarly we don't need to set the
namespace to `""` explicitly, it is the default.
* SA: reimplement db metrics as custom collector.
The modern Prometheus golang API supports translating between legacy
metric sources on the fly with a custom collector. We can use this
approach to collect the metrics from `gorp.DbMap`'s via the `sql.DB`
type's `Stats` function and the returned `sql.DbStats` struct.
This is a cleaner solution overall (we can lose the DB metrics updating
go routine) and it avoids the need to use the now-removed `Set` method
of the `prometheus.Counter` type.
* test: Update CountHistogramSamples.
The `With` function of `prometheus.HistogramVec` types we tend to use as
the argument to `test.CountHistogramSamples` changed to return
a `prometheus.Observer`. Since we only use this function in test
contexts, and only with things that cast back to
a `prometheus.Histogram` we take that approach to fix the problem
without updating call-sites.
The idea expressed in this comment isn't representative of the
Boulder cmds. E.g. There's no top level "App Shell" in use and the
`NewAppShell`, `Action` and `Run` functions ref'd do not exist.
These errors show up in the Publisher at shutdown during integration
test runs, because the Publisher is trying to write responses from RPCs
that were slow due to the ct-test-srv's LatencySchedule. This
specifically happens only for the optional submission of "final"
certificates.
The gRPC INFO log lines clutter up integration test output, and we've never
had a use for them in production (they are mostly about details of
connection status).
A very large number of the logger calls are of the form log.Function(fmt.Sprintf(...)).
Rather than sprinkling fmt.Sprintf at every logger call site, provide formatting versions
of the logger functions and call these directly with the format and arguments.
While here remove some unnecessary trailing newlines and calls to String/Error.
This commit updates the `boulder-ra` and `boulder-ca` commands to refuse
to start if their configured `MaxNames` is 0 (the default value). This
should always be set to a positive number.
This commit also updates `csr/csr.go` to always apply the max names
check since it will never be 0 after the change above.
Also refactor `FailOnError` to pull out a separate `Fail` function.
Related to https://github.com/letsencrypt/boulder/issues/3632
Some of the pprof handlers have to be accessed through
pprof.Handler("string"), while some have to be accessed through an
exported var in pprof. We weren't doing the latter before, which meant
some key handlers like Profile weren't available.
In #3167 I removed expvar, thinking it was unused, but it turns out the RA
exports the last issuance time, and core/util.go has a function to export
BuildID, both of which are used in monitoring. This wasn't caught at compile
time because the global expvar package was happy to register the exports even
though there was no handler to serve them.
There were two bugs in #3167:
All process-level stats got prefixed with "boulder", which broke dashboards.
All request_time stats got dropped, because measured_http was using the prometheus DefaultRegisterer.
To fix, this PR plumbs through a scope object to measured_http, and uses an empty prefix when calling NewProcessCollector().
Previously, we used prometheus.DefaultRegisterer to register our stats, which uses global state to export its HTTP stats. We also used net/http/pprof's behavior of registering to the default global HTTP ServeMux, via DebugServer, which starts an HTTP server that uses that global ServeMux.
In this change, I merge DebugServer's functions into StatsAndLogging. StatsAndLogging now takes an address parameter and fires off an HTTP server in a goroutine. That HTTP server is newly defined, and doesn't use DefaultServeMux. On it is registered the Prometheus stats handler, and handlers for the various pprof traces. In the process I split StatsAndLogging internally into two functions: makeStats and MakeLogger. I didn't port across the expvar variable exporting, which serves a similar function to Prometheus stats but which we never use.
One nice immediate effect of this change: Since StatsAndLogging now requires and address, I noticed a bunch of commands that called StatsAndLogging, and passed around the resulting Scope, but never made use of it because they didn't run a DebugServer. Under the old StatsD world, these command still could have exported their stats by pushing, but since we moved to Prometheus their stats stopped being collected. We haven't used any of these stats, so instead of adding debug ports to all short-lived commands, or setting up a push gateway, I simply removed them and switched those commands to initialize only a Logger, no stats.
Fixes#3020.
In order to write integration tests for some features, especially related to rate limiting, rechecking of CAA, and expiration of authzs, orders, and certs, we need to be able to fake the passage of time in integration tests.
To do so, this change switches out all clock.Default() instances for cmd.Clock(), which can be set manually with the FAKECLOCK environment variable. integration-test.py now starts up all servers once before the main body of tests, with FAKECLOCK set to a date 70 days ago, and does some initial setup for a new integration test case. That test case tries to fetch a 70-day-old authz URL, and expects it to 404.
In order to make this work, I also had to change a number of our test binaries to shut down cleanly in response to SIGTERM. Without that change, stopping the servers between the setup phase and the main tests caused startservers.check() to fail, because some processes exited with nonzero status.
Note: This is an initial stab at things, to prove out the technique. Long-term, I think we will want to use an idiom where test cases are classes that have a number of optional setup phases that may be run at e.g. 70 days prior and 5 days prior. This could help us avoid a proliferation of global state as we add more time-dependent test cases.
Previously, we would produce an error an a nonzero status code on shutdown,
because gRPC's GracefulStop would cause s.Serve() to return an error. Now we
filter that specific error and treat it as success. This also allows us to kill
process with SIGTERM instead of SIGKILL in integration tests.
Fixes#2410.
This used to be used for AMQP queue names. Now that AMQP is gone, these consts
were only used when printing a version string at startup. This changes
VersionString to just use the name of the current program, and removes
`const clientName = ` from many of our main.go's.
This removes the config and code to output to statsd.
- Change `cmd.StatsAndLogging` to output a `Scope`, not a `Statter`.
- Remove the prefixing of component name (e.g. "VA") in front of stats; this was stripped by `autoProm` but now no longer needs to be.
- Delete vendored statsd client.
- Delete `MockStatter` (generated by gomock) and `mocks.Statter` (hand generated) in favor of mocking `metrics.Scope`, which is the interface we now use everywhere.
- Remove a few unused methods on `metrics.Scope`, and update its generated mock.
- Refactor `autoProm` and add `autoRegisterer`, which can be included in a `metrics.Scope`, avoiding global state. `autoProm` now registers everything with the `prometheus.Registerer` it is given.
- Change va_test.go's `setup()` to not return a stats object; instead the individual tests that care about stats override `va.stats` directly.
Fixes#2639, #2733.
StatsAndLogging is called early enough in each program that it precedes any gRPC
setup code that might need SetLogger already to have been set.
Fixes#2383
The `Version()` function is a less useful alternative to
`VersionString()` and isn't necessary. It used a fixed "0.1.0" prefix
that doesn't match a release tag, it also doesn't print Go & host
information that `VersionString()` does.
Less code = less bugs!
Implements a less RPC focused signal catch/shutdown method. Certain things that probably could also use this (i.e. `ocsp-updater`) haven't been given it as they would require rather substantial changes to allow for a graceful shutdown approach.
Fixes#2298.
This vendors the Prometheus client code, and exports metrics on the debug port, under `/metrics`.
This will currently export just the default metrics, like `go_goroutines`, `process_cpu_seconds_total`, `process_open_fds`, and `process_resident_memory_bytes`. Later work will start exporting Boulder-specific metrics, but this will allow Ops to start configuring scraping of Prometheus metrics in production.
Tests pass:
```
$ git diff master Godeps/ | sed -ne 's/^+.*ImportPath": "//p' | tr -d '",' | xargs go test
ok github.com/beorn7/perks/quantile 0.562s
ok github.com/matttproud/golang_protobuf_extensions/pbutil 0.003s
ok github.com/prometheus/client_golang/prometheus 34.418s
ok github.com/prometheus/client_golang/prometheus/promhttp 0.003s
? github.com/prometheus/client_model/go [no test files]
ok github.com/prometheus/common/expfmt 0.019s
ok github.com/prometheus/common/internal/bitbucket.org/ww/goautoneg 0.002s
ok github.com/prometheus/common/model 0.003s
ok github.com/prometheus/procfs 0.008s
```
Part of #2284
Updates #1699.
Adds a new package, `features`, which exposes methods to set and check if various internal features are enabled. The implementation uses global state to store the features so that services embedded in another service do not each require their own features map in order to check if something is enabled.
Requires a `boulder-tools` image update to include `golang.org/x/tools/cmd/stringer`.
Moves the wfe to it's own config file.
Each config will now belong in `test/config` and `test/config-next` analogous to `boulder-config` and `boulder-config-next`.
Under the new defaults, if the syslog section is missing, we'll use the default
config that we use in prod: no logs to stdout, INFO and below to syslog.
This allows us to remove the syslog section from prod configs, and potentially
move it to individual service configs in the future.
* Improve syslog defaults.
* Add stdout logging for purger test.
* Use plain int for sysloglevel.
* Fix JSON syntax
* Fix syslog config for expired-authz-purger.
https://github.com/letsencrypt/boulder/pull/1932
Aaron Gable
Jacob Hoffman-Andrews
Roland Bracewell Shoemaker
Daniel McCarney
Roland Bracewell Shoemaker
Joel Sing
Daniel McCarney
Blake Griffith
Ben Irving