feat(kuma-cp) envoy configs for fault injections #649

lobkovilya · 2020-03-30T22:32:59Z

Summary

Final major part for implementing Fault Injections. Add configurers to envoy configuration.
Based on: https://github.com/Kong/kuma/pull/647

pkg/core/faultinjections/matcher.go

pkg/xds/envoy/listeners/fault_injection_configurer.go

jakubdyszkiewicz · 2020-03-31T08:06:32Z

pkg/xds/envoy/listeners/util.go

+
+var bandwidthRegex = regexp.MustCompile(`(\d*)\s?([gmk]?bps)`)
+
+func ConvertBandwidth(bandwidth *wrappers.StringValue) (*envoy_filter_fault.FaultRateLimit_FixedLimit_, error) {


This function works only for Fault Injection (envoy_filter_fault.FaultRateLimit_FixedLimit_) so it's not util in any way. It should be placed in fault configurer.

jakubdyszkiewicz · 2020-03-31T08:12:32Z

pkg/xds/envoy/routes/reset_tags_configurer.go

@@ -0,0 +1,19 @@
+package routes


If you use the code from other PR, please open a PR against that PR. It makes the code review easier because I don't have to reread the code.

So this PR should not be to master but to feat/header-with-tags, then when feat/header-with-tags is merged to master you can rebase and change the target of this PR to master

jakubdyszkiewicz · 2020-03-31T12:38:51Z

I think there is one more edge case with this solution.

Let's assume there is dataplane with 2 inbounds

inbounds:
- port: 8080
  tags:
    service: web
- port: 8081
  tags:
    service: web-api

and we've got such policy

sources:
- service: edge
destinations:
- service: web
conf:
...

I think this will be applied on both edge->web and edge->web-api and not only on edge->web-api.

The logic the matching logic should be exactly the same with TrafficPermission as with FaultInjection.
You should run the matching logic for every inbound interface. Keep in mind that TrafficPermission seems to work with addition (apply all that matches), not the best match right now (it was the first policy, I think we had a different concept in mind then and it's not unified).

jakubdyszkiewicz · 2020-04-02T08:39:47Z

pkg/xds/envoy/routes/tags_configurer_test.go

+            requestHeadersToAdd:
+            - header:
+                key: x-kuma-tags
+                value: '&tag1=value11,value12&&tag2=value21&&tag3=value31,value32,value33&'`,


why there are double & now?

With regex we have to care about 2 important things:

we don't match the suffix of the Key

we don't match the prefix of the Value

The first condition is covered by & before each Key. The second one is covered by [,&] after each Value.
So in the case of a single ampersand, we might match it with the end of the Value and we have nothing to match with the beginning of the next Key.

Also, that could be solved by an additional comma after the last value - &tag1=value1,value2,&tag2=value1&. Probably additional comma will be more understandable, what do you think?

pkg/xds/envoy/tags/match_test.go

jakubdyszkiewicz · 2020-04-02T09:31:22Z

pkg/core/policy/dataplane_matcher.go

 // Every matching DataplanePolicy gets a rank (score) defined as a maximum number of tags in a matching selector.
 // DataplanePolicy with an empty list of selectors is considered a match with a rank (score) of 0.
 // DataplanePolicy with an empty selector (one that has no tags) is considered a match with a rank (score) of 0.
 // In case if there are multiple DataplanePolicies with the same rank (score), the policy created last is chosen.
-func SelectDataplanePolicy(dataplane *mesh.DataplaneResource, policies []DataplanePolicy) DataplanePolicy {
+func SelectDataplanePolicy(matching MatchingTags, policies []DataplanePolicy) DataplanePolicy {


This is clever solution, but can we take a step back?

The solution with reusing DataplanePolicy for matching ConnectionPolicy with adapter is DRY, but I must say this is a little bit hard to follow. This is one of the most important logic in our code, so I want it to be as understandable as possible.

I'd rather have simple understandable interfaces with clear input. I prefer to have a little bit of copying instead of using wrong abstraction (https://www.sandimetz.com/blog/2016/1/20/the-wrong-abstraction)

Additionally, the code from FaultInjectionMatcher should be in policy since it will be soon used for TrafficPermission and also sooner or later on TrafficLogs, so FaultInjectionMatcher should be as thin as possible.

I experimented on branch and here is the result https://github.com/Kong/kuma/commit/bc37646d9a9943f0ec9e870267c5142c99d18c64
we don't need te full logic from DataplaneMatcher because with ConnectionPolicies we enforce that there are sources and destinations.

What do you think?

pkg/xds/envoy/tags/serialize.go

jakubdyszkiewicz · 2020-04-02T09:44:27Z

pkg/xds/envoy/tags/match_test.go

+				"tag3": {"value3": true, "value4": true},
+			},
+			selector: mesh_proto.SingleValueTagSet{
+				"tag1": "value1",


can we change this to value2 just to see if we match later tag from MultiValuteTagSet?

I can't see the change

I added a new test case lower:

Entry("match the latter valuer", testCase{ serviceTags: mesh_proto.MultiValueTagSet{ "tag1": {"value1": true, "value2": true}, }, selector: mesh_proto.SingleValueTagSet{ "tag1": "value2", }, expected: true, }),

jakubdyszkiewicz · 2020-04-02T09:50:31Z

pkg/xds/envoy/listeners/fault_injection_configurer.go

+		Abort: convertAbort(f.faultInjection.Conf.GetAbort()),
+		Headers: []*envoy_api_v2_route.HeaderMatcher{
+			createHeaders(f.faultInjection.SourceTags()),
+		},


Please remind me. Why can't we generate separate envoy_api_v2_route.HeaderMatcher for every selector.Match from FaultInjection? This would simplify the logic of constructing gigantic regex with ORs to construct regex for every selector.Match

I discovered that HeaderMatcher (at least with the same header's name) works as AND. We also have an option to generate separate fault filters, but that's probably bulkier solution

Thanks, you are right
"A match will happen if all the headers in the config are present in the request with the same values (or based on presence if the value field is not in the config)."
https://www.envoyproxy.io/docs/envoy/latest/api-v2/config/filter/http/fault/v2/fault.proto

I'd lean towards picking a solution with generating many fault filters just to avoid crazy long regexes (btw. is 500 the limit of chars in regex?) and also for easier migration to x-kuma-tag-* in the future, but we can stick to or regex solution as well for now.

This is not exactly chars in regex:

This field controls the RE2 “program size” which is a rough estimate of how complex a compiled regex is to evaluate. A regex that has a program size greater than the configured value will fail to compile. In this case, the configured max program size can be increased or the regex can be simplified. If not specified, the default is 100.

But there is a linear dependency between the size in chars and "program size".
In both cases (many filters or complex regex) we will be limited by that "program size" and limited with number of tags/selectors in the policy.

I'd stay with long regex for this PR and later we can compare which approach is faster - e.g. 10 filter faults with program size 500 or one filter fault with program size 5000

Oh and also we probably can calculate "program size" on the fly

pkg/xds/envoy/listeners/fault_injection_configurer.go

jakubdyszkiewicz

some minors + please add CHANGELOG.md and it's ready to merge.

jakubdyszkiewicz · 2020-04-03T14:18:21Z

pkg/core/policy/dataplane_matcher.go

-// SelectDataplanePolicy given a Dataplane definition and a list of DataplanePolicy returns the "best matching" DataplanePolicy.
-// A DataplanePolicy is considered a match if one of the inbound interfaces of a Dataplane or tag section on Gateway Dataplane has all tags of DataplanePolicy's selector.
+// SelectDataplanePolicy given a something that could be matched (mesh_proto.Dataplane,
+// mesh_proto.Dataplane_Networking_Inbound, etc.) and a list of DataplanePolicy returns the "best matching" DataplanePolicy.


Comment should be reverted

jakubdyszkiewicz · 2020-04-03T14:24:08Z

pkg/xds/envoy/tags/match_test.go

+				"tag3": {"value3": true, "value4": true},
+			},
+			selector: mesh_proto.SingleValueTagSet{
+				"tag1": "value1",


I can't see the change

jakubdyszkiewicz · 2020-04-03T14:26:48Z

pkg/xds/server/components.go

@@ -4,6 +4,8 @@ import (
 	"context"
 	"time"

+	"github.com/Kong/kuma/pkg/core/faultinjections"


I think this should be in a group with other kuma imports. This goimport linter is not ideal unfortunately :(

lobkovilya requested a review from a team March 30, 2020 22:32

jakubdyszkiewicz requested changes Mar 31, 2020

View reviewed changes

jakubdyszkiewicz reviewed Apr 2, 2020

View reviewed changes

lobkovilya force-pushed the feat/fault-injection-envoy branch from d578ed6 to 0fdd25b Compare April 3, 2020 11:17

jakubdyszkiewicz approved these changes Apr 3, 2020

View reviewed changes

lobkovilya force-pushed the feat/fault-injection-envoy branch from 3f83558 to 015ae82 Compare April 6, 2020 10:17

lobkovilya and others added 23 commits April 6, 2020 17:18

feat(kuma-cp) save service's tags to header

c4daf41

feat(kuma-cp) make check

7f02433

feat(kuma-cp) make configurer more specific for tags

9248c03

feat(kuma-cp) cover configurers with tests

7386740

feat(kuma-cp) make check

2ce9323

feat(kuma-cp) resolve review comments

78c71bf

feat(kuma-cp) fix test

f9e095f

feat(kuma-cp) fix tests

27dce97

feat(kuma-cp) resolve review comments

90f385e

feat(kuma-cp) add configurer for FaultInjection policy

88919fd

feat(kuma-cp) make check

b081d84

feat(kuma-cp) convert tags matcher to headers

d30cf9b

feat(kuma-cp) create separate package for tags

72a8fea

feat(kuma-cp) fix test

01d0a8a

feat(kuma-cp) add support of asterisk into matcher

bafa37f

feat(kuma-cp) fix test

d84d39a

feat(kuma-cp) fix edge cases for matcher and configurer

77af50f

feat(kuma-cp) resolve review comments

a27b0ff

feat(kuma-cp) remove commented code

75fee84

feat(kuma-cp) fix test

f6758a5

review policies

81a731d

feat(kuma-cp) resolve review comments

67cfeb5

feat(kuma-cp) after-rebase fixes

319d2e6

lobkovilya added 2 commits April 6, 2020 17:18

feat(kuma-cp) resolve code review issues

5dde5e6

feat(kuma-cp) CHANGELOG

1034ab7

lobkovilya force-pushed the feat/fault-injection-envoy branch from 015ae82 to 1034ab7 Compare April 6, 2020 10:19

Merge branch 'master' into feat/fault-injection-envoy

3e3058e

lobkovilya merged commit 6005e4c into master Apr 6, 2020

lobkovilya deleted the feat/fault-injection-envoy branch May 1, 2020 15:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(kuma-cp) envoy configs for fault injections #649

feat(kuma-cp) envoy configs for fault injections #649

lobkovilya commented Mar 30, 2020 •

edited

jakubdyszkiewicz Mar 31, 2020

jakubdyszkiewicz Mar 31, 2020

jakubdyszkiewicz commented Mar 31, 2020

jakubdyszkiewicz Apr 2, 2020

lobkovilya Apr 2, 2020

jakubdyszkiewicz Apr 2, 2020

jakubdyszkiewicz Apr 2, 2020

jakubdyszkiewicz Apr 3, 2020

lobkovilya Apr 4, 2020

jakubdyszkiewicz Apr 2, 2020

lobkovilya Apr 2, 2020

jakubdyszkiewicz Apr 2, 2020

lobkovilya Apr 2, 2020

lobkovilya Apr 2, 2020

jakubdyszkiewicz left a comment

jakubdyszkiewicz Apr 3, 2020

jakubdyszkiewicz Apr 3, 2020

jakubdyszkiewicz Apr 3, 2020


		var bandwidthRegex = regexp.MustCompile(`(\d*)\s?([gmk]?bps)`)

		func ConvertBandwidth(bandwidth wrappers.StringValue) (envoy_filter_fault.FaultRateLimit_FixedLimit_, error) {

feat(kuma-cp) envoy configs for fault injections #649

feat(kuma-cp) envoy configs for fault injections #649

Conversation

lobkovilya commented Mar 30, 2020 • edited

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubdyszkiewicz commented Mar 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakubdyszkiewicz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lobkovilya commented Mar 30, 2020 •

edited