Added HA backend for postgres based on dynamodb model #5731

bjorndolk · 2018-11-08T10:59:15Z

This is a clone on the excellent dynamodb implementation, with database specifics replaced to postgres.

There is another related postgres PR #2700. However that implementation does not pass the genric HA tests and I believe its lock model is inferior to the one implemented in dynamodb.

Please consider this contribution as a substitute to #2700

bjorndolk · 2018-11-08T11:35:02Z

Edit: no longer an issue.
Reviewer should probalby take a peek at this thread https://groups.google.com/forum/#!topic/vault-tool/1Sx2aedKt94

jefferai · 2018-11-08T14:28:23Z

@bjorndolk I don't really know anything about Postgres so I can't review it. I do know that the Dynamo implementation is pretty jank and a lot of people have had issues with it -- it's the reason ha_enabled is off by default -- so I'm not sure basing an implementation on that model is the right move.

bjorndolk · 2018-11-09T09:56:03Z

@jefferai It was the implemention I liked most. One difference in this implementation is that this implementation use a central clock instead on serveral possibly out of sync clocks. I can imagine dynamodb platform having latency issues which may cause problems. A postgres database will typically be responsive.

Can you please try to help answering the question wether or not the forward call from standbys to masters is expected to work or not when only implementing the basics in HA backend. I tried to dig into the vault code to find this out myself, but I got lost. I suspect implementation of ServiceDescovery interface may be needed for this to work. For us we, we dont need forward calls to work. We can solve this with loadbalancer config.

bjorndolk · 2018-11-09T10:28:06Z

I browsed dynamodb issues:
#5077
This implementation separates HA data from Data-data :). Dynamodb stores the ha_keyvalue in the key-value store rather in a HA key store as this implementation does. I chosed to do so since its easier to do transactions on one table rather than two.

physical/postgresql/postgresql_test.go

and modified code to setup HA tables for tests

bjorndolk · 2018-12-21T08:06:37Z

Merged with #5926 to have a test that uses a docker backend postgres database as part of testing.

bjorndolk · 2019-01-10T11:35:11Z

I have done some more testing and scripted the setup for creating a docker database and three competing vault instances running postgres HA. The script does not verify anything it is only a help to setup an environment to play around in. Killing and starting/unsealing vault instances making sure master is moved correctly.
It would be nice if this type of tests was part of standard backend storage unit tests in a generic form to allow for all backends to be tested. Hopefully someone will provide that in the future.

vault-pg-ha-test.txt

…exists, hereby fixing the problem with missing Active Node address for passive nodes.

bjorndolk · 2019-01-10T14:49:11Z

@jefferai I have now sorted out the problem with forwards calls to passive nodes as discussed above. I had missunderstood the meaning of what a "held" lock is, anyway it is sorted now. As you can see in history this also comes with a docker test setup now.

Also I dont think there is a need for deeper postgres knowlegde to review this. The interaction with the database is in my mind simple and easy to comprehend.

ncabatoff · 2019-04-03T16:06:04Z

Hi @bjorndolk,

Thank you for being patient with us. I'm going to help you get this merged. Bear with me please because I'm still coming up to speed with vault HA backends. I'm going to provide some initial thoughts on your patch in isolation, then spend some time digging in to other HA backends, at which point I'll likely have some further thoughts.

physical/postgresql/postgresql.go

website/source/docs/configuration/storage/postgresql.html.md

physical/postgresql/postgresql.go

website/source/docs/configuration/storage/postgresql.html.md

physical/postgresql/postgresql.go

bjorndolk · 2019-04-11T06:24:31Z

@ncabatoff, Thanks for not letting my work go to waste. I will dig into your comments hopefully this or next week.

…mode. Add PostgreSQLLock.renewTicker to manage the lock renewal lifecycle. Add PostgreSQLLock fields ttl, renewInterval, and retryInterval: this allows modifying the default values in tests. In Lock(), use the stopCh argument to control goroutine termination instead of making out own. Remove unused done channel. tryToLock/writeItem() weren't doing quite what the dynamodb versions that inspired them did: in the dynamodb version, when a lock wasn't obtained because it was held by another instance, the lock attempt would be retried. In this version we weren't doing that because any failure to grab the lock for whatever reason was treated as an error and resulted in a write to the errors chan. To address this writeItem now also returns a bool indicating whether it wrote exactly 1 row, indicating that the upsert succeeded. When (false, nil) is returned that means no error occurred but also that no lock was obtained, so tryToLock will retry. periodicallyRenewLock() exits when it doesn't lock successfully, and closes its done channel. This obviates the need for watch(), which has been removed. Unlock() stops the ticker used in periodicallyRenewLock, which saves us from falling victim to the same problem as hashicorp#5828. Added testPostgreSQLLockRenewal which attempts to duplicate in spirit the test from hashicorp#6512, and testPostgresSQLLockTTL which is the exact equivalent of the corresponding DynamoDB test.

postgres ha support review fixes and tests

bjorndolk · 2019-04-17T06:48:57Z

@ncabatoff I have merged your PR addressing your own review without changes. Thanks for helping out on this.

bjorndolk · 2019-04-17T06:56:00Z

@ncabatoff since you are reviewing physical backends more generally. It would be nice if...
*There was a implemation of the logic by hashicorp and a backend could be developed only implemnting database specific operations. Instead of copy pasting antother backend logic as done here.
*Better generic backend tests that actually started serveral running nodes and beeing killed/restarted. I bet if those existed some very serious bugs would be found in some existing backends.

bjorndolk · 2019-04-17T07:34:20Z

I have successfully tested started 3 instances and made sure active node is moved around while killing the master. Used attached setup.
ha-test-setup.txt

physical/postgresql/postgresql.go

physical/postgresql/postgresql_test.go

kalafut

Looks pretty good! Just a few questions plus some small cleanup suggestions.

bjorndolk · 2019-04-25T13:46:03Z

@ncabatoff I'll look into this tomorrow.

physical/postgresql/postgresql.go

Co-Authored-By: bjorndolk <44286676+bjorndolk@users.noreply.github.com>

Space in comment Co-Authored-By: bjorndolk <44286676+bjorndolk@users.noreply.github.com>

space in comment Co-Authored-By: bjorndolk <44286676+bjorndolk@users.noreply.github.com>

Accepting ncabatoff changes Addressing review comments from kalafut. General cleanup + handling of no lock found case.

bjorndolk · 2019-04-26T06:15:15Z

@ncabatoff I have merged your PR without change
I granted you access to my fork. I put my full trust in you, You are currently the one doing the work, I am grateful for it.

ncabatoff · 2019-04-26T12:27:15Z

@ncabatoff I have merged your PR without change
I granted you access to my fork. I put my full trust in you, You are currently the one doing the work, I am grateful for it.

Thanks @bjorndolk, we're grateful that you did the first 90% of the work!

bjorndolk · 2019-04-26T13:35:29Z

@ncabatoff @kalafut Thanks for helping out, I think your input and work has simplified and generally greatly improved quality. I think its in good shape. The one thing I can be critical of now is that the timing values for gaurding locks possibly should be softconfigurable. Maybe someone would like to decrease it to one seconds or less so to minimize failover time. But that is a nice to have rather than must have.

ncabatoff · 2019-04-28T19:37:55Z

I was keen to see what this would look like in practice, so I created a docker-compose setup to run Postgres, two Vault instances with suicidal tendencies and a third Vault instance to allow them to auto-unseal, Consul to discover which Vaults are healthy, a client to generate load on them, and Prometheus/Grafana/consul-exporter to visualize all that. If you want to give it a try see https://github.com/ncabatoff/visualize-vault-ha .

bjorndolk · 2019-05-02T05:15:25Z

@ncabatoff That looks really cool, I wish I had more time to check that out. I did run into problems satisfying the golang 1.11 requirement. I hope to get some time to resolve that and try your stuff.

…nto postgres-ha-support

Added HA backend for postgres

982b139

bjorndolk mentioned this pull request Nov 8, 2018

physical/postgresql: Add support for high availability #2700

Closed

bjorndolk added 2 commits December 10, 2018 10:13

Docker support for postgres backend testing

1768f1d

Bug in handling of postgres connection url for non docker testing

ed6ed5a

bjorndolk mentioned this pull request Dec 10, 2018

Physical postgres docker test #5926

Merged

lk95jofr suggested changes Dec 20, 2018

View reviewed changes

physical/postgresql/postgresql_test.go Outdated Show resolved Hide resolved

bjorndolk added 3 commits December 20, 2018 14:18

Fix HA enabled check in test

058731e

Merge branch 'physical-postgres-docker-test' into postgres-ha-support

40bac6f

and modified code to setup HA tables for tests

Formatting + actually configure test to run HA tests

68c2f9b

bjorndolk added 4 commits December 21, 2018 09:40

Test should fail if it cannot retrieve pg version

2f3f712

Merge branch 'physical-postgres-docker-test' into postgres-ha-support

0402379

internal helperfunctions pascalCasing

59d81fa

Merge remote-tracking branch 'upstream/master' into postgres-ha-support

aaa6d6b

Changed lock.Value behaviour to always return held when a lock value …

9baeb6f

…exists, hereby fixing the problem with missing Active Node address for passive nodes.

ncabatoff reviewed Apr 3, 2019

View reviewed changes

bjorndolk mentioned this pull request Apr 17, 2019

postgres ha support review fixes and tests bjorndolk/vault#1

Merged

Merge pull request #1 from ncabatoff/ncabatoff-postgres-ha-support

a5f45e1

postgres ha support review fixes and tests

kalafut reviewed Apr 18, 2019

View reviewed changes

physical/postgresql/postgresql.go Outdated Show resolved Hide resolved

kalafut reviewed Apr 18, 2019

View reviewed changes

physical/postgresql/postgresql.go Outdated Show resolved Hide resolved

kalafut reviewed Apr 18, 2019

View reviewed changes

physical/postgresql/postgresql.go Show resolved Hide resolved

kalafut reviewed Apr 19, 2019

View reviewed changes

physical/postgresql/postgresql.go Outdated Show resolved Hide resolved

kalafut reviewed Apr 19, 2019

View reviewed changes

physical/postgresql/postgresql_test.go Outdated Show resolved Hide resolved

kalafut reviewed Apr 19, 2019

View reviewed changes

Address review comments.

4d97c84

kalafut reviewed Apr 25, 2019

View reviewed changes

physical/postgresql/postgresql.go Show resolved Hide resolved

ncabatoff and others added 2 commits April 25, 2019 15:43

Value(): don't treat "no lock found" case as an error.

e4b4fdb

Update physical/postgresql/postgresql.go

4fa185c

Co-Authored-By: bjorndolk <44286676+bjorndolk@users.noreply.github.com>

bjorndolk dismissed ncabatoff’s stale review via 4fa185c April 26, 2019 05:14

kalafut and others added 6 commits April 26, 2019 07:15

Update physical/postgresql/postgresql.go

d61305b

Space in comment Co-Authored-By: bjorndolk <44286676+bjorndolk@users.noreply.github.com>

Update physical/postgresql/postgresql.go

4e258e4

Space in comment Co-Authored-By: bjorndolk <44286676+bjorndolk@users.noreply.github.com>

Update physical/postgresql/postgresql.go

090252b

Space in comment Co-Authored-By: bjorndolk <44286676+bjorndolk@users.noreply.github.com>

Update physical/postgresql/postgresql_test.go

aaa6848

space in comment Co-Authored-By: bjorndolk <44286676+bjorndolk@users.noreply.github.com>

Merge pull request #2 from ncabatoff/postgres-ha-support-review-fixes

f5c4d9c

Accepting ncabatoff changes Addressing review comments from kalafut. General cleanup + handling of no lock found case.

Merge remote-tracking branch 'upstream/master' into postgres-ha-support

ebbfbf2

Verify user has PG >= 9.5 before we allow HA to be enabled.

993fed1

ncabatoff approved these changes Apr 26, 2019

View reviewed changes

kalafut approved these changes May 6, 2019

View reviewed changes

Merge branch 'master' of ssh://git.luolix.top-ncabatoff/hashicorp/vault i…

48643db

…nto postgres-ha-support

ncabatoff merged commit 96e81be into hashicorp:master May 10, 2019

krish7919 mentioned this pull request Sep 4, 2019

Documentation: ha_table parameter in configuration docs #7416

Closed

michelvocks mentioned this pull request Nov 8, 2019

Vault can't handle properly AWS RDS postgresql multi-az failover #6792

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added HA backend for postgres based on dynamodb model #5731

Added HA backend for postgres based on dynamodb model #5731

bjorndolk commented Nov 8, 2018

bjorndolk commented Nov 8, 2018 •

edited

Loading

jefferai commented Nov 8, 2018

bjorndolk commented Nov 9, 2018 •

edited

Loading

bjorndolk commented Nov 9, 2018 •

edited

Loading

bjorndolk commented Dec 21, 2018

bjorndolk commented Jan 10, 2019 •

edited

Loading

bjorndolk commented Jan 10, 2019 •

edited

Loading

ncabatoff commented Apr 3, 2019

bjorndolk commented Apr 11, 2019

bjorndolk commented Apr 17, 2019

bjorndolk commented Apr 17, 2019 •

edited

Loading

bjorndolk commented Apr 17, 2019

kalafut left a comment

bjorndolk commented Apr 25, 2019

bjorndolk commented Apr 26, 2019 •

edited

Loading

ncabatoff commented Apr 26, 2019

bjorndolk commented Apr 26, 2019

ncabatoff commented Apr 28, 2019 •

edited

Loading

bjorndolk commented May 2, 2019

Added HA backend for postgres based on dynamodb model #5731

Added HA backend for postgres based on dynamodb model #5731

Conversation

bjorndolk commented Nov 8, 2018

bjorndolk commented Nov 8, 2018 • edited Loading

jefferai commented Nov 8, 2018

bjorndolk commented Nov 9, 2018 • edited Loading

bjorndolk commented Nov 9, 2018 • edited Loading

bjorndolk commented Dec 21, 2018

bjorndolk commented Jan 10, 2019 • edited Loading

bjorndolk commented Jan 10, 2019 • edited Loading

ncabatoff commented Apr 3, 2019

bjorndolk commented Apr 11, 2019

bjorndolk commented Apr 17, 2019

bjorndolk commented Apr 17, 2019 • edited Loading

bjorndolk commented Apr 17, 2019

kalafut left a comment

Choose a reason for hiding this comment

bjorndolk commented Apr 25, 2019

bjorndolk commented Apr 26, 2019 • edited Loading

ncabatoff commented Apr 26, 2019

bjorndolk commented Apr 26, 2019

ncabatoff commented Apr 28, 2019 • edited Loading

bjorndolk commented May 2, 2019

bjorndolk commented Nov 8, 2018 •

edited

Loading

bjorndolk commented Nov 9, 2018 •

edited

Loading

bjorndolk commented Nov 9, 2018 •

edited

Loading

bjorndolk commented Jan 10, 2019 •

edited

Loading

bjorndolk commented Jan 10, 2019 •

edited

Loading

bjorndolk commented Apr 17, 2019 •

edited

Loading

bjorndolk commented Apr 26, 2019 •

edited

Loading

ncabatoff commented Apr 28, 2019 •

edited

Loading