Clean up default socket factory in TransportSocketMatcherImpl

[noah8713]

Title: Allow skipping use_alpn when auto_config is set for httpprotocoloptions

Description:

As a part of istio/istio#36299 to enable case preserve for http1.1 headers, using auto_config breaks communication between istiod and ingress gateway as config push fails because envory rejects the clusters config with error

ALPN configured for cluster outbound|443||xx which has a non-ALPN transport socket.

For raw transport socket, if there is no alpn support where its simple tls for upstream, can we allow setting use_alpn to false as there is no over-ride option?

if (options.has_auto_config()) {
    use_http2_ = true;
    use_alpn_ = true;
  }

Goal is to ensure allowing http2 traffic to be passed via istio ingressgateway and at the same time cater to http1.1 case preserve for clusters using http1. Is there any trivial fix or suggestions to solve it optimally?

[optional Relevant Links:]

Details to reproduce in istio/istio#36299

[mattklein123]

Can you provide a more complete Envoy config of what is causing this issue (no Istio details please). It's hard for me to understand the actual problem.

[noah8713]

Can you provide a more complete Envoy config of what is causing this issue (no Istio details please). It's hard for me to understand the actual problem.

Sure: e.g when applying below filter for enabling case preserve using auto_config vs explicit_http_config,

apiVersion: networking.istio.io/v1alpha3
kind: EnvoyFilter
metadata:
  name: header-case-preserve-1.10
  namespace: istio-system
  labels:
    operator.istio.io/component: "Pilot"
spec:
  configPatches:
  - applyTo: CLUSTER
    match:
      context: GATEWAY
      proxy:
        proxyVersion: '^1\.10.*'
    patch:
      operation: MERGE
      value:
        typed_extension_protocol_options:
          envoy.extensions.upstreams.http.v3.HttpProtocolOptions:
            "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions
            commonHttpProtocolOptions:
              idleTimeout: 60s
            auto_config:
              http_protocol_options:
                header_key_format:
                  stateful_formatter:
                    name: preserve_case  # preserve header case for response from backend service
                    typed_config:
                      "@type": type.googleapis.com/envoy.extensions.http.header_formatters.preserve_case.v3.PreserveCaseFormatterConfig

envoy continuously fails for error/rejecting any new changes seen from proxy logs below:

 Internal:Error adding/updating cluster(s) outbound|443||httpbin-svc.httpbin.svc.45.x.io: ALPN configured for cluster outbound|443||httpbin-svc.httpbin.svc.45.x.io which has a non-ALPN transport socket: name: "outbound|443||httpbin-svc.httpbin.svc.45.x.io"
typed_extension_protocol_options {
  key: "envoy.extensions.upstreams.http.v3.HttpProtocolOptions"
  value {
    [type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions] {
      common_http_protocol_options {
        idle_timeout {
          seconds: 60
        }
      }
      auto_config {
        http_protocol_options {
          header_key_format {
            stateful_formatter {
              name: "preserve_case"
              typed_config {
                [type.googleapis.com/envoy.extensions.http.header_formatters.preserve_case.v3.PreserveCaseFormatterConfig] {
                }
              }
            }
          }
        }
      }
    }
  }
}

envoy config dump

[mattklein123]

Can you put the full config dump in a public gist?

[mattklein123]

Also can you describe the desired flow at a high level? I'm still confused about what "auto" functionality you are trying to achieve without using ALPN.

[noah8713]

Can you put the full config dump in a public gist?

Sure: dump-gist

[noah8713]

Also can you describe the desired flow at a high level? I'm still confused about what "auto" functionality you are trying to achieve without using ALPN.

If we have n services where n/2 services are http1 and n/2 services are http2, need to skip http1.1 case preserve if the service is http2 automatically vs defining case preserve explicitly for all n/2 services . Hence, trying to leverage auto_config by creating just one envoy filter and let case preserve by default kick in for http1 outbound services, but it seems use_alpn is mandatory if auto_config is chosen.

Also, envoy cluster match is exact string match and no regex. So will end up creating n/2 filters for n/2 http1 services using case preserve which can be cumbersome so want to know if we can optimize here.

Also, not sure if we have limit on number of envoy filters that we can create/support ;e.g. if we have 1k services with http1 needing case preserve, will there be a big performance hit if we create 1k envoyfilters with cluster match on those services. Wanted to double confirm

[mattklein123]

That config dump is huge. Can you provide some kind of simplified example of what you want to achieve? I still don't understand the goal here. "auto" config basically means to use ALPN to figure out what protocol to use. If you aren't doing auto how are you choosing whether to use H1 or H2?

[noah8713]

Sure. amended for a sample cluster/service in the same gist to shorten the dump file.
For how are you choosing whether to use H1 or H2?
thats defined in the k8s service spec using protocol e.g. for protocol grpc, it creates filter chain as

explicitHttpConfig:
     http2ProtocolOptions{}

Hence, asked if using auto_config, why alpn is mandatory ? or rather shouldnt it not auto translate to select without alpn whether h1/h2.

[mattklein123]

Hence, asked if using auto_config, why alpn is mandatory ? or rather shouldnt it not auto translate to select without alpn whether h1/h2.

Because this is what auto does. It determiens what to use based on ALPN. Given that you are already deciding whether to use H1 or H2 based on some label, I think you just want to populate an explicit_http_config for h1 for h1 services.

[noah8713]

dump file

Agree as thats the current behavior. Issue is to configure explicit_http_config with case preserve, we will have to populate explicit_http_config for h1 services explicitly for all clusters. Also cluster match on name only support exact name and hence if we have n clusters from same namespaces, leveraging regex would be good too (but cluster match doesnt support regex) vs n filters for all those clustes. Can we add regex support too?

e.g.

Kind: Envoyfilter
Context: gateway
cluster:
    name: "*.x.*"

Hence, in case of gateway where it allows passing both h1 and h2 traffic, using explicit_http_config with preserve case disables h2 traffic so ask is to use something like

Context: gateway
cluster: 
    name: 'xxxxx'
AutoHttpConfig:
     use_alpn: false
     httpProtocolOptions:
          preserve_case_xxx

So if services that use h2, should auto ignore preserve_case as its explicitly for h1 services only. So is it doable or should we support this case and see further?

[mattklein123]

we will have to populate explicit_http_config for h1 services explicitly for all clusters

You are already doing this for h2 services. Fundamentally, you have to tell envoy how to pick a protocol. Either it's prior knowledge or you have to use ALPN.

[noah8713]

You are already doing this for h2 services. Fundamentally, you have to tell envoy how to pick a protocol. Either it's prior knowledge or you have to use ALPN.

Thanks. Yes thats how its done by istio for now based on protocol .Will take a note that autoconfig will solely rely on alpn and we cannot tweak it and rather keep it as is. So it seems may be we will have to define an option in istio/k8s service itself ; say if service is h1 and needs case preserve, add an option and it should generate relevant config with preserve_case option on top of h1.

Lastly, can we consider extending cluster name match to regex as an add on in envoy ? I am not sure of the original intent on why it was kept as exact match and no regex being added. Would love to see if it can be done as an add on.

[bbassingthwaite]

I believe I am experiencing a similar issue:

The config that is erroring out for me:

eds_cluster_config {
  eds_config {
    ads {
    }
    resource_api_version: V3
  }
}
connect_timeout {
  seconds: 5
}
lb_policy: LEAST_REQUEST
circuit_breakers {
  thresholds {
    max_connections {
      value: 1000
    }
    max_pending_requests {
      value: 2000
    }
    max_requests {
      value: 4000
    }
    max_retries {
    }
    track_remaining: true
  }
}
common_lb_config {
  healthy_panic_threshold {
  }
}
close_connections_on_host_health_failure: true
typed_extension_protocol_options {
  key: "envoy.extensions.upstreams.http.v3.HttpProtocolOptions"
  value {
    [type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions] {
      common_http_protocol_options {
        idle_timeout {
          seconds: 1
        }
      }
      auto_config {
      }
    }
  }
}
transport_socket_matches {
  name: "default"
  transport_socket {
    name: "envoy.transport_sockets.tls"
    typed_config {
      [type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext] {
        common_tls_context {
          tls_params {
            tls_minimum_protocol_version: TLSv1_2
            tls_maximum_protocol_version: TLSv1_3
            cipher_suites: "[ECDHE-ECDSA-AES128-GCM-SHA256|ECDHE-ECDSA-CHACHA20-POLY1305]"
            cipher_suites: "[ECDHE-RSA-AES128-GCM-SHA256|ECDHE-RSA-CHACHA20-POLY1305]"
            cipher_suites: "ECDHE-ECDSA-AES256-GCM-SHA384"
            cipher_suites: "ECDHE-RSA-AES256-GCM-SHA384"
            cipher_suites: "ECDHE-ECDSA-AES128-SHA"
            cipher_suites: "ECDHE-RSA-AES128-SHA"
            cipher_suites: "ECDHE-ECDSA-AES256-SHA"
            cipher_suites: "ECDHE-RSA-AES256-SHA"
            cipher_suites: "AES128-GCM-SHA256"
            cipher_suites: "AES256-GCM-SHA384"
            cipher_suites: "AES128-SHA"
            cipher_suites: "AES256-SHA"
          }
          alpn_protocols: "h2"
          alpn_protocols: "http/1.1"
        }
      }
    }
  }
}
track_cluster_stats {
  request_response_sizes: true
}

This config works fine with explicit http2 but changing it to auto breaks. I've tried removing the alpn_protocols but with no luck. Am I missing something? I tried digging into the code but it seems that the validation thinks my socket doesn't support alpn but it should.

[bbassingthwaite]

Changing from transport_socket_matches to transport_socket fixes my issue.

[mattklein123]

Changing from transport_socket_matches to transport_socket fixes my issue.

Hmm, it seems like that might be a bug but I would need to look at the code. The issue is probably that if there is a matching system the config can't verify what will end up getting matched so it can verify that it supports ALPN. In your case if you only have a single match I think that is the right thing to do.

[noah8713]

Changing from transport_socket_matches to transport_socket fixes my issue.

This is being generated from istio in our case when we define the filter as we dont specify transport_socket as it always selects socket match explicitly.

transportSocketMatches:
  - match:
      tlsMode: istio
    name: tlsMode-istio
    transportSocket:
      name: envoy.transport_sockets.tls
      typedConfig:
        '@type': type.googleapis.com/envoy.extensions.transport_sockets.tls.v3.UpstreamTlsContext
        commonTlsContext:
          alpnProtocols:
          - istio-peer-exchange
          - istio
          combinedValidationContext:

[bbassingthwaite]

This issue also manifests itself when wrapping a TLS socket with PROXY protocol, and I think the tap socket also same issue.

[noah8713]

@mattklein123 Please advice; what is the proposal/suggestion here to move forward for changes/fix further when applying to multiple clusters?
May be even have a regex support for cluster match as that would solve problem without using auto_config with alpn as a pre-req or we fix auto_config with alpn settings in general for transport_socket_matches.

[alyssawilk]

given that auto_config only works for transport sockets with ALPN I'm not convinced allowing it for transport socket match is going to be safe. We could arguably change the config validation to runtime failures, but I don't think that would be desireable for the common case.
The tap and proxy proto cases do seem buggy - they both use passthrough socket which doesn't actually return ALPN support based on the wrapped socket. I'm happy to fix that issue now.

[alyssawilk]

OK for the matcher case, I think the problem is actually the inverse - the matcher ALPN is not considered at all, only the "default socket config". I'll go fix this. This makes the validation more strict rather than more permissive and will not help.

Reading ClusterImplBase though, it looks like the default socket matcher is always config.transport_socket() and if no transport socket is configured, it defaults to a raw buffer socket. Arguably if your matcher config has its own catch-all such that the default will never be used we could allow auto_config but that's complicated enough I'm not inclined to implement it. I think there ought to instead be a way for Istio to override the default socket if there isn't already (cc @lizan and @lambdai for that)

[noah8713]

OK for the matcher case, I think the problem is actually the inverse - the matcher ALPN is not considered at all, only the "default socket config". I'll go fix this. This makes the validation more strict rather than more permissive and will not help.

Reading ClusterImplBase though, it looks like the default socket matcher is always config.transport_socket() and if no transport socket is configured, it defaults to a raw buffer socket. Arguably if your matcher config has its own catch-all such that the default will never be used we could allow auto_config but that's complicated enough I'm not inclined to implement it. I think there ought to instead be a way for Istio to override the default socket if there isn't already (cc @lizan and @lambdai for that)

Thanks. So it seems transportSocket under transportSocketMatches can now be read correctly with this fix at-least ; pending tests on istio for this.

However for second match for raw socket ,

- match: {}
    name: tlsMode-disabled
    transportSocket:
      name: envoy.transport_sockets.raw_buffer

are you proposing to even skip/over-ride rawsocket/default injectiom from istio itself so that we can leverage auto_config transportsocket from transportmatches or rather set the ones under transportmatches as default from istio?
cc @howardjohn too

[howardjohn]

are you proposing to even skip/over-ride rawsocket/default injectiom from istio itself so that we can leverage auto_config transportsocket from transportmatches or rather set the ones under transportmatches as default from istio?

I don't think Istio is likely to make changes to our core logic to support this use case as we explicitly plan to use HTTP2 everywhere in the very near future, which means no more header case preservation.

May be even have a regex support for cluster match as that would solve problem without using auto_config with alpn as a pre-req or we fix auto_config with alpn settings in general for transport_socket_matches.

Please keep Istio discussions out of Envoy issues, it adds confusion and wastes Envoy maintainers time.

[noah8713]

are you proposing to even skip/over-ride rawsocket/default injectiom from istio itself so that we can leverage auto_config transportsocket from transportmatches or rather set the ones under transportmatches as default from istio?

I don't think Istio is likely to make changes to our core logic to support this use case as we explicitly plan to use HTTP2 everywhere in the very near future, which means no more header case preservation.

Thanks @howardjohn : Noted so that we can plan accordingly on our side.

May be even have a regex support for cluster match as that would solve problem without using auto_config with alpn as a pre-req or we fix auto_config with alpn settings in general for transport_socket_matches.

Please keep Istio discussions out of Envoy issues, it adds confusion and wastes Envoy maintainers time.

Ack. Yes. Its more generic/healthy conversation as to what layer to propagate the changes to as we have case preserve for http1 which is valid case and some services will continue to use so; more of how vs who/what layer should do it.

[bbassingthwaite]

For users who want to use both transport_socket_matches and auto_config. Could envoy not validate that all sockets provided in transport_socket_matches supports ALPN? That would be sufficient to support our use case.

[alyssawilk]

It's possible to use transport_socket_matches and auto_config today.
I think what you're asking is "for those of us who want to use auto_config, transport_socket_matches, and have some of the transport_socket_matches not use ALPN can we allow that?"
I think it would help me to understand why you'd want non-ALPN transport sockets for an auto_config cluster, and what you would want the cluster to do if a non-ALPN socket were selected for an HTTP route?

If you have a reasonable use case I think we'd be up for supporting it but

1. I would not want it on by default (though we can ask another maintainer for a second opinion)
2. whoever implemented it would have to add tests for all upstream alpn-compatible sockets operating with a non-ALPN transport socket to make sure they failed gracefully (as it would be a new code path exercisable in production).

I don't have cycles for (2) but if you find someone who would pick this up I can give them tips for how to implement and test.

[bbassingthwaite]

It's possible to use transport_socket_matches and auto_config today.

I believe this says otherwise or are you suggesting in addition to using transport_socket_matches, you also have to supply transport_socket to get around the validation? I was hoping that you could use transport_socket_matches without transport_socket and if you did that, all of the sockets provided in that list would need to support ALPN, but only if you chose AUTO.

I think what you're asking is "for those of us who want to use auto_config, transport_socket_matches, and have some of the transport_socket_matches not use ALPN can we allow that?"

Sorry I wasn't clear enough but that isn't what I was suggesting. I don't have a use case for needing to use auto without an ALPN socket.

[alyssawilk]

Ah, gotcha. I had misunderstood, thanks for making your request more clear :-)

If you look here: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/cluster/v3/cluster.proto

"If an endpoint metadata’s value under envoy.transport_socket_match does not match any TransportSocketMatch, socket configuration fallbacks to use the tls_context or transport_socket specified in this cluster."

If you don't configure a transport_socket, it defaults to raw_buffer, so the configuration is guarding you here against missing metadata, and your "ALPN" request going out in the clear where protocol can't be negotiated. Your "default" match is not really a default unless you clone the config into the transport_socket in that cluster, because transport_socket is always the default even if it's not explicitly configured :-(

[bbassingthwaite]

If a socket match with empty match criteria is provided, that always match any endpoint. For example, the “defaultToPlaintext” socket match in case above.

This is also from the docs :)

If an endpoint metadata’s value under envoy.transport_socket_match does not match any TransportSocketMatch

This makes it seem that if someone configures transport_socket_matches without an empty match, it would default to the transport_socket which makes sense IMO.

In the our use case, since we do supply a transport socket with an empty match to transport_socket_matches or a "default", the transport_socket never gets used.

[bbassingthwaite]

Was just thinking on this some more. We can move away from an empty match and rely on no matches defaulting to transport_socket.

It does seems like a mistake to have two ways to default a transport socket and users should opt to not use the empty match and instead rely on the default being transport_socket. You might consider deprecating that feature but it may not be worth the effort. Thanks for the help @alyssawilk and the speedy fix done in #19281. 🎉

[noah8713]

It's possible to use transport_socket_matches and auto_config today. I think what you're asking is "for those of us who want to use auto_config, transport_socket_matches, and have some of the transport_socket_matches not use ALPN can we allow that?" I think it would help me to understand why you'd want non-ALPN transport sockets for an auto_config cluster, and what you would want the cluster to do if a non-ALPN socket were selected for an HTTP route?

Thanks . Because
as per alpn bool in code , it indicated that it always reads default raw socket vs the one in transportsocketmatches as alpn is always set to true and doesnt have over-ride option. Hence, raised if we can disabling this validation would fix the issue and now the subsequent conversation made it clear on details/issue.
May be we can reframe the statement as @bbassingthwaite mentioned too to make auto_config read transport_socket_matches correctly and skip erroring on raw socket as they will never have alpn.
Hence, the problem still stays as with this bare minimum config below using case preserve with auto_config, envoy(crashes) keeps rejecting new config and fails continuously even using it against a single cluster match.

- applyTo: CLUSTER
    match:
      cluster:
        name: 'outbound|8000||foo.bar'
    patch:
      operation: MERGE
      value:
        typed_extension_protocol_options:
          envoy.extensions.upstreams.http.v3.HttpProtocolOptions:
            "@type": type.googleapis.com/envoy.extensions.upstreams.http.v3.HttpProtocolOptions
            commonHttpProtocolOptions:
              idleTimeout: 60s
            auto_config:
              http_protocol_options:
                header_key_format:
                  stateful_formatter:
                    name: preserve_case  # preserve header case for response from backend service
                    typed_config:
                      "@type": type.googleapis.com/envoy.extensions.http.header_formatters.preserve_case.v3.PreserveCaseFormatterConfig

Hence, may be we can fix this bug in the first place. Since default raw socket is always there, if match entry as null{} under transportsocketmatches,

- match: {}
    name: tlsMode-disabled
    transportSocket:
      name: envoy.transport_sockets.raw_buffer

when using auto_config skip erroring (or may be warn vs exception) since non-empty match exists which has alpn details as per sample config dump. If alpn settings are not detected in any layer, error out. That way as per autoconfig usage in docs, upstream cluster with h2 will never react to case preserve as it doesnt exist for h2.

[alyssawilk]

oh interesting, I'd missed that (scanning too fast).
Looking at the code, it's true but really not clear. TransportSocketMatcherImpl always takes a default match as part of the constructor, the default match is created from transport_socket, and explicit hard-coded to the raw socket if transport_socket is not present.
TransportSocketMatcherImpl::resolve returns a match based on metadata, and if there's no match returns that "default raw" default factory. It's only poking into a third file and looking at Metadata::metadataLabelMatch that you can see if there's no matches, that it will always return true, so always match, so never return that default.

All of which is to say I agree with the above - it's way confusing having two ways to configure defaults, especially given one of them will create an unconfigured and unnecessary socket factory which will never be used. I would say given all the above it would make sense for matchers with a default match to not consider the socket_factory's ALPN-status but really what should happen is that whole section of code should be refactored to not create the "default socket factory" in the case where 1) it's not the default and 2) it can't actually be used.

[alyssawilk]

(and/or clean up the APIS so there's not 2 ways to configure defaults)
Anyway, marking as help wanted as I think there's work worth doing here (even if I still sadly can't sign on to do it)

[nezdolik]

Hi @alyssawilk, i can work on this.