Skip to content

feat: Add Azure provider support#258

Open
IkeM-L wants to merge 16 commits into
finos:mainfrom
hmxlabs:IML/azure-provider
Open

feat: Add Azure provider support#258
IkeM-L wants to merge 16 commits into
finos:mainfrom
hmxlabs:IML/azure-provider

Conversation

@IkeM-L

@IkeM-L IkeM-L commented Jun 19, 2026

Copy link
Copy Markdown

The Azure portion of #240

@IkeM-L IkeM-L requested a review from a team as a code owner June 19, 2026 09:47
@fgogolli fgogolli changed the title Azure support feat: Add Azure provider support Jun 22, 2026

@fgogolli fgogolli 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.

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.

) 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."""

@fgogolli fgogolli Jun 22, 2026

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.

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.

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

@fgogolli fgogolli Jun 22, 2026

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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

Comment thread src/orb/domain/base/value_objects.py Outdated


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

@fgogolli fgogolli Jun 22, 2026

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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

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

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.

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.

base_path = azure_extensions.native_spec.spec_file_base_path

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

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.

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.

@IkeM-L IkeM-L Jun 23, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

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

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.

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.

Comment thread pyproject.toml
"azure-mgmt-resource>=25.0.0",
"azure-mgmt-resource-subscriptions>=1.0.0b1",
]

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.

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 fgogolli 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.

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.

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

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.

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.

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

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.

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.

Comment thread src/orb/domain/base/value_objects.py Outdated


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

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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

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

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.

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.

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

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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.



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

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.

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.

Comment thread pyproject.toml
"httpx>=0.27.0",
"PyYAML>=6.0.0", # Exported API (lazy-loaded)
"jsonschema>=4.17.0",
"azure-core>=1.38.2",

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.

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]"].

if len(name) <= 64:
return name

digest = hashlib.sha1(name.encode("utf-8")).hexdigest()[:8]

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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

@fgogolli

fgogolli commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

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

fgogolli commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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):

  1. VMSS/SingleVM finalize too earlycheck_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.)
  2. Dead scheduler branchtemplate_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:

  1. Auth strategy isn't registeredAzureAuthStrategy is implemented but register_azure_extensions() never calls AuthRegistry.register("azure", …), so it's unreachable via lookup.
  2. 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).
  3. 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 SHA1SHA256 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!

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

Copy link
Copy Markdown
Contributor

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 IkeM-L force-pushed the IML/azure-provider branch from e306b44 to 0395164 Compare July 15, 2026 13:19
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.

3 participants