Merge branch 'master' into crd-validator2

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

istio.deps

@@ -4,6 +4,6 @@
 		"repoName": "api",
 		"prodBranch": "master",
 		"file": "istio_api.bzl",
-		"lastStableSHA": "0ac7998e828072627e0329cf5b21b02fbe01ee04"
+		"lastStableSHA": "2b5fabb787e4fa030edb7cfb7000890f31c4c73e"
 	}
 ]

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 = """

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  <attribute name, value> 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
 

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

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 {

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{

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