-
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 support for VPC Service Controls (VPC SC) Secure Data Exchange #4509
Conversation
Hello! I am a robot who works on Magic Modules PRs. I have detected that you are a community contributor, so your PR will be assigned to someone with a commit-bit on this repo for initial review. Thanks for your contribution! A human will be with you soon. @slevenick, please review this PR or find an appropriate assignee. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 4122 insertions(+), 96 deletions(-)) |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=173138" |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccAccessContextManager|TestAccRedisInstance_redisInstancePrivateServiceExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=173140" |
Tests failed during RECORDING mode: TestAccRedisInstance_redisInstancePrivateServiceExample Please fix these to complete your PR |
@slevenick I've run tests for the failed one locally, with a blank project within my org. The test passes successfully, looks like the failure could be a transient error. Any suggestion on how to retry the PR's tests here or other way to proceed? Thanks!
|
have multiple `IngressPolicies`, each of which is evaluated | ||
separately. Access is granted if any `Ingress Policy` grants it. | ||
Must be empty for a perimeter bridge. | ||
item_type: !ruby/object:Api::Type::NestedObject |
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 likely need to mark some of the subfields here as Required
, or at least mark some as AtLeastOneOf
/ExactlyOneOf
. Probably Required because this is inside an array which makes the other two options not viable
We try to do this for nested objects because allowing users to specify empty object often causes problems. Can we mark at least one field on every level of the object as Required?
@@ -157,6 +157,12 @@ overrides: !ruby/object:Overrides::ResourceOverrides | |||
vars: | |||
access_level_name: "chromeos_no_lock" | |||
service_perimeter_name: "restrict_storage" | |||
- !ruby/object:Provider::Terraform::Examples | |||
name: "access_context_manager_service_perimeter_secure_data_exchange" | |||
skip_test: 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.
Why skip test here?
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.
Well, these are all marked as skip test because they are only for example generation. Can we add a test for this to the handwritten test file so our CI will run them regularly?
I believe it is this file: https://github.com/GoogleCloudPlatform/magic-modules/blob/master/mmv1/third_party/terraform/tests/resource_access_context_manager_service_perimeter_test.go.erb
The redis test is flaky, don't worry about that. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 4148 insertions(+), 102 deletions(-)) |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=174927" |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccServiceUsageConsumerQuotaOverride_regionConsumerQuotaOverrideExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=174929" |
Tests failed during RECORDING mode: TestAccServiceUsageConsumerQuotaOverride_regionConsumerQuotaOverrideExample Please fix these to complete your PR |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 4152 insertions(+), 102 deletions(-)) |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=174933" |
@slevenick I have added the
In regards to testing you mentioned, I had originally submitted a test in the following file: Both the test for this feature and the one failing in CI/CD ( |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccServiceUsageConsumerQuotaOverride_regionConsumerQuotaOverrideExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=174937" |
Tests failed during RECORDING mode: TestAccServiceUsageConsumerQuotaOverride_regionConsumerQuotaOverrideExample 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.
I think many of the fields can be marked as required: true
in api.yaml. Marking required fields as required helps users correctly configure resources
I'm guessing a little here because I'm not very familiar with this resource, so if they are not actually required then we shouldn't add it. But most of these things seem like they must be filled out. If a field is marked as required it only must be specified if the object it belongs in is specified, so it won't force a resource that doesn't specify ingress_policies or egress_policies to include them
Defines the conditions on the source of a request causing this `IngressPolicy` | ||
to apply. | ||
at_least_one_of: | ||
- status.0.ingress_policies.ingressFrom |
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.
Unfortunately I don't believe this will work. Because ingress_policies is an array we would have to hard-code the index here, but since it is an array that allows multiple entries we could only ever check the first entry with a hard coded index.
name: 'identities' | ||
item_type: Api::Type::String | ||
description: / | ||
A list of identities that are allowed access through this ingress policy. |
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 there a reason this would ever be empty or not specified? Can we make this required: 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.
The API allows you to specify either identities
or identity_type
, so I think what's needed is an exactly_one_of
. The same goes for egress_policies.egressFrom.identities
.
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.
This was how I had the validation properties set on a PR that was trying to do the same thing as this one: 23baeae
- !ruby/object:Api::Type::Array | ||
name: 'sources' | ||
description: / | ||
Sources that this `IngressPolicy` authorizes access from. |
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 we make this required: true also?
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 have been reviewing the API and running some tests against it. The API does not seem to have any requirement of required fields or "at least / exactly one of".
For example
- it is possible to create a blank
ingressPolicy
without aningressFrom
oringressTo
. - It is possible to create a blank
ingressPolicy.ingressFrom
without specifying anidentityType
,identities
,allowedIdentity
orsources
. - An
ingressFrom
can be empty, have onlysources
, or have asources
+identityType
- There are incompatible combinations, but those should be obvious to the user. If the user still goes ahead the error is self explanatory. E.g. can't mix
identityType: ANY_IDENTITY
and at thiese same time specify users inidentities
. The error message raised is:[...] Error in IngressFrom: 'identities' field should be empty when identity_type field is not set to IDENTITY_TYPE_UNSPECIFIED.
- The rules seem to apply equally for
egress
My recommendation is to remove all the at_least_one_of
in this PR. Any other suggestions?
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, I guess that is the only way to handle this
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 5 files changed, 4122 insertions(+), 96 deletions(-)) |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=177323" |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceGoogleCloudRunLocations_basic|TestAccDataSourceDNSKeys_noDnsSec|TestAccDataSourceDnsManagedZone_basic|TestAccDataSourceGameServicesGameServerDeploymentRollout_basic|TestAccDataSourceGoogleActiveFolder_default|TestAccDataSourceComputeNetworkEndpointGroup|TestAccDataSourceDNSKeys_basic|TestAccDataSourceCloudIdentityGroupMemberships_basic|TestAccDataSourceCloudIdentityGroups_basic|TestAccDataSourceGoogleActiveFolder_space|TestAccDataSourceGoogleBigqueryDefaultServiceAccount_basic|TestAccDataSourceGoogleAppEngineDefaultServiceAccount_basic|TestAccDataSourceGoogleBillingAccount_byShortName|TestAccDataSourceGoogleBillingAccount_byFullName|TestAccDataSourceGoogleCloudRunService_optionalProject|TestAccDataSourceGoogleActiveFolder_dash|TestAccDataSourceGoogleCloudRunService_basic|TestAccDataSourceGoogleClientOpenIDUserinfo_basic|TestAccDataSourceGoogleCloudFunctionsFunction_basic|TestAccDataSourceComposerEnvironment_basic|TestAccDataSourceComposerImageVersions_basic|TestAccDataSourceGoogleComputeDefaultServiceAccount_basic|TestAccDataSourceComputeImage|TestAccDataSourceComputeBackendBucket_basic|TestAccDataSourceComputeBackendService_basic|TestAccDataSourceComputeAddress|TestAccDataSourceGoogleForwardingRule|TestAccDataSourceComputeImageFilter|TestAccDataSourceComputeGlobalAddress|TestAccDataSourceGoogleComputeInstanceGroup_basic|TestAccDataSourceGoogleComputeInstanceGroup_withNamedPort|TestAccInstanceTemplateDatasource_name|TestAccDataSourceComputeInstanceSerialPort_basic|TestAccInstanceTemplateDatasource_filter_mostRecent|TestAccDataSourceComputeNodeTypes_basic|TestAccComputeRegions_basic|TestAccInstanceTemplateDatasource_filter|TestAccDataSourceGoogleNetwork|TestAccDataSourceComputeInstance_basic|TestAccDataSourceComputeRegionSslCertificate|TestAccDataSourceGoogleComputeInstanceGroup_fromIGM|TestAccDataSourceComputeResourcePolicy|TestAccDataSourceComputeRouter|TestAccComputeZones_basic|TestAccDataSourceComputeSslCertificate|TestAccComputeZones_filter|TestAccDataSourceGoogleSslPolicy|TestAccDataSourceGoogleSubnetwork|TestAccDataSourceGoogleVpnGateway|TestAccContainerEngineVersions_basic|TestAccContainerClusterDatasource_zonal|TestAccContainerClusterDatasource_regional|TestAccContainerEngineVersions_filtered|TestAccDataSourceGoogleFolderOrganizationPolicy_basic|TestAccDataSourceGoogleFolder_byFullName|TestAccDataSourceGoogleFolder_lookupOrganization|TestAccDataSourceGoogleFolder_byShortName|TestAccDataSourceIAMRole|TestAccDataSourceGoogleKmsCryptoKey_basic|TestAccDataKmsSecretCiphertext_basic|TestAccDataSourceGoogleGlobalForwardingRule|TestAccDataSourceGoogleKmsKeyRing_basic|TestAccDataSourceGoogleMonitoringUptimeCheckIps_basic|TestAccDataSourceGoogleKmsCryptoKeyVersion_basic|TestAccDataSourceGoogleOrganization_byShortName|TestAccDataSourceGoogleOrganization_byFullName|TestAccDataSourceGoogleProjects_basic|TestAccDataSourceGoogleProjectOrganizationPolicy_basic|TestAccDatasourceGoogleServiceAccountKey_basic|TestAccDatasourceGoogleServiceAccount_basic|TestAccDataSourceGoogleProject_basic|TestAccDataSourceGoogleStorageProjectServiceAccount_basic|TestAccDataSourceGoogleSQLCaCerts_basic|TestAccDataSourceGoogleStorageTransferProjectServiceAccount_basic|TestAccDataSourceIAMBetaWorkloadIdentityPoolProvider_basic|TestAccDataSourceIAMBetaWorkloadIdentityPool_basic|TestAccDataSourceGoogleMonitoringNotificationChannel_byDisplayName|TestAccDataSourceGoogleMonitoringNotificationChannel_byTypeAndLabel|TestAccDataSourceGoogleMonitoringNotificationChannel_byDisplayNameAndType|TestAccDataSourceGoogleMonitoringNotificationChannel_UserLabel|TestAccDataSourceMonitoringService_AppEngine|TestAccRedisInstanceDatasource_basic|TestAccDataSourceGooglePubsubTopic_optionalProject|TestAccDataSourceGooglePubsubTopic_basic|TestAccRuntimeconfigConfigDatasource_basic|TestAccIapClient_Datasource_basic|TestAccRuntimeconfigVariableDatasource_basic|TestAccDatasourceSecretManagerSecretVersion_latest|TestAccDatasourceSecretManagerSecretVersion_basic|TestAccTPUTensorflowVersions_basic|TestAccDataSourceSqlDatabaseInstance_basic|TestAccDataSourceStorageBucketObjectContent_Basic|TestAccDataSourceGoogleIamTestablePermissions_basic|TestAccApiGatewayGateway_apigatewayGatewayBasicExampleUpdated|TestAccBigQueryDataset_cmek|TestAccBigQueryJob_bigqueryJobCopyExample|TestAccBigQueryJob_bigqueryJobCopyTableReferenceExample|TestAccBigqueryDataTransferConfig|TestAccBigQueryTable_Kms|TestAccBillingBudget_billingBudgetBasicExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=177326" |
Tests failed during RECORDING mode: TestAccRuntimeconfigConfigDatasource_basic|TestAccRuntimeconfigVariableDatasource_basic|TestAccBigQueryJob_bigqueryJobCopyTableReferenceExample|TestAccApiGatewayGateway_apigatewayGatewayBasicExampleUpdated Please fix these to complete your PR |
@slevenick mind taking a look at the status of this PR? I've reverted the at least one of requirements according to the API definition. |
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 the addition!
Added support for ingress and egress policies in a VPC Service Controls perimeter.
Context: https://cloud.google.com/access-context-manager/docs/reference/rest/v1/accessPolicies.servicePerimeters
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)