-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
id of instance templates now relies on the unique id of the resource #7358
Conversation
/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 ( 4 files changed, 6 insertions(+), 4 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeBackendService_withBackendAndMaxUtilization|TestAccComputeBackendService_withBackendAndIAP|TestAccComputeInstanceFromTemplate_overrideMetadataDotStartupScript|TestAccComputeRegionAutoscaler_update|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeRegionBackendService_withBackendInternal|TestAccComputeRegionBackendService_regionBackendServiceBalancingModeExample|TestAccComputeRegionPerInstanceConfig_update|TestAccComputeRegionAutoscaler_scaleInControl|TestAccComputeRegionAutoscaler_scalingSchedule|TestAccComputeRegionAutoscaler_scaleDownControl|TestAccComputeRegionAutoscaler_regionAutoscalerBasicExample|TestAccComputePerInstanceConfig_update|TestAccRegionInstanceGroupManager_distributionPolicy|TestAccRegionInstanceGroupManager_autoHealingPolicies|TestAccComputeInstanceTemplate_regionDisks|TestAccRegionInstanceGroupManager_versions|TestAccComputeInstanceTemplate_spot|TestAccComputeInstanceTemplate_disks|TestAccComputeInstanceTemplate_managedEnvoy|TestAccComputeInstanceTemplate_queueCount|TestAccComputeInstanceTemplate_networkIPAddress|TestAccRegionInstanceGroupManager_update|TestAccComputeInstanceTemplate_nictype_update|TestAccComputeInstanceTemplate_diskResourcePolicies|TestAccInstanceGroupManager_waitForStatus|TestAccComputeInstanceTemplate_withScratchDisk|TestAccRegionInstanceGroupManager_targetSizeZero|TestAccInstanceGroupManager_stateful|TestAccInstanceGroupManager_autoHealingPolicies|TestAccRegionInstanceGroupManager_basic|TestAccInstanceGroupManager_versions|TestAccInstanceGroupManager_update|TestAccComputeInstanceTemplate_enableDisplay|TestAccInstanceGroupManager_targetSizeZero|TestAccComputeInstanceTemplate_networkIP|TestAccComputeInstanceTemplate_AdvancedMachineFeatures|TestAccInstanceGroupManager_basic|TestAccComputeInstanceTemplate_networkTier|TestAccComputeInstanceFromTemplate_012_removableFields|TestAccComputeInstanceTemplate_IPv6|TestAccComputeInstanceFromTemplate_overrideScheduling|TestAccComputeInstanceTemplate_ConfidentialInstanceConfigMain|TestAccComputeInstanceFromTemplate_overrideScratchDisk|TestAccComputeInstanceFromTemplate_overrideAttachedDisk|TestAccComputeInstanceTemplate_shieldedVmConfig2|TestAccComputeInstanceTemplate_IP|TestAccComputeInstanceFromTemplate_overrideBootDisk|TestAccComputeInstanceTemplate_spot_maxRunDuration|TestAccComputeInstanceTemplate_preemptible|TestAccComputeInstanceFromTemplate_basic|TestAccComputeInstanceTemplate_imageShorthand|TestAccComputeInstanceTemplate_sourceSnapshotEncryptionKey|TestAccComputeInstanceTemplate_basic|TestAccComputeInstanceTemplate_shieldedVmConfig1|TestAccComputeInstanceTemplate_reservationAffinities|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample|TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample|TestAccComputeRegionBackendService_withBackendMultiNic|TestAccComputeRegionBackendService_withBackendInternalManaged|TestAccComputeBackendService_withBackend|TestAccComputeInstanceTemplate_sourceImageEncryptionKey|TestAccComputeForwardingRule_update|TestAccComputeForwardingRule_forwardingRuleHttpLbExample|TestAccComputeAutoscaler_scaleInControlFixed|TestAccComputeAutoscaler_scalingSchedule|TestAccComputeInstanceTemplate_instanceResourcePolicies|TestAccComputeInstanceTemplate_soleTenantNodeAffinities|TestAccComputeAutoscaler_scaleInControl|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccComputeAutoscaler_scaleDownControl|TestAccComputeAutoscaler_multicondition|TestAccComputeAutoscaler_update|TestAccComputeAutoscaler_autoscalerBasicExample|TestAccComputeBackendService_withMaxConnectionsPerInstance|TestAccComputeBackendService_withMaxConnections|TestAccComputeAutoscaler_autoscalerSingleInstanceExample|TestAccComputeInstanceTemplate_EncryptKMS|TestAccComputeInstanceTemplate_minCpuPlatform|TestAccComputeGlobalForwardingRule_globalForwardingRuleInternalExample|TestAccComputeGlobalForwardingRule_externalHttpLbMigBackendCustomHeaderExample|TestAccComputeGlobalForwardingRule_externalTcpProxyLbMigBackendExample|TestAccComputeInstanceTemplate_guestAcceleratorSkip|TestAccComputeInstanceTemplate_guestAccelerator|TestAccComputeInstanceTemplate_secondaryAliasIpRange|TestAccComputeInstanceTemplate_primaryAliasIpRange|TestAccComputeInstanceTemplate_metadata_startup_script|TestAccComputeInstanceTemplate_subnet_auto|TestAccComputeInstanceTemplate_subnet_custom|TestAccInstanceTemplateDatasource_filter|TestAccInstanceTemplateDatasource_filter_mostRecent|TestAccInstanceTemplateDatasource_name|TestAccDataSourceGoogleComputeInstanceGroup_fromIGM|TestAccDataSourceGoogleComputeInstanceGroupManager|TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample |
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 ( 8 files changed, 135 insertions(+), 14 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeBackendService_withBackendAndMaxUtilization|TestAccComputeBackendService_withBackendAndIAP|TestAccComputeInstanceFromTemplate_overrideMetadataDotStartupScript|TestAccComputeRegionAutoscaler_update|TestAccComputeRegionBackendService_withBackendMultiNic|TestAccComputeRegionBackendService_withBackendInternalManaged|TestAccComputeRegionBackendService_withBackendInternal|TestAccRegionInstanceGroupManager_update|TestAccRegionInstanceGroupManager_targetSizeZero|TestAccRegionInstanceGroupManager_basic|TestAccComputeRegionBackendService_regionBackendServiceBalancingModeExample|TestAccComputePerInstanceConfig_statefulIps|TestAccComputePerInstanceConfig_update|TestAccComputeRegionPerInstanceConfig_statefulIps|TestAccComputeRegionPerInstanceConfig_update|TestAccComputeInstanceTemplate_guestAcceleratorSkip|TestAccComputeInstanceTemplate_guestAccelerator|TestAccComputeInstanceTemplate_secondaryAliasIpRange|TestAccInstanceGroupManager_update|TestAccComputeInstanceTemplate_spot_maxRunDuration|TestAccInstanceGroupManager_targetSizeZero|TestAccInstanceGroupManager_basic|TestAccComputeInstanceTemplate_sourceSnapshotEncryptionKey|TestAccComputeInstanceTemplate_sourceImageEncryptionKey|TestAccComputeInstanceTemplate_primaryAliasIpRange|TestAccComputeInstanceTemplate_metadata_startup_script|TestAccComputeInstanceTemplate_managedEnvoy|TestAccComputeInstanceFromTemplate_012_removableFields|TestAccComputeInstanceTemplate_subnet_custom|TestAccComputeInstanceTemplate_queueCount|TestAccComputeInstanceTemplate_nictype_update|TestAccComputeInstanceTemplate_diskResourcePolicies|TestAccComputeInstanceTemplate_subnet_auto|TestAccComputeGlobalForwardingRule_externalHttpLbMigBackendCustomHeaderExample|TestAccComputeGlobalForwardingRule_externalTcpProxyLbMigBackendExample|TestAccRegionInstanceGroupManager_distributionPolicy|TestAccRegionInstanceGroupManager_autoHealingPolicies|TestAccRegionInstanceGroupManager_versions|TestAccComputeInstanceTemplate_networkIPAddress|TestAccComputeInstanceTemplate_spot|TestAccComputeInstanceTemplate_AdvancedMachineFeatures|TestAccComputeAutoscaler_scalingSchedule|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccComputeInstanceTemplate_networkIP|TestAccComputeBackendService_withBackend|TestAccComputeRegionAutoscaler_scaleInControl|TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample|TestAccComputeInstanceFromTemplate_overrideScheduling|TestAccComputeAutoscaler_scaleDownControl|TestAccComputeInstanceTemplate_ConfidentialInstanceConfigMain|TestAccComputeInstanceFromTemplate_overrideScratchDisk|TestAccComputeAutoscaler_multicondition|TestAccComputeRegionAutoscaler_scalingSchedule|TestAccComputeAutoscaler_update|TestAccComputeInstanceFromTemplate_overrideAttachedDisk|TestAccComputeRegionAutoscaler_scaleDownControl|TestAccComputeBackendService_withMaxConnectionsPerInstance|TestAccComputeRegionAutoscaler_regionAutoscalerBasicExample|TestAccComputeBackendService_withMaxConnections|TestAccComputeInstanceTemplate_IPv6|TestAccComputeAutoscaler_autoscalerBasicExample|TestAccComputeInstanceTemplate_shieldedVmConfig2|TestAccComputeInstanceTemplate_IP|TestAccComputeInstanceTemplate_shieldedVmConfig1|TestAccComputeInstanceTemplate_preemptible|TestAccComputeInstanceTemplate_imageShorthand|TestAccComputeInstanceTemplate_instanceResourcePolicies|TestAccComputeInstanceTemplate_basic|TestAccComputeAutoscaler_autoscalerSingleInstanceExample|TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample|TestAccComputeForwardingRule_forwardingRuleHttpLbExample|TestAccComputeGlobalForwardingRule_internalLoadBalancing|TestAccInstanceGroupManager_stateful|TestAccInstanceGroupManager_versions|TestAccComputeInstanceTemplate_EncryptKMS|TestAccComputeAutoscaler_scaleInControl|TestAccInstanceGroupManager_autoHealingPolicies|TestAccComputeInstanceTemplate_minCpuPlatform|TestAccComputeInstanceFromTemplate_basic|TestAccInstanceGroupManager_waitForStatus|TestAccComputeAutoscaler_scaleInControlFixed|TestAccComputeInstanceFromTemplate_overrideBootDisk|TestAccComputeGlobalForwardingRule_globalForwardingRuleInternalExample|TestAccComputeInstanceTemplate_reservationAffinities|TestAccDataSourceGoogleComputeInstanceGroup_fromIGM|TestAccDataSourceGoogleComputeInstanceGroupManager|TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample|TestAccInstanceTemplateDatasource_filter_mostRecent|TestAccInstanceTemplateDatasource_filter|TestAccInstanceTemplateDatasource_name |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
The test failure may due to dangling resources in our test project from some previous tests (possibly test run in other PR). Would you mind modifying the test Also rebasing the PR should fix Sorry about the extra work. |
Thanks for the hints! I adjusted the PR as requested. |
/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 ( 8 files changed, 138 insertions(+), 17 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 4 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstanceTemplate_maintenance_interval|TestAccComputeInstanceTemplate_sourceSnapshotEncryptionKey|TestAccCloudRunV2Job_cloudrunv2JobFullUpdate |
Tests passed during RECORDING mode: All tests passed |
Hi Shuya! |
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 overall! Only one small question. Also, would you mind rebasing the PR? Thanks you so much for the work!
Just want to make sure do we know that if this change will affect existing resource? Have you tested it locally?
mmv1/third_party/terraform/resources/resource_compute_instance_group_manager.go.erb
Show resolved
Hide resolved
/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 ( 8 files changed, 139 insertions(+), 15 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 6 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstanceTemplate_sourceSnapshotEncryptionKey|TestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample|TestAccComposerEnvironment_withEncryptionConfigComposer2|TestAccDataSourceGoogleComputeInstanceGroupManager|TestAccDataSourceDnsManagedZone_basic |
I'm not sure we can make this change in Adding a new attribute field and modifying examples to use that one, as well as adding a note to avoid using We can change the |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Hi @rileykarson, Unfortunately this is somewhat more complicated. As one of my early attempts, I did try adding a new field to expose the unique id of the instance template. The problem is, even if you create a MIG with a unique id based instance template reference, Google APIs return the MIG with the name based instance template. Like here:
When Terraform reads it back:
Then, Terraform complains "After applying this test step, the plan was not empty." as it thinks the MIG has changed. This means, adding a new field alone is not enough; the comparison logic is to be adjusted as well. What is a DSF? :$ Instead of adding a brand new field (and confusing users which of the 3 to use), can we make this with changing the structure of self-link only? In other words, do you consider changing self-links as breaking change as well? |
A DSF is a "DiffSuppressFunc", a change to the comparison logic. It allows you to define a rule to let Terraform preserve the value stored in state rather than than use the value written in config. For example- if an API returned "FOO" and the user had written "bar", and those were equivalent.
That should cause problems with the current implementation as well, I think? It's almost certainly gonna have a nonobvious interaction going on based on DSFs working as described above,
Yeah- changing an output value's format is generally a breaking change. This is because a user using it as a reference- including across provider versions / configurations- would be impacted. If there was zero impact to an existing config it'd be fine, but this change will definitely have one. |
TestAccDataSourceDnsManagedZone_basic failure is unrelated. No need to worry about it and sorry for the noise. Thanks for the refactoring. Start reviewing. |
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.
Overall LGTM! Only a couple of small comments. Thanks for including detailed testing!
mmv1/third_party/terraform/website/docs/d/compute_instance_template.html.markdown
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/d/compute_instance_template.html.markdown
Outdated
Show resolved
Hide resolved
mmv1/third_party/terraform/data_sources/data_source_google_compute_instance_template.go.erb
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/r/compute_instance_from_template.html.markdown
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/r/compute_instance_group_manager.html.markdown
Show resolved
Hide resolved
mmv1/third_party/terraform/website/docs/r/compute_region_instance_group_manager.html.markdown
Show resolved
Hide resolved
/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 ( 14 files changed, 444 insertions(+), 27 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 7 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccNetworkServicesGateway_update|TestAccFirebaserulesRelease_BasicRelease|TestAccComputeInstanceFromTemplate_self_link_unique|TestAccComputeInstanceTemplate_with18TbScratchDisk|TestAccInstanceTemplateDatasource_self_link_unique|TestAccDataSourceGoogleCloudAssetResourcesSearchAll_basic|TestAccDataSourceDnsManagedZone_basic |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
/gcbrun To kick off VCR replaying to check if cassettes are recorded 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.
LGTM! Would you mind resolving file conflicts? Thanks!
…ore a follow-up operation (e.g. MIG creation) is started.
I just rebased this change again. Thanks! |
Thank you! |
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 ( 14 files changed, 443 insertions(+), 28 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 31 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccWorkstationsWorkstationConfig_workstationConfigEncryptionKeyExample|TestAccNetworkServicesGateway_update|TestAccFrameworkProviderBasePath_setBasePath|TestAccFrameworkProviderMeta_setModuleName|TestAccComputeRegionPerInstanceConfig_statefulIps|TestAccComputeRegionBackendService_regionBackendServiceBalancingModeExample|TestAccComputeInstanceTemplate_subnet_custom|TestAccComputeInstanceTemplate_subnet_auto|TestAccComputeInstanceFromRegionTemplate_basic|TestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample|TestAccComputeForwardingRule_internalTcpUdpLbWithMigBackendExample|TestAccComputeForwardingRule_internalHttpLbWithMigBackendExample|TestAccComputeInstanceFromTemplate_self_link_unique|TestAccComputeInstanceTemplate_IPv6|TestAccComputeGlobalForwardingRule_externalHttpLbMigBackendCustomHeaderExample|TestAccComputeGlobalForwardingRule_externalTcpProxyLbMigBackendExample|TestAccDataSourceGoogleFirebaseAppleAppConfig|TestAccInstanceGroupManager_stateful|TestAccComputeInstanceTemplate_secondaryAliasIpRange|TestAccInstanceTemplateDatasource_self_link_unique|TestAccComputeRegionBackendService_withBackendMultiNic|TestAccCloudRunV2Job_cloudrunv2JobFullUpdate|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccComposerEnvironment_withEncryptionConfigComposer2|TestAccComposerEnvironment_withEncryptionConfigComposer1|TestAccComputePerInstanceConfig_statefulIps|TestAccDataSourceDnsRecordSet_basic|TestAccDataSourceDNSKeys_basic|TestAccDataSourceDNSKeys_noDnsSec|TestAccDataSourceDnsManagedZone_basic|TestAccComputeForwardingRule_forwardingRuleHttpLbExample |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Several tests got terminated during RECORDING mode. |
clean up unneeded cassettes |
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 ( 14 files changed, 443 insertions(+), 28 deletions(-)) |
Tests analyticsTotal tests: Action takenFound 13 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccComputeForwardingRule_forwardingRuleRegionalHttpXlbExample|TestAccComposerEnvironment_withEncryptionConfigComposer2|TestAccComputeInstanceFromTemplate_self_link_unique|TestAccComputeInstanceFromRegionTemplate_basic|TestAccFrameworkProviderMeta_setModuleName|TestAccFrameworkProviderBasePath_setBasePath|TestAccInstanceTemplateDatasource_self_link_unique|TestAccDataSourceGoogleFirebaseAppleAppConfig|TestAccFirebaserulesRelease_BasicRelease|TestAccDataSourceDnsManagedZone_basic|TestAccDataSourceDNSKeys_noDnsSec|TestAccDataSourceDnsRecordSet_basic|TestAccDataSourceDNSKeys_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.
Thanks!
This is a security improvement to prevent instance templates being replaced after an instance template is created and before it is used by another resource (e.g. MIG creation). It introduces the
self_link_unique
computed attribute forgoogle_compute_instance_template
's that can be used to prevent TOCTOU attacks when resources are created in an untrusted environment.If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
in the generated providers to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)