storageconfig: move StoreEncryptionSpec to cli

The separate encryption specs are a byproduct of having to use a
separate command-line flag. This change moves the related code to the
cli package.

Epic: none
Release note: None

Author: RaduBerinde

pkg/base/store_spec.go

@@ -333,64 +333,3 @@ func (ssl *StoreSpecList) Set(value string) error {
 	}
 	return nil
 }
-
-// PopulateWithEncryptionOpts iterates through the EncryptionSpecList and looks
-// for matching paths in the StoreSpecList and WAL failover config. Any
-// unmatched EncryptionSpec causes an error.
-func PopulateWithEncryptionOpts(
-	storeSpecs StoreSpecList,
-	walFailoverConfig *storageconfig.WALFailover,
-	encryptionSpecs storageconfig.EncryptionSpecList,
-) error {
-	for _, es := range encryptionSpecs.Specs {
-		var storeMatched bool
-		for i := range storeSpecs.Specs {
-			if !es.PathMatches(storeSpecs.Specs[i].Path) {
-				continue
-			}
-
-			// Found a matching path.
-			if storeSpecs.Specs[i].EncryptionOptions != nil {
-				return fmt.Errorf("store with path %s already has an encryption setting",
-					storeSpecs.Specs[i].Path)
-			}
-
-			storeSpecs.Specs[i].EncryptionOptions = &es.Options
-			storeMatched = true
-			break
-		}
-		pathMatched, err := maybeSetExternalPathEncryption(&walFailoverConfig.Path, es)
-		if err != nil {
-			return err
-		}
-		prevPathMatched, err := maybeSetExternalPathEncryption(&walFailoverConfig.PrevPath, es)
-		if err != nil {
-			return err
-		}
-		if !storeMatched && !pathMatched && !prevPathMatched {
-			return fmt.Errorf("no usage of path %s found for encryption setting: %v", es.Path, es)
-		}
-	}
-	return nil
-}
-
-// maybeSetExternalPathEncryption updates an ExternalPath to contain the provided
-// encryption options if the path matches. The ExternalPath must not already have
-// an encryption setting.
-func maybeSetExternalPathEncryption(
-	externalPath *storageconfig.ExternalPath, es storageconfig.StoreEncryptionSpec,
-) (found bool, err error) {
-	if !externalPath.IsSet() || !es.PathMatches(externalPath.Path) {
-		return false, nil
-	}
-	// NB: The external paths WALFailoverConfig.Path and
-	// WALFailoverConfig.PrevPath are only ever set in single-store
-	// configurations. In multi-store with among-stores failover mode, these
-	// will be empty (so we won't encounter the same path twice).
-	if externalPath.Encryption != nil {
-		return false, fmt.Errorf("WAL failover path %s already has an encryption setting",
-			externalPath.Path)
-	}
-	externalPath.Encryption = &es.Options
-	return true, nil
-}

pkg/cli/context.go

@@ -720,23 +720,3 @@ var userfileCtx struct {
 func setUserfileContextDefaults() {
 	userfileCtx.recursive = false
 }
-
-// GetServerCfgStores provides direct public access to the StoreSpecList inside
-// serverCfg. This is used by CCL code to populate some fields.
-//
-// WARNING: consider very carefully whether you should be using this.
-// If you are not writing CCL code that performs command-line flag
-// parsing, you probably should not be using this.
-func GetServerCfgStores() base.StoreSpecList {
-	return serverCfg.Stores
-}
-
-// GetWALFailoverConfig provides direct public access to the WALFailoverConfig
-// inside serverCfg. This is used by CCL code to populate some fields.
-//
-// WARNING: consider very carefully whether you should be using this.
-// If you are not writing CCL code that performs command-line flag
-// parsing, you probably should not be using this.
-func GetWALFailoverConfig() *storageconfig.WALFailover {
-	return &serverCfg.StorageConfig.WALFailover
-}

pkg/cli/debug_ear.go

@@ -20,7 +20,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
-	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
@@ -169,7 +168,7 @@ with their env type and encryption settings (if applicable).
 // fillEncryptionOptionsForStore fills the EnvConfig fields
 // based on the --enterprise-encryption flag value.
 func fillEncryptionOptionsForStore(dir string, cfg *fs.EnvConfig) error {
-	opts, err := storageconfig.EncryptionOptionsForStore(dir, encryptionSpecs)
+	opts, err := encryptionOptionsForStore(dir, encryptionSpecs)
 	if err != nil {
 		return err
 	}

pkg/cli/ear_test.go

@@ -18,7 +18,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
-	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -44,7 +43,7 @@ func TestDecrypt(t *testing.T) {
 
 	// Spin up a new encrypted store.
 	encSpecStr := fmt.Sprintf("path=%s,key=%s,old-key=plain", dir, keyPath)
-	encSpec, err := storageconfig.NewStoreEncryptionSpec(encSpecStr)
+	encSpec, err := parseStoreEncryptionSpec(encSpecStr)
 	require.NoError(t, err)
 	encOpts := &encSpec.Options
 
@@ -126,7 +125,7 @@ func TestList(t *testing.T) {
 
 	// Spin up a new encrypted store.
 	encSpecStr := fmt.Sprintf("path=%s,key=%s,old-key=plain", dir, keyPath)
-	encSpec, err := storageconfig.NewStoreEncryptionSpec(encSpecStr)
+	encSpec, err := parseStoreEncryptionSpec(encSpecStr)
 	require.NoError(t, err)
 	encOpts := &encSpec.Options
 	env, err := fs.InitEnv(ctx, vfs.Default, dir, fs.EnvConfig{EncryptionOptions: encOpts}, nil /* statsCollector */)

pkg/cli/flags.go

@@ -62,7 +62,7 @@ var storeSpecs base.StoreSpecList
 var goMemLimit int64
 var tenantIDFile string
 var localityFile string
-var encryptionSpecs storageconfig.EncryptionSpecList
+var encryptionSpecs encryptionSpecList
 
 // initPreFlagsDefaults initializes the values of the global variables
 // defined above.
@@ -431,7 +431,11 @@ func init() {
 
 		// Add a new pre-run command to match encryption specs to store specs.
 		AddPersistentPreRunE(cmd, func(cmd *cobra.Command, _ []string) error {
-			return populateStoreSpecsEncryption()
+			return populateWithEncryptionOpts(
+				serverCfg.Stores,
+				&serverCfg.StorageConfig.WALFailover,
+				encryptionSpecs,
+			)
 		})
 	}
 
@@ -1485,17 +1489,6 @@ func mtStartSQLFlagsInit(cmd *cobra.Command) error {
 	return nil
 }
 
-// populateStoreSpecsEncryption is a PreRun hook that matches store encryption
-// specs with the parsed stores and populates some fields in the StoreSpec and
-// WAL failover config.
-func populateStoreSpecsEncryption() error {
-	return base.PopulateWithEncryptionOpts(
-		GetServerCfgStores(),
-		GetWALFailoverConfig(),
-		encryptionSpecs,
-	)
-}
-
 // sizeFlagVal is a pflag.Value wrapper for storageconfig.Size. It can be
 // set to an absolute bytes amount or a percentage.
 type sizeFlagVal struct {

pkg/cli/flags_test.go

@@ -27,6 +27,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security/securitytest"
 	"github.com/cockroachdb/cockroach/pkg/server/status"
+	"github.com/cockroachdb/cockroach/pkg/storage/storageconfig"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/util"
@@ -1731,3 +1732,134 @@ func TestTenantIDFromFile(t *testing.T) {
 			require.Eventually(t, func() bool { return runSuccessfuly.Load() }, 10*time.Second, 10*time.Millisecond)
 		})
 }
+
+// TestParseEncryptionSpec verifies that the --enterprise-encryption arguments
+// are correctly parsed into storeEncryptionSpecs.
+func TestParseEncryptionSpec(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	absDataPath, err := filepath.Abs("data")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	testCases := []struct {
+		value       string
+		expectedErr string
+		expected    storeEncryptionSpec
+	}{
+		// path
+		{",", "no path specified", storeEncryptionSpec{}},
+		{"", "no path specified", storeEncryptionSpec{}},
+		{"/mnt/hda1", "field not in the form <key>=<value>: /mnt/hda1", storeEncryptionSpec{}},
+		{"path=", "no value specified for path", storeEncryptionSpec{}},
+		{"path=~/data", "path cannot start with '~': ~/data", storeEncryptionSpec{}},
+		{"path=data,path=data2", "path field was used twice in encryption definition", storeEncryptionSpec{}},
+
+		// The same logic applies to key and old-key, don't repeat everything.
+		{"path=data", "no key specified", storeEncryptionSpec{}},
+		{"path=data,key=new.key", "no old-key specified", storeEncryptionSpec{}},
+
+		// Rotation period.
+		{"path=data,key=new.key,old-key=old.key,rotation-period", "field not in the form <key>=<value>: rotation-period", storeEncryptionSpec{}},
+		{"path=data,key=new.key,old-key=old.key,rotation-period=", "no value specified for rotation-period", storeEncryptionSpec{}},
+		{"path=data,key=new.key,old-key=old.key,rotation-period=1", `could not parse rotation-duration value: 1: time: missing unit in duration "1"`, storeEncryptionSpec{}},
+		{"path=data,key=new.key,old-key=old.key,rotation-period=1d", `could not parse rotation-duration value: 1d: time: unknown unit "d" in duration "1d"`, storeEncryptionSpec{}},
+
+		// Good values. Note that paths get absolutized so we start most of them
+		// with / so we can used fixed expected values.
+		{
+			"path=/data,key=/new.key,old-key=/old.key", "",
+			storeEncryptionSpec{Path: "/data",
+				Options: storageconfig.EncryptionOptions{
+					KeyFiles:              &storageconfig.EncryptionKeyFiles{CurrentKey: "/new.key", OldKey: "/old.key"},
+					DataKeyRotationPeriod: int64(storageconfig.DefaultRotationPeriod / time.Second),
+				},
+			},
+		},
+		{
+			"path=/data,key=/new.key,old-key=/old.key,rotation-period=1h", "",
+			storeEncryptionSpec{Path: "/data",
+				Options: storageconfig.EncryptionOptions{
+					KeyFiles:              &storageconfig.EncryptionKeyFiles{CurrentKey: "/new.key", OldKey: "/old.key"},
+					DataKeyRotationPeriod: int64(time.Hour / time.Second),
+				},
+			},
+		},
+		{
+			"path=/data,key=plain,old-key=/old.key,rotation-period=1h", "",
+			storeEncryptionSpec{Path: "/data",
+				Options: storageconfig.EncryptionOptions{
+					KeyFiles:              &storageconfig.EncryptionKeyFiles{CurrentKey: "plain", OldKey: "/old.key"},
+					DataKeyRotationPeriod: int64(time.Hour / time.Second),
+				},
+			},
+		},
+		{
+			"path=/data,key=/new.key,old-key=plain,rotation-period=1h", "",
+			storeEncryptionSpec{Path: "/data",
+				Options: storageconfig.EncryptionOptions{
+					KeyFiles:              &storageconfig.EncryptionKeyFiles{CurrentKey: "/new.key", OldKey: "plain"},
+					DataKeyRotationPeriod: int64(time.Hour / time.Second),
+				},
+			},
+		},
+
+		// One relative path to test absolutization.
+		{
+			"path=data,key=/new.key,old-key=/old.key", "",
+			storeEncryptionSpec{Path: absDataPath,
+				Options: storageconfig.EncryptionOptions{
+					KeyFiles:              &storageconfig.EncryptionKeyFiles{CurrentKey: "/new.key", OldKey: "/old.key"},
+					DataKeyRotationPeriod: int64(storageconfig.DefaultRotationPeriod / time.Second),
+				},
+			},
+		},
+
+		// Special path * is not absolutized.
+		{
+			"path=*,key=/new.key,old-key=/old.key", "",
+			storeEncryptionSpec{Path: "*",
+				Options: storageconfig.EncryptionOptions{
+					KeyFiles:              &storageconfig.EncryptionKeyFiles{CurrentKey: "/new.key", OldKey: "/old.key"},
+					DataKeyRotationPeriod: int64(storageconfig.DefaultRotationPeriod / time.Second),
+				},
+			},
+		},
+	}
+
+	for i, testCase := range testCases {
+		storeEncryptionSpec, err := parseStoreEncryptionSpec(testCase.value)
+		if err != nil {
+			if len(testCase.expectedErr) == 0 {
+				t.Errorf("%d(%s): no expected error, got %s", i, testCase.value, err)
+			}
+			if testCase.expectedErr != fmt.Sprint(err) {
+				t.Errorf("%d(%s): expected error \"%s\" does not match actual \"%s\"", i, testCase.value,
+					testCase.expectedErr, err)
+			}
+			continue
+		}
+		if len(testCase.expectedErr) > 0 {
+			t.Errorf("%d(%s): expected error %s but there was none", i, testCase.value, testCase.expectedErr)
+			continue
+		}
+		if !reflect.DeepEqual(testCase.expected, storeEncryptionSpec) {
+			t.Errorf("%d(%s): actual doesn't match expected\nactual:   %+v\nexpected: %+v", i,
+				testCase.value, storeEncryptionSpec, testCase.expected)
+		}
+
+		// Now test String() to make sure the result can be parsed.
+		storeEncryptionSpecString := storeEncryptionSpec.String()
+		storeEncryptionSpec2, err := parseStoreEncryptionSpec(storeEncryptionSpecString)
+		if err != nil {
+			t.Errorf("%d(%s): error parsing String() result: %s", i, testCase.value, err)
+			continue
+		}
+		// Compare strings to deal with floats not matching exactly.
+		if !reflect.DeepEqual(storeEncryptionSpecString, storeEncryptionSpec2.String()) {
+			t.Errorf("%d(%s): actual doesn't match expected\nactual:   %#+v\nexpected: %#+v", i, testCase.value,
+				storeEncryptionSpec, storeEncryptionSpec2)
+		}
+	}
+}