storage: enable switching env through encryption flags #20225

mberhault · 2017-11-22T15:48:00Z

Part of encryption work. See Encryption RFC

Add --enterprise-encryption flag and match it to stores.
Enabling encryption forces the use of the "Switching Env"
store version.

If encryption is not requested, we do not bump the version. This is
to allow downgrades to older binaries that do not support the switching
env. This will be changed after "a few" releases.

cockroach-teamcity · 2017-11-22T15:48:07Z

This change is

mberhault · 2017-11-29T19:56:47Z

Pulled in some commits from other WIP (and closed others). See commit descriptions.

mberhault · 2017-11-29T22:46:05Z

I'd appreciate a look, I need this functionality to keep going on the libroach side.

tbg · 2017-11-29T22:51:28Z

Looking, sorry for the delay!

mberhault · 2017-11-29T23:06:03Z

pkg/base/store_spec.go

+	// EncryptionSpec is non-nil after MatchStoreAndEncryptionSpecs if there is an encryption
+	// spec with matching path.
+	//	EncryptionSpec *StoreEncryptionSpec
+	ExtraFields map[string]string


btw: this clearly needs to go, it's not like we can easily pass a string->string map to cgo, and even if we could, that's a bit fragile.
I'm still trying to figure out if I can have some blind CCLOptions of some type, but it's a bit tricky.

tbg · 2017-11-29T23:26:09Z

Looks good so far! The meat of my comments are predictably in the area of ccl-oss glueing. I'll hold off on looking at the ExtraFields until prompted otherwise.

The C++ looked mostly trivial so far, but if there's any subtlety in then, I have missed it. I'm also not an experienced C++ programmer (or one at all).

Reviewed 9 of 9 files at r1, 7 of 7 files at r2, 13 of 13 files at r3, 5 of 5 files at r4.
Review status: all files reviewed at latest revision, 15 unresolved discussions, all commit checks successful.

c-deps/libroach/db.cc, line 101 at r1 (raw file):

  // Construct a new DBImpl from the specified DB and Env. Both the DB
  // and Env will be deleted when the DBImpl is deleted. It is ok to
  // pass NULL for the Env.

Comment needs an update now that there are two envs. What is the role of each?

c-deps/libroach/env_switching.cc, line 22 at r1 (raw file):

//                 |        |
//                 |   encrypted_env (EnvWrapper)
//                 |        |

\        /
  \    /
    \/
base_env
   (Env)

and a similar cone at the top. I had to scratch my head for a couple dozen seconds.

c-deps/libroach/env_switching.cc, line 30 at r1 (raw file):

};

rocksdb::Env* NewSwitchingEnv(rocksdb::Env* base_env) {

Do we ever call this without an env? If not, probably safer to disallow it.

pkg/base/store_spec.go, line 69 at r3 (raw file):

	InMemory    bool
	Attributes  roachpb.Attributes
	// EncryptionSpec is non-nil after MatchStoreAndEncryptionSpecs if there is an encryption

What's happening here?

pkg/base/store_spec.go, line 170 at r3 (raw file):

		}

		switch field {

const path = "path"

and then use that twice below.

pkg/base/store_spec.go, line 69 at r4 (raw file):

	InMemory        bool
	Attributes      roachpb.Attributes
	UseSwitchingEnv bool

Needs explanation.

pkg/base/store_spec.go, line 73 at r4 (raw file):

Previously, mberhault (marc) wrote…

btw: this clearly needs to go, it's not like we can easily pass a string->string map to cgo, and even if we could, that's a bit fragile.
I'm still trying to figure out if I can have some blind CCLOptions of some type, but it's a bit tricky.

Ack, I'll just ignore this part for now.

pkg/ccl/baseccl/encryption_spec.go, line 39 at r3 (raw file):

func (es StoreEncryptionSpec) String() string {
	// All fields are set.
	return fmt.Sprintf("path=%s,key=%s,old-key=%s,rotation-period=%s",

Why not https://golang.org/pkg/net/url/#URL and avoid a hand-rolled format (simply strip the <Scheme>://)? That avoids lots of subtleties with the characters allowed in paths and looks familiar to everyone.

pkg/ccl/baseccl/encryption_spec.go, line 129 at r3 (raw file):

	var buffer bytes.Buffer
	for _, ss := range encl.Specs {
		fmt.Fprintf(&buffer, "--enterprise-encryption=%s ", ss)

I'm confused why this returns the hardcoded flag name.

pkg/ccl/baseccl/encryption_spec.go, line 161 at r4 (raw file):

// MatchStoreAndEncryptionSpecs iterates through the StoreEncryptionSpecList and looks
// for matching paths in the StoreSpecList.
// Any unmatched StoreEncryptionSpec causes an error.

The comment doesn't mention that we're going to manipulate the store spec list. Should this be called MergeEncryptionSpecsIntoStoreSpecs?

pkg/ccl/cliccl/start.go, line 40 at r3 (raw file):

	// This relies upon init-order between packages.
	cli.StartCmd.Flags().VarP(&storeEncryptionSpecs, "enterprise-encryption", "", encryptionFlagDesc)
	cli.ExtraStartPreRunHook = matchStoreEncryptionSpecs

Just do

wrapped := cli.StartCmd.PersistentPreRunE
cli.StartCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
  if err := wrapped(cmd, args); err != nil {
    return err
  }
  return matchStoreEncryptionSpecs(cmd, args)
}

and there is no need for ExtraStartPreRunHook.

To make this independent of init order, you can make both setters to PersistentPreRunE use this pattern.

pkg/ccl/cliccl/start.go, line 46 at r3 (raw file):

// parsed stores.
func matchStoreEncryptionSpecs() error {
	return baseccl.MatchStoreAndEncryptionSpecs(cli.ServerCfg.Stores, storeEncryptionSpecs)

Instead of exposing all of ServerCfg, just expose a getter into serverCfg.Stores() from cli that is clearly marked as something you shouldn't use in other contexts.

pkg/cli/context.go, line 61 at r3 (raw file):

}

// ServerCfg is the config for the `start` command.

Not exclusively, I believe.

pkg/cli/start.go, line 654 at r3 (raw file):

				if spec.ExtraFields != nil {
					for k, v := range spec.ExtraFields {
						fmt.Fprintf(tw, "  %s:\t%s\n", k, v)

Ignoring for now.

pkg/storage/engine/enginepb/registry.proto, line 40 at r2 (raw file):

message FileEntry {
  string Filename = 1;

Add a comment specifying if this is a relative or absolute path. I suppose it's relative (to what?) to allow moving of mount points.
There is also the concern of moving a HDD from a Linux to a Windows machine. Will things break because the path separator changes? It might be better to store a repeated string with individual path components.

Comments from Reviewable

mberhault · 2017-11-30T01:03:46Z

Thanks for the review. I'll loop back around when I've sorted out how to properly pass CCL options through rocksdb, most likely not the ExtraFields.

The only trick in the C++ code is who owns what for deletion. The envs aren't actually used by DBImpl, they're only held as scoped pointers to delete when DBImpl is deleted. This will change around quite a bit in future encryption
work.

And if it makes you feel better: I don't know any experienced C++ programmers, just gray-haired ones.

Review status: all files reviewed at latest revision, 15 unresolved discussions, all commit checks successful.

c-deps/libroach/db.cc, line 101 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Comment needs an update now that there are two envs. What is the role of each?

Done. I still need to figure out exactly who will own what. Right now, they're only passed to the DBImpl to be deleted properly. They're only used by the parent DBEngine through the options.

c-deps/libroach/env_switching.cc, line 22 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

\        /
  \    /
    \/
base_env
   (Env)
and a similar cone at the top. I had to scratch my head for a couple dozen seconds.

Done. I used straight lines because it otherwise breaks single-line comments.
Switched to a multi-line comment instead and some pretty little boxes around each entity.

c-deps/libroach/env_switching.cc, line 30 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Do we ever call this without an env? If not, probably safer to disallow it.

options.env is NULL by default. Currently, it is only set for in-memory DBs.

pkg/base/store_spec.go, line 69 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

What's happening here?

uh. oops, old comment. Replaced with a relevant one.

pkg/base/store_spec.go, line 170 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

const path = "path"
and then use that twice below.

Done and in the EncryptionSpec as well.

pkg/base/store_spec.go, line 69 at r4 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Needs explanation.

Done.

pkg/ccl/baseccl/encryption_spec.go, line 39 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Why not https://golang.org/pkg/net/url/#URL and avoid a hand-rolled format (simply strip the <Scheme>://)? That avoids lots of subtleties with the characters allowed in paths and looks familiar to everyone.

I'm following the StoreSpec. I want to keep the same behavior, but I especially want to make sure the parsing is exactly the same as we need to match paths.

pkg/ccl/baseccl/encryption_spec.go, line 129 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

I'm confused why this returns the hardcoded flag name.

Ported over from StoreSpec. While I agree that referring to the flag variable would be best, I'm not a fan of the added dependency. base and cli (or their ccl variants) are already heavily depended upon. That said, we talking about cliflags, not cli so it's definitely doable.

pkg/ccl/baseccl/encryption_spec.go, line 161 at r4 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

The comment doesn't mention that we're going to manipulate the store spec list. Should this be called MergeEncryptionSpecsIntoStoreSpecs?

Done. Renamed to PopulateStoreSpecsWithEncryption to make it clear what we're mutating.

pkg/ccl/cliccl/start.go, line 40 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Just do
wrapped := cli.StartCmd.PersistentPreRunE
cli.StartCmd.PersistentPreRunE = func(cmd *cobra.Command, args []string) error {
  if err := wrapped(cmd, args); err != nil {
    return err
  }
  return matchStoreEncryptionSpecs(cmd, args)
}
and there is no need for ExtraStartPreRunHook.

To make this independent of init order, you can make both setters to PersistentPreRunE use this pattern.

Good idea. Done here and in cli/cli.go for the start command.

pkg/ccl/cliccl/start.go, line 46 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Instead of exposing all of ServerCfg, just expose a getter into serverCfg.Stores() from cli that is clearly marked as something you shouldn't use in other contexts.

Done with a warning.

pkg/cli/context.go, line 61 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Not exclusively, I believe.

ugh. Right you are. dropped the comment anyway now that it's no longer public.

pkg/storage/engine/enginepb/registry.proto, line 40 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Add a comment specifying if this is a relative or absolute path. I suppose it's relative (to what?) to allow moving of mount points.
There is also the concern of moving a HDD from a Linux to a Windows machine. Will things break because the path separator changes? It might be better to store a repeated string with individual path components.

Most likely relative but the Env doesn't get relative paths so I'll have to generate those myself.
As for the path separators, they are hard-coded to / in rocksdb. eg: this is how you get the MANIFEST path:
https://github.com/facebook/rocksdb/blob/master/util/filename.cc#L142

Comments from Reviewable

mberhault · 2017-11-30T02:53:49Z

Review status: 18 of 28 files reviewed at latest revision, 15 unresolved discussions, all commit checks successful.

pkg/base/store_spec.go, line 73 at r4 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Ack, I'll just ignore this part for now.

@petermattis, @bdarnell: any thoughts on the best way to pass go-CCL objects blindly through rocksdb.go::open() and into libroach-CCL? I'll poke at trying to pass a C++ object that's only known in libroachccl, but I'm not sure go will be too happy about that.

Comments from Reviewable

petermattis · 2017-11-30T03:02:42Z

Without looking at this code: pass an encoded protobuf. I’ll take a look tomorrow.

…

On Wed, Nov 29, 2017 at 9:54 PM marc ***@***.***> wrote: Review status: 18 of 28 files reviewed at latest revision, 15 unresolved discussions, all commit checks successful. ------------------------------ *pkg/base/store_spec.go, line 73 at r4 <https://reviewable.io:443/reviews/cockroachdb/cockroach/20225#-L-7IR0AC778-MlT7fQq-r4-73:-L-9p7Vzzz8KDJX1seDu:bkj1ug> (raw file <https://github.com/cockroachdb/cockroach/blob/42e6632b956ab718197529b9e828d85811fc4b4c/pkg/base/store_spec.go#L73>):* *Previously, tschottdorf (Tobias Schottdorf) wrote…* Ack, I'll just ignore this part for now. @petermattis <https://github.com/petermattis>, @bdarnell <https://github.com/bdarnell>: any thoughts on the best way to pass go-CCL objects blindly through rocksdb.go::open() and into libroach-CCL? I'll poke at trying to pass a C++ object that's only known in libroachccl, but I'm not sure go will be too happy about that. ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/cockroachdb/cockroach/20225>* — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20225 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF6f94xz-eH27aGflGZ83ROKoJGXGMxyks5s7hjIgaJpZM4Qnm7_> .

mberhault · 2017-11-30T12:21:01Z

That's one option and it would work fine for opaque config settings (eg: the encryption flag).

My other problem is figuring out how to have go-CCL setup cpp-CCL hooks into libroach for CCL-only code. Between overridden env and all the communication (stats reporting, periodic loops, etc..) we'll have a lot of CCL code spread between go and cpp that needs to communicate.

Review status: 18 of 28 files reviewed at latest revision, 15 unresolved discussions, all commit checks successful.

Comments from Reviewable

tbg · 2017-11-30T15:24:12Z

Reviewed 11 of 11 files at r5.
Review status: all files reviewed at latest revision, 9 unresolved discussions, all commit checks successful.

c-deps/libroach/db.cc, line 101 at r1 (raw file):

Previously, mberhault (marc) wrote…

Done. I still need to figure out exactly who will own what. Right now, they're only passed to the DBImpl to be deleted properly. They're only used by the parent DBEngine through the options.

Ok. I assume that'll be done by the end of this PR? Otherwise, please insert a TODO.

c-deps/libroach/env_switching.cc, line 22 at r1 (raw file):

Previously, mberhault (marc) wrote…

Done. I used straight lines because it otherwise breaks single-line comments.
Switched to a multi-line comment instead and some pretty little boxes around each entity.

💓

pkg/base/store_spec.go, line 73 at r4 (raw file):

Previously, mberhault (marc) wrote…

@petermattis, @bdarnell: any thoughts on the best way to pass go-CCL objects blindly through rocksdb.go::open() and into libroach-CCL? I'll poke at trying to pass a C++ object that's only known in libroachccl, but I'm not sure go will be too happy about that.

Like Pete said, a protobuf comes to mind. Or you can use a URL if human readable is preferred. ear://<path>?<key>=<val>&<key>=<val>.

pkg/ccl/baseccl/encryption_spec.go, line 39 at r3 (raw file):

Previously, mberhault (marc) wrote…

I'm following the StoreSpec. I want to keep the same behavior, but I especially want to make sure the parsing is exactly the same as we need to match paths.

Ah. That makes sense, but then it seems to makes sense to have a method that is called by both which parses the string into a map[string]string.

pkg/ccl/baseccl/encryption_spec.go, line 129 at r3 (raw file):

Previously, mberhault (marc) wrote…

Ported over from StoreSpec. While I agree that referring to the flag variable would be best, I'm not a fan of the added dependency. base and cli (or their ccl variants) are already heavily depended upon. That said, we talking about cliflags, not cli so it's definitely doable.

Hmm, StoreSpec also has this same problem. I have a mild preference for cleaning up both to refer to the variable name, if nothing else to highlight the dependency.

pkg/cli/context.go, line 70 at r5 (raw file):

// 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 should should be using this.

should -> you and while you're here anyway, insert a newline before the warning.

pkg/cli/flags.go, line 164 at r5 (raw file):

	// The following only runs for `start`.
	// Make sure we save and call any existing PreRun hook as well.
	wrapped := StartCmd.PersistentPreRunE

Optional nit: you could even factor this out,

// AddPersistentPreRunE adds the given hook after any existing pre run hooks for the command.
// This allows hooks to be set without wiping out an existing hook.
func AddPersistentPreRunE(cmd *cobra.Command, hook func(*cobra.Command, []string) error) {
  wrapped := cmd.PersistentPreRunE
  cmd.PersistentPreRunE = func(c *cobra.Command, a []string) error {
    if wrapped != nil {
      if err := wrapped(c, a); err != nil { return err }
    }
    return hook(c, a)
  }
}

which makes it easy to grep for the "other" half of the hook setting and creates a distinguished place for explaining what's happening.

pkg/storage/engine/enginepb/registry.proto, line 40 at r2 (raw file):

Previously, mberhault (marc) wrote…

Most likely relative but the Env doesn't get relative paths so I'll have to generate those myself.
As for the path separators, they are hard-coded to / in rocksdb. eg: this is how you get the MANIFEST path:
https://github.com/facebook/rocksdb/blob/master/util/filename.cc#L142

Interesting. And that just works on Windows? Worth making a note to test if nothing else.
If you're not ready to add a comment in this PR because it's still up in the air, please leave a TODO in its place.

Comments from Reviewable

petermattis · 2017-11-30T16:56:14Z

Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful.

pkg/base/store_spec.go, line 73 at r4 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Like Pete said, a protobuf comes to mind. Or you can use a URL if human readable is preferred. ear://<path>?<key>=<val>&<key>=<val>.

For options, I would go with comma separated, semicolon separated or query-components (ampersand separated). I might not have enough context here to be giving good advice, though.

Comments from Reviewable

mberhault · 2017-11-30T16:58:32Z

Review status: 4 of 28 files reviewed at latest revision, 9 unresolved discussions.

c-deps/libroach/db.cc, line 101 at r1 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Ok. I assume that'll be done by the end of this PR? Otherwise, please insert a TODO.

Adding a TODO for now. Implementing the Switching Env is probably done separately.

pkg/base/store_spec.go, line 73 at r4 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Like Pete said, a protobuf comes to mind. Or you can use a URL if human readable is preferred. ear://<path>?<key>=<val>&<key>=<val>.

Yeah, my bigger concern is that a lot of the encryption objects on the C++ side will be CCL. This mean initializing them in go-ccl code, then passing them through somehow. I'm still investigating, so I'll take the ExtraFields out for now.

pkg/ccl/baseccl/encryption_spec.go, line 39 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Ah. That makes sense, but then it seems to makes sense to have a method that is called by both which parses the string into a map[string]string.

True, hence why I'm sharing path parsing. Another kink is that the StoreSpec can have an optional path field when a single value is specified. Still, it makes sense to generalize the code. Putting in a TODO.

pkg/ccl/baseccl/encryption_spec.go, line 129 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Hmm, StoreSpec also has this same problem. I have a mild preference for cleaning up both to refer to the variable name, if nothing else to highlight the dependency.

Changed the StoreSpec to use the FlagInfo. Added a TODO for the encryption spec. I'll refactor CCL flags to use cliflags right after this PR (it's a bunch of code movement, I'd rather keep it separate)

pkg/cli/context.go, line 70 at r5 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

should -> you and while you're here anyway, insert a newline before the warning.

Done.

pkg/cli/flags.go, line 164 at r5 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Optional nit: you could even factor this out,

// AddPersistentPreRunE adds the given hook after any existing pre run hooks for the command.
// This allows hooks to be set without wiping out an existing hook.
func AddPersistentPreRunE(cmd *cobra.Command, hook func(*cobra.Command, []string) error) {
  wrapped := cmd.PersistentPreRunE
  cmd.PersistentPreRunE = func(c *cobra.Command, a []string) error {
    if wrapped != nil {
      if err := wrapped(c, a); err != nil { return err }
    }
    return hook(c, a)
  }
}

which makes it easy to grep for the "other" half of the hook setting and creates a distinguished place for explaining what's happening.

Good idea, done.

pkg/cli/start.go, line 654 at r3 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Ignoring for now.

I'm taking this out for now.

pkg/storage/engine/enginepb/registry.proto, line 40 at r2 (raw file):

Previously, tschottdorf (Tobias Schottdorf) wrote…

Interesting. And that just works on Windows? Worth making a note to test if nothing else.
If you're not ready to add a comment in this PR because it's still up in the air, please leave a TODO in its place.

Done.

Comments from Reviewable

mberhault · 2017-11-30T16:59:11Z

Ok, I'm removing the extra fields and will leave this to a subsequent PR. I'd like to merge this one as is, then refactor some of the cli flags and spec parsing.

Review status: 4 of 28 files reviewed at latest revision, 9 unresolved discussions, some commit checks pending.

Comments from Reviewable

benesch · 2017-11-30T17:00:40Z

Drive by comment: Little known fact that Windows will handle forward slashes in paths just fine, save for maybe some real obscure APIs. It’s drive letters (ie, constructing absolute paths) that really cause trouble.

…

On Thu, Nov 30, 2017 at 11:56 AM Peter Mattis ***@***.***> wrote: Review status: all files reviewed at latest revision, 8 unresolved discussions, all commit checks successful. ------------------------------ *pkg/base/store_spec.go, line 73 at r4 <https://reviewable.io:443/reviews/cockroachdb/cockroach/20225#-L-7IR0AC778-MlT7fQq-r4-73:-L-Cq_UZ3Y-j-tAFh6FL:b3zm7vv> (raw file <https://github.com/cockroachdb/cockroach/blob/42e6632b956ab718197529b9e828d85811fc4b4c/pkg/base/store_spec.go#L73>):* *Previously, tschottdorf (Tobias Schottdorf) wrote…* Like Pete said, a protobuf comes to mind. Or you can use a URL if human readable is preferred. ear://<path>?<key>=<val>&<key>=<val>. For options, I would go with comma separated, semicolon separated or query-components (ampersand separated). I might not have enough context here to be giving good advice, though. ------------------------------ *Comments from Reviewable <https://reviewable.io:443/reviews/cockroachdb/cockroach/20225>* — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#20225 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA15IGI1T0AvFQPSTfI3YJTEy9EHC7RMks5s7t5HgaJpZM4Qnm7_> .

tbg · 2017-11-30T17:02:20Z

LGTM On Thu, Nov 30, 2017 at 12:01 PM Nikhil Benesch <notifications@github.com> wrote:

Drive by comment: Little known fact that Windows will handle forward slashes in paths just fine, save for maybe some real obscure APIs. It’s drive letters (ie, constructing absolute paths) that really cause trouble. On Thu, Nov 30, 2017 at 11:56 AM Peter Mattis ***@***.***> wrote: > Review status: all files reviewed at latest revision, 8 unresolved > discussions, all commit checks successful. > ------------------------------ > > *pkg/base/store_spec.go, line 73 at r4 > < https://reviewable.io:443/reviews/cockroachdb/cockroach/20225#-L-7IR0AC778-MlT7fQq-r4-73:-L-Cq_UZ3Y-j-tAFh6FL:b3zm7vv > > (raw file > < https://github.com/cockroachdb/cockroach/blob/42e6632b956ab718197529b9e828d85811fc4b4c/pkg/base/store_spec.go#L73 >):* > *Previously, tschottdorf (Tobias Schottdorf) wrote…* > > Like Pete said, a protobuf comes to mind. Or you can use a URL if human > readable is preferred. ear://<path>?<key>=<val>&<key>=<val>. > > For options, I would go with comma separated, semicolon separated or > query-components (ampersand separated). I might not have enough context > here to be giving good advice, though. > ------------------------------ > > *Comments from Reviewable > <https://reviewable.io:443/reviews/cockroachdb/cockroach/20225>* > > — > You are receiving this because your review was requested. > Reply to this email directly, view it on GitHub > < #20225 (comment) >, > or mute the thread > < https://github.com/notifications/unsubscribe-auth/AA15IGI1T0AvFQPSTfI3YJTEy9EHC7RMks5s7t5HgaJpZM4Qnm7_ > > . > — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#20225 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AE135ILDbBSUuV6FTRkKLr_snnW2hC08ks5s7t9OgaJpZM4Qnm7_> .

--

…

-- Tobias

encryption-at-rest work. Add --enterprise-encryption flag and match it to stores. Enabling encryption forces the use of the "Switching Env" store version. If encryption is not requested, we do not bump the version. This is to allow downgrades to older binaries that do not support the switching env. This will be changed after "a few" releases.

mberhault requested a review from a team November 22, 2017 15:48

mberhault force-pushed the marc/custom_env branch from cafb525 to 6f96ae4 Compare November 27, 2017 19:40

mberhault requested review from bdarnell and tbg November 27, 2017 21:55

This was referenced Nov 27, 2017

RFC: encryption at rest #19785

Merged

security: encryption at rest #19783

Closed

mberhault force-pushed the marc/custom_env branch 3 times, most recently from 77ea557 to f01fd80 Compare November 29, 2017 12:48

mberhault requested a review from a team November 29, 2017 15:00

mberhault requested a review from a team as a code owner November 29, 2017 15:03

mberhault requested a review from a team November 29, 2017 15:03

mberhault mentioned this pull request Nov 29, 2017

WIP: cli: add enterprise-encryption flag. #20260

Closed

mberhault force-pushed the marc/custom_env branch 2 times, most recently from efcab4a to 010f3cd Compare November 29, 2017 18:00

mberhault changed the title ~~WIP: core: switching env for encryption support.~~ WIP: core: enable switching env through encryption flags Nov 29, 2017

mberhault changed the title ~~WIP: core: enable switching env through encryption flags~~ WIP: storage: enable switching env through encryption flags Nov 29, 2017

mberhault force-pushed the marc/custom_env branch 4 times, most recently from 3b6e04a to 42e6632 Compare November 29, 2017 21:09

mberhault commented Nov 29, 2017

View reviewed changes

mberhault force-pushed the marc/custom_env branch from b9b05c0 to b34cb90 Compare November 30, 2017 01:08

mberhault force-pushed the marc/custom_env branch from b34cb90 to aa89def Compare November 30, 2017 16:58

mberhault force-pushed the marc/custom_env branch from aa89def to d44fe30 Compare November 30, 2017 17:11

mberhault changed the title ~~WIP: storage: enable switching env through encryption flags~~ storage: enable switching env through encryption flags Nov 30, 2017

mberhault force-pushed the marc/custom_env branch from d44fe30 to dc0e674 Compare November 30, 2017 17:35

mberhault merged commit e62bcc6 into cockroachdb:master Nov 30, 2017

mberhault deleted the marc/custom_env branch November 30, 2017 18:34

jseldess mentioned this pull request Dec 8, 2017

Encryption at rest / Transparent Data Encryption (TDE) cockroachdb/docs#2220

Closed

storage: enable switching env through encryption flags #20225

storage: enable switching env through encryption flags #20225

Uh oh!

Conversation

mberhault commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Nov 22, 2017

Uh oh!

mberhault commented Nov 29, 2017

Uh oh!

mberhault commented Nov 29, 2017

Uh oh!

tbg commented Nov 29, 2017

Uh oh!

mberhault Nov 29, 2017

Choose a reason for hiding this comment

Uh oh!

tbg commented Nov 29, 2017

Uh oh!

mberhault commented Nov 30, 2017

Uh oh!

mberhault commented Nov 30, 2017

Uh oh!

petermattis commented Nov 30, 2017 via email

Uh oh!

mberhault commented Nov 30, 2017

Uh oh!

tbg commented Nov 30, 2017

Uh oh!

petermattis commented Nov 30, 2017

Uh oh!

mberhault commented Nov 30, 2017

Uh oh!

mberhault commented Nov 30, 2017

Uh oh!

benesch commented Nov 30, 2017 via email

Uh oh!

tbg commented Nov 30, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mberhault commented Nov 22, 2017 •

edited

Loading