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

Add dr-token Flag to Autopilot CLI #21165

Merged
merged 7 commits into from Jul 27, 2023
Merged

Add dr-token Flag to Autopilot CLI #21165

merged 7 commits into from Jul 27, 2023

Conversation

ltcarbonell
Copy link
Contributor

For other operator commands such as list-peers we support a -dr-token flag:

vault operator raft list-peers -dr-token="<DR_OP_TOKEN>"

For autopilot commands, though, you have to pass it in via VAULT_TOKEN:

VAULT_TOKEN="<DR_OP_TOKEN>" vault operator raft autopilot state
VAULT_TOKEN="<DR_OP_TOKEN>" vault operator raft autopilot get-config
VAULT_TOKEN="<DR_OP_TOKEN>" vault operator raft autopilot set-config

These changes will allow the token to be passed via flag, as well as maintaining the VAULT_TOKEN env variable method

VAULT_TOKEN=$dr_token vault operator raft autopilot state
vault operator raft autopilot state -dr-token=$dr_token

VAULT_TOKEN=$dr_token vault operator raft autopilot get-config
vault operator raft autopilot get-config -dr-token=$dr_token

VAULT_TOKEN=$dr_token vault operator raft autopilot set-config \
    -dead-server-last-contact-threshold=10 \
    -server-stabilization-time=30 \
    -cleanup-dead-servers=true \
    -min-quorum=3
VAULT_TOKEN=$dr_token vault operator raft autopilot get-config    
vault operator raft autopilot set-config \
    -dead-server-last-contact-threshold=15 \
    -server-stabilization-time=45 \
    -cleanup-dead-servers=false \
    -min-quorum=3 \
    -dr-token=$dr_token 
vault operator raft autopilot get-config -dr-token=$dr_token

@ltcarbonell ltcarbonell added this to the 1.13.4 milestone Jun 13, 2023
@@ -138,6 +155,10 @@ func (c *OperatorRaftAutopilotSetConfigCommand) Run(args []string) int {
data["disable_upgrade_migration"] = c.flagDisableUpgradeMigration.Get()
}

if c.flagDRToken != "" {
client.SetToken(c.flagDRToken)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious about this pattern of using client.SetToken() for passing along the DR token. If this works, why not use it for the state and get config endpoints too, instead of creating special methods for them?

Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't verified it, but the fact that DR operation tokens are their own parameters to API calls makes me dubious that this works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had a similar thought/questions. And from looking into the code and the original change to do it that way (#10856) It still wasn't very clear to me. Perhaps @vishalnayak might be able to provide some light, since they made the original change (albeit a while back).

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear to me that this is a valid pattern. I would prefer if this was done in a similar style to how it's done for other raft commands that support a -dr-token flag, e.g. list-peers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand what you are saying now. I will work on that 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind 🤦 . I can see from your terminal example above that it must be valid, otherwise passing in the DR operation token via VAULT_TOKEN wouldn't have worked. It seems confusing that we support that way of using DR operation tokens, but many places in our documentation shows explicitly passing them in via their own parameter. 🤷

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I looked at this a bit more and it looks like this is where we set a DR token (if passed through), or default to the ClientToken. https://github.com/hashicorp/vault-enterprise/blob/main/vault/replication_api_ent.go#L803-L808. I think you are correct that it is probably better to set this token explicitly, rather than just set a client token (since this looks like it is already built in). I went ahead and pushed that commit for you to review.

command/operator_raft_autopilot_state.go Outdated Show resolved Hide resolved
api/sys_raft.go Show resolved Hide resolved
@anwittin anwittin modified the milestones: 1.13.4, 1.13.5 Jun 16, 2023
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Jul 20, 2023
@github-actions
Copy link

Build Results:
All builds succeeded! ✅

@github-actions
Copy link

CI Results:
All Go tests succeeded! ✅

@ltcarbonell ltcarbonell removed this from the 1.13.5 milestone Jul 20, 2023
@ltcarbonell ltcarbonell enabled auto-merge (squash) July 27, 2023 15:20
@ltcarbonell ltcarbonell merged commit 727c73c into main Jul 27, 2023
95 checks passed
@ltcarbonell ltcarbonell deleted the drtokenflag branch July 27, 2023 15:35
@ltcarbonell ltcarbonell added backport/1.12.x backport/1.13.x Backport changes to `release/1.13.x` labels Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/1.13.x Backport changes to `release/1.13.x` backport/1.14.x Backport changes to `release/1.14.x` hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed pr/no-milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants