Skip to content

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

Merged
merged 2 commits into from
Dec 9, 2019
Merged

AzureFirewall with IpGroups (LATEST) #10674

merged 2 commits into from
Dec 9, 2019

Conversation

ssripadham
Copy link
Contributor

@ssripadham ssripadham commented Dec 7, 2019

Description

- `Azure Firewall will support IpGroups can be used in the network/application/dnat rules as described below.
  • As a source or destination address in AZFW network rules
  • As a source address in AZFW application rules
  • As a source address in DNAT rules. `

1. Azure Firewall Application Rules

     "AzureFirewallApplicationRule": {
         "properties": {
           "name": {
             "type": "string",
             "description": "Name of the application rule."
           },
           "description": {
             "type": "string",
             "description": "Description of the rule."
           },
           ...
           "sourceIpGroups": {
             "type": "array",
             "description": "List of source IP Groups for this rule.",
             "items": {
               "type": "string"
             }
           },
       ...
     }
    }

2. Azure Firewall NAT Rules

   "AzureFirewallNatRule": {
         "properties": {
           "name": {
             "type": "string",
             "description": "Name of the NAT rule."
           },
           "description": {
             "type": "string",
             "description": "Description of the rule."
           },
          ...

    "sourceIpGroups": {
      "type": "array",
      "description": "List of source IP Groups for this rule.",
      "items": {
        "type": "string"
      }
    }
       …
   }
   }

3. Azure Firewall Network Rules

    "AzureFirewallNetworkRule": {
         "properties": {
           "name": {
             "type": "string",
             "description": "Name of the network rule."
           },
           "description": {
             "type": "string",
             "description": "Description of the rule."
           },
          ...
           "sourceIpGroups": {
             "type": "array",
             "description": "List of source IP Groups for this rule.",
             "items": {
               "type": "string"
             }
           },
           "destinationIpGroups": {
             "type": "array",
             "description": "List of destination IP Groups for this rule.",
             "items": {
               "type": "string"
             }
           },
       ...
    }
   }

Checklist

  • I have read the Submitting Changes section of CONTRIBUTING.md
  • The title of the PR is clear and informative
  • The appropriate ChangeLog.md file(s) has been updated:
    • For any service, the ChangeLog.md file can be found at src/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
    • A snippet outlining the change(s) made in the PR should be written under the ## Upcoming Release header -- no new version header should be added
  • The PR does not introduce breaking changes
  • If applicable, the changes made in the PR have proper test coverage
  • For public API changes to cmdlets:
    • a cmdlet design review was approved for the changes in this repository (Microsoft internal only)
    • the markdown help files have been regenerated using the commands listed here

@@ -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))
Copy link
Collaborator

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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.

Copy link
Collaborator

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

// One of SourceAddress or SourceIpGroup must be present
if ((SourceAddress == null) && (SourceIpGroup == null))
{
throw new ArgumentException("Either SourceAddress or SourceIpGroup is required.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -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.");
Copy link
Collaborator

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"?

@VeryEarly VeryEarly self-requested a review December 9, 2019 06:44
@VeryEarly VeryEarly merged commit d4b0a2c into master Dec 9, 2019
@dingmeng-xue dingmeng-xue deleted the ipgroup_azfw_branch branch May 13, 2022 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants