feat: Add Azure provider support by IkeM-L · Pull Request #258 · finos/open-resource-broker

IkeM-L · 2026-06-19T09:47:25Z

The Azure portion of #240

fgogolli

Scope: VMSS Flexible + Uniform + SingleVM + CycleCloud, with dedicated test files across all three handlers. The dominant action needed before merge is a rebase onto dd4c390 - main has moved substantially since this branch was cut, and several contracts introduced there need to be integrated.

Four-PR plan for context

The broader merge sequence is: #253 (provider registries + OperationOutcome + CheckHostsStatusResult, already on main) → #256 (Azure scaffold, may be superseded) → #257 (AWS rebase alignment) → this PR (#258, Azure provider). That means this PR should be rebased to pick up dd4c390 directly, absorb the Azure-specific items that were in the original #256 scaffold, and align with the contracts #253 introduced.

Critical correctness issue - VMSS fulfillment_final

VMSSHandler.acquire_hosts_async returns fulfillment_final: True with instances: [] and provisioning_state: "creating". In provisioning_orchestration_service.py, fulfillment_final=True short-circuits the len(instances) >= count check and marks the request terminal immediately - on the first cycle, with zero machines registered. VMSS provisions asynchronously, so the intent is clearly "poll via status checks", but the flag says the opposite: "stop, we're done." Every VMSS acquire request will be silently abandoned. The test suite doesn't catch this because test_vmss_handler.py asserts provisioning_state == "creating" and operation_status == "submitted" but never runs the result through the orchestration service.

Two paths to fix: set fulfillment_final: False and implement check_hosts_status returning the count-based terminal signal, or keep the async-only approach but ensure the orchestration path reads the async result correctly. See inline on vmss_handler.py.

Rebase-alignment items from #253

After rebasing onto dd4c390:

check_hosts_status on AWS handlers now returns CheckHostsStatusResult(instances, fulfilment=ProviderFulfilment(...)) and is called via asyncio.to_thread. The AWS strategy unwraps .instances and .fulfilment separately and writes provider_fulfilment into metadata. Without this, all in-flight AWS acquire requests will stall on the first status poll post-merge. The Azure strategy uses a fully async dispatch path (never hits the sync check_hosts_status contract), but the AWS path must be restored.
OperationOutcome (Accepted, Completed, Failed, RequiresFollowUp) is the typed return contract for handler operations on main. The Azure handlers return AzureAcquireHostsResult (a TypedDict) - this is a parallel type system rather than an extension of the agreed type. A decision is needed: extend OperationOutcome for Azure, or document why the Azure strategy is exempt.
provider_config: Optional[BaseModel] (populated via TemplateExtensionRegistry) is the field type on TemplateDTO on main. The PR uses dict[str, Any]. After rebase, reconcile the serialisation path.
DefaultsLoaderRegistry in config/loader.py: main iterates the registry; the PR hardcodes an {"aws": ..., "azure": ...} dict. A third provider registered via the registry at runtime will have its defaults silently ignored.

Azure-specific items to address

vm_size, vm_sizes, placement_split_strategy, placement_primary_share_percent, placement_regions, placement_zones are currently on the base Template. AWS uses none of them. They belong on AzureTemplate(Template) in providers/azure/domain/template/. See inline on template_aggregate.py.
PlacementSplitStrategy (GREEDY/HYBRID) encodes Azure spot-placement scoring. It should live in providers/azure/domain/ rather than domain/base/value_objects.py.
AzureAllocationStrategy in providers/azure/domain/template/value_objects.py is correctly placed. AllocationStrategy in the shared domain is also fine since AWS uses it too - but see inline for a minor naming note.
AzureAuthStrategy exists but is not registered with AuthRegistry. AWS does this in register_aws_auth_strategies(). See inline on registration.py.
CycleCloudRequestContext.to_metadata() writes cyclecloud_credential_path into persisted request metadata. See inline on cyclecloud_handler.py.
AzureNativeSpecService._load_spec_file has no path containment check. AWS has the same gap. See inline - fix both in one go.
azure-mgmt-resource-subscriptions>=1.0.0b1 is a pre-release dependency. See inline on pyproject.toml.
ProviderType has PROVIDER1/Provider2 placeholder members. Drop before merge. See inline on provider_interfaces.py.
azure_client.py:644-647: try/except Exception: raise is a no-op; the debug log after it is unreachable. See inline.
template_catalog_service.py:46: the TODO comment correctly identifies get_active_strategy() as broken at runtime - either implement it or delete the dead path before merge.
Two logger.info calls in credential_factory.py should be debug. See inline.

Test quality

Almost every Azure test mock uses bare MagicMock() without spec=. The factory helpers make_vmss_azure_client() and make_single_vm_azure_client() in strategy_test_support.py are the root cause - they create unspec'd roots that propagate. An SDK method rename or attribute typo will silently pass. The Azure SDK types (ComputeManagementClient, etc.) are importable at test time. Worth fixing the two factory helpers to propagate spec'd doubles.

Additional gaps: SingleVM quota-error classification is untested through canonical_azure_error_code; the Uniform VMSS check_hosts_status_async native async path stubs _list_vmss_instances_async directly rather than exercising the SDK path; CycleCloud partial-release failure (a CycleCloudNodeError raised inside _resolve_release_node_targets_async) is an unhandled raise path with no test.

Minor

[tool.pycodestyle] max-line-length = 100 in pyproject.toml - pycodestyle isn't in the active linting chain; this is dead config.

fgogolli · 2026-06-22T15:09:43Z

+            ) from exc
+
+    async def check_hosts_status_async(self, request: Request) -> list[AzureHandlerStatusResult]:
+        """Async status query for VMSS members using the Azure async Compute SDK."""


After rebase onto dd4c390, check_hosts_status on AWS handlers returns CheckHostsStatusResult(instances, fulfilment=ProviderFulfilment(...)) rather than a plain list. The AWS strategy unwraps .fulfilment and writes provider_fulfilment into request metadata; without that key, RequestStatusService raises ProviderContractError on every status poll for in-flight AWS requests. The Azure strategy routes via a fully async dispatch and never calls the sync check_hosts_status directly, so there's no runtime failure for Azure, but the PR's AWS strategy path needs to be reconciled with the main contract.

fgogolli · 2026-06-22T15:09:43Z

+                    "operation_status": "submitted",
+                    "error_codes": [],
+                    "fulfillment_final": True,
+                },


BLOCKER - fulfillment_final: True with instances: [] and provisioning_state: "creating" is contradictory. provisioning_orchestration_service.py computes is_final = (not has_capacity_error and len(instances) >= count) or fulfillment_final. With fulfillment_final=True this short-circuits immediately: the request is marked terminal on the first poll cycle with zero machines registered and is never polled again. VMSS provisions asynchronously, so the intent is clearly "poll via status checks", but this flag signals "done". Set fulfillment_final: False here so the count-based path drives termination. The existing tests don't catch this because none of them run the acquire result through the orchestration service.

VMSS and SingleVM do set fulfillment_final=True on create submission. This is intentional, it stops the provisioning retry/top-up loop so no more create attempts are made after submitting the provider resource while Azure is creating the resources. However, the persisted request-status update still sets a successful request with resource_ids and zero instances to IN_PROGRESS, not completed or failed.

The request does not resolve immediately with zero machines. It remains pending for later status synchronization. It is updated to completed or failed later via the follow up context.

fgogolli · 2026-06-22T15:09:43Z

+
+
+class AllocationStrategy(str, Enum):
+    """Allocation strategy enumeration."""


AllocationStrategy is legitimately shared - AWS EC2 Fleet uses LOWEST_PRICE/DIVERSIFIED/CAPACITY_OPTIMIZED and Azure uses SPOT_PLACEMENT_SCORE, so keeping it in domain/base/ is correct. PlacementSplitStrategy (GREEDY/HYBRID) is Azure-only: it encodes Azure spot-placement scoring splits and is unused outside providers/azure/. Move it to providers/azure/domain/ before merge.

This does not seem to be true? I have synced main to get the update, but it does not have an AllocationStrategy.

fgogolli · 2026-06-22T15:09:43Z

+                "fleet_errors": fleet_errors,
+                "cyclecloud_url": base_url,
+                "cyclecloud_credential_path": credential_path,
+                "cyclecloud_verify_ssl": verify_ssl,


Security (Medium) - cyclecloud_credential_path is written into request metadata that gets persisted. Any API response surfacing request metadata (e.g. GET /requests/{id}/status) exposes a host filesystem path to callers. Store a logical alias or config key instead of the raw path. The field is already repr=False on CycleCloudCredentialData.password; applying the same treatment here would at least prevent it appearing in logs, but the metadata persistence risk remains.

fgogolli · 2026-06-22T15:09:43Z

+            base_path = azure_extensions.native_spec.spec_file_base_path
+
+        return read_json_file(f"{base_path}/{file_path}")
+


Security (Medium) - file_path comes from template.provider_api_spec_file, which is operator-supplied. No check that os.path.realpath(f"{base_path}/{file_path}") starts with os.path.realpath(base_path) before the read. A value like ../../etc/passwd reads outside the intended directory. AWS AWSNativeSpecService._load_spec_file has the same pattern, so this is a parity fix for both providers: validate containment before calling read_json_file.

Do you want me to fix AWS as well? I assumed copying this pattern was fine as it is operator provided data, sorry.

fgogolli · 2026-06-22T15:09:43Z

+        logger.error(format_import_error("azure-identity", exc))
+        raise
+    logger.info("Azure DefaultAzureCredential initialised")
+    return credential


Use logger.debug rather than logger.info here. Credential initialisation is operational noise at INFO level, and if a client_id or similar field is ever interpolated into this message it becomes an info-disclosure risk in environments where INFO logs are forwarded externally.

fgogolli · 2026-06-22T15:09:43Z

+    "azure-mgmt-resource>=25.0.0",
+    "azure-mgmt-resource-subscriptions>=1.0.0b1",
 ]



Security (Low) - azure-mgmt-resource-subscriptions>=1.0.0b1 is a pre-release dependency with no stable release. Beta SDKs carry API-stability and supply-chain risk in production. Pin to the specific beta version currently validated rather than an open >= floor, and track the GA release to migrate.

fgogolli

Supplemental inline comments to accompany the updated top-level review. The main review covers the VMSS fulfillment_final correctness issue, the AWS strategy contract reconciliation, and the PlacementSplitStrategy move. These six anchor: ProviderType placeholder cleanup, vm_size/vm_sizes move to AzureTemplate, dead try/except in azure_client.py, broken get_active_strategy() path in template_catalog_service.py, and the missing AuthRegistry/DefaultsLoaderRegistry registrations.

fgogolli · 2026-06-22T16:38:22Z

    AWS = "aws"
+    AZURE = "azure"
    PROVIDER1 = "provider1"
    Provider2 = "provider2"


PROVIDER1 = "provider1" and Provider2 = "provider2" are placeholder stubs. They'll appear in every CLI completion, API response listing provider types, and any exhaustive-match check. Drop both before merge - if additional providers are planned, they can be added when the implementation exists.

fgogolli · 2026-06-22T16:38:22Z

    # Instance configuration
    instance_type: Optional[str] = None
+    vm_size: Optional[str] = None
+    vm_sizes: list[str] = Field(default_factory=list)


vm_size and vm_sizes are Azure-specific field names - AWS uses instance_type/instance_types. The base Template aggregate is shared across all providers; adding Azure vocabulary here means every GCP or other provider inherits these fields without asking. Move vm_size, vm_sizes, and the placement_* fields below to AzureTemplate(Template) in providers/azure/domain/template/. AzureTemplate already redefines vm_size as a required str, so the move is mostly mechanical.

fgogolli · 2026-06-22T16:38:22Z

+
+
+class PlacementSplitStrategy(str, Enum):
+    """Placement-plan split strategy enumeration."""


PlacementSplitStrategy encodes Azure spot-placement scoring split logic (greedy vs hybrid). Nothing outside providers/azure/ uses it. Move to providers/azure/domain/ so the shared domain layer stays provider-agnostic. AllocationStrategy above is legitimately shared and can stay.

While this is only currently implemented for azure, nothing about it seems inherently azure?

fgogolli · 2026-06-22T16:38:22Z

+        try:
+            await resource.close()
+        except Exception:
+            raise


except Exception: raise is a no-op - it catches and immediately re-raises without doing anything. The self._logger.debug(...) call on the next line is unreachable on any exception path. Remove the try/except entirely and keep just the await resource.close() and the debug log.

fgogolli · 2026-06-22T16:38:22Z

+            scheduler_strategy = cast(
+                SchedulerTemplateStrategy | None,
+                scheduler_registry.get_active_strategy(),  # type: ignore[attr-defined]
+            )


The TODO comment correctly identifies that get_active_strategy() doesn't exist on SchedulerRegistry and raises AttributeError at runtime - the outer try/except silently swallows it and always falls back to hardcoded templates. This code path has never executed. Either implement get_active_strategy() on the registry or delete the unreachable branch and call get_fallback_templates() directly. Shipping known-broken code with a TODO is a maintenance trap.

This is an issue shared with AWS, but I wasn't confident enough about the intent to implement the method myself. I wanted to ensure that the Azure provider did not behave incorrectly once it was implemented.

fgogolli · 2026-06-22T16:38:22Z

+
+
+def register_azure_extensions(logger: Optional["LoggingPort"] = None) -> None:
+    """Register Azure template extensions with the global registry."""


register_azure_extensions() wires CLISpecRegistry, TemplateExtensionRegistry, and ProviderSettingsRegistry - three of the five registries main introduced. AuthRegistry and DefaultsLoaderRegistry are not called. AWS does both in register_aws_auth_strategies() and registers AWSDefaultsLoader in registration.py. AzureAuthStrategy exists in providers/azure/auth/ but is unreachable via the AuthRegistry lookup path. Add register_azure_auth_strategies() and at minimum an empty AzureDefaultsLoader so the registry contracts are fulfilled symmetrically.

fgogolli · 2026-06-22T16:38:22Z

+    "httpx>=0.27.0",
    "PyYAML>=6.0.0", # Exported API (lazy-loaded)
    "jsonschema>=4.17.0",
+    "azure-core>=1.38.2",


After rebase, please put the Azure SDKs under an [azure] optional-dependencies extra rather than the core [project.dependencies] list. Main has the pattern in place: [aws] exists as an opt-in alias for boto3, [all-providers] is a meta-extra. With 9 azure-* SDKs in core, every non-Azure deployment pulls in ~hundreds of MB of unused transitive deps. Add azure = [...] under [project.optional-dependencies] and extend all-providers to ["orb-py[aws,azure]"].

IkeM-L · 2026-06-23T09:23:29Z

+        if len(name) <= 64:
+            return name
+
+        digest = hashlib.sha1(name.encode("utf-8")).hexdigest()[:8]


This is not used in a security-relevant context, do I still need to address it to satisfy the scanner?

fgogolli · 2026-07-06T12:14:51Z

Now that #285 has merged, this PR needs a rebase with several adjustments.

This PR carries the full #256 shared-code diff. On rebase, please drop the files that have already landed via #285 and pick up only the Azure-specific content.

The --azure-* CLI flags are currently added directly to build_parser, providers_add, and providers_update blocks in cli/args.py. Main now routes provider-specific flags through CLISpecRegistry. Please add an AzureCLISpec class (mirror src/orb/providers/aws/cli/aws_cli_spec.py) that exposes the flags via add_arguments() and register it via the registry.

providers/registration.py on main uses _REGISTERED_PROVIDERS = ["aws"]. Please add "azure" to that list instead of calling register_azure_provider directly. The Azure DI wiring currently in bootstrap/infrastructure_services.py should move to initialize_azure_provider(container) inside the Azure registration module.

Correctness item: AzureProviderStrategy currently declares get_available_credential_sources, test_credentials, get_credential_requirements, and get_cli_provider_config as instance methods. init_command_handler invokes them on the class before any strategy instance exists. Please decorate all four with @classmethod — otherwise orb init --provider-type azure will not discover credentials.

fgogolli · 2026-07-08T13:40:10Z

Huge effort here — a full Azure provider across VMSS, single-VM, and CycleCloud paths, with a self-contained registration.py, a typed AzureTemplate, and ~46% test coverage. Thank you for the thoroughness! 🙏 A few important items to work through:

Two correctness items worth a close look (before merge):

VMSS/SingleVM finalize too early — check_hosts_status returns fulfillment_final: True while instances is still empty and state is creating. The orchestration service checks fulfillment_final before the instance count, so the request is marked terminal on the first poll with zero machines. Returning fulfillment_final: False on the create-submission result lets the poll loop continue until instances actually register. (Same fix in vmss_handler.py and single_vm_handler.py.)
Dead scheduler branch — template_catalog_service.py calls scheduler_registry.get_active_strategy(), which doesn't exist on SchedulerRegistry, so the try/except silently falls back every time. Either wiring up the accessor or removing the branch (calling get_fallback_templates() directly) would make the intent clear.

Sequencing heads-up (rebase-time):

#287 lands on main first and removes provider_data from Template + adds a shared CLI parser. Azure uses vm_size (not instance_type) so the rename is neutral for you, but: (a) the vm_size/placement_* fields added to the shared Template should live on AzureTemplate only; (b) the --azure-* args in cli/args.py will conflict with the shared parser — moving them into AzureCLISpec resolves both.

Worth addressing before merge:

Auth strategy isn't registered — AzureAuthStrategy is implemented but register_azure_extensions() never calls AuthRegistry.register("azure", …), so it's unreachable via lookup.
Azure SDKs in core deps — the nine azure-* packages should move to an [azure] optional-extra (and pin azure-mgmt-resource-subscriptions to the exact 1.0.0b1 rather than an open >= on a pre-release).
A couple of security touch-ups: the CycleCloud credential path is written into request metadata (surfaced via the status API) — a config-key reference would avoid leaking the filesystem path; the native-spec loader could use a realpath containment check; and SHA1 → SHA256 in the deployment-name hash unblocks the Semgrep CI check (no functional change).

Nice-to-haves: get_resource_id_pattern() for ARM IDs; confirming the VMSS cleanup coordinator's background loop is actually started (via start_daemon_services()); dropping the PROVIDER1/Provider2 stubs; downgrading the credential-init logger.info to debug.

This is a big, well-tested contribution and the provider structure is spot on. Let us know where you'd like more detail on any of these!

fgogolli · 2026-07-10T10:34:55Z

Heads-up for when you next rebase on main: an upcoming change (PR #292) adds a bootstrap provider-completeness assertion. At startup it verifies that every provider registered in ProviderRegistry also has matching entries in the five satellite registries:

CLISpecRegistry
FieldMappingRegistry
DefaultsLoaderRegistry
TemplateExtensionRegistry
TemplateExampleGeneratorRegistry

If a provider registers its type but is missing any of these, startup now fails fast with an error naming exactly which registrations are missing (rather than silently degrading at a later call site — which was the root cause of some earlier provider-onboarding bugs).

Practically: make sure your provider's initialize_<name>_provider(...) + register_<name>_services_with_di(...) populate all five registries (the aws and k8s providers in src/orb/providers/*/registration.py are the reference pattern). Once #292 lands we're happy to help wire anything that's missing — just flagging early so a rebase doesn't surprise you with a startup failure. Thanks for the contribution!

IkeM-L requested a review from a team as a code owner June 19, 2026 09:47

fgogolli mentioned this pull request Jun 22, 2026

feat: provider extension points, ProviderFulfilment contract, auth security fixes #253

Merged

23 tasks

fgogolli changed the title ~~Azure support~~ feat: Add Azure provider support Jun 22, 2026

This was referenced Jun 22, 2026

feat: Common code changes required for Azure and GCP support #256

Open

feat: Add Azure and GCP support #240

Closed

fgogolli reviewed Jun 22, 2026

View reviewed changes

fgogolli mentioned this pull request Jun 22, 2026

feat: Add GCP provider support #257

Open

fgogolli reviewed Jun 22, 2026

View reviewed changes

github-advanced-security AI found potential problems Jun 22, 2026

View reviewed changes

fgogolli mentioned this pull request Jul 7, 2026

chore(providers): onboarding prep — unblock Azure/GCP/OCI merges #287

Merged

7 tasks

IkeM-L force-pushed the IML/azure-provider branch from 22a6df0 to a69f326 Compare July 8, 2026 15:13

ikemilian-lewis added 15 commits July 15, 2026 10:51

feat: add Azure provider

f162f35

Resolve type issues after rebase

70c94b7

fix: address PR comments

7ad5b4b

chore: minimize Azure provider rebase surface

75958ca

fix: align Azure provider with main registries

8522b2f

feat(azure): rebase based on Main not Common Code

88e09cb

chore: ruff

958eaf3

fix: register auth

0a93855

fix: ruff check

886ff89

fix: httpx dependency in Azure

9b5e2f9

fix: ruff and toml

738317a

fix: lazy loading and 3.11 dependencies

11a0e57

fix: small issues

cfea023

fix: ci pyright

b1dacaf

fix: rebase error handling and registration

0395164

IkeM-L force-pushed the IML/azure-provider branch from e306b44 to 0395164 Compare July 15, 2026 13:19

fix: split azure runtime and test dependencies

4766a89



		class AllocationStrategy(str, Enum):
		"""Allocation strategy enumeration."""

		base_path = azure_extensions.native_spec.spec_file_base_path

		return read_json_file(f"{base_path}/{file_path}")



		class PlacementSplitStrategy(str, Enum):
		"""Placement-plan split strategy enumeration."""



		def register_azure_extensions(logger: Optional["LoggingPort"] = None) -> None:
		"""Register Azure template extensions with the global registry."""

Uh oh!

Conversation

IkeM-L commented Jun 19, 2026

Uh oh!

fgogolli left a comment • 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!

fgogolli Jun 22, 2026 • 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!

fgogolli Jun 22, 2026 • 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!

fgogolli Jun 22, 2026 • 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!

IkeM-L Jun 23, 2026 • 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!

fgogolli left a comment

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!

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!

fgogolli commented Jul 6, 2026

Uh oh!

fgogolli commented Jul 8, 2026

Uh oh!

fgogolli commented Jul 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fgogolli left a comment •

edited

Loading

fgogolli Jun 22, 2026 •

edited

Loading

fgogolli Jun 22, 2026 •

edited

Loading

fgogolli Jun 22, 2026 •

edited

Loading

IkeM-L Jun 23, 2026 •

edited

Loading