Fixes issue where a CE response is truncated #6758

gab-satchi · 2023-02-15T20:27:42Z

Proposed Changes

🐛 Fixes an issue where a Cloud Event in a response from a sink was truncated to 1024 bytes

Pre-review Checklist

At least 80% unit test coverage
E2E tests for any new behavior
Docs PR for any user-facing impact
Spec PR for any new API feature
Conformance test for any change to the spec

Release Note

🐛 Fixes an issue where a Cloud Event in a response from a sink was truncated to 1024 bytes

Before the changes in #6521, the response was being truncated when an error occurred. That felt wrong as well, so with these changes, response body isn't truncated regardless of status.

codecov · 2023-02-15T20:35:59Z

Codecov Report

Base: 80.47% // Head: 80.47% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (52f5f71) compared to base (f022034).
Patch coverage: 50.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6758      +/-   ##
==========================================
- Coverage   80.47%   80.47%   -0.01%     
==========================================
  Files         236      236              
  Lines       12164    12162       -2     
==========================================
- Hits         9789     9787       -2     
  Misses       1888     1888              
  Partials      487      487

Impacted Files	Coverage Δ
pkg/channel/message_dispatcher.go	`76.65% <50.00%> (-0.21%)`	⬇️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

evankanderson

It would be good to include an e2e broker conformance test that could be used to test the Kafka and RabbitMQ implementations as well.

evankanderson · 2023-02-15T20:43:08Z

pkg/channel/message_dispatcher.go

 			execInfo.ResponseBody = []byte(fmt.Sprintf("dispatch error: %s", err.Error()))
 		} else {
-			execInfo.ResponseBody = body[:readLen]
+			execInfo.ResponseBody = body.Bytes()


Do you still want to truncate the payload here in error cases?

(I suspect the right thing to do may actually be to truncate the overall message, preferring to cut payload over other attributes.)

We don't check if the response is a cloud event in error cases. The payload gets truncated in a later step by a transformer and then put in as a cloud event extension on the original message. It's then sent to the dead letter. So I think the truncation still happens, it's just no longer part of the executeRequest

I'll double check the above and add a conformance test

I'll double check the above and add a conformance test

@gab-satchi Did you add some conformance test for this?

@matzew, Just an e2e test right now. I can add one under here but if we're looking for one that could be reused by other implementations then that seems like a bigger effort.

I'm seeing all the delivery based ones just TODO-ed here

matzew · 2023-02-16T06:01:52Z

response from a sink!

I had a different test, added as e2e, for a "large" incoming request: https://github.com/knative/eventing/pull/6666/files

@gab-satchi wanna add a similar test for the respone?

And looks like we should backport this to 1.8+ (given the referenced PR landed after 1.7 release)

gab-satchi · 2023-02-16T15:49:01Z

Thanks @matzew, added a similar test

gab-satchi · 2023-02-16T20:01:58Z

/retest

matzew · 2023-02-23T09:15:42Z

I'd like to get this, eventually, backported to 1.9 and 1.8 as well, @gab-satchi

thanks for looking at this

/lgtm
/hold b/c of conformance test question

matzew

/lgtm
/approve

knative-prow · 2023-02-27T06:59:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gab-satchi, matzew

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]

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

matzew · 2023-02-27T06:59:18Z

/cherry-pick release-1.9

knative-prow-robot · 2023-02-27T06:59:20Z

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

In response to this:

/cherry-pick release-1.9

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 · 2023-02-27T06:59:26Z

/cherry-pick release-1.8

knative-prow-robot · 2023-02-27T06:59:27Z

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

In response to this:

/cherry-pick release-1.8

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 · 2023-02-27T10:37:56Z

/unhold

knative-prow-robot · 2023-02-27T11:18:15Z

@matzew: new pull request created: #6781

In response to this:

/cherry-pick release-1.9

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 · 2023-02-27T11:18:53Z

@matzew: new pull request created: #6782

In response to this:

/cherry-pick release-1.8

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.

cherry-pick of #6758 ```release-note 🐛 Fixes an issue where a Cloud Event in a response from a sink was truncated to 1024 bytes ```

… (knative#6783) cherry-pick of knative#6758 ```release-note 🐛 Fixes an issue where a Cloud Event in a response from a sink was truncated to 1024 bytes ```

… (knative#6783) (#141) cherry-pick of knative#6758 ```release-note 🐛 Fixes an issue where a Cloud Event in a response from a sink was truncated to 1024 bytes ``` Co-authored-by: Gab Satchi <[email protected]>

## Proposed Changes - 🐛 Fixes an issue where a Cloud Event in a response from a sink was truncated to 1024 bytes ### Pre-review Checklist  - [x] **At least 80% unit test coverage** - [x] **E2E tests** for any new behavior - [x] **Docs PR** for any user-facing impact - [x] **Spec PR** for any new API feature - [x] **Conformance test** for any change to the spec **Release Note** ```release-note 🐛 Fixes an issue where a Cloud Event in a response from a sink was truncated to 1024 bytes ``` Before the changes in knative#6521, the response was being truncated when an error occurred. That felt wrong as well, so with these changes, response body isn't truncated regardless of status.

Fixes issue where a CE response is truncated

aba8ef2

knative-prow bot requested review from aslom and lberk February 15, 2023 20:27

knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Feb 15, 2023

evankanderson reviewed Feb 15, 2023

View reviewed changes

matzew mentioned this pull request Feb 16, 2023

Message content being cut off #6749

Closed

Adds e2e test for long CE response.

52f5f71

knative-prow bot added area/test-and-release Test infrastructure, tests or release 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 Feb 16, 2023

pierDipi requested a review from matzew February 21, 2023 14:25

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

knative-prow bot assigned matzew Feb 23, 2023

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Feb 23, 2023

matzew approved these changes Feb 27, 2023

View reviewed changes

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 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 Feb 27, 2023

knative-prow bot merged commit 52574ce into knative:main Feb 27, 2023

knative-prow-robot mentioned this pull request Feb 27, 2023

[release-1.9] Fixes issue where a CE response is truncated #6781

Closed

knative-prow-robot mentioned this pull request Feb 27, 2023

[release-1.8] Fixes issue where a CE response is truncated #6782

Closed

gab-satchi mentioned this pull request Feb 27, 2023

[release-1.8] Fixes issue where a CE response is truncated #6782 #6783

Merged

This was referenced Feb 28, 2023

[release-1.8] Fixes issue where a CE response is truncated #6782 (#6783) openshift-knative/eventing#141

Merged

[release-1.9] Fixes issue where a CE response is truncated (#6784) openshift-knative/eventing#142

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes issue where a CE response is truncated #6758

Fixes issue where a CE response is truncated #6758

gab-satchi commented Feb 15, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading

evankanderson left a comment

evankanderson Feb 15, 2023

gab-satchi Feb 15, 2023

matzew Feb 23, 2023

gab-satchi Feb 23, 2023

matzew commented Feb 16, 2023

gab-satchi commented Feb 16, 2023

gab-satchi commented Feb 16, 2023

matzew commented Feb 23, 2023

matzew left a comment

knative-prow bot commented Feb 27, 2023

matzew commented Feb 27, 2023

knative-prow-robot commented Feb 27, 2023

matzew commented Feb 27, 2023

knative-prow-robot commented Feb 27, 2023

matzew commented Feb 27, 2023

knative-prow-robot commented Feb 27, 2023

knative-prow-robot commented Feb 27, 2023

Fixes issue where a CE response is truncated #6758

Fixes issue where a CE response is truncated #6758

Conversation

gab-satchi commented Feb 15, 2023 • edited Loading

Proposed Changes

Pre-review Checklist

codecov bot commented Feb 15, 2023 • edited Loading

Codecov Report

evankanderson left a comment

Choose a reason for hiding this comment

evankanderson Feb 15, 2023

Choose a reason for hiding this comment

gab-satchi Feb 15, 2023

Choose a reason for hiding this comment

matzew Feb 23, 2023

Choose a reason for hiding this comment

gab-satchi Feb 23, 2023

Choose a reason for hiding this comment

matzew commented Feb 16, 2023

gab-satchi commented Feb 16, 2023

gab-satchi commented Feb 16, 2023

matzew commented Feb 23, 2023

matzew left a comment

Choose a reason for hiding this comment

knative-prow bot commented Feb 27, 2023

matzew commented Feb 27, 2023

knative-prow-robot commented Feb 27, 2023

matzew commented Feb 27, 2023

knative-prow-robot commented Feb 27, 2023

matzew commented Feb 27, 2023

knative-prow-robot commented Feb 27, 2023

knative-prow-robot commented Feb 27, 2023

gab-satchi commented Feb 15, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading