Fix binary events with no data and ce overrides not being delivered #3475

Cali0707 · 2023-11-22T16:04:11Z

Fixes bug where binary events with ce overrides don't get delivered

Proposed Changes

Deserialize events from the kafka record headers if needed

Release Note

:bug: Fixed bug where binary events with no data were not delivered if cloud event overrides were enabled

Docs

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2023-11-22T16:04:36Z

/cc @pierDipi
I'm running just the test first to show the bug, will push my fix in a bit :)

Cali0707 · 2023-11-22T16:05:06Z

/hold
until after the fix gets pushed

Cali0707 · 2023-11-22T16:05:24Z

/cherry-pick release-1.12
/cherry-pick release-1.11

knative-prow-robot · 2023-11-22T16:05:27Z

@Cali0707: once the present PR merges, I will cherry-pick it on top of release-1.12 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.12
/cherry-pick release-1.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

codecov · 2023-11-22T16:09:07Z

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (ee8d67d) 58.46% compared to head (d0076cb) 63.04%.
Report is 50 commits behind head on main.

Files	Patch %	Lines
...tcher/impl/consumer/NullCloudEventInterceptor.java	73.91%	5 Missing and 1 partial ⚠️
...ve/eventing/kafka/broker/dispatcher/main/Main.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3475      +/-   ##
============================================
+ Coverage     58.46%   63.04%   +4.58%     
- Complexity        0      839     +839     
============================================
  Files            91      182      +91     
  Lines          9338    12452    +3114     
  Branches          0      268     +268     
============================================
+ Hits           5459     7850    +2391     
- Misses         3443     4000     +557     
- Partials        436      602     +166

Flag	Coverage Δ
java-unittests	`76.64% <68.00%> (?)`

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 · 2023-11-22T16:20:05Z

/cherry-pick release-v1.11

knative-prow-robot · 2023-11-22T16:20:08Z

@Cali0707: once the present PR merges, I will cherry-pick it on top of release-v1.11 in a new PR and assign it to you.

In response to this:

/cherry-pick release-v1.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2023-11-22T18:21:46Z

/unhold

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2023-11-22T20:13:22Z

/test reconciler-tests-namespaced-broker

Cali0707 · 2023-11-22T22:50:23Z

/test reconciler-tests-namespaced-broker

Cali0707 · 2023-11-23T20:45:44Z

/cc @matzew

Cali0707 · 2023-11-23T20:46:49Z

The bug can be seen with the test failure from before I pushed the fix here

The test is now passing with the fix

pierDipi · 2023-11-24T07:00:01Z

...a/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/CloudEventOverridesMutator.java

Can we add a few unit test as well?

A good scenario to test is a chain RecordDispatcherMutatorChain similar to the one we use in reality

final var recordDispatcher = new RecordDispatcherMutatorChain( new RecordDispatcherImpl( consumerVerticleContext, getFilter(), egressSubscriberSender, egressDeadLetterSender, responseHandler, offsetManager, ConsumerTracer.create( ((VertxInternal) vertx).tracer(), consumerVerticleContext.getConsumerConfigs(), TracingPolicy.PROPAGATE), Metrics.getRegistry()), new CloudEventOverridesMutator( consumerVerticleContext.getResource().getCloudEventOverrides()));

passing an InvalidCloudEvent object

pierDipi · 2023-11-24T07:10:07Z

...a/dev/knative/eventing/kafka/broker/dispatcher/impl/consumer/CloudEventOverridesMutator.java

+    private CloudEvent maybeDeserializeFromHeaders(ConsumerRecord<Object, CloudEvent> record) {
+        if (record.value() != null) {
+            return record.value();
+        }
+        // A valid CloudEvent in the CE binary protocol binding of Kafka
+        // might be composed by only Headers.
+        //
+        // KafkaConsumer doesn't call the deserializer if the value
+        // is null.
+        //
+        // That means that we get a record with a null value and some CE
+        // headers even though the record is a valid CloudEvent.
+        return cloudEventDeserializer.deserialize(record.topic(), record.headers(), null);
+    }


This logic is duplicated from RecordDispatcherImpl, can we extract a common method, usable by both CloudEventOverridesMutator and RecordDispatcherImpl

Signed-off-by: Calum Murray <cmurray@redhat.com>

Cali0707 · 2024-01-05T19:29:03Z

/cc @pierDipi

I've added unit tests now, and fixed the order of the interceptors (you were correct)

pierDipi

/lgtm
/approve

Thanks!

knative-prow · 2024-01-08T15:39:42Z

[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

pierDipi · 2024-01-08T15:39:44Z

/retest-required

Cali0707 · 2024-01-08T18:24:43Z

/test reconciler-tests

Cali0707 · 2024-01-08T19:43:01Z

/test reconciler-tests

Cali0707 · 2024-01-08T21:44:11Z

/retest-required

Cali0707 · 2024-01-08T22:23:48Z

/test reconciler-tests-namespaced-broker

knative-prow-robot · 2024-01-09T17:37:35Z

@Cali0707: new pull request created: #3579

In response to this:

/cherry-pick release-1.12
/cherry-pick release-1.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2024-01-09T17:38:11Z

@Cali0707: new pull request created: #3580

In response to this:

/cherry-pick release-1.12
/cherry-pick release-1.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

knative-prow-robot · 2024-01-09T17:38:45Z

@Cali0707: cannot checkout release-v1.11: error checking out "release-v1.11": exit status 1 error: pathspec 'release-v1.11' did not match any file(s) known to git

In response to this:

/cherry-pick release-v1.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Cali0707 · 2024-01-09T20:19:06Z

/cherry-pick release-1.11

knative-prow-robot · 2024-01-09T20:19:43Z

@Cali0707: new pull request could not be created: failed to create pull request against knative-extensions/eventing-kafka-broker#release-1.11 from head knative-prow-robot:cherry-pick-3475-to-release-1.11: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for knative-prow-robot:cherry-pick-3475-to-release-1.11."}],"documentation_url":"https://docs.github.com/rest/pulls/pulls#create-a-pull-request"}

In response to this:

/cherry-pick release-1.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…native-extensions#3475) * Added regression test for binary events with extensions Signed-off-by: Calum Murray <cmurray@redhat.com> * deserialize the event from the headers if the record value is null Signed-off-by: Calum Murray <cmurray@redhat.com> * update codegen Signed-off-by: Calum Murray <cmurray@redhat.com> * use an interceptor instead of handling it in the mutatorchain Signed-off-by: Calum Murray <cmurray@redhat.com> * added unit test for interceptor chain Signed-off-by: Calum Murray <cmurray@redhat.com> * fixed order of interceptors Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com>

…ing delivered (#3475) (#3595) * Fix binary events with no data and ce overrides not being delivered (#3475) * Added regression test for binary events with extensions Signed-off-by: Calum Murray <cmurray@redhat.com> * deserialize the event from the headers if the record value is null Signed-off-by: Calum Murray <cmurray@redhat.com> * update codegen Signed-off-by: Calum Murray <cmurray@redhat.com> * use an interceptor instead of handling it in the mutatorchain Signed-off-by: Calum Murray <cmurray@redhat.com> * added unit test for interceptor chain Signed-off-by: Calum Murray <cmurray@redhat.com> * fixed order of interceptors Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com> * fix InterceptorChainTest build issues Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com>

…native-extensions#3475) * Added regression test for binary events with extensions Signed-off-by: Calum Murray <cmurray@redhat.com> * deserialize the event from the headers if the record value is null Signed-off-by: Calum Murray <cmurray@redhat.com> * update codegen Signed-off-by: Calum Murray <cmurray@redhat.com> * use an interceptor instead of handling it in the mutatorchain Signed-off-by: Calum Murray <cmurray@redhat.com> * added unit test for interceptor chain Signed-off-by: Calum Murray <cmurray@redhat.com> * fixed order of interceptors Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com>

…eing delivered (#972) * Fix binary events with no data and ce overrides not being delivered (knative-extensions#3475) * Added regression test for binary events with extensions Signed-off-by: Calum Murray <cmurray@redhat.com> * deserialize the event from the headers if the record value is null Signed-off-by: Calum Murray <cmurray@redhat.com> * update codegen Signed-off-by: Calum Murray <cmurray@redhat.com> * use an interceptor instead of handling it in the mutatorchain Signed-off-by: Calum Murray <cmurray@redhat.com> * added unit test for interceptor chain Signed-off-by: Calum Murray <cmurray@redhat.com> * fixed order of interceptors Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com> * fix: make InterceptorChain class static to fix compiler error Signed-off-by: Calum Murray <cmurray@redhat.com> --------- Signed-off-by: Calum Murray <cmurray@redhat.com>

Added regression test for binary events with extensions

ec634bd

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/test labels Nov 22, 2023

knative-prow bot requested review from creydr and Leo6Leo November 22, 2023 16:04

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 22, 2023

knative-prow bot requested a review from pierDipi November 22, 2023 16:04

knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2023

Cali0707 changed the title ~~Added regression test for binary events with extensions~~ Fix binary events with no data and ce overrides not being delivered Nov 22, 2023

deserialize the event from the headers if the record value is null

e41a49e

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot added the area/data-plane label Nov 22, 2023

knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 22, 2023

update codegen

3fab071

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot requested a review from matzew November 23, 2023 20:45

pierDipi reviewed Nov 24, 2023

View reviewed changes

use an interceptor instead of handling it in the mutatorchain

1ef7431

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 24, 2023

fixed order of interceptors

d0076cb

Signed-off-by: Calum Murray <cmurray@redhat.com>

knative-prow bot requested a review from pierDipi January 5, 2024 19:29

pierDipi approved these changes Jan 8, 2024

View reviewed changes

knative-prow bot assigned pierDipi Jan 8, 2024

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jan 8, 2024

knative-prow bot merged commit 679fbe8 into knative-extensions:main Jan 9, 2024
37 checks passed

knative-prow-robot mentioned this pull request Jan 9, 2024

[release-1.12] Fix binary events with no data and ce overrides not being delivered #3579

Closed

knative-prow-robot mentioned this pull request Jan 9, 2024

[release-1.11] Fix binary events with no data and ce overrides not being delivered #3580

Closed

Cali0707 mentioned this pull request Jan 11, 2024

[release-1.12] Fix binary events with no data and ce overrides not being delivered (#3475) #3595

Merged

Cali0707 mentioned this pull request Jan 11, 2024

[release-1.11] Fix binary events with no data and ce overrides not being delivered (#3475) #3596

Closed

Cali0707 mentioned this pull request Jan 29, 2024

[release-v1.12] Fix binary events with no data and ce overrides not being delivered openshift-knative/eventing-kafka-broker#971

Closed

Cali0707 mentioned this pull request Jan 29, 2024

[release-v1.11] Fix binary events with no data and ce overrides not being delivered openshift-knative/eventing-kafka-broker#972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix binary events with no data and ce overrides not being delivered #3475

Fix binary events with no data and ce overrides not being delivered #3475

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

knative-prow-robot commented Nov 22, 2023

codecov bot commented Nov 22, 2023 •

edited

Loading

Cali0707 commented Nov 22, 2023

knative-prow-robot commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 23, 2023

Cali0707 commented Nov 23, 2023

pierDipi Nov 24, 2023 •

edited

Loading

pierDipi Nov 24, 2023 •

edited

Loading

pierDipi Nov 24, 2023

Cali0707 commented Jan 5, 2024

pierDipi left a comment

knative-prow bot commented Jan 8, 2024

pierDipi commented Jan 8, 2024

Cali0707 commented Jan 8, 2024

Cali0707 commented Jan 8, 2024

Cali0707 commented Jan 8, 2024

Cali0707 commented Jan 8, 2024

knative-prow-robot commented Jan 9, 2024

knative-prow-robot commented Jan 9, 2024

knative-prow-robot commented Jan 9, 2024

Cali0707 commented Jan 9, 2024

knative-prow-robot commented Jan 9, 2024

Fix binary events with no data and ce overrides not being delivered #3475

Fix binary events with no data and ce overrides not being delivered #3475

Conversation

Cali0707 commented Nov 22, 2023

Proposed Changes

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

knative-prow-robot commented Nov 22, 2023

codecov bot commented Nov 22, 2023 • edited Loading

Codecov Report

Cali0707 commented Nov 22, 2023

knative-prow-robot commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 22, 2023

Cali0707 commented Nov 23, 2023

Cali0707 commented Nov 23, 2023

pierDipi Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

pierDipi Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

pierDipi Nov 24, 2023

Choose a reason for hiding this comment

Cali0707 commented Jan 5, 2024

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Jan 8, 2024

pierDipi commented Jan 8, 2024

Cali0707 commented Jan 8, 2024

Cali0707 commented Jan 8, 2024

Cali0707 commented Jan 8, 2024

Cali0707 commented Jan 8, 2024

knative-prow-robot commented Jan 9, 2024

knative-prow-robot commented Jan 9, 2024

knative-prow-robot commented Jan 9, 2024

Cali0707 commented Jan 9, 2024

knative-prow-robot commented Jan 9, 2024

codecov bot commented Nov 22, 2023 •

edited

Loading

pierDipi Nov 24, 2023 •

edited

Loading

pierDipi Nov 24, 2023 •

edited

Loading