Skip to content

Commit

Permalink
chore: add a warning for gRPC conflicting error page
Browse files Browse the repository at this point in the history
  • Loading branch information
@ciarams87
ciarams87 committed Nov 18, 2021
1 parent fea8b93 commit 18a28c4
Show file tree
Hide file tree
Showing 6 changed files with 262 additions and 29 deletions.
16 changes: 8 additions & 8 deletions internal/configs/version2/nginx-plus.virtualserver.tmpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand All @@ -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;
Expand All @@ -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;
Expand Down
18 changes: 9 additions & 9 deletions internal/configs/version2/nginx.virtualserver.tmpl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Expand All @@ -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;
Expand All @@ -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;
Expand Down
72 changes: 63 additions & 9 deletions internal/configs/virtualserver.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 != "" {
Expand All @@ -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
}

Expand Down Expand Up @@ -426,7 +459,7 @@ func (vsc *virtualServerConfigurator) GenerateVirtualServerConfig(vsEx *VirtualS
matchesRoutes,
len(splitClients),
vsc.cfgParams,
r.ErrorPages,
errorPages,
errorPageIndex,
vsLocSnippets,
vsc.enableSnippets,
Expand All @@ -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...)
Expand All @@ -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)

Expand All @@ -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]
Expand Down Expand Up @@ -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 {
Expand Down
135 changes: 135 additions & 0 deletions internal/configs/virtualserver_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
28 changes: 28 additions & 0 deletions tests/data/virtual-server-grpc-mixed/virtual-server-updated.yaml
View file
Original file line number Diff line number Diff line change
@@ -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
Loading

0 comments on commit 18a28c4

Please sign in to comment.