cockroachdb: add high-availability support #12965

DuskEagle · 2021-10-29T19:26:11Z

This commit adds high-availability support to the CockroachDB backend. The
locking strategy implemented is heavily influenced from the very similar
Postgres backend.

hashicorp-cla · 2021-10-29T19:26:15Z

All committers have signed the CLA.

heatherezell · 2021-10-29T19:27:56Z

Hi there! Thanks for this submission. Please sign the CLA, and we can get this into review. :)
(Note: If you've signed the CLA at an organizational level, please make sure your organization is publicly viewable, so the CLA bot can pick that up.)
Edit to add: And already signed! Awesome. :)

DuskEagle · 2021-11-18T19:01:51Z

Hi @hsimon-hashicorp , is there anything I can help with to get the review process rolling on this PR?

heatherezell · 2021-11-18T19:16:34Z

Hi there @DuskEagle - I'll nudge our engineers about this again. Thanks for your patience. :)

ncabatoff

Some minor comments/questions, overall it's looking good.

ncabatoff · 2021-11-24T15:11:54Z

physical/cockroachdb/cockroachdb.go

@@ -51,6 +56,11 @@ func NewCockroachDBBackend(conf map[string]string, logger log.Logger) (physical.
 		return nil, fmt.Errorf("missing connection_url")
 	}

+	var haEnabled bool
+	if haEnabledStr, ok := conf["ha_enabled"]; ok && haEnabledStr == "true" {


This is fine as is, but since in Go an absent key looked up in a map results in the zero value, and this is a map[string]string, you can just say haEnabled = conf["ha_enabled"] == "true" if you want to be a bit more succinct.

Good point, I've made the change.

ncabatoff · 2021-11-24T15:22:54Z

physical/cockroachdb/cockroachdb.go

+				" UPDATE SET (ha_identity, ha_key, ha_value, valid_until) = ($1, $2, $3, NOW() + $4) " +
+				" WHERE (t.valid_until < NOW() AND t.ha_key = $2) OR " +
+				" (t.ha_identity = $1 AND t.ha_key = $2)  ",
+			"delete": "DELETE FROM " + dbHATable + " WHERE ha_identity = $1 AND ha_key = $2",


Doesn't hurt to do it this way, and I know you're basing this on the PG storage which does this. But I realized today that we don't need to include ha_identity in this DELETE, since there's a unique index on ha_key. So if you want you could remove that part of the WHERE clause.

That's a good point! I've removed that part of the where clause.

ncabatoff · 2021-11-24T15:29:21Z

physical/cockroachdb/cockroachdb.go

+			"upsert": "INSERT INTO " + dbHATable + " as t (ha_identity, ha_key, ha_value, valid_until)" +
+				" VALUES ($1, $2, $3, NOW() + $4) " +
+				" ON CONFLICT (ha_key) DO " +
+				" UPDATE SET (ha_identity, ha_key, ha_value, valid_until) = ($1, $2, $3, NOW() + $4) " +


I know little about CockroachDB, but I thought it was fully compatible with PG. You've been diligent about adapting the PG approach here, but I noticed this one discrepancy: instead of NOW() + $4 * INTERVAL '1 seconds' you have simply NOW() + $4. Why's that?

What's happening here is that $4 is being passed in as a string type, not an integer type. When we attempt to to $4 * INTERVAL '1 seconds', we get a conversion error:

ERROR: unsupported binary operator: <string> * <interval> (desired <interval>) SQLSTATE: 22023

We would have the option of casting $4 to an integer, and then multiplying by an interval: NOW() + $4::int * INTERVAL '1 seconds'.

However, $4 is already a value in seconds, and so casting to an int just to multiply by 1 second is unnecessary. It's more succinct to just add the string value directly to NOW().

DuskEagle

Apologies for the delay. I've addressed your comments now :).

DuskEagle · 2022-03-10T19:57:20Z

physical/cockroachdb/cockroachdb.go

@@ -51,6 +56,11 @@ func NewCockroachDBBackend(conf map[string]string, logger log.Logger) (physical.
 		return nil, fmt.Errorf("missing connection_url")
 	}

+	var haEnabled bool
+	if haEnabledStr, ok := conf["ha_enabled"]; ok && haEnabledStr == "true" {


Good point, I've made the change.

DuskEagle · 2022-03-10T20:28:17Z

physical/cockroachdb/cockroachdb.go

+				" UPDATE SET (ha_identity, ha_key, ha_value, valid_until) = ($1, $2, $3, NOW() + $4) " +
+				" WHERE (t.valid_until < NOW() AND t.ha_key = $2) OR " +
+				" (t.ha_identity = $1 AND t.ha_key = $2)  ",
+			"delete": "DELETE FROM " + dbHATable + " WHERE ha_identity = $1 AND ha_key = $2",


That's a good point! I've removed that part of the where clause.

DuskEagle · 2022-03-10T20:28:19Z

physical/cockroachdb/cockroachdb.go

+			"upsert": "INSERT INTO " + dbHATable + " as t (ha_identity, ha_key, ha_value, valid_until)" +
+				" VALUES ($1, $2, $3, NOW() + $4) " +
+				" ON CONFLICT (ha_key) DO " +
+				" UPDATE SET (ha_identity, ha_key, ha_value, valid_until) = ($1, $2, $3, NOW() + $4) " +


What's happening here is that $4 is being passed in as a string type, not an integer type. When we attempt to to $4 * INTERVAL '1 seconds', we get a conversion error:

ERROR: unsupported binary operator: <string> * <interval> (desired <interval>) SQLSTATE: 22023

We would have the option of casting $4 to an integer, and then multiplying by an interval: NOW() + $4::int * INTERVAL '1 seconds'.

However, $4 is already a value in seconds, and so casting to an int just to multiply by 1 second is unnecessary. It's more succinct to just add the string value directly to NOW().

ncabatoff

Looks good! I have a couple of comments that you can choose to address, or not: I'm willing to merge regardless. Let me know how you'd like to proceed.

ncabatoff · 2022-03-18T13:17:57Z

physical/cockroachdb/cockroachdb_ha.go

+
+	var (
+		success = make(chan struct{})
+		errors  = make(chan error)


You might want to make this channel buffered. I saw some output from a static analysis tool recently showing that in the PG implementation, there's an issue here when tryToLock attempts to write to the error channel, but Lock has already returned due to stopCh being closed. In addition, you could make that write to the error channel in Lock tolerate blockage by using the optional send syntax.

I've taken your suggestions and made the channel buffered and the send non-blocking.

ncabatoff · 2022-03-18T13:18:45Z

physical/cockroachdb/cockroachdb_test.go

@@ -33,7 +34,15 @@ func prepareCockroachDBTestContainer(t *testing.T) (func(), *Config) {
 		if tableName == "" {
 			tableName = defaultTableName
 		}
-		return func() {}, &Config{*s, "vault." + tableName}
+		haTableName := os.Getenv("CR_HA_TABLE")


I don't think you need Getenv in this test. I know it's present in some of the existing tests, but it's vestigial mostly.

I've removed the os.Getenv calls for the CR_TABLE and CR_HA_TABLE environment variables, so the tests will use the default values.

This commit adds high-availability support to the CockroachDB backend. The locking strategy implemented is heavily influenced from the very similar Postgres backend.

DuskEagle

Thanks for the review! I've made the changes you recommend, if they look good to you I'd like to see this merged :).

DuskEagle · 2022-03-28T20:47:58Z

physical/cockroachdb/cockroachdb_ha.go

+
+	var (
+		success = make(chan struct{})
+		errors  = make(chan error)


I've taken your suggestions and made the channel buffered and the send non-blocking.

DuskEagle · 2022-03-28T20:48:51Z

physical/cockroachdb/cockroachdb_test.go

@@ -33,7 +34,15 @@ func prepareCockroachDBTestContainer(t *testing.T) (func(), *Config) {
 		if tableName == "" {
 			tableName = defaultTableName
 		}
-		return func() {}, &Config{*s, "vault." + tableName}
+		haTableName := os.Getenv("CR_HA_TABLE")


I've removed the os.Getenv calls for the CR_TABLE and CR_HA_TABLE environment variables, so the tests will use the default values.

ncabatoff · 2022-03-29T17:11:20Z

Not sure why those PG tests are failing in the remote-docker CI tests, but I'm confident it's unrelated to these changes.

ncabatoff · 2022-03-29T17:12:14Z

Thanks @DuskEagle !

DuskEagle · 2022-03-29T21:07:52Z

Thank you @ncabatoff !

HA support for CockroachDB was added in hashicorp#12965. This commit updates the docs to reflect that support.

HA support for CockroachDB was added in #12965. This commit updates the docs to reflect that support.

vercel bot temporarily deployed to Preview – vault-storybook October 29, 2021 19:26 Inactive

vercel bot temporarily deployed to Preview – vault October 29, 2021 19:26 Inactive

heatherezell added the ecosystem label Oct 29, 2021

DuskEagle force-pushed the cockroachdb-ha branch from 99c383b to 89d07f6 Compare October 29, 2021 19:31

vercel bot temporarily deployed to Preview – vault October 29, 2021 19:31 Inactive

vercel bot temporarily deployed to Preview – vault-storybook October 29, 2021 19:31 Inactive

DuskEagle force-pushed the cockroachdb-ha branch from 89d07f6 to 86d3887 Compare November 16, 2021 19:38

vercel bot temporarily deployed to Preview – vault November 16, 2021 19:38 Inactive

vercel bot temporarily deployed to Preview – vault-storybook November 16, 2021 19:38 Inactive

heatherezell added core/storage enhancement and removed ecosystem labels Nov 18, 2021

ncabatoff reviewed Nov 24, 2021

View reviewed changes

DuskEagle force-pushed the cockroachdb-ha branch from 86d3887 to de341b4 Compare March 10, 2022 20:28

DuskEagle commented Mar 10, 2022

View reviewed changes

ncabatoff approved these changes Mar 18, 2022

View reviewed changes

cockroachdb: add high-availability support

66e3575

This commit adds high-availability support to the CockroachDB backend. The locking strategy implemented is heavily influenced from the very similar Postgres backend.

DuskEagle force-pushed the cockroachdb-ha branch from de341b4 to 66e3575 Compare March 28, 2022 20:50

DuskEagle commented Mar 28, 2022

View reviewed changes

ncabatoff added this to the 1.10.1 milestone Mar 29, 2022

ncabatoff added the backport/1.10.x label Mar 29, 2022

ncabatoff merged commit 7e58f3c into hashicorp:main Mar 29, 2022

hc-github-team-secure-vault-core mentioned this pull request Mar 29, 2022

Backport of cockroachdb: add high-availability support into release/1.10.x #14759

Merged

DuskEagle added a commit to DuskEagle/vault that referenced this pull request Jun 30, 2022

docs/configuration: document CockroachDB HA mode

9bdfaf8

HA support for CockroachDB was added in hashicorp#12965. This commit updates the docs to reflect that support.

DuskEagle mentioned this pull request Jun 30, 2022

docs/configuration: document CockroachDB HA mode #16202

Merged

taoism4504 pushed a commit that referenced this pull request Jul 11, 2022

docs/configuration: document CockroachDB HA mode (#16202)

029a299

HA support for CockroachDB was added in #12965. This commit updates the docs to reflect that support.

hc-github-team-secure-vault-core mentioned this pull request Jul 11, 2022

Backport of docs/configuration: document CockroachDB HA mode into release/1.11.x #16272

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cockroachdb: add high-availability support #12965

cockroachdb: add high-availability support #12965

DuskEagle commented Oct 29, 2021

hashicorp-cla commented Oct 29, 2021 •

edited

Loading

heatherezell commented Oct 29, 2021 •

edited

Loading

DuskEagle commented Nov 18, 2021

heatherezell commented Nov 18, 2021

ncabatoff left a comment

ncabatoff Nov 24, 2021

DuskEagle Mar 10, 2022

ncabatoff Nov 24, 2021

DuskEagle Mar 10, 2022

ncabatoff Nov 24, 2021

DuskEagle Mar 10, 2022

DuskEagle left a comment

DuskEagle Mar 10, 2022

DuskEagle Mar 10, 2022

DuskEagle Mar 10, 2022

ncabatoff left a comment

ncabatoff Mar 18, 2022

DuskEagle Mar 28, 2022

ncabatoff Mar 18, 2022

DuskEagle Mar 28, 2022

DuskEagle left a comment

DuskEagle Mar 28, 2022

DuskEagle Mar 28, 2022

ncabatoff commented Mar 29, 2022

ncabatoff commented Mar 29, 2022

DuskEagle commented Mar 29, 2022

cockroachdb: add high-availability support #12965

cockroachdb: add high-availability support #12965

Conversation

DuskEagle commented Oct 29, 2021

hashicorp-cla commented Oct 29, 2021 • edited Loading

heatherezell commented Oct 29, 2021 • edited Loading

DuskEagle commented Nov 18, 2021

heatherezell commented Nov 18, 2021

ncabatoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DuskEagle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncabatoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DuskEagle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncabatoff commented Mar 29, 2022

ncabatoff commented Mar 29, 2022

DuskEagle commented Mar 29, 2022

hashicorp-cla commented Oct 29, 2021 •

edited

Loading

heatherezell commented Oct 29, 2021 •

edited

Loading