Config plan

Problems:

• Some of our constructors do file I/O or other expensive setup, when this should be done by helper functions outside of the constructor.
• Some of our components have a hodge-podge of exported and unexported fields, which creates subtle incorrectness.*
• The large config object in cmd/config.go is too broad, so it's not clear from a config file which elements are used by which components.
• The large config object in cmd/config.go depends on the va and publisher packages, because the config embeds types which are part of the constructor signature for those objects.

Plan:

Long term, each main.go should define a config struct inside it. To reduce duplication, this config struct will embed or contain other config structs, like AMQPConfig and DBConfig today.

Our config structs should be a thin layer on top of JSON. Any custom unmarshal methods we define on config structs should provide purely syntactic transforms. For instance, turning a string into a time.Duration is fine, or base64-decoding a string into []byte. But loading file contents should be done by a helper, not by UnmarshalJSON.

Components like VA or Publisher should not depend on config structs. This will mean some duplication of fields, but is worth it for separation of concerns. For instance, we have:

type PortConfig struct {
  HTTPPort  int
  TLSPort int
}
type ValidationAuthorityImpl struct {
  ...
  httpPort     int
  tlsPort    int
  ...
}

This seems redundant but the distinction becomes more clear if there is some initialization involved. Here's a hypothetical AMQP config change:

package rpc_confg // cmd/rpc_config ?
type AMQPConfig struct {
  TLS *TLSConfig
  ...
}
type TLSConfig struct {
  CertFile   *string
  KeyFile    *string
  CACertFile *string
}
func (tc *TLSConfig) Load() (*tls.Config, err) {
  certDER, err := ioutil.ReadFile(tc.CertFile)
  ...
}

package rpc
type AMQPRPCClient struct {
  ...
  tls *tls.Config
}

package main // cmd/boulder-ra/main.go
...
  tlsConf, err := conf.AMQP.TLSConfig.Load()
  saClient, err := rpc.NewAMQPRPCClient(..., tlsConf, ...)

Because tls takes file I/O and a separate object construction to build the tls.Config, we do it once in each cmd/.../main.go, with the assistance of the Load() helper, and pass it into any components that need it. Note that tls is unexported in rpc. It must be assigned by the AMQPRPCClient constructor.

Note that providing everything in the constructor has three problems:

• Calls to the constructor get very long and are likely to be copy-pasted rather than written from scratch, leading to errors.
• Positional arguments with the same type are likely to be confused. For instance, in the PortConfig example, if we turned HTTPPort and TLSPort into constructor arguments, anyone constructing a VA would have to remember (or look up) that HTTPPort was parameter 4 and TLSPort was parameter 5.
• Unexported fields that are set to default values in the constructor (e.g. clk = clock.Default()) cannot be overridden in tests outside of the package.

If these issues become a large problem, there are two good solutions: The Builder design pattern, where we have an intermediate object whose role is to accumulate named parameters, or the functional approach detailed at http://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis.

However, for the short term, I think it makes sense to work towards providing all prerequisites in constructors and making most fields of non-trivial objects unexported. We also want to move towards doing no file I/O, network connections, or other expensive operations in constructors. This is achievable without introducing new concepts into our codebase, and we can easily add those concepts as they become necessary.

*For instance, the result of concurrently assigning to an exported field and accessing it is usually not defined by the components. Also, the set of exported fields that must be assigned a working value for an object to work is not well defined. This is particularly a pattern in assigning client RPC objects to a service object like RA.