-
Notifications
You must be signed in to change notification settings - Fork 381
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
feat: Updated Network Manager module to support Routing Configuration feature #4096
base: main
Are you sure you want to change the base?
Conversation
…cies in Bicep files
… access values and improve descriptions
…and improve routing configuration module
…etwork manager module and tests
Important The "Needs: Triage 🔍" label must be removed once the triage process is complete! Tip For additional guidance on how to triage this issue/PR, see the BRM Issue Triage documentation. |
Important If this is a module-related PR, being submitted by the sole owner of the module, the AVM core team must review and approve it (as module owners can't approve their own PRs). To indicate this PR needs the core team''s attention, apply the "Needs: Core Team 🧞" label! The core team will only review and approve PRs that have this label applied! |
}[]? | ||
|
||
type networkManagerScopeAccessType = ('Connectivity' | 'SecurityAdmin')[] | ||
type networkManagerScopeAccessType = ('Connectivity' | 'SecurityAdmin' | 'Routing')[]? |
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.
type networkManagerScopeAccessType = ('Connectivity' | 'SecurityAdmin' | 'Routing')[]? | |
type networkManagerScopeAccessType = ('Connectivity' | 'SecurityAdmin' | 'Routing') |
As per soon to be released specs, both the array
and nullable
indicators should be set at the parameter, not the UDT-side. The reasoning is simply to make it easier to understand a parameter/property by looking at it.
So this change should go along with updating the parameter to
param networkManagerScopeAccesses networkManagerScopeAccessType[]?
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.
Note: The same idea applies to all other types
params: { | ||
name: routingConfiguration.name | ||
networkManagerName: networkManager.name | ||
description: routingConfiguration.?description ?? '' |
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.
Are the fallback defaults here intended? If not provided for a nullable parameter they just won't be used. In other words: I'd only recommend setting a fallback default here if you definitely want to not pass null
.
name: ruleCollection.name | ||
description: ruleCollection.?description ?? '' | ||
appliesTo: ruleCollection.appliesTo | ||
disableBgpRoutePropagation: ruleCollection.?disableBgpRoutePropagation ?? true |
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.
disableBgpRoutePropagation: ruleCollection.?disableBgpRoutePropagation ?? true | |
disableBgpRoutePropagation: ruleCollection.?disableBgpRoutePropagation |
Redundant as the deployment will default to the parameters default (true
) regardless if not provided.
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.
Great work @ahmadabdalla, as always. Really only have an ask regarding the declaration of arrays
& nullable
for UDTs if you can make room for it :)
Description
Azure Virtual Network Manager (AVNM) recently introduced a new feature that allows users to configure routing settings for their virtual networks. See concept-user-defined-route documentation.
ℹ️ Routing Configuration is Generally Available in many Azure regions: See UDR Management - General Availability. Given the optional usage of this feature, This should not impact existing usage of AVNM.
While enabling this feature, the following had to be updated on the module:
routing-configuration
,rule-collection
andrule
.2024-05-01
as per (https://learn.microsoft.com/en-us/azure/templates/microsoft.network/networkmanagers?pivots=deployment-language-bicep)roleAssignments
andlocks
.None
feature mode to support capabilities like IPAM and Virtual Network Verifier.IPAM Pool Contributor
toIPAM Pool User
, Still keeps the same GUID.memberType
for Subnets for Network Groups to support the new routing feature. However, set the default to virtual network as it is the common scenario.import
functionality to reduce code duplication and import types from child module to parents.Pipeline Reference
Type of Change
version.json
:version.json
.version.json
.Checklist
Set-AVMModule
locally to generate the supporting module files.