-
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
Update data_loss_prevention_discovery_config to include field support for OtherCloudDiscoveryTarget #12114
base: main
Are you sure you want to change the base?
Conversation
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @SirGitsalot, 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_data_loss_prevention_discovery_config" "primary" {
other_cloud_starting_location {
aws_location {
all_asset_inventory_assets = # value needed
}
}
targets {
other_cloud_target {
conditions {
amazon_s3_bucket_conditions {
bucket_types = # value needed
object_storage_classes = # value needed
}
}
filter {
single_resource {
amazon_s3_bucket {
aws_account {
account_id = # value needed
}
bucket_name = # value needed
}
}
}
}
}
}
|
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_data_loss_prevention_discovery_config" "primary" {
other_cloud_starting_location {
aws_location {
all_asset_inventory_assets = # value needed
}
}
targets {
other_cloud_target {
conditions {
amazon_s3_bucket_conditions {
bucket_types = # value needed
object_storage_classes = # value needed
}
}
filter {
single_resource {
amazon_s3_bucket {
aws_account {
account_id = # value needed
}
bucket_name = # value needed
}
}
}
}
}
}
|
Tests analyticsTotal tests: 63 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
|
1 similar comment
Tests analyticsTotal tests: 63 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. |
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them 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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_data_loss_prevention_discovery_config" "primary" {
other_cloud_starting_location {
aws_location {
all_asset_inventory_assets = # value needed
}
}
targets {
other_cloud_target {
conditions {
amazon_s3_bucket_conditions {
bucket_types = # value needed
object_storage_classes = # value needed
}
}
filter {
single_resource {
amazon_s3_bucket {
aws_account {
account_id = # value needed
}
bucket_name = # value needed
}
}
}
}
}
}
|
Tests analyticsTotal tests: 63 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. |
/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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_data_loss_prevention_discovery_config" "primary" {
other_cloud_starting_location {
aws_location {
all_asset_inventory_assets = # value needed
}
}
targets {
other_cloud_target {
conditions {
amazon_s3_bucket_conditions {
bucket_types = # value needed
object_storage_classes = # value needed
}
}
filter {
single_resource {
amazon_s3_bucket {
aws_account {
account_id = # value needed
}
bucket_name = # value needed
}
}
}
}
}
}
|
Tests analyticsTotal tests: 63 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. |
/gcbrun |
/gcbrun |
1 similar comment
/gcbrun |
/gcbrun |
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.
One nit and a suggestion for a config addition, but otherwise LGTM!
- name: 'awsLocation' | ||
type: NestedObject | ||
properties: | ||
- name: 'accountId' |
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.
According to the docs it's possible to specify accountId
or allAssetInventoryAssets
, but not both. I'm not sure if leaving both unset is valid, but in either case you can improve the user experience by adding either:
- conflicts to both fields, if they're allowed to be unset
- exactly_one_of if one of them is required to be set
} | ||
resource "google_organization_iam_member" "dlp_role" { | ||
org_id = "%{organization}" | ||
role = "roles/dlp.orgdriver" |
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: there's some weird indentation in the test configs (GitHub doesn't show it, but I'll bet it's due to a combination of tabs and spaces)
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 adding roles/dlp.admin
will do the trick, that change will need to be made in the CI Terraform config
return acctest.Nprintf(` | ||
data "google_project" "project" { | ||
} | ||
resource "google_organization_iam_member" "service_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.
These IAM grants are the reason that the tests are failing on our CI system - the service account running the test doesn't have permission to make org-level IAM changes. What are the minimum IAM roles needed to create the config?
My reading of the docs makes me think that granting the SA running the test roles/dlp.admin
at the org level will be enough to run the test:
If you don't have the Organization Administrator (roles/resourcemanager.organizationAdmin) or Security Admin (roles/iam.securityAdmin) role, you can still create a scan configuration. After you create the scan configuration, someone in your organization who has one of these roles must grant discovery access to the service 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.
Updated CI terraform config; will see if tests pass
@patrickmoy, this PR is waiting for action from you. If no action is taken, this PR will be closed in 28 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
@patrickmoy, this PR is waiting for action from you. If no action is taken, this PR will be closed in 14 days. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
@patrickmoy, this PR is waiting for action from you. If no action is taken, this PR will be closed in 2 weekdays. Please address any comments or change requests, or re-request review from a core reviewer if no action is required. This notification can be disabled with the |
/gcbrun |
2 similar comments
/gcbrun |
/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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_data_loss_prevention_discovery_config" "primary" {
other_cloud_starting_location {
aws_location {
all_asset_inventory_assets = # value needed
}
}
targets {
other_cloud_target {
conditions {
amazon_s3_bucket_conditions {
bucket_types = # value needed
object_storage_classes = # value needed
}
}
filter {
single_resource {
amazon_s3_bucket {
aws_account {
account_id = # value needed
}
bucket_name = # value needed
}
}
}
}
}
}
|
Tests analyticsTotal tests: 63 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. |
@SirGitsalot This PR has been waiting for review for 3 weekdays. Please take a look! Use the label |
/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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_data_loss_prevention_discovery_config" "primary" {
other_cloud_starting_location {
aws_location {
all_asset_inventory_assets = # value needed
}
}
targets {
other_cloud_target {
conditions {
amazon_s3_bucket_conditions {
bucket_types = # value needed
object_storage_classes = # value needed
}
}
filter {
single_resource {
amazon_s3_bucket {
aws_account {
account_id = # value needed
}
bucket_name = # value needed
}
}
}
}
}
}
|
1 similar comment
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_data_loss_prevention_discovery_config" "primary" {
other_cloud_starting_location {
aws_location {
all_asset_inventory_assets = # value needed
}
}
targets {
other_cloud_target {
conditions {
amazon_s3_bucket_conditions {
bucket_types = # value needed
object_storage_classes = # value needed
}
}
filter {
single_resource {
amazon_s3_bucket {
aws_account {
account_id = # value needed
}
bucket_name = # value needed
}
}
}
}
}
}
|
Tests analyticsTotal tests: 63 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
|
1 similar comment
Tests analyticsTotal tests: 63 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. |
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
…veryTarget, which currently only supports AWS S3 buckets.
/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.
Missing test reportYour PR includes resource fields which are not covered by any test. Resource: resource "google_data_loss_prevention_discovery_config" "primary" {
other_cloud_starting_location {
aws_location {
all_asset_inventory_assets = # value needed
}
}
targets {
other_cloud_target {
conditions {
amazon_s3_bucket_conditions {
bucket_types = # value needed
object_storage_classes = # value needed
}
}
filter {
single_resource {
amazon_s3_bucket {
aws_account {
account_id = # value needed
}
bucket_name = # value needed
}
}
}
}
}
}
|
1 similar comment
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_data_loss_prevention_discovery_config" "primary" {
other_cloud_starting_location {
aws_location {
all_asset_inventory_assets = # value needed
}
}
targets {
other_cloud_target {
conditions {
amazon_s3_bucket_conditions {
bucket_types = # value needed
object_storage_classes = # value needed
}
}
filter {
single_resource {
amazon_s3_bucket {
aws_account {
account_id = # value needed
}
bucket_name = # value needed
}
}
}
}
}
}
|
Tests analyticsTotal tests: 63 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
|
1 similar comment
Tests analyticsTotal tests: 63 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. |
🔴 Tests failed during RECORDING mode: 🔴 Errors occurred during RECORDING mode. Please fix them to complete your PR. |
@GoogleCloudPlatform/terraform-team @SirGitsalot This PR has been waiting for review for 1 week. Please take a look! Use the label |
Note that OtherCloudDiscoveryTarget currently only supports AWS S3 buckets in this update.
Release Note Template for Downstream PRs (will be copied)
See Write release notes for guidance.