Skip to content

[C] e2e: mqttv5 custom topics, and a ci-c-e2e-mqttv5 workflow - #413

Open
Ewerton Scaboro da Silva (ewertons) wants to merge 4 commits into
mainfrom
c/e2e-mqttv5-custom-topics
Open

Ewerton Scaboro da Silva (ewertons) wants to merge 4 commits into
mainfrom
c/e2e-mqttv5-custom-topics

Conversation

@ewertons

Copy link
Copy Markdown
Contributor

Adds e2e tests for the mqttv5 custom topic client (#409), and a ci-c-e2e-mqttv5 workflow to run mqttv5 e2e suites. Further mqttv5 clients can be added to it as separate jobs.

Test (c/tests/e2e/tests/e2e_mqttv5_custom_topic_test.c)

The device provisions through DPS (e2e_device.c) onto an mqttv5 hub with the custom topic templates e2e/{deviceId}/# and e2e/shared/#. Cases:

  1. Baseline: mqttv5 telemetry reaches the events endpoint. This separates environment problems from custom-topic problems.
  2. QoS 1 to e2e/{deviceId}/telemetry is acknowledged and reaches the events endpoint.
  3. QoS 0 to e2e/shared/alerts reaches the events endpoint.
  4. e2e-unlisted/telemetry (no template) is refused and not routed.
  5. e2e/not-{deviceId}/telemetry (another device's id) is refused and not routed.
  6. The session keeps publishing after the refusals.

It is built only with AZ_IOT_BUILD_E2E_MQTTV5, so ci-c-e2e (ctest -R e2e) does not pick it up.

Workflow (.github/workflows/ci-c-e2e-mqttv5.yml)

  • adr-namespace calls ci-c-e2e-mqttv5-adr-namespace.yml first.
  • custom-topics / linux:
    1. Builds the test.
    2. Issues a per-run device from the shared X.509 group CA with e2e-shared-device.ps1 -Prefix E2E_MQTTV5_SHARED.
    3. Reads the events endpoint through consumer group e2e-<run % 10>.
  • Skips when E2E_MQTTV5_SHARED_ID_SCOPE is unset, and for pull requests from forks.
  • Triggers: pull requests touching mqttv5 or core device code, adapters, the e2e harness or these workflows; nightly; manual.

c/eng/e2e-shared-device.ps1

  • New -Prefix E2E_MQTTV5_SHARED reads that environment's variables.
  • E2E_MQTTV5_SHARED_DPS_HOST maps to IOT_DPS_GLOBAL_ENDPOINT.
  • The default and -Csr outputs are unchanged.

Validation

  • Test, run locally in DPS mode against the shared mqttv5 environment (Linux, Paho):
    • cases 1, 4, 5 and 6 pass;
    • cases 2 and 3 fail: the hub acknowledges the custom-topic publishes, but they never reach the events endpoint (90 s wait), while the baseline telemetry does. This was reproduced over several runs and also with the az_mqtt adapter, which shows the refusals as PUBACK 0x87. It is a hub-side routing issue, so this workflow's job will fail on those two cases until it is fixed.
  • The workflow's Run step was run locally in PowerShell with an equivalent test config.
  • e2e-shared-device.ps1 was tested with a local CA in every mode: default, -Prefix E2E_MQTTV5_SHARED, -Csr, missing variables, and the iothubowner policy rejected. The issued leaf verifies against the CA.
  • actionlint 1.7.12 and zizmor 1.30.1: clean. The reusable-workflow call keeps ./ with an inline zizmor ignore, because actionlint 1.7.12 rejects the $/ form.
  • Repository checks and clang-format 18: clean.
  • Not run in GitHub Actions yet: the OIDC login for pull requests in the namespace job, and device issuance from the real group CA secret.

- e2e_mqttv5_custom_topic_test.c: on an mqttv5 hub with the templates
  e2e/{deviceId}/# and e2e/shared/#, QoS 1 and QoS 0 publishes reach the
  events endpoint; a topic no template allows and another device's id are
  refused without a disconnect, and the session keeps publishing.
  Telemetry runs first as a baseline. Built with AZ_IOT_BUILD_E2E_MQTTV5.
- ci-c-e2e-mqttv5.yml: runs mqttv5 suites on the shared mqttv5 environment,
  one job per client, after the ADR namespace job. Skips when
  E2E_MQTTV5_SHARED_ID_SCOPE is unset and for pull requests from forks.
- e2e-shared-device.ps1: -Prefix E2E_MQTTV5_SHARED reads that environment's
  variables, and E2E_MQTTV5_SHARED_DPS_HOST sets the DPS endpoint. Default
  unchanged.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Changes recommended

The workflow knowingly runs consistently failing cases, and timed-out publications can retain dangling callback contexts.

2 open findings
What changed in this PR

Adds MQTT v5 custom-topic end-to-end coverage and dedicated CI infrastructure.

Changes:

  • Tests allowed, refused, and post-refusal publishing scenarios.
  • Adds a shared-environment workflow and device configuration prefix.
  • Documents and gates the specialized suite.
File Description
c/​tests/​e2e/​tests/​e2e_mqttv5_custom_topic_test.c Adds custom-topic E2E scenarios.
c/​tests/​e2e/​CMakeLists.txt Registers the test target.
c/​eng/​e2e-shared-device.ps1 Supports MQTT v5 environment variables.
c/​docs/​eng/​end-to-end-tests.md Documents the suite and workflow.
c/​cmake/​az_iot_options.cmake Adds the MQTT v5 E2E build option.
.github/​workflows/​ci-c-e2e-mqttv5.yml Builds and runs the suite in CI.

🧠 Review effort: Balanced


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread c/tests/e2e/tests/e2e_mqttv5_custom_topic_test.c
Comment thread .github/workflows/ci-c-e2e-mqttv5.yml
…kip switch

- Publish records live in the fixture, so a PUBACK after a timed-out wait
  cannot complete a freed stack record.
- AZ_IOT_E2E_SKIP_CUSTOM_TOPIC_ROUTING turns the routed checks into skips
  when nothing arrives; ci-c-e2e-mqttv5 sets it while the hub does not
  deliver custom topic messages to the events endpoint.
Copilot AI balanced review requested due to automatic review settings October 11, 2026 08:10

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The refusal tests accept unrelated operational failures as successful evidence of topic rejection.

1 open finding
1 resolved since last review
Previously missed (1)

In code that hasn't changed since last review

Medium severity Reject unrelated failures in unlisted-topic assertion

c/​tests/​e2e/​tests/​e2e_mqttv5_custom_topic_test.c:327

This assertion accepts every failure, so a transient disconnect, timeout, or quota error would make the test pass without proving that the unlisted topic was refused. The two documented outcomes for this scenario are AZ_IOT_ERR_PUBLISH_REFUSED when the adapter preserves PUBACK 0x87 and AZ_IOT_ERR_MQTT when it does not; restrict the assertion to those values.

This issue also appears on line 340 of the same file.

🧠 Review effort: Balanced

AZ_IOT_ERR_PUBLISH_REFUSED (PUBACK 0x87) or AZ_IOT_ERR_MQTT from an adapter
that does not report the code; a timeout or disconnect now fails the case.
Copilot AI balanced review requested due to automatic review settings October 11, 2026 08:18
@ewertons

Copy link
Copy Markdown
Contributor Author

Re: refusal cases accepting unrelated failures: fixed in c672073. They now pass only on AZ_IOT_ERR_PUBLISH_REFUSED (PUBACK 0x87) or AZ_IOT_ERR_MQTT (adapter without reason codes); a timeout, disconnect or quota error fails them.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The workflow can fail on secretless Dependabot runs, and the negative routing checks can miss delayed unauthorized delivery.

0 open findings

1 resolved since last review
Previously missed (3)

In code that hasn't changed since last review

Medium severity Exclude Dependabot PRs from secret-dependent workflow jobs

.github/​workflows/​ci-c-e2e-mqttv5.yml:54

Dependabot pull requests are treated like fork-originated workflows for secret access even when head.repo.full_name equals this repository. This repository schedules weekly GitHub Actions updates, so an update to this workflow can satisfy this condition while AZURE_CLIENT_ID, the CA, and the connection-string secrets are empty, producing a guaranteed failing e2e run. Exclude dependabot[bot] from the PR path rather than starting the secret-dependent jobs.

Medium severity Use the full routing window for negative delivery tests

c/​tests/​e2e/​tests/​e2e_mqttv5_custom_topic_test.c:33

The negative cases declare a message “not routed” after only 30 seconds, while the same endpoint is allowed 90 seconds to deliver routed messages. A forbidden message delivered after 30 seconds would therefore make these tests pass incorrectly. Observe non-delivery for the same routing window unless the service has a documented shorter upper bound.

Low severity Fix unsupported MQTT v5 direct-connect claim

c/​tests/​e2e/​tests/​e2e_mqttv5_custom_topic_test.c:9

The direct-connect claim is not supported by this fixture: e2e_device_connect() leaves connection_profile at its MQTT v3 default in direct-hub mode, so the profile check below rejects it before the custom-topic client initializes. Remove this claim, or add a way for the shared fixture to declare MQTT v5 for direct connections.

🧠 Review effort: Balanced

…on-delivery

- ci-c-e2e-mqttv5 skips pull requests from Dependabot, which get no secrets.
- Refused publishes are watched for the same 90 s as routed ones.
- The test header no longer claims direct-hub mode; e2e_device connects
  directly as mqttv3.
Copilot AI balanced review requested due to automatic review settings October 11, 2026 08:33
@ewertons

Copy link
Copy Markdown
Contributor Author

Addressed in ee268cc:

  • Dependabot pull requests now skip the workflow (github.actor != 'dependabot[bot]').
  • Refused publishes are watched for non-delivery over the same 90 s window as routed ones.
  • Removed the direct-hub claim from the test header; the suite provisions through DPS.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 Needs a closer look

The workflow depends on unverified production OIDC and CA-backed provisioning paths, while routing assertions remain conditionally skipped.

0 open findings

🧠 Review effort: Balanced

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants