gloo icon indicating copy to clipboard operation
gloo copied to clipboard

Gateways false boolean TLS and HCM setting override not working

Open DoroNahari opened this issue 2 years ago • 5 comments

Gloo Edge Version

1.11.x (latest stable)

Kubernetes Version

1.22.x

Describe the bug

This feature https://github.com/solo-io/gloo/pull/6414 presented an option to override settings with delegated gateways. As described in the feature: preventChildOverrides=false | {"parent":{"foo":"bar"}, "child":{"foo":"baz"}} --> {"foo":"baz"}

When setting a parent gateway with a boolean prop true and a child gateway with the same boolean prop false, the override is not working.

For example: Parent gateway:

apiVersion: gateway.solo.io/v1
kind: Gateway
metadata:
  name: gateway-proxy-ssl
  namespace: gloo-system
spec:
  bindAddress: '::'
  bindPort: 8443
  hybridGateway:
    delegatedHttpGateways:
      httpConnectionManagerSettings:
        acceptHttp10: true
      selector:
        labels:
          hybrid-gateway: ssl
  proxyNames:
    - gateway-proxy
  ssl: true
  useProxyProto: false

Child gateway:

apiVersion: gateway.solo.io/v1
kind: MatchableHttpGateway
metadata:
  name: default-ssl-http-gateway-2
  namespace: apigw-gloo-ee
  labels:
    hybrid-gateway: ssl
spec:
  httpGateway:
    virtualServiceSelector:
      http-gateway: default-ssl
    options:
        httpConnectionManagerSettings:
          acceptHttp10: false

...

The observed behavior was that http1.0 yet was accepted

DoroNahari avatar May 19 '22 12:05 DoroNahari

this is a real ...ish issue. It's mostly a logic problem. Consider the field SkipXffAppend, which has this autogenerated getter:

func (x *HttpConnectionManagerSettings) GetSkipXffAppend() bool {
	if x != nil {
		return x.SkipXffAppend
	}
	return false
}

because booleans cannot be nil, our current implementation gives us these possibilities:

parent | child  | outcome
------ | ------ | -------
true   | true   | true    # as-expected
true   | false  | true    # raises issue, as _optimally_, we want "false"
true   | unset  | true    # as-expected
false  | true   | true    # as-expected
false  | false  | false   # as-expected
false  | unset  | false   # as-expected
unset  | true   | true    # as-expected
unset  | false  | false   # as-expected
unset  | unset  | false   # as-expected

it is totally possible to logically write code to override true to false, exactly as we want. The problem comes when we consider all unset user fields will always be false in such a case.

consider such a case, where a child value of unset could override a parent's true:

parent | child  | outcome
------ | ------ | -------
true   | true   | true    # as-expected
true   | false  | true    # as-expected (we are now happy)
true   | unset  | false   # raises a _new_ issue
false  | true   | true    # as-expected
false  | false  | false   # as-expected
false  | unset  | false   # as-expected
unset  | true   | true    # as-expected
unset  | false  | false   # as-expected
unset  | unset  | false   # as-expected

gunnar-solo avatar May 20 '22 14:05 gunnar-solo

mergo (the merging library we're currently using) calls this concept "overwriting with a 0 value" to be more data structure agnostic (consider enums, empty strings, the number 0...). Similar user issue.

By specifying mergo.WithOverwriteWithEmptyValue, we could adopt behavior that solves your original issue ...at the expense of unset values overwriting set values

gunnar-solo avatar May 20 '22 14:05 gunnar-solo

Three possible approaches to resolve this issue:

  • accept the limitations described above, on way or the other
  • a large breaking change/mass deprecation where we convert all bools to *wrappers.BoolValue (so they can be nil)
  • muck with our unmarshalling from crd code, such that we preserve/store the keys of resourceCrd.Spec as a list onto "some" field

gunnar-solo avatar May 20 '22 14:05 gunnar-solo

Related to https://github.com/solo-io/gloo/issues/6532

chrisgaun avatar Jun 07 '22 13:06 chrisgaun

We have come across another scenario where this issue rises. When setting a parent gateway with a non-0 integer prop, and a child gateway with a zero-valued prop, the override doesn't work. This happens for instance when trying to override the 'HttpConnectionManagerSettings_CodecType' value. The 0 valued child prop is not propagated, and therefore can't override the parent's non-0 value.

Tyzanol avatar Jun 28 '22 07:06 Tyzanol

We have come across another scenario where this issue rises. When setting a parent gateway with a non-0 integer prop, and a child gateway with a zero-valued prop, the override doesn't work. This happens for instance when trying to override the 'HttpConnectionManagerSettings_CodecType' value. The 0 valued child prop is not propagated, and therefore can't override the parent's non-0 value. https://github.com/solo-io/gloo/issues/6476#issuecomment-1168362941

Hi, The issue when using HttpConnectionManagerSettings_CodecType = 0 still appears.

Seems that the type of HttpConnectionManagerSettings_CodecType is int32, instead of *int32

Thanks for your reference.

LinMoskovitch avatar Mar 19 '23 14:03 LinMoskovitch