Skip to content
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 custom resolver support for v2 Address messages #1477

Closed
htuch opened this issue Aug 16, 2017 · 7 comments
Closed

Add custom resolver support for v2 Address messages #1477

htuch opened this issue Aug 16, 2017 · 7 comments
Assignees
Labels

Comments

@htuch
Copy link
Member

htuch commented Aug 16, 2017

In the v2 API, we allow Address messages to specify a resolver plugin. This will transform logical addresses to concrete addresses. This supports address virtualization and address namespaces that are non-standard. This issue will track the implementation of the registration facility, invocation of the resolver at appropriate code sites and an example resolver for testing.

The resolver should be capable of returning an arbitrary Address::Instance subclass, which will be able to support a notion of a string representation for both the logical and physical address. These can then be plumbed into logs, stats and generated headers as distinct concepts.

@akonradi
Copy link
Contributor

akonradi commented Oct 5, 2017

This seemed straightforward until I started looking at the already-existing-but-unrelated DNS resolver, which resolves names asynchronously. It seems like that's the right way to resolve names in general, but the code paths that would use this expect config parsing to be a synchronous affair.

@htuch
Copy link
Member Author

htuch commented Oct 5, 2017

I guess the question is how dynamic do we need to be? We could do resolution at config ingestion time, which would probably be fine for Google, but the general resolver scheme would probably also support this happening at bind/connect time. I'd be fine with a synchronous config time load resolution as a first step, because that's all that Google needs.

The use cases for the resolver so far have been IP NAT and port name mapping (from a named port to a specific port). I think there was also the idea we would use SRV in the Address for existing DNS resolution, but that can follow the existing path.

@htuch
Copy link
Member Author

htuch commented Oct 5, 2017

Thinking about how onConfigUpdate() works today, the interface is very synchronous. Would it be simpler to do the resolution at bind time (caching results for performance)?

@akonradi
Copy link
Contributor

akonradi commented Oct 5, 2017

We could defer resolution until bind(), connect(), or ip() are called if they're already treated as blocking methods. A fancier implementation could even start an asynchronous request when the Instance is first created to reduce the amount of time spent blocking, but that might be overkill.

@akonradi
Copy link
Contributor

akonradi commented Oct 5, 2017

After talking with @htuch in person, this is going to stay completely synchronous for now - no remote lookups, no network access. The calling code could be modified to support asynchronous lookups, but it would require significant changes and a lot more callbacks sprinkled around.

@mattklein123
Copy link
Member

@htuch @akonradi +1 let's keep it simple/synchronous unless we have a clear need for something else.

htuch pushed a commit that referenced this issue Oct 13, 2017
Add Address::ResolverFactory and Address::Resolver interfaces for instantiating Address::Instances from strings in protos. The interface is completely synchronous, so all translation must be done inline without any network access, meaning this can't be used to resolve hostnames via DNS. Doing so would require making more things asynchronous; this can be done, but let's not do it until it's necessary.

This is progress towards #1477

Signed-off-by: Alex Konradi <akonradi@google.com>
@mattklein123
Copy link
Member

I think this is done. Closing.

rshriram pushed a commit to rshriram/envoy that referenced this issue Oct 30, 2018
Automatic merge from submit-queue.

Remove using EndUserAuthenticationPolicySpec

**What this PR does / why we need it**:

Remove code of using EndUserAuthenticationPolicySpec

Issue: istio/istio#4744
**Release note**:

```release-note
None
```
jpsim pushed a commit that referenced this issue Nov 28, 2022
Description: make TagsBuilder initializer public
Risk Level: low
Testing: unit/ci
Signed-off-by: Jingwei Hao <jingwei.hao@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
jpsim pushed a commit that referenced this issue Nov 29, 2022
Description: make TagsBuilder initializer public
Risk Level: low
Testing: unit/ci
Signed-off-by: Jingwei Hao <jingwei.hao@gmail.com>
Signed-off-by: JP Simard <jp@jpsim.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants