Repository: knative-sandbox/net-kourier
Pull request #338 (2020-11-05 11:10:50)

Do not reconcile endpoints if the ready addresses haven't changed
This should significantly reduce churn produced by endpoint reconciles as we'll no longer reconcile if non ready endpoints are added for example.

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/338#" 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/master/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":[]} -->
codecov[bot]: # [Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/338?src=pr&el=h1) Report
> Merging [#338](https://codecov.io/gh/knative-sandbox/net-kourier/pull/338?src=pr&el=desc) into [master](https://codecov.io/gh/knative-sandbox/net-kourier/commit/397572bee815fab5733bcbafd6133ab62f3cf28d?el=desc) will **not change** coverage.
> The diff coverage is `n/a`.

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

```diff
@@           Coverage Diff           @@
##           master     #338   +/-   ##
=======================================
  Coverage   68.84%   68.84%           
=======================================
  Files          13       13           
  Lines         552      552           
=======================================
  Hits          380      380           
  Misses        143      143           
  Partials       29       29           
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/knative-sandbox/net-kourier/pull/338?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/338?src=pr&el=footer). Last update [397572b...048648e](https://codecov.io/gh/knative-sandbox/net-kourier/pull/338?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/pkg/envoy/api/http_connection_manager.go b/pkg/envoy/api/http_connection_manager.go
index 7cb0ee9e..ac144a93 100644
--- a/pkg/envoy/api/http_connection_manager.go
+++ b/pkg/envoy/api/http_connection_manager.go
@@ -29,7 +29,7 @@ import (
 	"knative.dev/net-kourier/pkg/config"
 )
 
-func NewHTTPConnectionManager(virtualHosts []*route.VirtualHost) httpconnectionmanagerv2.HttpConnectionManager {
+func NewHTTPConnectionManager() *httpconnectionmanagerv2.HttpConnectionManager {
 	var filters []*httpconnectionmanagerv2.HttpFilter
 
 	if config.ExternalAuthz.Enabled {
@@ -42,22 +42,23 @@ func NewHTTPConnectionManager(virtualHosts []*route.VirtualHost) httpconnectionm
 	}
 	filters = append(filters, &routerFilter)
 
-	return httpconnectionmanagerv2.HttpConnectionManager{
-		CodecType:  httpconnectionmanagerv2.HttpConnectionManager_AUTO,
-		StatPrefix: "ingress_http",
-		RouteSpecifier: &httpconnectionmanagerv2.HttpConnectionManager_RouteConfig{
-			RouteConfig: &v2.RouteConfiguration{
-				Name:         "local_route",
-				VirtualHosts: virtualHosts,
-			},
-		},
+	return &httpconnectionmanagerv2.HttpConnectionManager{
+		CodecType:   httpconnectionmanagerv2.HttpConnectionManager_AUTO,
+		StatPrefix:  "ingress_http",
 		HttpFilters: filters,
 		AccessLog:   accessLogs(),
 	}
 }
 
-func NewRDSHTTPConnectionManager(routeConfigName string) httpconnectionmanagerv2.HttpConnectionManager_Rds {
-	return httpconnectionmanagerv2.HttpConnectionManager_Rds{
+func NewRouteConfig(name string, virtualHosts []*route.VirtualHost) *v2.RouteConfiguration {
+	return &v2.RouteConfiguration{
+		Name:         name,
+		VirtualHosts: virtualHosts,
+	}
+}
+
+func NewRDSHTTPConnectionManager(routeConfigName string) *httpconnectionmanagerv2.HttpConnectionManager_Rds {
+	return &httpconnectionmanagerv2.HttpConnectionManager_Rds{
 		Rds: &httpconnectionmanagerv2.Rds{
 			ConfigSource: &envoy_api_v2_core.ConfigSource{
 				ConfigSourceSpecifier: &envoy_api_v2_core.ConfigSource_Ads{
@@ -91,19 +92,21 @@ func accessLogs() []*envoy_accesslog_v2.AccessLog {
 // only the virtual hosts in the route config. It filters the virtual hosts
 // keeping only the ones whose domains contain all the domains received in the
 // params.
-func filterByDomains(connManager *httpconnectionmanagerv2.HttpConnectionManager, domains []string) httpconnectionmanagerv2.HttpConnectionManager {
-	currentVirtualHosts := connManager.GetRouteSpecifier().(*httpconnectionmanagerv2.HttpConnectionManager_RouteConfig).RouteConfig.GetVirtualHosts()
+func filterByDomains(connManager *httpconnectionmanagerv2.HttpConnectionManager, domains []string) *httpconnectionmanagerv2.HttpConnectionManager {
+	rc := connManager.GetRouteSpecifier().(*httpconnectionmanagerv2.HttpConnectionManager_RouteConfig).RouteConfig
 
-	res := NewHTTPConnectionManager(currentVirtualHosts)
+	res := NewHTTPConnectionManager()
 
 	var newVirtualHosts []*route.VirtualHost
-	for _, virtualHost := range currentVirtualHosts {
+	for _, virtualHost := range rc.GetVirtualHosts() {
 		if containsAll(virtualHost.Domains, domains) {
 			newVirtualHosts = append(newVirtualHosts, virtualHost)
 		}
 	}
 
-	res.GetRouteSpecifier().(*httpconnectionmanagerv2.HttpConnectionManager_RouteConfig).RouteConfig.VirtualHosts = newVirtualHosts
+	res.RouteSpecifier = &httpconnectionmanagerv2.HttpConnectionManager_RouteConfig{
+		RouteConfig: NewRouteConfig(rc.Name, newVirtualHosts),
+	}
 
 	return res
 }
diff --git a/pkg/envoy/api/listener.go b/pkg/envoy/api/listener.go
index 02600391..684c1293 100644
--- a/pkg/envoy/api/listener.go
+++ b/pkg/envoy/api/listener.go
@@ -140,7 +140,7 @@ func createFilterChainsForTLS(manager *httpconnmanagerv2.HttpConnectionManager,
 		// matching rules. For each sniMatch, we just need the rules defined for
 		// the same hosts defined in the sniMatch
 		connManager := filterByDomains(manager, sniMatch.hosts)
-		filters, err := createFilters(&connManager)
+		filters, err := createFilters(connManager)
 		if err != nil {
 			return nil, err
 		}
diff --git a/pkg/generator/caches.go b/pkg/generator/caches.go
index cd7e1aaf..5798500c 100644
--- a/pkg/generator/caches.go
+++ b/pkg/generator/caches.go
@@ -41,7 +41,7 @@ type Caches struct {
 	translatedIngresses map[types.NamespacedName]*translatedIngress
 	clusters            *ClustersCache
 	domainsInUse        sets.String
-	routeConfig         []v2.RouteConfiguration
+	routeConfig         []*v2.RouteConfiguration
 	listeners           []*v2.Listener
 	statusVirtualHost   *route.VirtualHost
 
@@ -164,7 +164,7 @@ func (caches *Caches) ToEnvoySnapshot() (cache.Snapshot, error) {
 		// in the Knative serving test suite fails sometimes.
 		// Ref: https://github.com/knative/serving/blob/f6da03e5dfed78593c4f239c3c7d67c5d7c55267/test/conformance/ingress/update_test.go#L37
 		caches.routeConfig[i].ValidateClusters = &wrappers.BoolValue{Value: true}
-		routes[i] = &caches.routeConfig[i]
+		routes[i] = caches.routeConfig[i]
 	}
 
 	listeners := make([]cache.Resource, len(caches.listeners))
diff --git a/pkg/generator/generator.go b/pkg/generator/generator.go
index 2f099128..af8b4051 100644
--- a/pkg/generator/generator.go
+++ b/pkg/generator/generator.go
@@ -63,73 +63,52 @@ func UpdateInfoForIngress(ctx context.Context, caches *Caches, ing *v1alpha1.Ing
 	return caches.UpdateIngress(ctx, ingressTranslation)
 }
 
-func listenersFromVirtualHosts(ctx context.Context, externalVirtualHosts []*route.VirtualHost,
+func listenersFromVirtualHosts(
+	ctx context.Context,
+	externalVirtualHosts []*route.VirtualHost,
 	clusterLocalVirtualHosts []*route.VirtualHost,
 	sniMatches []*envoy.SNIMatch,
-	kubeclient kubeclient.Interface, caches *Caches) ([]*v2.Listener, error) {
-
-	var listeners []*v2.Listener
-
-	externalManager := envoy.NewHTTPConnectionManager(externalVirtualHosts)
-	internalManager := envoy.NewHTTPConnectionManager(clusterLocalVirtualHosts)
-
-	internalRouteConfig := internalManager.GetRouteConfig()
-	externalRouteConfig := externalManager.GetRouteConfig()
-
-	// We need to keep these for the case of HTTPS with SNI routing
-	originalExternalManager := externalManager
-	originalExternalVHosts := externalRouteConfig.VirtualHosts
-
-	// Set proper names so those can be referred later.
-	internalRouteConfig.Name = internalRouteConfigName
-	externalRouteConfig.Name = externalRouteConfigName
-
-	// Now we save the RouteConfigs with the proper name and all the virtualhosts etc.. into the cache.
-	caches.routeConfig = []v2.RouteConfiguration{}
-	caches.routeConfig = append(caches.routeConfig, *externalRouteConfig)
-	caches.routeConfig = append(caches.routeConfig, *internalRouteConfig)
-
-	// Now let's forget about the cache, and override the internal manager to point to the RDS and look for the proper
-	// names.
-	internalRDSHTTPConnectionManager := envoy.NewRDSHTTPConnectionManager(internalRouteConfigName)
-	internalManager.RouteSpecifier = &internalRDSHTTPConnectionManager
-
-	// Set the discovery to ADS
-	externalRDSHTTPConnectionManager := envoy.NewRDSHTTPConnectionManager(externalRouteConfigName)
-	externalManager.RouteSpecifier = &externalRDSHTTPConnectionManager
-
-	// CleanUp virtual hosts.
-	externalRouteConfig.VirtualHosts = []*route.VirtualHost{}
-	internalRouteConfig.VirtualHosts = []*route.VirtualHost{}
-
-	externalHTTPEnvoyListener, err := newExternalHTTPEnvoyListener(&externalManager)
+	kubeclient kubeclient.Interface,
+	caches *Caches) ([]*v2.Listener, error) {
+
+	// First, we save the RouteConfigs with the proper name and all the virtualhosts etc. into the cache.
+	externalRouteConfig := envoy.NewRouteConfig(externalRouteConfigName, externalVirtualHosts)
+	internalRouteConfig := envoy.NewRouteConfig(internalRouteConfigName, clusterLocalVirtualHosts)
+	caches.routeConfig = []*v2.RouteConfiguration{externalRouteConfig, internalRouteConfig}
+
+	// Now we setup connection managers, that reference the routeconfigs via RDS.
+	externalManager := envoy.NewHTTPConnectionManager()
+	internalManager := envoy.NewHTTPConnectionManager()
+	externalManager.RouteSpecifier = envoy.NewRDSHTTPConnectionManager(externalRouteConfig.Name)
+	internalManager.RouteSpecifier = envoy.NewRDSHTTPConnectionManager(internalRouteConfig.Name)
+	externalHTTPEnvoyListener, err := envoy.NewHTTPListener(externalManager, config.HTTPPortExternal)
 	if err != nil {
 		return nil, err
 	}
-	listeners = append(listeners, externalHTTPEnvoyListener)
-
-	internalEnvoyListener, err := newInternalEnvoyListener(&internalManager)
+	internalEnvoyListener, err := envoy.NewHTTPListener(internalManager, config.HTTPPortInternal)
 	if err != nil {
 		return nil, err
 	}
-	listeners = append(listeners, internalEnvoyListener)
+
+	listeners := []*v2.Listener{externalHTTPEnvoyListener, internalEnvoyListener}
 
 	// Configure TLS Listener. If there's at least one ingress that contains the
 	// TLS field, that takes precedence. If there is not, TLS will be configured
 	// using a single cert for all the services if the creds are given via ENV.
 	if len(sniMatches) > 0 {
 		// TODO: Can we make this work with "HttpConnectionManager_Rds"?
-		externalRouteConfig.VirtualHosts = originalExternalVHosts
-		externalHTTPSEnvoyListener, err := newExternalHTTPSEnvoyListener(
-			&originalExternalManager, sniMatches,
-		)
+		sniManager := envoy.NewHTTPConnectionManager()
+		sniManager.RouteSpecifier = &httpconnmanagerv2.HttpConnectionManager_RouteConfig{
+			RouteConfig: externalRouteConfig,
+		}
+		externalHTTPSEnvoyListener, err := newExternalHTTPSEnvoyListener(sniManager, sniMatches)
 		if err != nil {
 			return nil, err
 		}
 		listeners = append(listeners, externalHTTPSEnvoyListener)
 	} else if useHTTPSListenerWithOneCert() {
 		externalHTTPSEnvoyListener, err := newExternalEnvoyListenerWithOneCert(
-			ctx, &externalManager, kubeclient,
+			ctx, externalManager, kubeclient,
 		)
 		if err != nil {
 			return nil, err
@@ -167,14 +146,6 @@ func newExternalEnvoyListenerWithOneCert(ctx context.Context, manager *httpconnm
 	return envoy.NewHTTPSListener(manager, config.HTTPSPortExternal, certificateChain, privateKey)
 }
 
-func newExternalHTTPEnvoyListener(manager *httpconnmanagerv2.HttpConnectionManager) (*v2.Listener, error) {
-	return envoy.NewHTTPListener(manager, config.HTTPPortExternal)
-}
-
-func newInternalEnvoyListener(manager *httpconnmanagerv2.HttpConnectionManager) (*v2.Listener, error) {
-	return envoy.NewHTTPListener(manager, config.HTTPPortInternal)
-}
-
 func newExternalHTTPSEnvoyListener(manager *httpconnmanagerv2.HttpConnectionManager, sniMatches []*envoy.SNIMatch) (*v2.Listener, error) {
 	return envoy.NewHTTPSListenerWithSNI(manager, config.HTTPSPortExternal, sniMatches)
 }
diff --git a/pkg/reconciler/ingress/controller.go b/pkg/reconciler/ingress/controller.go
index edd8b855..de5f7931 100644
--- a/pkg/reconciler/ingress/controller.go
+++ b/pkg/reconciler/ingress/controller.go
@@ -24,6 +24,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/cache"
 	"knative.dev/net-kourier/pkg/config"
 	envoy "knative.dev/net-kourier/pkg/envoy/server"
@@ -192,12 +193,25 @@ func NewController(ctx context.Context, cmw configmap.Watcher) *controller.Impl
 		),
 	))
 
-	endpointsInformer.Informer().AddEventHandler(controller.HandleAll(
-		controller.EnsureTypeMeta(
-			tracker.OnChanged,
-			corev1.SchemeGroupVersion.WithKind("Endpoints"),
-		),
-	))
+	viaTracker := controller.EnsureTypeMeta(
+		tracker.OnChanged,
+		corev1.SchemeGroupVersion.WithKind("Endpoints"))
+	endpointsInformer.Informer().AddEventHandler(cache.ResourceEventHandlerFuncs{
+		AddFunc:    viaTracker,
+		DeleteFunc: viaTracker,
+		UpdateFunc: func(old interface{}, new interface{}) {
+			before := readyAddresses(old.(*corev1.Endpoints))
+			after := readyAddresses(new.(*corev1.Endpoints))
+
+			// If the ready addresses have not changed, there is no reason for us to
+			// reconcile this endpoint, so why bother?
+			if before.Equal(after) {
+				return
+			}
+
+			viaTracker(new)
+		},
+	})
 
 	secretInformer.Informer().AddEventHandler(controller.HandleAll(
 		controller.EnsureTypeMeta(
@@ -238,3 +252,23 @@ func getReadyIngresses(ctx context.Context, knativeClient networkingClientSet.Ne
 	}
 	return ingressesToWarm, nil
 }
+
+func readyAddresses(eps *corev1.Endpoints) sets.String {
+	var count int
+	for _, subset := range eps.Subsets {
+		count += len(subset.Addresses)
+	}
+
+	if count == 0 {
+		return nil
+	}
+
+	ready := make(sets.String, count)
+	for _, subset := range eps.Subsets {
+		for _, address := range subset.Addresses {
+			ready.Insert(address.IP)
+		}
+	}
+
+	return ready
+}


Test patch:
diff --git a/pkg/envoy/api/http_connection_manager_test.go b/pkg/envoy/api/http_connection_manager_test.go
index 82e35a08..407e7a23 100644
--- a/pkg/envoy/api/http_connection_manager_test.go
+++ b/pkg/envoy/api/http_connection_manager_test.go
@@ -19,35 +19,14 @@ package envoy
 import (
 	"testing"
 
-	route "github.com/envoyproxy/go-control-plane/envoy/api/v2/route"
 	accesslog_v2 "github.com/envoyproxy/go-control-plane/envoy/config/accesslog/v2"
 	envoy_config_filter_accesslog_v2 "github.com/envoyproxy/go-control-plane/envoy/config/filter/accesslog/v2"
-	httpconnectionmanagerv2 "github.com/envoyproxy/go-control-plane/envoy/config/filter/network/http_connection_manager/v2"
 	"github.com/golang/protobuf/ptypes"
 	"gotest.tools/assert"
 )
 
-var testVirtualHosts = []*route.VirtualHost{{
-	Name:    "helloworld-go",
-	Domains: []string{"helloworld-go.default.127.0.0.1.nip.io"},
-	Routes: []*route.Route{
-		{
-			Name: "helloworld-route",
-		},
-	},
-}}
-
-func TestCreatesManagerWithVirtualHosts(t *testing.T) {
-	connManager := NewHTTPConnectionManager(testVirtualHosts)
-
-	VirtualHosts := connManager.RouteSpecifier.(*httpconnectionmanagerv2.HttpConnectionManager_RouteConfig).
-		RouteConfig.VirtualHosts
-
-	assert.DeepEqual(t, VirtualHosts, testVirtualHosts)
-}
-
 func TestCreatesManagerThatOutputsToStdOut(t *testing.T) {
-	connManager := NewHTTPConnectionManager(testVirtualHosts)
+	connManager := NewHTTPConnectionManager()
 	accessLog := connManager.AccessLog[0]
 	accessLogPathAny := accessLog.ConfigType.(*envoy_config_filter_accesslog_v2.AccessLog_TypedConfig).TypedConfig
 	fileAccesLog := &accesslog_v2.FileAccessLog{}
diff --git a/pkg/envoy/api/listener_test.go b/pkg/envoy/api/listener_test.go
index f7a7e241..79cba6e6 100644
--- a/pkg/envoy/api/listener_test.go
+++ b/pkg/envoy/api/listener_test.go
@@ -26,7 +26,7 @@ import (
 	core "github.com/envoyproxy/go-control-plane/envoy/api/v2/core"
 	envoy_api_v2_listener "github.com/envoyproxy/go-control-plane/envoy/api/v2/listener"
 	route "github.com/envoyproxy/go-control-plane/envoy/api/v2/route"
-	httpconnectionmanagerv2 "github.com/envoyproxy/go-control-plane/envoy/config/filter/network/http_connection_manager/v2"
+	httpconnmanagerv2 "github.com/envoyproxy/go-control-plane/envoy/config/filter/network/http_connection_manager/v2"
 	"github.com/envoyproxy/go-control-plane/pkg/wellknown"
 	"github.com/golang/protobuf/ptypes"
 	"gotest.tools/assert"
@@ -34,9 +34,9 @@ import (
 )
 
 func TestNewHTTPListener(t *testing.T) {
-	manager := NewHTTPConnectionManager([]*route.VirtualHost{})
+	manager := NewHTTPConnectionManager()
 
-	l, err := NewHTTPListener(&manager, 8080)
+	l, err := NewHTTPListener(manager, 8080)
 	if err != nil {
 		t.Error(err)
 	}
@@ -48,12 +48,12 @@ func TestNewHTTPListener(t *testing.T) {
 }
 
 func TestNewHTTPSListener(t *testing.T) {
-	manager := NewHTTPConnectionManager([]*route.VirtualHost{})
+	manager := NewHTTPConnectionManager()
 
 	certChain := []byte("some_certificate_chain")
 	privateKey := []byte("some_private_key")
 
-	l, err := NewHTTPSListener(&manager, 8081, certChain, privateKey)
+	l, err := NewHTTPSListener(manager, 8081, certChain, privateKey)
 	if err != nil {
 		t.Error(err)
 	}
@@ -87,9 +87,12 @@ func TestNewHTTPSListenerWithSNI(t *testing.T) {
 		"vHost2", []string{"another_host.com", "another_host.com:*"}, []*route.Route{},
 	)
 
-	manager := NewHTTPConnectionManager([]*route.VirtualHost{vHost1, vHost2})
+	manager := NewHTTPConnectionManager()
+	manager.RouteSpecifier = &httpconnmanagerv2.HttpConnectionManager_RouteConfig{
+		RouteConfig: NewRouteConfig("test", []*route.VirtualHost{vHost1, vHost2}),
+	}
 
-	listener, err := NewHTTPSListenerWithSNI(&manager, 8443, sniMatches)
+	listener, err := NewHTTPSListenerWithSNI(manager, 8443, sniMatches)
 	if err != nil {
 		t.Error(err)
 	}
@@ -159,14 +162,14 @@ func getTLSCreds(filterChain *envoy_api_v2_listener.FilterChain) (certChain []by
 
 // Note: Returns an error when there are multiple virtual hosts configured
 func getVHostDomains(filterChain *envoy_api_v2_listener.FilterChain) ([]string, error) {
-	connManager := httpconnectionmanagerv2.HttpConnectionManager{}
+	connManager := httpconnmanagerv2.HttpConnectionManager{}
 	err := ptypes.UnmarshalAny(filterChain.Filters[0].GetTypedConfig(), &connManager)
 
 	if err != nil {
 		return nil, err
 	}
 
-	routeConfig := connManager.GetRouteSpecifier().(*httpconnectionmanagerv2.HttpConnectionManager_RouteConfig).RouteConfig
+	routeConfig := connManager.GetRouteSpecifier().(*httpconnmanagerv2.HttpConnectionManager_RouteConfig).RouteConfig
 
 	if len(routeConfig.VirtualHosts) > 1 {
 		return nil, fmt.Errorf("more than one virtual host configured")
diff --git a/pkg/generator/caches_test.go b/pkg/generator/caches_test.go
index 637a34fb..5abd991b 100644
--- a/pkg/generator/caches_test.go
+++ b/pkg/generator/caches_test.go
@@ -211,7 +211,7 @@ func TestValidateIngress(t *testing.T) {
 	assert.Error(t, err, ErrDomainConflict.Error())
 }
 
-func getVHostsNames(routeConfigs []v2.RouteConfiguration) []string {
+func getVHostsNames(routeConfigs []*v2.RouteConfiguration) []string {
 	var res []string
 
 	for _, routeConfig := range routeConfigs {