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

Consider removal of custom DNS resolution #2635

Closed
lukesteensen opened this issue May 18, 2020 · 10 comments · Fixed by #2812
Closed

Consider removal of custom DNS resolution #2635

lukesteensen opened this issue May 18, 2020 · 10 comments · Fixed by #2812
Assignees
Labels
domain: networking Anything related to Vector's networking meta: idea Anything in the idea phase. Needs further discussion and consensus before work can begin. needs: approval Needs review & approval before work can begin.

Comments

@lukesteensen
Copy link
Member

Opening this issue to solicit feedback. To be clear, no decision has yet been made here. If you feel strongly one way or the other (or know someone who might) please let us know!

Background

Custom DNS resolution was suggested in #827 and implemented in #1118. Notably, these were both in anticipation of user demand, not in response to any actual demand (as far as I know).

Since then there have been numerous issues opened to "finish" implementing this everywhere (e.g. #2359, #1954). Support is still technically incomplete.

The Problem

The immediate problem that sparked this discussion was #2511, which has led @LucioFranco down a rabbit hole of very strange failures that shouldn't have anything to do with DNS. It has been a huge expenditure of time and there is still no solution in sight.

The larger problem that has been on my mind for a while is how invasive this feature is. As shown by #2359, it adds an extra wrinkle to anything that could make a network request, forcing us to customize HTTP clients that are built into many higher-level libraries. This is a significant barrier to contributors, often for virtually no benefit. And once it's done, all the extra wiring has a meaningful ongoing maintenance cost.

Proposal

Unless it turns out that a meaningful number of users are relying on this feature, we should rip out the whole thing and add documentation on how to achieve the same result with a system-level resolver.

As far as I can tell, there is no meaningful use of this feature and it has proven to be a significant burden to maintain. Combined with the fact that there are very reasonable alternatives that do not involve Vector at all, this seems like a clear case for cutting the feature.

I propose that we circulate this issue to give users a chance to weigh in, and then make a decision based on that feedback in two weeks time.

@lukesteensen lukesteensen added meta: idea Anything in the idea phase. Needs further discussion and consensus before work can begin. needs: approval Needs review & approval before work can begin. domain: networking Anything related to Vector's networking labels May 18, 2020
@binarylogic
Copy link
Contributor

binarylogic commented May 18, 2020

👍 this makes sense to me. The only piece of context I have is Kubernetes. We had a user request custom DNS support in our chat as a means of service discovery. I assume this can be configured and handled at the system level there too. cc @MOZGIII for confirmation.

@MOZGIII
Copy link
Contributor

MOZGIII commented May 18, 2020

Vector should be using in-cluster DNS service in k8s, and a system resolver in a pod is automatically configured for that. It also works with our current setup (with custom DNS resolvers) out of the box. There are cases where custom DNS resolvers are beneficial (mainly - to configure DNS resolution per-topology-unit), but the standard k8s setup is not one of those cases.

In general, configurable DNS resolvers is a very cool feature, and it may be very helpful in dare situations. On the other hand, it indeed has a maintenance cost, and complicates contributions. However, even if we're not going to use custom resolvers, we'd still want to configure HTTP clients - so I'm not sure if we'll be saving on complexity there. To sum up, I don't have a strong opinion, but if I had to pick I'd rather vote for keeping the flexibility around the DNS resolvers, than for removing it.

@MOZGIII
Copy link
Contributor

MOZGIII commented May 18, 2020

The goauth crate (it's the problem as the #2359) is problematic on it's own, since the interface it exposes is inflexible. IMO, this is just a bad design.
Regarding #1954 - we'd still need the source context for executors. A lot of sources just do tokio01::spawn, but it looks like they really all of them should be using executors too. I just want to say that we need contexts not just for the resolvers.

@LucioFranco
Copy link
Contributor

By removing trust-dns we can also close this #2464 and #2455 and ensure the advisory list has been updated accordingly.

@lukesteensen
Copy link
Member Author

However, even if we're not going to use custom resolvers, we'd still want to configure HTTP clients - so I'm not sure if we'll be saving on complexity there.

This may or may not be true, but at the very least we can do it only in the cases where we have a specific reason to do so. Right now there is an expectation that every network call will use the configured custom resolvers, even in cases where that's not very useful.

I just want to say that we need contexts not just for the resolvers.

Agreed, this is not about removing contexts. Those are still useful for other reasons, executors being one of them.

@MOZGIII
Copy link
Contributor

MOZGIII commented May 19, 2020

This may or may not be true, but at the very least we can do it only in the cases where we have a specific reason to do so. Right now there is an expectation that every network call will use the configured custom resolvers, even in cases where that's not very useful.

We still have to carefully pick the HTTP clients, so that they don't do things they aren't supposed to - like create their own runtimes (i.e. reactors). What's your opinion on that? I assume we want to keep all the inner-workings under control. Making sure we pass our own clients everywhere is just an easy way to achieve that. But I guess it's a bigger problem. Either way, I'm really interested in what is our policy here, cause it affects what crates are effectively available for use in the project.

@lukesteensen
Copy link
Member Author

We still have to carefully pick the HTTP clients, so that they don't do things they aren't supposed to - like create their own runtimes (i.e. reactors). What's your opinion on that? I assume we want to keep all the inner-workings under control.

Our policy is to be pragmatic. We want to make sure clients are well-behaved, but we don't want to put an undue burden on ourselves by reimplementing everything from scratch.

Removing custom DNS resolution simplifies our definition of "well-behaved" and gives us one less thing to worry about. That doesn't mean we're completely relieved of worrying about HTTP clients.

@Hoverbear
Copy link
Contributor

Ref seanmonstar/reqwest#437

@binarylogic binarylogic assigned ktff and unassigned lukesteensen Jun 4, 2020
@binarylogic
Copy link
Contributor

@ktff we've decided to move forward with this. Let me know if you'd like to discuss how this will work, but it should hopefully be a straight-forward removal.

@lukesteensen
Copy link
Member Author

To be a little more specific, this involves removing the dns_servers global option and the trust-dns library in favor of a simpler implementation that relies on the system resolver. Tokio already includes a simple implementation that is likely what we should use.

We'll still do DNS resolution, it just won't use custom DNS servers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain: networking Anything related to Vector's networking meta: idea Anything in the idea phase. Needs further discussion and consensus before work can begin. needs: approval Needs review & approval before work can begin.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants