Env Flag Filtering

[maxcoulombe]

Summary

• added filtering for env flags when generating command parsing warnings

• removed some duplication and created constants so new flags are properly handled automatically if added to the defined collection

Problem

Env flags passed at the end of a command generates undesired warnings, e.g.:

vault read sys/auth/approle/tune -format=json
Flags must be provided before positional arguments. The following arguments will not be parsed as flags: [-format=json]
(...)

Context

Some flags are not tied to a command but to the general execution environment and are valid when passed at the very end. For example: format=json. Warnings introduced during this pull request could create false positives when environment flags are used appropriately as it treated any trailing flags as a potential user mistake.

With this fix env flags are acceptable both as flags options and args
vault <command> [options] [path] [args]

Example

$ vault read sys/auth/approle/tune --format=json
{
  "request_id": "33cafd05-197f-3fcb-53ba-bc1bf3689f23",
  "lease_id": "",
  "lease_duration": 0,
  "renewable": false,
  "data": {
    "default_lease_ttl": 2764800,
    "description": "",
    "force_no_cache": false,
    "max_lease_ttl": 2764800,
    "token_type": "default-service"
  },
  "warnings": [
    "Endpoint ignored these unrecognized parameters: [--format]"
  ]
}

$ vault read --format=json sys/auth/approle/tune
{
  "request_id": "2386f2f2-8c5c-915b-ffe3-7df7941cc088",
  "lease_id": "",
  "lease_duration": 0,
  "renewable": false,
  "data": {
    "default_lease_ttl": 2764800,
    "description": "",
    "force_no_cache": false,
    "max_lease_ttl": 2764800,
    "token_type": "default-service"
  },
  "warnings": null
}

[tomhjp]

LGTM, just naming + nits

[fairclothjm]

I did some testing with this. Here is something of interest that I found:

Vault v1.9.3:

$ vault read sys/auth/approle/tune --foo=bar --format=json
{
  "request_id": "5af11310-0b6e-466a-5770-6e8f30b5724a",
  "lease_id": "",
  "lease_duration": 0,
  "renewable": false,
  "data": {
    "default_lease_ttl": 2764800,
    "description": "",
    "force_no_cache": false,
    "max_lease_ttl": 2764800,
    "token_type": "default-service"
  },
  "warnings": null
}

Vault vault-2165-EnvFlagFiltering branch:

$ vault read sys/auth/approle/tune --foo=bar --format=json
Command flags must be provided before positional arguments. The following arguments will not be parsed as flags: [--foo=bar]
{
  "request_id": "a9057da5-a383-9279-9666-c4f00367dbbb",
  "lease_id": "",
  "lease_duration": 0,
  "renewable": false,
  "data": {
    "default_lease_ttl": 2764800,
    "description": "",
    "force_no_cache": false,
    "max_lease_ttl": 2764800,
    "token_type": "default-service"
  },
  "warnings": [
    "Endpoint ignored these unrecognized parameters: [--foo --format]"
  ]
}

I would consider this a breaking change. Maybe we do want this behavior though? In which case, we should call this out in the upgrade guide and the changelog entry.

It might be worth getting some more opinions here.

[tomhjp]

I think the warnings within the JSON response payload are due to this PR: #14962. Is that what you're referring to as a breaking change, or the CLI warning "The following arguments will not be parsed as flags: [--foo=bar]"?

It does seem odd though that --format is included in the JSON payload warnings.

[fairclothjm]

@tomhjp The breaking change I was referring to is the inclusion of Command flags must be provided before positional arguments. The following arguments will not be parsed as flags: [--foo=bar] before the returned json response. Users that were parsing json with tools like jq will be broken. I consider this to be a quite common use case so I am quite sensitive to it. Maybe this is ok though and we move forward but make sure to document the change?

As for the warnings, yes at first I was a bit confused. But maybe it makes sense if "endpoint" only refers to the /tune portion? But if by endpoint we mean Vault then I am not sure it makes sense.

[maxcoulombe]

I agree if the format is explicitly specified as json the output should be json-valid no matter the circumstances such as potential warnings. Thanks for pointing it out.

I added a check on the desired format before printing the warnings, let me know if you think it's a proper way of hangling this.

I believe with this adjustment since the warnings do not modify the behavior of the command but simply provide additional information on how it was already parsed we can consider this as non-breaking? Or we should still add a note to the upgrade guide?

Sample results:

max@max-Precision-7560:~/projects/vault$ vlto kv put -mount=secret foo bar=baz -asd=qwe
Command flags must be provided before positional arguments. The following arguments will not be parsed as flags: [-asd=qwe]
= Secret Path =
secret/data/foo

======= Metadata =======
Key                Value
---                -----
created_time       2022-08-17T18:33:45.196800638Z
custom_metadata    <nil>
deletion_time      n/a
destroyed          false
version            3
max@max-Precision-7560:~/projects/vault$ vlto kv put -mount=secret foo bar=baz -asd=qwe --format=json
{
  "request_id": "6a317946-18b0-618f-6431-67ba5bfcd351",
  "lease_id": "",
  "lease_duration": 0,
  "renewable": false,
  "data": {
    "created_time": "2022-08-17T18:33:46.650979208Z",
    "custom_metadata": null,
    "deletion_time": "",
    "destroyed": false,
    "version": 4
  },
  "warnings": null
}
max@max-Precision-7560:~/projects/vault

[tomhjp]

Good point about the JSON output 👍 agreed on the approach here