[PR from CLI tool] Hygiene changes in MQTT demos and tests #1128

aggarw13 · 2020-08-14T18:29:16Z

Hygiene Changes

Upgrade integration tests to use TLS mutual auth. Also, update CMake build for integration tests to use command-line defined CMake variables for credentials.
Remove default value of BROKER_ENDPOINT as test.mosquitto.org in demo_config.h files to avoid suggesting use of the public server. (The public Mosquitto server is unreliable as mentioned in their official documentation). Instead use comment blocks to direct customers to setup Mosquitto locally.
Fix build warning in mqtt.c from logging call.

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

…d setup to use CMake cache variables for credentials

…emos-and-tests

codecov-commenter · 2020-08-14T18:30:01Z

Codecov Report

Merging #1128 into development will increase coverage by 2.30%.
The diff coverage is 100.00%.

@@               Coverage Diff               @@
##           development    #1128      +/-   ##
===============================================
+ Coverage        96.54%   98.85%   +2.30%     
===============================================
  Files                9        4       -5     
  Lines             5643     1309    -4334     
  Branches           641      393     -248     
===============================================
- Hits              5448     1294    -4154     
+ Misses               9        0       -9     
+ Partials           186       15     -171

Impacted Files	Coverage Δ
libraries/standard/mqtt/src/mqtt.c	`99.48% <ø> (+5.06%)`	⬆️
libraries/standard/mqtt/src/mqtt_lightweight.c	`97.42% <ø> (+2.90%)`	⬆️
libraries/standard/mqtt/src/mqtt_state.c	`98.46% <ø> (+2.52%)`	⬆️
libraries/standard/http/src/http_client.c	`100.00% <100.00%> (+12.51%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 792032c...0eae5f7. Read the comment docs.

muneebahmed10 · 2020-08-14T18:54:54Z

demos/mqtt/mqtt_demo_basic_tls/demo_config.h

+ * Instructions to run Mosquitto on Docker container can be viewed in the
+ * README.md of the root directory.
+ *
+ * #define BROKER_ENDPOINT               "...insert here..."


I see value in being able to run the demos quickly without having the change a bunch of demo_config.h files or adding extra cmake flags. If we recommend running a broker locally, why not just change all of these to "localhost" instead?

Not sure whether that is necessarily helpful in speeding up the experience of running demos. With localhost, the customer would need to have a broker setup on the same machine. (The customer may very well want to setup their broker on a remote host).
A default value would be more valuable in speeding up the customer experience of running the demo, if the broker is publicly available (as was the case with test.mosquitto.org).
I can instead update the comments to use localhost instead of "...insert here..."

If they want to set up a broker on a remote host, that's their choice, just like they were able to choose to not use test.mosquitto.org before this change. Our wording of running the broker "locally" implies that they'd run it on the same machine as the posix demos, so localhost would be an obvious choice for a default.

If we want these demos to not build without editing these files or without specifying a cmake flag, then we don't need to make any changes; I'm just pointing out that making the change would mean one less cmake flag/one less file to edit for someone who is following our instructions on setting up a local broker

Have added the default configuration back, as I don't strongly feel against it.

Is the mosquitto broker installed and running by default on a machine? If not, when I run this demo , I will get connection refused error - at which point I need to go and debug why the connection was refused and eventually realize the that I do not have a mosquitto broker setup.

Wouldn't a #error be better telling me that I need to setup mosquitto broker.

Yes, I agree that having the default value of localhost can cause more developer effort in identifying the failure cause. Will remove the default value in a separate PR

nateglims · 2020-08-14T19:10:09Z

/bot run checks

demos/mqtt/mqtt_demo_basic_tls/demo_config.h

…k suggestion

aggarw13 · 2020-08-14T23:30:55Z

Alternate option to what? Seems like the only option we give

Fixed the wording

…s-iot-device-sdk-embedded-C into hygiene/mqtt-demos-and-tests

demos/mqtt/mqtt_demo_plaintext/demo_config.h

muneebahmed10 · 2020-08-14T23:41:36Z

libraries/standard/mqtt/integration-test/test_config.h

+ * This certificate should be PEM-encoded.
 */
-#ifndef CLIENT_IDENTIFIER
-    #define CLIENT_IDENTIFIER    "testclient"


Was this meant to be removed?

Yes, as this config wasn't used by the integration test. There are separate macros used by the tests

muneebahmed10 · 2020-08-14T23:43:07Z

libraries/standard/mqtt/integration-test/test_config.h

-
-/**
- * @brief Length of MQTT server host name.
+ * @note If using AWS IoT Core, the endpoint can be found in the AWS IoT console


Can the AWS IoT Core be used for all of our integration tests? Don't we have tests for the retain flag, which AWS IoT doesn't support?

Good point, have removed all comments pointing to use of AWS IoT from the configs

muneebahmed10 · 2020-08-14T23:58:37Z

libraries/standard/mqtt/integration-test/mqtt_system_test.c

    memset( &opensslCredentials, 0u, sizeof( OpensslCredentials_t ) );
    opensslCredentials.pRootCaPath = SERVER_ROOT_CA_CERT_PATH;
+    opensslCredentials.pClientCertPath = CLIENT_CERT_PATH;
+    opensslCredentials.pPrivateKeyPath = CLIENT_PRIVATE_KEY_PATH;


Should this be here? Do the system tests need mutual auth?

I was suggested to use mutual auth by @abhidixi11. They don't strictly require, but we should run the integration tests on the transport layer that we require for connecting to AWS

libraries/standard/mqtt/integration-test/CMakeLists.txt

muneebahmed10 · 2020-08-15T00:06:29Z

libraries/standard/mqtt/integration-test/CMakeLists.txt

+if(ROOT_CA_CERT_PATH)
+    target_compile_definitions(
+        ${stest_name} PRIVATE
+            SERVER_ROOT_CA_CERT_PATH="${ROOT_CA_CERT_PATH}"


Should this be an absolute path?

It is supposed to be provided by the user through CMake configuration.

Something of this sort is needed? https://github.com/aws/aws-iot-device-sdk-embedded-C/blob/development/demos/CMakeLists.txt#L58

I was referring to the CMake configuration command passed by the user.
I don't see much value in pre-pending the absolute path with $PWD as that would pre-pend the path (to the certificate) from where the cmake command is run (which is most likely in the build directory), and can cause unexpected failure to occur due to the path pre-pending. As the CI team will be providing the absolute path to the credential files in their CMake command, I think that suffices.

…n tests

…t in basic tls demo_config

nateglims · 2020-08-17T19:25:49Z

/bot run checks

* Upgrade MQTT integration test to use mutual auth, and update its build setup to use CMake cache variables for credentials * Fix build warning from logging * Remove references to test.mosquitto.org from demos

Archit Aggarwal added 5 commits August 14, 2020 18:09

Upgrade MQTT integration test to use mutual auth, and update its buil…

5b206c3

…d setup to use CMake cache variables for credentials

Fix build warning from logging

3895075

Remove references to test.mosquitto.org from demos

28f8b75

Minor hygiene in MQTT integration test configs

7e6461b

Merge remote-tracking branch 'origin/development' into hygiene/mqtt-d…

f025a5e

…emos-and-tests

muneebahmed10 reviewed Aug 14, 2020

View reviewed changes

Archit Aggarwal added 3 commits August 14, 2020 19:13

Add spellcheck flagged words to demos/lexicon.txt

9557d63

Add html and developerguide to mqtt/lexicon.txt

107122f

Merge branch 'development' into hygiene/mqtt-demos-and-tests

429264a

muneebahmed10 reviewed Aug 14, 2020

View reviewed changes

demos/mqtt/mqtt_demo_basic_tls/demo_config.h Outdated Show resolved Hide resolved

Add default value for BROKER_ENDPOINT as "localhost" based on feedbac…

7621886

…k suggestion

Archit Aggarwal added 2 commits August 14, 2020 23:31

Merge branch 'hygiene/mqtt-demos-and-tests' of github.com:aggarw13/aw…

10e289f

…s-iot-device-sdk-embedded-C into hygiene/mqtt-demos-and-tests

Fix typos for BROKER_ENDPOINT default definition

161f341

muneebahmed10 previously approved these changes Aug 14, 2020

View reviewed changes

muneebahmed10 reviewed Aug 14, 2020

View reviewed changes

More hygiene updates

37f0344

aggarw13 dismissed muneebahmed10’s stale review via 37f0344 August 14, 2020 23:51

muneebahmed10 previously approved these changes Aug 14, 2020

View reviewed changes

muneebahmed10 reviewed Aug 14, 2020

View reviewed changes

muneebahmed10 reviewed Aug 15, 2020

View reviewed changes

Remove CLIENT_IDENTIFIER definition from CMakeLists.txt of integratio…

e17c728

…n tests

aggarw13 dismissed muneebahmed10’s stale review via e17c728 August 15, 2020 00:08

aggarw13 requested a review from muneebahmed10 August 15, 2020 00:10

muneebahmed10 previously approved these changes Aug 15, 2020

View reviewed changes

Remove default value of ROOT_CA_CERT_PATH as test.mosquitto.org's cer…

bbd9b40

…t in basic tls demo_config

aggarw13 dismissed muneebahmed10’s stale review via bbd9b40 August 16, 2020 22:22

Fix typo

0eae5f7

aggarw13 mentioned this pull request Aug 16, 2020

[PR from CLI tool] Add MQTT demo for connection sharing with subscription manager #1098

Merged

muneebahmed10 approved these changes Aug 17, 2020

View reviewed changes

aggarg approved these changes Aug 18, 2020

View reviewed changes

aggarw13 merged commit def655c into aws:development Aug 18, 2020

aggarw13 deleted the hygiene/mqtt-demos-and-tests branch August 18, 2020 18:22

aggarw13 mentioned this pull request Aug 18, 2020

Hygiene changes in demo configuration #1136

Merged

Uh oh!

[PR from CLI tool] Hygiene changes in MQTT demos and tests #1128

[PR from CLI tool] Hygiene changes in MQTT demos and tests #1128

Uh oh!

Conversation

aggarw13 commented Aug 14, 2020

Hygiene Changes

Uh oh!

codecov-commenter commented Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

muneebahmed10 Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nateglims commented Aug 14, 2020

Uh oh!

Uh oh!

aggarw13 commented Aug 14, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aggarw13 Aug 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

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!

nateglims commented Aug 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Aug 14, 2020 •

edited

Loading

muneebahmed10 Aug 14, 2020 •

edited

Loading

aggarw13 Aug 14, 2020 •

edited

Loading