Repository: knative-sandbox/net-kourier
Pull request #376 (2020-11-23 15:02:35)

[RELEASE-0.19] Backport update to Envoy 1.14.4
This backports the update to Envoy 1.14.4 and the naming changes to the release-0.19 branch. Why? To make a seamless upgrade possible.

I'm working on upgrade tests for the master branch and currently, the upgrade drops a lot of requests. That is because the envoy control-plane library on master isn't compatible with Envoy 1.12. So there's no intermediate ground where both gateways can talk to the same control-plane pod. Bummer.

This backports just enough to make 0.18 upgradeable to 0.19 (tested) and then 0.19 to HEAD (tested too).

/assign @jmprusi @davidor 

knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/net-kourier/pull/376#" title="Author self-approved">markusthoemmes</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fnet-kourier).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/net-kourier/blob/release-0.19/OWNERS)~~ [markusthoemmes]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
markusthoemmes: /hold

I'll run this through the Serving test-suite, just to be sure I didn't break anything here.
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-sandbox-net-kourier-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/generator/ingress_translator.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/376/pull-knative-sandbox-net-kourier-go-coverage/1330889507175665664/artifacts/line-cov.html#file0) | 88.5% | 76.0% | -12.5

knative-prow-robot: @markusthoemmes: The following test **failed**, say `/retest` to rerun all failed tests:

Test name | Commit | Details | Rerun command
--- | --- | --- | ---
pull-knative-sandbox-net-kourier-unit-tests | 6761b0cf4933c2fbd49e025304715bda3cd01e39 | [link](https://prow.knative.dev/view/gs/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/376/pull-knative-sandbox-net-kourier-unit-tests/1330889507137916928) | `/test pull-knative-sandbox-net-kourier-unit-tests`

[Full PR test history](https://gubernator.knative.dev/pr/knative-sandbox_net-kourier/376). [Your PR dashboard](https://gubernator.knative.dev/pr/markusthoemmes).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
knative-metrics-robot: The following is the coverage report on the affected files.
Say `/test pull-knative-sandbox-net-kourier-go-coverage` to re-run this coverage report

File | Old Coverage | New Coverage | Delta
---- |:------------:|:------------:|:-----:
[pkg/generator/ingress_translator.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/376/pull-knative-sandbox-net-kourier-go-coverage/1330890918814814208/artifacts/line-cov.html#file0) | 88.5% | 76.0% | -12.5

codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/376?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`release-0.19@f95f82b`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/net-kourier/pull/376/graphs/tree.svg?width=650&height=150&src=pr&token=KmlRo23zX4)](https://codecov.io/gh/knative-sandbox/net-kourier/pull/376?src=pr&el=tree)

```diff
@@               Coverage Diff               @@
##             release-0.19     #376   +/-   ##
===============================================
  Coverage                ?   78.87%           
===============================================
  Files                   ?       14           
  Lines                   ?      535           
  Branches                ?        0           
===============================================
  Hits                    ?      422           
  Misses                  ?       84           
  Partials                ?       29           
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/376?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/376?src=pr&el=footer). Last update [f95f82b...7800ca5](https://codecov.io/gh/knative-sandbox/net-kourier/pull/376?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

markusthoemmes: /unhold

Serving seems happy
knative-prow-robot: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative-sandbox/net-kourier/pull/376#pullrequestreview-536757774" title="LGTM">davidor</a>*, *<a href="https://github.com/knative-sandbox/net-kourier/pull/376#" title="Author self-approved">markusthoemmes</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative-sandbox%2Fnet-kourier).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative-sandbox/net-kourier/blob/release-0.19/OWNERS)~~ [davidor,markusthoemmes]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/config/300-gateway.yaml b/config/300-gateway.yaml
index b98204d0..1dc7a457 100644
--- a/config/300-gateway.yaml
+++ b/config/300-gateway.yaml
@@ -35,7 +35,7 @@ spec:
             - --log-level info
           command:
             - /usr/local/bin/envoy
-          image: docker.io/maistra/proxyv2-ubi8:1.1.5
+          image: docker.io/maistra/proxyv2-ubi8:2.0.0
           name: kourier-gateway
           ports:
             - name: http2-external
@@ -156,7 +156,9 @@ data:
                             - "*"
                           routes:
                             - match:
-                                regex: '/(certs|stats(/prometheus)?|server_info|clusters|listeners|ready)?'
+                                safe_regex:
+                                  google_re2: {}
+                                  regex: '/(certs|stats(/prometheus)?|server_info|clusters|listeners|ready)?'
                                 headers:
                                   - name: ':method'
                                     exact_match: GET
diff --git a/go.sum b/go.sum
index d3eb3688..414ab8df 100644
--- a/go.sum
+++ b/go.sum
@@ -1099,7 +1099,6 @@ k8s.io/kube-openapi v0.0.0-20200410145947-bcb3869e6f29/go.mod h1:F+5wygcW0wmRTnM
 k8s.io/utils v0.0.0-20200324210504-a9aa75ae1b89/go.mod h1:sZAwmy6armz5eXlNoLmJcl4F1QuKu7sr+mFQ0byX7Ew=
 k8s.io/utils v0.0.0-20200603063816-c1c6865ac451 h1:v8ud2Up6QK1lNOKFgiIVrZdMg7MpmSnvtrOieolJKoE=
 k8s.io/utils v0.0.0-20200603063816-c1c6865ac451/go.mod h1:jPW/WVKK9YHAvNhRxK0md/EJ228hCsBRufyofKtW8HA=
-knative.dev/hack v0.0.0-20201102193445-9349aeeb6701 h1:at6mUfi8gHWlBRd/qNM66JvUQ4C964cOpsy9hzac+7c=
 knative.dev/hack v0.0.0-20201102193445-9349aeeb6701/go.mod h1:PHt8x8yX5Z9pPquBEfIj0X66f8iWkWfR0S/sarACJrI=
 knative.dev/hack v0.0.0-20201103151104-3d5abc3a0075 h1:YAgWplKIy4O5e3F5vUUECmXAAyZ0M5ymo6fCt1jeZhs=
 knative.dev/hack v0.0.0-20201103151104-3d5abc3a0075/go.mod h1:PHt8x8yX5Z9pPquBEfIj0X66f8iWkWfR0S/sarACJrI=
diff --git a/pkg/generator/ingress_translator.go b/pkg/generator/ingress_translator.go
index b1eaeb13..8f2b7b49 100644
--- a/pkg/generator/ingress_translator.go
+++ b/pkg/generator/ingress_translator.go
@@ -91,7 +91,9 @@ func (translator *IngressTranslator) translateIngress(ctx context.Context, ingre
 	externalHosts := make([]*route.VirtualHost, 0, len(ingress.Spec.Rules))
 	clusters := make([]*v2.Cluster, 0, len(ingress.Spec.Rules))
 
-	for _, rule := range ingress.Spec.Rules {
+	for i, rule := range ingress.Spec.Rules {
+		ruleName := fmt.Sprintf("(%s/%s).Rules[%d]", ingress.Namespace, ingress.Name, i)
+
 		routes := make([]*route.Route, 0, len(rule.HTTP.Paths))
 		for _, httpPath := range rule.HTTP.Paths {
 			// Default the path to "/" if none is passed.
@@ -100,8 +102,14 @@ func (translator *IngressTranslator) translateIngress(ctx context.Context, ingre
 				path = "/"
 			}
 
+			pathName := fmt.Sprintf("%s.Paths[%s]", ruleName, path)
+
 			wrs := make([]*route.WeightedCluster_ClusterWeight, 0, len(httpPath.Splits))
 			for _, split := range httpPath.Splits {
+				// The FQN of the service is sufficient here, as clusters towards the
+				// same service are supposed to be deduplicated anyway.
+				splitName := fmt.Sprintf("%s/%s", split.ServiceNamespace, split.ServiceName)
+
 				if err := trackService(translator.tracker, split.ServiceName, ingress); err != nil {
 					return nil, err
 				}
@@ -156,18 +164,16 @@ func (translator *IngressTranslator) translateIngress(ctx context.Context, ingre
 				}
 
 				connectTimeout := 5 * time.Second
-				cluster := envoy.NewCluster(split.ServiceName+path, connectTimeout, publicLbEndpoints, http2, typ)
+				cluster := envoy.NewCluster(splitName, connectTimeout, publicLbEndpoints, http2, typ)
 				clusters = append(clusters, cluster)
 
-				weightedCluster := envoy.NewWeightedCluster(split.ServiceName+path, uint32(split.Percent), split.AppendHeaders)
+				weightedCluster := envoy.NewWeightedCluster(splitName, uint32(split.Percent), split.AppendHeaders)
 				wrs = append(wrs, weightedCluster)
 			}
 
 			if len(wrs) != 0 {
-				// Note: routeName uses the non-defaulted path.
-				routeName := ingress.Name + "_" + ingress.Namespace + "_" + httpPath.Path
 				routes = append(routes, envoy.NewRoute(
-					routeName, matchHeadersFromHTTPPath(httpPath), path, wrs, 0, httpPath.AppendHeaders, httpPath.RewriteHost))
+					pathName, matchHeadersFromHTTPPath(httpPath), path, wrs, 0, httpPath.AppendHeaders, httpPath.RewriteHost))
 			}
 		}
 
@@ -182,9 +188,9 @@ func (translator *IngressTranslator) translateIngress(ctx context.Context, ingre
 				"client":     "kourier",
 				"visibility": string(rule.Visibility),
 			}, ingress.GetLabels())
-			virtualHost = envoy.NewVirtualHostWithExtAuthz(ingress.Name, contextExtensions, domainsForRule(rule), routes)
+			virtualHost = envoy.NewVirtualHostWithExtAuthz(ruleName, contextExtensions, domainsForRule(rule), routes)
 		} else {
-			virtualHost = envoy.NewVirtualHost(ingress.Name, domainsForRule(rule), routes)
+			virtualHost = envoy.NewVirtualHost(ruleName, domainsForRule(rule), routes)
 		}
 
 		internalHosts = append(internalHosts, virtualHost)


Test patch:
diff --git a/pkg/generator/ingress_translator_test.go b/pkg/generator/ingress_translator_test.go
index a4774829..fff7bcd2 100644
--- a/pkg/generator/ingress_translator_test.go
+++ b/pkg/generator/ingress_translator_test.go
@@ -130,7 +130,7 @@ func TestTrafficSplits(t *testing.T) {
 	assertWeightedClusterCorrect(
 		t,
 		weightedClusters[0],
-		"hello-world-rev1/",
+		"default/hello-world-rev1",
 		uint32(60),
 		map[string]string{
 			"Knative-Serving-Namespace": "default",
@@ -142,7 +142,7 @@ func TestTrafficSplits(t *testing.T) {
 	assertWeightedClusterCorrect(
 		t,
 		weightedClusters[1],
-		"hello-world-rev2/",
+		"default/hello-world-rev2",
 		uint32(40),
 		map[string]string{
 			"Knative-Serving-Namespace": "default",
@@ -154,7 +154,7 @@ func TestTrafficSplits(t *testing.T) {
 	assert.Equal(
 		t,
 		true,
-		clustersExist([]string{"hello-world-rev1/", "hello-world-rev2/"}, ingressTranslation.clusters),
+		clustersExist([]string{"default/hello-world-rev1", "default/hello-world-rev2"}, ingressTranslation.clusters),
 	)
 }