Aerospike storage backend #10131

reugn · 2020-10-12T14:30:44Z

This adds support for Aerospike as a (non-HA) storage backend for Vault.

hashicorp-cla · 2020-10-12T14:30:48Z

All committers have signed the CLA.

reugn · 2020-11-03T16:31:08Z

@jasonodonnell , @calvn , @catsby , @Valarissa , @vishalnayak , @kalafut could you please review this PR?

kalafut · 2020-12-04T19:36:36Z

@reugn Thanks for the PR! Yes, we'll be reviewing this soon.

kalafut

Looks pretty good! Some initial feedback, mostly minor.

kalafut · 2020-12-09T17:01:47Z

physical/aerospike/aerospike.go

+		return nil, err
+	}
+
+	return &AerospikeBackend{


Since (per the docs), set/namespace/hostname are all required, should we be validating != "" before successfully returning?

Looks like set is optional, and both namespace and hostname now have default values so we should be good.

kalafut · 2020-12-09T17:04:20Z

physical/aerospike/aerospike.go

+		return err
+	}
+
+	writePolicy := aero.NewWritePolicy(0, 0)


I comment or docs link that has some details about this would be helpful.

kalafut · 2020-12-09T17:06:46Z

physical/aerospike/aerospike.go

+
+	record, err := a.client.Get(nil, aeroKey)
+	if err != nil {
+		if err.Error() == "Key not found" {


Is the compared-to error exported by chance?

kalafut · 2020-12-09T17:10:32Z

physical/aerospike/aerospike.go

+
+	return &physical.Entry{
+		Key:   key,
+		Value: record.Bins[valueBin].([]byte),


Considering indexing against valueBin in advance so we can check for the presence of the key and error (otherwise we'll panic on nil)

kalafut · 2020-12-09T17:12:24Z

physical/aerospike/aerospike.go

+}
+
+// Put is used to insert or update an entry.
+func (a *AerospikeBackend) Put(ctx context.Context, entry *physical.Entry) error {


s/ctx/_ on these unused ctx parameters

kalafut · 2020-12-09T17:17:16Z

physical/aerospike/aerospike.go

+		}
+		recordKey := res.Record.Bins[keyBin].(string)
+		if strings.HasPrefix(recordKey, prefix) {
+			trimPrefix := strings.Replace(recordKey, prefix, "", 1)


strings.TrimPrefix is a little simpler.

kalafut · 2020-12-09T17:20:18Z

physical/aerospike/aerospike.go

+				keyList = append(keyList, keys[0])
+			} else {
+				withSlash := keys[0] + "/"
+				if !listContains(keyList, withSlash) {


We have a helper you can use:

vault/sdk/helper/strutil/strutil.go

Line 26 in 8af1f3b

func StrListContains(haystack []string, needle string) bool {

kalafut · 2020-12-09T17:25:57Z

physical/aerospike/aerospike.go

+}
+
+func hash(s string) string {
+	h := fnv.New32a()


Any reason this was chosen in particular? 32-bits is a fairly small hash size. We'd typically use sha256 to satisfy both irreversibility and collision avoidance.

kalafut · 2020-12-09T17:27:55Z

website/pages/docs/configuration/storage/aerospike.mdx

+
+# Aerospike Storage Backend
+
+The Aerospike storage backend is used to persist Vault's data in


Suggested change

The Aerospike storage backend is used to persist Vault's data in

The Aerospike storage backend is used to persist Vault's data in an

kalafut · 2020-12-09T17:28:07Z

website/pages/docs/configuration/storage/aerospike.mdx

+page_title: Aerospike - Storage Backends - Configuration
+sidebar_title: Aerospike
+description: |-
+  The Aerospike storage backend is used to persist Vault's data in Aerospike


Suggested change

The Aerospike storage backend is used to persist Vault's data in Aerospike

The Aerospike storage backend is used to persist Vault's data in an Aerospike

kalafut · 2020-12-09T18:51:56Z

website/pages/docs/configuration/storage/aerospike.mdx

@@ -0,0 +1,50 @@
+---


Need to update the JSON to reference this new file as well:

vault/website/data/docs-navigation.js

Line 67 in f66c721

category: 'storage',

calvn · 2020-12-10T01:28:18Z

physical/aerospike/aerospike.go

+	policy.User = conf["username"]
+	policy.Password = conf["password"]
+
+	port, err := strconv.Atoi(conf["port"])


If port is not provided, the error returned does not make it obvious that this is the case. Consider checking the value beforehand separately.

strconv.Atoi: parsing "": invalid syntax

physical/aerospike/aerospike.go

calvn · 2020-12-10T01:41:39Z

physical/aerospike/aerospike_test.go

+		ContainerName:   "aerospikedb",
+		ImageTag:        "latest",
+		Ports:           []string{"3000/tcp", "3001/tcp", "3002/tcp", "3003/tcp"},
+		DoNotAutoRemove: true,


Is this strictly necessary? This leaves dangling containers on the host, and may result in errors if tests are ran multiple times due to colliding container name.

website/pages/docs/configuration/storage/aerospike.mdx

reugn · 2020-12-10T15:36:20Z

@kalafut , @calvn thanks for the review. Fixes and improvements were done.

calvn · 2021-01-05T18:22:53Z

go.mod

@@ -17,6 +17,7 @@ require (
 	github.com/SAP/go-hdb v0.14.1
 	github.com/Sectorbob/mlab-ns2 v0.0.0-20171030222938-d3aa0c295a8a
 	github.com/StackExchange/wmi v0.0.0-20190523213315-cbe66965904d // indirect
+	github.com/aerospike/aerospike-client-go v3.1.0+incompatible


I'm a bit curious as to why this particular pseudo-version was chosen since v3.1.1 is available.

Interesting, the +incompatible label is actually how go/mod sees the release: https://pkg.go.dev/github.com/aerospike/[email protected]+incompatible?tab=versions

Since v3.1.1+incompatible has been released, we should consider bumping it to that version (unless there's a particular reason not to)

calvn · 2021-01-05T18:24:58Z

physical/aerospike/aerospike.go

+
+	defaultNamespace = "test"
+
+	defaultHost = "127.0.0.1"


Nit: Since this is assigned as the default hostname, can we rename this to defaultHostname?

calvn

Some minor suggestions, otherwise looks good!

calvn · 2021-01-05T19:28:34Z

website/pages/docs/configuration/storage/aerospike.mdx

+
+- `auth_mode` `(string: "INTERNAL")` - Specifies the authentication mode when user/password is defined (INTERNAL/EXTERNAL).
+
+- `timeout` `(int: 30000)` - Initial host connection timeout duration in milliseconds.


Where does the timeout and idle_timeout values come from?

physical/aerospike/aerospike.go

calvn · 2021-01-05T19:34:59Z

go.mod

@@ -17,6 +17,7 @@ require (
 	github.com/SAP/go-hdb v0.14.1
 	github.com/Sectorbob/mlab-ns2 v0.0.0-20171030222938-d3aa0c295a8a
 	github.com/StackExchange/wmi v0.0.0-20190523213315-cbe66965904d // indirect
+	github.com/aerospike/aerospike-client-go v3.1.0+incompatible


Interesting, the +incompatible label is actually how go/mod sees the release: https://pkg.go.dev/github.com/aerospike/[email protected]+incompatible?tab=versions

Since v3.1.1+incompatible has been released, we should consider bumping it to that version (unless there's a particular reason not to)

calvn · 2021-01-05T19:38:45Z

physical/aerospike/aerospike.go

+		return nil, err
+	}
+
+	return &AerospikeBackend{


Looks like set is optional, and both namespace and hostname now have default values so we should be good.

reugn · 2021-01-06T08:21:56Z

@calvn for some reason, it doesn't pass the test-go-race-remote-docker with the aerospike client v3.1.1 .

calvn · 2021-01-12T17:18:28Z

Can you give v3.1.1 another try? The failure seems to be from a non-related test:

Failed
=== RUN   TestRaft_SnapshotAPI_RekeyRotate_Backward
--- FAIL: TestRaft_SnapshotAPI_RekeyRotate_Backward (0.00s)

There also seems to be a conflict with go.sum since your latest push. It's looking ready though!

reugn · 2021-01-12T18:43:40Z

Still failed.

calvn · 2021-01-12T19:02:22Z

Looks like that's a known flaky test. I'm kicking off CI again to see if we can get a passing run.

calvn · 2021-01-12T19:34:05Z

@reugn can you merge master into this PR to get the latest set of changes and ensure that things are still working? Some of the flakiness and test failures might have already been addressed on master.

# Conflicts: # go.sum

calvn · 2021-01-12T20:03:59Z

Can you move the docs page to website/content/docs/configuration/storage/? We recently changed where these files live.

add an Aerospike storage backend

d7c2aae

reugn added 2 commits October 12, 2020 18:34

go mod vendor

404471e

add Aerospike storage configuration docs

08c898d

kalafut suggested changes Dec 9, 2020

View reviewed changes

kalafut self-assigned this Dec 9, 2020

kalafut reviewed Dec 9, 2020

View reviewed changes

calvn reviewed Dec 10, 2020

View reviewed changes

physical/aerospike/aerospike.go Show resolved Hide resolved

calvn reviewed Dec 10, 2020

View reviewed changes

website/pages/docs/configuration/storage/aerospike.mdx Outdated Show resolved Hide resolved

review fixes

14fccc9

reugn requested review from kalafut and calvn December 10, 2020 15:36

calvn reviewed Jan 5, 2021

View reviewed changes

calvn approved these changes Jan 5, 2021

View reviewed changes

reugn added 2 commits January 5, 2021 22:14

bump aerospike client to v3.1.1

a8fc7a7

rename the defaultHostname variable

e234700

reugn force-pushed the aerospike_backend branch from 263443e to e234700 Compare January 6, 2021 07:43

calvn self-requested a review January 12, 2021 17:18

reugn force-pushed the aerospike_backend branch from ae3b7a0 to e234700 Compare January 12, 2021 17:59

Merge branch 'master' into aerospike_backend

f7082b0

# Conflicts: # go.sum

relocate the docs page

4f93cf6

calvn added this to the 1.7 milestone Jan 12, 2021

calvn merged commit 6423be8 into hashicorp:master Jan 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Aerospike storage backend #10131

Aerospike storage backend #10131

reugn commented Oct 12, 2020

hashicorp-cla commented Oct 12, 2020 •

edited

Loading

reugn commented Nov 3, 2020 •

edited

Loading

kalafut commented Dec 4, 2020

kalafut left a comment

kalafut Dec 9, 2020

calvn Jan 5, 2021

kalafut Dec 9, 2020

kalafut Dec 9, 2020

kalafut Dec 9, 2020

kalafut Dec 9, 2020

kalafut Dec 9, 2020

kalafut Dec 9, 2020

kalafut Dec 9, 2020

kalafut Dec 9, 2020

kalafut Dec 9, 2020

kalafut Dec 9, 2020 •

edited

Loading

calvn Dec 10, 2020

calvn Dec 10, 2020

reugn commented Dec 10, 2020 •

edited

Loading

calvn Jan 5, 2021

calvn Jan 5, 2021

calvn Jan 5, 2021

calvn left a comment

calvn Jan 5, 2021

calvn Jan 5, 2021

calvn Jan 5, 2021

reugn commented Jan 6, 2021

calvn commented Jan 12, 2021

reugn commented Jan 12, 2021

calvn commented Jan 12, 2021

calvn commented Jan 12, 2021 •

edited

Loading

calvn commented Jan 12, 2021


		# Aerospike Storage Backend

		The Aerospike storage backend is used to persist Vault's data in


		- `auth_mode` `(string: "INTERNAL")` - Specifies the authentication mode when user/password is defined (INTERNAL/EXTERNAL).

		- `timeout` `(int: 30000)` - Initial host connection timeout duration in milliseconds.

Aerospike storage backend #10131

Aerospike storage backend #10131

Conversation

reugn commented Oct 12, 2020

hashicorp-cla commented Oct 12, 2020 • edited Loading

reugn commented Nov 3, 2020 • edited Loading

kalafut commented Dec 4, 2020

kalafut 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

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

kalafut Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reugn commented Dec 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvn 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

reugn commented Jan 6, 2021

calvn commented Jan 12, 2021

reugn commented Jan 12, 2021

calvn commented Jan 12, 2021

calvn commented Jan 12, 2021 • edited Loading

calvn commented Jan 12, 2021

hashicorp-cla commented Oct 12, 2020 •

edited

Loading

reugn commented Nov 3, 2020 •

edited

Loading

kalafut Dec 9, 2020 •

edited

Loading

reugn commented Dec 10, 2020 •

edited

Loading

calvn commented Jan 12, 2021 •

edited

Loading