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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1353370
Add dr-token flag for raft autopilot
ltcarbonell Jun 13, 2023
daf23cf
Add changelog
ltcarbonell Jun 13, 2023
cf8ecfc
Cleanup bad patch apply
ltcarbonell Jun 21, 2023
ac2c742
Merge branch 'main' into drtokenflag
ltcarbonell Jul 6, 2023
68eb27c
Merge branch 'main' into drtokenflag
ltcarbonell Jul 20, 2023
f78792d
Set dr token explicitly
ltcarbonell Jul 21, 2023
da43356
Merge branch 'main' into drtokenflag
ltcarbonell Jul 27, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 14 additions & 0 deletions api/sys_raft.go
View file
Open in desktop
Expand Up @@ -276,6 +276,11 @@ func (c *Sys) RaftAutopilotState() (*AutopilotState, error) {
return c.RaftAutopilotStateWithContext(context.Background())
}

// RaftAutopilotStateWithToken wraps RaftAutopilotStateWithContext using the given token.
func (c *Sys) RaftAutopilotStateWithDRToken(drToken string) (*AutopilotState, error) {
return c.RaftAutopilotStateWithContext(context.WithValue(context.Background(), "dr-token", drToken))
}

// RaftAutopilotStateWithContext returns the state of the raft cluster as seen by autopilot.
func (c *Sys) RaftAutopilotStateWithContext(ctx context.Context) (*AutopilotState, error) {
ctx, cancelFunc := c.c.withConfiguredTimeout(ctx)
Expand Down Expand Up @@ -316,11 +321,20 @@ func (c *Sys) RaftAutopilotConfiguration() (*AutopilotConfig, error) {
return c.RaftAutopilotConfigurationWithContext(context.Background())
}

// RaftAutopilotConfigurationWithDRToken wraps RaftAutopilotConfigurationWithContext using the given token.
func (c *Sys) RaftAutopilotConfigurationWithDRToken(drToken string) (*AutopilotConfig, error) {
return c.RaftAutopilotConfigurationWithContext(context.WithValue(context.Background(), "dr-token", drToken))
}

// RaftAutopilotConfigurationWithContext fetches the autopilot config.
func (c *Sys) RaftAutopilotConfigurationWithContext(ctx context.Context) (*AutopilotConfig, error) {
ctx, cancelFunc := c.c.withConfiguredTimeout(ctx)
defer cancelFunc()

if ctx.Value("dr-token") != nil {
c.c.SetToken(ctx.Value("dr-token").(string))
}

ltcarbonell marked this conversation as resolved.
Show resolved Hide resolved
r := c.c.NewRequest(http.MethodGet, "/v1/sys/storage/raft/autopilot/configuration")

resp, err := c.c.rawRequestWithContext(ctx, r)
Expand Down
3 changes: 3 additions & 0 deletions changelog/21165.txt
View file
Open in desktop
@@ -0,0 +1,3 @@
```release-note:bug
raft/autopilot: Add dr-token flag for raft autopilot cli commands
```
23 changes: 19 additions & 4 deletions command/operator_raft_autopilot_get_config.go
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"strings"

"github.com/hashicorp/vault/api"
"github.com/mitchellh/cli"
"github.com/posener/complete"
)
Expand All @@ -18,6 +19,7 @@ var (

type OperatorRaftAutopilotGetConfigCommand struct {
*BaseCommand
flagDRToken string
}

func (c *OperatorRaftAutopilotGetConfigCommand) Synopsis() string {
Expand All @@ -37,6 +39,17 @@ Usage: vault operator raft autopilot get-config
func (c *OperatorRaftAutopilotGetConfigCommand) Flags() *FlagSets {
set := c.flagSet(FlagSetHTTP | FlagSetOutputFormat)

f := set.NewFlagSet("Command Options")

f.StringVar(&StringVar{
Name: "dr-token",
Target: &c.flagDRToken,
Default: "",
EnvVar: "",
Completion: complete.PredictAnything,
Usage: "DR operation token used to authorize this request (if a DR secondary node).",
})

return set
}

Expand Down Expand Up @@ -70,10 +83,12 @@ func (c *OperatorRaftAutopilotGetConfigCommand) Run(args []string) int {
return 2
}

config, err := client.Sys().RaftAutopilotConfiguration()
if err != nil {
c.UI.Error(err.Error())
return 2
var config *api.AutopilotConfig
switch {
case c.flagDRToken != "":
config, err = client.Sys().RaftAutopilotConfigurationWithDRToken(c.flagDRToken)
default:
config, err = client.Sys().RaftAutopilotConfiguration()
}

if config == nil {
Expand Down
21 changes: 21 additions & 0 deletions command/operator_raft_autopilot_set_config.go
View file
Open in desktop
Expand Up @@ -26,6 +26,7 @@ type OperatorRaftAutopilotSetConfigCommand struct {
flagMinQuorum uint
flagServerStabilizationTime time.Duration
flagDisableUpgradeMigration BoolPtr
flagDRToken string
}

func (c *OperatorRaftAutopilotSetConfigCommand) Synopsis() string {
Expand All @@ -50,36 +51,52 @@ func (c *OperatorRaftAutopilotSetConfigCommand) Flags() *FlagSets {
f.BoolPtrVar(&BoolPtrVar{
Name: "cleanup-dead-servers",
Target: &c.flagCleanupDeadServers,
Usage: "Controls whether to remove dead servers from the Raft peer list periodically or when a new server joins.",
})

f.DurationVar(&DurationVar{
Name: "last-contact-threshold",
Target: &c.flagLastContactThreshold,
Usage: "Limit on the amount of time a server can go without leader contact before being considered unhealthy.",
})

f.DurationVar(&DurationVar{
Name: "dead-server-last-contact-threshold",
Target: &c.flagDeadServerLastContactThreshold,
Usage: "Limit on the amount of time a server can go without leader contact before being considered failed. This takes effect only when cleanup_dead_servers is set.",
})

f.Uint64Var(&Uint64Var{
Name: "max-trailing-logs",
Target: &c.flagMaxTrailingLogs,
Usage: "Amount of entries in the Raft Log that a server can be behind before being considered unhealthy.",
})

f.UintVar(&UintVar{
Name: "min-quorum",
Target: &c.flagMinQuorum,
Usage: "Minimum number of servers allowed in a cluster before autopilot can prune dead servers. This should at least be 3.",
})

f.DurationVar(&DurationVar{
Name: "server-stabilization-time",
Target: &c.flagServerStabilizationTime,
Usage: "Minimum amount of time a server must be in a stable, healthy state before it can be added to the cluster.",
})

f.BoolPtrVar(&BoolPtrVar{
Name: "disable-upgrade-migration",
Target: &c.flagDisableUpgradeMigration,
Usage: "Whether or not to perform automated version upgrades.",
})

f.StringVar(&StringVar{
Name: "dr-token",
Target: &c.flagDRToken,
Default: "",
EnvVar: "",
Completion: complete.PredictAnything,
Usage: "DR operation token used to authorize this request (if a DR secondary node).",
})

return set
Expand Down Expand Up @@ -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.

}

secret, err := client.Logical().Write("sys/storage/raft/autopilot/configuration", data)
if err != nil {
c.UI.Error(err.Error())
Expand Down
30 changes: 21 additions & 9 deletions command/operator_raft_autopilot_state.go
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"strings"

"github.com/hashicorp/vault/api"
"github.com/mitchellh/cli"
"github.com/posener/complete"
)
Expand All @@ -19,6 +20,7 @@ var (

type OperatorRaftAutopilotStateCommand struct {
*BaseCommand
flagDRToken string
}

func (c *OperatorRaftAutopilotStateCommand) Synopsis() string {
Expand All @@ -38,6 +40,17 @@ Usage: vault operator raft autopilot state
func (c *OperatorRaftAutopilotStateCommand) Flags() *FlagSets {
set := c.flagSet(FlagSetHTTP | FlagSetOutputFormat)

f := set.NewFlagSet("Command Options")

f.StringVar(&StringVar{
Name: "dr-token",
Target: &c.flagDRToken,
Default: "",
EnvVar: "",
Completion: complete.PredictAnything,
Usage: "DR operation token used to authorize this request (if a DR secondary node).",
})

// The output of the state endpoint contains nested values and is not fit for
// the default "table" display format. Override the default display format to
// "pretty", both in the flag and in the UI.
Expand Down Expand Up @@ -69,21 +82,20 @@ func (c *OperatorRaftAutopilotStateCommand) Run(args []string) int {
return 1
}

args = f.Args()
switch len(args) {
case 0:
default:
c.UI.Error(fmt.Sprintf("Incorrect arguments (expected 0, got %d)", len(args)))
return 1
}

ltcarbonell marked this conversation as resolved.
Show resolved Hide resolved
client, err := c.Client()
if err != nil {
c.UI.Error(err.Error())
return 2
}

state, err := client.Sys().RaftAutopilotState()
var state *api.AutopilotState
switch {
case c.flagDRToken != "":
state, err = client.Sys().RaftAutopilotStateWithDRToken(c.flagDRToken)
default:
state, err = client.Sys().RaftAutopilotState()
}

if err != nil {
c.UI.Error(fmt.Sprintf("Error checking autopilot state: %s", err))
return 2
Expand Down