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
[EventEngine] Change TXT lookup result type to std::vector<std::string>
#33030
Conversation
This change will need be cherrypicked. |
return absl::StartsWith( | ||
s, kServiceConfigAttributePrefix); | ||
}); | ||
if (result != service_config->end()) { |
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 appears to differ from previous logic where we simply concatenate the grpc_config prefix to all returned values. Instead now it searches for a result with the given prefix. Is there a specific reason ?
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.
yeah, I think the previous logic is not correct (this code is still under development and not used until EventEngine::DNSResolver is ready) since gRPC's service config data starts with grpc_config=
followed by the JSON-formatted data. See: https://github.com/grpc/proposal/blob/master/A2-service-configs-in-dns.md#encoding-in-dns-txt-records.
Thanks for finding & fixing this API problem, Yijie! Please make sure it gets added to the release notes, since it's a public API. |
…g> (grpc#33030) One TXT lookup query can return multiple TXT records (see the following example). `EventEngine::DNSResolver` should return all of them to let the caller (e.g. `event_engine_client_channel_resolver`) decide which one they would use. ``` $ dig TXT wikipedia.org ; <<>> DiG 9.18.12-1+build1-Debian <<>> TXT wikipedia.org ;; global options: +cmd ;; Got answer: ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 49626 ;; flags: qr rd ra; QUERY: 1, ANSWER: 3, AUTHORITY: 0, ADDITIONAL: 1 ;; OPT PSEUDOSECTION: ; EDNS: version: 0, flags:; udp: 512 ;; QUESTION SECTION: ;wikipedia.org. IN TXT ;; ANSWER SECTION: wikipedia.org. 600 IN TXT "google-site-verification=AMHkgs-4ViEvIJf5znZle-BSE2EPNFqM1nDJGRyn2qk" wikipedia.org. 600 IN TXT "yandex-verification: 35c08d23099dc863" wikipedia.org. 600 IN TXT "v=spf1 include:wikimedia.org ~all" ``` Note that this change also deviates us from the iomgr's DNSResolver API which uses std::string as the result type. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. -->
… record parsing (#33131) This is a mistake made in #33030. `sizeof()` would count the null byte terminated the C string and would cause us to skip a byte if it is used as the index to `result->substr()`. This would also crash if `result` only contains `grpc_config=` as @drfloob pointed out. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. -->
… record parsing (grpc#33131) This is a mistake made in grpc#33030. `sizeof()` would count the null byte terminated the C string and would cause us to skip a byte if it is used as the index to `result->substr()`. This would also crash if `result` only contains `grpc_config=` as @drfloob pointed out. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. -->
…g> (#33030) One TXT lookup query can return multiple TXT records (see the following example). `EventEngine::DNSResolver` should return all of them to let the caller (e.g. `event_engine_client_channel_resolver`) decide which one they would use. ``` $ dig TXT wikipedia.org ; <<>> DiG 9.18.12-1+build1-Debian <<>> TXT wikipedia.org ;; global options: +cmd ;; Got answer: ;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 49626 ;; flags: qr rd ra; QUERY: 1, ANSWER: 3, AUTHORITY: 0, ADDITIONAL: 1 ;; OPT PSEUDOSECTION: ; EDNS: version: 0, flags:; udp: 512 ;; QUESTION SECTION: ;wikipedia.org. IN TXT ;; ANSWER SECTION: wikipedia.org. 600 IN TXT "google-site-verification=AMHkgs-4ViEvIJf5znZle-BSE2EPNFqM1nDJGRyn2qk" wikipedia.org. 600 IN TXT "yandex-verification: 35c08d23099dc863" wikipedia.org. 600 IN TXT "v=spf1 include:wikimedia.org ~all" ``` Note that this change also deviates us from the iomgr's DNSResolver API which uses std::string as the result type. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. -->
… record parsing (#33131) This is a mistake made in #33030. `sizeof()` would count the null byte terminated the C string and would cause us to skip a byte if it is used as the index to `result->substr()`. This would also crash if `result` only contains `grpc_config=` as @drfloob pointed out. <!-- If you know who should review your pull request, please assign it to that person, otherwise the pull request would get assigned randomly. If your pull request is for a specific language, please add the appropriate lang label. -->
std::vector<std::string>
One TXT lookup query can return multiple TXT records (see the following example).
EventEngine::DNSResolver
should return all of them to let the caller (e.g.event_engine_client_channel_resolver
) decide which one they would use.Note that this change also deviates us from the iomgr's DNSResolver API which uses std::string as the result type.