Repository: knative-sandbox/net-kourier
Pull request #634 (2021-09-09 01:51:54)

Set ValidateClusters in NewRouteConfig
Current code adds the validation one by one, but we always need it.
So this patch sets ValidateClusters in NewRouteConfig by default.

/kind cleanup


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/634#" title="Author self-approved">nak3</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/main/OWNERS)~~ [nak3]

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":[]} -->
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/caches.go](https://storage.cloud.google.com/knative-prow/pr-logs/pull/knative-sandbox_net-kourier/634/pull-knative-sandbox-net-kourier-go-coverage/1435782989924536320/artifacts/line-cov.html#file1) | 69.2% | 68.2% | -1.0

codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/634?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) Report
> Merging [#634](https://codecov.io/gh/knative-sandbox/net-kourier/pull/634?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (94eebdc) into [main](https://codecov.io/gh/knative-sandbox/net-kourier/commit/1f4223a872666ef65a66dfd755e41daebb51e8e8?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) (1f4223a) will **decrease** coverage by `0.09%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/knative-sandbox/net-kourier/pull/634/graphs/tree.svg?width=650&height=150&src=pr&token=KmlRo23zX4&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)](https://codecov.io/gh/knative-sandbox/net-kourier/pull/634?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)

```diff
@@            Coverage Diff             @@
##             main     #634      +/-   ##
==========================================
- Coverage   70.36%   70.27%   -0.10%     
==========================================
  Files          18       18              
  Lines         658      656       -2     
==========================================
- Hits          463      461       -2     
  Misses        170      170              
  Partials       25       25              
```


| [Impacted Files](https://codecov.io/gh/knative-sandbox/net-kourier/pull/634?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox) | Coverage Δ | |
|---|---|---|
| [pkg/generator/caches.go](https://codecov.io/gh/knative-sandbox/net-kourier/pull/634/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-cGtnL2dlbmVyYXRvci9jYWNoZXMuZ28=) | `62.72% <ø> (-0.99%)` | :arrow_down: |
| [pkg/envoy/api/http\_connection\_manager.go](https://codecov.io/gh/knative-sandbox/net-kourier/pull/634/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox#diff-cGtnL2Vudm95L2FwaS9odHRwX2Nvbm5lY3Rpb25fbWFuYWdlci5nbw==) | `94.44% <100.00%> (+0.15%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/634?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/634?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Last update [1f4223a...94eebdc](https://codecov.io/gh/knative-sandbox/net-kourier/pull/634?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative-sandbox).

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/634#pullrequestreview-749949191" title="Approved">markusthoemmes</a>*, *<a href="https://github.com/knative-sandbox/net-kourier/pull/634#" title="Author self-approved">nak3</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/main/OWNERS)~~ [markusthoemmes,nak3]

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/pkg/envoy/api/http_connection_manager.go b/pkg/envoy/api/http_connection_manager.go
index 002e43bf..d574b02a 100644
--- a/pkg/envoy/api/http_connection_manager.go
+++ b/pkg/envoy/api/http_connection_manager.go
@@ -28,6 +28,7 @@ import (
 	"github.com/envoyproxy/go-control-plane/pkg/wellknown"
 	"google.golang.org/protobuf/types/known/anypb"
 	"google.golang.org/protobuf/types/known/durationpb"
+	"google.golang.org/protobuf/types/known/wrapperspb"
 
 	"knative.dev/net-kourier/pkg/config"
 )
@@ -86,5 +87,10 @@ func NewRouteConfig(name string, virtualHosts []*route.VirtualHost) *route.Route
 	return &route.RouteConfiguration{
 		Name:         name,
 		VirtualHosts: virtualHosts,
+		// Without this validation we can generate routes that point to non-existing clusters
+		// That causes some "no_cluster" errors in Envoy and the "TestUpdate"
+		// in the Knative serving test suite fails sometimes.
+		// Ref: https://github.com/knative/serving/blob/f6da03e5dfed78593c4f239c3c7d67c5d7c55267/test/conformance/ingress/update_test.go#L37
+		ValidateClusters: wrapperspb.Bool(true),
 	}
 }
diff --git a/pkg/generator/caches.go b/pkg/generator/caches.go
index 40a9e2b8..924465e6 100644
--- a/pkg/generator/caches.go
+++ b/pkg/generator/caches.go
@@ -28,7 +28,6 @@ import (
 	cachetypes "github.com/envoyproxy/go-control-plane/pkg/cache/types"
 	cache "github.com/envoyproxy/go-control-plane/pkg/cache/v3"
 	"github.com/google/uuid"
-	"google.golang.org/protobuf/types/known/wrapperspb"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
@@ -219,14 +218,6 @@ func generateListenersAndRouteConfigs(
 	externalTLSRouteConfig := envoy.NewRouteConfig(externalTLSRouteConfigName, externalTLSVirtualHosts)
 	internalRouteConfig := envoy.NewRouteConfig(internalRouteConfigName, clusterLocalVirtualHosts)
 
-	// Without this we can generate routes that point to non-existing clusters
-	// That causes some "no_cluster" errors in Envoy and the "TestUpdate"
-	// in the Knative serving test suite fails sometimes.
-	// Ref: https://github.com/knative/serving/blob/f6da03e5dfed78593c4f239c3c7d67c5d7c55267/test/conformance/ingress/update_test.go#L37
-	externalRouteConfig.ValidateClusters = wrapperspb.Bool(true)
-	externalTLSRouteConfig.ValidateClusters = wrapperspb.Bool(true)
-	internalRouteConfig.ValidateClusters = wrapperspb.Bool(true)
-
 	// Now we setup connection managers, that reference the routeconfigs via RDS.
 	externalManager := envoy.NewHTTPConnectionManager(externalRouteConfig.Name, cfg.Kourier.EnableServiceAccessLogging)
 	externalTLSManager := envoy.NewHTTPConnectionManager(externalTLSRouteConfig.Name, cfg.Kourier.EnableServiceAccessLogging)


Test patch:
diff --git a/pkg/envoy/api/http_connection_manager_test.go b/pkg/envoy/api/http_connection_manager_test.go
index e5b6dbae..086107a2 100644
--- a/pkg/envoy/api/http_connection_manager_test.go
+++ b/pkg/envoy/api/http_connection_manager_test.go
@@ -25,6 +25,7 @@ import (
 	"google.golang.org/protobuf/proto"
 	"google.golang.org/protobuf/testing/protocmp"
 	"google.golang.org/protobuf/types/known/anypb"
+	"google.golang.org/protobuf/types/known/wrapperspb"
 	"gotest.tools/v3/assert"
 )
 
@@ -55,8 +56,9 @@ func TestNewRouteConfig(t *testing.T) {
 
 	got := NewRouteConfig("test", []*route.VirtualHost{vhost})
 	want := &route.RouteConfiguration{
-		Name:         "test",
-		VirtualHosts: []*route.VirtualHost{vhost},
+		Name:             "test",
+		VirtualHosts:     []*route.VirtualHost{vhost},
+		ValidateClusters: wrapperspb.Bool(true),
 	}
 
 	assert.DeepEqual(t, got, want, protocmp.Transform())