-
Notifications
You must be signed in to change notification settings - Fork 515
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
Conversation
There was a problem hiding this 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.
networking/topology.md
Outdated
| 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 | |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Thanks for the review, @hallihan! |
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 :)AzureBastionSubnet
subnet size, and the related IP table