-
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
Public IP Support for AlloyDB #10331
Conversation
…on update + add e2e tests
G'day! I apologize - but is there an ETA for this change? I had a change on #10100, which got closed in favor of this change - would love to provide any support to get this merged! Thank you! |
Hello! I'm hoping to send this PR for review next Wednesday or Thursday after meeting with the TF team to discuss about internal details on the API and supporting that in TF. |
API currently prohibits public IP to be created on instance creation so we have to use the custom code to get around this undesirable behavior in TF. Also removed the more friendly user error messages custom code in the update path in favor of putting the information in the field descriptions.
Hello! I am a robot. Tests will require approval from a repository maintainer to run. @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. |
mmv1/third_party/terraform/services/alloydb/resource_alloydb_instance_test.go
Show resolved
Hide resolved
/gcbrun |
I'm not sure why this is breaking, I'd guess we changed something on main recently. Can you rebase/merge main into this? |
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_alloydb_instance" "primary" {
network_config = # value needed
network_config {
authorized_external_networks = # value needed
}
public_ip_address = # 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_alloydb_instance" "primary" {
network_config = # value needed
network_config {
authorized_external_networks = # value needed
}
public_ip_address = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 39 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbBackup_alloydbBackupBasicTestExample|TestAccAlloydbBackup_createBackupWithMandatoryFields|TestAccAlloydbBackup_update|TestAccAlloydbBackup_usingCMEK|TestAccAlloydbCluster_alloydbSecondaryClusterBasicTestExample|TestAccAlloydbCluster_restore|TestAccAlloydbCluster_secondaryClusterDefinedSecondaryConfigButClusterTypeIsPrimary|TestAccAlloydbCluster_secondaryClusterMissingSecondaryConfig|TestAccAlloydbCluster_secondaryClusterPromote|TestAccAlloydbCluster_secondaryClusterPromoteAndAddAndDeleteAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_secondaryClusterPromoteAndAddContinuousBackupConfig|TestAccAlloydbCluster_secondaryClusterPromoteAndDeleteOriginalPrimary|TestAccAlloydbCluster_secondaryClusterPromoteAndDeleteTimeBasedRetentionPolicy|TestAccAlloydbCluster_secondaryClusterPromoteAndSimultaneousUpdate|TestAccAlloydbCluster_secondaryClusterPromoteAndUpdate|TestAccAlloydbCluster_secondaryClusterPromoteWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_secondaryClusterUpdate|TestAccAlloydbCluster_secondaryClusterUsingCMEK|TestAccAlloydbCluster_secondaryClusterWithNetworkConfig|TestAccAlloydbCluster_secondaryClusterWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_secondaryInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_alloydbInstanceBasicTestExample|TestAccAlloydbInstance_alloydbSecondaryInstanceBasicTestExample|TestAccAlloydbInstance_clientConnectionConfig|TestAccAlloydbInstance_createInstanceWithMandatoryFields|TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_createPrimaryAndReadPoolInstance|TestAccAlloydbInstance_networkConfig|TestAccAlloydbInstance_secondaryInstanceMaximumFields|TestAccAlloydbInstance_secondaryInstanceUpdateDatabaseFlag|TestAccAlloydbInstance_secondaryInstanceUpdateMachineConfig|TestAccAlloydbInstance_secondaryInstanceUpdateQueryInsightConfig|TestAccAlloydbInstance_secondaryInstanceWithReadPoolInstance|TestAccAlloydbInstance_update|TestAccAlloydbUser_alloydbUserBuiltinTestExample|TestAccAlloydbUser_alloydbUserIamTestExample|TestAccAlloydbUser_updatePassword_BuiltIn|TestAccAlloydbUser_updateRoles_BuiltIn|TestAccAlloydbUser_updateRoles_IAM |
1 similar comment
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 39 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbBackup_alloydbBackupBasicTestExample|TestAccAlloydbBackup_createBackupWithMandatoryFields|TestAccAlloydbBackup_update|TestAccAlloydbBackup_usingCMEK|TestAccAlloydbCluster_alloydbSecondaryClusterBasicTestExample|TestAccAlloydbCluster_restore|TestAccAlloydbCluster_secondaryClusterDefinedSecondaryConfigButClusterTypeIsPrimary|TestAccAlloydbCluster_secondaryClusterMissingSecondaryConfig|TestAccAlloydbCluster_secondaryClusterPromote|TestAccAlloydbCluster_secondaryClusterPromoteAndAddAndDeleteAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_secondaryClusterPromoteAndAddContinuousBackupConfig|TestAccAlloydbCluster_secondaryClusterPromoteAndDeleteOriginalPrimary|TestAccAlloydbCluster_secondaryClusterPromoteAndDeleteTimeBasedRetentionPolicy|TestAccAlloydbCluster_secondaryClusterPromoteAndSimultaneousUpdate|TestAccAlloydbCluster_secondaryClusterPromoteAndUpdate|TestAccAlloydbCluster_secondaryClusterPromoteWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_secondaryClusterUpdate|TestAccAlloydbCluster_secondaryClusterUsingCMEK|TestAccAlloydbCluster_secondaryClusterWithNetworkConfig|TestAccAlloydbCluster_secondaryClusterWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_secondaryInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_alloydbInstanceBasicTestExample|TestAccAlloydbInstance_alloydbSecondaryInstanceBasicTestExample|TestAccAlloydbInstance_clientConnectionConfig|TestAccAlloydbInstance_createInstanceWithMandatoryFields|TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_createPrimaryAndReadPoolInstance|TestAccAlloydbInstance_networkConfig|TestAccAlloydbInstance_secondaryInstanceMaximumFields|TestAccAlloydbInstance_secondaryInstanceUpdateDatabaseFlag|TestAccAlloydbInstance_secondaryInstanceUpdateMachineConfig|TestAccAlloydbInstance_secondaryInstanceUpdateQueryInsightConfig|TestAccAlloydbInstance_secondaryInstanceWithReadPoolInstance|TestAccAlloydbInstance_update|TestAccAlloydbUser_alloydbUserBuiltinTestExample|TestAccAlloydbUser_alloydbUserIamTestExample|TestAccAlloydbUser_updatePassword_BuiltIn|TestAccAlloydbUser_updateRoles_BuiltIn|TestAccAlloydbUser_updateRoles_IAM |
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_alloydb_instance" "primary" {
network_config = # value needed
network_config {
authorized_external_networks = # value needed
}
public_ip_address = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 39 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbBackup_alloydbBackupBasicTestExample|TestAccAlloydbBackup_createBackupWithMandatoryFields|TestAccAlloydbBackup_update|TestAccAlloydbBackup_usingCMEK|TestAccAlloydbCluster_alloydbSecondaryClusterBasicTestExample|TestAccAlloydbCluster_restore|TestAccAlloydbCluster_secondaryClusterDefinedSecondaryConfigButClusterTypeIsPrimary|TestAccAlloydbCluster_secondaryClusterMissingSecondaryConfig|TestAccAlloydbCluster_secondaryClusterPromote|TestAccAlloydbCluster_secondaryClusterPromoteAndAddAndDeleteAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_secondaryClusterPromoteAndAddContinuousBackupConfig|TestAccAlloydbCluster_secondaryClusterPromoteAndDeleteOriginalPrimary|TestAccAlloydbCluster_secondaryClusterPromoteAndDeleteTimeBasedRetentionPolicy|TestAccAlloydbCluster_secondaryClusterPromoteAndSimultaneousUpdate|TestAccAlloydbCluster_secondaryClusterPromoteAndUpdate|TestAccAlloydbCluster_secondaryClusterPromoteWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_secondaryClusterUpdate|TestAccAlloydbCluster_secondaryClusterUsingCMEK|TestAccAlloydbCluster_secondaryClusterWithNetworkConfig|TestAccAlloydbCluster_secondaryClusterWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_secondaryInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_alloydbInstanceBasicTestExample|TestAccAlloydbInstance_alloydbSecondaryInstanceBasicTestExample|TestAccAlloydbInstance_clientConnectionConfig|TestAccAlloydbInstance_createInstanceWithMandatoryFields|TestAccAlloydbInstance_createInstanceWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_createPrimaryAndReadPoolInstance|TestAccAlloydbInstance_networkConfig|TestAccAlloydbInstance_secondaryInstanceMaximumFields|TestAccAlloydbInstance_secondaryInstanceUpdateDatabaseFlag|TestAccAlloydbInstance_secondaryInstanceUpdateMachineConfig|TestAccAlloydbInstance_secondaryInstanceUpdateQueryInsightConfig|TestAccAlloydbInstance_secondaryInstanceWithReadPoolInstance|TestAccAlloydbInstance_update|TestAccAlloydbUser_alloydbUserBuiltinTestExample|TestAccAlloydbUser_alloydbUserIamTestExample|TestAccAlloydbUser_updatePassword_BuiltIn|TestAccAlloydbUser_updateRoles_BuiltIn|TestAccAlloydbUser_updateRoles_IAM |
Looks like most of them failed due to a quota issue, probably because the e2e test was being run so much as I was making commits. They should pass on a retry. Other than that I forgot the quotes around the cidr_range in my new test Errors:
|
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_alloydb_instance" "primary" {
network_config = # value needed
network_config {
authorized_external_networks = # value needed
}
public_ip_address = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 18 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbCluster_alloydbSecondaryClusterBasicTestExample|TestAccAlloydbCluster_secondaryClusterPromote|TestAccAlloydbCluster_secondaryClusterPromoteAndAddAndDeleteAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_secondaryClusterPromoteAndAddContinuousBackupConfig|TestAccAlloydbCluster_secondaryClusterPromoteAndDeleteOriginalPrimary|TestAccAlloydbCluster_secondaryClusterPromoteAndDeleteTimeBasedRetentionPolicy|TestAccAlloydbCluster_secondaryClusterPromoteAndSimultaneousUpdate|TestAccAlloydbCluster_secondaryClusterPromoteAndUpdate|TestAccAlloydbCluster_secondaryClusterPromoteWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_secondaryClusterUpdate|TestAccAlloydbCluster_secondaryClusterUsingCMEK|TestAccAlloydbCluster_secondaryClusterWithNetworkConfig|TestAccAlloydbCluster_secondaryClusterWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_alloydbSecondaryInstanceBasicTestExample|TestAccAlloydbInstance_networkConfig|TestAccAlloydbInstance_secondaryInstanceUpdateDatabaseFlag|TestAccAlloydbInstance_secondaryInstanceUpdateMachineConfig|TestAccAlloydbInstance_secondaryInstanceUpdateQueryInsightConfig |
|
Where are you seeing the quota issues? I'm seeing the following: |
I see the quota issues in the debug logs. Also see it in us-south1 as well |
/gcbrun cleaned up some extra clusters |
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_alloydb_instance" "primary" {
network_config = # value needed
network_config {
authorized_external_networks = # value needed
}
public_ip_address = # value needed
}
|
Tests analyticsTotal tests: Click here to see the affected service packages
Action takenFound 17 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected testsTestAccAlloydbCluster_alloydbSecondaryClusterBasicTestExample|TestAccAlloydbCluster_secondaryClusterPromote|TestAccAlloydbCluster_secondaryClusterPromoteAndAddAndDeleteAutomatedBackupPolicyAndInitialUser|TestAccAlloydbCluster_secondaryClusterPromoteAndAddContinuousBackupConfig|TestAccAlloydbCluster_secondaryClusterPromoteAndDeleteOriginalPrimary|TestAccAlloydbCluster_secondaryClusterPromoteAndDeleteTimeBasedRetentionPolicy|TestAccAlloydbCluster_secondaryClusterPromoteAndSimultaneousUpdate|TestAccAlloydbCluster_secondaryClusterPromoteAndUpdate|TestAccAlloydbCluster_secondaryClusterPromoteWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbCluster_secondaryClusterUpdate|TestAccAlloydbCluster_secondaryClusterUsingCMEK|TestAccAlloydbCluster_secondaryClusterWithNetworkConfig|TestAccAlloydbCluster_secondaryClusterWithNetworkConfigAndAllocatedIPRange|TestAccAlloydbInstance_alloydbSecondaryInstanceBasicTestExample|TestAccAlloydbInstance_secondaryInstanceUpdateDatabaseFlag|TestAccAlloydbInstance_secondaryInstanceUpdateMachineConfig|TestAccAlloydbInstance_secondaryInstanceUpdateQueryInsightConfig |
|
@slevenick Could you take another look at this PR? The VCR tests passed after you cleaned up the project |
The linked issue talks about adding this to the beta provider. Is it supposed to be in GA or only beta? If only beta you will need to mark it as such in the YAML, and add version guards on all the custom code. |
It should be in GA as well. I can create a bug if need be for Github, but on the AlloyDB side we had one filed already to keep track of this |
Description:
Add support to enable public IP on an AlloyDB instance, and set a list of authorized external networks that are able to connect to the public IP of the instance. This PR has some custom code to allow instances to be created with public IP enabled as the API currently prohibits it. I plan on removing this custom code in a future PR once the API is updated, but this shouldn't be a breaking change.
This is for beta and GA terraform support.
Issue:
Release Note Template for Downstream PRs (will be copied)