Allow specifying global L7 ILB in dns_record_set routing policy #8470

hkundag · 2023-07-27T20:00:31Z

This resource currently only allows specifying regional L4 ILB targets to health check in routing policies (see the #6665). We will add support for specifying regional L7 ILBs (regional internal HTTP(S) load balancers) as well.

b/293348842

If this PR is for Terraform, I acknowledge that I have:

Searched through the issue tracker for an open issue that this either resolves or contributes to, commented on it to claim it, and written "fixes {url}" or "part of {url}" in this PR description. If there were no relevant open issues, I opened one and commented that I would like to work on it (not necessary for very small changes).
Ensured that all new fields I added that can be set by a user appear in at least one example (for generated resources) or third_party test (for handwritten resources or update tests).
Generated Terraform providers, and ran make test and make lint in the generated providers to ensure it passes unit and linter tests.
Ran relevant acceptance tests using my own Google Cloud project and credentials (If the acceptance tests do not yet pass or you are unable to run them, please let your reviewer know).
Read Write release notes before writing my release note below.

Release Note Template for Downstream PRs (will be copied)

dns: added `globalL7ilb` enum support to the `routing_policy.load_balancer_type` field in `google_dns_record_set`

modular-magician · 2023-07-27T20:01:07Z

Hello! I am a robot. It looks like you are a community contributor. @melinath, 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.

melinath

commenting to remove from my review queue since this is still a draft. Let me know when it's ready for review :-)

modular-magician · 2023-08-10T00:03:08Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 205 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 3 files changed, 205 insertions(+), 2 deletions(-))

modular-magician · 2023-08-10T01:52:34Z

Tests analytics

Total tests: 2939
Passed tests 2636
Skipped tests: 302
Affected tests: 1

Action taken

Found 1 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccDNSRecordSet_routingPolicy

Get to know how VCR tests work

modular-magician · 2023-08-10T02:07:16Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccDNSRecordSet_routingPolicy[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

hkundag · 2023-08-10T02:08:31Z

Ready for review!

modular-magician · 2023-08-14T14:18:12Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 205 insertions(+), 2 deletions(-))
Terraform Beta: Diff ( 3 files changed, 205 insertions(+), 2 deletions(-))

modular-magician · 2023-08-14T15:36:59Z

Tests analytics

Total tests: 2945
Passed tests 2638
Skipped tests: 302
Affected tests: 5

Action taken

Found 5 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccBigQueryDataTable_bigtable|TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample|TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample|TestAccDataSourceGoogleServiceAccountIdToken_impersonation

Get to know how VCR tests work

modular-magician · 2023-08-14T15:48:44Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccBigQueryDataTable_bigtable[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileAnyclusterExample[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample[Debug log]
TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample[Debug log]
TestAccDataSourceGoogleServiceAccountIdToken_impersonation[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

melinath · 2023-08-15T17:42:25Z

mmv1/third_party/terraform/website/docs/r/dns_record_set.html.markdown

@@ -339,6 +339,97 @@ resource "google_compute_network" "prod" {
 }
 ```

+#### Primary-Backup with a Cross-region internal Application Load Balancer


In general, the goal of the examples in the reference documentation is to show basic usage of the resource, not to cover every possible configuration. However, the examples that are provided would ideally be complete / copy-pasteable and runnable.

Looking closer at this example, I'd lean towards not including it, since it's essentially identical to "Primary-Backup with a regional L7 ILB" (except for the load_balancer_type value, which should already be covered by the reference docs, and region, which it would make sense to not specify for a global resource.)

So I'd recommend:

Delete this example (and possibly "Primary-Backup with a regional L7 ILB" as well since it's basically identical to "Primary-Backup") from the docs. You could also modify "Primary-Backup" to make sure that example is the recommended path.

Update the docs for load_balancer_type to include globalL7ilb as a valid value

modular-magician · 2023-08-15T18:37:46Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 115 insertions(+), 101 deletions(-))
Terraform Beta: Diff ( 3 files changed, 115 insertions(+), 101 deletions(-))

modular-magician · 2023-08-15T18:47:58Z

Hi there, I'm the Modular magician. I've detected the following information about your changes:

Diff report

Your PR generated some diffs in downstreams - here they are.

Terraform GA: Diff ( 3 files changed, 115 insertions(+), 101 deletions(-))
Terraform Beta: Diff ( 3 files changed, 115 insertions(+), 101 deletions(-))

modular-magician · 2023-08-15T19:58:02Z

Tests analytics

Total tests: 2951
Passed tests 2646
Skipped tests: 302
Affected tests: 3

Action taken

Found 3 affected test(s) by replaying old test recordings. Starting RECORDING based on the most recent commit. Click here to see the affected tests

TestAccCertificateManagerCertificate_certificateManagerSelfManagedCertificateExample|TestAccCertificateManagerCertificate_certificateManagerGoogleManagedCertificateIssuanceConfigExample|TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileCloudsqlExample

Get to know how VCR tests work

modular-magician · 2023-08-15T20:11:49Z

$\textcolor{green}{\textsf{Tests passed during RECORDING mode:}}$
TestAccCertificateManagerCertificate_certificateManagerSelfManagedCertificateExample[Debug log]
TestAccCertificateManagerCertificate_certificateManagerGoogleManagedCertificateIssuanceConfigExample[Debug log]
TestAccDatabaseMigrationServiceConnectionProfile_databaseMigrationServiceConnectionProfileCloudsqlExample[Debug log]

Rerun these tests in REPLAYING mode to catch issues

$\textcolor{green}{\textsf{No issues found for passed tests after REPLAYING rerun.}}$

$\textcolor{green}{\textsf{All tests passed!}}$
View the build log or the debug log for each test

melinath

LGTM

melinath · 2023-08-15T23:01:02Z

Merging according to the passing VCR tests even though the statuses seem to be messed up.

…leCloudPlatform#8470) * Allow specifying global L7 ILB in dns_record_set routing policy * fix url_map default_service in docs * fix forwarding rule reference in rrset for cross-region test * don't specify region for globalL7ilb routing policy item * add backend subnet for FR IP address provisioning * pass backendSubnetName to cross-region L7 test case * remove google-beta req from test backend subnet * remove extra primary-backup examples * add globalL7ilb value to docs

modular-magician requested a review from melinath July 27, 2023 20:01

This comment was marked as outdated.

Sign in to view

melinath reviewed Jul 27, 2023

View reviewed changes

modular-magician requested a review from melinath August 8, 2023 18:33

This comment was marked as outdated.

Sign in to view

hkundag force-pushed the gl7 branch from 1e37484 to ec041ab Compare August 8, 2023 21:21

This comment was marked as outdated.

Sign in to view

hkundag marked this pull request as ready for review August 10, 2023 02:08

hkundag added 6 commits August 14, 2023 14:06

fix url_map default_service in docs

4d91ae4

fix forwarding rule reference in rrset for cross-region test

53a9d69

don't specify region for globalL7ilb routing policy item

2beb55d

add backend subnet for FR IP address provisioning

b1d1174

pass backendSubnetName to cross-region L7 test case

cde9aab

remove google-beta req from test backend subnet

381c543

hkundag force-pushed the gl7 branch from f64e7e7 to 381c543 Compare August 14, 2023 14:08

modular-magician requested a review from melinath August 14, 2023 14:09

melinath requested changes Aug 15, 2023

View reviewed changes

hkundag added 2 commits August 15, 2023 17:52

remove extra primary-backup examples

d02f504

add globalL7ilb value to docs

8de4f20

modular-magician requested a review from melinath August 15, 2023 18:30

melinath approved these changes Aug 15, 2023

View reviewed changes

melinath merged commit 5c249d2 into GoogleCloudPlatform:main Aug 15, 2023
7 of 8 checks passed

Allow specifying global L7 ILB in dns_record_set routing policy #8470

Allow specifying global L7 ILB in dns_record_set routing policy #8470

Conversation

hkundag commented Jul 27, 2023 • edited Loading

modular-magician commented Jul 27, 2023

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

melinath left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

This comment was marked as outdated.

modular-magician commented Aug 10, 2023

Diff report

modular-magician commented Aug 10, 2023

Tests analytics

Action taken

modular-magician commented Aug 10, 2023

Rerun these tests in REPLAYING mode to catch issues

hkundag commented Aug 10, 2023

modular-magician commented Aug 14, 2023

Diff report

modular-magician commented Aug 14, 2023

Tests analytics

Action taken

modular-magician commented Aug 14, 2023

Rerun these tests in REPLAYING mode to catch issues

melinath Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

modular-magician commented Aug 15, 2023

Diff report

modular-magician commented Aug 15, 2023

Diff report

modular-magician commented Aug 15, 2023

Tests analytics

Action taken

modular-magician commented Aug 15, 2023

Rerun these tests in REPLAYING mode to catch issues

melinath left a comment

Choose a reason for hiding this comment

melinath commented Aug 15, 2023

hkundag commented Jul 27, 2023 •

edited

Loading

melinath Aug 15, 2023 •

edited

Loading