Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: requests not matching defined routes trigger per-route filters #2663

Merged
merged 2 commits into from
Feb 20, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -155,14 +155,3 @@ xdsIR:
distinct: false
name: ""
prefix: /foo
- backendWeights:
invalid: 0
valid: 0
directResponse:
statusCode: 404
hostname: gateway.envoyproxy.io
name: gateway_envoyproxy_io/catch-all-return-404
pathMatch:
distinct: false
name: ""
prefix: /
Original file line number Diff line number Diff line change
Expand Up @@ -272,25 +272,3 @@ xdsIR:
distinct: false
name: ""
prefix: /bar
- backendWeights:
invalid: 0
valid: 0
directResponse:
statusCode: 404
hostname: www.foo.com
name: www_foo_com/catch-all-return-404
pathMatch:
distinct: false
name: ""
prefix: /
- backendWeights:
invalid: 0
valid: 0
directResponse:
statusCode: 404
hostname: www.bar.com
name: www_bar_com/catch-all-return-404
pathMatch:
distinct: false
name: ""
prefix: /
Original file line number Diff line number Diff line change
Expand Up @@ -62,15 +62,15 @@ httpRoutes:
name: httproute-2
spec:
hostnames:
- www.foo.com
- www.example.com
parentRefs:
- namespace: envoy-gateway
name: gateway-1
sectionName: http
rules:
- matches:
- path:
value: "/"
value: "/bar"
backendRefs:
- name: service-1
port: 8080
Expand Down
21 changes: 5 additions & 16 deletions internal/gatewayapi/testdata/securitypolicy-with-oidc.out.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ httpRoutes:
namespace: default
spec:
hostnames:
- www.foo.com
- www.example.com
parentRefs:
- name: gateway-1
namespace: envoy-gateway
Expand All @@ -97,7 +97,7 @@ httpRoutes:
port: 8080
matches:
- path:
value: /
value: /bar
status:
parents:
- conditions:
Expand Down Expand Up @@ -256,8 +256,8 @@ xdsIR:
port: 8080
protocol: HTTP
weight: 1
hostname: www.foo.com
name: httproute/default/httproute-2/rule/0/match/0/www_foo_com
hostname: www.example.com
name: httproute/default/httproute-2/rule/0/match/0/www_example_com
oidc:
clientID: client1.apps.googleusercontent.com
clientSecret: Y2xpZW50MTpzZWNyZXQK
Expand All @@ -272,15 +272,4 @@ xdsIR:
pathMatch:
distinct: false
name: ""
prefix: /
- backendWeights:
invalid: 0
valid: 0
directResponse:
statusCode: 404
hostname: www.example.com
name: www_example_com/catch-all-return-404
pathMatch:
distinct: false
name: ""
prefix: /
prefix: /bar
53 changes: 0 additions & 53 deletions internal/gatewayapi/translator.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,8 @@
package gatewayapi

import (
"fmt"
"strings"

"golang.org/x/exp/maps"
"k8s.io/apimachinery/pkg/runtime/schema"
"k8s.io/utils/ptr"
gwapiv1 "sigs.k8s.io/gateway-api/apis/v1"

egv1a1 "github.com/envoyproxy/gateway/api/v1alpha1"
Expand Down Expand Up @@ -207,61 +203,12 @@ func (t *Translator) Translate(resources *Resources) *TranslateResult {
// Sort xdsIR based on the Gateway API spec
sortXdsIRMap(xdsIR)

// Add a catch-all route for each HTTP listener if needed
addCatchAllRoute(xdsIR)

return newTranslateResult(gateways, httpRoutes, grpcRoutes, tlsRoutes,
tcpRoutes, udpRoutes, clientTrafficPolicies, backendTrafficPolicies,
securityPolicies, xdsIR, infraIR)

}

// For filters without native per-route support, we need to add a catch-all route
// to ensure that these filters are disabled for non-matching requests.
// https://github.com/envoyproxy/gateway/issues/2507
func addCatchAllRoute(xdsIR map[string]*ir.Xds) {
for _, i := range xdsIR {
for _, http := range i.HTTP {
var needCatchAllRoutePerHost = make(map[string]bool)
for _, r := range http.Routes {
if r.ExtAuth != nil || r.BasicAuth != nil || r.OIDC != nil {
needCatchAllRoutePerHost[r.Hostname] = true
}
}

// skip if there is already a catch-all route
for host := range needCatchAllRoutePerHost {
for _, r := range http.Routes {
if (r.Hostname == host &&
r.PathMatch != nil &&
r.PathMatch.Prefix != nil &&
*r.PathMatch.Prefix == "/") &&
len(r.HeaderMatches) == 0 &&
len(r.QueryParamMatches) == 0 {
delete(needCatchAllRoutePerHost, host)
}
}
}

for host, needCatchAllRoute := range needCatchAllRoutePerHost {
if needCatchAllRoute {
underscoredHost := strings.ReplaceAll(host, ".", "_")
http.Routes = append(http.Routes, &ir.HTTPRoute{
Name: fmt.Sprintf("%s/catch-all-return-404", underscoredHost),
PathMatch: &ir.StringMatch{
Prefix: ptr.To("/"),
},
DirectResponse: &ir.DirectResponse{
StatusCode: 404,
},
Hostname: host,
})
}
}
}
}
}

// GetRelevantGateways returns GatewayContexts, containing a copy of the original
// Gateway with the Listener statuses reset.
func (t *Translator) GetRelevantGateways(gateways []*gwapiv1.Gateway) []*GatewayContext {
Expand Down
51 changes: 3 additions & 48 deletions internal/xds/translator/basicauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ package translator

import (
"errors"
"fmt"

corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
routev3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
Expand Down Expand Up @@ -35,6 +34,7 @@ var _ httpFilter = &basicAuth{}
// patchHCM builds and appends the basic_auth Filters to the HTTP Connection Manager
// if applicable, and it does not already exist.
// Note: this method creates an basic_auth filter for each route that contains an BasicAuth config.
// The filter is disabled by default. It is enabled on the route level.
func (*basicAuth) patchHCM(mgr *hcmv3.HttpConnectionManager, irListener *ir.HTTPListener) error {
var errs error

Expand Down Expand Up @@ -77,7 +77,8 @@ func buildHCMBasicAuthFilter(route *ir.HTTPRoute) (*hcmv3.HttpFilter, error) {
}

return &hcmv3.HttpFilter{
Name: basicAuthFilterName(route),
Name: basicAuthFilterName(route),
Disabled: true,
ConfigType: &hcmv3.HttpFilter_TypedConfig{
TypedConfig: basicAuthAny,
},
Expand Down Expand Up @@ -116,52 +117,6 @@ func (*basicAuth) patchResources(*types.ResourceVersionTable, []*ir.HTTPRoute) e
return nil
}

// patchRouteCfg patches the provided route configuration with the basicAuth filter
// if applicable.
// Note: this method disables all the basicAuth filters by default. The filter will
// be enabled per-route in the typePerFilterConfig of the route.
func (*basicAuth) patchRouteConfig(routeCfg *routev3.RouteConfiguration, irListener *ir.HTTPListener) error {
if routeCfg == nil {
return errors.New("route configuration is nil")
}
if irListener == nil {
return errors.New("ir listener is nil")
}

var errs error
for _, route := range irListener.Routes {
if !routeContainsBasicAuth(route) {
continue
}

filterName := basicAuthFilterName(route)
filterCfg := routeCfg.TypedPerFilterConfig

if _, ok := filterCfg[filterName]; ok {
// This should not happen since this is the only place where the basicAuth
// filter is added in a route.
errs = errors.Join(errs, fmt.Errorf(
"route config already contains basicAuth config: %+v", route))
continue
}

// Disable all the filters by default. The filter will be enabled
// per-route in the typePerFilterConfig of the route.
routeCfgAny, err := anypb.New(&routev3.FilterConfig{Disabled: true})
if err != nil {
errs = errors.Join(errs, err)
continue
}

if filterCfg == nil {
routeCfg.TypedPerFilterConfig = make(map[string]*anypb.Any)
}

routeCfg.TypedPerFilterConfig[filterName] = routeCfgAny
}
return errs
}

// patchRoute patches the provided route with the basicAuth config if applicable.
// Note: this method enables the corresponding basicAuth filter for the provided route.
func (*basicAuth) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
Expand Down
4 changes: 0 additions & 4 deletions internal/xds/translator/cors.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,10 +165,6 @@ func (*cors) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
return nil
}

func (*cors) patchRouteConfig(*routev3.RouteConfiguration, *ir.HTTPListener) error {
return nil
}

func (c *cors) patchResources(*types.ResourceVersionTable, []*ir.HTTPRoute) error {
return nil
}
51 changes: 3 additions & 48 deletions internal/xds/translator/extauth.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ package translator

import (
"errors"
"fmt"
"net/url"

corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
Expand Down Expand Up @@ -38,6 +37,7 @@ var _ httpFilter = &extAuth{}
// patchHCM builds and appends the ext_authz Filters to the HTTP Connection Manager
// if applicable, and it does not already exist.
// Note: this method creates an ext_authz filter for each route that contains an ExtAuthz config.
// The filter is disabled by default. It is enabled on the route level.
// TODO: zhaohuabing avoid duplicated HTTP filters
func (*extAuth) patchHCM(mgr *hcmv3.HttpConnectionManager, irListener *ir.HTTPListener) error {
var errs error
Expand Down Expand Up @@ -80,7 +80,8 @@ func buildHCMExtAuthFilter(route *ir.HTTPRoute) (*hcmv3.HttpFilter, error) {
}

return &hcmv3.HttpFilter{
Name: extAuthFilterName(route),
Name: extAuthFilterName(route),
Disabled: true,
ConfigType: &hcmv3.HttpFilter_TypedConfig{
TypedConfig: extAuthAny,
},
Expand Down Expand Up @@ -244,52 +245,6 @@ func createExtServiceXDSCluster(rd *ir.RouteDestination, tCtx *types.ResourceVer
return nil
}

// patchRouteCfg patches the provided route configuration with the extAuth filter
// if applicable.
// Note: this method disables all the extAuth filters by default. The filter will
// be enabled per-route in the typePerFilterConfig of the route.
func (*extAuth) patchRouteConfig(routeCfg *routev3.RouteConfiguration, irListener *ir.HTTPListener) error {
if routeCfg == nil {
return errors.New("route configuration is nil")
}
if irListener == nil {
return errors.New("ir listener is nil")
}

var errs error
for _, route := range irListener.Routes {
if !routeContainsExtAuth(route) {
continue
}

filterName := extAuthFilterName(route)
filterCfg := routeCfg.TypedPerFilterConfig

if _, ok := filterCfg[filterName]; ok {
// This should not happen since this is the only place where the extAuth
// filter is added in a route.
errs = errors.Join(errs, fmt.Errorf(
"route config already contains extAuth config: %+v", route))
continue
}

// Disable all the filters by default. The filter will be enabled
// per-route in the typePerFilterConfig of the route.
routeCfgAny, err := anypb.New(&routev3.FilterConfig{Disabled: true})
if err != nil {
errs = errors.Join(errs, err)
continue
}

if filterCfg == nil {
routeCfg.TypedPerFilterConfig = make(map[string]*anypb.Any)
}

routeCfg.TypedPerFilterConfig[filterName] = routeCfgAny
}
return errs
}

// patchRoute patches the provided route with the extAuth config if applicable.
// Note: this method enables the corresponding extAuth filter for the provided route.
func (*extAuth) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
Expand Down
4 changes: 0 additions & 4 deletions internal/xds/translator/fault.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,6 @@ func (*fault) patchResources(*types.ResourceVersionTable, []*ir.HTTPRoute) error
return nil
}

func (*fault) patchRouteConfig(routeCfg *routev3.RouteConfiguration, irListener *ir.HTTPListener) error {
return nil
}

// patchRoute patches the provided route with the fault config if applicable.
// Note: this method enables the corresponding fault filter for the provided route.
func (*fault) patchRoute(route *routev3.Route, irRoute *ir.HTTPRoute) error {
Expand Down
Loading
Loading