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

chore(kuma-cp) generate static outbound routes #1098

Merged
merged 2 commits into from
Oct 22, 2020

Conversation

jakubdyszkiewicz
Copy link
Contributor

Summary

This PR changes that Routes for outbound listeners are send embedded in listeners instead of dynamically through RDS.
We already use embedded route in the inbound listener

Motivation

RDS really makes sense when you have many routes to one listener and you change them a lot.
Right now we have 1 route per listener and we don't change it after we create it.
Listener does not need to wait for routes anymore (listener is warming before it receives routes through RDS)
This saves us extra discovery request/response and potential bugs of warming like this one #1012

Fix #1012 because there is no warming because of RDS anymore

I also turned validateCluster from Route as recommended in docs
https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/route.proto
Users may wish to override the default behavior in certain cases (for example when using CDS with a static route table).
(also it did not make sense to have it to true on RDS, because there was no guarantee that Cluster was delivered before route)

I also needed to adjust Proxy Template Modification so modifications on VirtualHost that are embedded in the listener.

Documentation

  • Internal changes only

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz requested a review from a team as a code owner October 22, 2020 12:22
@nickolaev
Copy link
Contributor

nickolaev commented Oct 22, 2020

But docs say that validateCluster => defaults to true if the route table is statically defined? Is that what you had in mind?
Nevermind I misread it.

Copy link
Contributor

@nickolaev nickolaev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
@jakubdyszkiewicz jakubdyszkiewicz merged commit 3320211 into master Oct 22, 2020
@jakubdyszkiewicz jakubdyszkiewicz deleted the chore/static-routes branch October 22, 2020 14:03
jakubdyszkiewicz added a commit that referenced this pull request Oct 22, 2020
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
jakubdyszkiewicz added a commit that referenced this pull request Oct 22, 2020
Signed-off-by: Jakub Dyszkiewicz <jakub.dyszkiewicz@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handle listener warming
2 participants