-
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
Add fields related to health checked targets to dns_record_set resource #6665
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've detected that you're a community contributor. @shuyama1, a repository maintainer, has been assigned to assist you and help review your changes. ❓ First time contributing? Click here for more detailsYour assigned reviewer will help review your code by:
You can help make sure that review is quick by running local tests and ensuring they're passing in between each push you make to your PR's branch. Also, try to leave a comment with each push you make, as pushes generally don't generate emails. If your reviewer doesn't get back to you within a week after your most recent change, please feel free to leave a comment on the issue asking them to take a look! In the absence of a dedicated review dashboard most maintainers manage their pending reviews through email, and those will sometimes get lost in their inbox. |
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 558 insertions(+), 117 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeInstance_soleTenantNodeAffinities|TestAccCGCSnippet_eventarcWorkflowsExample|TestAccFirebaserulesRelease_BasicRelease|TestAccDNSRecordSet_routingPolicy|TestAccDNSRecordSet_changeRouting |
Tests passed during RECORDING mode: Tests failed during RECORDING mode: Please fix these to complete your PR |
Elem: geoPolicySchema, | ||
ExactlyOneOf: []string{"routing_policy.0.wrr", "routing_policy.0.geo", "routing_policy.0.primary_backup"}, | ||
}, | ||
"enable_geo_fencing": { |
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.
Some background on this field:
The API structure for the geo
policy is actually an object that contains a list called items
. Previously, items
was the only field, so it was flattened to a list called geo
for the Terraform config. However, enable_fencing
was added to the root of the original geo
object, and was meant to apply to all items. For example, the new API structure is:
"geo": {
"enable_fencing": true,
"items": [
{ "location": "us-central1", rrdatas: [ ... ] }
{ "location": "us-east1", rrdatas: [ ... ] }
]
}
If we were to refactor the Terraform config to match this structure, it would be a breaking change, so instead I added the new field one level up, under routing_policy
. The field should only apply to geo
configurations, so I included "geo" in the name of the field, and added the ConflictsWith
constraints you see below to recreate that.
Optional: true, | ||
Description: "Specifies whether to enable fencing for backup geo queries.", | ||
}, | ||
"trickle_ratio": { |
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.
Note that gcloud uses trickle_ratio
and enabled_geo_fencing
field names as well, even though they do not exactly match the API.
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.
Personally I have a slight preference for matching the API but matching gcloud for a handwritten resource seems reasonable.
}, | ||
"region": { | ||
Type: schema.TypeString, | ||
Optional: 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.
I was not able to come up with a test scenario with an internal load balancer that was global, so I did not confirm whether region
was required in this case. I've left it Optional
because I assume that it is not needed for global load balancers.
return fmt.Sprintf(` | ||
resource "google_compute_network" "default" { |
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.
These test scenarios are loosely based off of https://registry.terraform.io/providers/hashicorp/google/latest/docs/resources/compute_forwarding_rule and a set of internal tests that is linked in b/239887327.
load_balancer_type = "regionalL4ilb" | ||
ip_address = google_compute_forwarding_rule.default.ip_address | ||
port = "80" | ||
ip_protocol = "tcp" |
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 should really be google_compute_forwarding_rule.default.project.ip_protocol
, but because of the difference in casing, I think users will need to specify it explicitly like this :/
#### Primary-Backup | ||
|
||
```hcl | ||
resource "google_dns_record_set" "a" { |
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.
While the tests are probably ok, I think we should have someone from the service team check out that this example actually makes sense for real-world usage, and possibly provide others.
I left some comments here because I am going on leave after today, so hopefully they are enough to help whoever picks this up. |
452c135
to
1bec308
Compare
Hi! I'm the modular magician. Your PR generated some diffs in downstreams - here they are. Diff report:Terraform GA: Diff ( 3 files changed, 558 insertions(+), 117 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccDNSRecordSet_routingPolicy|TestAccDNSRecordSet_changeRouting |
Tests failed during RECORDING mode: 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 ( 3 files changed, 643 insertions(+), 117 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccComputeForwardingRule_update|TestAccDNSRecordSet_changeRouting|TestAccDNSRecordSet_routingPolicy |
Additional changes implemented:
|
d162bf1
to
f795d08
Compare
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 ( 3 files changed, 643 insertions(+), 117 deletions(-)) |
Tests analyticsTotal tests: Action takenTriggering VCR tests in RECORDING mode for the tests that failed during VCR. Click here to see the failed testsTestAccFirebaserulesRelease_BasicRelease |
Elem: &schema.Schema{ | ||
Type: schema.TypeString, | ||
}, | ||
}, | ||
"health_checked_targets": { |
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.
should this & rrdatas now be marked as either ConflictsWith
or ExactlyOneOf
?
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.
Based on the API doc, "healthCheckedTargets
can be specified along with rrdata
within the item". So I guess no ConflictsWith
or ExactlyOneOf
is needed here. I also tested it locally and it seems that both fields can be specified at the same time.
Optional: true, | ||
Description: "Specifies whether to enable fencing for backup geo queries.", | ||
}, | ||
"trickle_ratio": { |
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.
Personally I have a slight preference for matching the API but matching gcloud for a handwritten resource seems reasonable.
This resource used to support two types of routing policies: weighted round robin (wrr) and geo location (geo). Now we will be supporting a third option: primary-backup.
In addition, the existing routing types are being updated in two ways:
enabled_geo_fencing
is being added to support fencing behavior of allgeo
itemshealth_checked_targets
is being added to bothwrr
andgeo
, which provides another option for configuring the routing behaviorAll together, this work aims to support health checking capabilities for internal load balancers.
b/239887327
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)