Skip to content

Run pre-TLS callbacks for cleartext listeners - #1003

Open
CodyPubNub wants to merge 2 commits into
cloudflare:mainfrom
CodyPubNub:pre-tls-callback-cleartext
Open

Run pre-TLS callbacks for cleartext listeners#1003
CodyPubNub wants to merge 2 commits into
cloudflare:mainfrom
CodyPubNub:pre-tls-callback-cleartext

Conversation

@CodyPubNub

Copy link
Copy Markdown
Contributor

Summary

  • invoke PreTlsProcess before both TLS and cleartext protocol handling
  • update its documentation to reflect cleartext support
  • add regression coverage for plain TCP listeners

Motivation

I ran into this while adding Proxy Protocol v2 support for AWS Network Load
Balancer target groups.

Some of our NLB listeners terminate TLS and forward cleartext HTTP with a PPv2
preamble to Pingora, while others retain TLS through to Pingora. We use the
same PreTlsProcess implementation for both paths so it can consume the
preamble and populate the original client and destination addresses before
HTTP or TLS processing begins.

Pingora currently invokes that callback only when its listener has TLS
configured. On the TLS-terminated NLB path, the Pingora listener is cleartext,
so the callback is skipped and the HTTP parser receives the binary PPv2 header
instead of the request line.

Invoking the existing callback before selecting the TLS or cleartext path lets
the same connection-preamble handler support both configurations without
introducing another hook.

Follow-up to #799. Refs #132.

API naming

The cleartext case came up during review of #799, where
post_l4_handshake was suggested as a name that would describe both TLS and
non-TLS listeners.

I kept the existing PreTlsProcess API in this PR to keep the implementation
focused, but that means its name no longer describes its full behavior. It also
means an existing callback configured on a mixed TLS/cleartext Listeners
collection would begin running for its cleartext endpoints.

Would maintainers prefer to:

  • rename the API now, accepting the source-level break;
  • retain the existing name and behavior change; or
  • introduce a backward-compatible alias or shim?

I am happy to adjust this before merge. I have avoided introducing a parallel
hook or additional feature gating until there is a preferred direction,
particularly since those choices can add permanent complexity or work to the
connection path for applications that do not opt in.

- process connection preambles before either TLS or cleartext protocols
- document the callback's cleartext behavior
- cover plain TCP listeners with a regression test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant