Add kafka ce extensions for partition and offset

[Cali0707]

Fixes #3072

Proposed Changes

• Expose kafka partition and offset as new CE extensions

Release Note

Events coming from kafka components now have knativekafkaoffset and knativekafkapartition cloudevents extension attributes set

Docs

[knative-prow]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[Cali0707]

/test all

[Cali0707]

@matzew @pierDipi it looks like a lot of tests are failing because they don't expect the event to have the two new extensions I've added. Since the issue doesn't specify that this needs to be an CE extension, I'm wondering - should I switch my implementation to just set a header (and not a CE extension)?

I personally think the CE extension approach is better since it will let users write filters for different conditions if they want to on their triggers. However, I'd rather check first before I fix all the tests :)

[pierDipi]

@Cali0707 the spec says this https://github.com/knative/specs/blob/main/specs/eventing/data-plane.md#event-acknowledgement-and-delivery-retry:

In general, senders MAY add or update other CloudEvent attributes on each delivery attempt; see observability for an example case.

so tests should not fail when attributes are added

[pierDipi]

Also we can ask the author of the issue to give us feedback as the exit criteria on the issue may be ambiguous as we're using binary content mode to send events over HTTP, so CE attributes are headers

The incoming HTTP request contains the offset and partition information.

[Cali0707]

@Cali0707 the spec says this https://github.com/knative/specs/blob/main/specs/eventing/data-plane.md#event-acknowledgement-and-delivery-retry:

In general, senders MAY add or update other CloudEvent attributes on each delivery attempt; see observability for an example case.

so tests should not fail when attributes are added

I think the issue is the tests are expecting a specific cloud event currently, which we aren't matching?

For example:

     topology.go:179: unexpected event routing behaviour (-want, +got) =   []event.Event{
          	{
          		Mode: "",
          		Attributes: event.ContextAttributes{
          			... // 1 ignored and 7 identical fields
          			DataContentEncoding: "",
          			DataContentType:     "text/json",
          			Extensions: event.Extensions{
          				... // 4 identical entries
          				"extime":                "2020-03-21T12:34:56.78Z",
          				"exurl":                 "http://example.com/source",
        + 				"knativekafkaoffset":    "0",
        + 				"knativekafkapartition": "0",
          			},
          		},
          		TransportExtensions: nil,
          		Data:                `"hello"`,
          	},
          } 

Should I be fixing these @pierDipi ?

[matzew]

well:

        + 				"knativekafkaoffset":    "0",
        + 				"knativekafkapartition": "0",

when we add those to each request, I guess we either need to check they are there, or ignore them (being more lenient on extra headers, which may have a downside)

[Cali0707]

/test all

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bfeb3c3) 62.30% compared to head (59220e8) 62.24%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3464      +/-   ##
============================================
- Coverage     62.30%   62.24%   -0.06%     
- Complexity      843      844       +1     
============================================
  Files           186      186              
  Lines         12588    12579       -9     
  Branches        272      272              
============================================
- Hits           7843     7830      -13     
- Misses         4133     4136       +3     
- Partials        612      613       +1     

Flag	Coverage Δ
java-unittests	74.80% <100.00%> (-0.20%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Cali0707]

/test all

[Cali0707]

/test reconciler-tests

[matzew]

can we make those private static final String ?

[matzew]

I understand the prefix of knative... but wondering if the actual upstream/spec, would want to say something on the metadata in https://github.com/cloudevents/spec/blob/main/cloudevents/bindings/kafka-protocol-binding.md ?

[Cali0707]

I didn't see anything in particular while reading the docs there, I'll ask at the serverless WG meeting tomorrow

[Cali0707]

From today's cloudevents call, we should use our own attribute names prefixed by knative and then document those in our repo

[matzew]

that's fair and thanks for raising this

[matzew]

see knative/eventing#7467

[Cali0707]

/cc @pierDipi
Do you think we can get this into the release?

[Cali0707]

/retest

[pierDipi]

/lgtm
/approve

[knative-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, pierDipi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Cali0707,pierDipi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Cali0707]

@pierDipi do you think this needs a corresponding docs update?

[Cali0707]

/retest

[Cali0707]

/retest

[Cali0707]

/cc @pierDipi @Leo6Leo

could I get an LGTM again after fixing the build issue?

[Cali0707]

/retest

[Leo6Leo]

/lgtm

[Cali0707]

/retest-required

[Cali0707]

/retest