Very basic feature flag impl (letsencrypt#1705)

Roland Bracewell Shoemaker · jsha · commit 239bf9ae0a9e · 2016-09-20T16:29:01.000-07:00
Updates letsencrypt#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`.
diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -50,6 +50,14 @@ Note that there are some config fields that we want to be a hard requirement. To
 
 In general, we would like our deploy process to be: deploy new code + old config; then immediately after deploy the same code + new config. This makes deploys cheaper so we can do them more often, and allows us to more readily separate deploy-triggered problems from config-triggered problems.
 
+## Flag-gating features
+
+When adding significant new features or replacing existing RPCs the `boulder/features` package should be used to gate its usage. To add a flag a new `const FeatureFlag` should be added and its default value specified in `features.features`in `features/features.go`. In order to test if the flag is enabled elsewhere in the codebase you can use `features.Enabled(features.ExampleFeatureName)` which returns a `bool` indicating if the flag is enabled or not.
+
+Each service should include a `map[string]features.FeatureFlag` named `Features` in its configuration object at the top level and call `features.Set` with that map immediately after parsing the configuration.
+
+Feature flags are meant to be used temporarily and should not be used for permanent boolean configuration options. Once a feature has been enabled in both staging and production the flag should be removed making the previously gated functionality the default in future deployments.
+
 ## Flag-gated RPCs
 
 When you add a new RPC to a Boulder service (e.g. `SA.GetFoo()`), all components that call that RPC should wrap those calls in some flag. Generally this will be a boolean config field `UseFoo`. Since `UseFoo`'s zero value is false, a deploy with the existing config will not call `SA.GetFoo()`. Then, once the deploy is complete and we know that all SA instances support the `GetFoo()` RPC, we do a followup config deploy that sets `UseFoo()` to true.
diff --git a/Dockerfile b/Dockerfile
@@ -1,6 +1,6 @@
 # To minimize the fetching of various layers this image and tag should
 # be used as the base of the bhsm container in boulder/docker-compose.yml
-FROM letsencrypt/boulder-tools:2016-09-09
+FROM letsencrypt/boulder-tools:2016-09-16
 
 # Boulder exposes its web application at port TCP 4000
 EXPOSE 4000 4002 4003 8053 8055
diff --git a/bdns/dns.go b/bdns/dns.go
@@ -10,9 +10,10 @@ import (
 	"time"
 
 	"github.com/jmhodges/clock"
-	"github.com/letsencrypt/boulder/metrics"
 	"github.com/miekg/dns"
 	"golang.org/x/net/context"
+
+	"github.com/letsencrypt/boulder/metrics"
 )
 
 func parseCidr(network string, comment string) net.IPNet {
diff --git a/ca/ca.go b/ca/ca.go
@@ -374,7 +374,14 @@ func (ca *CertificateAuthorityImpl) GenerateOCSP(ctx context.Context, xferObj co
 func (ca *CertificateAuthorityImpl) IssueCertificate(ctx context.Context, csr x509.CertificateRequest, regID int64) (core.Certificate, error) {
 	emptyCert := core.Certificate{}
 
-	if err := csrlib.VerifyCSR(&csr, ca.maxNames, &ca.keyPolicy, ca.PA, ca.forceCNFromSAN, regID); err != nil {
+	if err := csrlib.VerifyCSR(
+		&csr,
+		ca.maxNames,
+		&ca.keyPolicy,
+		ca.PA,
+		ca.forceCNFromSAN,
+		regID,
+	); err != nil {
 		ca.log.AuditErr(err.Error())
 		return emptyCert, core.MalformedRequestError(err.Error())
 	}
diff --git a/ca/ca_test.go b/ca/ca_test.go
@@ -182,7 +182,6 @@ func setup(t *testing.T) *testCtx {
 		Expiry:       "8760h",
 		LifespanOCSP: cmd.ConfigDuration{Duration: 45 * time.Minute},
 		MaxNames:     2,
-		DoNotForceCN: true,
 		CFSSL: cfsslConfig.Config{
 			Signing: &cfsslConfig.Signing{
 				Profiles: map[string]*cfsslConfig.SigningProfile{
@@ -284,6 +283,7 @@ func TestIssueCertificate(t *testing.T) {
 		testCtx.keyPolicy,
 		testCtx.logger)
 	test.AssertNotError(t, err, "Failed to create CA")
+	ca.forceCNFromSAN = false
 	ca.Publisher = &mocks.Publisher{}
 	ca.PA = testCtx.pa
 	sa := &mockSA{}
@@ -557,6 +557,7 @@ func TestAllowNoCN(t *testing.T) {
 		testCtx.keyPolicy,
 		testCtx.logger)
 	test.AssertNotError(t, err, "Couldn't create new CA")
+	ca.forceCNFromSAN = false
 	ca.Publisher = &mocks.Publisher{}
 	ca.PA = testCtx.pa
 	ca.SA = &mockSA{}
@@ -721,13 +722,13 @@ func TestExtensions(t *testing.T) {
 		return cert
 	}
 
-	// With enableMustStaple = false, should issue successfully and not add
+	// With ca.enableMustStaple = false, should issue successfully and not add
 	// Must Staple.
 	stats.EXPECT().Inc(metricCSRExtensionTLSFeature, int64(1)).Return(nil)
 	noStapleCert := sign(mustStapleCSR)
 	test.AssertEquals(t, countMustStaple(t, noStapleCert), 0)
 
-	// With enableMustStaple = true, a TLS feature extension should put a must-staple
+	// With ca.enableMustStaple = true, a TLS feature extension should put a must-staple
 	// extension into the cert
 	ca.enableMustStaple = true
 	stats.EXPECT().Inc(metricCSRExtensionTLSFeature, int64(1)).Return(nil)
diff --git a/cmd/admin-revoker/main.go b/cmd/admin-revoker/main.go
@@ -147,7 +147,7 @@ func main() {
 	}
 
 	var c config
-	err = cmd.ReadJSONFile(*configFile, &c)
+	err = cmd.ReadConfigFile(*configFile, &c)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	ctx := context.Background()
diff --git a/cmd/boulder-ca/main.go b/cmd/boulder-ca/main.go
@@ -127,7 +127,7 @@ func main() {
 	}
 
 	var c config
-	err := cmd.ReadJSONFile(*configFile, &c)
+	err := cmd.ReadConfigFile(*configFile, &c)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	go cmd.DebugServer(c.CA.DebugAddr)
diff --git a/cmd/boulder-publisher/main.go b/cmd/boulder-publisher/main.go
@@ -45,7 +45,7 @@ func main() {
 	}
 
 	var c config
-	err := cmd.ReadJSONFile(*configFile, &c)
+	err := cmd.ReadConfigFile(*configFile, &c)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	go cmd.DebugServer(c.Publisher.DebugAddr)
diff --git a/cmd/boulder-ra/main.go b/cmd/boulder-ra/main.go
@@ -86,7 +86,7 @@ func main() {
 	}
 
 	var c config
-	err := cmd.ReadJSONFile(*configFile, &c)
+	err := cmd.ReadConfigFile(*configFile, &c)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	go cmd.DebugServer(c.RA.DebugAddr)
diff --git a/cmd/boulder-sa/main.go b/cmd/boulder-sa/main.go
@@ -36,7 +36,7 @@ func main() {
 	}
 
 	var c config
-	err := cmd.ReadJSONFile(*configFile, &c)
+	err := cmd.ReadConfigFile(*configFile, &c)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	go cmd.DebugServer(c.SA.DebugAddr)
diff --git a/cmd/boulder-va/main.go b/cmd/boulder-va/main.go
@@ -68,7 +68,7 @@ func main() {
 	}
 
 	var c config
-	err := cmd.ReadJSONFile(*configFile, &c)
+	err := cmd.ReadConfigFile(*configFile, &c)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	go cmd.DebugServer(c.VA.DebugAddr)
diff --git a/cmd/boulder-wfe/main.go b/cmd/boulder-wfe/main.go
@@ -74,7 +74,7 @@ func main() {
 	}
 
 	var c config
-	err := cmd.ReadJSONFile(*configFile, &c)
+	err := cmd.ReadConfigFile(*configFile, &c)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	go cmd.DebugServer(c.WFE.DebugAddr)
diff --git a/cmd/cert-checker/main.go b/cmd/cert-checker/main.go
@@ -263,7 +263,7 @@ func main() {
 	}
 
 	var config config
-	err := cmd.ReadJSONFile(*configFile, &config)
+	err := cmd.ReadConfigFile(*configFile, &config)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	stats, err := metrics.NewStatter(config.Statsd.Server, config.Statsd.Prefix)
diff --git a/cmd/expiration-mailer/main.go b/cmd/expiration-mailer/main.go
@@ -338,7 +338,7 @@ func main() {
 	}
 
 	var c config
-	err := cmd.ReadJSONFile(*configFile, &c)
+	err := cmd.ReadConfigFile(*configFile, &c)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	go cmd.DebugServer(c.Mailer.DebugAddr)
diff --git a/cmd/ocsp-responder/main.go b/cmd/ocsp-responder/main.go
@@ -164,7 +164,7 @@ as generated by Boulder's single-ocsp command.
 	}
 
 	var c config
-	err := cmd.ReadJSONFile(*configFile, &c)
+	err := cmd.ReadConfigFile(*configFile, &c)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	go cmd.DebugServer(c.OCSPResponder.DebugAddr)
diff --git a/cmd/ocsp-updater/main.go b/cmd/ocsp-updater/main.go
@@ -595,7 +595,7 @@ func main() {
 	}
 
 	var c config
-	err := cmd.ReadJSONFile(*configFile, &c)
+	err := cmd.ReadConfigFile(*configFile, &c)
 	cmd.FailOnError(err, "Reading JSON config file into config structure")
 
 	conf := c.OCSPUpdater
diff --git a/cmd/shell.go b/cmd/shell.go
@@ -20,7 +20,7 @@ import (
 	"encoding/json"
 	"encoding/pem"
 	"errors"
-	_ "expvar" // For DebugServer, below.
+	"expvar" // For DebugServer, below.
 	"fmt"
 	"io/ioutil"
 	"log"
@@ -37,6 +37,7 @@ import (
 	"github.com/go-sql-driver/mysql"
 
 	"github.com/letsencrypt/boulder/core"
+	"github.com/letsencrypt/boulder/features"
 	blog "github.com/letsencrypt/boulder/log"
 	"github.com/letsencrypt/boulder/metrics"
 )
@@ -192,6 +193,8 @@ func LoadCert(path string) (cert []byte, err error) {
 //
 //   go cmd.DebugServer(c.XA.DebugAddr)
 func DebugServer(addr string) {
+	m := expvar.NewMap("enabled-features")
+	features.Export(m)
 	if addr == "" {
 		log.Fatalf("unable to boot debug server because no address was given for it. Set debugAddr.")
 	}
@@ -205,19 +208,15 @@ func DebugServer(addr string) {
 	}
 }
 
-// ReadJSONFile takes a file path as an argument and attempts to
+// ReadConfigFile takes a file path as an argument and attempts to
 // unmarshal the content of the file into a struct containing a
 // configuration of a boulder component.
-func ReadJSONFile(filename string, out interface{}) error {
+func ReadConfigFile(filename string, out interface{}) error {
 	configData, err := ioutil.ReadFile(filename)
 	if err != nil {
 		return err
 	}
-	err = json.Unmarshal(configData, out)
-	if err != nil {
-		return err
-	}
-	return nil
+	return json.Unmarshal(configData, out)
 }
 
 // VersionString produces a friendly Application version string.
diff --git a/docker-compose.yml b/docker-compose.yml
@@ -31,7 +31,7 @@ boulder:
 bhsm:
     # To minimize the fetching of various layers this should match
     # the FROM image and tag in boulder/Dockerfile
-    image: letsencrypt/boulder-tools:2016-09-09
+    image: letsencrypt/boulder-tools:2016-09-16
     environment:
         PKCS11_DAEMON_SOCKET: tcp://0.0.0.0:5657
     command: /usr/local/bin/pkcs11-daemon /usr/lib/softhsm/libsofthsm.so
diff --git a/features/featureflag_string.go b/features/featureflag_string.go
diff --git a/features/features.go b/features/features.go
@@ -0,0 +1,86 @@
+//go:generate stringer -type=FeatureFlag
+
+package features
+
+import (
+	"expvar"
+	"fmt"
+	"sync"
+)
+
+type FeatureFlag int
+
+const (
+	unused FeatureFlag = iota // unused is used for testing
+)
+
+// List of features and their default value, protected by fMu
+var features = map[FeatureFlag]bool{
+	unused: false,
+}
+
+var fMu = new(sync.RWMutex)
+
+var initial = map[FeatureFlag]bool{}
+
+var nameToFeature = make(map[string]FeatureFlag, len(features))
+
+func init() {
+	for f, v := range features {
+		nameToFeature[f.String()] = f
+		initial[f] = v
+	}
+}
+
+// expvar.Set requires a type that satisfies the expvar.Var interface,
+// since neither string nor bool implement this interface we require
+// a basic shim.
+type boolVar bool
+
+func (b boolVar) String() string { return fmt.Sprintf("%t", b) }
+
+// Set accepts a list of features and whether they should
+// be enabled or disabled, it will return a error if passed
+// a feature name that it doesn't know
+func Set(featureSet map[string]bool) error {
+	fMu.Lock()
+	defer fMu.Unlock()
+	for n, v := range featureSet {
+		f, present := nameToFeature[n]
+		if !present {
+			return fmt.Errorf("feature '%s' doesn't exist", n)
+		}
+		features[f] = v
+	}
+	return nil
+}
+
+// Export populates a expvar.Map with the state of all
+// of the features.
+func Export(m *expvar.Map) {
+	fMu.RLock()
+	defer fMu.RUnlock()
+	for f, v := range features {
+		m.Set(f.String(), boolVar(v))
+	}
+}
+
+// Enabled returns true if the feature is enabled or false
+// if it isn't, it will panic if passed a feature that it
+// doesn't know.
+func Enabled(n FeatureFlag) bool {
+	fMu.RLock()
+	defer fMu.RUnlock()
+	v, present := features[n]
+	if !present {
+		panic(fmt.Sprintf("feature '%s' doesn't exist", n))
+	}
+	return v
+}
+
+// Reset resets the features to their initial state
+func Reset() {
+	fMu.Lock()
+	defer fMu.Unlock()
+	features = initial
+}
diff --git a/features/features_test.go b/features/features_test.go
@@ -0,0 +1,43 @@
+package features
+
+import (
+	"expvar"
+	"testing"
+
+	"github.com/letsencrypt/boulder/test"
+)
+
+func TestFeatures(t *testing.T) {
+	features = map[FeatureFlag]bool{
+		unused: false,
+	}
+	test.Assert(t, !Enabled(unused), "'unused' shouldn't be enabled")
+
+	err := Set(map[string]bool{"unused": true})
+	test.AssertNotError(t, err, "Set shouldn't have failed setting existing features")
+	test.Assert(t, Enabled(unused), "'unused' should be enabled")
+
+	Reset()
+	test.Assert(t, !Enabled(unused), "'unused' shouldn't be enabled")
+
+	err = Set(map[string]bool{"non-existent": true})
+	test.AssertError(t, err, "Set should've failed trying to enable a non-existent feature")
+
+	defer func() {
+		if r := recover(); r == nil {
+			t.Errorf("Enabled did not panic on an unknown feature")
+		}
+	}()
+	features = map[FeatureFlag]bool{}
+	Enabled(unused)
+}
+
+func TestExport(t *testing.T) {
+	features = map[FeatureFlag]bool{
+		unused: false,
+	}
+	m := expvar.NewMap("testing")
+	Export(m)
+	v := m.Get("unused")
+	test.AssertEquals(t, v.String(), "false")
+}
diff --git a/ra/ra_test.go b/ra/ra_test.go
diff --git a/test/boulder-tools/build.sh b/test/boulder-tools/build.sh
diff --git a/test/config-next/ra.json b/test/config-next/ra.json
diff --git a/test/config-next/va.json b/test/config-next/va.json
diff --git a/test/setup.sh b/test/setup.sh

Original file line number	Diff line number	Diff line change
`@@ -147,7 +147,7 @@ func main() {`
`147`	`147`	`}`
`148`	`148`
`149`	`149`	`var c config`
`150`		`- err = cmd.ReadJSONFile(*configFile, &c)`
	`150`	`+ err = cmd.ReadConfigFile(*configFile, &c)`
`151`	`151`	`cmd.FailOnError(err, "Reading JSON config file into config structure")`
`152`	`152`
`153`	`153`	`ctx := context.Background()`
Original file line number	Diff line number	Diff line change
`@@ -127,7 +127,7 @@ func main() {`
`127`	`127`	`}`
`128`	`128`
`129`	`129`	`var c config`
`130`		`- err := cmd.ReadJSONFile(*configFile, &c)`
	`130`	`+ err := cmd.ReadConfigFile(*configFile, &c)`
`131`	`131`	`cmd.FailOnError(err, "Reading JSON config file into config structure")`
`132`	`132`
`133`	`133`	`go cmd.DebugServer(c.CA.DebugAddr)`
Original file line number	Diff line number	Diff line change
`@@ -45,7 +45,7 @@ func main() {`
`45`	`45`	`}`
`46`	`46`
`47`	`47`	`var c config`
`48`		`- err := cmd.ReadJSONFile(*configFile, &c)`
	`48`	`+ err := cmd.ReadConfigFile(*configFile, &c)`
`49`	`49`	`cmd.FailOnError(err, "Reading JSON config file into config structure")`
`50`	`50`
`51`	`51`	`go cmd.DebugServer(c.Publisher.DebugAddr)`
Original file line number	Diff line number	Diff line change
`@@ -86,7 +86,7 @@ func main() {`
`86`	`86`	`}`
`87`	`87`
`88`	`88`	`var c config`
`89`		`- err := cmd.ReadJSONFile(*configFile, &c)`
	`89`	`+ err := cmd.ReadConfigFile(*configFile, &c)`
`90`	`90`	`cmd.FailOnError(err, "Reading JSON config file into config structure")`
`91`	`91`
`92`	`92`	`go cmd.DebugServer(c.RA.DebugAddr)`
Original file line number	Diff line number	Diff line change
`@@ -36,7 +36,7 @@ func main() {`
`36`	`36`	`}`
`37`	`37`
`38`	`38`	`var c config`
`39`		`- err := cmd.ReadJSONFile(*configFile, &c)`
	`39`	`+ err := cmd.ReadConfigFile(*configFile, &c)`
`40`	`40`	`cmd.FailOnError(err, "Reading JSON config file into config structure")`
`41`	`41`
`42`	`42`	`go cmd.DebugServer(c.SA.DebugAddr)`