Skip to content

refactor(device_update): replace azure-sdk fork with local impl#155

Merged
JanZachmann merged 3 commits intoomnect:mainfrom
JanZachmann:chore
Apr 8, 2026
Merged

refactor(device_update): replace azure-sdk fork with local impl#155
JanZachmann merged 3 commits intoomnect:mainfrom
JanZachmann:chore

Conversation

@JanZachmann
Copy link
Copy Markdown
Contributor

@JanZachmann JanZachmann commented Apr 7, 2026

Summary

  • Removes all 5 azure-sdk-for-rust fork dependencies (azure_core, azure_identity, azure_iot_deviceupdate, azure_storage, azure_storage_blobs)
  • Replaces with local implementations under src/device_update/ using existing crates (oauth2, reqwest, hmac, sha2)
  • Adds hmac as the only new dependency

Motivation

The upstream azure-sdk-for-rust removed azure_iot_deviceupdate from its workspace (Nov 2022, never re-enabled) and no longer supports SAS URL generation in azure_storage_blob. Continuing with the fork is not viable long-term.

Changes

New module structure under src/device_update/:

  • token.rs — OAuth2 client_credentials flow with token caching (via oauth2 crate)
  • client.rs — ADU REST client for import/delete with exponential backoff polling
  • blob_sas.rs — HMAC-SHA256 Service SAS URL generation for Azure Blob Storage

Improvements over the fork's implementation:

  • Shared reqwest::Client (fixes per-request resource leak)
  • Token caching with expiry tracking (fixes token re-fetch on every request)
  • Handles both HTTP 200 and 202 responses
  • Exponential backoff for operation polling (2s → 30s cap)
  • No Content-Type header on DELETE requests

Also bumps tar dev-dependency to 0.4.45 (RUSTSEC-2026-0067/0068).

Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com>
…tion

The upstream azure-sdk-for-rust removed azure_iot_deviceupdate from its
workspace and no longer supports SAS URL generation in azure_storage_blob.
This replaces all 5 azure SDK fork dependencies with local implementations
using crates already in the dependency tree (oauth2, reqwest, hmac, sha2).

New module structure under src/device_update/:
- token.rs: OAuth2 client_credentials flow with token caching
- client.rs: ADU REST client (import + delete with exponential backoff)
- blob_sas.rs: HMAC-SHA256 Service SAS URL generation

Also bumps tar dev-dependency to 0.4.45 to fix RUSTSEC-2026-0067/0068.

Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

@HarryWaschkeit HarryWaschkeit left a comment

Choose a reason for hiding this comment

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

Code Review

Overall this is a solid refactor with genuine improvements (shared reqwest::Client, token caching, correct SAS signing, better HTTP status handling). A few real issues below.


🐛 Bugs

1. .gitignore ignores itself
The PR adds .gitignore as an entry in .gitignore. Current clones are unaffected (the file is already tracked), but new clones will see .gitignore as untracked — effectively hiding all ignore rules from git status. This is an AI tooling artifact that leaked into the PR.

2. client.rs — infinite polling loop with no escape hatch
poll_operation uses exponential backoff (capped at 30s) but has no maximum iteration count or wall-clock timeout. If the ADU service returns Running/NotStarted/Unknown indefinitely (service bug, stuck deployment, etc.), the CLI hangs forever. Suggest wrapping the loop in tokio::time::timeout or adding a max-attempts counter.


🔒 Reliability

3. No request timeout on reqwest::Client (client.rs)
reqwest::Client::new() has no timeout. Slow or unresponsive ADU endpoints will cause import/delete/poll to hang indefinitely. Use reqwest::ClientBuilder with a timeout.

4. No timeout on token HTTP client (token.rs)
oauth2::reqwest::ClientBuilder correctly disables redirects but doesn't set a timeout. If the Azure AD token endpoint is unreachable, get_token() will hang.


📝 Nits

5. Stale comment in main.rs

// storage_account_client logs cleartext credentials, the others are just unnecessarily verbose.

This referred to the old Azure SDK client that was just removed. The current reqwest filter has no credential-leak risk — update or remove the comment.

6. blob_sas.rs — version comment mismatch
The comment says "StringToSign for version 2020-12-06 and later" but SAS_VERSION is "2022-11-02". The format is identical across these versions, but the comment should reference the correct version.


💬 Worth discussing

7. AI tooling files in project .gitignore
CLAUDE.md, GEMINI.md, setup-ai.sh, .user-context.md are developer-local AI assistant files. They don't belong in the shared project .gitignore — each developer should add them to their global ~/.gitignore_global instead.

8. project-context.md committed to repo
Fine if intentional and team-approved. The file's header comment explains its purpose clearly.


✅ What looks good

  • StringToSign in blob_sas.rs is correct per the Azure 2020-12-06+ spec (15 \n separating 16 fields, all optional fields correctly empty)
  • Token caching with Mutex<Option<CachedToken>> and 60s expiry margin is correct and thread-safe
  • Single shared reqwest::Client in DeviceUpdateClient is the right pattern
  • HTTP 200 + 202 handling and OperationStatus::Unknown passthrough are proper defensive patterns
  • No Content-Type header on DELETE is correct
  • Cargo.lock reduction (~500 lines) looks legitimate

- Remove .gitignore self-referencing entry
- Add 10-minute timeout to operation polling loop
- Add 30s request timeout to reqwest::Client
- Add 30s timeout to OAuth2 token HTTP client
- Remove stale comment about storage_account_client in main.rs
- Fix SAS version comment to reference sv=2022-11-02

Signed-off-by: Jan Zachmann <50990105+JanZachmann@users.noreply.github.com>
@JanZachmann JanZachmann merged commit cf1abaa into omnect:main Apr 8, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants