-
Notifications
You must be signed in to change notification settings - Fork 4k
Support Default Disabled Rules in AppGW WAF Manifest with ComputedDisabledRules Property #28327
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
base: main
Are you sure you want to change the base?
Conversation
…th the new readonly property ComputedDisabledRules
…s validation throughout
Thanks for your contribution! The pull request validation has started. Please revisit this comment for updated status. |
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.
Pull Request Overview
This PR implements support for default disabled rules in Application Gateway WAF (Web Application Firewall) policies by introducing a new read-only property ComputedDisabledRules
. The feature allows WAF rule authors to set rules as disabled by default in the manifest, while users can still override these defaults.
Key changes include:
- Added a new
ComputedDisabledRules
property that shows the effective state of disabled rules - Implemented AutoMapper configuration for the new model classes
- Added comprehensive test coverage for the computed disabled rules functionality
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
PSApplicationGatewayFirewallPolicyManagedRuleSetRuleGroup.cs | New model class for representing rule groups with disabled rules |
PSApplicationGatewayFirewallPolicyManagedRuleSet.cs | Added ComputedDisabledRules property to existing managed rule set model |
NetworkResourceManagerProfile.cs | Added AutoMapper configuration for the new model class |
ChangeLog.md | Documented the new ComputedDisabledRules property and affected cmdlets |
ApplicationGatewayTests.ps1 | Added comprehensive test function for the new functionality |
ApplicationGatewayTests.cs | Updated test runner to execute the new test |
Comments suppressed due to low confidence (1)
src/Network/Network.Test/ScenarioTests/ApplicationGatewayTests.cs:379
- The test method name was changed from TestApplicationGatewayFirewallPolicyWithCustomBlockResponse to TestApplicationGatewayFirewallPolicyComputedDisabledRules, but this appears to be replacing an existing test rather than adding a new one. The original custom block response test coverage may have been lost.
public void TestApplicationGatewayFirewallPolicyComputedDisabledRules()
@@ -28,5 +28,7 @@ public partial class PSApplicationGatewayFirewallPolicyManagedRuleSet | |||
public string RuleSetVersion { get; set; } | |||
[Ps1Xml(Target = ViewControl.Table)] | |||
public List<PSApplicationGatewayFirewallPolicyManagedRuleGroupOverride> RuleGroupOverrides { get; set; } | |||
[Ps1Xml(Target = ViewControl.Table)] | |||
public List<PSApplicationGatewayFirewallPolicyManagedRuleSetRuleGroup> ComputedDisabledRules { get; private set; } |
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 setter for ComputedDisabledRules is private, but there's no public method to populate this property. Consider adding a public setter or an initialization method to ensure the property can be properly populated during object creation and mapping operations.
public List<PSApplicationGatewayFirewallPolicyManagedRuleSetRuleGroup> ComputedDisabledRules { get; private set; } | |
public List<PSApplicationGatewayFirewallPolicyManagedRuleSetRuleGroup> ComputedDisabledRules { get; set; } |
Copilot uses AI. Check for mistakes.
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
@yoavmal please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
NOTE:
This was already reviewed and approved here: PR. Since release-network-2024-10-01 isn't considered for main yet, I cherry-picked the feature into main.
Description
Default Disabled Rules
The Default Disabled Rules feature allows certain rules in a managed rule group to be disabled by default. This is different from the previous behavior, where all rules were automatically enabled unless the user explicitly disabled them.
With this feature, rule authors (i.e., us - AppGW WAF team) can mark specific rules as disabled in the manifest. Users can then override this default disabled rules by explicitly enabling those rules in their WAF policy. The user's override always takes priority—if a user enables a rule that’s disabled by default, the rule becomes enabled.
As part of this feature, a new read-only property called
ComputedDisabledRules
was introduced. This property shows the final list of rules that are effectively disabled, based on:This property makes it easier to understand which rules are effectively disabled.
Design Document:
https://microsoftapc-my.sharepoint.com/:w:/r/personal/neerajsingh_microsoft_com/_layouts/15/Doc.aspx?sourcedoc=%7B5449A54B-248C-41BD-8D4B-2EE3BB64E302%7D&file=Default%20disabled%20rules%20design%20document.docx&action=default&mobileredirect=true
Minimum API Version Required
2024-07-01
Swagger PR link / SDK link
Main - https://github.com/Azure/azure-rest-api-specs-pr/pull/19894
Minor - property naming fix - https://github.com/Azure/azure-rest-api-specs-pr/pull/20162
Request Examples
https://github.com/Azure/azure-rest-api-specs-pr/blob/main/specification/network/resource-manager/Microsoft.Network/stable/2024-07-01/examples/ApplicationGatewayAvailableWafRuleSetsGet.json
https://github.com/Azure/azure-rest-api-specs-pr/blob/main/specification/network/resource-manager/Microsoft.Network/stable/2024-07-01/examples/WafListAllPolicies.json
https://github.com/Azure/azure-rest-api-specs-pr/blob/main/specification/network/resource-manager/Microsoft.Network/stable/2024-07-01/examples/WafListPolicies.json
https://github.com/Azure/azure-rest-api-specs-pr/blob/main/specification/network/resource-manager/Microsoft.Network/stable/2024-07-01/examples/WafPolicyGet.json
Mandatory Checklist
Please choose the target release of Azure PowerShell. (⚠️ Target release is a different concept from API readiness. Please click below links for details.)
Check this box to confirm: I have read the Submitting Changes section of
CONTRIBUTING.md
and reviewed the following information:ChangeLog.md
file(s) appropriatelysrc/{{SERVICE}}/{{SERVICE}}/ChangeLog.md
.## Upcoming Release
header in the past tense.ChangeLog.md
if no new release is required, such as fixing test case only.