From 2bf96076c697bbda2c6de6abb55dd175676fe81d Mon Sep 17 00:00:00 2001 From: Alice Wasko <alicewasko@datawire.io> Date: Thu, 20 Apr 2023 04:41:31 -0700 Subject: [PATCH] Extension: fix pointer error (#1323) --- internal/extension/testutils/hooks.go | 31 ++++++++++++++------ internal/xds/translator/extension.go | 42 +++++++++++++++++++++------ 2 files changed, 55 insertions(+), 18 deletions(-) diff --git a/internal/extension/testutils/hooks.go b/internal/extension/testutils/hooks.go index 3905923de79..d42867163ba 100644 --- a/internal/extension/testutils/hooks.go +++ b/internal/extension/testutils/hooks.go @@ -16,6 +16,7 @@ import ( listenerV3 "github.com/envoyproxy/go-control-plane/envoy/config/listener/v3" routeV3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" tlsV3 "github.com/envoyproxy/go-control-plane/envoy/extensions/transport_sockets/tls/v3" + "github.com/golang/protobuf/proto" "google.golang.org/protobuf/types/known/durationpb" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) @@ -24,16 +25,20 @@ type XDSHookClient struct{} // PostRouteModifyHook returns a modified version of the route using context info and the passed in extensionResources func (c *XDSHookClient) PostRouteModifyHook(route *routeV3.Route, routeHostnames []string, extensionResources []*unstructured.Unstructured) (*routeV3.Route, error) { + // Simulate an error an extension may return + if route.Name == "extension-post-xdsroute-hook-error" { + return nil, errors.New("route hook resource error") + } + + // Setup a new route to avoid operating directly on the passed in pointer for better test coverage that the + // route we are returning gets used properly + modifiedRoute := proto.Clone(route).(*routeV3.Route) for _, extensionResource := range extensionResources { - // Simulate an error an extension may return - if route.Name == "extension-post-xdsroute-hook-error" { - return nil, errors.New("route hook resource error") - } - route.ResponseHeadersToAdd = append(route.ResponseHeadersToAdd, + modifiedRoute.ResponseHeadersToAdd = append(modifiedRoute.ResponseHeadersToAdd, &coreV3.HeaderValueOption{ Header: &coreV3.HeaderValue{ Key: "mock-extension-was-here-route-name", - Value: route.Name, + Value: modifiedRoute.Name, }, }, &coreV3.HeaderValueOption{ @@ -68,7 +73,7 @@ func (c *XDSHookClient) PostRouteModifyHook(route *routeV3.Route, routeHostnames }, ) } - return route, nil + return modifiedRoute, nil } // PostVirtualHostModifyHook returns a modified version of the virtualhost with a new route injected @@ -78,7 +83,10 @@ func (c *XDSHookClient) PostVirtualHostModifyHook(vh *routeV3.VirtualHost) (*rou if vh.Name == "extension-post-xdsvirtualhost-hook-error" { return nil, fmt.Errorf("extension post xds virtual host hook error") } else if vh.Name == "extension-listener" { - vh.Routes = append(vh.Routes, &routeV3.Route{ + // Setup a new VirtualHost to avoid operating directly on the passed in pointer for better test coverage that the + // VirtualHost we are returning gets used properly + modifiedVH := proto.Clone(vh).(*routeV3.VirtualHost) + modifiedVH.Routes = append(modifiedVH.Routes, &routeV3.Route{ Name: "mock-extension-inserted-route", Action: &routeV3.Route_DirectResponse{ DirectResponse: &routeV3.DirectResponseAction{ @@ -86,6 +94,7 @@ func (c *XDSHookClient) PostVirtualHostModifyHook(vh *routeV3.VirtualHost) (*rou }, }, }) + return modifiedVH, nil } return vh, nil } @@ -100,7 +109,11 @@ func (c *XDSHookClient) PostHTTPListenerModifyHook(l *listenerV3.Listener) (*lis if l.Name == "extension-post-xdslistener-hook-error" { return nil, fmt.Errorf("extension post xds listener hook error") } else if l.Name == "extension-listener" { - l.StatPrefix = "mock-extension-inserted-prefix" + // Setup a new Listener to avoid operating directly on the passed in pointer for better test coverage that the + // Listener we are returning gets used properly + modifiedListener := proto.Clone(l).(*listenerV3.Listener) + modifiedListener.StatPrefix = "mock-extension-inserted-prefix" + return modifiedListener, nil } return l, nil } diff --git a/internal/xds/translator/extension.go b/internal/xds/translator/extension.go index c1248c7129d..0c925a75149 100644 --- a/internal/xds/translator/extension.go +++ b/internal/xds/translator/extension.go @@ -9,6 +9,10 @@ package translator import ( + "errors" + "fmt" + "reflect" + clusterv3 "github.com/envoyproxy/go-control-plane/envoy/config/cluster/v3" listenerv3 "github.com/envoyproxy/go-control-plane/envoy/config/listener/v3" routev3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3" @@ -17,11 +21,10 @@ import ( resourcev3 "github.com/envoyproxy/go-control-plane/pkg/resource/v3" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "github.com/envoyproxy/gateway/internal/ir" - "github.com/envoyproxy/gateway/internal/xds/types" - "github.com/envoyproxy/gateway/api/config/v1alpha1" extensionTypes "github.com/envoyproxy/gateway/internal/extension/types" + "github.com/envoyproxy/gateway/internal/ir" + "github.com/envoyproxy/gateway/internal/xds/types" ) func processExtensionPostRouteHook(route *routev3.Route, vHost *routev3.VirtualHost, irRoute *ir.HTTPRoute, em *extensionTypes.Manager) error { @@ -53,9 +56,9 @@ func processExtensionPostRouteHook(route *routev3.Route, vHost *routev3.VirtualH // If the extension returned a modified Route, then copy its to the one that was passed in as a reference if modifiedRoute != nil { - // Overwrite the pointer for the original route. - // Uses nolint because of the ineffectual assignment check - route = modifiedRoute //nolint + if err = deepCopyPtr(modifiedRoute, route); err != nil { + return err + } } return nil } @@ -81,9 +84,9 @@ func processExtensionPostVHostHook(vHost *routev3.VirtualHost, em *extensionType // If the extension returned a modified Virtual Host, then copy its to the one that was passed in as a reference if modifiedVH != nil { - // Overwrite the pointer for the original virtual host. - // Uses nolint because of the ineffectual assignment check - vHost = modifiedVH //nolint + if err = deepCopyPtr(modifiedVH, vHost); err != nil { + return err + } } return nil @@ -168,3 +171,24 @@ func processExtensionPostTranslationHook(tCtx *types.ResourceVersionTable, em *e return nil } + +func deepCopyPtr(src interface{}, dest interface{}) error { + if src == nil || dest == nil { + return errors.New("cannot deep copy nil pointer") + } + srcVal := reflect.ValueOf(src) + destVal := reflect.ValueOf(src) + if srcVal.Kind() == reflect.Ptr && destVal.Kind() == reflect.Ptr { + srcElem := srcVal.Elem() + destVal = reflect.New(srcElem.Type()) + destElem := destVal.Elem() + destElem.Set(srcElem) + reflect.ValueOf(dest).Elem().Set(destVal.Elem()) + } else { + return fmt.Errorf("cannot deep copy pointers to different kinds src %v != dest %v", + srcVal.Kind(), + destVal.Kind(), + ) + } + return nil +}