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

Fix defaults for autopilot config update #10559

Merged
merged 2 commits into from
Jul 6, 2021
Merged

Fix defaults for autopilot config update #10559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
14059c2
Fix defaults for autopilot config update
jkirschner-hashicorp Jul 2, 2021
1353ee4
Add changelog for PR 10559
jkirschner-hashicorp Jul 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .changelog/10559.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
api: Fix default values used for optional fields in autopilot configuration update (POST to `/v1/operator/autopilot/configuration`) [[GH-10558](https://github.com/hashicorp/consul/issues/10558)]
Copy link
Contributor

Choose a reason for hiding this comment

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

the changelog tool will add the github link for us. I guess it's not strictly necessary to remove it, just an FYI that you don't need to add it

Suggested change
api: Fix default values used for optional fields in autopilot configuration update (POST to `/v1/operator/autopilot/configuration`) [[GH-10558](https://github.com/hashicorp/consul/issues/10558)]
api: Fix default values used for optional fields in autopilot configuration update (POST to `/v1/operator/autopilot/configuration`)

```
2 changes: 1 addition & 1 deletion agent/operator_endpoint.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ func (s *HTTPHandlers) OperatorAutopilotConfiguration(resp http.ResponseWriter,
s.parseDC(req, &args.Datacenter)
s.parseToken(req, &args.Token)

var conf api.AutopilotConfiguration
conf := api.NewAutopilotConfiguration()
if err := decodeBody(req.Body, &conf); err != nil {
return nil, BadRequestError{Reason: fmt.Sprintf("Error parsing autopilot config: %v", err)}
}
Expand Down
22 changes: 19 additions & 3 deletions agent/operator_endpoint_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,21 @@ func TestOperator_AutopilotSetConfiguration(t *testing.T) {
a := NewTestAgent(t, "")
defer a.Shutdown()

// Provide a non-default value only for CleanupDeadServers.
// Expect all other fields to be updated with default values
// (except CreateIndex and ModifyIndex).
body := bytes.NewBuffer([]byte(`{"CleanupDeadServers": false}`))
expected := structs.AutopilotConfig{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Improved test by forcing comparison of full configuration object (except CreateIndex and ModifyIndex), per review

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Side note: I got a bit tripped up between AutopilotConfig and AutopilotConfiguration

CleanupDeadServers: false, // only non-default value
LastContactThreshold: 200 * time.Millisecond,
MaxTrailingLogs: 250,
MinQuorum: 0,
ServerStabilizationTime: 10 * time.Second,
RedundancyZoneTag: "",
DisableUpgradeMigration: false,
UpgradeVersionTag: "",
}

req, _ := http.NewRequest("PUT", "/v1/operator/autopilot/configuration", body)
resp := httptest.NewRecorder()
if _, err := a.srv.OperatorAutopilotConfiguration(resp, req); err != nil {
Expand All @@ -453,9 +467,11 @@ func TestOperator_AutopilotSetConfiguration(t *testing.T) {
if err := a.RPC("Operator.AutopilotGetConfiguration", &args, &reply); err != nil {
t.Fatalf("err: %v", err)
}
if reply.CleanupDeadServers {
t.Fatalf("bad: %#v", reply)
}

// For equality comparison check, ignore CreateIndex and ModifyIndex
expected.CreateIndex = reply.CreateIndex
expected.ModifyIndex = reply.ModifyIndex
require.Equal(t, expected, reply)
}

func TestOperator_AutopilotCASConfiguration(t *testing.T) {
Expand Down
17 changes: 17 additions & 0 deletions api/operator_autopilot.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,23 @@ type AutopilotConfiguration struct {
ModifyIndex uint64
}

// Defines default values for the AutopilotConfiguration type, consistent with
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved function description above declaration, per review

// https://www.consul.io/api-docs/operator/autopilot#parameters-1
func NewAutopilotConfiguration() AutopilotConfiguration {
cfg := AutopilotConfiguration{
CleanupDeadServers: true,
LastContactThreshold: NewReadableDuration(200 * time.Millisecond),
MaxTrailingLogs: 250,
MinQuorum: 0,
ServerStabilizationTime: NewReadableDuration(10 * time.Second),
RedundancyZoneTag: "",
DisableUpgradeMigration: false,
UpgradeVersionTag: "",
}

return cfg
}

// ServerHealth is the health (from the leader's point of view) of a server.
type ServerHealth struct {
// ID is the raft ID of the server.
Expand Down