Release - 0.4.0: Cherry Pick Fixes (#1350)

* Extension: fix pointer error (#1323) (cherry picked from commit 2bf9607) Signed-off-by: AliceProxy <alicewasko@datawire.io> * fix: add the namespace resource within helm templates (#1332) Add the namespace resource within helm templates This is unfortunate workaround due the difference in UX between `helm template` and `helm install` The project recommends `helm install` as a way to install EG which supports a `--create-namespace` flag to create a namespace However we also generate a static YAML using `helm template` as part of the release artficat so a user can install the YAML directly using `kubectl` instead of `helm` . The issue here is `helm template` does not support `--create-namespace`, so instead this commit adds a knob called `createNamespace` to the Helm chart which is `false` by default, but turned on during `make generate-manifests` Fixes: #1307 Signed-off-by: Arko Dasgupta <arko@tetrate.io> (cherry picked from commit 9d6d699) Signed-off-by: AliceProxy <alicewasko@datawire.io> --------- Signed-off-by: Arko Dasgupta <arko@tetrate.io> Signed-off-by: AliceProxy <alicewasko@datawire.io> Co-authored-by: Arko Dasgupta <arkodg@users.noreply.github.com>
envoyproxy · Apr 24, 2023 · c92514f · c92514f
1 parent 36839fd
commit c92514f
Show file tree

Hide file tree

Showing 14 changed files with 77 additions and 19 deletions.
diff --git a/charts/gateway-helm/templates/certgen-rbac.yaml b/charts/gateway-helm/templates/certgen-rbac.yaml
@@ -2,6 +2,7 @@ apiVersion: v1
 kind: ServiceAccount
 metadata:
   name: {{ include "eg.fullname" . }}-certgen
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
   annotations:
@@ -11,6 +12,7 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
 metadata:
   name: {{ include "eg.fullname" . }}-certgen
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
   annotations:
@@ -29,6 +31,7 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: RoleBinding
 metadata:
   name: {{ include "eg.fullname" . }}-certgen
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
   annotations:

diff --git a/charts/gateway-helm/templates/certgen.yaml b/charts/gateway-helm/templates/certgen.yaml
@@ -2,6 +2,7 @@ apiVersion: batch/v1
 kind: Job
 metadata:
   name: {{ include "eg.fullname" . }}-certgen
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
   annotations:

diff --git a/charts/gateway-helm/templates/envoy-gateway-config.yaml b/charts/gateway-helm/templates/envoy-gateway-config.yaml
@@ -2,6 +2,7 @@ apiVersion: v1
 kind: ConfigMap
 metadata:
   name: envoy-gateway-config
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
 data:

diff --git a/charts/gateway-helm/templates/envoy-gateway-deployment.yaml b/charts/gateway-helm/templates/envoy-gateway-deployment.yaml
@@ -2,13 +2,15 @@ apiVersion: v1
 kind: ServiceAccount
 metadata:
   name: envoy-gateway
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
 ---
 apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: envoy-gateway
+  namespace: '{{ .Release.Namespace }}'
   labels:
     control-plane: envoy-gateway
   {{- include "eg.labels" . | nindent 4 }}

diff --git a/charts/gateway-helm/templates/envoy-gateway-metrics-service.yaml b/charts/gateway-helm/templates/envoy-gateway-metrics-service.yaml
@@ -2,6 +2,7 @@ apiVersion: v1
 kind: Service
 metadata:
   name: envoy-gateway-metrics-service
+  namespace: '{{ .Release.Namespace }}'
   labels:
     control-plane: envoy-gateway
   {{- include "eg.labels" . | nindent 4 }}

diff --git a/charts/gateway-helm/templates/envoy-gateway-service.yaml b/charts/gateway-helm/templates/envoy-gateway-service.yaml
@@ -2,6 +2,7 @@ apiVersion: v1
 kind: Service
 metadata:
   name: envoy-gateway
+  namespace: '{{ .Release.Namespace }}'
   labels:
     control-plane: envoy-gateway
   {{- include "eg.labels" . | nindent 4 }}

diff --git a/charts/gateway-helm/templates/infra-manager-rbac.yaml b/charts/gateway-helm/templates/infra-manager-rbac.yaml
@@ -2,6 +2,7 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
 metadata:
   name: {{ include "eg.fullname" . }}-infra-manager
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
 rules:
@@ -29,6 +30,7 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: RoleBinding
 metadata:
   name: {{ include "eg.fullname" . }}-infra-manager
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
 roleRef:

diff --git a/charts/gateway-helm/templates/leader-election-rbac.yaml b/charts/gateway-helm/templates/leader-election-rbac.yaml
@@ -2,6 +2,7 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: Role
 metadata:
   name: {{ include "eg.fullname" . }}-leader-election-role
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
 rules:
@@ -41,6 +42,7 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: RoleBinding
 metadata:
   name: {{ include "eg.fullname" . }}-leader-election-rolebinding
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
 roleRef:

diff --git a/charts/gateway-helm/templates/metrics-reader-rbac.yaml b/charts/gateway-helm/templates/metrics-reader-rbac.yaml
@@ -2,6 +2,7 @@ apiVersion: rbac.authorization.k8s.io/v1
 kind: ClusterRole
 metadata:
   name: {{ include "eg.fullname" . }}-metrics-reader
+  namespace: '{{ .Release.Namespace }}'
   labels:
   {{- include "eg.labels" . | nindent 4 }}
 rules:

diff --git a/charts/gateway-helm/templates/namespace.yaml b/charts/gateway-helm/templates/namespace.yaml
@@ -0,0 +1,6 @@
+{{ if .Values.createNamespace }}
+apiVersion: v1
+kind: Namespace
+metadata:
+  name: '{{ .Release.Namespace }}' 
+{{ end }}    
diff --git a/charts/gateway-helm/values.tmpl.yaml b/charts/gateway-helm/values.tmpl.yaml
@@ -42,3 +42,4 @@ envoyGatewayMetricsService:
     protocol: TCP
     targetPort: https
 
+createNamespace: false
diff --git 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,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
 }

diff --git 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
+}
diff --git a/tools/make/kube.mk b/tools/make/kube.mk
@@ -125,7 +125,7 @@ generate-manifests: helm-generate ## Generate Kubernetes release manifests.
 	@$(LOG_TARGET)
 	@$(call log, "Generating kubernetes manifests")
 	mkdir -p $(OUTPUT_DIR)/
-	helm template eg charts/gateway-helm --include-crds --set deployment.envoyGateway.imagePullPolicy=$(IMAGE_PULL_POLICY) > $(OUTPUT_DIR)/install.yaml
+	helm template --set createNamespace=true eg charts/gateway-helm --include-crds --set deployment.envoyGateway.imagePullPolicy=$(IMAGE_PULL_POLICY) --namespace envoy-gateway-system > $(OUTPUT_DIR)/install.yaml
 	@$(call log, "Added: $(OUTPUT_DIR)/install.yaml")
 	cp examples/kubernetes/quickstart.yaml $(OUTPUT_DIR)/quickstart.yaml
 	@$(call log, "Added: $(OUTPUT_DIR)/quickstart.yaml")