-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Added new profile type "URL_FILTERING" for SecurityProfile #13342
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 profile type "URL_FILTERING" for SecurityProfile #13342
Conversation
… for SecurityProfile; - Added urlFilteringProfile field to SecurityProfileGroup;
- Added default url_filters to SecurityProfile and SecurityProfileGroup tests;
…urity-security-profile
…urity-security-profile
|
Hello! I am a robot. Tests will require approval from a repository maintainer to run. Googlers: For automatic test runs see go/terraform-auto-test-runs. @slevenick, 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. |
| - 'urlFilteringProfile' | ||
| - 'customMirroringProfile' | ||
| - 'customInterceptProfile' | ||
| - name: 'urlFilteringProfile' |
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 you add an update test for this field? Adding/deleting items in the url_filters set is the main thing to test
|
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.
|
Tests analyticsTotal tests: 70 Click here to see the affected service packages
Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
slevenick
left a comment
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.
Beta-only tests need provider = google-beta declared in the config
|
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.
|
Tests analyticsTotal tests: 70 Click here to see the affected service packages
Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
…ority 2147483647 cannot be updated); - Changed update test step 2 to now update rule priority 1;
|
@slevenick This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
|
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.
|
Tests analyticsTotal tests: 70 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 tests
|
|
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
I'm having problems running the tests for this resource locally (facing some ADC auth problems when creating organization level resources), can you show me the logs for the failing update test? |
|
@GoogleCloudPlatform/terraform-team @slevenick This PR has been waiting for review for 1 week. Please take a look! Use the label |
slevenick
left a comment
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.
Yeah it looks like the default rule is causing issues:
It fails with resource_network_security_security_profile_test.go:121: Step 1/8 error: After applying this test step, the plan was not empty.
- url_filters {
- filtering_action = "DENY" -> null
- priority = 2147483647 -> null
- urls = [
- "*",
] -> null
}
}
}
Looks like it's getting a default profile set automatically
… auto-created url_filter; - Removed from example tests the url_filter using default priority (2147483647) as these are currently immutable;
|
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.
|
Tests analyticsTotal tests: 71 Click here to see the affected service packages
Action takenFound 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests
|
|
🟢 Tests passed during RECORDING mode: 🟢 No issues found for passed tests after REPLAYING rerun. 🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
|
|
||
| priorityFlatten := flattenNetworkSecuritySecurityProfileUrlFilteringProfileUrlFiltersPriority(original["priority"], d, config) | ||
| // Do not include the auto created default url_filter coming back from the api unless the user included it in his config | ||
| if priorityFlatten == 2147483647 && !resourceDataContainsDefaultFilter { |
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 think this is causing the failure, the default priority is included in the config but we aren't seeing it in state after import:
=== CONT TestAccNetworkSecuritySecurityProfile_networkSecuritySecurityProfileUrlFilteringUpdate
resource_network_security_security_profile_test.go:121: Step 8/10 error running import: ImportStateVerify attributes not equivalent. Difference is shown below. The - symbol indicates attributes missing after import.
map[string]string{
- "url_filtering_profile.#": "1",
- "url_filtering_profile.0.%": "1",
}
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.
Or the place above where we check for this priority specifically
…ering_profile field instead of only url_filters;
|
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.
|
Tests analyticsTotal tests: 71 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 tests
|
Adds the option to manage a new profile type
URL_FILTERINGfornetworksecuritySecurityProfile and SecurityProfileGroup, adding theurl_filtering_profilefield and related tests to those resources.Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.