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 max_entry_size to sanitized config output #20044

Merged
merged 5 commits into from
Apr 14, 2023
Merged

add max_entry_size to sanitized config output #20044

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a19311c
add max_entry_size to sanitized config output
ccapurso Apr 7, 2023
3e1167b
add changelog entry
ccapurso Apr 7, 2023
cc23ded
add test parallelism
ccapurso Apr 12, 2023
8a9cfb2
add inmem test case
ccapurso Apr 12, 2023
ca39491
use named struct fields for TestSysConfigState_Sanitized cases
ccapurso Apr 13, 2023
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
4 changes: 4 additions & 0 deletions changelog/20044.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
```release-note:improvement
core: Add a `raft` sub-field to the `storage` and `ha_storage` details provided by the
`/sys/config/state/sanitized` endpoint in order to include the `max_entry_size`.
```
20 changes: 18 additions & 2 deletions command/server/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1133,23 +1133,39 @@ func (c *Config) Sanitized() map[string]interface{} {

// Sanitize storage stanza
if c.Storage != nil {
storageType := c.Storage.Type
sanitizedStorage := map[string]interface{}{
"type": c.Storage.Type,
"type": storageType,
"redirect_addr": c.Storage.RedirectAddr,
"cluster_addr": c.Storage.ClusterAddr,
"disable_clustering": c.Storage.DisableClustering,
}

if storageType == "raft" {
sanitizedStorage["raft"] = map[string]interface{}{
"max_entry_size": c.Storage.Config["max_entry_size"],
}
}

result["storage"] = sanitizedStorage
}

// Sanitize HA storage stanza
if c.HAStorage != nil {
haStorageType := c.HAStorage.Type
sanitizedHAStorage := map[string]interface{}{
"type": c.HAStorage.Type,
"type": haStorageType,
"redirect_addr": c.HAStorage.RedirectAddr,
"cluster_addr": c.HAStorage.ClusterAddr,
"disable_clustering": c.HAStorage.DisableClustering,
}

if haStorageType == "raft" {
sanitizedHAStorage["raft"] = map[string]interface{}{
"max_entry_size": c.HAStorage.Config["max_entry_size"],
}
}

result["ha_storage"] = sanitizedHAStorage
}

Expand Down
237 changes: 180 additions & 57 deletions http/sys_config_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,70 +9,193 @@ import (
"testing"

"github.com/go-test/deep"
"github.com/hashicorp/vault/command/server"
"github.com/hashicorp/vault/internalshared/configutil"
"github.com/hashicorp/vault/vault"
)

func TestSysConfigState_Sanitized(t *testing.T) {
var resp *http.Response

core, _, token := vault.TestCoreUnsealed(t)
ln, addr := TestServer(t, core)
defer ln.Close()
TestServerAuth(t, addr, token)

resp = testHttpGet(t, token, addr+"/v1/sys/config/state/sanitized")
testResponseStatus(t, resp, 200)

var actual map[string]interface{}
var expected map[string]interface{}

configResp := map[string]interface{}{
"api_addr": "",
"cache_size": json.Number("0"),
"cluster_addr": "",
"cluster_cipher_suites": "",
"cluster_name": "",
"default_lease_ttl": json.Number("0"),
"default_max_request_duration": json.Number("0"),
"disable_cache": false,
"disable_clustering": false,
"disable_indexing": false,
"disable_mlock": false,
"disable_performance_standby": false,
"disable_printable_check": false,
"disable_sealwrap": false,
"experiments": nil,
"raw_storage_endpoint": false,
"detect_deadlocks": "",
"introspection_endpoint": false,
"disable_sentinel_trace": false,
"enable_ui": false,
"log_format": "",
"log_level": "",
"max_lease_ttl": json.Number("0"),
"pid_file": "",
"plugin_directory": "",
"plugin_file_uid": json.Number("0"),
"plugin_file_permissions": json.Number("0"),
"enable_response_header_hostname": false,
"enable_response_header_raft_node_id": false,
"log_requests_level": "",
cases := []struct {
name string
storageConfig *server.Storage
haStorageConfig *server.Storage
expectedStorageOutput map[string]interface{}
expectedHAStorageOutput map[string]interface{}
}{
{
"raft storage",
mpalmi marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: these test cases would be a bit more readable if we used named struct fields, e.g.

{
  name: "raft storage",
  storageConfig: &server.Storage{...},
  ...
}

Copy link
Contributor Author

@ccapurso ccapurso Apr 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Good point, I'll change that. GoLand has a UI treatment to identify which fields are which but that doesn't help those using a different IDE, or reading it in GitHub. Thanks for the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See ca39491

&server.Storage{
Type: "raft",
RedirectAddr: "http://127.0.0.1:8200",
ClusterAddr: "http://127.0.0.1:8201",
DisableClustering: false,
Config: map[string]string{
"path": "/storage/path/raft",
"node_id": "raft1",
"max_entry_size": "2097152",
},
},
nil,
map[string]interface{}{
"type": "raft",
"redirect_addr": "http://127.0.0.1:8200",
"cluster_addr": "http://127.0.0.1:8201",
"disable_clustering": false,
"raft": map[string]interface{}{
"max_entry_size": "2097152",
},
},
nil,
},
{
"inmem storage, no HA storage",
&server.Storage{
Type: "inmem",
RedirectAddr: "http://127.0.0.1:8200",
ClusterAddr: "http://127.0.0.1:8201",
DisableClustering: false,
},
nil,
map[string]interface{}{
"type": "inmem",
"redirect_addr": "http://127.0.0.1:8200",
"cluster_addr": "http://127.0.0.1:8201",
"disable_clustering": false,
},
nil,
},
{
"inmem storage, raft HA storage",
&server.Storage{
Type: "inmem",
RedirectAddr: "http://127.0.0.1:8200",
ClusterAddr: "http://127.0.0.1:8201",
DisableClustering: false,
},
&server.Storage{
Type: "raft",
RedirectAddr: "http://127.0.0.1:8200",
ClusterAddr: "http://127.0.0.1:8201",
DisableClustering: false,
Config: map[string]string{
"path": "/storage/path/raft",
"node_id": "raft1",
"max_entry_size": "2097152",
},
},
map[string]interface{}{
"type": "inmem",
"redirect_addr": "http://127.0.0.1:8200",
"cluster_addr": "http://127.0.0.1:8201",
"disable_clustering": false,
},
map[string]interface{}{
"type": "raft",
"redirect_addr": "http://127.0.0.1:8200",
"cluster_addr": "http://127.0.0.1:8201",
"disable_clustering": false,
"raft": map[string]interface{}{
"max_entry_size": "2097152",
},
},
},
}

expected = map[string]interface{}{
"lease_id": "",
"renewable": false,
"lease_duration": json.Number("0"),
"wrap_info": nil,
"warnings": nil,
"auth": nil,
"data": configResp,
}
for _, tc := range cases {
tc := tc
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should this be removed?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is a common pattern to capture the loop variable inside a closure (see: #16872 for an example).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for calling that out. I meant to add t.Parallel to these which is why the capturing is present. I'll add parallelism.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See cc23ded


t.Run(tc.name, func(t *testing.T) {
t.Parallel()

var resp *http.Response
confRaw := &server.Config{
Storage: tc.storageConfig,
HAStorage: tc.haStorageConfig,
SharedConfig: &configutil.SharedConfig{
Listeners: []*configutil.Listener{
{
Type: "tcp",
Address: "127.0.0.1",
},
},
},
}

conf := &vault.CoreConfig{
RawConfig: confRaw,
}

core, _, token := vault.TestCoreUnsealedWithConfig(t, conf)
ln, addr := TestServer(t, core)
defer ln.Close()
TestServerAuth(t, addr, token)

resp = testHttpGet(t, token, addr+"/v1/sys/config/state/sanitized")
testResponseStatus(t, resp, 200)

var actual map[string]interface{}
var expected map[string]interface{}

configResp := map[string]interface{}{
"api_addr": "",
"cache_size": json.Number("0"),
"cluster_addr": "",
"cluster_cipher_suites": "",
"cluster_name": "",
"default_lease_ttl": json.Number("0"),
"default_max_request_duration": json.Number("0"),
"disable_cache": false,
"disable_clustering": false,
"disable_indexing": false,
"disable_mlock": false,
"disable_performance_standby": false,
"disable_printable_check": false,
"disable_sealwrap": false,
"experiments": nil,
"raw_storage_endpoint": false,
"detect_deadlocks": "",
"introspection_endpoint": false,
"disable_sentinel_trace": false,
"enable_ui": false,
"log_format": "",
"log_level": "",
"max_lease_ttl": json.Number("0"),
"pid_file": "",
"plugin_directory": "",
"plugin_file_uid": json.Number("0"),
"plugin_file_permissions": json.Number("0"),
"enable_response_header_hostname": false,
"enable_response_header_raft_node_id": false,
"log_requests_level": "",
"listeners": []interface{}{
map[string]interface{}{
"config": nil,
"type": "tcp",
},
},
"storage": tc.expectedStorageOutput,
}

if tc.expectedHAStorageOutput != nil {
configResp["ha_storage"] = tc.expectedHAStorageOutput
}

expected = map[string]interface{}{
"lease_id": "",
"renewable": false,
"lease_duration": json.Number("0"),
"wrap_info": nil,
"warnings": nil,
"auth": nil,
"data": configResp,
}

testResponseBody(t, resp, &actual)
expected["request_id"] = actual["request_id"]
testResponseBody(t, resp, &actual)
expected["request_id"] = actual["request_id"]

if diff := deep.Equal(actual, expected); len(diff) > 0 {
t.Fatalf("bad mismatch response body: diff: %v", diff)
if diff := deep.Equal(actual, expected); len(diff) > 0 {
t.Fatalf("bad mismatch response body: diff: %v", diff)
}
})
}
}