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

Integrated Storage Cloud Auto-Join #10095

Merged
merged 15 commits into from Oct 13, 2020
Merged

Conversation

alexanderbez
Copy link
Contributor

No description provided.

@alexanderbez alexanderbez marked this pull request as ready for review October 6, 2020 16:08
vault/raft.go Outdated Show resolved Hide resolved
@@ -1116,3 +1151,14 @@ type answerResp struct {
Peers []raft.Peer `json:"peers"`
TLSKeyring *raft.TLSKeyring `json:"tls_keyring"`
}

func newDiscover() (*discover.Discover, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this helper still needed now that we're not adding k8s?

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 we can safely remove this 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But on second thought, just in case the go-discover API changes, I figure it's safe to be explicit. So I'm leaning towards keeping this.

Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

Minor comments, overall looks good!

@ncabatoff
Copy link
Collaborator

Oh, except test-go is failing for some reason. Better fix that before merging.

@swayne275
Copy link
Contributor

1025 files changed haha, which ones should I care about?

@alexanderbez
Copy link
Contributor Author

1025 files changed haha, which ones should I care about?

All the non-vendor ones ;-p

command/operator_raft_join.go Outdated Show resolved Hide resolved

leaderInfos := []*raft.LeaderJoinInfo{
{
AutoJoin: "provider=aws region=eu-west-1 tag_key=consul tag_value=tag access_key_id=a secret_access_key=a",
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there a set of params that the user has to specify to use auto join? Like, provider and region make sense, but are there other fields that can be used instead of the fields here that specify, as I understand, an IAM user? If so, would it be helpful to have a list of fields that autoJoin can use to join a cluster?

I might also just have this question because I'm new to the raft library :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. It's entirely ambiguous and dependent on the cloud provider and operator infrastructure. See go-discover.

Copy link
Contributor

@swayne275 swayne275 left a comment

Choose a reason for hiding this comment

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

Looks good!

@alexanderbez alexanderbez merged commit b67da26 into master Oct 13, 2020
@alexanderbez alexanderbez deleted the core/is-cloud-auto-join-oss branch October 13, 2020 20:26
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.

None yet

4 participants