xds/googlec2p: enable DirectPath over Interconnect support for on-prem clients#9133
xds/googlec2p: enable DirectPath over Interconnect support for on-prem clients#9133Pranjali-2501 wants to merge 1 commit into
Conversation
Pranjali-2501
added
the
Type: Behavior Change
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #9133 +/- ##
==========================================
+ Coverage 83.20% 83.23% +0.02%
==========================================
Files 414 414
Lines 33489 33500 +11
==========================================
+ Hits 27865 27884 +19
+ Misses 4214 4203 -11
- Partials 1410 1413 +3
🚀 New features to boost your workflow:
|
|
Unassigning myself for the first review from @mbissa to complete. |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for non-GCE environments in the google-c2p resolver via a force-xds query parameter and updates node configuration logic to handle off-GCP scenarios. The review feedback identifies several improvement opportunities: validating the boolean value of the force-xds parameter to prevent forcing xDS when set to 'false', using buffered channels for goroutines to avoid potential leaks, and ensuring the node ID implementation aligns with the PR description's requirement for UUIDs instead of random integers.
| isGCE := onGCE() | ||
| _, forceXds := t.URL.Query()["force-xds"] |
There was a problem hiding this comment.
The current implementation checks for the presence of the force-xds query parameter but does not validate its value. This means that a URI like google-c2p:///target?force-xds=false would still force the xDS path, which is counter-intuitive. Consider checking the value of the parameter to ensure it is not explicitly set to false.
isGCE := onGCE()
q := t.URL.Query()
forceXds := len(q["force-xds"]) > 0 && q.Get("force-xds") != "false"| // This should be fine in most of the cases. In certain error cases, this | ||
| // could block Dial() for up to 10 seconds (each blocking call has its own | ||
| // goroutine). | ||
| zoneCh, ipv6CapableCh := make(chan string), make(chan bool) |
There was a problem hiding this comment.
It is a best practice in Go to use buffered channels when a goroutine is expected to send a single value and then terminate. This prevents the goroutine from leaking if the receiver (the main thread in Build) were to stop waiting for any reason (e.g., a panic elsewhere).
| zoneCh, ipv6CapableCh := make(chan string), make(chan bool) | |
| zoneCh, ipv6CapableCh := make(chan string, 1), make(chan bool, 1) |
| node := map[string]any{ | ||
| "id": fmt.Sprintf("C2P-%d", randInt()), | ||
| "locality": map[string]any{"zone": zone}, | ||
| "id": fmt.Sprintf("%s-%d", prefix, randInt()), |
There was a problem hiding this comment.
The pull request description mentions that the xDS Client Node ID should use a UUID as a suffix (e.g., C2P-non-gcp-UUID), but the implementation continues to use a random integer from randInt(). If a UUID is required for uniqueness across clients in the service mesh, consider using a more robust identifier as stated in the PR description.
| desc: "query_param_value_false", | ||
| rawQuery: "force-xds=false", | ||
| }, |
There was a problem hiding this comment.
This test case expects force-xds=false to successfully trigger the xDS resolver path. If the implementation is updated to respect the boolean value of the query parameter, this test case should be moved to a separate test that verifies the fallback to DNS behavior.
3 participants
This PR add support for on-premises clients using Google Cloud Interconnect to connect to GCP services via DirectPath by enabling a forced xDS/C2P resolver path.
Changes:
google-c2presolver to parse call target URLs for theforce-xdsquery parameter.TRAFFICDIRECTOR_DIRECTPATH_C2P_IPV6_CAPABLEnode metadata flag totruesince Interconnect is planned exclusively for IPv6 clients."C2P-non-gcp-UUID".RELEASE NOTES: N/A