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 adding Route Discovery Service (RDS) API #298

Closed
mattklein123 opened this issue Dec 16, 2016 · 4 comments · Fixed by #464
Closed

Consider adding Route Discovery Service (RDS) API #298

mattklein123 opened this issue Dec 16, 2016 · 4 comments · Fixed by #464
Assignees
Labels
enhancement Feature requests. Not bugs or questions.
Milestone

Comments

@mattklein123
Copy link
Member

Now that we have SDS and soon CDS, there is no reason that we couldn't also add "RDS." This would allow the HTTP connection manager to dynamically reload the route table via an API call. The implementation is not very complicated. There hasn't been any explicit request for this feature yet but I can see that we are heading in this direction so I thought I would file the issue.

@rshriram
Copy link
Member

Actually, this would be very useful. CDS would allow us to add upstream blocks, but without RDS, CDS alone would not be that useful. Ultimately, the route table is the place where one points to the cluster for a given path/host. So, RDS and CDS go hand in hand.
The runtime configurations under route config, may be a little redundant in this case, but it would still be useful when people are not using RDS.

@mattklein123
Copy link
Member Author

@rshriram yes, that's my thinking. I think there are still many use cases for static route configs depending on deployment. (For example I don't really see Lyft moving away from that for a long time), but given that it's not that hard to implement, I think it could be useful for some use cases.

@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label Dec 17, 2016
@rshriram
Copy link
Member

@mattklein123 thinking about this a little more, we now have multiple components of the config files being obtained via 3 different REST api calls. (RDS, CDS, and SDS). My concern is that there are too many moving parts in this config, that could potentially be managed independently in an inconsistent manner.

To put things into perspective, there are typically three mutations to the config file that happen during the lifetime of an Envoy process:

  1. Addition/deletion of upstream blocks (aka clusters) when new services or new versions of a service are created
  2. Addition/deletion of IPs to upstream blocks (aka clusters)
  3. Addition/deletion of route blocks that decide which of the upstream blocks/clusters to pick for a given request based on path prefix, header match, etc.
    (a). Updates to routing rules such as adjustments to weights, rate limits, etc.

(1) is accomplished by the CDS proposal.
(2) is accomplished by SDS (if one were to use SDS inside the CDS clusters. If I use static IPs, then (1) will take care of (2) as well).
(3) should be taken care of by RDS.
(3a) by Envoy Runtime.

However, any routing rule needs to refer to a cluster in order to be valid. Given this dependency, why not merge (1) and (3) (i.e., CDS and RDS) into one single service ? The APIs can be the same as the CDS apis that you recently proposed.

Periodically, Envoy obtains the new routes and the clusters that these routes refer to (they can contain static IPs or SDS entries).

Am I missing something here?

@mattklein123
Copy link
Member Author

@rshriram a few things:

  1. All three APIs can be implemented by the same remote server. Envoy doesn't care. So if it's helpful from a consistency perspective they can all go to the same place.

  2. I don't think it's strictly required moving forward that a route always point to a currently known cluster. For example, there has already been a feature request to have the cluster specified directly in a header (for example the :authority header). Because of this, the router already has to be changed to support returning a 404 if the cluster does not exist.

In my mind, I think it's fine to have different API calls for the different use cases and decouple them and in the future allow cluster to refer to an unknown cluster.

From an operator perspective, if the same backend server implements CDS/RDS, if the implementation wishes it can make sure that there are atomic updates.

From the Envoy perspective, the cluster manager and the http router are totally different systems and I think having a single API provide both goes against the separation of concerns that are already in place.

@mattklein123 mattklein123 self-assigned this Dec 31, 2016
@mattklein123 mattklein123 added this to the 1.2.0 milestone Dec 31, 2016
mattklein123 added a commit that referenced this issue Feb 10, 2017
rshriram pushed a commit to rshriram/envoy that referenced this issue Oct 30, 2018
PiotrSikora pushed a commit to PiotrSikora/envoy that referenced this issue Feb 19, 2021
alyssawilk added a commit that referenced this issue Oct 9, 2023
#30044)

UNCLEANLY Revert " notifier: adding basic oncall notifications (#29820)"

This reverts commit c495291.

Signed-off-by: Alyssa Wilk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants