Skip to content

Move ADR certificate management to the certificate-authority model - #447

Merged
Ewerton Scaboro da Silva (ewertons) merged 50 commits into
masterfrom
port-adr-certificate-authority-model
Sep 17, 2026
Merged

Ewerton Scaboro da Silva (ewertons) merged 50 commits into
masterfrom
port-adr-certificate-authority-model

Conversation

@ewertons

@ewertons Ewerton Scaboro da Silva (ewertons) commented Sep 4, 2026 •

Copy link
Copy Markdown
Contributor

Certificate management provisioning no longer works: the ADR object model it targeted was public preview and has been replaced.

A certificate policy used to hang off a namespace credential and be referenced from an enrollment by a single name. It now hangs off an issuing CA, and an enrollment references it by three names that must travel together. The hub and DPS used to be pointed at a namespace and a shared user-assigned identity when they were created; the relationship is now expressed on the namespace, as endpoints linked to it afterwards, with each resource authenticating as its own system-assigned identity.

Changes

  • Create the root -> issuing CA -> certificate policy chain, after linking, so ADR has a hub to sync the issuing CA certificate to. A policy created under a root is rejected with PolicyRequiresIssuingCa.
  • Link the hub and DPS to the namespace and wait for the endpoints, retrying while the namespace identity grants replicate, and healing a namespace left Failed by an otherwise successful link.
  • Write enrollments over the DPS service API, carrying the three policy names. The CLI cannot express them: it only ever offered --credential-policy, and the az iot adr group ships only in preview builds and still models the retired shape.
  • Push the linked namespace into the DPS data plane with a tags-only update. Committing a link does not push scale-unit configuration, so without it every enrollment write fails with errorCode 400004.
  • Skip linked-hub creation under certificate management, where ADR chooses the provisioning targets and the DPS list is read-only (IH409313).
  • Read linked hubs from the namespace endpoints. They were read from the DPS resource captured before the hub is attached, so the list was always empty.
  • Drop the credential sync, the Contributor grant to the IoT Hub first-party application, and the Onboarding role, all of which belonged to the previous model.
  • Keep the azure-iot extension version pin. ADR no longer needs it -- the namespace, the certificate authorities and the link all go to ARM directly -- but the pin has a separate reason that still holds: 0.32.0b1 returns the device-facing hostname from az iot hub connection-string show, which this module reads for the service-side clients. Only the ADR justification for the pin is removed, not the pin.

TestEnvironmentInfo.AzureAdrPolicyName is replaced by AdrPolicy, and the generated test configuration exports the namespace and certificate authority names alongside ADR_CERT_MGMT_POLICY_NAME. No other file in the repository referenced the old property.

Verification

Run locally against PowerShell 7.4.6; the module parses and imports cleanly, and 37 assertions pass across two suites:

  • Unit: policy reference all-or-nothing semantics, enrollment body shape, TestEnvironmentInfo JSON round-trip, and the DPS SAS token checked against an independent implementation of the same HMAC-SHA256 construction.
  • Flow: the provisioning sequence driven against a stubbed CLI, asserting the three certificate authority resources are created in order with the policy under the issuing CA, the enrollment PUT goes to the service API at the ADR-aware api-version with all three policy names and no credentialPolicyName, and no retired command or flag is used.

Live provisioning against a subscription has not been run and is the remaining validation: create an environment with -EnableCertificateManagement, confirm the chain and endpoints reach Succeeded, and confirm the first enrollment write is not rejected.

Certificate management provisioning no longer works: the ADR object model it
targeted was public preview and has been replaced.

A certificate policy used to hang off a namespace credential and be referenced
from an enrollment by a single name. It now hangs off an issuing CA, and an
enrollment references it by three names that must travel together. The hub and
DPS used to be pointed at a namespace and a shared user-assigned identity when
they were created; the relationship is now expressed on the namespace, as
endpoints linked to it afterwards, with each resource authenticating as its own
system-assigned identity.

- Create the root -> issuing CA -> certificate policy chain, after linking, so
  ADR has a hub to sync the issuing CA certificate to. A policy created under a
  root is rejected with PolicyRequiresIssuingCa.
- Link the hub and DPS to the namespace and wait for the endpoints, retrying
  while the namespace identity's grants replicate and healing a namespace left
  Failed by an otherwise successful link.
- Write enrollments over the DPS service API, carrying the three policy names.
  The CLI cannot express them: it only ever offered --credential-policy.
- Push the linked namespace into the DPS data plane with a tags-only update.
  Committing a link does not push scale-unit configuration, so without it every
  enrollment write fails with errorCode 400004.
- Skip linked-hub creation under certificate management, where ADR chooses the
  provisioning targets and the DPS's own list is read-only.
- Read linked hubs from the namespace endpoints. They were read from the DPS,
  which is captured before the hub is attached, so the list was always empty.
- Drop the extension version pin, the credential sync, the Contributor grant to
  the IoT Hub first-party application and the Onboarding role, all of which
  belonged to the previous model.

TestEnvironmentInfo.AzureAdrPolicyName is replaced by AdrPolicy, and the
generated test configuration exports the namespace and certificate authority
names alongside ADR_CERT_MGMT_POLICY_NAME.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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 ADR namespace linking payload appears to use the IoT Hub endpoint type for the DPS endpoint, which is likely to break provisioning links at runtime.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the PowerShell provisioning/test module to use the post-public-preview Azure Device Registry (ADR) certificate-authority model for certificate management, replacing the previous CLI-driven ADR surface area with direct ARM + DPS service-API calls.

Changes:

  • Removes the azure-iot CLI extension version pin and routes ADR operations through az rest (ARM) instead of az iot adr.
  • Adds ADR provisioning/linking helpers (namespace creation, endpoint linking, CA chain + policy creation) and waits/polling utilities.
  • Writes DPS enrollments via the DPS service REST API to carry the three-part ADR policy reference (namespace/CA/policy).
File summaries
File Description
scripts/Azure.Iot.Sdk.Test.psm1 Reworks ADR certificate management provisioning and DPS enrollment creation to match the new certificate-authority model.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

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

Comment thread scripts/Azure.Iot.Sdk.Test.psm1 Outdated
The DPS provisioning endpoint was sent with the IoT Hub resource type, which
mis-models the link. Three further problems in the same function, found while
confirming that one:

- The hub messaging endpoint was missing its provisioning availability, so it
  was linked but not offered as a provisioning target.
- The link was submitted as a properties-only update. The saga starts on a
  namespace write carrying location and identity.
- Reconciling a namespace left Failed re-sent the endpoints, which is rejected
  as immutable, and judged the result on the first read, which still reports
  Failed for a while. It is now a tags-only update, polled to Succeeded.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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 updated E2E config generators dereference TestEnvInfo.AdrPolicy.* unconditionally, which will throw when certificate management isn’t enabled (AdrPolicy is null), breaking config generation for non-ADR environments.

Review details

Suppressed comments (7)

Previously missed (2) — in code that hasn't changed since the last review.

scripts/Azure.Iot.Sdk.Test.psm1:1577

  • The ADR/DPS api-version constants are future-dated relative to the current date (e.g. 2026-11-02-preview). If these versions aren't available in the target cloud/subscription yet, every az rest call will fail with an invalid/unsupported api-version error. Consider allowing these to be overridden via environment variables (or module parameters) so runs can be unblocked without code changes.
    scripts/Azure.Iot.Sdk.Test.psm1:3150
  • TestEnvInfo.AdrPolicy can be $null when certificate management wasn't enabled (e.g., environments created without -EnableCertificateManagement or JSON loaded from older configs). These lines dereference .NamespaceName/etc unconditionally, which will throw and prevent config generation even for non-ADR runs. Consider emitting empty strings when AdrPolicy is null (matching the previous string-based behavior).

This issue also appears in the following locations of the same file:

  • line 3179
  • line 3256
  • line 3278
  • line 3340
  • line 3370

scripts/Azure.Iot.Sdk.Test.psm1:3181

  • Same null-dereference issue as the PowerShell block above: this bash config generation path also assumes TestEnvInfo.AdrPolicy is non-null and will throw when it isn't. Emit empty strings when AdrPolicy is null to keep config generation working for non-ADR environments.
            "export ADR_CERT_MGMT_NAMESPACE_NAME=`"$($TestEnvInfo.AdrPolicy.NamespaceName)`""
            "export ADR_CERT_MGMT_CERTIFICATE_AUTHORITY_NAME=`"$($TestEnvInfo.AdrPolicy.CertificateAuthorityName)`""
            "export ADR_CERT_MGMT_POLICY_NAME=`"$($TestEnvInfo.AdrPolicy.CertificatePolicyName)`""

scripts/Azure.Iot.Sdk.Test.psm1:3258

  • New-AzIotNetSDKE2ETestConfig dereferences TestEnvInfo.AdrPolicy.* unconditionally. If the environment was created without certificate management, AdrPolicy is $null and config generation will throw. Use empty strings (or conditionally omit these variables) when AdrPolicy is not set.
            "`$env:ADR_CERT_MGMT_NAMESPACE_NAME = `"$($TestEnvInfo.AdrPolicy.NamespaceName)`""
            "`$env:ADR_CERT_MGMT_CERTIFICATE_AUTHORITY_NAME = `"$($TestEnvInfo.AdrPolicy.CertificateAuthorityName)`""
            "`$env:ADR_CERT_MGMT_POLICY_NAME = `"$($TestEnvInfo.AdrPolicy.CertificatePolicyName)`""

scripts/Azure.Iot.Sdk.Test.psm1:3280

  • Same null-dereference issue as the PowerShell block above: the bash path assumes TestEnvInfo.AdrPolicy is non-null and will throw otherwise. Emit empty strings when AdrPolicy is $null to preserve backward compatibility.
            "export ADR_CERT_MGMT_NAMESPACE_NAME=`"$($TestEnvInfo.AdrPolicy.NamespaceName)`""
            "export ADR_CERT_MGMT_CERTIFICATE_AUTHORITY_NAME=`"$($TestEnvInfo.AdrPolicy.CertificateAuthorityName)`""
            "export ADR_CERT_MGMT_POLICY_NAME=`"$($TestEnvInfo.AdrPolicy.CertificatePolicyName)`""

scripts/Azure.Iot.Sdk.Test.psm1:3342

  • New-AzIotPythonSDKE2ETestConfig dereferences TestEnvInfo.AdrPolicy.* unconditionally. If AdrPolicy is $null (non-certificate-management environment), this will throw and block config generation. Consider emitting empty strings when AdrPolicy is not present.
            "`$env:ADR_CERT_MGMT_NAMESPACE_NAME = `"$($TestEnvInfo.AdrPolicy.NamespaceName)`""
            "`$env:ADR_CERT_MGMT_CERTIFICATE_AUTHORITY_NAME = `"$($TestEnvInfo.AdrPolicy.CertificateAuthorityName)`""
            "`$env:ADR_CERT_MGMT_POLICY_NAME = `"$($TestEnvInfo.AdrPolicy.CertificatePolicyName)`""

scripts/Azure.Iot.Sdk.Test.psm1:3372

  • Same null-dereference issue as the PowerShell block above: this bash block also assumes TestEnvInfo.AdrPolicy is non-null and will throw otherwise. Emit empty strings when AdrPolicy is $null.
            "export ADR_CERT_MGMT_NAMESPACE_NAME=`"$($TestEnvInfo.AdrPolicy.NamespaceName)`""
            "export ADR_CERT_MGMT_CERTIFICATE_AUTHORITY_NAME=`"$($TestEnvInfo.AdrPolicy.CertificateAuthorityName)`""
            "export ADR_CERT_MGMT_POLICY_NAME=`"$($TestEnvInfo.AdrPolicy.CertificatePolicyName)`""
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

The generated test configuration reads the three policy names directly. With no
certificate management the reference was null, which renders as empty strings
normally but throws under Set-StrictMode, so config generation could break for
environments that never asked for ADR.

An environment now always carries a reference; it is simply incomplete when
certificate management is off. Callers already gate on IsComplete(), so nothing
else changes, and the emitted values match the previous empty strings.

The ADR and DPS api-versions can also be overridden from the environment, so a
cloud or region where one is not registered can be unblocked without a code
change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ewertons

Copy link
Copy Markdown
Contributor Author

Addressed both points from the second review in 4fdd94c.

Null AdrPolicy in the config generators. Confirmed: it renders as empty strings normally but throws under Set-StrictMode -Version 3.0, so the risk is real. Rather than guard twelve call sites, an environment now always carries a reference and it is simply incomplete when certificate management is off. Callers already gate on IsComplete(), so the emitted values match the previous empty-string behaviour. Covered by a test that renders the three names under StrictMode.

Future-dated api-versions. These are the versions the certificate-authority model exists at, so they cannot be lowered, but they are now overridable via ADR_API_VERSION, DPS_CONTROL_PLANE_API_VERSION and DPS_ENROLLMENT_API_VERSION so a cloud or region where one is not registered can be unblocked without a code change.

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

It makes large, high-impact changes to live Azure provisioning behavior (preview ARM api-versions, role/linking sagas, and DPS service-API enrollment writes) that require human validation in a real subscription.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread scripts/Azure.Iot.Sdk.Test.psm1 Outdated
Removing the version pin means the stable azure-iot extension is installed, and
'az iot dps create' accepts identity flags only in preview builds of it. The DPS
was being created with --mi-system-assigned, which the stable extension rejects
as an unrecognized argument, so provisioning would have failed on exactly the
version the unpinned install now selects.

The DPS is created plain again and its system-assigned identity is turned on
through ARM, alongside the rest of the ADR wiring, and only when certificate
management is enabled. The IoT Hub keeps --mi-system-assigned, which is a core
CLI flag rather than an extension one.

An extension that is already installed is also updated rather than left as it
is, so a long-lived agent cannot keep provisioning with a stale one.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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

There is a confirmed null-dereference bug in the new ARM provisioning-state wait loop, and the default ADR/DPS API versions are dated in the future and may break provisioning without overrides.

Review details

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

scripts/Azure.Iot.Sdk.Test.psm1:501

  • Wait-AzProvisioningState calls (Invoke-AzRest -AllowFailure).properties.provisioningState directly. When Invoke-AzRest returns $null (any transient ARM/CLI failure), this will throw a null-reference and abort the wait loop, defeating the purpose of -AllowFailure.
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Wait-AzProvisioningState read the provisioning state straight off the result of
a call it had explicitly allowed to fail. These resources are polled for
minutes, and a transient ARM or CLI failure along the way says nothing about the
provisioning itself, so the failed read is now treated as an unknown state for
that attempt and the wait continues against its deadline.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ewertons

Copy link
Copy Markdown
Contributor Author

Addressed the latest review in 80b9b3f.

Null dereference in Wait-AzProvisioningState. Valid. Confirmed the behaviour: $null.properties.provisioningState yields $null normally but throws under Set-StrictMode -Version 3.0, and either way reading the state straight off a call that was explicitly allowed to fail contradicted the intent. A failed read is now an unknown state for that attempt, and the wait continues against its deadline. Covered by a test that fails the first read and requires the wait to still settle.

Future-dated api-versions. These cannot be lowered: the certificate-authority resource tree exists only at 2026-11-02-preview and later, which is the whole reason for this change; the earlier versions expose the retired model. They are overridable via ADR_API_VERSION, DPS_CONTROL_PLANE_API_VERSION and DPS_ENROLLMENT_API_VERSION for a cloud or region where one is not registered.

Local verification: module parses and imports, 43 assertions pass across the unit and flow suites. Live provisioning against a subscription remains the outstanding validation, as noted in the description.

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 defaults include future-dated api-version strings and there is at least one reliability issue around masking az extension update failures that should be addressed before merging.

Review details

Suppressed comments (1)

scripts/Azure.Iot.Sdk.Test.psm1:167

  • Install-AzureIotCliExtension unconditionally suppresses az extension update stderr and then forces $LASTEXITCODE = 0. This hides real update failures (e.g., transient network issues, extension install corruption), which can later surface as confusing provisioning errors while the log claims the extension was updated. It’s safer to ignore only the known "already up to date" failure mode and fail fast for other errors.
        # Already up to date is reported as a failure, so the exit code is reset rather than checked.
        Write-Host "Azure IoT extension $($Extension.version) found; updating."
        az extension update --name azure-iot --only-show-errors 2>$null | Out-Null
        $global:LASTEXITCODE = 0
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

The update suppressed stderr and reset the exit code, on the belief that being
already up to date is reported as a failure. It is not: with no newer version
available the command warns and returns successfully. The suppression therefore
bought nothing and hid genuine failures, which would have surfaced later as
confusing provisioning errors against a log claiming the extension was updated.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ewertons

Copy link
Copy Markdown
Contributor Author

Addressed in 8fac417.

Masked az extension update failures. Correct, and my justifying comment was simply wrong. Checked the CLI source: when no newer version is available update_extension hits NoExtensionCandidatesError, logs a warning and returns, so the command exits successfully. Being already up to date was never reported as a failure, so the suppression bought nothing and only hid real errors. Both the redirect and the exit-code reset are gone; it now fails fast via Stop-OnError.

Future-dated api-versions. As above, these cannot be lowered without reverting to the retired model, and all three are overridable from the environment.

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

There are confirmed null-handling bugs in tags-only PATCH flows (which can break reconciliation/dataplane sync) and a polling robustness issue that can fail provisioning on transient ARM reads.

Review details

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

scripts/Azure.Iot.Sdk.Test.psm1:1704

  • Connect-AdrNamespace polls for endpoint linkingState for up to 15 minutes, but a single transient failure to read the namespace via Invoke-AzRest will currently stop provisioning immediately. This is inconsistent with Wait-AzProvisioningState, which treats transient ARM/CLI read failures as non-fatal during polling.
    scripts/Azure.Iot.Sdk.Test.psm1:1732
  • In the namespace reconciliation path, $Namespace.tags can be $null when the resource has no tags; calling .PSObject.Properties on $null will throw and prevent recovery from provisioningState=Failed. Guard the tag enumeration so the reconcile PATCH always succeeds.

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

scripts/Azure.Iot.Sdk.Test.psm1:1837

  • Sync-DpsAdrConfiguration builds $Tags by enumerating (Invoke-AzRest ...).tags.PSObject.Properties, but .tags can be $null when no tags are set, causing the best-effort sync to always fail early (and skip the PATCH that triggers the dataplane push). Guard the tag enumeration so the PATCH is attempted even with no existing tags.
    try {
        $Tags = @{}
        (Invoke-AzRest -Url $Url).tags.PSObject.Properties | %{ $Tags[$_.Name] = $_.Value }
        $Tags["AdrDataplaneSyncUtc"] = (Get-Date).ToUniversalTime().ToString("o")
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Both tags-only updates copied the resource's current tags by piping the tags
property into ForEach-Object. Piping a null still runs the body once, with a
null key, so on a resource carrying no tags the copy threw. Neither the ADR
namespace nor the DPS is created with tags, so this was the normal case, not an
edge one: the namespace reconcile that recovers a link left Failed would throw
instead of recovering, and the DPS data-plane push would fail before it was
attempted, leaving every enrollment write to fail with errorCode 400004.

The namespace link poll also gave up the whole run on a single failed read, over
a window of up to fifteen minutes. It now treats a failed read the same way the
provisioning-state wait does: an attempt spent, not a failure.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Its only consumer was the old certificate-management role-assignment branch,
which is gone, so leaving the assignment suggested the behaviour still varies
with how the caller signed in.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ewertons

Copy link
Copy Markdown
Contributor Author

Addressed the suppressed finding from the last review in the latest commit: $IsAzureAccountServicePrincipal was left assigned but unread once the old certificate-management role-assignment branch went, so it is removed. Verified it had exactly one consumer on master and none here.

master repinned the extension and now installs it from the release wheel. This
branch had removed the pin on the grounds that ADR no longer needs the preview
`az iot adr` group, which is true but was not the only reason to pin: 0.32.0b1
returns the device-facing hostname from `az iot hub connection-string show`, and
this module reads that string five times for the service-side clients.

So the pin stays exactly as master has it, including remove-and-reinstall rather
than update, which would have moved off the pinned version. Only the ADR
justification is rewritten to say that part is no longer why.
@ewertons

Copy link
Copy Markdown
Contributor Author

Merged master in; the branch was conflicting.

The conflict was the extension pin. This branch had removed it because ADR no longer needs the preview az iot adr group — true, but not the only reason to pin. master has since repinned to 0.30.0b2 and installs from the release wheel, and one of its reasons applies here too: 0.32.0b1 returns the device-facing hostname from az iot hub connection-string show, which this module reads five times for the service-side clients.

So the pin is kept exactly as master has it, including remove-and-reinstall rather than update — my version updated to whatever was newest, which would have moved off the pin. Only the ADR justification is rewritten to say that part is no longer why it is pinned. Tests updated to assert the pinned wheel is installed and not updated away from.

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

Address the three moderate issues before approval.

Review details

Suppressed comments (3)

scripts/Azure.Iot.Sdk.Test.psm1:2895

  • The DPS identity's IotHubDataContributorRoleId grant on $AzureIoTHub.id is created above, but this wait block only verifies the DPS identity's roles on the namespace. Because device registration runs through the DPS managed identity and provider RBAC is eventually consistent, the function can return and the first provisioning attempt can race this unpropagated hub grant; include the hub-scope data-contributor role in the propagation wait (or otherwise retry that operation).
                @{ Assignee = $DpsPrincipalId;          Role = $script:ContributorRoleId;           Scope = $AdrNamespaceId },
                @{ Assignee = $AdrNamespacePrincipalId; Role = $script:ContributorRoleId;           Scope = $AdrNamespaceId },
                @{ Assignee = $DpsPrincipalId;          Role = $script:AdrContributorRoleId;        Scope = $AdrNamespaceId },
                @{ Assignee = $DpsPrincipalId;          Role = $script:IotHubDataContributorRoleId; Scope = $AzureIoTHub.id },

scripts/Azure.Iot.Sdk.Test.psm1:1677

  • The initial DELETE is issued with -AllowFailure, so a transient 5xx/CLI failure is discarded and the loop only polls the still-present namespace for 10 minutes; it never retries the deletion. In the recovery path this turns a recoverable delete outage into a timeout instead of recreating the namespace. Retry the DELETE on transient errors (while treating a confirmed 404 as already deleted) before entering the confirmation loop.
    }

scripts/Azure.Iot.Sdk.Test.psm1:2797

  • This validation is reached only after Azure login, CLI extension installation, and resource-group creation/update, so an invalid -EnableCertificateManagement -NoDps invocation still mutates Azure before throwing and may leave a newly created resource group behind. Move the mutually-exclusive-parameter check to the start of New-AzIotTestEnvironment, before any provisioning side effects.

        # Created and tagged in one call, so the group is never observable in an untagged
        # state that the leftover-resource cleanup would have to skip forever.
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Three things a review caught:

- the -NoDps / -EnableCertificateManagement conflict was rejected only after login,
  extension install and resource-group creation, so a bad invocation mutated Azure and
  could leave a resource group behind. It is checked before any of that now.
- the namespace delete discarded a transient failure, leaving the confirmation loop to
  poll a namespace nothing had asked to remove; a brief outage became the full ten-minute
  timeout instead of a recreate. The DELETE is retried on a transient failure.
- the propagation wait covered the DPS identity's namespace grants but not its IoT Hub
  Data Contributor grant, which device registration runs on. Waiting for the namespace
  ones and not that one leaves the first registration racing it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ewertons

Copy link
Copy Markdown
Contributor Author

Addressed the three suppressed findings from the last review in the latest commit.

  • The -NoDps / -EnableCertificateManagement conflict was rejected only after login, extension install and resource-group creation, so a bad invocation mutated Azure and could leave a group behind. Checked first now.
  • The namespace delete discarded a transient failure and then polled a namespace nothing had asked to remove, turning a brief outage into the full ten-minute timeout instead of a recreate. The DELETE is retried.
  • The propagation wait covered the DPS identity on the namespace but not its IoT Hub Data Contributor grant, which device registration actually runs on. Now waited for.

Local gates green.

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 deletion retry path suppresses failures, and the retained extension pin conflicts with the documented change.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Lite

Comment thread scripts/Azure.Iot.Sdk.Test.psm1 Outdated
Comment thread scripts/Azure.Iot.Sdk.Test.psm1 Outdated
…ly sees

The DELETE retry added earlier never fired: -AllowFailure resets the exit code,
so the retry helper saw success every time. It uses the throwing form now, and a
not-found is treated as already deleted rather than propagated.

Two diagnostics, prompted by the observation that an endpoint can sit in a
section this code never read:

- the `updating` endpoint section is reported alongside messaging and
  provisioning, so an endpoint that is missing from the two we submit can be
  told apart from one that moved;
- the namespace principal is compared against the one its role grants were made
  against. If a write ever replaced it, the grants point at a principal that no
  longer exists, and ADR reports that as the linked resource being unreadable --
  the same message as a grant that has not replicated. The comparison rides the
  read the poll already does, so it costs no extra call.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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

Unresolved provisioning-state and DPS synchronization error handling issues remain, and live provisioning has not been validated.

Review details

Suppressed comments (1)

scripts/Azure.Iot.Sdk.Test.psm1:2048

  • This catch covers failures from the initial DPS GET and the PATCH as well as a timeout while an accepted PATCH is settling. The module's own description says the link commit does not update the data plane, so a permanent GET/PATCH failure means the subsequent enrollment retries cannot repair the missing namespace configuration and will all end in 400004 after a long delay. Only swallow a post-acceptance settling timeout; propagate a failed read or rejected PATCH immediately so the real provisioning error is visible.
    catch {
        Write-Host "DPS ADR configuration push did not settle cleanly ($($_.Exception.Message)); continuing, as enrollment creation waits for the data plane anyway."
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread scripts/Azure.Iot.Sdk.Test.psm1 Outdated
@ewertons

Copy link
Copy Markdown
Contributor Author

Tested the region hypothesis and two diagnostic gaps in one run (build subscription, resources in eastus2euap instead of centraluseuap, everything else unchanged). Three results, all negative, which is useful:

  1. Region is not it. Same failure: provisioning=<no endpoint>, hub-1=Failed (LinkableResourceNotReady), across both recovery cycles. Provisioning resources in the same region as the working ADU environment changes nothing.
  2. The namespace identity is not being replaced by the link. The new check compares the principal the grants were made against with the one the namespace carries during the link; it never fired. So "the grants point at a principal that no longer exists" is ruled out as the cause of LinkableResourceNotReady.
  3. The DPS endpoint is not migrating to another section. The poll now reports the updating section too; it stayed empty throughout. So the provisioning endpoint genuinely is never created, rather than being created somewhere this code was not reading.

Also fixed this round, from review: the namespace DELETE retry added earlier was a no-op (-AllowFailure resets the exit code, so the retry helper saw success and never retried) — it uses the throwing form now, with a not-found treated as already deleted. And the PR description no longer claims the extension pin is dropped; it is kept, for a reason unrelated to ADR.

Local gates: 20 unit and 42 flow assertions. Teardown succeeded; no resources left behind.

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

A critical identity mismatch can leave the environment unusable, and reconciliation mishandles a pre-existing Failed state.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

scripts/Azure.Iot.Sdk.Test.psm1:1936

  • The reconciliation path cannot tolerate the transient Failed state it explicitly documents: Wait-AzProvisioningState treats any provisioningState = Failed as terminal and throws on the first poll. If the namespace still reports its pre-reconcile Failed state after this tags-only PATCH, setup aborts instead of waiting for the reconciliation to reach Succeeded; use a reconciliation-specific poll that ignores that pre-existing state until the update settles.
        Invoke-AzRest -Method PATCH -Url $Url -Body @{ tags = $Tags } | Out-Null

        Wait-AzProvisioningState -Url $Url -Step "ADR namespace reconcile" -TimeoutSeconds 300
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread scripts/Azure.Iot.Sdk.Test.psm1 Outdated
…entity

Two things a review caught:

- the namespace reconcile waited with a helper that treats the first Failed read
  as terminal, which is precisely the state the reconcile exists to clear. The
  comment above it already said the state lingers after the PATCH is accepted,
  so the heal threw on the condition it was added to fix and setup never reached
  the certificate authority. It now waits through Failed until it clears or the
  deadline passes.
- a namespace identity that no longer matches the principal its grants were made
  against was logged and accepted. The link can still report success, but every
  later ADR call then runs as an identity holding nothing. It is raised instead,
  on a message the recovery cycle recognises, so the namespace is recreated and
  regranted rather than carried forward unusable.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ewertons

Copy link
Copy Markdown
Contributor Author

Tested the provider re-registration hypothesis (run 35030268243, build subscription, centraluseuap, everything else unchanged). It is not that.

az provider register was run for both Microsoft.Devices and Microsoft.DeviceRegistry with --wait, before any provisioning:

  • Microsoft.Devices registrationState: Registered before, Registered after
  • Microsoft.DeviceRegistry registrationState: Registered before, Registered after

The re-registration completed successfully — the pipeline identity has the rights for it — and changed nothing. So "features registered but never exposed on the provider" is eliminated for this subscription.

That run did not reach linking, for an unrelated reason worth recording: the DPS never left Activating, polled for the full 1200 s (134 polls) before timing out. The IoT Hub created normally in the same run. Previous runs in this subscription created the DPS in a couple of minutes, so this looks like a transient regional condition rather than anything in this change — nothing in the diff touches DPS creation, and the only difference from the known-failing baseline was two az provider register calls that were no-ops.

So the link itself is still untested against re-registration, but the hypothesis it was meant to test is already ruled out by the registration states being unchanged.

Also fixed this round, from review: the namespace reconcile waited with a helper that treats the first Failed read as terminal — the exact state the reconcile exists to clear, so the heal threw on the condition it was added to fix; it now waits through it. And a namespace identity that no longer matches the principal its grants were made against is raised rather than logged and accepted, so the recovery cycle recreates and regrants instead of carrying on with grants that hold nothing.

Local gates: 20 unit and 42 flow assertions. Teardown succeeded.

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

A moderate failure-handling issue remains, and live provisioning has not been validated.

Review details

Suppressed comments (1)

scripts/Azure.Iot.Sdk.Test.psm1:2066

  • This catch treats a definitive control-plane failure (for example an authorization/API-version error or the DPS resource reaching Failed) the same as a transient timeout and continues. Enrollment retries only recognize 403000/400004, so the original sync failure is then hidden or reported much later as a misleading data-plane error. Keep the best-effort path for transient/settling failures, but rethrow non-retryable failures so the run stops with the actual cause.
    catch {
        Write-Host "DPS ADR configuration push did not settle cleanly ($($_.Exception.Message)); continuing, as enrollment creation waits for the data plane anyway."
    }
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

Copilot AI added 4 commits September 16, 2026 23:49
The reconcile PATCH carried over whatever tags the namespace already had. The
feature tag on the namespace selects which identity the endpoint link authorizes
against, so a namespace reaching the reconcile without it would be healed into a
state the link still cannot use. Re-assert the tags instead of only copying them.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
DPS returns 400004 both for a namespace its data plane has not picked up yet, which clears on
its own, and for an api-version it does not support, which never does. The enrollment retry
treated both as transient and walked the full backoff ladder on the permanent one, taking about
31 minutes to report a request that was rejected outright on the first attempt.

Adds -StopOnPattern to Invoke-WithRetry, matched against the same captured output and taking
precedence over -RetryOnPattern, and uses it to exclude the unsupported-api-version case.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tions

Linking at 2026-11-02-preview succeeded on one subscription and failed every attempt on another
with LinkableResourceNotReady, which was indistinguishable from unreplicated role assignments.
2026-11-01-preview links first attempt on both, with no other change: provisioning now completes
on the subscription where it had never linked, and still completes on the one where it did.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…odule

master moved the framework out of the single scripts/Azure.Iot.Sdk.Test.psm1
into scripts/AzIotSdkTest/ as a manifested module, leaving that path a shim.
This branch had rewritten the old file, so every change had to move with it.

The split changed no code, so the parts are a contiguous partition of the file
this branch started from. Each change was carried to the part that now owns the
lines it touched: AzureCommon (ARM/REST helpers and retry), Dps (certificate
authority, policy and enrollment), Models, Provisioning, TestConfig, Common.

The shim is master's, unchanged. No part boundary moved, no export was added or
removed, and the module's diff against master is the same 1092 insertions and
184 deletions this branch carried before the move.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@ewertons
Ewerton Scaboro da Silva (ewertons) merged commit 5734267 into master Sep 17, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants