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.

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

Add support for -output flag in spire server federation commands #3660

Merged

Conversation

guilhermocc
Copy link
Contributor

Pull Request check list ✔️

  • Commit conforms to CONTRIBUTING.md?
  • Proper tests/regressions included?
  • Documentation updated?

Affected functionality 🤔
The following spire-server federation commands are affected:

  • create
  • delete
  • list
  • refresh
  • show
  • update

Description of change ✍️
Enable output format definition for spire-server federation commands, using cliprinter.

Which issue this PR fixes
Ongoing work for #1354

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
@guilhermocc guilhermocc changed the title Add -output flag support for spire server federation commands Add support for -output flag for spire server federation commands Dec 2, 2022
@guilhermocc guilhermocc changed the title Add support for -output flag for spire server federation commands Add support for -output flag in spire server federation commands Dec 2, 2022
@guilhermocc guilhermocc force-pushed the use-cli-printer-server-federation-commands branch from 5136174 to 51e8a0f Compare December 2, 2022 18:29
Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
return err
}

createResp, ok := results[0].(*trustdomainv1.BatchCreateFederationRelationshipResponse)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it possible that results is empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

theoretically yes, but with the current implementation, we are constantly passing at least one proto message to the printer call (line 68), so currently there is no way to reproduce this scenario

Comment on lines +148 to +149
expErrPretty: "Error: trust domain is required\n",
expErrJSON: "Error: trust domain is required\n",
Copy link
Collaborator

Choose a reason for hiding this comment

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

if the expected value and test code for json and pretty errors are the same, why it is required? (the same comment is for all unit tests for all commands)

Copy link
Contributor Author

@guilhermocc guilhermocc Dec 7, 2022

Choose a reason for hiding this comment

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

So, I've identified two scenarios in which we can have two different behaviors for printing the result when some error occurs:

  • An error can occur before we make an API call, and in that case, the error message will be the same for pretty or json formats, one example is the test case that you highlighted here.
  • An error can occur after the API call, on the server side, and it can come described in the API response. In this case, it wouldn't make sense to print the same error message as pretty format, since we have a structured API response, and this error indication is contained in it, so for json format option, it makes more sense to continue representing the server response with json. You can find a test case example on line 319 (Federation relationships that failed to be created are printed).

Copy link
Collaborator

Choose a reason for hiding this comment

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

But if that is the case, are not we returning inconsistent errors? |
since in some cases (in this example only one) we'll be able get text but in another ones we'll get a json

I think we must be consistent, and if we expect json return json. or just return always text

Copy link
Contributor Author

@guilhermocc guilhermocc Dec 9, 2022

Choose a reason for hiding this comment

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

So, I understand the point in which it can be seen as inconsistent since we may not return a json output for all possible scenarios, the thing is, I've based myself on the first PR opened for using the cliprinter in the fetch jwt token command, and as we can see, we were only formatting the output when we actually get a result for the provided action the command does (after a successful API call). At first look at this behavior, it did not seem inconsistent to me, and one of the reasons for that is that we already have this same behavior in other broadly used CLI tools like kubectl, aws-cli, or gcloud:

gcloud
aws
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We've brought this subject to discussion in our last contributor sync; taking into account the examples provided here and the fact that we have services that include RPCs with multiple response statuses (in batch), we have agreed on the following:

  • Print errors in text format
  • Print the API response in the requested format, regardless of whether it includes a message with status different than OK.

Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks @guilhermocc

I may add the further clarification that, any API response for which the request response code is OK, the payload of the response should be printed in the format requested, even if the response payload itself includes gRPC codes other than OK.

Any API response code that is not OK, or any error encountered while issuing the request or reading the response, should be printed in pretty print format.

cmd/spire-server/cli/federation/create_test.go Outdated Show resolved Hide resolved
"https_spiffe": {
"endpoint_spiffe_id": "spiffe://other.org/bundle"
},
"trust_domain_bundle": null
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure about including null fields

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, so initially, in the cliprinter package, we were using a configuration for marshalling proto messages into json that did not emit unpopulated fields, which was causing some fields with primitive types with zeroed values to not be included in the json (Count: 0, or SomeBool: false are some examples of message fields that weren't being included in the json). We then updated it to use the EmitUnpopulated config from the lib we are using, and according to its documentation it starts to include all kinds of unpopulated fields in the json.

I see that we could reduce the "noise" in our json responses by omitting unpopulated pointer fields, but since it will involve some custom code and probably an update in other tests that are using the cliprinter, I would prefer to make this improvement in another PR, what do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah a separated one makes sense to me

return fmt.Errorf("failed to delete federation relationship %q: %s", result.TrustDomain, result.Status.Message)
func prettyPrintDelete(env *commoncli.Env, results ...interface{}) error {
if deleteResp, ok := results[0].(*trustdomain.BatchDeleteFederationRelationshipResponse); ok {
result := deleteResp.Results[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

may we validate response size?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, we should validate it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

expectOut: "Bundle refreshed\n",
refreshResp: &emptypb.Empty{},
expectOutPretty: "Bundle refreshed\n",
expectOutJSON: "{}",
Copy link
Collaborator

Choose a reason for hiding this comment

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

mmm not sure about this,
empty json for a success sounds strange to me...
an options is to return a types.Status

Copy link
Contributor Author

@guilhermocc guilhermocc Dec 7, 2022

Choose a reason for hiding this comment

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

Yeah, so initially, I thought about mirroring the json output structure with the proto message definition for all commands, in a way that users already have documentation of what kind of data to expect by running each of the commands. Since this API call returns an *emptypb.Empty type, the final result was an empty json. Nevertheless, for this specific command, I don't see a problem of not mirroring the json response with the proto message, adding a status here would sure make things more clear, increasing usability, having in mind that this is our final objective, I will proceed with the update 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 67 to 70
federationRelationships, err := getRelationships(c.config, c.path)
if err != nil {
return err
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

relationships are getted twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ops, I just let it pass, we need these relationships for building the request and them for composing the result in pretty format, im going to update it to remove redundant calls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines +50 to +51
-output value
Desired output format (pretty, json)
Copy link
Collaborator

Choose a reason for hiding this comment

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

why it does not have a default value? generally we put a default when you have options (the same comment for all commands)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It already has a default value (pretty) but we are not documenting it here, going to update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: updating this description message will result in updating other commands tests, I will update it in an isolated PR

…ction calls

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Copy link
Collaborator

@MarcosDY MarcosDY left a comment

Choose a reason for hiding this comment

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

LGTM!

@MarcosDY MarcosDY merged commit 5b6f29e into spiffe:main Dec 20, 2022
divaspathak pushed a commit to divaspathak/spire that referenced this pull request Dec 24, 2022
…ffe#3660)

* Add -output flag support for spire server federation commands

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Signed-off-by: divaspathak <divaspathak@gmail.com>
@evan2645 evan2645 added this to the 1.5.4 milestone Dec 27, 2022
stevend-uber pushed a commit to stevend-uber/spire that referenced this pull request Oct 16, 2023
…ffe#3660)

* Add -output flag support for spire server federation commands

Signed-off-by: Guilherme Carvalho <guilhermbrsp@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants