Ping source should treat "data" as binary

[duglin]

Today it appears that the data is required to be JSON, I'd like to suggest that this be changed and we support any random set of bytes (as long as it can be serialized into the CR's yaml/json). From an end user's point of view they will only see two things... the CR/CLI and the data that is delivered to the sink. There is no reason to for the user (in either of those cases) to deal with JSON. If there's Knative internal reasons to use JSON then we should hide those from the user - but I do not believe this is the case. Imagine a situation where the user is dealing with, say, XML... I do not believe it would be appropriate to require them to install a JSON parser into the sink/Ksvc when they would like to specify data as <doit/>. The CloudEvent delivered to the sink uses the binary format, which means the HTTP Body can be anything - if treat data as just a raw set of bytes then we should be able to support this

/cc @lionelvillard @cr22rc

[lionelvillard]

jsonData (aka data) still accepts non-json payload. When the payload is not JSON, the data is wrapped around a JSON object with one field, body, whose value is jsonData. As you said we should fix that and send the data as is.

So here a plan for v1:

• rename jsonData to data
• send events via binary content mode (no JSON wrapper)
• v1beta1 to v1 conversion webhook does the wrapping for non-JSON data (v1beta1 semantic is preserved)
• proper documentation

[lberk]

/assign @lionelvillard

[n3wscott]

I see, I think this was legacy from when the go sdk was not quite good enough to parse a json string body, and I added the wrapper to help with that issue.

If we are going to go down this path, I recommend:

• not promoting PingSource to v1.
• adding data Base64
• adding data Content Encoding

I wonder if the answer is we do something like:

spec:
  data:
    string: <based on content type, we could auto-convert this to binary or send it as a json string by default>
    json: <original spec.jsonData here, with lionels wrapper pop trick>
    contentType: "application/xml" 
    binary: <base64 data>  

Something like that ^

[cr22rc]

I thought we wanted to move away from any data manipulation?
string Is just what's in the body may it be json, xml, etc.
binary binary is encoded in base64 we only decode it before sending.
json should be handled by string
contentType is just meta data we pass along as the cloud event content type. What's in there we don't care.

[capri-xiyue]

I see, I think this was legacy from when the go sdk was not quite good enough to parse a json string body, and I added the wrapper to help with that issue.

If we are going to go down this path, I recommend:

• not promoting PingSource to v1.
• adding data Base64
• adding data Content Encoding

I wonder if the answer is we do something like:

spec:
  data:
    string: <based on content type, we could auto-convert this to binary or send it as a json string by default>
    json: <original spec.jsonData here, with lionels wrapper pop trick>
    contentType: "application/xml" 
    binary: <base64 data>  

Something like that ^

@n3wscott
For example, if user wants to send PingSource with contentType "application/xml" and specify data as <doit/>,
Should the user specify PingSource look like this,

spec:
 data:
  string:  "<doit/>"
  contentType: "application/xml" 
  binary: "PGRvaXQvPg=="

(PGRvaXQvPg== is the base64 encoded for <doit/>)
And the event user received will look like this:

{
    "datacontenttype" : "application/xml",
    "data" : "<doit/>"
}

Maybe I missed some context.
What's the usage of spec.data.binary here? Or spec.data.binary won't be used in this scenario, spec.data.binary will be used for contentTypes that have not been registered for encoder and decoder in cloudevents sdk-go like application/octet-stream ?

[n3wscott]

(PGRvaXQvPg== is the base64 encoded for )
And the event user received will look like this:

I was pulling from how secrets work, because it sucks to have to create a base64 string as the operator. It would be better to accept a string and then based on content type, interpret what to do with it. See my original comments

[n3wscott]

Something we could look at is how I define cloudevents in the conformance tool: https://github.com/cloudevents/conformance/blob/master/yaml/v1.0/v1_minimum.yaml

Possibly take some of those ideas and use that same shape for the next version of pingsource spec?