fix: properly handle HTTP/2 connection reuse

[haq204]

Description

When multiple domains use the same certificate (e.g. the server has a certificate that can be used for domain domain.com and subdomains a.domain.com and b.domain.com) and the server supports HTTP/2, the browser will reuse the same connection for requests to domain.com, a.domain.com, and b.domain.com.

In 1.14 this wasn't an issue because all virtual hosts lived on a single filter chain.

It's a bit different in 2.y and above. To increase host and route matching performance, virtual hosts were split across multiple filter chains, using SNI for server name matching if TLS is available. For the non-TLS case, all virtual hosts are still combined under one filter chain since hostname matching in the filter_chain_match isn't available in the cleartext case.

In more detail, let's say you have the following Hosts and TLSContext configuration:

apiVersion: getambassador.io/v3alpha1
kind: TLSContext
metadata:
  name: my-tls-context
  namespace: default
spec:
  secret: tls-cert
  hosts: ["example.com", "foo.example.com"]
---
apiVersion: getambassador.io/v3alpha1
kind: Host
metadata:
  name: basic-host-1
  namespace: default
spec:
  hostname: 'example.com'
  tlsSecret:
    name: tls-cert
  tlsContext: 
    name: my-tls-context
---
apiVersion: getambassador.io/v3alpha1
kind: Host
metadata:
  name: basic-host-2
  namespace: default
spec:
  hostname: 'foo.example.com'
  tlsSecret:
    name: tls-cert
  tlsContext: 
    name: my-tls-context

This results in the following filter chain configuration:

filter_chains:
- filter_chain_match:
    server_names: ["example.com"]
    transport_protocol: tls
  transport_socket:
    name: envoy.transport_sockets.tls
  filters:
  - name: envoy.filters.network.http_connection_manager
    typed_config:
    "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
    stat_prefix: ingress_https
    route_config:
      virtual_hosts:
      - domains:
        - "example.com"
        routes: []

- filter_chain_match:
    server_names: ["foo.example.com"]
    transport_protocol: tls
  transport_socket:
    name: envoy.transport_sockets.tls
  filters:
  - name: envoy.filters.network.http_connection_manager
    typed_config:
    "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
    stat_prefix: ingress_https
    route_config:
      virtual_hosts:
      - domains:
        - "foo.example.com"
        routes: []

Each Host is added as a virtual_host in separate filter chains.

This can an issue when reusing TLS connections. A connection is established to the virtual_host that is first visited (e.g. foo.example.com) but that same connection will be reused when visiting the other virtual_host (example.com). Since the virtual_hosts live on separate filter chains, this results in a 404 because it won't be able to find foo.example.com in the route table.

The Fix

This updates so that Hosts sharing the same TLSContext will be coalesced into a single filter chain with both hosts added as server names for the filter_chain_match.

With the same Hosts and TLSContext configuration above, now the generated filter chain roughly looks like:

filter_chains:
- filter_chain_match:
    server_names: ["example.com", "foo.example.com"]
    transport_protocol: tls
  transport_socket:
    name: envoy.transport_sockets.tls
  filters:
  - name: envoy.filters.network.http_connection_manager
    typed_config:
    "@type": type.googleapis.com/envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager
    stat_prefix: ingress_https
    route_config:
    virtual_hosts:
      - domains:
        - "example.com"
        routes: []
      - domains:
        - "foo.example.com"
        routes: []

This allows for HTTP/2 connection reuse in the browser.

If a Host and TCPMapping share the same TLSContext then the existing behavior is retained, a separate filter chain is created for each. Same for the inline and implicit tls cases. i.e we do not support connection reuse with Host.tls or if only Host.tlsSecret is provided without a TLSContext.

Misc

• Added make targets format/python, lint/python to run just the python linters and formatters
• Refactored the diagd listener unit tests. Some of those tests run various test cases but because they are all under a single test function, pytest will see it as just one test. This can make debugging failed tests a little cumbersome when trying to figure out which specific test case failed. So refactor to use pytest's parametrize decorator to parametrize those test functions for all of the test cases. This lets pytest know about each test case and will treat them as sub-tests under the main unit test. This also makes it possible to run a specific test case from pytest e.g. pytest tests/unit/test_listener.py -k '<unit-test-name>[subtest-id]'.

Before

pytest -v tests/unit/test_listener.py -k "test_listener_filterchain"           
========================================================================================= test session starts ==========================================================================================
platform linux -- Python 3.10.9, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 -- /home/hamzah/.virtualenvs/ambassador/bin/python
cachedir: .pytest_cache
rootdir: /home/hamzah/src/emissary-ingress/emissary, configfile: pytest.ini
plugins: cov-4.0.0, rerunfailures-11.1.1
collected 6 items / 5 deselected / 1 selected                                                                                                                                                          

tests/unit/test_listener.py::TestListener::test_listener_filterchain_vhost_generation PASSED 

After

pytest -v tests/unit/test_listener.py -k "test_listener_filterchain"
======================================================== test session starts ========================================================
platform linux -- Python 3.10.9, pytest-6.2.5, py-1.11.0, pluggy-1.0.0 -- /home/hamzah/.virtualenvs/ambassador/bin/python
cachedir: .pytest_cache
rootdir: /home/hamzah/src/emissary-ingress/emissary, configfile: pytest.ini
plugins: cov-4.0.0, rerunfailures-11.1.1
collected 22 items / 14 deselected / 8 selected                                                                                     

tests/unit/test_listener.py::TestListener::test_listener_filterchain_generation[host_missing_tls] PASSED                      [ 12%]
tests/unit/test_listener.py::TestListener::test_listener_filterchain_generation[no_host] PASSED                               [ 25%]
tests/unit/test_listener.py::TestListener::test_listener_filterchain_generation[prefix_wildcard_and_hostname_with_port] PASSED [ 37%]
tests/unit/test_listener.py::TestListener::test_listener_filterchain_generation[two_hosts_using_different_tls_contexts] PASSED [ 50%]
tests/unit/test_listener.py::TestListener::test_listener_filterchain_generation[host_and_tcp_mapping_using_different_tls_contexts] PASSED [ 62%]
tests/unit/test_listener.py::TestListener::test_listener_filterchain_generation[two_hosts_sharing_tls_context] PASSED         [ 75%]
tests/unit/test_listener.py::TestListener::test_listener_filterchain_generation[host_and_tcp_mapping_sharing_tls_context] PASSED [ 87%]
tests/unit/test_listener.py::TestListener::test_listener_filterchain_generation[two_hosts_sharing_tls_context_missing_host] PASSED [100%]

Related Issues

fixes #2403

Testing

Added some additional unit test cases testing various scenarios to capture as many corner cases as I can and to ensure backwards compatibility with the existing behavior (hosts sharing tls context, host & tcpmapping sharing tls context, hosts/tcpmappings using different tls contexts, etc.)

Checklist

• Does my change need to be backported to a previous release?

  • What backport versions were discussed with the Maintainers in the Issue?

• I made sure to update CHANGELOG.md.

  Remember, the CHANGELOG needs to mention:

  • Any new features
  • Any changes to our included version of Envoy
  • Any non-backward-compatible changes
  • Any deprecations

• This is unlikely to impact how Ambassador performs at scale.

  Remember, things that might have an impact at scale include:

  • Any significant changes in memory use that might require adjusting the memory limits
  • Any significant changes in CPU use that might require adjusting the CPU limits
  • Anything that might change how many replicas users should use
  • Changes that impact data-plane latency/scalability

• My change is adequately tested.

  Remember when considering testing:

  • Your change needs to be specifically covered by tests.
    • Tests need to cover all the states where your change is relevant: for example, if you add a behavior that can be enabled or disabled, you'll need tests that cover the enabled case and tests that cover the disabled case. It's not sufficient just to test with the behavior enabled.
  • You also need to make sure that the entire area being changed has adequate test coverage.
    • If existing tests don't actually cover the entire area being changed, add tests.
    • This applies even for aspects of the area that you're not changing – check the test coverage, and improve it if needed!
  • We should lean on the bulk of code being covered by unit tests, but...
  • ... an end-to-end test should cover the integration points

• I updated DEVELOPING.md with any any special dev tricks I had to use to work on this code efficiently.

• The changes in this PR have been reviewed for security concerns and adherence to security best practices.