Support different levels validation for JSON reader stream #452

xiekeyang · 2016-11-07T03:15:03Z

As previous discussion among developers, to use selectable strict mode, for validating media type of referenced objects. After enabling strict media types unmatched to OCI will be expected failure.

each object media type in manifest and manifest list should be validated.
add unit test cases for un/strict mode, with expected result.
image-tool repo should introduce the new flag if this PR be landed.
rename previous functions from xxDescendants to xxMediaType, because functions validate all objects media type, not only descendants.

Signed-off-by: xiekeyang xiekeyang@huawei.com

wking · 2016-11-07T21:47:23Z

schema/validator.go

+		if strict {
+			return fmt.Errorf("error: manifest has an unknown media type: %s\n", header.MediaType)
+		}
+		fmt.Printf("warning: manifest has an unknown media type: %s\n", header.MediaType)


In #403 I avoided repeating this string by setting up the error (once) and then either warning about it or returning it depending on strict.

Updated in
6934d41
3da0cd8
a2d0b36

I code it as,

msg := fmt.Sprintf(...) if strict { return fmt.Errorf("error: %s", msg) } fmt.Printf("warning: %s\n", msg)

I honestly like fmt.Sprintf to generate message, because it may be error or warning type.
fmt.Errorf and fmt.Printf can distinguish returning or omitting behavior.

wking · 2016-11-07T21:49:35Z

There's a reasonable amount of overlap between this PR and #403, but I'm happy to rebase #403 if this one lands first.

xiekeyang · 2016-11-08T03:45:30Z

There's a reasonable amount of overlap between this PR and #403, but
I'm happy to rebase #403 if this one lands first.

Ah they have really some overlap. We should resolve the conflict if one landed.

wking · 2016-11-08T04:56:03Z

schema/validator.go

+	if header.MediaType != string(v1.MediaTypeImageManifestList) {
+		msg := fmt.Sprintf("manifest list has an unknown media type: %s", header.MediaType)
+		if strict {
+			return fmt.Errorf("error: %s", msg)


I'm fine with the “warning: ” prefix you have below, but I don't think you need an “error: ” prefix for the error string. You know it's an error because its Go type is error, having “error” in the string payload is redundant. If you want to display these to users with an error prefix (which is fine with me), then prefix the error with “error: ” when you print it to stderr (or wherever). But don't add the prefix here.

Agree with you and fix in 25825f3 7b13896 and 4daf4f7

I have hesitated to this, and then comply that we are usual to wrap error message(body). Actually I like error messages to be concise.

wking · 2016-11-08T04:56:40Z

schema/validator.go

+			if strict {
+				return fmt.Errorf("error: manifest %s has an unknown media type: %s\n", manifest.Digest, manifest.MediaType)
+			}
+			fmt.Printf("warning: manifest %s has an unknown media type: %s\n", manifest.Digest, manifest.MediaType)


You can do the same Sprintf (or whatever) unification here.

Yeah, it is a nit, fix in 25825f3#diff-4a672c41642ea30ebf88bb87685cf72aR119

wking · 2016-11-08T08:57:57Z

schema/manifestlist_test.go

@@ -0,0 +1,106 @@
+// Copyright 2016 The Linux Foundation


This looks like it will conflict with your in-flight #444 (also adding a schema/manifestlist_test.go from scratch).

Right. I would resolve the conflict after any PR landed firstly.
The test cases in these PRs are for different purpose.

I haven't looked into #444 again, but I think manifestlist_test.go needs a success test with OCI-defined mediaTypes for both the root mediaType and the manifests[0].mediaType.

xiekeyang · 2016-11-17T01:59:40Z

@stevvooe @vbatts
This PR is adding strict mode for schema media type, as maintainers suggestion. I think it is ready, PTAL, thanks.

philips

Overall OK but needs docs and removal of random printing.

philips · 2016-11-17T03:49:46Z

schema/validator.go

@@ -46,17 +47,17 @@ func (e ValidationError) Error() string {
 }

 // 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, strict bool) error {


can you please describe what strict does?

I add doc in d9b5499#diff-4a672c41642ea30ebf88bb87685cf72aR49. PTAL.

philips · 2016-11-17T03:50:47Z

schema/validator.go

+			if strict {
+				return fmt.Errorf("%s", msg)
+			}
+			fmt.Printf("warning: %s\n", msg)


erm, I don't think this package should just randomly printf stuff

@philips , here I keep previous non-strict behavior(warning) unchanged. And I think it seems had better to warn customer his media-type is not OCI format, even be accepted.
Or do you have some proposal for this?

I think we should stick to printing warnings for this PR.

In follow-up work we can add a validation argument which can hold a test-harness handle (e.g. a more generic form of the harness *tap.T I mocked up in opencontainers/image-tools@4f0b3c1b). Then these test functions can register errors and warnings without worrying about how the results are being collected or displayed. But we don't need to bite all of that off in one chunk ;).

wking · 2016-11-17T05:34:00Z

schema/validator.go

@@ -46,17 +47,18 @@ func (e ValidationError) Error() string {
 }

 // Validate validates the given reader against the schema of the wrapped media type.
-func (v Validator) Validate(src io.Reader) error {
+// strict: accept the customized media type when value is false, or reject when true.


I think better docs for strict are “require the validated object to only contain content which the OCI defines (or references) in the spec”. More notes on assorted shades of validation strictness in opencontainers/image-tools#66.

@wking I think your words may be good as help message for customer, but I really want to present the design purpose here, as my words.

What design purpose are my words missing? This comment is going to be in user-facing godocs, so I think we need to cover both audiences.

fixed in b68d692#diff-4a672c41642ea30ebf88bb87685cf72aR50

wking · 2016-11-17T05:52:06Z

schema/manifestlist_test.go

+			manifestList: `
+{
+  "schemaVersion": 2,
+  "mediaType": "application/customized+json",


This fails now (since #404), because your example is not a manifest list. I think you want to update this mediaType to application/vnd.oci.image.manifest.list.v1+json, but you should be able to leave the rest of this particular JSON blob alone.

wking · 2016-11-17T05:54:22Z

schema/manifestlist_test.go

+			manifestList: `
+{
+  "schemaVersion": 2,
+  "mediaType": "application/customized+json",


This will fail regardless of strict (since #404). I'm fine leaving a test for that in place (with fail: true), but the test should use strict: false to show that the failure is due to you not passing a manifest-list, and not due to particular validation strictness.

xiekeyang · 2016-11-17T07:07:59Z

@opencontainers/image-spec-maintainers @wking ,
Now validation rule for JSON object is that the root object media-type MUST be OCI format, or will be reject.
But we allow the referenced objects, like manifest in manifest-list, config in manifest, to be not OCI format(non-strict mode).
If we really want that? I remember we had discussed this(#404 (comment)), but I still not fully understand why. WDYT?
I would fix this patch after agreement achieved.

xiekeyang · 2016-11-17T13:53:43Z

@opencontainers/image-spec-maintainers @wking
I fix the patch, in accordance with #404.

Only valid referenced object;
Unit test only for referenced object;

PTAL

wking · 2016-11-17T15:46:41Z

schema/manifest_test.go

+    "size": 1470,
+    "digest": "sha256:c86f7763873b6c0aae22d963bab59b4f5debbed6685761b5951584f6efb0633b"
+  },
+  "layers": []


This is currently illegal (although possibly not validated). See #407, which is trying to make it legal. For the purpose of this test, you should use the usual strictly-valid dummy layer from the other tests.

@wking , but current schema really allows empty layer slice(I've debug it). I think spec also allow empty. I honest dislike to avoid unclear of undetermined stuff in specific patch.
To add more test case for optional layers is another story(IMO).

On Thu, Nov 17, 2016 at 06:00:35PM -0800, xiekeyang wrote:

… but current schema really allows empty layer slice (I've debug it). I think spec also allow empty…

I don't think the spec allows empty layers, based on:

The array MUST have the base image at index 0.

#407, and #318 before it, are attempting to lift that restriction. But I agree that you don't want to get involved in that discussion in this PR. By using the usual single-entry layers array here, you would avoid getting involved.

@wking You are right. Excuse me to misunderstood the previous discussion. I fix it in a183979#diff-6493def706b32045f8d30e7a4b860c46R158.
What you mentioned seems a bug, which I would fix following up.

wking · 2016-11-17T15:49:35Z

schema/spec_test.go

@@ -73,7 +73,7 @@ func validate(t *testing.T, name string) {
 			continue
 		}

-		err = schema.Validator(example.Mediatype).Validate(strings.NewReader(example.Body))
+		err = schema.Validator(example.Mediatype).Validate(strings.NewReader(example.Body), false)


It seems like we should be able to validate spec examples strictly.

Yes, you are right. This test is for spec validation, which MUST be strict OCI format coincidence. false will lead this test case loss effectiveness. fix in b68d692#diff-5b73117945d63fe6253dccb9c210d74fR76.

wking · 2016-11-18T09:35:08Z

The diff from both b68d692 and a183979 looks fine to me, although I think we might want to squash the commits together. As it stands running the test suite on just b68d692 raises: $ make test go test -race -cover github.com/opencontainers/image-spec/schema github.com/opencontainers/image-spec/specs-go github.com/opencontainers/image-spec/specs-go/v1 # github.com/opencontainers/image-spec/schema_test schema/manifest_test.go:118: not enough arguments in call to schema.MediaTypeManifest.Validate FAIL github.com/opencontainers/image-spec/schema [build failed] …

xiekeyang · 2016-11-18T10:00:29Z

squashed in 833391c

wking · 2016-11-18T10:21:51Z

833391c looks good to me :).

stevvooe · 2016-11-21T21:24:02Z

How can we define a validation framework that can support different levels of validation? This PR sub-divides the validation space into media types and not media types. This creates the problem that we now have to define what belongs in strict and what doesn't.

Wouldn't it make sense if we have options to control the suites of validation? For example, to get the "strict" behavior, one might configure this as:

Validate(r, ValidateFields, ValidateMediaTypes)

We could also have explicit sets of validation that meet certain support-levels:

var ValidateV1Strict []Validator{ValidateFields, ValidateMediaTypes}

Validate(r, ValidateV2Strict...)

Without a definition of what "strict" means, we'll be in the position that this will be under definition of the implementation, where if we are clear what is being validated, we can assure users they are in line with the specification.

wking · 2016-11-21T21:38:52Z

On Mon, Nov 21, 2016 at 01:24:03PM -0800, Stephen Day wrote:

This creates the problem that we now have to define what belongs in
strict and what doesn't.

There's a definition here 1. I think we can tighten that defintion
up (maybe on the heels of #432?), but the wording in 1 seems like a
reasonable start (and we can update it if we think of better wording
before #432 gives us somewhere to put formal definitions).

Wouldn't it make sense if we have options to control the suites of validation?…
Validate(r, ValidateFields, ValidateMediaTypes)

I'm fine with this sort of breakdown, but if we want to support it I
think we're want to supply functions:

Validate (everything)
ValidateFields (only validate the fields)
ValidateMediaTypes (only validate the referenced media types)

And it seems orthogonal to how strictly we do each of those
validations. E.g. you can have “uses a field that's not defined in
the spec” passing a relaxed validation (it's legal 2) but failing
strict validation (the extension field doesn't have OCI-defined
semantics). Or you can have “uses a layer media type that's not
defined in the spec” passing relaxed validation (it's legal 3) but
failing strict validation (an application/zip layer doesn't have
OCI-defined semantics).

stevvooe · 2016-11-21T22:30:27Z

@wking Please let the submitter respond to code review comments.

Again, you linked me to something without any context and I have no clue what you mean.

xiekeyang · 2016-11-22T02:49:08Z

@stevvooe I mostly understand and agree your expectation. Let me refactor the PR and re-submit sooner.

xiekeyang · 2016-11-22T08:02:16Z

@stevvooe
With current PR, image tool usage will be like,

$ oci-valid-tool --strict=false customized-manifest.json
Warn: referenced config has an unknown media type: application/vnd.oci.customized.config.v1+json
OK!

$ oci-valid-tool --strict=true customized-manifest.json
Error: referenced config has an unknown media type: application/vnd.oci.customized.config.v1+json
Failed!

Customized json is failed under strict mode, and prompt warning non-strict mode.

I think your proposal want tool to be more flexible, like,

$ oci-valid-tool --level [fields] customized-manifest.json
OK!

$ oci-valid-tool --level [fields,ref-media] customized-manifest.json
Failed!

And you want to involve all possible objects(in future) to level array, right?

However, I still have a question. You said,

This creates the problem that we now have to define what belongs in strict and what doesn't.

Actually I think belongs in strict means all of customized fields image spec mentioned. I assume(not sure) consumer just want to check all customized fields totally, by one strict flag. Do you think consumer really want to set series fields in level flag? If it is some complex to consumer?

xiekeyang · 2016-11-24T09:35:30Z

I refactor patch according to @stevvooe proposal of level usage and function interface. It is done in https://github.com/xiekeyang/image-spec/blob/6b47a182a3b6499df2bead20016d6e4eec7fdb4e/schema/validator.go.

To provide validating function array for different level objects.
To support schema and referenced media type validation.
To allow image tools or consumer to select different validation levels for specific require.
To add unit test cases for new interface.
cc @stevvooe @philips @vbatts PTAL.

xiekeyang · 2016-11-30T01:30:40Z

@stevvooe @philips If the latest commit of 21ef4e8 and d898abe OK? PTAL, thanks a lot!

wking · 2016-12-01T18:31:14Z

schema/validator.go

+
+	for _, manifest := range header.Manifests {
+		if manifest.MediaType != string(v1.MediaTypeImageManifest) {
+			return fmt.Errorf("manifest %s has an unknown media type: %s", manifest.Digest, manifest.MediaType)


This approach loses the strict distinction you had before. I don't see a way to invoke this to error out when a media type is truly invalid (i.e. not compliant with RFC 6838, #448) vs. not necessarily supported by an OCI-compliant implementation (e.g. a non-OCI type). I think we need to restore the strict option you had before so we can make distinctions like that (more on strictness in opencontainers/image-tools#66).

@wking
After former proposal from @stevvooe, I think we have made agreement. Current commit follows up the proposal thought, to use function slice. Consumer can select objects(fields, media type...) at will.

I don't see a way to invoke this to error out when a media type is truly invalid (i.e. not compliant with RFC 6838, #448) vs. not necessarily supported by an OCI-compliant implementation (e.g. a non-OCI type).

Under truly invalid case, JSON schema prompts error.
Under non-OCI type, validator will prompt error only if ref-type(here is media type) function is selected. Or it is validated successfully.
Honestly I'm not very willing roll back the former strict framework, which has been rejected. And we had better to wait @stevvooe's feedback.
cc @stevvooe WDYT?

Under truly invalid case, JSON schema prompts error.

This is not always true. For example, the hostname / UTS-namespace requirement (a case of the general namespace no-tweaking requirement) is not covered by the JSON Schema.

Honestly I'm not very willing roll back the former strict framework, which has been rejected. And we had better to wait @stevvooe's feedback.

Yeah, better to not change anything until we know what the maintainers are looking for.

And to give a JSON Schema limitation for this spec, we don't currently check to see that descriptors don't use the reserved data. And I'm not sure how you'd phrase that in JSON Schema since we have to allow other unrecognized properties.

And to give a JSON Schema limitation for this spec, we don't currently check to see that descriptors don't use the reserved data. And I'm not sure how you'd phrase that in JSON Schema since we have to allow other unrecognized properties.

It didn't, and I don't think it belongs to this patch.

I don't think it belongs to this patch.

I agree that we don't want to cover the reserved data as part of this PR. This PR is about two things:

Broadly, setting a precedent for handling verification for behaviour which images MAY use but which implementations MUST support.

Narrowly, adding optional validation for per-location media types, which are a special case of the broader issue.

So while I have no desire to expand the narrow target, I want to try and solve it in a way that will cover the broader issue. I think the strict option sets a precedent which will scale well as we extend it to other spec statements.

I'm less clear on how the “pass in an array of validator functions” will scale. It seems like it might end up with a separate validator for each spec statement? Or maybe we'll end up with a ValidateOnlyContainsOCIRequiredSettings validator? Or…? It's not clear to me how we're expected to apply this approach to future spec statements.

xiekeyang · 2016-12-06T02:25:28Z

@stevvooe If current patch looks as what you want?

vbatts · 2017-02-15T22:27:16Z

This SGTM. Though @stevvooe recommends this needs a technical proposal on how this kind of feature ties to the requirements of compliance and certification

stevvooe · 2017-02-16T03:34:06Z

@vbatts We can go forward with this approach until we have better requirements.

xiekeyang · 2017-02-16T03:58:58Z

@vbatts @stevvooe got it. I'd go on finishing this pr.

xiekeyang · 2017-02-16T06:34:42Z

@stevvooe I think you agree the current implementation of pr, to present selectable media validating function, which is controlled by function-map. In future we can add more val function for another objects if necessary. Is that right?

xiekeyang · 2017-02-16T06:54:26Z

@stevvooe I'm considering to use interface for Validator functions, but not figure out yet.

vbatts · 2017-02-17T18:44:13Z

Is this needing to block a vote for RC5?

xiekeyang · 2017-02-20T01:59:19Z

According to milestone, it is blocking RC5. If @stevvooe @wking @philips accept its map framework for validation, I can finish update in 2 or 3 days. Or for any other proposal, please put up freely, and I would take more analysis. Thanks a lot!

xiekeyang · 2017-02-20T02:20:36Z

Or we can consider to move this milestone to next release.

xiekeyang · 2017-02-20T10:45:31Z

@opencontainers/image-spec-maintainers I update this, to keep functional map, and add a bug fix commit. PTAL.

wking · 2017-02-20T20:31:16Z

image-layout.md

@@ -134,7 +134,7 @@ The `imageLayoutVersion` value will align with the OCI Image Specification versi

 ### oci-layout Example

-```json
+```json,title=Image%20Layout&valid=enabling&mediatype=oci%2Dlayout


I'm in favor of adding a title, but “Image Layout” is the whole structure, not just the oci-layout file. Can we follow the section headers and call this “oci-layout file” or some such?

What is valid=enabling about?

I don't think we want to invent a one-off media type here. If we want a media type for this file format (e.g. for example validation), I think we should declare a media type for the schema like we do with our other schemas (e.g. here and here for manifests).

Sorry, valid=enabling is a redundant debugging item, should be removed.

I extract this and put it on #579.

wking · 2017-02-20T20:33:58Z

image-layout.md

@@ -162,7 +162,7 @@ Those tags will often be represented in an image-layout repository with matching

 ### Index Example

-```json,title=Image%20Index&mediatype=application/vnd.oci.image.index.v1%2Bjson
+```json


I think we still want to validate this example as an index even if it doesn't occur in image-index.md. And titles seem like they'd always be a good idea. Why are you removing this?

I remove this title as not to valid it. I understand that you want to valid it.
But I think spec_test.go should only valid those who are announced only in itself *.md. So I remove it, for it is just a reference. It is IMO, to be not sure.

wking · 2017-02-20T20:34:43Z

schema/validator.go

@@ -26,14 +26,42 @@ import (
 	"github.com/xeipuuv/gojsonschema"
 )

+type validateRefMediaFunc func(r io.Reader) error
+
+var vlidateRefMediaMap = map[Validator]validateRefMediaFunc{


vlidateRefMediaMap → validateRefMediaMap.

wking · 2017-02-20T20:38:35Z

schema/validator.go

+}
+
+// ValidateFunc provides the type of function to validate specific object.
+type ValidateFunc func(r io.Reader, v Validator) error


If we expect to be applying a series of these to validate a single blob, using an io.Reader seems wasteful. Is there some Go type magic we can use to pass a parsed JSON object through to the validator to let us only deserialize the JSON once?

I didn't find out the better way yet. At the same time, I still think it had better to input io reader stream to. ValidateRefFuncs are public functions. Consumer should be allowed called them directly. Here we provide io reader to customer, who can use them freely, and it allows expend their job in future.

ValidateRefFuncs are public functions.

That doesn't really matter. We could always provide io.Reader and parsed-JSON flavored versions if we figure out how to write a function that takes an already-parsed-from-JSON object.

wking · 2017-02-20T20:40:39Z

schema/validator.go

 // Validator wraps a media type string identifier
 // and implements validation against a JSON schema.
 type Validator string

-type validateDescendantsFunc func(r io.Reader) error
+// Validate validates the given reader against the schema and OCI format defined in spec.


I don't see anything about “the schema” or “OCI format defined in the spec” in your current implementation (3ad5d1b). This function is just “read r into memory and call all of funcList on the content, dying on the first error”.

wking · 2017-02-20T20:44:15Z

schema/validator.go

+		buf, err := ioutil.ReadAll(r)
+		if err != nil {
+			return errors.Wrap(err, "unable to read the document file")
+		}


No need to bother with this, since you're only calling a single f. Just pass your reader along:

if ok { if f == nil { … } return f(r) }

Yes, this should be fixed.

wking · 2017-02-20T20:45:40Z

schema/validator.go

+		return f(bytes.NewReader(buf))
+	}
+
+	return nil


I think we should be raising the same unimplemented error here as you currently have for f == nil. How about:

f, ok := vlidateRefMediaMap[v] if ok && f != nil { return f(r) } return fmt.Errorf("referenced media type validation %q unimplemented", v)

I'm hard to agree for this. Some validations such as descriptor, layout doesn't exist in function map, which make no sense. They should be ignore, but not prompt error.

Some validations such as descriptor, layout doesn't exist in function map, which make no sense.

We can validate descriptors against the JSON Schema. And if you want JSON Schema validation handled elsewhere, we can define a no-op validator for types where we know there are no children. But I think silently ignoring validating for all types that have no validator registered is setting ourselves up for “oops, that wasn't valid, but we forgot to add a validator for $TYPE”.

This establishes function list, to allow consumer to select items for validation. A based validation level is JSON schema, and a optional item is the media-type of their referenced fields. Signed-off-by: xiekeyang <xiekeyang@huawei.com>

xiekeyang · 2017-03-06T11:27:06Z

@stevvooe The stuff under discussion is that the argument variable for ValidRefFuncs should be io.Reader type or json type. I prefer to reader. WDYT?
cc @opencontainers/image-spec-maintainers

xiekeyang · 2017-03-08T01:37:37Z

I'm so regretful that I've make big effort on this, but it can't attract maintainers attention. Although I still think it really bring benefit according to pre discussion, I've to close it.

xiekeyang changed the title ~~strict mode for json object reference~~ strict mode for validating media type of referenced objects in manifest Nov 7, 2016

xiekeyang changed the title ~~strict mode for validating media type of referenced objects in manifest~~ Add strict mode parameter, to validate media types in manifest JSON object Nov 7, 2016

xiekeyang changed the title ~~Add strict mode parameter, to validate media types in manifest JSON object~~ Add strict mode parameter, to validate media types in JSON object Nov 7, 2016

wking reviewed Nov 7, 2016

View reviewed changes

wking reviewed Nov 8, 2016

View reviewed changes

philips suggested changes Nov 17, 2016

View reviewed changes

wking reviewed Nov 17, 2016

View reviewed changes

xiekeyang changed the title ~~Add strict mode parameter, to validate media types in JSON object~~ Support different levels validation for JSON reader stream Nov 24, 2016

wking mentioned this pull request Dec 1, 2016

schema: add checkPlatform #469

Closed

wking reviewed Dec 1, 2016

View reviewed changes

wking mentioned this pull request Jan 3, 2017

Modify the test's config type #510

Merged

wking mentioned this pull request Feb 3, 2017

schema: add checkPlatform #491

Merged

vbatts added this to the v1.0.0-rc5 milestone Feb 15, 2017

vbatts mentioned this pull request Feb 15, 2017

Technical-proposal needed for compliance and certification #573

Open

xiekeyang changed the title ~~Support different levels validation for JSON reader stream~~ [wip] Support different levels validation for JSON reader stream Feb 16, 2017

xiekeyang changed the title ~~[wip] Support different levels validation for JSON reader stream~~ Support different levels validation for JSON reader stream Feb 20, 2017

vbatts modified the milestones: v1.0.0, v1.0.0-rc5 Feb 20, 2017

wking reviewed Feb 20, 2017

View reviewed changes

xiekeyang mentioned this pull request Feb 22, 2017

fix error valid target in image-layout #579

Merged

xiekeyang closed this Mar 8, 2017

wking mentioned this pull request Mar 13, 2017

Intra-blob JSON validation correctness and image-spec release policy #406

Open

Support different levels validation for JSON reader stream #452

Support different levels validation for JSON reader stream #452

Conversation

xiekeyang commented Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Nov 7, 2016 via email

xiekeyang commented Nov 8, 2016

Choose a reason for hiding this comment

xiekeyang Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang commented Nov 17, 2016

philips left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang commented Nov 17, 2016 • edited Loading

xiekeyang commented Nov 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wking commented Nov 18, 2016 via email

xiekeyang commented Nov 18, 2016

wking commented Nov 18, 2016 via email

stevvooe commented Nov 21, 2016

wking commented Nov 21, 2016

stevvooe commented Nov 21, 2016

xiekeyang commented Nov 22, 2016

xiekeyang commented Nov 22, 2016 • edited Loading

xiekeyang commented Nov 24, 2016

xiekeyang commented Nov 30, 2016

Choose a reason for hiding this comment

xiekeyang Dec 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang commented Dec 6, 2016

vbatts commented Feb 15, 2017

stevvooe commented Feb 16, 2017

xiekeyang commented Feb 16, 2017

xiekeyang commented Feb 16, 2017

xiekeyang commented Feb 16, 2017

vbatts commented Feb 17, 2017

xiekeyang commented Feb 20, 2017

xiekeyang commented Feb 20, 2017

xiekeyang commented Feb 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiekeyang commented Nov 7, 2016 •

edited

Loading

xiekeyang Nov 8, 2016 •

edited

Loading

xiekeyang Nov 8, 2016 •

edited

Loading

xiekeyang commented Nov 17, 2016 •

edited

Loading

xiekeyang commented Nov 22, 2016 •

edited

Loading

xiekeyang Dec 4, 2016 •

edited

Loading