Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tcpmappings: invalid envoy configuration generated when applying non-tls TCPMapping on listener with HTTP #4825

Open
LanceEa opened this issue Feb 2, 2023 · 0 comments
Labels
stale Issue is stale and will be closed t:bug Something isn't working

Comments

@LanceEa
Copy link
Contributor

LanceEa commented Feb 2, 2023

Describe the bug

When using TLS and SNI we support having a TCPMapping (TCP Proxy) configured on the same Listener (address/port) that we server HTTP traffic. Each unique SNI is given its own FilterChain and is able to act as either an L4 TCP Proxy or serve L7 HTTP connections. In this scenario, if there is a conflict then TCPMappings gets precedence and the Host are rejected with a notification.

In a cleartext scenario, this is not possible because Envoy does not have SNI to differentiate the incoming traffic. Therefore, if a user tries to apply a cleartext TCPMapping on the same Listener (address/port) as HTTP traffic then we should give the TCPMapping precedence and discard the Host for http with a notification. However, today if you were to try this we would still generate two Filter Chains for TCP and HTTP and both would have matching filter_chain_match which would cause Envoy to reject the configuration as invalid.

This makes logical sense that it wouldn't work but we shouldn't be generating invalid Envoy configuration and instead should be providing the user with the proper error notifications so that they can address discrepancies like this.

To Reproduce
Steps to reproduce the behavior:

  1. Follow Emissary-ingress Quick-start as outlined here:https://www.getambassador.io/docs/emissary/latest/tutorials/getting-started. This will produce a Listener that is server cleartext http.
  2. Apply a clear-text TCPMapping so that it is listening on TCP
---
apiVersion: getambassador.io/v3alpha1
kind: TCPMapping
metadata:
  name:  my-tcp-proxy
spec:
  port: 8080
  service: quote
  1. Check pod logs to see error message and invalid envoy configuration generated.

error adding listener '0.0.0.0:8080': filter chain 'httphost-shared' has the same matching rules defined as 'tcphost-GROUP: my-tcp-proxy'

Expected behavior

Emissary-ingress should generate valid Envoy configuration all the time. In this scenario we should give precedence to the TCPMapping and drop the HTTP Host with a notification to guide user.

This has to do with the fact that when we process a Listener and generate V3Chains we produce the following unique keys:

  • cleartext -> raw TCP
  • cleartext-{hostname} == per Host for http virtual_hosts - these get consolidated into single chain in later processing
  • tls-{sni} == mixed TCP w/TLS or HTTP w/TLS - tcp gets precedence if sni overlaps

We do check to see if there is a collision on the same V3Chain but since we store the cleartext TCP and cleartext HTTP in seperate keys the current checks we have don't catch this. Therefore, when processing http Host we need to add a check to see if there is an existing cleartext aka TCP already present and drop the host.

Versions (please complete the following information):

  • AES/Emissary-ingress v2.Y and v3.Y
@LanceEa LanceEa added the t:bug Something isn't working label Feb 2, 2023
@LanceEa LanceEa mentioned this issue Feb 2, 2023
6 tasks
@dosubot dosubot bot added the stale Issue is stale and will be closed label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issue is stale and will be closed t:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant