Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Azure Bastion Subnet to be /26 (instead of /27) #353

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Conversation

ckittel
Copy link
Member

@ckittel ckittel commented Oct 7, 2022

Fixes: #342

When this reference implementation was created, /27 was the correct size for Azure Bastion. The latest recommendation is /26 -- luckily enough room was left :)

  • Updated the AzureBastionSubnet subnet size, and the related IP table
  • Minor formatting & link changes on the IP table

Copy link
Contributor

@hallihan hallihan left a comment

Choose a reason for hiding this comment

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

Looks good, noticed one inconsistency that would be good to address before merging.

| Azure Application Gateway Subnet | - | [251] | - | - | - | 100 | 100 | 0 | 0 | 5 | 0 | 256 | 256 | 24 | 10.240.5.0/24 | 10.240.5.0 | 10.240.5.255 |
| Gateway Subnet (GatewaySubnet) | - | [27<sup>1</sup>] | - | - | - | 100 | 100 | 0 | 0 | 5 | 0 | 32 | 32 | 27 | 10.200.0.64/27 | 10.200.0.64 | 10.200.0.95 |
| Azure Firewall Subnet (AzureFirewallSubnet) | - | [59] | - | - | - | 100 | 100 | 0 | 0 | 5 | 0 | 64 | 64 | 26 | 10.200.0.0/26 | 10.200.0.0 | 10.200.0.63 |
| Azure Bastion Subnet (AzureBastionSubnet) | - | [50<sup>2</sup>] | - | - | - | 100 | 100 | 0 | 0 | 5 | 0 | 64 | 64 | 26 | 10.200.0.128/26 | 10.200.0.128 | 10.200.0.191 |
Copy link
Contributor

Choose a reason for hiding this comment

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

The superscript here seems to indicate that there is more information below, but # 2 in Additional Considerations is talking about a different subnet. Not new for this PR, but maybe we can clean that up?

Copy link
Member Author

Choose a reason for hiding this comment

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

The superscript link works, it's literally the whole text, when clicked, goes to that data source, but I see what you mean how it could be confused with the numbered list below. Let me fix that! Good idea.

Copy link
Member Author

@ckittel ckittel Oct 7, 2022

Choose a reason for hiding this comment

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

Double fix, removed the (now) unnecessary superscript -- they were used as differentiators before, and also converted the numbered list into a bullet list in eb674cb

@ckittel
Copy link
Member Author

ckittel commented Oct 7, 2022

Thanks for the review, @hallihan!

@ckittel ckittel merged commit 512326b into main Oct 7, 2022
@ckittel ckittel deleted the subnet-size-tweak branch October 7, 2022 19:11
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 AzureBastionSubnet size
2 participants