fix(cli): Status code on api error

When using the json output of the cli (`-format=json`), a successful
call to the controller API would place the HTTP status code in the JSON
as `status_code`. However, if there was an error from the API the status
code would be placed in the JSON as `status`. In order to be consistent,
this adds a `status_code` field to the error response case. For
backwards compatibility it still populates `status` in the error case.

This inconsistency was missed partially due to a bug in the CLI tests.
The assertion on status code was not valid and would always be
successful. Thus this also fixes the assertions and tests.

CHANGELOG.md

@@ -34,6 +34,12 @@ Canonical reference for changes, improvements, and bugfixes for Boundary.
   subcommand, will have the subtype set to `vault-generic`. The deprecated
   subtype and subcommands will be removed in boundary 0.14.0, at which point
   `vault-generic` must be used.
+* In Boundary 0.1.8 using the `-format=json` option with the cli would provide
+  a `status_code` for successful API requests from the cli. However, in the
+  case where an error was returned, the JSON would use `status` instead. This
+  inconsistency has been fixed, with `status_code` being used in both cases.
+  For error cases `status` will still be populated, but is deprecated and will
+  be removed in 0.14.0.
 
 ### New and Improved
 
@@ -68,6 +74,11 @@ Canonical reference for changes, improvements, and bugfixes for Boundary.
 * data warehouse: Fix bug that caused credential dimensions to not get
     associated with session facts ([PR](https://github.com/hashicorp/boundary/pull/2787)).
 * sessions: Fix two authorizeSession race conditions in handleProxy. ([PR](https://github.com/hashicorp/boundary/pull/2795))
+* cli: When using `-format=json` the JSON was inconsistent in how it reported
+  status codes. In successful cases it would use `status_code`, but in error
+  cases it would use `status`. Now `status_code` is used in both cases. In
+  error cases `status` is still populated, see the deprecations above for
+  more details. ([PR](https://github.com/hashicorp/boundary/pull/2887))
 
 ## 0.11.2 (2022/12/09)
 

internal/cmd/base/format.go

@@ -12,6 +12,7 @@ import (
 	"github.com/hashicorp/boundary/api"
 	"github.com/hashicorp/boundary/api/plugins"
 	"github.com/hashicorp/boundary/api/scopes"
+	"github.com/hashicorp/boundary/version"
 	"github.com/mitchellh/cli"
 	"github.com/mitchellh/go-wordwrap"
 	"github.com/pkg/errors"
@@ -181,16 +182,32 @@ func (c *Command) PrintApiError(in *api.Error, contextStr string, opt ...Option)
 	opts := getOpts(opt...)
 	switch Format(c.UI) {
 	case "json":
+		var b []byte
+		if version.SupportsFeature(version.Binary, version.IncludeStatusInCli) {
 			output := struct {
 				Context    string          `json:"context,omitempty"`
+				StatusCode int             `json:"status_code"`
 				Status     int             `json:"status"`
 				ApiError   json.RawMessage `json:"api_error"`
 			}{
 				Context:    contextStr,
+				StatusCode: in.Response().StatusCode(),
 				Status:     in.Response().StatusCode(),
 				ApiError:   in.Response().Body.Bytes(),
 			}
-		b, _ := JsonFormatter{}.Format(output)
+			b, _ = JsonFormatter{}.Format(output)
+		} else {
+			output := struct {
+				Context    string          `json:"context,omitempty"`
+				StatusCode int             `json:"status_code"`
+				ApiError   json.RawMessage `json:"api_error"`
+			}{
+				Context:    contextStr,
+				StatusCode: in.Response().StatusCode(),
+				ApiError:   in.Response().Body.Bytes(),
+			}
+			b, _ = JsonFormatter{}.Format(output)
+		}
 		c.UI.Error(string(b))
 
 	default:

internal/tests/cli/boundary/_accounts.bash

@@ -8,7 +8,7 @@ function read_account() {
 }
 
 function delete_account() {
-  boundary accounts delete -id $1
+  boundary accounts delete -id $1 -format json
 }
 
 function list_accounts() {

internal/tests/cli/boundary/_credential_libraries.bash

@@ -20,7 +20,7 @@ function read_credential_library() {
 }
 
 function delete_credential_library() {
-  boundary credential-libraries delete -id $1
+  boundary credential-libraries delete -id $1 -format json
 }
 
 function list_credential_libraries() {

internal/tests/cli/boundary/_credential_stores.bash

@@ -19,7 +19,7 @@ function read_credential_store() {
 }
 
 function delete_credential_store() {
-  boundary credential-stores delete -id $1
+  boundary credential-stores delete -id $1 -format json
 }
 
 function list_credential_stores() {

internal/tests/cli/boundary/_credentials.bash

@@ -32,7 +32,7 @@ function read_credential() {
 }
 
 function delete_credential() {
-  boundary credentials delete -id $1
+  boundary credentials delete -id $1 -format json
 }
 
 function list_credentials() {

internal/tests/cli/boundary/_groups.bash

@@ -9,7 +9,7 @@ function read_group() {
 }
 
 function delete_group() {
-  boundary groups delete -id $1
+  boundary groups delete -id $1 -format json
 }
 
 function list_groups() {

internal/tests/cli/boundary/_helpers.bash

@@ -24,9 +24,8 @@ function strip_all() {
 function has_status_code() {
   local json=$1
   local code=$2
-  if [ echo "$json"|jq -c ".status_code == $code" ]; then
-    return 1
-  fi
+  echo "checking .status_code == $code in $json"
+  echo "$json" | jq -e ".status_code == $code"
 }
 
 diag() {

internal/tests/cli/boundary/_host_catalogs.bash

@@ -15,7 +15,7 @@ function read_host_catalog() {
 }
 
 function delete_host_catalog() {
-  boundary host-catalogs delete -id $1
+  boundary host-catalogs delete -id $1 -format json
 }
 
 function list_host_catalogs() {

internal/tests/cli/boundary/_hosts.bash

@@ -16,7 +16,7 @@ function read_host() {
 }
 
 function delete_host() {
-  boundary hosts delete -id $1
+  boundary hosts delete -id $1 -format json
 }
 
 function list_hosts() {

internal/tests/cli/boundary/_roles.bash

@@ -17,7 +17,7 @@ function read_role() {
 }
 
 function delete_role() {
-  boundary roles delete -id $1
+  boundary roles delete -id $1 -format json
 }
 
 function list_roles() {

internal/tests/cli/boundary/_scopes.bash

@@ -16,7 +16,7 @@ function read_scope() {
 function delete_scope() {
   local sid=$1
 
-  boundary scopes delete -id $sid
+  boundary scopes delete -id $sid -format json
 }
 
 function list_scopes() {

internal/tests/cli/boundary/_targets.bash

@@ -32,7 +32,7 @@ function read_target() {
 }
 
 function delete_target() {
-  boundary targets delete -id $1
+  boundary targets delete -id $1 -format json
 }
 
 function list_targets() {

internal/tests/cli/boundary/_users.bash

@@ -9,7 +9,7 @@ function read_user() {
 }
 
 function delete_user() {
-  boundary users delete -id $1
+  boundary users delete -id $1 -format json
 }
 
 function list_users() {

internal/tests/cli/boundary/_workers.bash

@@ -15,7 +15,7 @@ function update_worker() {
 }
 
 function delete_worker() {
-  boundary workers delete -id $1
+  boundary workers delete -id $1 -format json
 }
 
 function list_workers() {

internal/tests/cli/boundary/credential_libraries.bats

@@ -73,6 +73,13 @@ export NEW_VAULT_LIB="test_vault"
   [ "$status" -eq 0 ]
   run has_status_code "$output" "204"
   [ "$status" -eq 0 ]
+
+  run delete_credential_library $clid
+  echo "$output"
+  [ "$status" -eq 1 ]
+  run has_status_code "$output" "404"
+  echo "$output"
+  [ "$status" -eq 0 ]
 }
 
 @test "boundary/credential-libraries: can create $NEW_VAULT_LIB vault library in credential store $NEW_STORE deprecated subcommand" {

version/feature_manager.go

@@ -23,6 +23,7 @@ type Feature int
 const (
 	UnknownFeature Feature = iota
 	MultiHopSessionFeature
+	IncludeStatusInCli
 )
 
 var featureMap map[Feature]MetadataConstraint
@@ -52,6 +53,10 @@ func init() {
 			Constraints: mustNewConstraints(">= 0.1.0"), // This feature exists at 0.1.0 and above
 		}
 	*/
+	featureMap[IncludeStatusInCli] = MetadataConstraint{
+		MetaInfo:    []Metadata{OSS, HCP},
+		Constraints: mustNewConstraints("< 0.14.0"),
+	}
 }
 
 func metadataStringToMetadata(m string) Metadata {