Skip to content

Commit

Permalink
Stop accepting multiple TrafficSplits or both TrafficSplit and Traffi…
Browse files Browse the repository at this point in the history 
…cTarget on the same Service
  • Loading branch information
@jspdown
jspdown committed Jun 19, 2020
1 parent e59b861 commit c17c499
Show file tree
Hide file tree
Showing 5 changed files with 236 additions and 130 deletions.
91 changes: 58 additions & 33 deletions pkg/topology/builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,22 +44,22 @@ func (b *Builder) Build(resourceFilter *mk8s.ResourceFilter) (*Topology, error)
b.evaluateService(res, topology, svc)
}

// Populate services with traffic-target definitions.
for _, tt := range res.TrafficTargets {
b.evaluateTrafficTarget(res, topology, tt)
}

// Populate services with traffic-split definitions.
for _, ts := range res.TrafficSplits {
b.evaluateTrafficSplit(topology, ts)
}

// Populate services with traffic-target definitions.
for _, tt := range res.TrafficTargets {
b.evaluateTrafficTarget(res, topology, tt)
}

b.populateTrafficSplitsAuthorizedIncomingTraffic(topology)

return topology, nil
}

// evaluateService evaluates the given service. It adds the Service to the topology and it's selected Pods.
// evaluateService evaluates the given service. It adds the Service to the topology and its selected Pods.
func (b *Builder) evaluateService(res *resources, topology *Topology, svc *corev1.Service) {
svcKey := Key{svc.Name, svc.Namespace}

Expand Down Expand Up @@ -96,29 +96,35 @@ func (b *Builder) evaluateTrafficTarget(res *resources, topology *Topology, tt *
Namespace: tt.Namespace,
Service: svcKey,
Sources: sources,
Destination: ServiceTrafficTargetDestination{
ServiceAccount: tt.Destination.Name,
Namespace: tt.Destination.Namespace,
},
}

svcTTKey := ServiceTrafficTargetKey{
Service: svcKey,
TrafficTarget: Key{tt.Name, tt.Namespace},
}
topology.ServiceTrafficTargets[svcTTKey] = stt

svc, ok := topology.Services[svcKey]
// Since in the current version of the spec, the traffic will always go to a TrafficSplit, we don't need to evaluate
// Pods if there is already a TrafficSplit on the Service.
if ok && len(svc.TrafficSplits) > 0 {
b.Logger.Warnf("Service %q already has a TrafficSplit attached, TrafficTarget %q will not be evaluated on this service", svcKey, svcTTKey.TrafficTarget)

return
}

topology.ServiceTrafficTargets[svcTTKey] = stt

if !ok {
err := fmt.Errorf("unable to find Service %q", svcKey)
stt.AddError(err)
b.Logger.Errorf("Error building topology for TrafficTarget %q: %v", Key{tt.Name, tt.Namespace}, err)
b.Logger.Errorf("Error building topology for TrafficTarget %q: %v", svcTTKey.TrafficTarget, err)

continue
}

// Build the TrafficTarget Specs.
specs, err := b.buildTrafficTargetSpecs(res, tt)
var err error

stt.Specs, err = b.buildTrafficTargetSpecs(res, tt)
if err != nil {
err = fmt.Errorf("unable to build spec: %v", err)
stt.AddError(err)
Expand All @@ -127,40 +133,51 @@ func (b *Builder) evaluateTrafficTarget(res *resources, topology *Topology, tt *
continue
}

stt.Specs = specs

// Find out which are the destination pods.
for _, pod := range pods {
if pod.Status.PodIP == "" {
continue
}

stt.Destination.Pods = append(stt.Destination.Pods, getOrCreatePod(topology, pod))
}

// Find out which ports can be used on the destination service.
destPorts, err := b.getTrafficTargetDestinationPorts(svc, tt)
stt.Destination, err = b.buildTrafficTargetDestination(topology, tt, pods, svc)
if err != nil {
err = fmt.Errorf("unable to find destination ports on Service %q: %w", svcKey, err)
stt.AddError(err)
b.Logger.Errorf("Error building topology for TrafficTarget %q: %v", Key{tt.Name, tt.Namespace}, err)

continue
}

stt.Destination.Ports = destPorts

svc.TrafficTargets = append(svc.TrafficTargets, svcTTKey)

// Add the ServiceTrafficTarget to the source and destination pods.
addSourceAndDestinationToPods(topology, sources, svcTTKey)
}
}

func (b *Builder) buildTrafficTargetDestination(topology *Topology, tt *access.TrafficTarget, pods []*corev1.Pod, svc *Service) (ServiceTrafficTargetDestination, error) {
dest := ServiceTrafficTargetDestination{
ServiceAccount: tt.Destination.Name,
Namespace: tt.Destination.Namespace,
}

// Find out which are the destination pods.
for _, pod := range pods {
if pod.Status.PodIP == "" {
continue
}

dest.Pods = append(dest.Pods, getOrCreatePod(topology, pod))
}

var err error

// Find out which ports can be used on the destination service.
dest.Ports, err = b.getTrafficTargetDestinationPorts(svc, tt)
if err != nil {
return dest, fmt.Errorf("unable to find destination ports on Service %q: %w", Key{Namespace: svc.Namespace, Name: svc.Name}, err)
}

return dest, nil
}

func addSourceAndDestinationToPods(topology *Topology, sources []ServiceTrafficTargetSource, svcTTKey ServiceTrafficTargetKey) {
for _, source := range sources {
for _, podKey := range source.Pods {
// Skip pods which haven't been added to the topology.
// Skip pods which have not been added to the topology.
if _, ok := topology.Pods[podKey]; !ok {
continue
}
Expand All @@ -170,7 +187,7 @@ func addSourceAndDestinationToPods(topology *Topology, sources []ServiceTrafficT
}

for _, podKey := range topology.ServiceTrafficTargets[svcTTKey].Destination.Pods {
// Skip pods which haven't been added to the topology.
// Skip pods which have not been added to the topology.
if _, ok := topology.Pods[podKey]; !ok {
continue
}
Expand All @@ -190,9 +207,17 @@ func (b *Builder) evaluateTrafficSplit(topology *Topology, trafficSplit *split.T
}

tsKey := Key{trafficSplit.Name, trafficSplit.Namespace}
topology.TrafficSplits[tsKey] = ts

svc, ok := topology.Services[svcKey]
// The current version of the spec does not support having more than one TrafficSplit attached to the same service.
if ok && len(svc.TrafficSplits) > 0 {
b.Logger.Warnf("Service %q already has a TrafficSplit attached, TrafficSplit %q will not be evaluated", svcKey, tsKey)

return
}

topology.TrafficSplits[tsKey] = ts

if !ok {
err := fmt.Errorf("unable to find root Service %q", svcKey)
ts.AddError(err)
Expand Down
68 changes: 54 additions & 14 deletions pkg/topology/builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,8 @@ func TestTopologyBuilder_TrafficTargetSourcesForbiddenTrafficSplit(t *testing.T)
assert.Equal(t, 0, len(got.TrafficSplits[tsKey].Incoming))
}

// TestTopologyBuilder_EvaluatesIncomingTrafficSplit makes sure a topology can be built with TrafficSplits. It also
// checks that if multiple TrafficSplits are applied to the same Service, only one will be used.
func TestTopologyBuilder_EvaluatesIncomingTrafficSplit(t *testing.T) {
selectorAppA := map[string]string{"app": "app-a"}
selectorAppB := map[string]string{"app": "app-b"}
Expand Down Expand Up @@ -277,24 +279,62 @@ func TestTopologyBuilder_EvaluatesIncomingTrafficSplit(t *testing.T) {
svcKey := nn(svcB.Name, svcB.Namespace)
tsKeys := got.Services[svcKey].TrafficSplits

assert.Equal(t, 1, len(got.TrafficSplits[tsKeys[0]].Incoming))
assert.Equal(t, 1, len(got.TrafficSplits[tsKeys[1]].Incoming))
// Make sure the resulting Topology only has a single TrafficSplit.
assert.Len(t, tsKeys, 1)
assert.Len(t, got.TrafficSplits, 1)

for _, tsKey := range tsKeys {
ts := got.TrafficSplits[tsKey]
pod := got.Pods[ts.Incoming[0]]
incoming := got.TrafficSplits[tsKeys[0]].Incoming

if ts.Name == "ts2" {
assert.Equal(t, "10.10.1.2", pod.IP)
} else {
assert.Equal(t, "10.10.1.1", pod.IP)
}
assert.Equal(t, 1, len(incoming))

if tsKeys[0].Name == "ts2" {
assert.Equal(t, "10.10.1.2", got.Pods[incoming[0]].IP)
} else {
assert.Equal(t, "10.10.1.1", got.Pods[incoming[0]].IP)
}
}

// TestTopologyBuilder_BuildWithTrafficTargetAndTrafficSplit makes sure the topology can be built with TrafficTargets
// and TrafficSplits.
func TestTopologyBuilder_BuildWithTrafficTargetAndTrafficSplit(t *testing.T) {
// TestTopologyBuilder_BuildWithTrafficTarget makes sure a topology can be built using TrafficTargets.
func TestTopologyBuilder_BuildWithTrafficTarget(t *testing.T) {
selectorAppA := map[string]string{"app": "app-a"}
selectorAppB := map[string]string{"app": "app-b"}
annotations := map[string]string{}
svcPorts := []corev1.ServicePort{svcPort("port-8080", 8080, 8080)}

saA := createServiceAccount("my-ns", "service-account-a")
podA := createPod("my-ns", "app-a", saA, selectorAppA, "10.10.1.1")

saB := createServiceAccount("my-ns", "service-account-b")
svcB := createService("my-ns", "svc-b", annotations, svcPorts, selectorAppB, "10.10.1.16")
podB := createPod("my-ns", "app-b", saB, svcB.Spec.Selector, "10.10.2.1")

epB := createEndpoints(svcB, []*corev1.Pod{podB})

apiMatch := createHTTPMatch("api", []string{"GET", "POST"}, "/api")
metricMatch := createHTTPMatch("metric", []string{"GET"}, "/metric")
rtGrp := createHTTPRouteGroup("my-ns", "http-rt-grp", []spec.HTTPMatch{apiMatch, metricMatch})

ttMatch := []string{apiMatch.Name}
tt := createTrafficTarget("my-ns", "tt", saB, "8080", []*corev1.ServiceAccount{saA}, rtGrp, ttMatch)

k8sClient := fake.NewSimpleClientset(saA, saB, podA, podB, svcB, epB)
smiAccessClient := accessfake.NewSimpleClientset(tt)
smiSplitClient := splitfake.NewSimpleClientset()
smiSpecClient := specfake.NewSimpleClientset(rtGrp)

builder, err := createBuilder(k8sClient, smiAccessClient, smiSpecClient, smiSplitClient)
require.NoError(t, err)

resourceFilter := mk8s.NewResourceFilter()
got, err := builder.Build(resourceFilter)
require.NoError(t, err)

assertTopology(t, "testdata/topology-traffic-target.json", got)
}

// TestTopologyBuilder_BuildWithTrafficTargetAndTrafficSplitOnSameService makes sure a TrafficTarget won't be applied on
// a service if there is already a TrafficSplit applied to it.
func TestTopologyBuilder_BuildWithTrafficTargetAndTrafficSplitOnSameService(t *testing.T) {
selectorAppA := map[string]string{"app": "app-a"}
selectorAppB := map[string]string{"app": "app-b"}
selectorAppC := map[string]string{"app": "app-c"}
Expand Down Expand Up @@ -348,7 +388,7 @@ func TestTopologyBuilder_BuildWithTrafficTargetAndTrafficSplit(t *testing.T) {
got, err := builder.Build(resourceFilter)
require.NoError(t, err)

assertTopology(t, "testdata/topology-basic.json", got)
assertTopology(t, "testdata/topology-traffic-split-traffic-target.json", got)
}

// TestTopologyBuilder_BuildWithTrafficTargetSpecEmptyMatch makes sure that when TrafficTarget.Spec.Matches is empty,
Expand Down
85 changes: 3 additions & 82 deletions pkg/topology/testdata/topology-basic.json → ...opology-traffic-split-traffic-target.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,6 @@
"pods": [
"app-b@my-ns"
],
"trafficTargets": [
"svc-b@my-ns:tt@my-ns"
],
"trafficSplits": [
"ts@my-ns"
]
Expand Down Expand Up @@ -90,19 +87,13 @@
"name": "app-a",
"namespace": "my-ns",
"serviceAccount": "service-account-a",
"ip": "10.10.1.1",
"sourceOf": [
"svc-b@my-ns:tt@my-ns"
]
"ip": "10.10.1.1"
},
"app-b@my-ns": {
"name": "app-b",
"namespace": "my-ns",
"serviceAccount": "service-account-b",
"ip": "10.10.2.1",
"destinationOf": [
"svc-b@my-ns:tt@my-ns"
]
"ip": "10.10.2.1"
},
"app-c@my-ns": {
"name": "app-c",
Expand All @@ -117,77 +108,7 @@
"ip": "10.10.2.3"
}
},
"serviceTrafficTargets": {
"svc-b@my-ns:tt@my-ns": {
"service": "svc-b@my-ns",
"name": "tt",
"namespace": "my-ns",
"sources": [
{
"serviceAccount": "service-account-a",
"namespace": "my-ns",
"pods": [
"app-a@my-ns"
]
}
],
"destination": {
"serviceAccount": "service-account-b",
"namespace": "my-ns",
"ports": [
{
"name": "port-8080",
"protocol": "TCP",
"port": 8080,
"targetPort": 8080
}
],
"pods": [
"app-b@my-ns"
]
},
"specs": [
{
"httpRouteGroup": {
"kind": "HTTPRouteGroup",
"apiVersion": "specs.smi-spec.io/v1alpha1",
"metadata": {
"name": "http-rt-grp",
"namespace": "my-ns",
"creationTimestamp": null
},
"matches": [
{
"name": "api",
"methods": [
"GET",
"POST"
],
"pathRegex": "/api"
},
{
"name": "metric",
"methods": [
"GET"
],
"pathRegex": "/metric"
}
]
},
"httpMatches": [
{
"name": "api",
"methods": [
"GET",
"POST"
],
"pathRegex": "/api"
}
]
}
]
}
},
"serviceTrafficTargets": {},
"trafficSplits": {
"ts@my-ns": {
"name": "ts",
Expand Down

0 comments on commit c17c499

Please sign in to comment.