Add support for source-based routing #225
Conversation
| func (i *IstioRegistry) RouteRulesBySource(namespace string, instances []*ServiceInstance) []*proxyconfig.RouteRule { | ||
| out := make([]*proxyconfig.RouteRule, 0) | ||
| for _, rule := range i.RouteRules(namespace) { | ||
| if rule.Match != nil { |
There was a problem hiding this comment.
(optional) you could invert the rule.match != nil check here and get rid of the extra found variable, e.g.
if rule.Match == nil {
continue
}
for _, instance := range instances {
if rule.Match.Source != "" && rule.Match.Source != instance.Service.Hostname {
continue
}
var tags Tags = rule.Match.SourceTags
if !tags.SubsetOf(instance.Tags) {
continue
}
out = append(out, rule)
}There was a problem hiding this comment.
yeah, thanks. also need a break statement after append.
There was a problem hiding this comment.
actually, we'd have to duplicate code for rule.Match == nil. maybe leave it as is?
There was a problem hiding this comment.
Whoops, yeah. Leave it as is and ignore my comment.
|
Jenkins job manager/presubmit passed |
Codecov Report
@@ Coverage Diff @@
## master #225 +/- ##
=========================================
- Coverage 31.05% 30.9% -0.16%
=========================================
Files 19 19
Lines 1803 1812 +9
=========================================
Hits 560 560
- Misses 1187 1196 +9
Partials 56 56
Continue to review full report at Codecov.
|
|
Let's merge this, and send coverage tests in another PR. |
| @@ -146,7 +146,7 @@ type IstioRegistry struct { | |||
| ConfigRegistry | |||
| } | |||
|
|
|||
| // RouteRules lists all routing rules in a namespace (or all rules if namespace is "") | |||
| // RouteRules lists all unsorted routing rules in a namespace (or all rules if namespace is "") | |||
| func (i *IstioRegistry) RouteRules(namespace string) []*proxyconfig.RouteRule { | |||
There was a problem hiding this comment.
Why can't this be sorted by precedence and destination?
There was a problem hiding this comment.
We certainly need to sort route rules by all its fields. I documented precendece since that's how it works now.
| @@ -184,13 +184,28 @@ func buildOutboundFilters(instances []*model.ServiceInstance, services []*model. | |||
| suffix := sharedInstanceHost(instances) | |||
| httpConfigs := make(HTTPRouteConfigs) | |||
|
|
|||
| // get all the route rules applicable to the instances | |||
| rules := config.RouteRulesBySource("", instances) | |||
|
|
|||
There was a problem hiding this comment.
If instances is the set of all destination services in k8s, then this function basically does that rulesByDestination did.
There was a problem hiding this comment.
No, instances is co-located instances, not external destinations.
| precedence: 2 | ||
| match: | ||
| source: {{.source}}.{{.namespace}}.svc.cluster.local | ||
| source_tags: | ||
| version: v1 | ||
| http: |
There was a problem hiding this comment.
This needs to go as a separate test to ensure that other services are unable to call service "worldl
There was a problem hiding this comment.
We need two versions of world?
There was a problem hiding this comment.
No. Two versions of hello, hello v1 can only call world v1, so on. Otherwise this test as is, is not checking whether service hello-v2 can access service world-v1 or not.
There was a problem hiding this comment.
Yeah, we can do a follow-up test PR. I will get mocks set-up as well.
| @@ -184,13 +184,28 @@ func buildOutboundFilters(instances []*model.ServiceInstance, services []*model. | |||
| suffix := sharedInstanceHost(instances) | |||
| httpConfigs := make(HTTPRouteConfigs) | |||
|
|
|||
| // get all the route rules applicable to the instances | |||
| rules := config.RouteRulesBySource("", instances) | |||
There was a problem hiding this comment.
Where is this code expected to run? When running in a pod, this will be a single instance and not an array. This cannot run on the host as a daemon set. And even if it does, same concept applies.
There was a problem hiding this comment.
It's in a pod. If there are two services pointing to the pod, or two services with distinct ports, we'd get two elements in the array, wouldn't we?
There was a problem hiding this comment.
That's true. We really need to rename these variables in future for better readability. May be call these as localSvcInstances ?
There was a problem hiding this comment.
The top-level call takes instances and services, so I was just following that naming pattern. Pretend instance is always local instances throughout.
| found := false | ||
| for _, instance := range instances { | ||
| if rule.Match.Source != "" && rule.Match.Source != instance.Service.Hostname { | ||
| continue |
There was a problem hiding this comment.
I don't think you need arrays here. It's just the pod filtering out rules before generating the config.
There was a problem hiding this comment.
It's for per-node case, but I don't think it makes any difference how many instances are behind the proxy.
|
Please wait |
No description provided.