Extension: fix pointer error (#1323)

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,14 +83,18 @@ 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{
 					Status: uint32(200),
 				},
 			},
 		})
+		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
 }

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
+}