From 29249b4aadd4da3608db2d72cd30523333355cef Mon Sep 17 00:00:00 2001 From: Jacob Hoffman-Andrews Date: Thu, 21 Apr 2022 18:05:18 -0700 Subject: [PATCH] Add feature flag AllowUnrecognizedFeatures (#6056) By default, Boulder's feature flag code verifies that the list of flags being set (from a JSON file) maps to actually-existing flags. However, this gets in the way of a deployment strategy where feature flags are added to config templates during a staging deploy with "true" or "false" filled in depending on production or staging status - for instance, when rolling out a deprecation to staging ahead of production. If those configs get rolled to prod before the corresponding Boulder deploy, Boulder will refuse to start up, even though it would be fine to start up with the unrecognized flag ignored. The envisioned deployment behavior here is that prod will have AllowUnrecognizedFeatures: true while staging will have it set to false, to ensure that misspellings of feature flag names are caught during staging deploy. As a correlary, this assumes that the list of flags in configs will be the same between staging and prod, with only their values changing. --- features/featureflag_string.go | 5 +++-- features/features.go | 22 +++++++++++++++++----- features/features_test.go | 26 ++++++++++++++++++++++++++ 3 files changed, 46 insertions(+), 7 deletions(-) diff --git a/features/featureflag_string.go b/features/featureflag_string.go index 1716dcb69ab..e1900800ae8 100644 --- a/features/featureflag_string.go +++ b/features/featureflag_string.go @@ -34,11 +34,12 @@ func _() { _ = x[OldTLSOutbound-23] _ = x[OldTLSInbound-24] _ = x[SHA1CSRs-25] + _ = x[AllowUnrecognizedFeatures-26] } -const _FeatureFlag_name = "unusedPrecertificateRevocationStripDefaultSchemePortNonCFSSLSignerStoreIssuerInfoStreamlineOrderAndAuthzsV1DisableNewValidationsCAAValidationMethodsCAAAccountURIEnforceMultiVAMultiVAFullResultsMandatoryPOSTAsGETAllowV1RegistrationStoreRevokerInfoRestrictRSAKeySizesFasterNewOrdersRateLimitECDSAForAllServeRenewalInfoGetAuthzReadOnlyGetAuthzUseIndexCheckFailedAuthorizationsFirstAllowReRevocationMozRevocationReasonsOldTLSOutboundOldTLSInboundSHA1CSRs" +const _FeatureFlag_name = "unusedPrecertificateRevocationStripDefaultSchemePortNonCFSSLSignerStoreIssuerInfoStreamlineOrderAndAuthzsV1DisableNewValidationsCAAValidationMethodsCAAAccountURIEnforceMultiVAMultiVAFullResultsMandatoryPOSTAsGETAllowV1RegistrationStoreRevokerInfoRestrictRSAKeySizesFasterNewOrdersRateLimitECDSAForAllServeRenewalInfoGetAuthzReadOnlyGetAuthzUseIndexCheckFailedAuthorizationsFirstAllowReRevocationMozRevocationReasonsOldTLSOutboundOldTLSInboundSHA1CSRsAllowUnrecognizedFeatures" -var _FeatureFlag_index = [...]uint16{0, 6, 30, 52, 66, 81, 105, 128, 148, 161, 175, 193, 211, 230, 246, 265, 289, 300, 316, 332, 348, 378, 395, 415, 429, 442, 450} +var _FeatureFlag_index = [...]uint16{0, 6, 30, 52, 66, 81, 105, 128, 148, 161, 175, 193, 211, 230, 246, 265, 289, 300, 316, 332, 348, 378, 395, 415, 429, 442, 450, 475} func (i FeatureFlag) String() string { if i < 0 || i >= FeatureFlag(len(_FeatureFlag_index)-1) { diff --git a/features/features.go b/features/features.go index 52fd67bfb54..2e66ea025fd 100644 --- a/features/features.go +++ b/features/features.go @@ -4,6 +4,7 @@ package features import ( "fmt" + "strings" "sync" ) @@ -88,6 +89,9 @@ const ( // SHA1CSRs controls whether the /acme/finalize endpoint rejects CSRs that // are self-signed using SHA1. SHA1CSRs + // AllowUnrecognizedFeatures is internal to the features package: if true, + // skip error when unrecognized feature flag names are passed. + AllowUnrecognizedFeatures ) // List of features and their default value, protected by fMu @@ -118,6 +122,7 @@ var features = map[FeatureFlag]bool{ OldTLSOutbound: true, OldTLSInbound: true, SHA1CSRs: true, + AllowUnrecognizedFeatures: false, } var fMu = new(sync.RWMutex) @@ -134,17 +139,24 @@ func init() { } // 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 +// be enabled or disabled. In the presence of unrecognized +// flags, it will return an error or not depending on the +// value of AllowUnrecognizedFeatures. func Set(featureSet map[string]bool) error { fMu.Lock() defer fMu.Unlock() + var unknown []string for n, v := range featureSet { f, present := nameToFeature[n] - if !present { - return fmt.Errorf("feature '%s' doesn't exist", n) + if present { + features[f] = v + } else { + unknown = append(unknown, n) } - features[f] = v + } + if len(unknown) > 0 && !features[AllowUnrecognizedFeatures] { + return fmt.Errorf("unrecognized feature flag names: %s", + strings.Join(unknown, ", ")) } return nil } diff --git a/features/features_test.go b/features/features_test.go index dff198d68e4..7ab167ebf11 100644 --- a/features/features_test.go +++ b/features/features_test.go @@ -6,6 +6,32 @@ import ( "github.com/letsencrypt/boulder/test" ) +func TestAllowUnrecognizedFeatures(t *testing.T) { + features = map[FeatureFlag]bool{ + unused: false, + AllowUnrecognizedFeatures: false, + } + nameToFeature = map[string]FeatureFlag{ + "unused": unused, + "AllowUnrecognizedFeatures": AllowUnrecognizedFeatures, + } + err := Set(map[string]bool{ + "Z4lG0": true, + "AllowUnrecognizedFeatures": true, + }) + test.AssertNotError(t, err, "expected no error when setting an unrecognized feature") + + err = Set(map[string]bool{ + "Z4lG0": true, + "Zombo": true, + "AllowUnrecognizedFeatures": false, + }) + test.AssertError(t, err, "expected error when disallowing unrecognized features") + test.AssertContains(t, err.Error(), "unrecognized feature flag names: ") + test.AssertContains(t, err.Error(), "Z4lG0") + test.AssertContains(t, err.Error(), "Zombo") +} + func TestFeatures(t *testing.T) { features = map[FeatureFlag]bool{ unused: false,