-
Notifications
You must be signed in to change notification settings - Fork 4k
AzureFirewall with IpGroups (LATEST) #10674
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
Conversation
src/Network/Network/AzureFirewall/ApplicationRule/NewAzureFirewallApplicationRuleCommand.cs
Outdated
Show resolved
Hide resolved
@@ -90,6 +94,12 @@ public override void Execute() | |||
// Add some validation based on the type of RuleCollection (SNAT will be supported later) | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using similar approach for Destination Addresses in network rule.
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 similar approach is not appropriate either, breaking change fix needed
src/Network/Network/AzureFirewall/NetworkRule/NewAzureFirewallNetworkRuleCommand.cs
Show resolved
Hide resolved
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to put exception message here https://github.com/Azure/azure-powershell/blob/ipgroup_azfw_branch/src/Network/Network/Properties/Resources.resx
@@ -87,13 +102,13 @@ public override void Execute() | |||
// Only one of DestinationAddress or DestinationFqdns is allowed | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.md
ChangeLog.md
file(s) has been updated:ChangeLog.md
file can be found atsrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
## Upcoming Release
header -- no new version header should be added