-
Notifications
You must be signed in to change notification settings - Fork 327
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
kuma-cp: pick a single the most specific TrafficRoute for every outbound interface of a Dataplane #421
kuma-cp: pick a single the most specific TrafficRoute for every outbound interface of a Dataplane #421
Conversation
4497fa7
to
dfa44b6
Compare
…und interface of a Dataplane
dfa44b6
to
e22dd9a
Compare
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.
I like the tests, it helped me understand the code.
pkg/xds/topology/router.go
Outdated
if ok { | ||
for _, destination := range route.Spec.Conf { | ||
service, ok := destination.Destination[mesh_proto.ServiceTag] | ||
if ok { |
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.
What happens if there is no service tag here? I guess this should be caught be validation, but can we then log it if it's wrong or even fail with panic?
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.
I'm against a panic: a single invalid Dataplane
resource should not crash the entire Control Plane.
Logging creates too much noise. A metric might be a proper solution here.
Although, Dataplane
resource is invalid in the first place. So, users must have already been alarmed even without this implementation-specific metric.
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.
I'm adding a TODO here to consider exposing a metric
// skip dataplanes with invalid configuration | ||
continue | ||
} | ||
// TODO(yskopets): do we need to dedup? |
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.
I guess it would be safer to do it. Can you maybe add an issue so we don't forget about it?
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.
pkg/xds/topology/router.go
Outdated
}, | ||
Spec: mesh_proto.TrafficRoute{ | ||
Sources: []*mesh_proto.Selector{{ | ||
Match: mesh_proto.MatchService(mesh_proto.MatchAllTag), |
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.
maybe mesh_proto.MatchAll()
?
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.
added MatchAnyService()
bacbfd0
to
2d8c64b
Compare
2d8c64b
to
d48f59b
Compare
Summary
TrafficRoute
for every outbound interface of aDataplane
*
service: backend
wins overservice: *
N+1
tags is more specific than match byN
tagsservice: backend, version: v1
wins overservice: backend
service: backend, version: v1
wins overservice: backend, version: *
source
anddestination
selectors we sum up their numbers of exact and wildcard matching tagsTrafficRoute 1
:TrafficRoute 2
:TrafficRoute 1
is more specific since it has 3 tags but only 1 wildcard vs 3 tags and 2 wildcards in case ofTrafficRoute 2
Namespace
andName
of involvedTrafficRoutes
TrafficRoute
from the namespacedemo
wins over a rule from the namespacetest
TrafficRoute
namedAlice
wins overTrafficRoute
namedBob