From 18a28c449b8c06adf5b250dda86180deffbb2bdf Mon Sep 17 00:00:00 2001
From: Ciara Stacke <c.stacke@f5.com>
Date: Thu, 18 Nov 2021 16:01:42 +0000
Subject: [PATCH] chore: add a warning for gRPC conflicting error page

---
 .../version2/nginx-plus.virtualserver.tmpl    |  16 +--
 .../configs/version2/nginx.virtualserver.tmpl |  18 +--
 internal/configs/virtualserver.go             |  72 ++++++++--
 internal/configs/virtualserver_test.go        | 135 ++++++++++++++++++
 .../virtual-server-updated.yaml               |  28 ++++
 tests/suite/test_virtual_server_mixed_grpc.py |  22 ++-
 6 files changed, 262 insertions(+), 29 deletions(-)
 create mode 100644 tests/data/virtual-server-grpc-mixed/virtual-server-updated.yaml

diff --git a/internal/configs/version2/nginx-plus.virtualserver.tmpl b/internal/configs/version2/nginx-plus.virtualserver.tmpl
index 83a5e286ec..bde2936c63 100644
--- a/internal/configs/version2/nginx-plus.virtualserver.tmpl
+++ b/internal/configs/version2/nginx-plus.virtualserver.tmpl
@@ -340,14 +340,6 @@ server {
             {{ end }}
         {{ end }}
 
-        {{ range $e := $l.ErrorPages }}
-        error_page {{ $e.Codes }} {{ if ne 0 $e.ResponseCode }}={{ $e.ResponseCode }}{{ end }} "{{ $e.Name }}";
-        {{ end }}
-
-        {{ if $l.ProxyInterceptErrors }}
-        {{ $proxyOrGRPC }}_intercept_errors on;
-        {{ end }}
-
         {{ if $l.InternalProxyPass }}
         proxy_pass {{ $l.InternalProxyPass }};
         {{ end }}
@@ -373,6 +365,10 @@ server {
         {{ $proxyOrGRPC }}_buffer_size {{ $l.ProxyBufferSize }};
             {{ end }}
         
+        {{ if or $l.GRPCPass $l.ProxyInterceptErrors }}
+        {{ $proxyOrGRPC }}_intercept_errors on;
+        {{ end }}
+
             {{ if $l.GRPCPass }}
         error_page 400 = @grpc_internal;
         error_page 401 = @grpc_unauthenticated;
@@ -394,6 +390,10 @@ server {
         error_page 500 = @grpc_internal;
         error_page 501 = @grpc_internal;
             {{ end }}
+        
+        {{ range $e := $l.ErrorPages }}
+        error_page {{ $e.Codes }} {{ if ne 0 $e.ResponseCode }}={{ $e.ResponseCode }}{{ end }} "{{ $e.Name }}";
+        {{ end }}
 
             {{ if not $l.GRPCPass }}
         proxy_http_version 1.1;
diff --git a/internal/configs/version2/nginx.virtualserver.tmpl b/internal/configs/version2/nginx.virtualserver.tmpl
index a004d37228..6c4c933cfa 100644
--- a/internal/configs/version2/nginx.virtualserver.tmpl
+++ b/internal/configs/version2/nginx.virtualserver.tmpl
@@ -244,14 +244,6 @@ server {
         {{ $proxyOrGRPC }}_ssl_name {{ .SSLName }};
         {{ end }}
 
-        {{ range $e := $l.ErrorPages }}
-        error_page {{ $e.Codes }} {{ if ne 0 $e.ResponseCode }}={{ $e.ResponseCode }}{{ end }} "{{ $e.Name }}";
-        {{ end }}
-
-        {{ if $l.ProxyInterceptErrors }}
-        {{ $proxyOrGRPC }}_intercept_errors on;
-        {{ end }}
-
         {{ if $l.InternalProxyPass }}
         proxy_pass {{ $l.InternalProxyPass }};
         {{ end }}
@@ -276,7 +268,11 @@ server {
             {{ if $l.ProxyBufferSize }}
         {{ $proxyOrGRPC }}_buffer_size {{ $l.ProxyBufferSize }};
             {{ end }}
-        
+
+        {{ if or $l.GRPCPass $l.ProxyInterceptErrors }}
+        {{ $proxyOrGRPC }}_intercept_errors on;
+        {{ end }}
+
             {{ if $l.GRPCPass }}
         error_page 400 = @grpc_internal;
         error_page 401 = @grpc_unauthenticated;
@@ -298,6 +294,10 @@ server {
         error_page 500 = @grpc_internal;
         error_page 501 = @grpc_internal;
             {{ end }}
+        
+        {{ range $e := $l.ErrorPages }}
+        error_page {{ $e.Codes }} {{ if ne 0 $e.ResponseCode }}={{ $e.ResponseCode }}{{ end }} "{{ $e.Name }}";
+        {{ end }}
 
             {{ if not $l.GRPCPass }}
         proxy_http_version 1.1;
diff --git a/internal/configs/virtualserver.go b/internal/configs/virtualserver.go
index b688901af8..2df0d21dc7 100644
--- a/internal/configs/virtualserver.go
+++ b/internal/configs/virtualserver.go
@@ -27,6 +27,28 @@ const (
 	subRouteContext        = "subroute"
 )
 
+var grpcErrorList = map[int]bool{
+	400: true,
+	401: true,
+	403: true,
+	404: true,
+	405: true,
+	408: true,
+	413: true,
+	414: true,
+	415: true,
+	426: true,
+	429: true,
+	495: true,
+	496: true,
+	497: true,
+	500: true,
+	501: true,
+	502: true,
+	503: true,
+	504: true,
+}
+
 var incompatibleLBMethodsForSlowStart = map[string]bool{
 	"random":                          true,
 	"ip_hash":                         true,
@@ -125,6 +147,10 @@ func newUpstreamNamerForTransportServer(transportServer *conf_v1alpha1.Transport
 }
 
 func (namer *upstreamNamer) GetNameForUpstreamFromAction(action *conf_v1.Action) string {
+	if action == nil {
+		return ""
+	}
+
 	var upstream string
 	if action.Proxy != nil && action.Proxy.Upstream != "" {
 		upstream = action.Proxy.Upstream
@@ -372,11 +398,18 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig(vsEx *VirtualS
 	matchesRoutes := 0
 
 	variableNamer := newVariableNamer(vsEx.VirtualServer)
+	upstreamNamer := newUpstreamNamerForVirtualServer(vsEx.VirtualServer)
 
 	// generates config for VirtualServer routes
 	for _, r := range vsEx.VirtualServer.Spec.Routes {
+		errorPages := r.ErrorPages
+		upstreamName := upstreamNamer.GetNameForUpstreamFromAction(r.Action)
+		if upstreamName != "" && r.ErrorPages != nil {
+			upstream := crUpstreams[upstreamName]
+			errorPages = checkGrpcErrorPageCodes(r.ErrorPages, isGRPC(upstream.Type), upstreamName, vsc, vsEx)
+		}
 		errorPageIndex := len(errorPageLocations)
-		errorPageLocations = append(errorPageLocations, generateErrorPageLocations(errorPageIndex, r.ErrorPages)...)
+		errorPageLocations = append(errorPageLocations, generateErrorPageLocations(errorPageIndex, errorPages)...)
 
 		// ignore routes that reference VirtualServerRoute
 		if r.Route != "" {
@@ -391,8 +424,8 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig(vsEx *VirtualS
 			}
 
 			// store route error pages and route index for the referenced VirtualServerRoute in case they don't define their own
-			if len(r.ErrorPages) > 0 {
-				vsrErrorPagesFromVs[name] = r.ErrorPages
+			if len(errorPages) > 0 {
+				vsrErrorPagesFromVs[name] = errorPages
 				vsrErrorPagesRouteIndex[name] = errorPageIndex
 			}
 
@@ -426,7 +459,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig(vsEx *VirtualS
 				matchesRoutes,
 				len(splitClients),
 				vsc.cfgParams,
-				r.ErrorPages,
+				errorPages,
 				errorPageIndex,
 				vsLocSnippets,
 				vsc.enableSnippets,
@@ -444,7 +477,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig(vsEx *VirtualS
 			matchesRoutes++
 		} else if len(r.Splits) > 0 {
 			cfg := generateDefaultSplitsConfig(r, virtualServerUpstreamNamer, crUpstreams, variableNamer, len(splitClients),
-				vsc.cfgParams, r.ErrorPages, errorPageIndex, r.Path, vsLocSnippets, vsc.enableSnippets, len(returnLocations), isVSR, "", "")
+				vsc.cfgParams, errorPages, errorPageIndex, r.Path, vsLocSnippets, vsc.enableSnippets, len(returnLocations), isVSR, "", "")
 			addPoliciesCfgToLocations(routePoliciesCfg, cfg.Locations)
 
 			splitClients = append(splitClients, cfg.SplitClients...)
@@ -457,7 +490,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig(vsEx *VirtualS
 
 			proxySSLName := generateProxySSLName(upstream.Service, vsEx.VirtualServer.Namespace)
 
-			loc, returnLoc := generateLocation(r.Path, upstreamName, upstream, r.Action, vsc.cfgParams, r.ErrorPages, false,
+			loc, returnLoc := generateLocation(r.Path, upstreamName, upstream, r.Action, vsc.cfgParams, errorPages, false,
 				errorPageIndex, proxySSLName, r.Path, vsLocSnippets, vsc.enableSnippets, len(returnLocations), isVSR, "", "")
 			addPoliciesCfgToLocation(routePoliciesCfg, &loc)
 
@@ -473,12 +506,17 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig(vsEx *VirtualS
 		isVSR := true
 		upstreamNamer := newUpstreamNamerForVirtualServerRoute(vsEx.VirtualServer, vsr)
 		for _, r := range vsr.Spec.Subroutes {
-			errorPageIndex := len(errorPageLocations)
-			errorPageLocations = append(errorPageLocations, generateErrorPageLocations(errorPageIndex, r.ErrorPages)...)
 			errorPages := r.ErrorPages
+			upstreamName := upstreamNamer.GetNameForUpstreamFromAction(r.Action)
+			if upstreamName != "" && r.ErrorPages != nil {
+				upstream := crUpstreams[upstreamName]
+				errorPages = checkGrpcErrorPageCodes(r.ErrorPages, isGRPC(upstream.Type), upstreamName, vsc, vsEx)
+			}
+			errorPageIndex := len(errorPageLocations)
+			errorPageLocations = append(errorPageLocations, generateErrorPageLocations(errorPageIndex, errorPages)...)
 			vsrNamespaceName := fmt.Sprintf("%v/%v", vsr.Namespace, vsr.Name)
 			// use the VirtualServer error pages if the route does not define any
-			if r.ErrorPages == nil {
+			if errorPages == nil {
 				if vsErrorPages, ok := vsrErrorPagesFromVs[vsrNamespaceName]; ok {
 					errorPages = vsErrorPages
 					errorPageIndex = vsrErrorPagesRouteIndex[vsrNamespaceName]
@@ -2179,6 +2217,22 @@ func generateErrorPageName(errPageIndex int, index int) string {
 	return fmt.Sprintf("@error_page_%v_%v", errPageIndex, index)
 }
 
+func checkGrpcErrorPageCodes(errorPages []conf_v1.ErrorPage, isGRPC bool, uName string, vsc *virtualServerConfigurator, vsEx *VirtualServerEx) []conf_v1.ErrorPage {
+	var c []int
+	for _, e := range errorPages {
+		for _, code := range e.Codes {
+			if isGRPC && grpcErrorList[code] {
+				c = append(c, code)
+			}
+		}
+	}
+	if len(c) > 0 {
+		vsc.addWarningf(
+			vsEx.VirtualServer, "The error page configuration for the upstream %s is ignored for status code(s) %v, which cannot be used for GRPC upstreams.", uName, c)
+	}
+	return errorPages
+}
+
 func generateErrorPageCodes(codes []int) string {
 	var c []string
 	for _, code := range codes {
diff --git a/internal/configs/virtualserver_test.go b/internal/configs/virtualserver_test.go
index 6c40b631b9..adf11742c6 100644
--- a/internal/configs/virtualserver_test.go
+++ b/internal/configs/virtualserver_test.go
@@ -657,6 +657,141 @@ func TestGenerateVirtualServerConfig(t *testing.T) {
 	}
 }
 
+func TestGenerateVirtualServerConfigGrpcErrorPageWarning(t *testing.T) {
+	virtualServerEx := VirtualServerEx{
+		VirtualServer: &conf_v1.VirtualServer{
+			ObjectMeta: meta_v1.ObjectMeta{
+				Name:      "cafe",
+				Namespace: "default",
+			},
+			Spec: conf_v1.VirtualServerSpec{
+				Host: "cafe.example.com",
+				Upstreams: []conf_v1.Upstream{
+					{
+						Name:    "grpc",
+						Service: "grpc-svc",
+						Port:    50051,
+						Type:    "grpc",
+						TLS: conf_v1.UpstreamTLS{
+							Enable: true,
+						},
+					},
+				},
+				Routes: []conf_v1.Route{
+					{
+						Path: "/grpc-errorpage",
+						Action: &conf_v1.Action{
+							Pass: "grpc",
+						},
+						ErrorPages: []conf_v1.ErrorPage{
+							{
+								Codes: []int{404, 405},
+								Return: &conf_v1.ErrorPageReturn{
+									ActionReturn: conf_v1.ActionReturn{
+										Code: 200,
+										Type: "text/plain",
+										Body: "All Good",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		Endpoints: map[string][]string{
+			"default/grpc-svc:50051": {
+				"10.0.0.20:80",
+			},
+		},
+	}
+
+	baseCfgParams := ConfigParams{
+		ServerTokens:    "off",
+		Keepalive:       16,
+		ServerSnippets:  []string{"# server snippet"},
+		ProxyProtocol:   true,
+		SetRealIPFrom:   []string{"0.0.0.0/0"},
+		RealIPHeader:    "X-Real-IP",
+		RealIPRecursive: true,
+		HTTP2:           true,
+	}
+
+	expected := version2.VirtualServerConfig{
+		Upstreams: []version2.Upstream{
+			{
+				UpstreamLabels: version2.UpstreamLabels{
+					Service:           "grpc-svc",
+					ResourceType:      "virtualserver",
+					ResourceName:      "cafe",
+					ResourceNamespace: "default",
+				},
+				Name: "vs_default_cafe_grpc",
+				Servers: []version2.UpstreamServer{
+					{
+						Address: "10.0.0.20:80",
+					},
+				},
+				Keepalive: 16,
+			},
+		},
+		HTTPSnippets:  []string{},
+		LimitReqZones: []version2.LimitReqZone{},
+		Server: version2.Server{
+			ServerName:      "cafe.example.com",
+			StatusZone:      "cafe.example.com",
+			VSNamespace:     "default",
+			VSName:          "cafe",
+			ProxyProtocol:   true,
+			ServerTokens:    "off",
+			SetRealIPFrom:   []string{"0.0.0.0/0"},
+			RealIPHeader:    "X-Real-IP",
+			RealIPRecursive: true,
+			Snippets:        []string{"# server snippet"},
+			Locations: []version2.Location{
+				{
+					Path:                     "/grpc-errorpage",
+					ProxyPass:                "https://vs_default_cafe_grpc",
+					ProxyNextUpstream:        "error timeout",
+					ProxyNextUpstreamTimeout: "0s",
+					ProxyNextUpstreamTries:   0,
+					HasKeepalive:             true,
+					ErrorPages:               []version2.ErrorPage{{Name: "@error_page_0_0", Codes: "404 405", ResponseCode: 200}},
+					ProxyInterceptErrors:     true,
+					ProxySSLName:             "grpc-svc.default.svc",
+					ProxyPassRequestHeaders:  true,
+					ProxySetHeaders:          []version2.Header{{Name: "Host", Value: "$host"}},
+					ServiceName:              "grpc-svc",
+					GRPCPass:                 "grpcs://vs_default_cafe_grpc",
+				},
+			},
+			ErrorPageLocations: []version2.ErrorPageLocation{
+				{
+					Name:        "@error_page_0_0",
+					DefaultType: "text/plain",
+					Return:      &version2.Return{Text: "All Good"},
+				},
+			},
+		},
+	}
+
+	isPlus := false
+	isResolverConfigured := false
+	isWildcardEnabled := false
+	vsc := newVirtualServerConfigurator(&baseCfgParams, isPlus, isResolverConfigured, &StaticConfigParams{}, isWildcardEnabled)
+
+	result, warnings := vsc.GenerateVirtualServerConfig(&virtualServerEx, nil)
+	if diff := cmp.Diff(expected, result); diff != "" {
+		t.Errorf("TestGenerateVirtualServerConfigGrpcErrorPageWarning() mismatch (-want +got):\n%s", diff)
+	}
+
+	if len(warnings) < 1 {
+		t.Errorf("TestGenerateVirtualServerConfigGrpcErrorPageWarning did not return warnings when one was expected")
+	} else if len(warnings) > 1 {
+		t.Errorf("TestGenerateVirtualServerConfigGrpcErrorPageWarning generated too many warnings: %v", warnings)
+	}
+}
+
 func TestGenerateVirtualServerConfigWithSpiffeCerts(t *testing.T) {
 	virtualServerEx := VirtualServerEx{
 		VirtualServer: &conf_v1.VirtualServer{
diff --git a/tests/data/virtual-server-grpc-mixed/virtual-server-updated.yaml b/tests/data/virtual-server-grpc-mixed/virtual-server-updated.yaml
new file mode 100644
index 0000000000..32459967d1
--- /dev/null
+++ b/tests/data/virtual-server-grpc-mixed/virtual-server-updated.yaml
@@ -0,0 +1,28 @@
+apiVersion: k8s.nginx.org/v1
+kind: VirtualServer
+metadata:
+  name: virtual-server
+spec:
+  host: virtual-server.example.com
+  tls:
+    secret: virtual-server-tls-grpc-secret
+  upstreams:
+  - name: grpc1
+    service: grpc1-svc
+    port: 50051
+    type: grpc
+  - name: backend1
+    service: backend1-svc
+    port: 80
+  routes:
+  - path: "/helloworld.Greeter"
+    action:
+      pass: grpc1
+    errorPages:
+      - codes: [404]
+        return:
+          code: 200
+          body: "Original resource not found, but success!"
+  - path: "/backend1"
+    action:
+      pass: backend1
diff --git a/tests/suite/test_virtual_server_mixed_grpc.py b/tests/suite/test_virtual_server_mixed_grpc.py
index 3f8e795b5f..533ad5cd7d 100644
--- a/tests/suite/test_virtual_server_mixed_grpc.py
+++ b/tests/suite/test_virtual_server_mixed_grpc.py
@@ -7,10 +7,12 @@
 from suite.grpc.helloworld_pb2_grpc import GreeterStub
 from suite.resources_utils import create_example_app, wait_until_all_pods_are_ready, \
     delete_common_app, create_secret_from_yaml, replace_configmap_from_yaml, \
-    delete_items_from_yaml, get_first_pod_name
+    delete_items_from_yaml, get_first_pod_name, wait_before_test, get_events
 from suite.ssl_utils import get_certificate
-from suite.vs_vsr_resources_utils import get_vs_nginx_template_conf
-from suite.custom_assertions import assert_grpc_entries_exist, assert_proxy_entries_exist
+from suite.vs_vsr_resources_utils import get_vs_nginx_template_conf, \
+    patch_virtual_server_from_yaml
+from suite.custom_assertions import assert_grpc_entries_exist, assert_proxy_entries_exist, \
+    assert_event_and_count
 
 
 @pytest.fixture(scope="function")
@@ -102,3 +104,17 @@ def test_config_after_setup(self, kube_apis, ingress_controller_prerequisites, c
             except grpc.RpcError as e:
                 print(e.details())
                 pytest.fail("RPC error was not expected during call, exiting...")
+
+    @pytest.mark.parametrize("backend_setup", [{"app_type": "grpc-vs-mixed"}], indirect=True)
+    def test_config_error_page_warning(self, kube_apis, ingress_controller_prerequisites, crd_ingress_controller, 
+                                       backend_setup, virtual_server_setup):
+        text = f"{virtual_server_setup.namespace}/{virtual_server_setup.vs_name}"
+        vs_event_warning_text = f"Configuration for {text} was added or updated ; with warning(s):"
+        patch_virtual_server_from_yaml(kube_apis.custom_objects,
+                                        virtual_server_setup.vs_name,
+                                        f"{TEST_DATA}/virtual-server-grpc-mixed/virtual-server-updated.yaml",
+                                        virtual_server_setup.namespace)
+        wait_before_test()
+
+        events = get_events(kube_apis.v1, virtual_server_setup.namespace)
+        assert_event_and_count(vs_event_warning_text, 1, events)