[backport] gateway2: use safer merging to avoid assuming zero values …

…as being unset (#10559)

Signed-off-by: Shashank Ram <[email protected]>

changelog/v1.18.4/merge-check.yaml

@@ -0,0 +1,17 @@
+changelog:
+  - type: NON_USER_FACING
+    resolvesIssue: false
+    description: |
+      gateway2: use safer merging to avoid assuming zero values as being unset
+
+      The legacy Edge code uses ShallowMerge() which can undesirably overwrite
+      zero values mistaking them for unset values. RouteOptions merging in
+      GatewayV2 uses the same API, but this can result in undesirable effects
+      if the merging considers zero valued fields as being unset. To avoid
+      this, the options merging used by GatewayV2 relies on a safer merge
+      that only allows merging of values that can be set to Nil (pointers,
+      slices, maps, etc.) which works since all user-facing fields on the
+      RouteOptions are nil-able. Functionally, this is the same as before
+      due to all fields being nil-able, but is a bit clearer to readers.
+      Moreover, trying to merge a non-nil field will panic which can catch
+      potential misuse of the API.

projects/gateway2/translator/plugins/routeoptions/route_options_plugin_test.go

@@ -890,7 +890,7 @@ var _ = DescribeTable("mergeOptionsForRoute",
 			PrefixRewrite: &wrapperspb.StringValue{Value: "/dst"},
 			Timeout:       durationpb.New(10 * time.Second),
 		},
-		glooutils.OptionsMergedFull,
+		glooutils.OptionsMergedPartial, // PrefixRewrite is not overwritten
 	),
 	Entry("override and augment dst options with annotation: specific fields",
 		&gwv1.HTTPRoute{

projects/gloo/pkg/translator/ssl_configuration.go

@@ -72,7 +72,7 @@ func MergeSslConfig(dst, src *ssl.SslConfig) *ssl.SslConfig {
 
 	for i := range dstValue.NumField() {
 		dstField, srcField := dstValue.Field(i), srcValue.Field(i)
-		utils.ShallowMerge(dstField, srcField, false)
+		utils.ShallowMerge(dstField, srcField)
 	}
 
 	return dst

projects/gloo/pkg/utils/merge.go

@@ -25,14 +25,28 @@ const (
 	OptionsMergedFull
 )
 
-// ShallowMerge sets dst to the value of src, if src is non-zero and dst is zero-valued or overwrite=true.
+// ShallowMerge sets dst to the value of src, if src is non-zero and dst is zero-valued
 // It returns a boolean indicating whether src overwrote dst.
-func ShallowMerge(dst, src reflect.Value, overwrite bool) bool {
+func ShallowMerge(dst, src reflect.Value) bool {
 	if !src.IsValid() {
 		return false
 	}
 
-	if dst.CanSet() && !isEmptyValue(src) && (overwrite || isEmptyValue(dst)) {
+	if dst.CanSet() && !isEmptyValue(src) && isEmptyValue(dst) {
+		dst.Set(src)
+		return true
+	}
+
+	return false
+}
+
+// maySetValue sets dst to the value of src if:
+// - src is set (has a non-nil value) and
+// - dst is nil or overwrite is true
+//
+// It returns a boolean indicating whether src overwrote dst.
+func maySetValue(dst, src reflect.Value, overwrite bool) bool {
+	if src.CanSet() && !src.IsNil() && dst.CanSet() && (overwrite || dst.IsNil()) {
 		dst.Set(src)
 		return true
 	}
@@ -84,7 +98,7 @@ func ShallowMergeRouteOptions(dst, src *v1.RouteOptions) (*v1.RouteOptions, bool
 	overwrote := false
 	for i := range dstValue.NumField() {
 		dstField, srcField := dstValue.Field(i), srcValue.Field(i)
-		if srcOverride := ShallowMerge(dstField, srcField, false); srcOverride {
+		if srcOverride := ShallowMerge(dstField, srcField); srcOverride {
 			overwrote = true
 		}
 	}
@@ -110,7 +124,7 @@ func ShallowMergeVirtualHostOptions(dst, src *v1.VirtualHostOptions) (*v1.Virtua
 	overwrote := false
 	for i := range dstValue.NumField() {
 		dstField, srcField := dstValue.Field(i), srcValue.Field(i)
-		if srcOverride := ShallowMerge(dstField, srcField, false); srcOverride {
+		if srcOverride := ShallowMerge(dstField, srcField); srcOverride {
 			overwrote = true
 		}
 	}
@@ -147,23 +161,25 @@ func MergeRouteOptionsWithOverrides(dst, src *v1.RouteOptions, allowedOverrides
 	var dstFieldsOverwritten int
 	for i := range dstValue.NumField() {
 		dstField, srcField := dstValue.Field(i), srcValue.Field(i)
+
+		// NOTE: important to pre-compute this because dstFieldsOverwritten must be
+		// incremented based on the original value of dstField and not the overwritten value
+		dstIsSet := dstField.CanSet() && !dstField.IsNil()
+		if dstIsSet {
+			dstFieldsSet++
+		}
+
 		// Allow overrides if the field in dst is unset as merging from src into dst by default
 		// allows src to augment dst by setting fields unset in dst, hence the override check only
-		// applies when the field in dst is set: !isEmptyValue(dstField).
-		if !isEmptyValue(dstField) && overwriteByDefault && !(allowedOverrides.Has(wildcardField) ||
+		// applies when the field in dst is set (dstIsSet=true).
+		if dstIsSet && overwriteByDefault && !(allowedOverrides.Has(wildcardField) ||
 			allowedOverrides.Has(strings.ToLower(dstValue.Type().Field(i).Name))) {
 			continue
 		}
-		// NOTE: important to pre-compute this for use in the conditional below
-		// because dstFieldsOverwritten needs to be incremented based on the original value of dstField
-		// and not the state of the field after the merge
-		dstOverridable := dstField.CanSet() && !isEmptyValue(dstField)
-		if dstOverridable {
-			dstFieldsSet++
-		}
-		if srcOverride := ShallowMerge(dstField, srcField, overwriteByDefault); srcOverride {
+
+		if srcOverride := maySetValue(dstField, srcField, overwriteByDefault); srcOverride {
 			srcFieldsUsed++
-			if dstOverridable {
+			if dstIsSet {
 				dstFieldsOverwritten++
 			}
 		}