Skip to content

Commit 37cacac

Browse files
andrewbaptistandrewbaptist
committed
storage: use EncryptionOptions instead of byte[]
Previously the encryption options were passed as a byte[] instead of actual EncryptionOptions to allow them in CCL code. This change removes the unnecessary conversion. Epic: CRDB-41111 Release note: None
1 parent 041f4dd commit 37cacac

15 files changed

+143
-103
lines changed

‎pkg/base/config.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import (
1818
"github.com/cockroachdb/cockroach/pkg/roachpb"
1919
"github.com/cockroachdb/cockroach/pkg/security/username"
2020
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
21+
"github.com/cockroachdb/cockroach/pkg/storage/storagepb"
2122
"github.com/cockroachdb/cockroach/pkg/util/envutil"
2223
"github.com/cockroachdb/cockroach/pkg/util/mon"
2324
"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -933,10 +934,8 @@ type WALFailoverConfig struct {
933934
// configuration.
934935
type ExternalPath struct {
935936
Path string
936-
// EncryptionOptions is a serialized protobuf set by Go CCL code describing
937-
// the encryption-at-rest configuration. If encryption-at-rest has ever been
938-
// enabled on the store, this field must be set.
939-
EncryptionOptions []byte
937+
// EncryptionOptions is set if encryption is enabled.
938+
EncryptionOptions *storagepb.EncryptionOptions
940939
}
941940

942941
// IsSet returns whether or not the external path was provided.

‎pkg/base/store_spec.go

Lines changed: 7 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -215,10 +215,8 @@ type StoreSpec struct {
215215
// delete_range_flush_delay=2s
216216
// flush_split_bytes=4096
217217
PebbleOptions string
218-
// EncryptionOptions is a serialized protobuf set by Go CCL code and passed
219-
// through to C CCL code to set up encryption-at-rest. Must be set if and
220-
// only if encryption is enabled, otherwise left empty.
221-
EncryptionOptions []byte
218+
// EncryptionOptions is set if encryption is enabled.
219+
EncryptionOptions *storagepb.EncryptionOptions
222220
// ProvisionedRateSpec is optional.
223221
ProvisionedRateSpec ProvisionedRateSpec
224222
}
@@ -276,7 +274,7 @@ func (ss StoreSpec) String() string {
276274

277275
// IsEncrypted returns whether the StoreSpec has encryption enabled.
278276
func (ss StoreSpec) IsEncrypted() bool {
279-
return len(ss.EncryptionOptions) > 0
277+
return ss.EncryptionOptions != nil
280278
}
281279

282280
// fractionRegex is the regular expression that recognizes whether
@@ -639,16 +637,12 @@ func PopulateWithEncryptionOpts(
639637
}
640638

641639
// Found a matching path.
642-
if len(storeSpecs.Specs[i].EncryptionOptions) > 0 {
640+
if storeSpecs.Specs[i].EncryptionOptions != nil {
643641
return fmt.Errorf("store with path %s already has an encryption setting",
644642
storeSpecs.Specs[i].Path)
645643
}
646644

647-
opts, err := es.ToEncryptionOptions()
648-
if err != nil {
649-
return err
650-
}
651-
storeSpecs.Specs[i].EncryptionOptions = opts
645+
storeSpecs.Specs[i].EncryptionOptions = &es.Options
652646
found = true
653647
break
654648
}
@@ -661,15 +655,11 @@ func PopulateWithEncryptionOpts(
661655
// WALFailoverConfig.PrevPath are only ever set in single-store
662656
// configurations. In multi-store with among-stores failover mode, these
663657
// will be empty (so we won't encounter the same path twice).
664-
if len(externalPath.EncryptionOptions) > 0 {
658+
if externalPath.EncryptionOptions != nil {
665659
return fmt.Errorf("WAL failover path %s already has an encryption setting",
666660
externalPath.Path)
667661
}
668-
opts, err := es.ToEncryptionOptions()
669-
if err != nil {
670-
return err
671-
}
672-
externalPath.EncryptionOptions = opts
662+
externalPath.EncryptionOptions = &es.Options
673663
found = true
674664
}
675665

‎pkg/ccl/storageccl/engineccl/encrypted_fs.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -287,17 +287,16 @@ func init() {
287287
}
288288

289289
// newEncryptedEnv creates an encrypted environment and returns the vfs.FS to use for reading and
290-
// writing data. The optionBytes is a binary serialized storagepb.EncryptionOptions, so that non-CCL
291-
// code does not depend on CCL code.
290+
// writing data.
292291
//
293292
// See the comment at the top of this file for the structure of this environment.
294293
func newEncryptedEnv(
295-
unencryptedFS vfs.FS, fr *fs.FileRegistry, dbDir string, readOnly bool, optionBytes []byte,
294+
unencryptedFS vfs.FS,
295+
fr *fs.FileRegistry,
296+
dbDir string,
297+
readOnly bool,
298+
options *storagepb.EncryptionOptions,
296299
) (*fs.EncryptionEnv, error) {
297-
options := &storagepb.EncryptionOptions{}
298-
if err := protoutil.Unmarshal(optionBytes, options); err != nil {
299-
return nil, err
300-
}
301300
if options.KeySource != storagepb.EncryptionKeySource_KeyFiles {
302301
return nil, fmt.Errorf("unknown encryption key source: %d", options.KeySource)
303302
}

‎pkg/ccl/storageccl/engineccl/encrypted_fs_test.go

Lines changed: 17 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -236,15 +236,14 @@ func TestPebbleEncryption(t *testing.T) {
236236
keyFile128 := "111111111111111111111111111111111234567890123456"
237237
writeToFile(t, stickyRegistry.Get(stickyVFSID), "16.key", []byte(keyFile128))
238238

239-
encOptionsBytes, err := protoutil.Marshal(&storagepb.EncryptionOptions{
239+
encOptions := &storagepb.EncryptionOptions{
240240
KeySource: storagepb.EncryptionKeySource_KeyFiles,
241241
KeyFiles: &storagepb.EncryptionKeyFiles{
242242
CurrentKey: "16.key",
243243
OldKey: "plain",
244244
},
245245
DataKeyRotationPeriod: 1000, // arbitrary seconds
246-
})
247-
require.NoError(t, err)
246+
}
248247

249248
func() {
250249
// Initialize the filesystem env.
@@ -254,7 +253,7 @@ func TestPebbleEncryption(t *testing.T) {
254253
InMemory: true,
255254
Attributes: roachpb.Attributes{},
256255
Size: base.SizeSpec{InBytes: 512 << 20},
257-
EncryptionOptions: encOptionsBytes,
256+
EncryptionOptions: encOptions,
258257
StickyVFSID: stickyVFSID,
259258
},
260259
fs.ReadWrite,
@@ -303,7 +302,7 @@ func TestPebbleEncryption(t *testing.T) {
303302
InMemory: true,
304303
Attributes: roachpb.Attributes{},
305304
Size: base.SizeSpec{InBytes: 512 << 20},
306-
EncryptionOptions: encOptionsBytes,
305+
EncryptionOptions: encOptions,
307306
StickyVFSID: stickyVFSID,
308307
},
309308
fs.ReadWrite,
@@ -374,15 +373,14 @@ func TestPebbleEncryption2(t *testing.T) {
374373
addKeyAndValidate := func(
375374
key string, val string, encKeyFile string, oldEncFileKey string,
376375
) {
377-
encOptionsBytes, err := protoutil.Marshal(&storagepb.EncryptionOptions{
376+
encOptions := &storagepb.EncryptionOptions{
378377
KeySource: storagepb.EncryptionKeySource_KeyFiles,
379378
KeyFiles: &storagepb.EncryptionKeyFiles{
380379
CurrentKey: encKeyFile,
381380
OldKey: oldEncFileKey,
382381
},
383382
DataKeyRotationPeriod: 1000,
384-
})
385-
require.NoError(t, err)
383+
}
386384

387385
// Initialize the filesystem env.
388386
ctx := context.Background()
@@ -392,7 +390,7 @@ func TestPebbleEncryption2(t *testing.T) {
392390
InMemory: true,
393391
Attributes: roachpb.Attributes{},
394392
Size: base.SizeSpec{InBytes: 512 << 20},
395-
EncryptionOptions: encOptionsBytes,
393+
EncryptionOptions: encOptions,
396394
StickyVFSID: stickyVFSID,
397395
},
398396
fs.ReadWrite,
@@ -507,10 +505,10 @@ func (ptfs *plainTestFS) syncDir(t *testing.T) {}
507505
// encryptedTestFS is the encrypted FS being tested.
508506
type encryptedTestFS struct {
509507
// The base strict FS which is wrapped for error injection and encryption.
510-
mem *vfs.MemFS
511-
encOptionsBytes []byte
512-
errorProb float64
513-
errorRand *rand.Rand
508+
mem *vfs.MemFS
509+
encOptions *storagepb.EncryptionOptions
510+
errorProb float64
511+
errorRand *rand.Rand
514512

515513
encEnv *fs.EncryptionEnv
516514
}
@@ -543,7 +541,7 @@ func (etfs *encryptedTestFS) restart() error {
543541
return err
544542
}
545543
encEnv, err := newEncryptedEnv(
546-
fsMeta, fileRegistry, "", false, etfs.encOptionsBytes)
544+
fsMeta, fileRegistry, "", false, etfs.encOptions)
547545
if err != nil {
548546
return err
549547
}
@@ -574,14 +572,12 @@ func makeEncryptedTestFS(t *testing.T, errorProb float64, errorRand *rand.Rand)
574572
// TODO(sumeer): Do deterministic data key rotation. Inject kmTimeNow and
575573
// operations that advance time.
576574
encOptions.DataKeyRotationPeriod = 100000
577-
encOptionsBytes, err := protoutil.Marshal(&encOptions)
578-
require.NoError(t, err)
579575
etfs := &encryptedTestFS{
580-
mem: mem,
581-
encOptionsBytes: encOptionsBytes,
582-
errorProb: errorProb,
583-
errorRand: errorRand,
584-
encEnv: nil,
576+
mem: mem,
577+
encOptions: &encOptions,
578+
errorProb: errorProb,
579+
errorRand: errorRand,
580+
encEnv: nil,
585581
}
586582
require.NoError(t, etfs.restart())
587583
return etfs

‎pkg/cli/ear_test.go

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -46,8 +46,7 @@ func TestDecrypt(t *testing.T) {
4646
encSpecStr := fmt.Sprintf("path=%s,key=%s,old-key=plain", dir, keyPath)
4747
encSpec, err := storagepb.NewStoreEncryptionSpec(encSpecStr)
4848
require.NoError(t, err)
49-
encOpts, err := encSpec.ToEncryptionOptions()
50-
require.NoError(t, err)
49+
encOpts := &encSpec.Options
5150

5251
env, err := fs.InitEnv(ctx, vfs.Default, dir, fs.EnvConfig{EncryptionOptions: encOpts}, nil /* statsCollector */)
5352
require.NoError(t, err)
@@ -129,8 +128,7 @@ func TestList(t *testing.T) {
129128
encSpecStr := fmt.Sprintf("path=%s,key=%s,old-key=plain", dir, keyPath)
130129
encSpec, err := storagepb.NewStoreEncryptionSpec(encSpecStr)
131130
require.NoError(t, err)
132-
encOpts, err := encSpec.ToEncryptionOptions()
133-
require.NoError(t, err)
131+
encOpts := &encSpec.Options
134132
env, err := fs.InitEnv(ctx, vfs.Default, dir, fs.EnvConfig{EncryptionOptions: encOpts}, nil /* statsCollector */)
135133
require.NoError(t, err)
136134
p, err := storage.Open(ctx, env, cluster.MakeTestingClusterSettings())

‎pkg/storage/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -178,6 +178,7 @@ go_test(
178178
"//pkg/storage/disk",
179179
"//pkg/storage/enginepb",
180180
"//pkg/storage/fs",
181+
"//pkg/storage/storagepb",
181182
"//pkg/testutils",
182183
"//pkg/testutils/datapathutils",
183184
"//pkg/testutils/echotest",

‎pkg/storage/fs/BUILD.bazel

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ go_library(
2020
"//pkg/settings",
2121
"//pkg/storage/disk",
2222
"//pkg/storage/enginepb",
23+
"//pkg/storage/storagepb",
2324
"//pkg/util/buildutil",
2425
"//pkg/util/envutil",
2526
"//pkg/util/log",

‎pkg/storage/fs/encryption_at_rest.go

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"fmt"
1111
"io"
1212

13+
"github.com/cockroachdb/cockroach/pkg/storage/storagepb"
1314
"github.com/cockroachdb/errors"
1415
"github.com/cockroachdb/pebble/vfs"
1516
)
@@ -18,16 +19,20 @@ import (
1819
// and writing data. This should be initialized by calling engineccl.Init() before calling
1920
// NewPebble(). The optionBytes is a binary serialized storagepb.EncryptionOptions.
2021
var NewEncryptedEnvFunc func(
21-
fs vfs.FS, fr *FileRegistry, dbDir string, readOnly bool, optionBytes []byte,
22+
fs vfs.FS, fr *FileRegistry, dbDir string, readOnly bool, encryptionOptions *storagepb.EncryptionOptions,
2223
) (*EncryptionEnv, error)
2324

2425
// resolveEncryptedEnvOptions creates the EncryptionEnv and associated file
2526
// registry if this store has encryption-at-rest enabled; otherwise returns a
2627
// nil EncryptionEnv.
2728
func resolveEncryptedEnvOptions(
28-
ctx context.Context, unencryptedFS vfs.FS, dir string, encryptionOpts []byte, rw RWMode,
29+
ctx context.Context,
30+
unencryptedFS vfs.FS,
31+
dir string,
32+
encryptionOpts *storagepb.EncryptionOptions,
33+
rw RWMode,
2934
) (*FileRegistry, *EncryptionEnv, error) {
30-
if len(encryptionOpts) == 0 {
35+
if encryptionOpts == nil {
3136
// There's no encryption config. This is valid if the user doesn't
3237
// intend to use encryption-at-rest, and the store has never had
3338
// encryption-at-rest enabled. Validate that there's no file registry.

‎pkg/storage/fs/fs.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/cockroachdb/cockroach/pkg/cli/exit"
1818
"github.com/cockroachdb/cockroach/pkg/settings"
1919
"github.com/cockroachdb/cockroach/pkg/storage/disk"
20+
"github.com/cockroachdb/cockroach/pkg/storage/storagepb"
2021
"github.com/cockroachdb/cockroach/pkg/util/buildutil"
2122
"github.com/cockroachdb/cockroach/pkg/util/envutil"
2223
"github.com/cockroachdb/errors"
@@ -121,7 +122,7 @@ func InitEnvFromStoreSpec(
121122
// EnvConfig provides additional configuration settings for Envs.
122123
type EnvConfig struct {
123124
RW RWMode
124-
EncryptionOptions []byte
125+
EncryptionOptions *storagepb.EncryptionOptions
125126
}
126127

127128
// InitEnv initializes a new virtual filesystem environment.

‎pkg/storage/min_version_test.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"github.com/cockroachdb/cockroach/pkg/roachpb"
1515
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
1616
"github.com/cockroachdb/cockroach/pkg/storage/fs"
17+
"github.com/cockroachdb/cockroach/pkg/storage/storagepb"
1718
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
1819
"github.com/cockroachdb/cockroach/pkg/util/log"
1920
"github.com/cockroachdb/pebble/vfs"
@@ -118,7 +119,7 @@ func TestMinVersion_IsNotEncrypted(t *testing.T) {
118119
st := cluster.MakeClusterSettings()
119120
baseFS := vfs.NewMem()
120121
env, err := fs.InitEnv(ctx, baseFS, "", fs.EnvConfig{
121-
EncryptionOptions: []byte("foo"),
122+
EncryptionOptions: &storagepb.EncryptionOptions{},
122123
}, nil /* statsCollector */)
123124
require.NoError(t, err)
124125

@@ -136,7 +137,11 @@ func TestMinVersion_IsNotEncrypted(t *testing.T) {
136137
}
137138

138139
func fauxNewEncryptedEnvFunc(
139-
unencryptedFS vfs.FS, fr *fs.FileRegistry, dbDir string, readOnly bool, optionBytes []byte,
140+
unencryptedFS vfs.FS,
141+
fr *fs.FileRegistry,
142+
dbDir string,
143+
readOnly bool,
144+
_ *storagepb.EncryptionOptions,
140145
) (*fs.EncryptionEnv, error) {
141146
return &fs.EncryptionEnv{
142147
Closer: nopCloser{},

‎pkg/storage/open.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -257,11 +257,11 @@ func makeExternalWALDir(
257257
// If the store is encrypted, we require that all the WAL failover dirs also
258258
// be encrypted so that the user doesn't accidentally leak data unencrypted
259259
// onto the filesystem.
260-
if engineCfg.env.Encryption != nil && len(externalDir.EncryptionOptions) == 0 {
260+
if engineCfg.env.Encryption != nil && externalDir.EncryptionOptions == nil {
261261
return wal.Dir{}, errors.Newf("must provide --enterprise-encryption flag for %q, used as WAL failover path for encrypted store %q",
262262
externalDir.Path, engineCfg.env.Dir)
263263
}
264-
if engineCfg.env.Encryption == nil && len(externalDir.EncryptionOptions) != 0 {
264+
if engineCfg.env.Encryption == nil && externalDir.EncryptionOptions != nil {
265265
return wal.Dir{}, errors.Newf("must provide --enterprise-encryption flag for store %q, specified WAL failover path %q is encrypted",
266266
engineCfg.env.Dir, externalDir.Path)
267267
}

‎pkg/storage/open_test.go

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"github.com/cockroachdb/cockroach/pkg/roachpb"
1818
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
1919
"github.com/cockroachdb/cockroach/pkg/storage/fs"
20+
"github.com/cockroachdb/cockroach/pkg/storage/storagepb"
2021
"github.com/cockroachdb/cockroach/pkg/testutils/datapathutils"
2122
"github.com/cockroachdb/cockroach/pkg/util/leaktest"
2223
"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -63,7 +64,7 @@ func TestWALFailover(t *testing.T) {
6364

6465
var envConfig fs.EnvConfig
6566
if td.HasArg("encrypted-at-rest") {
66-
envConfig.EncryptionOptions = []byte("test-encryption-options")
67+
envConfig.EncryptionOptions = &storagepb.EncryptionOptions{}
6768
}
6869
if td.HasArg("read-only") {
6970
envConfig.RW = fs.ReadOnly
@@ -88,10 +89,10 @@ func TestWALFailover(t *testing.T) {
8889
}
8990
}
9091
if td.HasArg("path-encrypted") {
91-
cfg.Path.EncryptionOptions = []byte("path-encryption-options")
92+
cfg.Path.EncryptionOptions = &storagepb.EncryptionOptions{}
9293
}
9394
if td.HasArg("prev-path-encrypted") {
94-
cfg.PrevPath.EncryptionOptions = []byte("prev-encryption-options")
95+
cfg.PrevPath.EncryptionOptions = &storagepb.EncryptionOptions{}
9596
}
9697
openEnv := getEnv(openDir)
9798
if openEnv == nil {

‎pkg/storage/storagepb/BUILD.bazel

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,6 @@ go_library(
2525
visibility = ["//visibility:public"],
2626
deps = [
2727
"//pkg/cli/cliflags",
28-
"//pkg/util/protoutil",
2928
"@com_github_cockroachdb_errors//:errors",
3029
"@com_github_spf13_pflag//:pflag",
3130
],

0 commit comments

Comments
 (0)