Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

raft: add support for using backend for ha_storage #9193

Merged
merged 52 commits into from
Jun 23, 2020

Conversation

calvn
Copy link
Contributor

@calvn calvn commented Jun 10, 2020

No description provided.

@calvn calvn added this to the 1.5 milestone Jun 10, 2020
// If we encountered and error we should try to create the key
// again.
if backoff {
nextRotationTime = time.Now().Add(10 * time.Second)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious, is the 10 seconds here arbitrary? How many times do we foresee backoff to be hit before success?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is just an arbitrary value that's short enough for a retry. I'm assuming this is to guard against any potential errors encountered during rotation, such as inability to access physical storage due to some transient failure. @briankassouf might have more context on this.

Co-authored-by: Alexander Bezobchuk <[email protected]>
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Performed another review. Looks good @calvn 👍

c.logger.Error("failed to create raft TLS keyring", "error", err)
return nil, err
if raftBackend != nil {
if _, err := c.raftCreateTLSKeyring(ctx); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've wrapped these functions in that nil check a few places, but don't they already check if we are running raft storage in the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not necessary to wrap in this case. The other call is based on onInit though, so I'll leave that untouched.

Copy link
Contributor Author

@calvn calvn Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I've been trying to get away from returning (nil, nil) as a pattern. I'll keep this as-is and return (nil, error) instead on that method.

Copy link
Contributor

@vishalnayak vishalnayak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking great!

Copy link
Contributor

@briankassouf briankassouf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

@calvn calvn merged commit 045836d into master Jun 23, 2020
@briankassouf briankassouf deleted the feature-raft-ha-storage branch June 23, 2020 21:38
catsby added a commit that referenced this pull request Jun 24, 2020
* master:
  Entity and alias counts (#9262)
  Token gauge metrics implementation. (#9239)
  mfa: fix import path on test file (#9303)
  doc: update vault helm enterprise image examples (#9299)
  raft: add support for using backend for ha_storage (#9193)
  Document new and previously undocumented telemetry metrics: (#9283)
  Improve the performance of snapshot installs by using rename (#9247)
  docs: add additional info around transform for tweak and template type (#9203)
  Update CHANGELOG.md
  CL++: Add go version to server message output
andaley pushed a commit that referenced this pull request Jul 17, 2020
* raft: initial work on raft ha storage support

* add note on join

* add todo note

* raft: add support for bootstrapping and joining existing nodes

* raft: gate bootstrap join by reading leader api address from storage

* raft: properly check for raft-only for certain conditionals

* raft: add bootstrap to api and cli

* raft: fix bootstrap cli command

* raft: add test for setting up new cluster with raft HA

* raft: extend TestRaft_HA_NewCluster to include inmem and consul backends

* raft: add test for updating an existing cluster to use raft HA

* raft: remove debug log lines, clean up verifyRaftPeers

* raft: minor cleanup

* raft: minor cleanup

* Update physical/raft/raft.go

Co-authored-by: Brian Kassouf <[email protected]>

* Update vault/ha.go

Co-authored-by: Brian Kassouf <[email protected]>

* Update vault/ha.go

Co-authored-by: Brian Kassouf <[email protected]>

* Update vault/logical_system_raft.go

Co-authored-by: Brian Kassouf <[email protected]>

* Update vault/raft.go

Co-authored-by: Brian Kassouf <[email protected]>

* Update vault/raft.go

Co-authored-by: Brian Kassouf <[email protected]>

* address feedback comments

* address feedback comments

* raft: refactor tls keyring logic

* address feedback comments

* Update vault/raft.go

Co-authored-by: Alexander Bezobchuk <[email protected]>

* Update vault/raft.go

Co-authored-by: Alexander Bezobchuk <[email protected]>

* address feedback comments

* testing: fix import ordering

* raft: rename var, cleanup comment line

* docs: remove ha_storage restriction note on raft

* docs: more raft HA interaction updates with migration and recovery mode

* docs: update the raft join command

* raft: update comments

* raft: add missing isRaftHAOnly check for clearing out state set earlier

* raft: update a few ha_storage config checks

* Update command/operator_raft_bootstrap.go

Co-authored-by: Vishal Nayak <[email protected]>

* raft: address feedback comments

* raft: fix panic when checking for config.HAStorage.Type

* Update vault/raft.go

Co-authored-by: Alexander Bezobchuk <[email protected]>

* Update website/pages/docs/commands/operator/raft.mdx

Co-authored-by: Alexander Bezobchuk <[email protected]>

* raft: remove bootstrap cli command

* Update vault/raft.go

Co-authored-by: Brian Kassouf <[email protected]>

* Update vault/raft.go

Co-authored-by: Brian Kassouf <[email protected]>

* raft: address review feedback

* raft: revert vendored sdk

* raft: don't send applied index and node ID info if we're HA-only

Co-authored-by: Brian Kassouf <[email protected]>
Co-authored-by: Alexander Bezobchuk <[email protected]>
Co-authored-by: Vishal Nayak <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants