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

feat: add support for json output for notation verify #527

Merged
merged 24 commits into from
Feb 10, 2023

Conversation

byronchien
Copy link
Contributor

@byronchien byronchien commented Feb 2, 2023

allows json output for notation verify.

fixes notaryproject/roadmap#67, #498

Example output:

chienb@a07817b52895 notation % ./bin/notation verify $IMAGE --output json
{
    "reference": "localhost:5000/net-monitor@sha256:5a07385af4e6b6af81b0ebfd435aedccdfa3507f0609c658209e1aba57159b2b",
    "userMetadata": {
        "foo": "bar"
    },
    "result": "Success"
}

Note: PR is on top of changes that haven't been merged into main yet, so there's duplicate code from this PR adding metadata support.

suggested order for review: notation-go #261 => notation-core-go #111 => notation #527 (this one) =>notation #528

Byron Chien and others added 8 commits January 12, 2023 17:23
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
@byronchien
Copy link
Contributor Author

order to review:
#507
#527
#528

Byron Chien added 3 commits February 2, 2023 09:34
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
cmd/notation/verify.go Outdated Show resolved Hide resolved
specs/commandline/verify.md Outdated Show resolved Hide resolved
cmd/notation/verify.go Outdated Show resolved Hide resolved
cmd/notation/verify.go Outdated Show resolved Hide resolved
cmd/notation/verify.go Outdated Show resolved Hide resolved
Byron Chien added 3 commits February 6, 2023 15:05
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

There is no issue linked with this PR. Could you link at least one issue to this PR so that reviewers can have context?

Since this is a new feature, could you add E2E tests accordingly?

@byronchien
Copy link
Contributor Author

There is no issue linked with this PR. Could you link at least one issue to this PR so that reviewers can have context?

Since this is a new feature, could you add E2E tests accordingly?

linked the related metadata PR and issue in the initial comment. will add E2E tests for this PR

byronchien and others added 2 commits February 7, 2023 16:04
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
byronchien and others added 2 commits February 8, 2023 10:47
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
Signed-off-by: Byron Chien <chienb@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2023

Codecov Report

Merging #527 (79b3217) into main (4f573af) will decrease coverage by 0.15%.
The diff coverage is 3.70%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #527      +/-   ##
==========================================
- Coverage   36.34%   36.20%   -0.15%     
==========================================
  Files          30       30              
  Lines        1607     1616       +9     
==========================================
+ Hits          584      585       +1     
- Misses       1002     1010       +8     
  Partials       21       21              
Impacted Files Coverage Δ
cmd/notation/verify.go 23.20% <3.70%> (-0.94%) ⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shizhMSFT shizhMSFT changed the title Adds support for json output for notation verify feat: add support for json output for notation verify Feb 9, 2023
@shizhMSFT shizhMSFT changed the title feat: add support for json output for notation verify feat: add support for json output for notation verify Feb 9, 2023
Copy link
Contributor

@patrickzheng200 patrickzheng200 left a comment

Choose a reason for hiding this comment

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

LGTM

cmd/notation/verify.go Outdated Show resolved Hide resolved
cmd/notation/verify.go Outdated Show resolved Hide resolved
cmd/notation/verify.go Outdated Show resolved Hide resolved
- fail fast on unknown output format
- print warnings to stderr for both output formats
- omit empty metadata from json response

Signed-off-by: Byron Chien <chienb@amazon.com>
internal/ioutil/print.go Outdated Show resolved Hide resolved
internal/ioutil/print.go Outdated Show resolved Hide resolved
internal/ioutil/print.go Outdated Show resolved Hide resolved
cmd/notation/verify.go Outdated Show resolved Hide resolved
"userMetadata": {
"io.wabbit-networks.buildId": "123"
},
"result": "Success"
Copy link
Contributor

Choose a reason for hiding this comment

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

What if the verification fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for verification failure, no json is written to stdout, and the failure is logged to stderr.

Copy link
Contributor

Choose a reason for hiding this comment

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

@byronchien If JSON is printed only if the verification passes, what's the meaning of showing "result": "Success"?

/cc @priteshbandi @yizha1

Copy link
Contributor

Choose a reason for hiding this comment

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

When verification fails, I expect that the result would be failure or failed, and the failure reason will be included in the JSON object so that other scripts can parse it correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this PR is merged, I've created #546 to track.

Copy link
Contributor

@priteshbandi priteshbandi Feb 10, 2023

Choose a reason for hiding this comment

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

result can have two values, one is skip and other one is success. IMO for failure case displaying JSON for wont be useful because it wont contain any useful information which automation/script can use(non-zero exit code should suffice to show failure)

Copy link
Contributor

Choose a reason for hiding this comment

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

The JSON output is usually consumed by scripts or programs. How can them obtain the structured error message and detailed verification outcomes?

Copy link
Contributor

@priteshbandi priteshbandi Feb 10, 2023

Choose a reason for hiding this comment

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

To display error message/detailed verification output we need more than result field something like resultReason because result will only say failure. Also, in current state it wont be of much help since as there is only one genuine/expected failure use case i.e failed signature verification for all the signatures. Apart from genuine/expected error, all other errors should always be emitted as stdErr.

Are you suggest we should emit json for expected failure use cases or all failure use cases?


Moved conversation to #546 (comment)

byronchien and others added 2 commits February 9, 2023 09:58
Signed-off-by: Byron Chien <chienb@amazon.com>
- rename PrintObjectAsJson => PrintObjectAsJSON
- move output format constants to flags.go
- use switch for verify output behavior
- add documentation output methods
- call out failure behavior in spec

Signed-off-by: Byron Chien <chienb@amazon.com>
@priteshbandi priteshbandi merged commit 33c2281 into notaryproject:main Feb 10, 2023
priteshbandi pushed a commit to priteshbandi/notation that referenced this pull request Feb 15, 2023
priteshbandi pushed a commit to priteshbandi/notation that referenced this pull request Feb 15, 2023
…ryproject#527)"

This reverts commit 33c2281.

Signed-off-by: Pritesh Bandi <pritesb@amazon.com>
priteshbandi pushed a commit to priteshbandi/notation that referenced this pull request Feb 15, 2023
…ryproject#527)"

This reverts commit 33c2281.

Signed-off-by: Pritesh Bandi <pritesb@amazon.com>
priteshbandi pushed a commit to priteshbandi/notation that referenced this pull request Feb 15, 2023
…ryproject#527)"

This reverts commit 33c2281.

Signed-off-by: Pritesh Bandi <priteshbandi@gmail.com>
priteshbandi added a commit that referenced this pull request Feb 15, 2023
…" (#551)

This reverts commit 33c2281.

We are reverting #527 because we need to write spec first for json output.

Signed-off-by: Pritesh Bandi <priteshbandi@gmail.com>
duffney pushed a commit to duffney/notation that referenced this pull request Mar 30, 2023
…ryproject#527)" (notaryproject#551)

This reverts commit 33c2281.

We are reverting notaryproject#527 because we need to write spec first for json output.

Signed-off-by: Pritesh Bandi <priteshbandi@gmail.com>
Signed-off-by: Josh Duffney <jduffney@microsoft.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
6 participants