Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(data): Make MaxEventSize a service configuration setting #3891

Merged
merged 1 commit into from
Mar 16, 2022

Conversation

jinlinGuan
Copy link
Contributor

@jinlinGuan jinlinGuan commented Mar 3, 2022

close #3237
25000KB (25MB) by default

Signed-off-by: Ginny Guan [email protected]

If your build fails due to your commit message not passing the build checks, please review the guidelines here: https://github.com/edgexfoundry/edgex-go/blob/main/.github/Contributing.md

PR Checklist

Please check if your PR fulfills the following requirements:

  • I am not introducing a breaking change (if you are, flag in conventional commit message with BREAKING CHANGE: describing the break)
  • I am not introducing a new dependency (add notes below if you are)
  • I have added unit tests for the new feature or bug fix (if not, why?)
  • I have fully tested (add details below) this the new feature or bug fix (if not, why?)
  • I have opened a PR for the related docs change (if not, why?) will open once this PR approved

Testing Instructions

Build and run Core Data locally with rest of EdgeX non-secure services running in Docker, and

  • set maxEventSize to -1
  • set maxEventSize to 0
  • set maxEventSize to 1 and payload size over 1000 bytes

POST an event
e.g.

curl -ki http://localhost:59880/api/v2/event/Random-Integer-Device/Random-Integer-Device/Int8 -X POST -d '{  "apiVersion": "v2",
  "event": {
    "apiVersion": "v2",
    "deviceName": "Random-Integer-Device",
    "profileName": "Random-Integer-Device",
    "sourceName": "Int8",
    "id": "d5471d59-2810-419a-8744-18eb8fad3d65",
    "origin": 1602168089665565200, 
    "readings": [
      {
        "deviceName": "Random-Integer-Device",
        "resourceName": "Int8",
        "profileName": "Random-Integer-Device",
        "origin": 1602168089665565200,
        "valueType": "Int8",
        "value": "12"
      }
    ]
  }
}'

For message bus,
e.g. curl -ki http://localhost:59900/api/v2/device/name/Random-Binary-Device/Binary\?ds-pushevent\=yes

New Dependency Instructions (If applicable)

@lenny-goodell
Copy link
Member

@jinlinGuan , please remember to complete the PR template when you take this PR out of draft.

@cloudxxx8 cloudxxx8 added the hold Intended for PRs we want to flag for ongoing review label Mar 7, 2022
@cloudxxx8
Copy link
Member

Let's hold this PR and discuss the design here #3237 (comment)

@jinlinGuan jinlinGuan force-pushed the issue-3237 branch 4 times, most recently from fd81e8e to 5ef208f Compare March 11, 2022 07:45
internal/io/event.go Outdated Show resolved Hide resolved
internal/io/event.go Outdated Show resolved Hide resolved
internal/io/event.go Outdated Show resolved Hide resolved
internal/io/event.go Outdated Show resolved Hide resolved
internal/io/event.go Outdated Show resolved Hide resolved
@jinlinGuan jinlinGuan requested a review from cloudxxx8 March 14, 2022 04:57
@jinlinGuan jinlinGuan force-pushed the issue-3237 branch 2 times, most recently from 62e691f to 12daf8a Compare March 14, 2022 05:07
@cloudxxx8 cloudxxx8 requested a review from lenny-goodell March 14, 2022 05:36
@cloudxxx8 cloudxxx8 marked this pull request as ready for review March 14, 2022 05:36
Copy link
Member

@cloudxxx8 cloudxxx8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

internal/io/reader.go Outdated Show resolved Hide resolved
@jinlinGuan jinlinGuan force-pushed the issue-3237 branch 3 times, most recently from 8099421 to dc6b798 Compare March 14, 2022 06:56
cloudxxx8
cloudxxx8 previously approved these changes Mar 14, 2022
Copy link
Member

@cloudxxx8 cloudxxx8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines 78 to 90
dataBytes, readErr := io.ReadAll(r.Body)
if readErr != nil {
err = errors.NewCommonEdgeX(errors.KindIOError, "AddEventRequest I/O reading failed", nil)
} else {
err = utils.CheckPayloadSize(dataBytes, config.MaxEventSize*1000)
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should check r.ContentLength > config.MaxEventSize*1000 before attempting to read the body, otherwise this new settings doesn't really help since we are reading it no matter what.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r.ContentLength is a better option, but why do we limit the check for CBOR only?

Copy link
Member

@cloudxxx8 cloudxxx8 Mar 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lenny-intel Although the request usually contains ContentLength, it's not a required field.
Should we depend on it? It might be unknown.
https://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.4

// ContentLength records the length of the associated content.
// The value -1 indicates that the length is unknown.
// Values >= 0 indicate that the given number of bytes may
// be read from Body.
//
// For client requests, a value of 0 with a non-nil Body is
// also treated as unknown.
ContentLength int64

I suggest to check ContentLength first, and keep the utils.CheckPayloadSize(dataBytes, config.MaxEventSize*1000) step after ReadAll

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, check first, if -1 use utils.CheckPayloadSize

internal/core/data/controller/messaging/subscriber.go Outdated Show resolved Hide resolved
internal/core/data/controller/http/event_test.go Outdated Show resolved Hide resolved
Copy link
Member

@lenny-goodell lenny-goodell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good once address @cloudxxx8 comment on ContentLength

@cloudxxx8 cloudxxx8 removed the hold Intended for PRs we want to flag for ongoing review label Mar 16, 2022
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@cloudxxx8 cloudxxx8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat(data): Make MaxEventSize a service configuration setting
3 participants