diff --git a/bin/publish-docker-images.sh b/bin/publish-docker-images.sh index ba1e08290..481f231a3 100755 --- a/bin/publish-docker-images.sh +++ b/bin/publish-docker-images.sh @@ -54,6 +54,11 @@ for IMAGE in "${BAZEL_IMAGES[@]}"; do IMAGES+=("${IMAGE}") done +# Build Servicegraph +bazel ${BAZEL_STARTUP_ARGS} run ${BAZEL_ARGS} "//example/servicegraph/docker:servicegraph" +docker tag "istio/example/servicegraph/docker:servicegraph" "servicegraph" +IMAGES+=(servicegraph) + # Tag and push for IMAGE in ${IMAGES[@]}; do diff --git a/istio.deps b/istio.deps index 8126fb679..e5bc54382 100644 --- a/istio.deps +++ b/istio.deps @@ -4,6 +4,6 @@ "repoName": "api", "prodBranch": "master", "file": "istio_api.bzl", - "lastStableSHA": "0ac7998e828072627e0329cf5b21b02fbe01ee04" + "lastStableSHA": "2b5fabb787e4fa030edb7cfb7000890f31c4c73e" } ] \ No newline at end of file diff --git a/istio_api.bzl b/istio_api.bzl index 62c6854f6..19f6f54db 100644 --- a/istio_api.bzl +++ b/istio_api.bzl @@ -15,7 +15,7 @@ ################################################################################ # -ISTIO_API_SHA = "0ac7998e828072627e0329cf5b21b02fbe01ee04" +ISTIO_API_SHA = "2b5fabb787e4fa030edb7cfb7000890f31c4c73e" def go_istio_api_repositories(use_local=False): ISTIO_API_BUILD_FILE = """ diff --git a/pkg/expr/expr.go b/pkg/expr/expr.go index b7a3b3b9b..27094fc5e 100644 --- a/pkg/expr/expr.go +++ b/pkg/expr/expr.go @@ -390,6 +390,60 @@ func Parse(src string) (ex *Expression, err error) { return ex, nil } +// ExtractEQMatches extracts equality sub expressions from the match expression. +// It only extracts `attribute == literal` type equality matches. +// It returns a list of such that +// if **any** of these comparisons is false, the expression will evaluate to false. +// These sub expressions can be hoisted out of the match clause and evaluated separately. +// For example +// destination.service == "abc" -- Used to index rules by destination service. +// context.protocol == "tcp" -- Used to filter rules by context +func ExtractEQMatches(src string) (map[string]interface{}, error) { + ex, err := Parse(src) + if err != nil { + return nil, err + } + eqMap := make(map[string]interface{}) + extractEQMatches(ex, eqMap) + return eqMap, nil +} + +func recordIfEQ(fn *Function, eqMap map[string]interface{}) { + if fn.Name != "EQ" { + return + } + + // x == "y" + if fn.Args[0].Var != nil && fn.Args[1].Const != nil { + eqMap[fn.Args[0].Var.Name] = fn.Args[1].Const.Value + return + } + + // yoda style, "y" == x + if fn.Args[0].Const != nil && fn.Args[1].Var != nil { + eqMap[fn.Args[1].Var.Name] = fn.Args[0].Const.Value + } +} + +// parseEQMatches traverse down "LANDS" and record EQs of variable and constants. +func extractEQMatches(ex *Expression, eqMap map[string]interface{}) { + if ex.Fn == nil { + return + } + + recordIfEQ(ex.Fn, eqMap) + + // only recurse on AND function. + if ex.Fn.Name != "LAND" { + return + } + + //TODO remove collected equality expressions from AST + for _, arg := range ex.Fn.Args { + extractEQMatches(arg, eqMap) + } +} + // DefaultCacheSize is the default size for the expression cache. const DefaultCacheSize = 1024 diff --git a/pkg/expr/expr_test.go b/pkg/expr/expr_test.go index 49282b926..a6acc2e7e 100644 --- a/pkg/expr/expr_test.go +++ b/pkg/expr/expr_test.go @@ -48,6 +48,8 @@ func TestGoodParse(t *testing.T) { {`request.header["X-FORWARDED-HOST"] == "aaa"`, `EQ(INDEX($request.header, "X-FORWARDED-HOST"), "aaa")`}, {`source.ip | ip("0.0.0.0")`, `OR($source.ip, ip("0.0.0.0"))`}, {`match(service.name, "cluster1.ns.*")`, `match($service.name, "cluster1.ns.*")`}, + {`a.b == 3.14 && c == "d" && r.h["abc"] == "pqr" || r.h["abc"] == "xyz"`, + `LOR(LAND(LAND(EQ($a.b, 3.14), EQ($c, "d")), EQ(INDEX($r.h, "abc"), "pqr")), EQ(INDEX($r.h, "abc"), "xyz"))`}, } for idx, tt := range tests { t.Run(fmt.Sprintf("[%d] %s", idx, tt.src), func(t *testing.T) { @@ -63,6 +65,88 @@ func TestGoodParse(t *testing.T) { } } +func TestExtractMatches(t *testing.T) { + for _, tc := range []struct { + desc string + src string + m map[string]interface{} + }{ + { + desc: "no ANDS", + src: `a || b || "c" || ( a && b )`, + m: map[string]interface{}{}, + }, + { + desc: "EQ check with function", + src: `substring(a, 5) == "abc"`, + m: map[string]interface{}{}, + }, + { + desc: "single EQ check", + src: `origin.host == "9.0.10.1"`, + m: map[string]interface{}{ + "origin.host": "9.0.10.1", + }, + }, + { + desc: "top level OR --> cannot extract equality subexpressions", + src: `a.b == 3.14 && c == "d" && r.h["abc"] == "pqr" || r.h["abc"] == "xyz"`, + m: map[string]interface{}{}, + }, + { + desc: "yoda", + src: `"d" == c`, + m: map[string]interface{}{ + "c": "d", + }, + }, + { + desc: "only top level ANDS", + src: `a.b == 3.14 && "d" == c && (r.h["abc"] == "pqr" || r.h["abc"] == "xyz")`, + m: map[string]interface{}{ + "a.b": 3.14, + "c": "d", + }, + }, + { + desc: "only top level ANDS, attribute to attribute comparison excluded", + src: `a.b == 3.14 && c == d && (r.h["abc"] == "pqr" || r.h["abc"] == "xyz")`, + m: map[string]interface{}{ + "a.b": 3.14, + }, + }, + { + src: `c == d && (r.h["abc"] == "pqr" || r.h["abc"] == "xyz") && a.b == 3.14`, + m: map[string]interface{}{ + "a.b": 3.14, + }, + }, + { // c == d is not included because it is an attribute to attribute comparison. + src: `c == d && (r.h["abc"] == "pqr" || r.h["abc"] == "xyz") && a.b == 3.14 && context.protocol == "TCP"`, + m: map[string]interface{}{ + "context.protocol": "TCP", + "a.b": 3.14, + }, + }, + { + src: `destination.service == "mysvc.FQDN" && request.headers["x-id"] == "AAA"`, + m: map[string]interface{}{ + "destination.service": "mysvc.FQDN", + }, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + m, err := ExtractEQMatches(tc.src) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !reflect.DeepEqual(m, tc.m) { + t.Fatalf("got %v, want %v", m, tc.m) + } + }) + } +} + func TestNewConstant(t *testing.T) { tests := []struct { v string diff --git a/pkg/runtime/controller.go b/pkg/runtime/controller.go index ea508cf9c..d32624cf8 100644 --- a/pkg/runtime/controller.go +++ b/pkg/runtime/controller.go @@ -331,11 +331,33 @@ const ( istioProtocol = "istio-protocol" ) +// buildRule builds runtime representation of rule based on match condition. +func buildRule(k store.Key, r *cpb.Rule, rt ResourceType) (*Rule, error) { + rule := &Rule{ + match: r.Match, + name: k.String(), + rtype: rt, + } + + if len(r.Match) == 0 { + return rule, nil + } + + m, err := expr.ExtractEQMatches(r.Match) + if err != nil { + return nil, err + } + if ContextProtocolTCP == m[ContextProtocolAttributeName] { + rule.rtype.protocol = protocolTCP + } + + return rule, nil +} + // resourceType maps labels to rule types. func resourceType(labels map[string]string) ResourceType { - ip := labels[istioProtocol] rt := defaultResourcetype() - if ip == "tcp" { + if ContextProtocolTCP == labels[istioProtocol] { rt.protocol = protocolTCP } return rt @@ -358,11 +380,7 @@ func (c *Controller) processRules(handlerConfig map[string]*cpb.Handler, cfg := obj.Spec rulec := cfg.(*cpb.Rule) - rule := &Rule{ - selector: rulec.Match, - name: k.Name, - rtype: resourceType(obj.Metadata.Labels), - } + acts := c.processActions(rulec.Actions, handlerConfig, instanceConfig, ht, k.Namespace) ruleActions := make(map[adptTmpl.TemplateVariety][]*Action) @@ -371,7 +389,13 @@ func (c *Controller) processRules(handlerConfig map[string]*cpb.Handler, ruleActions[vr] = append(ruleActions[vr], cf) } } - + // resourceType is used for backwards compatibility with labels: [istio-protocol: tcp] + rt := resourceType(obj.Metadata.Labels) + rule, err := buildRule(k, rulec, rt) + if err != nil { + glog.Warningf("Unable to process match condition: %v", err) + continue + } rule.actions = ruleActions rn := ruleConfig[k.Namespace] if rn == nil { diff --git a/pkg/runtime/controller_test.go b/pkg/runtime/controller_test.go index 785891184..5d3bae994 100644 --- a/pkg/runtime/controller_test.go +++ b/pkg/runtime/controller_test.go @@ -119,6 +119,57 @@ func checkRulesInvariants(t *testing.T, rules rulesListByNamespace) { } } +func TestController_buildrule(t *testing.T) { + key := store.Key{Kind: "kind1", Namespace: "ns1", Name: "name1"} + for _, tc := range []struct { + desc string + match string + want protocol + err error + }{ + { + desc: "http service", + match: `request.headers["x-id"] == "tcp"`, + want: protocolHTTP, + }, + { + desc: "tcp service", + match: ContextProtocolAttributeName + "== \"tcp\"", + want: protocolTCP, + }, + { + desc: "bad expression", + match: ContextProtocolAttributeName + "=$ \"tcp\"", + err: errors.New("unable to parse expression"), + }, + } { + t.Run(tc.desc, func(t *testing.T) { + rinput := &cpb.Rule{ + Match: tc.match, + } + rt := defaultResourcetype() + rt.protocol = tc.want + want := &Rule{ + name: key.String(), + match: rinput.Match, + rtype: rt, + } + + r, err := buildRule(key, rinput, defaultResourcetype()) + + checkError(t, tc.err, err) + + if tc.err != nil { + return + } + + if !reflect.DeepEqual(r, want) { + t.Fatalf("Got %v, want: %v", r, want) + } + }) + } +} + func TestController_workflow(t *testing.T) { mcd := maxCleanupDuration defer func() { maxCleanupDuration = mcd }() @@ -416,8 +467,8 @@ func TestController_Resolve2(t *testing.T) { return rulesMapByNamespace{ "ns1": rulesByName{ "r1": &Rule{ - selector: "true", - name: "r1", + match: "true", + name: "r1", actions: map[adptTmpl.TemplateVariety][]*Action{ adptTmpl.TEMPLATE_VARIETY_CHECK: { &Action{ diff --git a/pkg/runtime/dispatcher.go b/pkg/runtime/dispatcher.go index 5609a1f98..781d52c70 100644 --- a/pkg/runtime/dispatcher.go +++ b/pkg/runtime/dispatcher.go @@ -111,7 +111,7 @@ func newDispatcher(mapper expr.Evaluator, rt Resolver, gp *pool.GoroutinePool) * // dispatcher is responsible for dispatching incoming API calls // to the configured adapters. It implements the Dispatcher interface. type dispatcher struct { - // mapper is the selector and expression evaluator. + // mapper is the match and expression evaluator. // It is not directly used by dispatcher. mapper expr.Evaluator diff --git a/pkg/runtime/resolver.go b/pkg/runtime/resolver.go index 5edb33d0d..78e82eed4 100644 --- a/pkg/runtime/resolver.go +++ b/pkg/runtime/resolver.go @@ -32,8 +32,8 @@ import ( // Rule represents a runtime view of cpb.Rule. type Rule struct { - // Selector from the original rule. - selector string + // Match condition from the original rule. + match string // Actions are stored in runtime format. actions map[adptTmpl.TemplateVariety][]*Action // Rule is a top level config object and it has a unique name. @@ -43,6 +43,11 @@ type Rule struct { rtype ResourceType } +func (r Rule) String() string { + return fmt.Sprintf("[name:<%s>, match:<%s>, type:%s, actions: %v", + r.name, r.match, r.rtype, r.actions) +} + // resolver is the runtime view of the configuration database. type resolver struct { // evaluator evaluates selectors @@ -81,17 +86,22 @@ func newResolver(evaluator expr.PredicateEvaluator, identityAttribute string, de } } -// DefaultConfigNamespace holds istio wide configuration. -const DefaultConfigNamespace = "istio-config-default" +const ( + // DefaultConfigNamespace holds istio wide configuration. + DefaultConfigNamespace = "istio-config-default" -// DefaultIdentityAttribute is attribute that defines config scopes. -const DefaultIdentityAttribute = "target.service" + // DefaultIdentityAttribute is attribute that defines config scopes. + DefaultIdentityAttribute = "destination.service" -// ContextProtocolAttributeName is the attribute that defines the protocol context. -const ContextProtocolAttributeName = "context.protocol" + // ContextProtocolAttributeName is the attribute that defines the protocol context. + ContextProtocolAttributeName = "context.protocol" -// expectedResolvedActionsCount is used to preallocate slice for actions. -const expectedResolvedActionsCount = 10 + // ContextProtocolTCP defines constant for tcp protocol. + ContextProtocolTCP = "tcp" + + // expectedResolvedActionsCount is used to preallocate slice for actions. + expectedResolvedActionsCount = 10 +) // Resolve resolves the in memory configuration to a set of actions based on request attributes. // Resolution is performed in the following order @@ -196,12 +206,12 @@ func (r *resolver) filterActions(rulesArr [][]*Rule, attrs attribute.Bag, nselected := 0 var err error ctxProtocol, _ := attrs.Get(ContextProtocolAttributeName) - tcp := ctxProtocol == "tcp" + tcp := ctxProtocol == ContextProtocolTCP for _, rules := range rulesArr { for _, rule := range rules { act := rule.actions[variety] - if act == nil { // do not evaluate selector if there is no variety specific action there. + if act == nil { // do not evaluate match if there is no variety specific action there. continue } // default rtype is HTTP + Check|Report|Preprocess @@ -213,8 +223,8 @@ func (r *resolver) filterActions(rulesArr [][]*Rule, attrs attribute.Bag, } // do not evaluate empty predicates. - if len(rule.selector) != 0 { - if selected, err = r.evaluator.EvalPredicate(rule.selector, attrs); err != nil { + if len(rule.match) != 0 { + if selected, err = r.evaluator.EvalPredicate(rule.match, attrs); err != nil { return nil, 0, err } if !selected { diff --git a/pkg/runtime/resolver_test.go b/pkg/runtime/resolver_test.go index da723454b..3df6cb1d3 100644 --- a/pkg/runtime/resolver_test.go +++ b/pkg/runtime/resolver_test.go @@ -127,7 +127,7 @@ func TestResolver_Resolve(t *testing.T) { err: "identity not found", }, { - desc: "failure selector error", + desc: "failure match error", bag: map[string]interface{}{ ia: "myservice.myns", }, @@ -135,8 +135,8 @@ func TestResolver_Resolve(t *testing.T) { {ns, 5}, {"myns", 3}, }, - selectError: "invalid selector syntax", - err: "invalid selector", + selectError: "invalid match syntax", + err: "invalid match", nactions: 0, }, } @@ -209,7 +209,7 @@ func assertResolverError(t *testing.T, got error, want string) { func newFakeRule(vr adptTmpl.TemplateVariety, length int) *Rule { return &Rule{ - selector: "request.size=2000", + match: "request.size=2000", actions: map[adptTmpl.TemplateVariety][]*Action{ vr: make([]*Action, length), }, diff --git a/testdata/config/prometheus.yaml b/testdata/config/prometheus.yaml index c6f3fe24e..b7f139ade 100644 --- a/testdata/config/prometheus.yaml +++ b/testdata/config/prometheus.yaml @@ -92,9 +92,8 @@ kind: rule metadata: name: promtcp namespace: istio-config-default - labels: - istio-protocol: tcp # needed so that mixer will only execute when context.protocol == TCP spec: + match: context.protocol == "tcp" actions: - handler: handler.prometheus instances: diff --git a/testdata/config/tcpMetrics.yaml b/testdata/config/tcpMetrics.yaml index 4c4f7f650..29502d722 100644 --- a/testdata/config/tcpMetrics.yaml +++ b/testdata/config/tcpMetrics.yaml @@ -3,8 +3,6 @@ kind: metric metadata: name: tcpbytesent namespace: istio-config-default - labels: - istio-protocol: tcp # needed so that mixer will only generate when context.protocol == tcp spec: value: connection.sent.bytes | 0 dimensions: @@ -19,8 +17,6 @@ kind: metric metadata: name: tcpbytereceived namespace: istio-config-default - labels: - istio-protocol: tcp # needed so that mixer will only generate when context.protocol == tcp spec: value: connection.received.bytes | 0 dimensions: