From c7c1285d5e503613eb54c33006a8c7cf54a46aab Mon Sep 17 00:00:00 2001 From: norbjd Date: Sun, 23 Jun 2024 17:35:52 +0200 Subject: [PATCH 1/5] Add RetryPolicy on VirtualHost for transient connection issues --- pkg/envoy/api/virtual_host.go | 50 ++++++++++++++---------- pkg/envoy/api/virtual_host_test.go | 24 +++++++++++- pkg/generator/ingress_translator.go | 20 +++++----- pkg/generator/ingress_translator_test.go | 23 ++++++++++- 4 files changed, 85 insertions(+), 32 deletions(-) diff --git a/pkg/envoy/api/virtual_host.go b/pkg/envoy/api/virtual_host.go index c6e483a74..0730045c6 100644 --- a/pkg/envoy/api/virtual_host.go +++ b/pkg/envoy/api/virtual_host.go @@ -21,39 +21,47 @@ import ( extAuthService "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/http/ext_authz/v3" "github.com/envoyproxy/go-control-plane/pkg/wellknown" "google.golang.org/protobuf/types/known/anypb" + "google.golang.org/protobuf/types/known/wrapperspb" ) +type VirtualHostOption func(*route.VirtualHost) + // NewVirtualHost creates a new VirtualHost. -func NewVirtualHost(name string, domains []string, routes []*route.Route) *route.VirtualHost { - return &route.VirtualHost{ +func NewVirtualHost(name string, domains []string, routes []*route.Route, options ...VirtualHostOption) *route.VirtualHost { + virtualHost := &route.VirtualHost{ Name: name, Domains: domains, Routes: routes, } + + for _, opt := range options { + opt(virtualHost) + } + + return virtualHost } -// NewVirtualHostWithExtAuthz creates a new VirtualHost with ExtAuthz settings. -func NewVirtualHostWithExtAuthz( - name string, - contextExtensions map[string]string, - domains []string, - routes []*route.Route) *route.VirtualHost { - - filter, _ := anypb.New(&extAuthService.ExtAuthzPerRoute{ - Override: &extAuthService.ExtAuthzPerRoute_CheckSettings{ - CheckSettings: &extAuthService.CheckSettings{ - ContextExtensions: contextExtensions, +func WithExtAuthz(contextExtensions map[string]string) VirtualHostOption { + return func(virtualHost *route.VirtualHost) { + filter, _ := anypb.New(&extAuthService.ExtAuthzPerRoute{ + Override: &extAuthService.ExtAuthzPerRoute_CheckSettings{ + CheckSettings: &extAuthService.CheckSettings{ + ContextExtensions: contextExtensions, + }, }, - }, - }) + }) - return &route.VirtualHost{ - Name: name, - Domains: domains, - Routes: routes, - TypedPerFilterConfig: map[string]*anypb.Any{ + virtualHost.TypedPerFilterConfig = map[string]*anypb.Any{ wellknown.HTTPExternalAuthorization: filter, - }, + } } +} +func WithRetryOnTransientUpstreamFailure() VirtualHostOption { + return func(virtualHost *route.VirtualHost) { + virtualHost.RetryPolicy = &route.RetryPolicy{ + RetryOn: "reset,connect-failure", + NumRetries: wrapperspb.UInt32(1), + } + } } diff --git a/pkg/envoy/api/virtual_host_test.go b/pkg/envoy/api/virtual_host_test.go index e8c10ee94..88e66ac1c 100644 --- a/pkg/envoy/api/virtual_host_test.go +++ b/pkg/envoy/api/virtual_host_test.go @@ -45,7 +45,7 @@ func TestVirtualHostWithExtAuthz(t *testing.T) { domains := []string{"foo", "bar"} routes := []*route.Route{{Name: "baz"}} - got := NewVirtualHostWithExtAuthz(name, nil, domains, routes) + got := NewVirtualHost(name, domains, routes, route.WithExtAuthz(nil)) want := &route.VirtualHost{ Name: name, Domains: domains, @@ -57,3 +57,25 @@ func TestVirtualHostWithExtAuthz(t *testing.T) { assert.DeepEqual(t, got.Routes, want.Routes, protocmp.Transform()) assert.Assert(t, got.TypedPerFilterConfig[wellknown.HTTPExternalAuthorization] != nil) } + +func TestVirtualHostWithRetryOnTransientUpstreamFailure(t *testing.T) { + name := "test" + domains := []string{"foo", "bar"} + routes := []*route.Route{{Name: "baz"}} + + got := NewVirtualHost(name, domains, routes, route.WithRetryOnTransientUpstreamFailure()) + want := &route.VirtualHost{ + Name: name, + Domains: domains, + Routes: routes, + RetryPolicy: &route.RetryPolicy{ + RetryOn: "reset,connect-failure", + NumRetries: wrapperspb.UInt32(1), + }, + } + + assert.Equal(t, got.Name, want.Name) + assert.DeepEqual(t, got.Domains, want.Domains) + assert.DeepEqual(t, got.Routes, want.Routes, protocmp.Transform()) + assert.DeepEqual(t, got.RetryPolicy, want.RetryPolicy, protocmp.Transform()) +} diff --git a/pkg/generator/ingress_translator.go b/pkg/generator/ingress_translator.go index a14b48a3f..5d0eba8fa 100644 --- a/pkg/generator/ingress_translator.go +++ b/pkg/generator/ingress_translator.go @@ -255,20 +255,22 @@ func (translator *IngressTranslator) translateIngress(ctx context.Context, ingre } var virtualHost, virtualTLSHost *route.VirtualHost + // TODO(norbjd): do we want to enable this by default? + virtualHostOptions := []route.VirtualHostOption{ + envoy.WithRetryOnTransientUpstreamFailure(), + } + if extAuthzEnabled { contextExtensions := kmeta.UnionMaps(map[string]string{ "client": "kourier", "visibility": string(rule.Visibility), }, ingress.GetLabels()) - virtualHost = envoy.NewVirtualHostWithExtAuthz(ruleName, contextExtensions, domainsForRule(rule), routes) - if len(tlsRoutes) != 0 { - virtualTLSHost = envoy.NewVirtualHostWithExtAuthz(ruleName, contextExtensions, domainsForRule(rule), tlsRoutes) - } - } else { - virtualHost = envoy.NewVirtualHost(ruleName, domainsForRule(rule), routes) - if len(tlsRoutes) != 0 { - virtualTLSHost = envoy.NewVirtualHost(ruleName, domainsForRule(rule), tlsRoutes) - } + virtualHostOptions = append(virtualHostOptions, envoy.WithExtAuthz(contextExtensions)) + } + + virtualHost = envoy.NewVirtualHost(ruleName, domainsForRule(rule), routes, virtualHostOptions...) + if len(tlsRoutes) != 0 { + virtualTLSHost = envoy.NewVirtualHost(ruleName, domainsForRule(rule), tlsRoutes, virtualHostOptions...) } localHosts = append(localHosts, virtualHost) diff --git a/pkg/generator/ingress_translator_test.go b/pkg/generator/ingress_translator_test.go index 065db4160..8054a3f04 100644 --- a/pkg/generator/ingress_translator_test.go +++ b/pkg/generator/ingress_translator_test.go @@ -87,6 +87,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -152,6 +153,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -227,6 +229,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -301,6 +304,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } vHostsRedirect := []*route.VirtualHost{ @@ -321,6 +325,7 @@ func TestIngressTranslator(t *testing.T) { }}, "/test"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -397,6 +402,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -520,6 +526,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -596,6 +603,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -656,6 +664,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -717,6 +726,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -854,6 +864,7 @@ func TestIngressTranslatorWithHTTPOptionDisabled(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } return &translatedIngress{ @@ -929,6 +940,7 @@ func TestIngressTranslatorWithHTTPOptionDisabled(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -1041,6 +1053,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -1115,6 +1128,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -1190,6 +1204,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -1265,6 +1280,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -1331,6 +1347,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -1401,6 +1418,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, + route.WithRetryOnTransientUpstreamFailure(), ), } @@ -1525,7 +1543,7 @@ func TestIngressTranslatorHTTP01Challenge(t *testing.T) { }, want: func() *translatedIngress { vHosts := []*route.VirtualHost{ - envoy.NewVirtualHostWithExtAuthz( + envoy.NewVirtualHost( "(simplens/simplename).Rules[0]", map[string]string{"client": "kourier", "visibility": "ExternalIP"}, []string{"foo.example.com", "foo.example.com:*"}, @@ -1540,6 +1558,8 @@ func TestIngressTranslatorHTTP01Challenge(t *testing.T) { nil, ""), }, + route.WithRetryOnTransientUpstreamFailure(), + route.WithExtAuthz(map[string]string{"client": "kourier", "visibility": "ExternalIP"}) ), } @@ -1652,6 +1672,7 @@ func TestIngressTranslatorDomainMappingDisableHTTP2(t *testing.T) { map[string]string{"foo": "bar"}, "bar.default.svc.cluster.local"), }, + route.WithRetryOnTransientUpstreamFailure(), ), } From 905f86e5cddea7f1a319d11985787b099a2d821e Mon Sep 17 00:00:00 2001 From: norbjd Date: Sun, 23 Jun 2024 18:26:37 +0200 Subject: [PATCH 2/5] Fix build issues --- pkg/envoy/api/virtual_host_test.go | 5 +++-- pkg/generator/ingress_translator.go | 2 +- pkg/generator/ingress_translator_test.go | 2 +- 3 files changed, 5 insertions(+), 4 deletions(-) diff --git a/pkg/envoy/api/virtual_host_test.go b/pkg/envoy/api/virtual_host_test.go index 88e66ac1c..79b2e75e4 100644 --- a/pkg/envoy/api/virtual_host_test.go +++ b/pkg/envoy/api/virtual_host_test.go @@ -22,6 +22,7 @@ import ( route "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" "github.com/envoyproxy/go-control-plane/pkg/wellknown" "google.golang.org/protobuf/testing/protocmp" + "google.golang.org/protobuf/types/known/wrapperspb" "gotest.tools/v3/assert" ) @@ -45,7 +46,7 @@ func TestVirtualHostWithExtAuthz(t *testing.T) { domains := []string{"foo", "bar"} routes := []*route.Route{{Name: "baz"}} - got := NewVirtualHost(name, domains, routes, route.WithExtAuthz(nil)) + got := NewVirtualHost(name, domains, routes, WithExtAuthz(nil)) want := &route.VirtualHost{ Name: name, Domains: domains, @@ -63,7 +64,7 @@ func TestVirtualHostWithRetryOnTransientUpstreamFailure(t *testing.T) { domains := []string{"foo", "bar"} routes := []*route.Route{{Name: "baz"}} - got := NewVirtualHost(name, domains, routes, route.WithRetryOnTransientUpstreamFailure()) + got := NewVirtualHost(name, domains, routes, WithRetryOnTransientUpstreamFailure()) want := &route.VirtualHost{ Name: name, Domains: domains, diff --git a/pkg/generator/ingress_translator.go b/pkg/generator/ingress_translator.go index 5d0eba8fa..b41cc8c57 100644 --- a/pkg/generator/ingress_translator.go +++ b/pkg/generator/ingress_translator.go @@ -256,7 +256,7 @@ func (translator *IngressTranslator) translateIngress(ctx context.Context, ingre var virtualHost, virtualTLSHost *route.VirtualHost // TODO(norbjd): do we want to enable this by default? - virtualHostOptions := []route.VirtualHostOption{ + virtualHostOptions := []envoy.VirtualHostOption{ envoy.WithRetryOnTransientUpstreamFailure(), } diff --git a/pkg/generator/ingress_translator_test.go b/pkg/generator/ingress_translator_test.go index 8054a3f04..4f12a3b80 100644 --- a/pkg/generator/ingress_translator_test.go +++ b/pkg/generator/ingress_translator_test.go @@ -1559,7 +1559,7 @@ func TestIngressTranslatorHTTP01Challenge(t *testing.T) { ""), }, route.WithRetryOnTransientUpstreamFailure(), - route.WithExtAuthz(map[string]string{"client": "kourier", "visibility": "ExternalIP"}) + route.WithExtAuthz(map[string]string{"client": "kourier", "visibility": "ExternalIP"}), ), } From cadcd1431d8943dec568a5a5fcce95404aa51b8d Mon Sep 17 00:00:00 2001 From: norbjd Date: Sun, 23 Jun 2024 18:28:46 +0200 Subject: [PATCH 3/5] Fix tests build issues --- pkg/generator/ingress_translator_test.go | 42 ++++++++++++------------ 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/pkg/generator/ingress_translator_test.go b/pkg/generator/ingress_translator_test.go index 4f12a3b80..5e84efb68 100644 --- a/pkg/generator/ingress_translator_test.go +++ b/pkg/generator/ingress_translator_test.go @@ -87,7 +87,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -153,7 +153,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -229,7 +229,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -304,7 +304,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } vHostsRedirect := []*route.VirtualHost{ @@ -325,7 +325,7 @@ func TestIngressTranslator(t *testing.T) { }}, "/test"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -402,7 +402,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -526,7 +526,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -603,7 +603,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -664,7 +664,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -726,7 +726,7 @@ func TestIngressTranslator(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -864,7 +864,7 @@ func TestIngressTranslatorWithHTTPOptionDisabled(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } return &translatedIngress{ @@ -940,7 +940,7 @@ func TestIngressTranslatorWithHTTPOptionDisabled(t *testing.T) { map[string]string{"foo": "bar"}, "rewritten.example.com"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -1053,7 +1053,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -1128,7 +1128,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -1204,7 +1204,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -1280,7 +1280,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -1347,7 +1347,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -1418,7 +1418,7 @@ func TestIngressTranslatorWithUpstreamTLS(t *testing.T) { map[string]string{"foo": "bar"}, ""), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } @@ -1558,8 +1558,8 @@ func TestIngressTranslatorHTTP01Challenge(t *testing.T) { nil, ""), }, - route.WithRetryOnTransientUpstreamFailure(), - route.WithExtAuthz(map[string]string{"client": "kourier", "visibility": "ExternalIP"}), + envoy.WithRetryOnTransientUpstreamFailure(), + envoy.WithExtAuthz(map[string]string{"client": "kourier", "visibility": "ExternalIP"}), ), } @@ -1672,7 +1672,7 @@ func TestIngressTranslatorDomainMappingDisableHTTP2(t *testing.T) { map[string]string{"foo": "bar"}, "bar.default.svc.cluster.local"), }, - route.WithRetryOnTransientUpstreamFailure(), + envoy.WithRetryOnTransientUpstreamFailure(), ), } From 711cb36d6709edc92260e75179e3e3b58a35c50e Mon Sep 17 00:00:00 2001 From: norbjd Date: Sun, 23 Jun 2024 18:49:01 +0200 Subject: [PATCH 4/5] Forgot to remove a parameter --- pkg/generator/ingress_translator_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/generator/ingress_translator_test.go b/pkg/generator/ingress_translator_test.go index 5e84efb68..71c417aca 100644 --- a/pkg/generator/ingress_translator_test.go +++ b/pkg/generator/ingress_translator_test.go @@ -1545,7 +1545,6 @@ func TestIngressTranslatorHTTP01Challenge(t *testing.T) { vHosts := []*route.VirtualHost{ envoy.NewVirtualHost( "(simplens/simplename).Rules[0]", - map[string]string{"client": "kourier", "visibility": "ExternalIP"}, []string{"foo.example.com", "foo.example.com:*"}, []*route.Route{envoy.NewRouteExtAuthzDisabled( "(simplens/simplename).Rules[0].Paths[/.well-known/acme-challenge/-VwB1vAXWaN6mVl3-6JVFTEvf7acguaFDUxsP9UzRkE]", From c48df1475ae70caef246b9e1a9994231c3507580 Mon Sep 17 00:00:00 2001 From: norbjd Date: Sun, 23 Jun 2024 19:33:52 +0200 Subject: [PATCH 5/5] Lint --- pkg/envoy/api/virtual_host_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/envoy/api/virtual_host_test.go b/pkg/envoy/api/virtual_host_test.go index 79b2e75e4..8d959997a 100644 --- a/pkg/envoy/api/virtual_host_test.go +++ b/pkg/envoy/api/virtual_host_test.go @@ -66,9 +66,9 @@ func TestVirtualHostWithRetryOnTransientUpstreamFailure(t *testing.T) { got := NewVirtualHost(name, domains, routes, WithRetryOnTransientUpstreamFailure()) want := &route.VirtualHost{ - Name: name, - Domains: domains, - Routes: routes, + Name: name, + Domains: domains, + Routes: routes, RetryPolicy: &route.RetryPolicy{ RetryOn: "reset,connect-failure", NumRetries: wrapperspb.UInt32(1),