-
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
fix networking_mode
in container_cluster
#5504
Conversation
Oops! It looks like you're using an unknown release-note type in your changelog entries:
Please only use the types listed in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/.ci/RELEASE_NOTES_GUIDE.md. |
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. @rileykarson, please review this PR or find an appropriate assignee. |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccContainerClusterDatasource_zonal|TestAccContainerClusterDatasource_regional|TestAccApigeeEnvironmentIamBindingGenerated|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudFunctionsFunction_vpcConnector|TestAccComposerEnvironment_withUpdateOnCreate|TestAccComputeBackendService_withMaxConnections|TestAccComputeBackendService_withMaxConnectionsPerInstance|TestAccComputeBackendService_withMaxRatePerEndpoint|TestAccComputeBackendService_withMaxConnectionsPerEndpoint|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerCluster_basic|TestAccContainerCluster_misc|TestAccContainerCluster_withConfidentialNodes|TestAccContainerCluster_withAddons|TestAccContainerCluster_withNotificationConfig|TestAccContainerCluster_withILBSubsetting|TestAccContainerCluster_withMasterAuthConfig_NoCert|TestAccContainerCluster_withNetworkPolicyEnabled|TestAccContainerCluster_withReleaseChannelEnabled|TestAccContainerCluster_withMasterAuthorizedNetworksConfig|TestAccContainerCluster_withTelemetryEnabled|TestAccContainerCluster_withReleaseChannelEnabledDefaultVersion|TestAccContainerCluster_regional|TestAccContainerCluster_regionalWithNodePool|TestAccContainerCluster_regionalWithNodeLocations|TestAccContainerCluster_withIntraNodeVisibility|TestAccContainerCluster_withVersion|TestAccContainerCluster_updateVersion|TestAccContainerCluster_withNodeConfig|TestAccContainerCluster_withNodeConfigScopeAlias|TestAccContainerCluster_withNodeConfigShieldedInstanceConfig|TestAccContainerCluster_withWorkloadMetadataConfig|TestAccContainerCluster_withNodePoolBasic|TestAccContainerCluster_withSandboxConfig|TestAccContainerCluster_network|TestAccContainerCluster_backend|TestAccContainerCluster_withNodePoolResize|TestAccContainerCluster_withNodePoolAutoscaling|TestAccContainerCluster_withNodePoolUpdateVersion|TestAccContainerCluster_withNodePoolMultiple|TestAccContainerCluster_withNodePoolNodeConfig|TestAccContainerCluster_withMaintenanceWindow|TestAccContainerCluster_withRecurringMaintenanceWindow|TestAccContainerCluster_withMaintenanceExclusionWindow|TestAccContainerCluster_deleteExclusionWindow|TestAccContainerCluster_withShieldedNodes|TestAccContainerCluster_nodeAutoprovisioningDefaults|TestAccContainerCluster_nodeAutoprovisioning|TestAccContainerCluster_errorAutopilotLocation|TestAccContainerCluster_withLoggingConfig|TestAccContainerCluster_withWorkloadIdentityConfig|TestAccContainerCluster_withAutoscalingProfile|TestAccContainerCluster_withSoleTenantGroup|TestAccContainerCluster_withBinaryAuthorization|TestAccContainerCluster_nodeAutoprovisioningDefaultsMinCpuPlatform|TestAccContainerCluster_errorNoClusterCreated|TestAccContainerCluster_withEnableKubernetesAlpha|TestAccContainerCluster_withDatabaseEncryption|TestAccContainerCluster_withResourceUsageExportConfig|TestAccContainerCluster_withDNSConfig|TestAccContainerNodePool_basic|TestAccContainerNodePool_basicWithClusterId|TestAccContainerNodePool_withNodeConfig|TestAccContainerNodePool_withWorkloadIdentityConfig|TestAccContainerNodePool_withSandboxConfig|TestAccContainerNodePool_withKubeletConfig|TestAccContainerNodePool_withLinuxNodeConfig|TestAccContainerNodePool_withUpgradeSettings|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccContainerNodePool_withManagement|TestAccContainerNodePool_withGPU|TestAccContainerNodePool_withNodeConfigScopeAlias|TestAccContainerNodePool_autoscaling|TestAccContainerNodePool_regionalAutoscaling|TestAccContainerNodePool_resize|TestAccContainerNodePool_regionalClusters|TestAccContainerNodePool_version|TestAccContainerNodePool_EmptyGuestAccelerator|TestAccContainerNodePool_shieldedInstanceConfig|TestAccContainerNodePool_012_ConfigModeAttr|TestAccContainerNodePool_ephemeralStorageConfig|TestAccContainerNodePool_gcfsConfig|TestAccGKEHubMembership_gkehubMembershipBasicExample|TestAccGKEHubMembership_gkehubMembershipIssuerExample|TestAccNotebooksInstance_notebookInstanceFullExample|TestAccNotebooksInstance_update You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=226543 |
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.
Oh, interesting- we have no tests that set this to ROUTES
apparently. Was the issue only with what was getting recorded in state? expandX
controls the message sent to the API, so it's unusual that routes-based clusters were getting created correctly and just misrecorded. I would have expected the created clusters to be VPC Native.
Sorry for the noise in the build steps, also! Our CI is angry at us.
Well the issue that we ran into was that the one test for this resource (to create a routes-baseded cluster) was failing due to this: Ah, I'm glad your CI is broken and it's not this PR. I was concerned I had really messed something up. |
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 small ask while you're here, do you mind unmarking the field as beta in https://github.com/GoogleCloudPlatform/magic-modules/blob/master/mmv1/third_party/terraform/website/docs/r/container_cluster.html.markdown? It seems that got missed at some point.
Yup, give me one second, I'll update this PR. |
`networking_mode` was incorrectly being set to `VPC_NATIVE` on routes-based clusters because the return value in `expandIPAllocationPolicy` was not setting `use_routes` to true.
d1ebf7a
to
86dcb97
Compare
Alrighty, after some Git struggles, I updated the resource description to remove the 'beta' tag. |
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, thanks! Sorry for the extraneous cleanup, I noticed the field beta tagged in the docs while reviewing.
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccDataSourceGoogleServiceAccountIdToken_impersonation|TestAccDatasourceGoogleServiceNetworkingPeeredDnsDomain_basic|TestAccApigeeEnvironmentIamMemberGenerated|TestAccApigeeEnvironmentIamPolicyGenerated|TestAccCloudFunctionsFunction_vpcConnector|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample|TestAccContainerCluster_errorAutopilotLocation|TestAccContainerNodePool_withInvalidUpgradeSettings You can view the result here: https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=227597 |
Tests failed during RECORDING mode: TestAccContainerCluster_errorAutopilotLocation|TestAccCloudFunctionsFunction_vpcConnector|TestAccContainerNodePool_withInvalidUpgradeSettings|TestAccComputeRegionNetworkEndpointGroup_regionNetworkEndpointGroupAppengineExample Please fix these to complete your PR |
Co-authored-by: Emily Cai <[email protected]>
networking_mode
was incorrectly being set toVPC_NATIVE
onroutes-based clusters because the return value in
expandIPAllocationPolicy
was not settinguse_routes
to true. This was causing one of KCC's tests to fail becausenetworking_mode
was changing when it is an immutable field.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)