resolver/google-c2p: introduce SetUniverseDomain API #7719
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7719 +/- ##
==========================================
+ Coverage 81.84% 82.09% +0.25%
==========================================
Files 361 362 +1
Lines 27827 28128 +301
==========================================
+ Hits 22775 23092 +317
+ Misses 3850 3841 -9
+ Partials 1202 1195 -7
|
| logger = internalgrpclog.NewPrefixLogger(grpclog.Component("directpath"), logPrefix) | ||
| universeDomainMu sync.Mutex | ||
| universeDomain = "" |
There was a problem hiding this comment.
Can you move the comment above to onGCE / randInt instead of the whole block, please? It no longer applies to everything.
xds/googledirectpath/googlec2p.go
Outdated
| // SetUniverseDomain informs the gRPC library of the TPC universe domain | ||
| // in which the process is running. It is the caller's responsibility to |
There was a problem hiding this comment.
Is there any documentation we can point to about this?
There was a problem hiding this comment.
Asking around, I'll get back to this comment after I hear back
There was a problem hiding this comment.
No documentation currently
xds/googledirectpath/googlec2p.go
Outdated
| // Arguments: | ||
| // | ||
| // d: The DNS domain for Google APIs in the current TPC universe | ||
| // (for example, "googleapis.com" or "apis-s3ns.fr"). |
There was a problem hiding this comment.
Is this a new Go style thing? I've not seen it before. If not, please follow the normal style of just mentioning it inline. Or I think it can just be removed and d renamed to domain.
There was a problem hiding this comment.
Sorry this is probably not golang style (this is something my editor suggested). Moved to a more inline comment.
xds/googledirectpath/googlec2p.go
Outdated
| return fmt.Errorf("universe domain cannot be empty") | ||
| } | ||
| if universeDomain == "" { | ||
| universeDomain = d |
There was a problem hiding this comment.
Do we need to do any paranoid validation in case SetUniverseDomain is called using unsanitized data?
I'm not sure that anything could actually go wrong if we don't (aside from it just not working), though.
There was a problem hiding this comment.
We perhaps could, but I'm not sure how aggressive we want to be in this check.
Because this is a DNS domain that we're configuring, standard DNS rules should apply:
-
The entire Traffic Director DNS name cannot exceed 255 characters (https://datatracker.ietf.org/doc/html/rfc1035#section-2.3.4). So we could perhaps do a simple check here that this domain does not exceed 255 characters. We could perhaps also do a more complicated check that each label does not exceed 63 characters.
-
Each character within a label should be a letter, number, or hyphen (https://datatracker.ietf.org/doc/html/rfc1034#section-3.5)
Asking around to see what other clients are doing here. I'll get back to this.
There was a problem hiding this comment.
Circling back on this.
In principle it seems very reasonable to add some input validation. And we can probably do it according to those standard DNS rules I mentioned.
However, client libraries are apparently not doing any such input validation here, and I've yet to hear of any actual specific validation that's been agreed upon.
So as for now, I think it's safest to not do any validation beyond the empty string check.
As you mentioned, things are expected to not work if the input is bad. Most likely DNS resolution of the Traffic Director endpoint will fail.
dfawley
changed the title
|
@dfawley PTAL |
purnesh42H
added
the
Area: Resolvers/Balancers
As title
@dfawley
RELEASE NOTES: none