verbose output object digest in validate #234

xiekeyang · 2016-09-01T02:22:25Z

add a verbose option, to output the object digest.
It is for issue #222.
the usage is like,

$ oci-image-tool validate --type config --verbose=true config-sample 
digest: sha256:2b8fd9751c4c0f5dd266fcae00707e67a2545ef34f9a29354585f93dac906749
OK

$ oci-image-tool validate --type config config-sample 
OK

If it make sense, I'd add the verbose to create command.

Signed-off-by: xiekeyang [email protected]

xiekeyang · 2016-09-01T02:57:04Z

ping @s-urbaniak ,@opencontainers/image-spec-maintainers, PTAL thank lots!

runcom · 2016-09-01T07:31:12Z

cmd/oci-image-tool/validate.go


+	cmd.Flags().BoolVar(
+		&v.verbose, "verbose", false,
+		fmt.Sprintf(`output format to validate. If unset, oci-image-tool will only output a OK or failed result.`),


what does it mean "output format to validate*? Should we also have -v?

runcom · 2016-09-01T07:34:43Z

@xiekeyang I tried this and it's only outputting the digest of the config in an imageLayout:

$ oci-image-tool validate --verbose --ref latest /home/amurdaca/src/github.com/projectatomic/skopeo/busybox
digest: sha256:3ca68bfc4a2b07c103848bc0de5ef47816fb06baef62632a2b5320dd2a2f7825
OK

$ tree busybox 
busybox
├── blobs
│   ├── sha256-2b8fd9751c4c0f5dd266fcae00707e67a2545ef34f9a29354585f93dac906749
│   ├── sha256-3ca68bfc4a2b07c103848bc0de5ef47816fb06baef62632a2b5320dd2a2f7825
│   └── sha256-8ddc19f16526912237dd8af81971d5e4dd0587907234be2b83e249518d5b673f
├── oci-layout
└── refs
    └── latest

2 directories, 5 files

$ cat busybox/blobs/sha256-3ca68bfc4a2b07c103848bc0de5ef47816fb06baef62632a2b5320dd2a2f7825
{"schemaVersion":2,"mediaType":"application/vnd.oci.image.manifest.v1+json","config":{"digest":"sha256:2b8fd9751c4c0f5dd266fcae00707e67a2545ef34f9a29354585f93dac906749","mediaType":"application/vnd.oci.image.serialization.config.v1+json","size":1459},"layers":[{"digest":"sha256:8ddc19f16526912237dd8af81971d5e4dd0587907234be2b83e249518d5b673f","mediaType":"application/vnd.oci.image.serialization.rootfs.tar.gzip","size":667590}],"annotations":null}%

I have a branch (runcom/image-spec@oci-image-tool) I'm working on for #222 (which is still WIP) which includes most of the fixes needed there and I though output of digests should have been the output of every object being validated (as opposed to what you did here which just print the digest of the config of an oci image).

My branch also contains fixes for the other point in #222.

runcom · 2016-09-01T07:35:52Z

cmd/oci-image-tool/validate.go

 	defer f.Close()

+	if v.verbose {
+		fd, err := os.Open(name)


this doesn't seem the right place to re-open the file just to get its digest, I would have done this inside each validation func (and as already explained, on every object being validated)

@runcom , @s-urbaniak , here I've some confusion point:

for imageLayout and image type, the digests are calculated and I can collect and output them directly. In these 2 cases, it is meaningful to verbose output the digests which validate the consistency.

However, for manifest, manifestList and config type, I didn't find the digest calculation. Meantime, I think it needn't to output the digest because it just validate the simple file's json schema that is not relevant consistency.

So, my suggestion is just output digests for imageLayout and image type, and output nothing for manifest, manifestList and config type.

What is your proposal? THanks a lot!

xiekeyang · 2016-09-01T08:28:38Z

@runcom Thanks for your review!

@xiekeyang I tried this and it's only outputting the digest of the config in an imageLayout:
I have a branch (runcom/image-spec@oci-image-tool) I'm working on for #222 (which is still WIP) which includes most of the fixes needed there and I though output of digests should have been the output of every object being validated (as opposed to what you did here which just print the digest of the config of an oci image).

Here I made inaccuracy to output only one digest for all that validate-config validate-manifest and validate-imageLayout case.
I agree that it should be improve to output all validated objects digest. I'd fix ASAP.

My branch also contains fixes for the other point in #222.

I read your branch. It seems my commit doesn't seriously conflict to your branch yet. May I fix my commit here, according to your proposal?
Thanks so much.

runcom · 2016-09-01T08:31:27Z

Yes, please go ahead and fix this here :)

xiekeyang · 2016-09-01T11:49:00Z

WIP

xiekeyang · 2016-09-02T09:36:02Z

@runcom , @s-urbaniak , here I've some confusion point:

for imageLayout and image type, the digests are calculated and I can collect and output them directly. In these 2 cases, it is meaningful to verbose output the digests which validate the consistency.

However, for manifest, manifestList and config type, I didn't find the digest calculation. Meantime, I think it needn't to output the digest because it just validate the simple file's json schema that is not relevant consistency.

So, my suggestion is just output digests for imageLayout and image type, and output nothing for manifest, manifestList and config type.

What is your proposal? Thanks a lot!

runcom · 2016-09-02T09:38:02Z

It you hash a manifest, manifest list or config you get their digests, why not calculate and print them? @s-urbaniak wdyt?

xiekeyang · 2016-09-02T09:46:47Z

@runcom @s-urbaniak
I think that just to validate their json schema, not hash consistency, so it seems no need to calculate and print digest. It is just my opinion, which I don' persist.
Anyway, hopefully we will find an agreement here and then I would finish the functionality.

xiekeyang · 2016-09-02T10:08:28Z

Additional, for manifest, manifest list or config case, the digest doesn't exist yet, which we should calculate for only print purpose. Is that a less meaningful way and will impact the program performance? right?

s-urbaniak · 2016-09-05T13:49:46Z

cmd/oci-image-tool/validate.go

 	if typ == "" {
 		if typ, err = autodetect(name); err != nil {
-			return errors.Wrap(err, "unable to determine type")
+			return []string{}, errors.Wrap(err, "unable to determine type")


return nil, errors.Wrap...

s-urbaniak · 2016-09-05T13:53:04Z

@xiekeyang @runcom verbose mode could imply additional computational overhead, I'd be fine with also printing the digests if being in verbose mode, so users have more insight.

@xiekeyang The return []string{}, errors.Wrap(...) pattern should be replaced with return nil, errors.Wrap(...)

xiekeyang · 2016-09-06T02:53:01Z

It is finished, PTAL.
/cc @runcom @s-urbaniak

xiekeyang · 2016-09-07T23:27:55Z

PTAL

xiekeyang · 2016-09-08T23:04:36Z

/cc @runcom @wking

wking · 2016-09-08T23:13:40Z

cmd/oci-image-tool/validate.go

+	cmd.Flags().BoolVarP(
+		&v.verbose, "verbose", "v", false,
+		fmt.Sprintf(`Output verbosely, including all objects digest to be validated. If unset, oci-image-tool will only output a OK or failed result.`),
+	)


Ocitools uses logrus and --log-level (e.g. see opencontainers/runtime-tools#97). I'd recommend we go that route instead of carrying a verbose boolean around with us.

This doesn't really seem to be log output. This is program output.

@stevvooe I also find that log and fmt are both used in program. Do we need to unite them by log, or keep the previous state?

@xiekeyang You don't unite these. Logging and output are different things. Typically, logging provides hints at how a program is doing, whereas the output is the primary purpose of the program.

Under a verbose flag, I would expect that to affect primary program output and not really have anything to do with logs, although this varies in practice.

On Thu, Sep 08, 2016 at 08:56:40PM -0700, Stephen Day wrote:

cmd.Flags().BoolVarP(

&v.verbose, "verbose", "v", false,

fmt.Sprintf(`Output verbosely, including all objects digest to be validated. If unset, oci-image-tool will only output a OK or failed result.`),

)

@xiekeyang You don't unite these. Logging and output are different
things. Typically, logging provides hints at how a program is doing,
whereas the output is the primary purpose of the program.

I agree with this.

Under a verbose flag, I would expect that to affect primary program
output and not really have anything to do with logs, although this
varies in practice.

This is surprising to me. For example, ‘cp -v …’ prints hints about
what the program is doing. Printing:

‘x’ -> ‘y’

is not the primary goal of the command (which is copying the files,
and not about writing anything to stdio). Can you point to an example
where --verbose affects the primary program output?

In this case, the goal of the program is to validate a ref. I expect
the primary purpose of the program is to output those PASS/FAIL test
results (e.g. in TAP 1, although we aren't using TAP here, more's
the pity ;). A --verbose or --log-level option should have no effect
on that test output. What it should do is adjust the log verbosity,
so we get information about the tests as they're being carried out.

This PR (as it stands with 4badd20) is sitting somewhere in the
middle. If the digest being validated is a log output (which I think
it should be), it should be covered by --verbose or --log-level and
printed to stderr when we start validating it (4badd20 collects
digests and prints them after the test passes). If the digest being
validated is part of the primary program output, we don't need
--verbose or --log-level, and should be printing it to stdout as a
sub-test of the ref validation. In TAP, that would look like:

TAP version 13
ok 1 - manifest.v1+json sha256:abc…
ok 2 - layer.tar+gzip sha256:def…
ok 3 - ref v1 (sha256:abc…)
ok 4 - manifest.v1+json sha256:123…
ok 5 - layer.tar+gzip sha256:456…
ok 6 - ref v2 (sha256:123…)
1..6

@wking I'm sure you can find exceptions to my generalizations. :)

For our purposes, I would expect log level changes to provide more information about what is happening, while an increase in verbosity would provide richer test output.

On Fri, Sep 09, 2016 at 02:43:03PM -0700, Stephen Day wrote:

… while an increase in verbosity would provide richer test output.

What is “richer test output”? Shouldn't we always run as many tests
as we have code for and leave it to TAP harnesses and such to distill
the data (if the user wants it distilled) 1?

xiekeyang · 2016-09-09T01:07:02Z

Actually I prefer to logrus. But I thought that will change current image tool's log system, and not sure that logrus is really indispensable. So I didn't put up the proposal.
I think we had better make agreement that if we take logrus or keep go/log.
And I rebase the pr.
/cc @wking @stevvooe @runcom

stevvooe · 2016-09-09T01:12:17Z

schema/validator.go


 // Validate validates the given reader against the schema of the wrapped media type.
-func (v Validator) Validate(src io.Reader) error {
+func (v Validator) Validate(src io.Reader, verbose bool, out *log.Logger) error {


Is there way we can do this without just piling on new arguments? Usually, one can tell they are on the path to spaghetti code when orthogonal arguments show up throughout the call chain.

Validate should only validate.

@stevvooe
What blocked me is that validate functions (ValidateLayout/Validate/(Validator).Validate) (maybe) MUST return new variable of digest list, for these functions calculate digests internally.
If digest list can be added to validate functions when returning, I can utilize them out of validate-function. Otherwise, spaghetti code seems can't work out the problem because of these internal digests variable.
new returning variable is needed.

I assume that you mean

func output(verbose bool, out *logger, validate functype){ // todo }

I mean a returned digests is needed from validate functype.

I was proposing a context like object here #288 (comment) and achieve what @s-urbaniak is saying here #234 (comment) as well

s-urbaniak · 2016-09-09T10:22:35Z

I am proposing classical dependency injection as per https://peter.bourgon.org/go-best-practices-2016/#program-design.

runcom · 2016-09-09T10:23:30Z

I think we had better make agreement that if we take logrus or keep go/log.

I don't see any difference here (please correct me if I'm wrong), both are used to log stuff - and I prefer logrus

xiekeyang · 2016-09-09T10:30:41Z

I think we had better make agreement that if we take logrus or keep go/log.
I don't see any difference here, both are used to log stuff.

They should be different. logrus present level, hook, etc.

runcom · 2016-09-09T10:32:42Z

Different as in what they actually do - which is logging (logrus has the same API as go/log)

xiekeyang · 2016-09-09T10:37:57Z

ping @s-urbaniak @stevvooe @wking @runcom
Let's discuss the best implementation way. And I would try to use it to finish the PR.

xiekeyang · 2016-09-09T11:05:14Z

ping @s-urbaniak @stevvooe @wking @runcom

What blocked me is that validate functions (ValidateLayout/Validate/(Validator).Validate) (maybe) MUST return new variable of digest list, for these functions calculate digests internally.

I think these functions should return the necessary digest list.

wking · 2016-09-09T15:36:03Z

On Fri, Sep 09, 2016 at 04:05:15AM -0700, xiekeyang wrote:

What blocked me is that validate functions
(ValidateLayout/Validate/(Validator).Validate) (maybe) MUST return
new variable of digest list, for these functions calculate digests
internally.

I think these functions should return the necessary digest list.

I think showing the digests of the blobs being fetched/validated
should happen when they are fetched/validated and not delayed until
later. That gets them out as soon as possible to help with
understanding crashes or invalid errors, and means you don't have to
return a list.

I'm waffling on whether this is logging output or program output. I
was previously leaning towards logging 1, but with #288 looking like
a longer slog, I'm shifting to think about these as (sub-)tests. The
TAP examples in [1,2] show how you could display this information as
you work through the validation logic (without collecting it somewhere
for later display). And if we treat these as (sub-)tests, there's no
need to hide them behind a --verbose flag (or similar). Print them
every time, and leave it to the caller to use a TAP harness [3](or
whatever) if they want a more compact view of the results.

runcom · 2016-09-09T15:40:45Z

I agree those are program steps not to be hidden behind a verbose flag @philips @vbatts @stevvooe wdyt

Signed-off-by: xiekeyang <[email protected]>

To add a optional log-level, as to output the object digest. It is to close issue #222. Signed-off-by: xiekeyang <[email protected]>

xiekeyang · 2016-09-11T08:59:20Z

Here I collect guys proposal, to improve the output mechanism.

To use logrus, this patch only introduce it for outputing validated objects's descriptor(media type, digest and size).
I don't refactor other loggers, for that @runcom is taking on the logger refactor, as not to conflict each other.
To use --log-level option, instead of --verbose, as ocitools present: https://github.com/opencontainers/ocitools/blob/master/cmd/ocitools/main.go#L16.
To output the objects's descriptor in validating functions, using debug level.

@stevvooe @s-urbaniak suggested to use spaghetti code or object function. Now I'm not sure if we need to export the descriptor from validation function, so I assume unnecessarily to use it. If you think that descriptor informations is useful out of validation functions, or current output is improper, I would amend the implementation.

Thanks for all of your good proposal, PTAL.

/cc @s-urbaniak @wking @stevvooe @runcom

stevvooe · 2016-09-13T19:38:39Z

@stevvooe @s-urbaniak suggested to use spaghetti code or object function.

:/

I suggested that we avoid spaghetti code.

I'm suggesting that we have an object walker to which you can attach functionality. Such a pattern might look as follows:

WalkImageLayout(imageLayout, func(desc Descriptor, err error) error) {
  if err := validate(imageLayout.Get(desc)); err != nil {
    log.Verbose("error %v", desc)
    return err
  }

  if verbose {
    log.Verbose("validated %v", desc)
  }

  return nil
})

This allows one to visit each item and do a few different things to it, while not overcoupling their implementation.

runcom · 2016-09-13T19:42:25Z

@stevvooe that's exactly the pattern I'm working on

philips · 2016-09-21T02:40:16Z

Closing as this code was moved to https://github.com/opencontainers/image-tools. Please re-open if necessary.

runcom reviewed Sep 1, 2016
View reviewed changes

s-urbaniak reviewed Sep 5, 2016
View reviewed changes

wking reviewed Sep 8, 2016
View reviewed changes

stevvooe reviewed Sep 9, 2016
View reviewed changes

xiekeyang mentioned this pull request Sep 9, 2016

cmd/oci-image-tool: use logrus to ease printing to stdout #288

Closed

xiekeyang added 2 commits September 11, 2016 14:18

introduce logrus package

be5bf56

Signed-off-by: xiekeyang <[email protected]>

verbose output object digest in validate

4ca214a

To add a optional log-level, as to output the object digest. It is to close issue #222. Signed-off-by: xiekeyang <[email protected]>

runcom mentioned this pull request Sep 15, 2016

Provide libraries opencontainers/image-tools#2

Open

philips added the component/oci-image-tool label Sep 21, 2016

philips closed this Sep 21, 2016

philips mentioned this pull request Sep 21, 2016

cmd/oci-image-tool: use logrus to ease printing to stdout opencontainers/image-tools#14

Open

wking mentioned this pull request Sep 22, 2016

validate media type of manifest and its descendants opencontainers/image-tools#10

Closed

wking mentioned this pull request Oct 6, 2016

validate media type of manifest descendants #341

Merged

wking mentioned this pull request Oct 28, 2016

Test Anything Protocol output for image validation opencontainers/image-tools#60

Open

xiekeyang deleted the verbose branch February 20, 2017 10:55

verbose output object digest in validate #234

verbose output object digest in validate #234

Uh oh!

Conversation

xiekeyang commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiekeyang commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom commented Sep 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiekeyang Sep 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiekeyang commented Sep 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

runcom commented Sep 1, 2016

Uh oh!

xiekeyang commented Sep 1, 2016

Uh oh!

xiekeyang commented Sep 2, 2016

Uh oh!

runcom commented Sep 2, 2016

Uh oh!

xiekeyang commented Sep 2, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiekeyang commented Sep 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-urbaniak commented Sep 5, 2016

Uh oh!

xiekeyang commented Sep 6, 2016

Uh oh!

xiekeyang commented Sep 7, 2016

Uh oh!

xiekeyang commented Sep 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiekeyang commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiekeyang Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

runcom Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

s-urbaniak commented Sep 9, 2016

Uh oh!

runcom commented Sep 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiekeyang commented Sep 1, 2016 •

edited

Loading

xiekeyang commented Sep 1, 2016 •

edited

Loading

xiekeyang Sep 2, 2016 •

edited

Loading

xiekeyang commented Sep 1, 2016 •

edited

Loading

xiekeyang commented Sep 2, 2016 •

edited

Loading

xiekeyang commented Sep 9, 2016 •

edited

Loading

xiekeyang Sep 9, 2016 •

edited

Loading

runcom Sep 9, 2016 •

edited

Loading

runcom commented Sep 9, 2016 •

edited

Loading

xiekeyang commented Sep 9, 2016 •

edited

Loading