Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Aug 18, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5b206c3
Upgrade MQTT integration test to use mutual auth, and update its buil…
Aug 14, 2020
3895075
Fix build warning from logging
Aug 14, 2020
28f8b75
Remove references to test.mosquitto.org from demos
Aug 14, 2020
7e6461b
Minor hygiene in MQTT integration test configs
Aug 14, 2020
f025a5e
Merge remote-tracking branch 'origin/development' into hygiene/mqtt-d…
Aug 14, 2020
9557d63
Add spellcheck flagged words to demos/lexicon.txt
Aug 14, 2020
107122f
Add html and developerguide to mqtt/lexicon.txt
Aug 14, 2020
429264a
Merge branch 'development' into hygiene/mqtt-demos-and-tests
Aug 14, 2020
7621886
Add default value for BROKER_ENDPOINT as "localhost" based on feedbac…
Aug 14, 2020
10e289f
Merge branch 'hygiene/mqtt-demos-and-tests' of github.com:aggarw13/aw…
Aug 14, 2020
161f341
Fix typos for BROKER_ENDPOINT default definition
Aug 14, 2020
37f0344
More hygiene updates
Aug 14, 2020
e17c728
Remove CLIENT_IDENTIFIER definition from CMakeLists.txt of integratio…
Aug 15, 2020
bbd9b40
Remove default value of ROOT_CA_CERT_PATH as test.mosquitto.org's cer…
Aug 16, 2020
0eae5f7
Fix typo
Aug 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion demos/lexicon.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ inc
initializerequestheaders
iot
lastcontrolpacketsent
md
metadata
methodlen
milli
Expand Down Expand Up @@ -91,6 +92,7 @@ publishpacketsent
pubrec
pubrel
qos
readme
resending
resp
sdk
Expand All @@ -111,4 +113,4 @@ uint
unsub
unsuback
uri
www
www
25 changes: 10 additions & 15 deletions demos/mqtt/mqtt_demo_basic_tls/demo_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,36 +50,31 @@
/**
* @brief MQTT server host name.
*
* This demo uses the Mosquitto test server. This is a public MQTT server; do not
* publish anything sensitive to this server.
* Mosquitto MQTT broker can run locally as an alternate option. Please refer to
* the instructions in https://mosquitto.org/ for running a Mosquitto broker
* locally.
* This demo can be run using the open-source Mosquitto broker tool.
* A Mosquitto MQTT broker can be setup locally for running this demo against
* it. Please refer to the instructions in https://mosquitto.org/ for running
* a Mosquitto broker locally.
* Alternatively,instructions to run Mosquitto server on Docker container can
* be viewed in the README.md of the root directory.
*/
#ifndef BROKER_ENDPOINT
#define BROKER_ENDPOINT "test.mosquitto.org"
#define BROKER_ENDPOINT "localhost"
#endif

/**
* @brief Length of MQTT server host name.
*/
#define BROKER_ENDPOINT_LENGTH ( ( uint16_t ) ( sizeof( BROKER_ENDPOINT ) - 1 ) )

/**
* @brief MQTT server port number.
*
* In general, port 8883 is for secured MQTT connections.
*/
#define BROKER_PORT ( 8883 )
#define BROKER_PORT ( 8883 )

/**
* @brief Path of the file containing the server's root CA certificate.
*
* This certificate should be PEM-encoded.
*
* #define ROOT_CA_CERT_PATH ".....insert here...."
*/
#ifndef ROOT_CA_CERT_PATH
#define ROOT_CA_CERT_PATH "certificates/mosquitto.org.crt"
#endif

/**
* @brief Length of path to server certificate.
Expand Down
5 changes: 5 additions & 0 deletions demos/mqtt/mqtt_demo_basic_tls/mqtt_demo_basic_tls.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,11 @@
#define NETWORK_BUFFER_SIZE ( 1024U )
#endif

/**
* @brief Length of MQTT server host name.
*/
#define BROKER_ENDPOINT_LENGTH ( ( uint16_t ) ( sizeof( BROKER_ENDPOINT ) - 1 ) )

/**
* @brief Length of client identifier.
*/
Expand Down
17 changes: 8 additions & 9 deletions demos/mqtt/mqtt_demo_lightweight/demo_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,24 +50,23 @@
/**
* @brief MQTT server host name.
*
* This demo uses the Mosquitto test server. This is a public MQTT server; do not
* publish anything sensitive to this server.
* This demo can be run using the open-source Mosquitto broker tool.
* A Mosquitto MQTT broker can be setup locally for running this demo against
* it. Please refer to the instructions in https://mosquitto.org/ for running
* a Mosquitto broker locally.
* Alternatively,instructions to run Mosquitto server on Docker container can
* be viewed in the README.md of the root directory.
*/
#ifndef BROKER_ENDPOINT
#define BROKER_ENDPOINT "test.mosquitto.org"
#define BROKER_ENDPOINT "localhost"
#endif

/**
* @brief Length of MQTT server host name.
*/
#define BROKER_ENDPOINT_LENGTH ( ( uint16_t ) ( sizeof( BROKER_ENDPOINT ) - 1 ) )

/**
* @brief MQTT server port number.
*
* In general, port 1883 is for unsecured MQTT connections.
*/
#define BROKER_PORT 1883
#define BROKER_PORT 1883

/**
* @brief MQTT client identifier.
Expand Down
5 changes: 5 additions & 0 deletions demos/mqtt/mqtt_demo_lightweight/mqtt_demo_lightweight.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,11 @@
#error "Please define a unique CLIENT_IDENTIFIER."
#endif

/**
* @brief Length of MQTT server host name.
*/
#define BROKER_ENDPOINT_LENGTH ( ( uint16_t ) ( sizeof( BROKER_ENDPOINT ) - 1 ) )

/**
* @brief The topic to subscribe and publish to in the example.
*
Expand Down
2 changes: 0 additions & 2 deletions demos/mqtt/mqtt_demo_mutual_auth/demo_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,6 @@
/**
* @brief Details of the MQTT broker to connect to.
*
* This is the Thing's Rest API Endpoint for AWS IoT.
*
* @note Your AWS IoT Core endpoint can be found in the AWS IoT console under
* Settings/Custom Endpoint, or using the describe-endpoint API.
*
Expand Down
10 changes: 7 additions & 3 deletions demos/mqtt/mqtt_demo_plaintext/demo_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,11 +50,15 @@
/**
* @brief MQTT server host name.
*
* This demo uses the Mosquitto test server. This is a public MQTT server; do not
* publish anything sensitive to this server.
* This demo can be run using the open-source Mosquitto broker tool.
* A Mosquitto MQTT broker can be setup locally for running this demo against
* it. Please refer to the instructions in https://mosquitto.org/ for running
* a Mosquitto broker locally.
* Alternatively,instructions to run Mosquitto server on Docker container can
* be viewed in the README.md of the root directory.
*/
#ifndef BROKER_ENDPOINT
#define BROKER_ENDPOINT "test.mosquitto.org"
#define BROKER_ENDPOINT "localhost"
#endif

/**
Expand Down
34 changes: 25 additions & 9 deletions libraries/standard/mqtt/integration-test/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,28 @@ create_test(${stest_name}
"${test_include_directories}"
)

# Copy the certificates and client key to the binary directory.
add_custom_command(
TARGET
${stest_name}
POST_BUILD
COMMAND ${CMAKE_COMMAND} -E copy_directory
"${CMAKE_CURRENT_LIST_DIR}/certificates"
"$<TARGET_FILE_DIR:${stest_name}>/certificates"
)
# Set preprocessor defines for test if configured in build.
if(BROKER_ENDPOINT)
target_compile_definitions(
${stest_name} PRIVATE
BROKER_ENDPOINT="${BROKER_ENDPOINT}"
)
endif()
if(ROOT_CA_CERT_PATH)
target_compile_definitions(
${stest_name} PRIVATE
SERVER_ROOT_CA_CERT_PATH="${ROOT_CA_CERT_PATH}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an absolute path?

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

)
endif()
if(CLIENT_CERT_PATH)
target_compile_definitions(
${stest_name} PRIVATE
CLIENT_CERT_PATH="${CLIENT_CERT_PATH}"
)
endif()
if(CLIENT_PRIVATE_KEY_PATH)
target_compile_definitions(
${stest_name} PRIVATE
CLIENT_PRIVATE_KEY_PATH="${CLIENT_PRIVATE_KEY_PATH}"
)
endif()
10 changes: 7 additions & 3 deletions libraries/standard/mqtt/integration-test/mqtt_system_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,10 @@
#error "SERVER_ROOT_CA_CERT_PATH should be defined for the MQTT integration tests."
#endif

#ifndef CLIENT_IDENTIFIER
#error "CLIENT_IDENTIFIER should be defined for the MQTT integration tests."
#endif
/**
* @brief Length of MQTT server host name.
*/
#define BROKER_ENDPOINT_LENGTH ( ( uint16_t ) ( sizeof( BROKER_ENDPOINT ) - 1 ) )

/**
* @brief A valid starting packet ID per MQTT spec. Start from 1.
Expand Down Expand Up @@ -711,6 +712,9 @@ void setUp()
memset( &incomingInfo, 0u, sizeof( MQTTPublishInfo_t ) );
memset( &opensslCredentials, 0u, sizeof( OpensslCredentials_t ) );
opensslCredentials.pRootCaPath = SERVER_ROOT_CA_CERT_PATH;
opensslCredentials.pClientCertPath = CLIENT_CERT_PATH;
opensslCredentials.pPrivateKeyPath = CLIENT_PRIVATE_KEY_PATH;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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


serverInfo.pHostName = BROKER_ENDPOINT;
serverInfo.hostNameLength = BROKER_ENDPOINT_LENGTH;
serverInfo.port = BROKER_PORT;
Expand Down
38 changes: 13 additions & 25 deletions libraries/standard/mqtt/integration-test/test_config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,22 +50,8 @@
/**
* @brief MQTT server host name.
*
* This test uses the Mosquitto test server. This is a public MQTT server; do not
* publish anything sensitive to this server.
* Mosquitto MQTT broker can run locally as an alternate option. Please refer to
* the instructions in https://mosquitto.org/ for running a Mosquitto broker
* locally.
* #define BROKER_ENDPOINT "...insert here..."
*/
#ifndef BROKER_ENDPOINT
#define BROKER_ENDPOINT "test.mosquitto.org"
#endif

/**
* @brief Length of MQTT server host name.
*/
#ifndef BROKER_ENDPOINT_LENGTH
#define BROKER_ENDPOINT_LENGTH ( ( uint16_t ) ( sizeof( BROKER_ENDPOINT ) - 1 ) )
#endif

/**
* @brief MQTT server port number.
Expand All @@ -75,21 +61,23 @@
#define BROKER_PORT ( 8883 )

/**
* @brief Path of the file containing the server's root CA certificate.
* @brief Path of the file containing the client certificate.
*
* This certificate should be PEM-encoded.
* #define CLIENT_CERT_PATH "...insert here..."
*/
#ifndef SERVER_ROOT_CA_CERT_PATH
#define SERVER_ROOT_CA_CERT_PATH "certificates/mosquitto.org.crt"
#endif

/**
* @brief MQTT client identifier.
* @brief Path of the file containing the client's private key.
*
* No two clients may use the same client identifier simultaneously.
* #define CLIENT_PRIVATE_KEY_PATH "...insert here..."
*/

/**
* @brief Path of the file containing the server's root CA certificate.
*
* This certificate should be PEM-encoded.
*
* * #define SERVER_ROOT_CA_CERT_PATH "...insert here..."
*/
#ifndef CLIENT_IDENTIFIER
#define CLIENT_IDENTIFIER "testclient"
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this meant to be removed?

Copy link
Author

@aggarw13 aggarw13 Aug 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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

#endif

#endif /* ifndef TEST_CONFIG_H_ */
4 changes: 3 additions & 1 deletion libraries/standard/mqtt/lexicon.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ deserializeack
deserialized
deserializepublish
deserializing
developerguide
didn
disconnectpacketsize
doesn
Expand Down Expand Up @@ -70,6 +71,7 @@ handleincomingpublish
handlekeepalive
hasn
headersize
html
https
ifndef
inc
Expand Down Expand Up @@ -298,4 +300,4 @@ xa
xb
xc
xd
xe
xe
4 changes: 2 additions & 2 deletions libraries/standard/mqtt/src/mqtt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2014,8 +2014,8 @@ MQTTStatus_t MQTT_GetSubAckStatusCodes( const MQTTPacketInfo_t * pSubackPacket,
else if( pSubackPacket->remainingLength < 3U )
{
LogError( ( "Invalid parameter: Packet remaining length is invalid: "
"Should be greater than 2 for SUBACK packet: InputRemainingLength=%u",
pSubackPacket->remainingLength ) );
"Should be greater than 2 for SUBACK packet: InputRemainingLength=%lu",
( unsigned long ) pSubackPacket->remainingLength ) );
status = MQTTBadParameter;
}
else
Expand Down