Skip to content

Enhance address space handling to accept numeric CIDR masks - #4734

Open
James Chapman (JC-wk) wants to merge 58 commits into
mainfrom
jc-wk/address-space-size-cidr
Open

Enhance address space handling to accept numeric CIDR masks#4734
James Chapman (JC-wk) wants to merge 58 commits into
mainfrom
jc-wk/address-space-size-cidr

Conversation

@JC-wk

@JC-wk James Chapman (JC-wk) commented Oct 27, 2025

Copy link
Copy Markdown
Collaborator

Resolves #4733

What is being addressed

Currently IP ranges are allocated based on t-shirt sizing s/m/l this introduces CIDR ranges to allow more granular subnet allocation

How is this addressed

  • Accepts cidr values as strings from "16" to "29" for subnet creation
  • Updates documentation
  • Updates CHANGELOG.md
  • Updates API version
  • Adds Tests

@github-actions

github-actions Bot commented Oct 27, 2025

Copy link
Copy Markdown

Unit Test Results

766 tests   766 ✅  8s ⏱️
  1 suites    0 💤
  1 files      0 ❌

Results for commit a6838f6.

♻️ This comment has been updated with latest results.

@JC-wk

Copy link
Copy Markdown
Collaborator Author

I've tested allocating a /27 and that worked in my environment

image

@JC-wk
James Chapman (JC-wk) marked this pull request as ready for review October 28, 2025 19:39
@JC-wk
James Chapman (JC-wk) requested a review from a team as a code owner October 28, 2025 19:39
Copilot AI balanced review requested due to automatic review settings May 22, 2026 13:39

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.

Pull request overview

Adds support in the API for requesting auto-assigned address spaces using numeric CIDR prefix lengths (as strings), extending beyond the existing small/medium/large presets while keeping backwards compatibility.

Changes:

  • Extend address space allocation logic to accept numeric CIDR masks (string values) and validate allowed ranges.
  • Add repository tests for numeric CIDR prefix requests.
  • Update workspace authoring documentation and the project changelog to describe the new behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
docs/tre-workspace-authors/authoring-workspace-templates.md Documents numeric CIDR mask support for address_space_size when requesting additional address spaces.
CHANGELOG.md Records the enhancement in the unreleased changelog.
api_app/tests_ma/test_db/test_repositories/test_workpaces_repository.py Adds tests covering successful numeric CIDR prefix requests.
api_app/models/schemas/workspace_template.py Updates sample schema description text to mention numeric CIDR masks.
api_app/db/repositories/workspaces.py Implements numeric CIDR prefix parsing/validation and routes it into new CIDR allocation.

Comment thread api_app/db/repositories/workspaces.py Outdated
Comment thread api_app/db/repositories/workspaces.py Outdated
Comment thread api_app/models/schemas/workspace_template.py

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.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

@maxmartin-cgi

Copy link
Copy Markdown
Collaborator

/test-extended

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown

🤖 pr-bot 🤖

🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/31003421731 (with refid c0294657)

(in response to this comment from maxmartin-cgi)

Copilot AI review requested due to automatic review settings August 10, 2026 11:12

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.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 17, 2026 12:13

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.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

templates/workspaces/base/template_schema.json:52

  • The phrase “CIDR value” is ambiguous and can be interpreted as including a leading slash (e.g. “/23”), but the implementation accepts a numeric mask string (e.g. “23”). Update the description to explicitly say it accepts a CIDR mask length as a string (e.g. "23"), and (optionally) include the supported numeric range to match validation.
      "description": "Network address size as a CIDR value or (small /24, medium /22, large /16 or custom with an IP range e.g. 10.2.1.0/25) to be used by the workspace.",

api_app/models/schemas/workspace_template.py:26

  • These descriptions don’t mention the newly supported numeric CIDR mask strings (e.g. "19"/"29") and also use the ambiguous “CIDR value” wording. Update both descriptions to explicitly document numeric mask-length strings (and ideally the validated range) so API consumers can discover the feature from the schema.
                description="Network address size as a CIDR value or (small /24, medium /22, large /16 or custom with an IP range e.g. 10.2.1.0/25) to be used by the workspace.")

api_app/models/schemas/workspace_template.py:75

  • These descriptions don’t mention the newly supported numeric CIDR mask strings (e.g. "19"/"29") and also use the ambiguous “CIDR value” wording. Update both descriptions to explicitly document numeric mask-length strings (and ideally the validated range) so API consumers can discover the feature from the schema.
                        "description": "Network address size as a CIDR value or (small /24, medium /22, large /16 or custom with an IP range e.g. 10.2.1.0/25) to be used by the workspace."

templates/workspaces/unrestricted/template_schema.json:58

  • Using raw numeric strings in enum may create a confusing UX in schema-driven UIs (values show as “24”, “23”, etc. without context). Consider switching to oneOf entries with const + title (e.g. title “/24 (256 IPs)”) so the UI is self-explanatory while still sending the expected string value.
      "enum": [
        "24",
        "23",
        "22",
        "21",
        "20",
        "19",
        "18",
        "17",
        "16",
        "small",
        "medium",
        "large",

Comment thread api_app/tests_ma/test_api/test_routes/test_workspaces.py
Copilot AI review requested due to automatic review settings August 17, 2026 12:18

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.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (4)

api_app/tests_ma/test_api/test_routes/test_workspaces.py:822

  • status.HTTP_422_UNPROCESSABLE_CONTENT is likely not defined in the FastAPI/Starlette status module (commonly it’s HTTP_422_UNPROCESSABLE_ENTITY). Also, HTTPException(detail=...) typically returns a JSON body like {"detail": "..."}, so asserting against response.text is fragile; assert against response.json()["detail"] (or the project’s established error envelope) instead.
        response = await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE_SERVICE, workspace_id=WORKSPACE_ID), json=workspace_service_input)

        assert response.status_code == status.HTTP_422_UNPROCESSABLE_CONTENT
        assert response.text == "'address_space_size' numeric value must be between 16 and 29"

templates/workspaces/base/template_schema.json:66

  • The API-side validation allows numeric masks "16" through "29", but this template schema only enumerates "16""24". If this schema is used for validation/UI generation, it will prevent consumers from using "25""29" even though the API supports it (and there’s a new test for "29"). Consider either (a) expanding the enum to include "25""29" in all affected template schemas, or (b) constraining the API validation/docs to match the template-supported range so behavior is consistent end-to-end.
    "address_space_size": {
      "type": "string",
      "title": "Address space size",
      "description": "Network address size as a CIDR value or (small /24, medium /22, large /16 or custom with an IP range e.g. 10.2.1.0/25) to be used by the workspace.",
      "default": "small",
      "enum": [
        "24",
        "23",
        "22",
        "21",
        "20",
        "19",
        "18",
        "17",
        "16",
        "small",
        "medium",
        "large",

docs/tre-workspace-authors/authoring-workspace-templates.md:115

  • These statements read as contradictory: line 114 says workspace templates are configured to support up to "24", while line 115 says you may configure a template up to "29". Recommend clarifying that "29" is only possible if the template schema/enum is extended accordingly (or update the actual schemas to match) so authors understand what is supported “out of the box” vs what requires template changes.
This parameter accepts the presets `small` (/24), `medium` (/22), `large` (/16), the literal value `custom` together with an explicit `address_space` CIDR (e.g. `10.2.1.0/25`), or a numeric CIDR mask as a string.
The API has support for allocating CIDR subnet masks from "16" to "29". Workspace templates are configured to support from "16" (65,536 IP addresses) to "24" (256 IP addresses).
Depending on the workspace service you are deploying you may configure a template with a CIDR up to "29" which has only 3 usable IP addresses as Azure reserves the first four and last address of every subnet.

api_app/db/repositories/workspaces.py:188

  • This error message doesn’t guide callers toward valid values, especially now that numeric masks are supported. Consider including the accepted presets (small|medium|large|custom), and/or the numeric range (1629) in the message to reduce trial-and-error and improve API UX.
        raise InvalidInput(f"Invalid 'address_space_size': {address_space_size}")

Copilot AI review requested due to automatic review settings August 17, 2026 13:14

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.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (4)

api_app/tests_ma/test_api/test_routes/test_workspaces.py:822

  • HTTPException(detail=...) responses are typically JSON ({"detail": "..."}) in FastAPI/Starlette, so asserting response.text equals the raw message is likely to fail (the body will usually be JSON). Prefer asserting response.json()["detail"] == ..., or (if the intent is plain text errors) change the route to return a PlainTextResponse for this error path for consistency.
        response = await client.post(app.url_path_for(strings.API_CREATE_WORKSPACE_SERVICE, workspace_id=WORKSPACE_ID), json=workspace_service_input)

        assert response.status_code == status.HTTP_422_UNPROCESSABLE_CONTENT
        assert response.text == "'address_space_size' numeric value must be between 16 and 29"

templates/workspaces/base/template_schema.json:66

  • The description is misleading/inconsistent with the allowed values: address_space_size is not a “CIDR value” (e.g. 10.2.1.0/25), and the enum values are mask-length strings like "24", not strings like "/24". Recommend rewording to clearly state accepted inputs (presets or a numeric mask length string like "23"), and separately describe that "custom" requires address_space to be an explicit CIDR (e.g. 10.2.1.0/25).
      "description": "Network address size as a CIDR value or (small /24, medium /22, large /16 or custom with an IP range e.g. 10.2.1.0/25) to be used by the workspace.",
      "default": "small",
      "enum": [
        "24",
        "23",
        "22",
        "21",
        "20",
        "19",
        "18",
        "17",
        "16",
        "small",
        "medium",
        "large",

docs/tre-workspace-authors/authoring-workspace-templates.md:115

  • Lines 114–115 read as contradictory: they state templates are configured only for "16""24" but then say you may configure up to "29". Suggest clarifying the distinction (e.g., “the built-in templates enumerate 16–24, but template authors can extend their own schema to 29” or similar), so readers understand what is supported by the API vs what the shipped templates allow by default.
This parameter accepts the presets `small` (/24), `medium` (/22), `large` (/16), the literal value `custom` together with an explicit `address_space` CIDR (e.g. `10.2.1.0/25`), or a numeric CIDR mask as a string.
The API has support for allocating CIDR subnet masks from "16" to "29". Workspace templates are configured to support from "16" (65,536 IP addresses) to "24" (256 IP addresses).
Depending on the workspace service you are deploying you may configure a template with a CIDR up to "29" which has only 3 usable IP addresses as Azure reserves the first four and last address of every subnet.

CHANGELOG.md:6

  • The changelog entry is added above the 0.29.0 section, but api_app/_version.py is bumped to 0.27.0 in this PR. If the changelog is meant to track product releases, please file this entry under the correct version section (or add an “Unreleased” header if that’s the convention) and ensure version bumps align with the changelog structure.
* Allow numeric CIDR masks in `address_space_size` (e.g. "23") when requesting auto-assigned address spaces; accepts numeric strings and validates the mask range. ([#4733](https://github.com/microsoft/AzureTRE/issues/4733))

Copilot AI review requested due to automatic review settings August 21, 2026 09:44

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.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Suppressed comments (1)

api_app/service_bus/deployment_status_updater.py:180

  • The new address-space release state machine is not covered by the existing deployment-status updater tests. Add cases for single-step finalization, multi-step pending/finalization ordering, and ETag retry/failure so CIDRs cannot be released early or left permanently reserved without detection.
            if (step_to_update.resourceType == ResourceType.Workspace
                    and step_to_update.resourceAction == RequestAction.Upgrade
                    and operation.action == RequestAction.UnInstall):
                if not await self._finalize_pending_workspace_address_space(resource_to_persist, operation):
                    return False

            if (step_to_update.templateStepId == "main"
                    and operation.action == RequestAction.UnInstall
                    and step_to_update.resourceType == ResourceType.WorkspaceService):
                cleanup_succeeded = await self._finalize_workspace_address_space(resource_to_persist, operation) \
                    if is_last_step else await self._mark_workspace_address_space_pending(resource_to_persist, operation)
                if not cleanup_succeeded:
                    return False

Comment thread api_app/service_bus/deployment_status_updater.py Outdated
Comment thread api_app/service_bus/deployment_status_updater.py Outdated
Copilot AI review requested due to automatic review settings August 21, 2026 12:02

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.

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated no new comments.

Suppressed comments (1)

api_app/tests_ma/test_service_bus/test_deployment_status_update.py:448

  • This test only verifies that cleanup is skipped before the main uninstall step. The new successful-uninstall path that removes the service CIDR, retries ETag conflicts, and controls message completion is never exercised. Please add positive cleanup and retry/failure tests so an incorrect address release or permanently retried status message is caught.
    with patch.object(status_updater, "_finalize_workspace_address_space", new_callable=AsyncMock) as finalize:
        assert await status_updater.update_status_in_database(message) is True

    finalize.assert_not_awaited()

Copilot AI review requested due to automatic review settings August 21, 2026 15:31

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.

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.

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.

Update address_space_size property to allow cidr subnets

5 participants