extension: fix pointer error

previously I was under the false impression that despite the linter
warning about an innefectual assignment that things were working as
intended. This was because the test cases directly operated on passed in
pointers instead of ensuring that the returned pointer was being used.

Signed-off-by: AliceProxy <alicewasko@datawire.io>

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