fix: Increase default MQTT payload size to 256 MiB by albinsuresh · Pull Request #3130 · thin-edge/thin-edge.io

albinsuresh · 2024-09-23T18:57:06Z

Proposed changes

Increase default MQTT payload size to 256 MiB
Make MQTT cloud payload size configurable

Types of changes

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
Documentation Update (if none of the other choices apply)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

#3124

Checklist

I have read the CONTRIBUTING doc
I have signed the CLA (in all commits with git commit -s)
I ran cargo fmt as mentioned in CODING_GUIDELINES
I used cargo clippy as mentioned in CODING_GUIDELINES
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

Further comments

codecov · 2024-09-23T19:07:54Z

Codecov Report

Attention: Patch coverage is 83.23699% with 29 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...on/tedge_config/src/tedge_config_cli/models/mod.rs	75.86%	1 Missing and 6 partials ⚠️
.../tedge_config/src/tedge_config_cli/tedge_config.rs	75.86%	0 Missing and 7 partials ⚠️
crates/common/mqtt_channel/src/tests.rs	80.64%	0 Missing and 6 partials ⚠️
crates/common/mqtt_channel/src/connection.rs	50.00%	4 Missing and 1 partial ⚠️
crates/extensions/c8y_mapper_ext/src/config.rs	60.00%	2 Missing ⚠️
crates/core/tedge_mapper/src/aws/mapper.rs	0.00%	1 Missing ⚠️
crates/core/tedge_mapper/src/az/mapper.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

📢 Thoughts on this report? Let us know!

reubenmiller · 2024-09-23T19:10:39Z

crates/core/tedge_mapper/src/aws/mapper.rs

            mqtt_schema,
            tedge_config.aws.mapper.timestamp_format,
            prefix.clone(),
+            tedge_config.az.mapper.mqtt.max_payload_size,


Copy/paste typo

Suggested change

tedge_config.az.mapper.mqtt.max_payload_size,

tedge_config.aws.mapper.mqtt.max_payload_size,

github-actions · 2024-09-23T19:22:12Z

Robot Results

✅ Passed	❌ Failed	⏭️ Skipped	Total	Pass %	⏱️ Duration
506	0	2	506	100	1h28m58.305465999s

didier-wenzek

The issue is addressed by the proposed fix. However, this introduces a weakness, and I would consider to limit the size of the messages actually processed by thin-edge, ignoring those too large.

didier-wenzek · 2024-09-24T07:37:08Z

crates/common/mqtt_channel/src/config.rs

-            max_packet_size: 1024 * 1024,
+            max_packet_size: MAX_PACKET_SIZE,


Looking closer to root cause of an infinite loop caused by a message that is too large, it appears that the issue is not rooted in the MQTT protocol per se, but is due to a bad combination of rumqttc and thin-edge.

when a message that is too large is received, rumqttc raises a PayloadSizeLimitExceeded error

when this error is received thin-edge do nothing beyond logging and adding some delay (which is useless)

rumqttc also cuts the connection and then reconnects.

the message is then resent by the broker that is still expecting an acknowledgement.

Unfortunately, the rumqttc API provides no means to skip that poison pill (the error structs does not contain the message pkid).

=> Removing the limit as done here fixes the issue. However, we can consider to add our own check with no limit on what is received from rumqttc but with a filter implemented by mqtt_channel to prune messages that are too large. Without this limit a rogue client might overwhelmed the mapper or the agent.

Resolved a4a6af9

rina23q

One suggestion

rina23q · 2024-09-24T13:50:24Z

crates/common/tedge_config/src/tedge_config_cli/tedge_config.rs

+            mqtt: {
+                /// The maximum message payload size that can be mapped to the cloud
+                #[tedge_config(example = "8192", default(variable = "C8Y_MQTT_PAYLOAD_LIMIT"))]
+                max_payload_size: u32,


The maximum size of u32 is much bigger than 256MiB. (4294967295 > 268435455). I think we need protection for such big input from user.

Resolved by f32c709

rina23q · 2024-09-24T13:51:54Z

crates/core/tedge_mapper/src/aws/mapper.rs

            mqtt_schema,
            tedge_config.aws.mapper.timestamp_format,
            prefix.clone(),
+            tedge_config.aws.mapper.mqtt.max_payload_size,


Related to my comment above, for example, here we can add the size check for aws.mapper.mqtt.max_payload_size

Implemented the validation at the tedge_config level itself, so that the user gets the feedback much earlier.

didier-wenzek

The code is correct. I would prefer to revert the default max size to the previous values.

didier-wenzek · 2024-09-25T13:58:47Z

crates/common/mqtt_channel/src/config.rs

    /// Maximum size for a message payload
    ///
-    /// Default: `1024 * 1024`.
+    /// Default: `268435455` (256 MB).


I would not change the default, now that the correct protection is implemented.

Suggested change

/// Default: `268435455` (256 MB).

/// Default: `1024 * 1024`.

Resolved by f43495e

crates/core/tedge/src/cli/mqtt/mod.rs

didier-wenzek

Approved

didier-wenzek · 2024-09-26T13:26:34Z

crates/common/tedge_config/src/tedge_config_cli/models/mod.rs

+    }
+}
+
+impl TryFrom<u32> for MqttPayloadLimit {


I wouldn't have gone so far. But since this has been done, we can keep it.

albinsuresh requested review from didier-wenzek, jarhodes314 and rina23q as code owners September 23, 2024 18:57

albinsuresh temporarily deployed to Test Pull Request September 23, 2024 18:57 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto September 23, 2024 19:05 — with GitHub Actions Inactive

reubenmiller reviewed Sep 23, 2024

View reviewed changes

albinsuresh force-pushed the fix/3124/increase-default-mqtt-message-size branch from 6b0ae87 to 59a1618 Compare September 24, 2024 06:18

albinsuresh had a problem deploying to Test Pull Request September 24, 2024 06:18 — with GitHub Actions Failure

didier-wenzek reviewed Sep 24, 2024

View reviewed changes

rina23q reviewed Sep 24, 2024

View reviewed changes

albinsuresh temporarily deployed to Test Pull Request September 25, 2024 13:28 — with GitHub Actions Inactive

didier-wenzek reviewed Sep 25, 2024

View reviewed changes

albinsuresh temporarily deployed to Test Auto September 25, 2024 14:03 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Pull Request September 26, 2024 13:03 — with GitHub Actions Inactive

albinsuresh requested review from didier-wenzek, reubenmiller and rina23q September 26, 2024 13:05

albinsuresh temporarily deployed to Test Auto September 26, 2024 13:27 — with GitHub Actions Inactive

didier-wenzek approved these changes Sep 26, 2024

View reviewed changes

albinsuresh added 2 commits September 27, 2024 04:24

Increase default MQTT payload size to 16 MiB

498fb4b

Make cloud MQTT max payload size configurable

7dbd1aa

albinsuresh force-pushed the fix/3124/increase-default-mqtt-message-size branch from f43495e to 7dbd1aa Compare September 27, 2024 04:24

albinsuresh temporarily deployed to Test Pull Request September 27, 2024 04:24 — with GitHub Actions Inactive

albinsuresh temporarily deployed to Test Auto September 27, 2024 04:30 — with GitHub Actions Inactive

albinsuresh added this pull request to the merge queue Sep 27, 2024

Merged via the queue into thin-edge:main with commit af73e0c Sep 27, 2024

albinsuresh mentioned this pull request Oct 1, 2024

Cumulocity mapper stuck in a disconnect-reconnect loop when it receives a message more than 1 MiB in size #3124

Closed

reubenmiller changed the title ~~Increase default MQTT payload size to 256 MiB~~ fix: Increase default MQTT payload size to 256 MiB Oct 2, 2024

reubenmiller added the theme:mqtt Theme: mqtt and mosquitto related topics label Oct 2, 2024

albinsuresh deleted the fix/3124/increase-default-mqtt-message-size branch October 22, 2024 06:45

	tedge_config.az.mapper.mqtt.max_payload_size,
	tedge_config.aws.mapper.mqtt.max_payload_size,

		max_packet_size: 1024 * 1024,
		max_packet_size: MAX_PACKET_SIZE,

	/// Default: `268435455` (256 MB).
	/// Default: `1024 * 1024`.

Conversation

albinsuresh commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed changes

Types of changes

Paste Link to the issue

Checklist

Further comments

Uh oh!

codecov bot commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Robot Results

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rina23q left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

didier-wenzek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

albinsuresh commented Sep 23, 2024 •

edited

Loading

codecov bot commented Sep 23, 2024 •

edited

Loading

github-actions bot commented Sep 23, 2024 •

edited

Loading