-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Added new resource "Security Policy Rule" #10434
Added new resource "Security Policy Rule" #10434
Conversation
…tests; - Added "patchRule" test to "SecurityPolicy"; - Added default value for "adaptive_protection_config > layer_7_ddos_defense_config > enable";
…ionSecurityPolicy, still not fully implemented in terraform for global securityPolicy);
- Removed patchRule test from SecurityPolicy (created during discovery);
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @shuyama1, a repository maintainer, has been assigned to review your changes. If you have not received review feedback within 2 business days, please leave a comment on this PR asking them to take a look. You can help make sure that review is quick by doing a self-review and by running impacted tests locally. |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_security_policy_rule" "primary" {
match = # value needed
match {
config = # value needed
expr = # value needed
expr {
description = # value needed
location = # value needed
title = # value needed
}
}
network_match = # value needed
network_match {
dest_ip_ranges = # value needed
dest_ports = # value needed
ip_protocols = # value needed
src_asns = # value needed
src_ip_ranges = # value needed
src_ports = # value needed
src_region_codes = # value needed
user_defined_fields = # value needed
user_defined_fields {
name = # value needed
values = # value needed
}
}
project = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeSecurityPolicyRule_basicUpdate|TestAccComputeSecurityPolicyRule_extendedUpdate|TestAccComputeSecurityPolicyRule_securityPolicyRuleBasicExample|TestAccComputeSecurityPolicyRule_securityPolicyRuleMultipleRulesExample|TestAccComputeSecurityPolicyRule_withRuleExpr |
|
@@ -0,0 +1,23 @@ | |||
resource "google_compute_security_policy" "default" { | |||
provider = google-beta |
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'm still in the middle of reviewing this PR. A quick suggestion is removing provider = google-beta
in the examples as this resource + its examples are no longer marked as beta-only in the latest commit
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_security_policy_rule" "primary" {
match {
expr {
description = # value needed
location = # value needed
title = # value needed
}
}
network_match {
dest_ip_ranges = # value needed
dest_ports = # value needed
ip_protocols = # value needed
src_asns = # value needed
src_ip_ranges = # value needed
src_ports = # value needed
src_region_codes = # value needed
user_defined_fields {
name = # value needed
values = # value needed
}
}
preconfigured_waf_config {
exclusion {
request_cookie {
value = # value needed
}
}
}
project = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeSecurityPolicyRule_withPreconfiguredWafConfig |
|
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_security_policy_rule" "primary" {
match {
expr {
description = # value needed
location = # value needed
title = # value needed
}
}
network_match {
dest_ip_ranges = # value needed
dest_ports = # value needed
ip_protocols = # value needed
src_asns = # value needed
src_ip_ranges = # value needed
src_ports = # value needed
src_region_codes = # value needed
user_defined_fields {
name = # value needed
values = # value needed
}
}
preconfigured_waf_config {
exclusion {
request_cookie {
value = # value needed
}
}
}
project = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
type = "CLOUD_ARMOR" | ||
} | ||
|
||
# A default rule is generated when creating the security_policy resource, import is needed to patch it |
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 same issue exists in the regional security policy rule resource: hashicorp/terraform-provider-google#15687
I commented out the fields inside the "network_match" block, and "title", "description" and "network" in the "expr" block. |
{ | ||
ResourceName: "google_compute_security_policy_rule.policy_rule", | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
}, | ||
{ | ||
Config: testAccComputeSecurityPolicyRule_postBasicUpdate(context), | ||
}, | ||
{ | ||
ResourceName: "google_compute_security_policy_rule.policy_rule", | ||
ImportState: true, | ||
ImportStateVerify: 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.
only nit - we use tabs here for spacing (we only use spaces for indentation in the resource configs)
{ | ||
ResourceName: "google_compute_security_policy_rule.policy_rule", | ||
ImportState: true, | ||
ImportStateVerify: 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.
nit - spacing
{ | ||
ResourceName: "google_compute_security_policy_rule.policy_rule", | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
}, | ||
{ | ||
Config: testAccComputeSecurityPolicyRule_extPosUpdateSamePriority(context), | ||
ExpectError: regexp.MustCompile("Cannot have rules with the same priorities."), |
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.
nit - spacing
{ | ||
ResourceName: "google_compute_security_policy_rule.policy_rule", | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
}, | ||
{ | ||
Config: testAccComputeSecurityPolicyRule_extPosUpdate(context), | ||
}, | ||
{ | ||
ResourceName: "google_compute_security_policy_rule.policy_rule", | ||
ImportState: true, | ||
ImportStateVerify: 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.
nit - spacing
{ | ||
ResourceName: "google_compute_security_policy_rule.policy_rule", | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
}, | ||
{ | ||
Config: testAccComputeSecurityPolicyRule_withPreconfiguredWafConfig_update(context), | ||
}, | ||
{ | ||
ResourceName: "google_compute_security_policy_rule.policy_rule", | ||
ImportState: true, | ||
ImportStateVerify: true, | ||
}, | ||
{ | ||
Config: testAccComputeSecurityPolicyRule_withPreconfiguredWafConfig_clear(context), | ||
Check: resource.ComposeTestCheckFunc( |
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.
nit - spacing
}, | ||
{ | ||
ResourceName: "google_compute_security_policy_rule.policy_rule", | ||
ImportState: true, | ||
ImportStateVerify: 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.
nit - spacing
description: | | ||
BGP Autonomous System Number associated with the source IP address. | ||
item_type: Api::Type::Integer | ||
# These fields are used for security policies of the type CLOUD_ARMOR_NETWORK, this type is not supported by the global security policy resource. |
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.
If the following fields won't be supported by the global security policy resource, I think we can just remove them.
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_security_policy_rule" "primary" {
project = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeSecurityPolicyRule_withPreconfiguredWafConfig |
|
- spacing fix;
Hi there, I'm the Modular magician. I've detected the following information about your changes: Diff reportYour PR generated some diffs in downstreams - here they are.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_compute_security_policy_rule" "primary" {
project = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
|
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.
Thank you!
Adds a new resource "google_compute_security_policy_rule", allowing an alternative to handling rules in the security policy resource itself. Functionally similar to regional security policy rules.
FIxes: hashicorp/terraform-provider-google#5622
Release Note Template for Downstream PRs (will be copied)