AzureFirewall with IpGroups (LATEST) - #10674
Conversation
| // if (MNM.AzureFirewallNatRCActionType.Dnat.Equals(ActionType)) | ||
| { | ||
| // One of SourceAddress or SourceIpGroup must be present | ||
| if ((SourceAddress == null) && (SourceIpGroup == null)) |
There was a problem hiding this comment.
it looks like you need two parameter sets, SourceAddress and SourceIpGroup will need to be mandatory for each.
There was a problem hiding this comment.
No, we do not want to use the parameter set. It lead us to complication in paramterset definition as in https://github.com/Azure/azure-powershell/pull/10668/files
We are using an approach where both SourceAddress & SourceIpGroup are optional and using checks to make sure atleast one of these is supplied by the user
There was a problem hiding this comment.
We are using similar approach for Destination Addresses in network rule.
There was a problem hiding this comment.
the similar approach is not appropriate either, breaking change fix needed
| // One of SourceAddress or SourceIpGroup must be present | ||
| if ((SourceAddress == null) && (SourceIpGroup == null)) | ||
| { | ||
| throw new ArgumentException("Either SourceAddress or SourceIpGroup is required."); |
There was a problem hiding this comment.
better to put exception message here https://github.com/Azure/azure-powershell/blob/ipgroup_azfw_branch/src/Network/Network/Properties/Resources.resx
| if ((DestinationAddress != null) && (DestinationFqdn != null)) | ||
| { | ||
| throw new ArgumentException("Both DestinationAddress and DestinationFqdns not allowed"); | ||
| throw new ArgumentException("Both DestinationAddress and DestinationFqdns not allowed."); |
There was a problem hiding this comment.
I understand you didn't create this message but this line is confusing, can you put something like "only one of DestinationAddress or DestinationFqdns is allowed"?
Description
1. Azure Firewall Application Rules
2. Azure Firewall NAT Rules
3. Azure Firewall Network Rules
Checklist
CONTRIBUTING.mdChangeLog.mdfile(s) has been updated:ChangeLog.mdfile can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md## Upcoming Releaseheader -- no new version header should be added