-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 contents in rules[].rateLimitOptionsfields for google_compute_security_policy to support Cloud Armor #7132
Added new contents in rules[].rateLimitOptionsfields for google_compute_security_policy to support Cloud Armor #7132
Conversation
Hello! I am a robot who works on Magic Modules PRs. I've detected that you're a community contributor. @zli82016, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
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. Terraform GA: Diff ( 2 files changed, 27 insertions(+), 6 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeSecurityPolicy_withRateLimit_withEnforceOnKeyConfigs|TestAccComputeSecurityPolicy_withRateLimitOptions |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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. Terraform GA: Diff ( 2 files changed, 27 insertions(+), 6 deletions(-)) |
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. Terraform GA: Diff ( 2 files changed, 27 insertions(+), 6 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeSecurityPolicy_withRateLimit_withEnforceOnKeyConfigs|TestAccFirebaserulesRelease_BasicRelease |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi there, I'm the Modular magician. I've detected the following information about your changes: Breaking Change(s) DetectedThe following breaking change(s) were detected within your pull request.
If you believe this detection to be incorrect please raise the concern with your reviewer. If you intend to make this change you will need to wait for a major release window. An Diff reportYour PR generated some diffs in downstreams - here they are. Terraform GA: Diff ( 2 files changed, 28 insertions(+), 8 deletions(-)) |
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. Terraform GA: Diff ( 2 files changed, 28 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeSecurityPolicy_withRateLimit_withEnforceOnKeyConfigs|TestAccComputeForwardingRule_update |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Type: schema.TypeList, | ||
Description: `Enforce On Key Config of this security policy`, | ||
Optional: true, | ||
MaxItems: 3, |
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.
Normally, we will leave the validation on server side in case the validation will change, unless the users requested 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.
It's an internal Google request to set this new providers. I don't know if a answer your question correctly.
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.
Sorry for the confusion. MaxItems: 3,
can be removed from this file. I think API side will have the same validation.
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.
Don't worry :D I already remove, thank you for let me know!
@@ -274,6 +274,30 @@ func TestAccComputeSecurityPolicy_withRateLimitWithRedirectOptions(t *testing.T) | |||
}) | |||
} | |||
|
|||
<% unless version == 'ga' -%> | |||
func TestAccComputeSecurityPolicy_withRateLimit_withEnforceOnKeyConfigs(t *testing.T) { |
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 test failed. You can check the errr message by clicking "Error Message" next to the failed test.
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.
Hi there!
The test failed because we need to change the enforceOnKey, this provider has "default" value and automatically is inserted on the tests:
The enforceOnKeyConfigs is a new providers that must not has the enforceOnKey in the same script.
Check the API doc bellow if you wish:
https://cloud.google.com/compute/docs/reference/rest/beta/securityPolicies
We can't remove the default value from enforceOnKey, we are search how we can solved this. When we run the test with enforceOnKey default value on we receive this error:
When we run the test without enforceOnKey default value the test passes.
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 tested after removing the Default: "ALL" for the field enforce_on_key
in this file and then run the test locally. The test sends the request to API.
Request:
{
"description": "updated description",
"name": "tf-test-418rgij846",
"rules": [
{
"action": "throttle",
"description": "default rule",
"headerAction": {},
"match": {
"config": {
"srcIpRanges": [
"*"
]
},
"versionedExpr": "SRC_IPS_V1"
},
"preview": false,
"priority": 2147483647,
"rateLimitOptions": {
"conformAction": "allow",
"exceedAction": "deny(403)",
"rateLimitThreshold": {
"count": 100,
"intervalSec": 60
}
}
}
]
}
Response:
{
"kind": "compute#securityPolicy",
"id": "184350908738827980",
"creationTimestamp": "2023-01-18T11:27:31.714-08:00",
"name": "tf-test-418rgij846",
"description": "updated description",
"rules": [
{
"kind": "compute#securityPolicyRule",
"description": "default rule",
"priority": 2147483647,
"match": {
"versionedExpr": "SRC_IPS_V1",
"config": {
"srcIpRanges": [
"*"
]
}
},
"action": "throttle",
"preview": false,
"rateLimitOptions": {
"rateLimitThreshold": {
"count": 100,
"intervalSec": 60
},
"conformAction": "allow",
"exceedAction": "deny(403)"
},
"headerAction": {}
}
],
"fingerprint": "mQvRGsyan_U=",
"selfLink": "https://www.googleapis.com/compute/v1/projects/local-mediator-361721/global/securityPolicies/tf-test-418rgij846",
"type": "CLOUD_ARMOR"
}
In the response, I do not see the field enforceOnKey
. Does that mean enforceOnKey
has the default value ALL for this 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.
Is the response expected from API side?
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.
Hello there!
About your test: yes, you're correct, you locally removed the Default: “ALL” from the enforceOnKey
field and the response doesn't show the value, right? This is expected to happen when you make this change.
The default value when inserted in a provider is automatically inserted when you generate a terraform test or a terraform script, that's the purpose of using the Default field.
I looked better about the in terraform-plugin-sdk (the library used to build providers) and the description about what it does when: the attribute Default
// Default indicates a value to set if this attribute is not set in the
// configuration. Default cannot be used with DefaultFunc or Required.
// Default is only supported if the Type is TypeBool, TypeFloat, TypeInt,
// or TypeString. Default cannot be used if the Schema is directly an
// implementation of an Elem field of another Schema, such as trying to
// set a default value for a TypeList or TypeSet.
//
// Changing either Default can be a breaking change, especially if the
// attribute has ForceNew enabled. If a default needs to change to align
// with changing assumptions in an upstream API, then it may be necessary
// to also implement resource state upgrade functionality to change the
// state to match or update read operation logic to align with the new
// default.
Default interface{}
The enforceOnKeyConfigs
is a new attribute and conflicts with enforceOnKey
, so only one must be set at a time, as stated in the [Google API documentation (https://cloud.google.com/compute/docs/reference/rest/beta/securityPolicies#:~:text=If%20enforceOnKeyConfigs%20is%20specified%2C%20enforceOnKey%20must%20not%20be%20specified):
I double checked and have created a test with enforceOnKeyConfigs
fields,and tested locally with enforceOnKey
Default: “ALL” inserted and I got an error with the message: “Only one of enforceOnKey and enforceOnKeyConfigs can be specified”. When I removed the Default: “ALL” from enforceOnKey
and ran the test again, it passed successfully.
(Conflict error between enforceOnKey
and enforceOnKeyConfigs
when default is inserted)
You can check on SDK documentation about the enforceOnKeyConfigs here
I tried to remove the enforceOnKey
Default: “ALL” field, but during the PR I got the breaking change, I can’t remove this.
So, our understanding is that the magic module pipeline runs some additional tests, and for some reason it doesn’t handle this conflicting case.
Currently, there is only one test that tests enforceOnKey
.
To test the enforceOnKeyConfigs, we cannot use enforceOnKey, but it sounds like a magic-module pipeline does some additional tests that automatically insert an enforceOnKey, enforcing that when we run locally the test, it PASS with success.
We are kind of lost here. Where is the issue? Can you help us with this?
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.
Config: https://paste.googleplex.com/4578722240790528
Terraform plan output: https://paste.googleplex.com/6415383668981760
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.
@afpesseti , after talked with the team, instead of removing the default value for the field enforceOnKey, we are going to do the following:
- Add
""
to the list of allowed values for the field enforceOnKey to allow the user to specify""
to avoid conflict with the new field. https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/resources/resource_compute_security_policy.go.erb#L265 - In the tests, when testing with the new field enforceOnKeyConfigs, in the config add
enforce_on_key = ""
to avoid conflict.
- In the doc of the new field enforceOnKeyConfigs, add a note that to set the field, enforce_on_key needs to be empty string.
Example:magic-modules/mmv1/third_party/terraform/website/docs/r/compute_instance.html.markdown
Line 74 in 62a62f2
**Note:** If you want to update this value (resize the VM) after initial creation, you must set [`allow_stopping_for_update`](#allow_stopping_for_update) to `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.
Hello there! Thank you both for the help! About the new steps, just so there are no loose ends: are they going to be done by your end? Should we also do any of the listed steps on our end?
Just double checking! :D
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.
Hello, @afpesseti , can you please do the listed steps? If you need any help, feel free to let us know. Thanks.
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.
Hi There! Steps done :D Thank you so much for help us ❤️
|
||
enforce_on_key_configs { | ||
enforce_on_key_type = "HTTP_HEADER" | ||
enforce_on_key_name = "user-agent" |
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.
weird indentation.
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'll adjust this. On editor is very different, we only see this weird indentation after we push the code. But we'll adjust and run again. Thank you for let me know :D
PreCheck: func() { testAccPreCheck(t) }, | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckComputeSecurityPolicyDestroyProducer(t), | ||
Steps: []resource.TestStep{ |
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.
Can more tests be added to update and remove enforce_on_key_configs?
Example: https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/tests/resource_compute_security_policy_test.go.erb#L80-L116
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.
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 don't see any different between testAccComputeSecurityPolicy_withRateLimitOptions_withEnforceOnKeyConfigs_withRedirectOptionsUpdate and testAccComputeSecurityPolicy_withRateLimitOptions_withEnforceOnKeyConfigs.
Also, I cannot tell what happened for the error of removal, as I cannot see the removal test.
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.
Hello there! Sorry for delay.
I was trying to create an enforceOnKeyConfig
update and removal test, but it didn't work, every removal or update failed. So, I had an idea: I created two tests, one with only one enforceOnKeyConfig
item using redirect options and other with multiple enforceOnKeyConfig
items. I took as an example the TestAccComputeSecurityPolicy_withRateLimitOptions
, there are two test with enforceOnKey
with two examples, but there is no update or removal test.
|
||
* `enforce_on_key_name` - (Optional) Rate limit key name applicable only for the following key types: HTTP_HEADER -- Name of the HTTP header whose value is taken as the key value. HTTP_COOKIE -- Name of the HTTP cookie whose value is taken as the key value. | ||
|
||
* `enforce_on_key_configs` - (Optional) If specified, any combination of values of enforce_on_key_type/enforce_on_key_name is treated as the key on which ratelimit threshold/action is enforced. You can specify up to 3 enforce_on_key_configs. If enforce_on_key_configs is specified, enforce_on_key must not be specified. Structure is [documented below](#nested_enforce_on_key_configs). |
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.
As enforce_on_key_configs
is only available in Beta API, can you please add Beta after Optional?
Please check the doc for the field preconfigured_waf_config
in the same file.
https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/third_party/terraform/website/docs/r/compute_security_policy.html.markdown?plain=1#L188
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.
Thanks for warning about this, fixed.
@afpesseti , can you please change the title and release note for this PR? The current title and release note are not related. Also, is there an open issue in github or buganizer for this PR? If is, please add to the description of this PR. |
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. Terraform GA: Diff ( 2 files changed, 28 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeSecurityPolicy_withRateLimit_withEnforceOnKeyConfigs |
Tests passed during RECORDING mode: All tests passed |
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.
LGTM.
Waiting for the review from @ScottSuarez |
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.
Asking for some documentation changes. Looks good otherwise.
mmv1/third_party/terraform/website/docs/r/compute_security_policy.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/r/compute_security_policy.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/r/compute_security_policy.html.markdown
Show resolved
Hide resolved
mmv1/third_party/terraform/resources/resource_compute_security_policy.go.erb
Show resolved
Hide resolved
…icy.html.markdown Co-authored-by: Scott Suarez <[email protected]>
…icy.html.markdown Co-authored-by: Scott Suarez <[email protected]>
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. Terraform GA: Diff ( 2 files changed, 30 insertions(+), 7 deletions(-)) |
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. Terraform GA: Diff ( 2 files changed, 112 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease|TestAccRegionInstanceGroupManager_stateful|TestAccApigeeAddonsConfig_apigeeAddonsTestExample|TestAccFrameworkProviderMeta_setModuleName|TestAccBillingSubaccount_basic|TestAccBillingSubaccount_renameOnDestroy|TestAccDataSourceDnsRecordSet_basic|TestAccSqlDatabaseInstance_withPrivateNetwork_withoutAllocatedIpRange |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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. Terraform GA: Diff ( 2 files changed, 72 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFrameworkProviderMeta_setModuleName|TestAccRegionInstanceGroupManager_stateful|TestAccDataSourceDnsRecordSet_basic|TestAccDataSourceDnsManagedZone_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
/gcbrun |
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. Terraform GA: Diff ( 2 files changed, 72 insertions(+), 7 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeRouterNat_update|TestAccApigeeAddonsConfig_apigeeAddonsTestExample|TestAccDataSourceDnsManagedZone_basic|TestAccFrameworkProviderMeta_setModuleName|TestAccDataSourceDnsRecordSet_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
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.
looks good !
…te_security_policy to support Cloud Armor (GoogleCloudPlatform#7132) Co-authored-by: Scott Suarez <[email protected]>
…te_security_policy to support Cloud Armor (GoogleCloudPlatform#7132) Co-authored-by: Scott Suarez <[email protected]>
API: https://cloud.google.com/compute/docs/reference/rest/beta/securityPolicies
If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)