chore(*) TrafficRoute add Split #1149
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nickolaev
force-pushed
the
feat/route_support_destination_wildcard
branch
from
974379d
to
09b3ac6
Compare
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
nickolaev
force-pushed
the
feat/route_support_destination_wildcard
branch
from
09b3ac6
to
e7ef54d
Compare
added 6 commits
November 12, 2020 23:22
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…destination_wildcard
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…destination_wildcard # Conflicts: # app/kumactl/pkg/install/k8s/control-plane/helmtemplates_vfsdata.go Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…destination_wildcard
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
nickolaev
force-pushed
the
feat/route_support_destination_wildcard
branch
from
328fed6
to
8857318
Compare
added 2 commits
November 16, 2020 15:21
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…destination_wildcard
…destination_wildcard
jakubdyszkiewicz
requested changes
Nov 16, 2020
pkg/defaults/mesh/traffic_route.go
Outdated
| // This will be dropped when TrafficRoute will be converted to Global Scope on K8S instead of Namespace Scope | ||
| // TrafficRoute needs to contain mesh name inside it. Otherwise if the name is the same (ex. "allow-all") creating new mesh would fail because there is already resource of name "allow-all" which is unique key on K8S | ||
| func defaultTrafficRouteName(meshName string) string { | ||
| return fmt.Sprintf("allow-all-%s.default", meshName) |
There was a problem hiding this comment.
the .default is not required anymore
pkg/xds/topology/router.go
Outdated
| service, ok := destination.Destination[mesh_proto.ServiceTag] | ||
| if !ok { | ||
| // ignore destinations without a `service` tag | ||
| // TODO(yskopets): consider adding a metric for this | ||
| continue | ||
| } | ||
| destinations[service] = destinations[service].Add(mesh_proto.MatchTags(destination.Destination)) | ||
| if service == mesh_proto.MatchAllTag { |
There was a problem hiding this comment.
This should be handled already. routes core_xds.RouteMap should not have * matches
pkg/xds/topology/router.go
Outdated
| Split: split, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
I don't think that's the algorithm that we agreed on the call. The wildcard can be placed on many tags.
The idea was
- Take traffic route
- Remove all tag pairs with wildcard
- Add all tags from outbound to traffic route if they do not already exist in the list
Examples
1)
Traffic Route
conf:
split:
- weight: 100
tags:
kuma.io/service: '*'
- Outbound
outbound:
- port: 1234
tags:
kuma.io/service: backend
=
conf:
split:
- weight: 100
tags:
kuma.io/service: 'backend'
Traffic Route
conf:
split:
- weight: 100
tags:
kuma.io/service: '*'
kuma.io/zone: aws
- Outbound
outbound:
- port: 1234
tags:
kuma.io/service: backend
=
conf:
split:
- weight: 100
tags:
kuma.io/service: 'backend'
kuma.io/zone: aws
Traffic Route
conf:
split:
- weight: 100
tags:
kuma.io/service: '*'
kuma.io/zone: '*'
- Outbound
outbound:
- port: 1234
tags:
kuma.io/service: backend
=
conf:
split:
- weight: 100
tags:
kuma.io/service: 'backend'
Traffic Route
conf:
split:
- weight: 100
tags:
kuma.io/service: '*'
kuma.io/zone: '*'
- Outbound
outbound:
- port: 1234
tags:
kuma.io/service: backend
kuma.io/zone: aws
=
conf:
split:
- weight: 100
tags:
kuma.io/service: 'backend'
kuma.io/zone: aws
Traffic Route
conf:
split:
- weight: 100
tags:
kuma.io/service: '*'
- Outbound
outbound:
- port: 1234
tags:
kuma.io/service: backend
version: 1
=
conf:
split:
- weight: 100
tags:
kuma.io/service: 'backend'
version: 1
Traffic Route
conf:
split:
- weight: 100
tags:
kuma.io/service: '*'
version: 1
- Outbound
outbound:
- port: 1234
tags:
kuma.io/service: backend
version: 2
=
conf:
split:
- weight: 100
tags:
kuma.io/service: 'backend'
version: 1
(Traffic Route should take precedence)
added 5 commits
November 16, 2020 16:26
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…destination_wildcard # Conflicts: # app/kumactl/pkg/install/k8s/control-plane/helmtemplates_vfsdata.go Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
pkg/defaults/mesh/traffic_route.go
Outdated
| ) | ||
|
|
||
| // Default traffic route needs to be stored with default suffix so on K8S it will be stored in the default namespace | ||
| // This will be dropped when TrafficRoute will be converted to Global Scope on K8S instead of Namespace Scope |
There was a problem hiding this comment.
Delete those two lines of comments, we don't have a suffix anymore
| @@ -224,6 +226,10 @@ func (_ OutboundProxyGenerator) determineSubsets(proxy *model.Proxy, outbound *k | |||
| // 0 assumes no traffic is passed there. Envoy doesn't support 0 weight, so instead of passing it to Envoy we just skip such cluster. | |||
| continue | |||
| } | |||
| if service == kuma_mesh.MatchAllTag { | |||
There was a problem hiding this comment.
this should not happen, because we replace this on the upper level, right?
pkg/defaults/mesh/traffic_route.go
Outdated
| // This will be dropped when TrafficRoute will be converted to Global Scope on K8S instead of Namespace Scope | ||
| // TrafficRoute needs to contain mesh name inside it. Otherwise if the name is the same (ex. "allow-all") creating new mesh would fail because there is already resource of name "allow-all" which is unique key on K8S | ||
| func defaultTrafficRouteName(meshName string) string { | ||
| return fmt.Sprintf("allow-all-%s", meshName) |
There was a problem hiding this comment.
nit: I think the name could be better. allow-all made sense in the context of traffic permissions. Here should be something like route-all-%? default-router-%s, router-%s?
pkg/xds/topology/router.go
Outdated
| }}, | ||
| }, | ||
| route := policy.(*mesh_core.TrafficRouteResource) | ||
| if route.Spec.GetConf().HasWildcard() { |
There was a problem hiding this comment.
I don't think this should rely on wildcard here. How about this case
Traffic Route
conf:
split:
- weight: 100
tags:
kuma.io/service: 'backend'
- Outbound
outbound:
- port: 1234
tags:
kuma.io/service: backend
version: 2
=
conf:
split:
- weight: 100
tags:
kuma.io/service: 'backend'
version: 2
added 4 commits
November 16, 2020 18:11
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
…destination_wildcard # Conflicts: # pkg/core/managers/apis/mesh/mesh_manager_test.go Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
jakubdyszkiewicz
approved these changes
Nov 16, 2020
| @@ -0,0 +1,10 @@ | |||
| package v1alpha1 | |||
|
|
|||
| func (c *TrafficRoute_Conf) HasWildcard() bool { | |||
There was a problem hiding this comment.
this is not needed anymore
Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Adding default TrafficRoute, Move the Configuration destination list to a Split field and make it support wildcard services
Documentation