Adding a filter for custom error respnse codes to be added as CE extensions #2657

matzew · 2022-09-19T08:55:57Z

Signed-off-by: Matthias Wessendorf [email protected]

Proposed Changes

adding custom header prefix for enhancing failed couldevents when sent to dead letter sink

Release Note

If a fail response contains a custom http header that starts with `kne-`, the prefix will be removed and the rest of the header will be used as the CE extension on the CloudEvent delivered to the dead letter sink.

Docs

matzew · 2022-09-19T08:56:09Z

TODO: will add test(s)

matzew · 2022-09-19T08:57:18Z

...er/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java

@@ -77,6 +78,7 @@ public class RecordDispatcherImpl implements RecordDispatcher {
  private static final String KN_ERROR_DEST_EXT_NAME = "knativeerrordest";
  private static final String KN_ERROR_CODE_EXT_NAME = "knativeerrorcode";
  private static final String KN_ERROR_DATA_EXT_NAME = "knativeerrordata";
+  private static final String EKB_ERROR_PREFIX       = "kne-";


@pierDipi I have no idea what is better kne- as in Knative Error?

Or ekb-, as in "eventing kafka broker", as that is more generic

I like kne-

matzew · 2022-09-19T08:57:39Z

...er/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java

@@ -292,6 +294,13 @@ private ConsumerRecordContext errorTransform(final ConsumerRecordContext recordC
    extensions.put(KN_ERROR_DEST_EXT_NAME, destination);
    extensions.put(KN_ERROR_CODE_EXT_NAME, String.valueOf(response.statusCode()));

+    // transform all headers keys to be lowercase and filter by prefix.
+    // afterwards extract the prefix and add them to the extensions map
+    response.headers().entries().stream().collect(Collectors.toMap(entry -> entry.getKey().toLowerCase(), entry -> entry.getValue()))


works, but perhaps can be written "nicer" ...

This should be much faster with less allocations and IMHO cleaner:

response.headers().forEach((k, v) -> { if (k.regionMatches(true, 0, EKB_ERROR_PREFIX, 0, EKB_ERROR_PREFIX.length())) { // aka startsWithIgnoreCase extensions.put(k.substring(EKB_ERROR_PREFIX.length()).toLowerCase(), v); } });

codecov · 2022-09-19T09:02:39Z

Codecov Report

Merging #2657 (0613635) into main (3470bcd) will increase coverage by 6.21%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #2657      +/-   ##
============================================
+ Coverage     55.92%   62.13%   +6.21%     
- Complexity        0      723     +723     
============================================
  Files            75      148      +73     
  Lines          7864    10406    +2542     
  Branches          0      229     +229     
============================================
+ Hits           4398     6466    +2068     
- Misses         3150     3489     +339     
- Partials        316      451     +135

Flag	Coverage Δ
java-unittests	`81.35% <100.00%> (?)`

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

Impacted Files	Coverage Δ
...a/broker/dispatcher/impl/RecordDispatcherImpl.java	`89.59% <100.00%> (ø)`
...venting/kafka/broker/core/tracing/TracingSpan.java	`90.90% <0.00%> (ø)`
...atcher/impl/filter/subscriptionsapi/NotFilter.java	`100.00% <0.00%> (ø)`
...g/kafka/broker/receiver/impl/ReceiverVerticle.java	`97.87% <0.00%> (ø)`
...cher/impl/filter/subscriptionsapi/CeSqlFilter.java	`100.00% <0.00%> (ø)`
...enting/kafka/broker/core/utils/Configurations.java	`64.51% <0.00%> (ø)`
...ker/core/reconciler/IngressReconcilerListener.java	`50.00% <0.00%> (ø)`
...ve/eventing/kafka/broker/core/metrics/Metrics.java	`76.92% <0.00%> (ø)`
...ative/eventing/kafka/broker/dispatcher/Filter.java	`100.00% <0.00%> (ø)`
... and 64 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

…nsions Signed-off-by: Matthias Wessendorf <[email protected]>

Signed-off-by: Matthias Wessendorf <[email protected]>

pierDipi · 2022-09-19T12:50:19Z

...er/src/main/java/dev/knative/eventing/kafka/broker/dispatcher/impl/RecordDispatcherImpl.java

+    // transform all headers keys to be lowercase and filter by prefix.
+    // afterwards extract the prefix and add them to the extensions map


The comment isn't true anymore.

@pierDipi I updated the comment

Signed-off-by: Matthias Wessendorf <[email protected]>

pierDipi

/lgtm
/approve

knative-prow · 2022-09-19T13:02:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, 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~~ [matzew,pierDipi]

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

matzew · 2022-09-19T13:04:44Z

/cherry-pick release-1.7

knative-prow-robot · 2022-09-19T13:04:45Z

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

In response to this:

/cherry-pick release-1.7

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.

matzew · 2022-09-19T13:04:53Z

/cherry-pick release-1.6

knative-prow-robot · 2022-09-19T13:04:54Z

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

In response to this:

/cherry-pick release-1.6

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.

matzew · 2022-09-19T13:36:03Z

/retest

matzew · 2022-09-19T13:46:10Z

/retest

matzew · 2022-09-19T14:13:28Z

/test integration-tests

matzew · 2022-09-19T14:13:37Z

/test reconciler-tests-namespaced-broker

matzew · 2022-09-19T14:13:49Z

/test channel-integration-tests-sasl-plain

knative-prow-robot · 2022-09-19T15:14:49Z

@matzew: new pull request created: #2659

In response to this:

/cherry-pick release-1.7

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 · 2022-09-19T15:15:27Z

@matzew: new pull request created: #2660

In response to this:

/cherry-pick release-1.6

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.

matzew commented Sep 19, 2022

View reviewed changes

knative-prow bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 19, 2022

matzew changed the title ~~WIP: Adding a filter for custom error respnse codes to be added as CE extensions~~ Adding a filter for custom error respnse codes to be added as CE extensions Sep 19, 2022

knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 19, 2022

matzew force-pushed the custom_error_extension_prefix branch from d2e91c3 to 86d702c Compare September 19, 2022 11:42

matzew mentioned this pull request Sep 19, 2022

[release-v1.4] Custom error extension prefix openshift-knative/eventing-kafka-broker#402

Merged

matzew added 2 commits September 19, 2022 13:47

Adding a filter for custom error respnse codes to be added as CE exte…

6a274a8

…nsions Signed-off-by: Matthias Wessendorf <[email protected]>

Adding simple unit test

9baa960

Signed-off-by: Matthias Wessendorf <[email protected]>

matzew force-pushed the custom_error_extension_prefix branch from 6312452 to 9baa960 Compare September 19, 2022 11:48

Adding rekt test

f284df5

Signed-off-by: Matthias Wessendorf <[email protected]>

knative-prow bot added area/test size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 19, 2022

pierDipi reviewed Sep 19, 2022

View reviewed changes

💄 update comment

0613635

Signed-off-by: Matthias Wessendorf <[email protected]>

pierDipi approved these changes Sep 19, 2022

View reviewed changes

knative-prow bot assigned pierDipi Sep 19, 2022

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2022

knative-prow bot merged commit 7e76988 into knative-extensions:main Sep 19, 2022

knative-prow-robot mentioned this pull request Sep 19, 2022

[release-1.7] Adding a filter for custom error respnse codes to be added as CE extensions #2659

Merged

knative-prow-robot mentioned this pull request Sep 19, 2022

[release-1.6] Adding a filter for custom error respnse codes to be added as CE extensions #2660

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a filter for custom error respnse codes to be added as CE extensions #2657

Adding a filter for custom error respnse codes to be added as CE extensions #2657

matzew commented Sep 19, 2022 •

edited

Loading

matzew commented Sep 19, 2022

matzew Sep 19, 2022 •

edited

Loading

pierDipi Sep 19, 2022

matzew Sep 19, 2022

pierDipi Sep 19, 2022 •

edited

Loading

codecov bot commented Sep 19, 2022 •

edited

Loading

pierDipi Sep 19, 2022

matzew Sep 19, 2022

pierDipi left a comment

knative-prow bot commented Sep 19, 2022

matzew commented Sep 19, 2022

knative-prow-robot commented Sep 19, 2022

matzew commented Sep 19, 2022

knative-prow-robot commented Sep 19, 2022

matzew commented Sep 19, 2022

matzew commented Sep 19, 2022

matzew commented Sep 19, 2022

matzew commented Sep 19, 2022

matzew commented Sep 19, 2022

knative-prow-robot commented Sep 19, 2022

knative-prow-robot commented Sep 19, 2022

		// transform all headers keys to be lowercase and filter by prefix.
		// afterwards extract the prefix and add them to the extensions map

Adding a filter for custom error respnse codes to be added as CE extensions #2657

Adding a filter for custom error respnse codes to be added as CE extensions #2657

Conversation

matzew commented Sep 19, 2022 • edited Loading

Proposed Changes

matzew commented Sep 19, 2022

matzew Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

pierDipi Sep 19, 2022

Choose a reason for hiding this comment

matzew Sep 19, 2022

Choose a reason for hiding this comment

pierDipi Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Sep 19, 2022 • edited Loading

Codecov Report

pierDipi Sep 19, 2022

Choose a reason for hiding this comment

matzew Sep 19, 2022

Choose a reason for hiding this comment

pierDipi left a comment

Choose a reason for hiding this comment

knative-prow bot commented Sep 19, 2022

matzew commented Sep 19, 2022

knative-prow-robot commented Sep 19, 2022

matzew commented Sep 19, 2022

knative-prow-robot commented Sep 19, 2022

matzew commented Sep 19, 2022

matzew commented Sep 19, 2022

matzew commented Sep 19, 2022

matzew commented Sep 19, 2022

matzew commented Sep 19, 2022

knative-prow-robot commented Sep 19, 2022

knative-prow-robot commented Sep 19, 2022

matzew commented Sep 19, 2022 •

edited

Loading

matzew Sep 19, 2022 •

edited

Loading

pierDipi Sep 19, 2022 •

edited

Loading

codecov bot commented Sep 19, 2022 •

edited

Loading