afa78d7 update introduces a bug

[lxhao61]

afa78d7
This update introduces a bug that causes SNI configuration to not work properly again, specifically the abc.caddy website cannot be accessed, the complete example is as follows:

{
	layer4 {
		:443 {
			@abc tls sni abc.caddy
			route @abc {
				proxy {
					upstream 127.0.0.1:5443
					proxy_protocol v1
				}
			}
			route {
				proxy {
					upstream 127.0.0.1:7443
					proxy_protocol v1
				}
			}
		}
		udp/:443 {
			route {
				proxy udp/127.0.0.1:7443
			}
		}
	}

	email your@email.com

	servers 127.0.0.1:5443 {
		listener_wrappers {
			proxy_protocol
			tls
		}
		protocols h1 h2
	}

	servers 127.0.0.1:7443 {
		listener_wrappers {
			proxy_protocol
			tls
		}
		protocols h1 h2 h3
	}
}

abc.caddy:5443 {
	bind 127.0.0.1
	file_server {
		root /var/www/html
	}
}

def.caddy:7443 {
	bind 127.0.0.1
	header Alt-Svc "h3=\":443\"; ma=2592000"
	file_server {
		root /var/www/html
	}
}

Note: abc.caddy supports HTTPS and HTTP/2. def.caddy supports HTTPS, HTTP/2, and HTTP/3.

[mholt]

Oh, thanks for opening an issue. Maybe @WeidiDeng can look at it when he has a chance

[WeidiDeng]

I thought about this case before. Sometimes there's a fallback route that doesn't require any matchers. You need to add another sni matcher in this case.

Here is the detailed explanation:

Before, caddy-l4 let the connection go through the matchers one by one. If one matcher blocks, the rest of the matchers won't run. f049165 made the matching async so when a matcher can't block matching because there isn't enough data.

However, the initial implementation assumes the client will first send some data that can be used for matching, and it breaks some of the most simple use cases, i.e. packet forwarding without any matchers, #212, 228. pr 229 will introduce the same behavior.

So now, all connections will go through the matcher loop without being read from. And in your case, the fallback route doesn't require any matcher, so it's selected by default. And the connection is not given a chance to be read.

Honestly, I don't know what's best to do in this case. If users want to multiplex different kinds of connections, the matchers should be made explicit.

[vnxme]

@WeidiDeng To be precise, do you mean the Caddyfile above has to be adjusted in the following way?

			@def tls sni def.caddy
			route @def {
				proxy {
					upstream 127.0.0.1:7443
					proxy_protocol v1
				}
			}

I didn't really got the point about the fallback routes. In which cases will they work? Or have we completely dropped the fallback routes support as a part of afa78d7? It would be great if you could share some Caddyfile examples to illustrate your vision.

[WeidiDeng]

What I mean about fallback routes is in the Caddyfile that matches all connections not matched by @abc tls sni abc.caddy, not the one in the commits which can't be customized. There is no support for user defined fallback routes.

@vnxme The modification is correct.

[lxhao61]

@WeidiDeng After reading your explanation and the official SNI example, combined with my actual needs, the example is adjusted as follows:

{
	layer4 {
		:443 {
			@abc tls sni abc.caddy
			route @abc {
				proxy {
					upstream 127.0.0.1:5443
					proxy_protocol v1
				}
			}
			@def tls
			route @def {
				proxy {
					upstream 127.0.0.1:7443
					proxy_protocol v1
				}
			}
		}
		udp/:443 {
			route {
				proxy udp/127.0.0.1:7443
			}
		}
	}

	email your@email.com

	servers 127.0.0.1:5443 {
		listener_wrappers {
			proxy_protocol
			tls
		}
		protocols h1 h2
	}

	servers 127.0.0.1:7443 {
		listener_wrappers {
			proxy_protocol
			tls
		}
		protocols h1 h2 h3
	}
}

abc.caddy:5443 {
	bind 127.0.0.1
	file_server {
		root /var/www/html
	}
}

def.caddy:7443 {
	bind 127.0.0.1
	header Alt-Svc "h3=\":443\"; ma=2592000"
	file_server {
		root /var/www/html
	}
}

Problem solved, thanks!

[ydylla]

So now, all connections will go through the matcher loop without being read from. And in your case, the fallback route doesn't require any matcher, so it's selected by default. And the connection is not given a chance to be read.
Honestly, I don't know what's best to do in this case. If users want to multiplex different kinds of connections, the matchers should be made explicit.

@WeidiDeng @mholt
With #229 this config should still have worked. Because it uses a different lazy prefetch mechanism. Instead of trying all routes without data in the first iteration, the first matcher that needs data triggers the prefetch. The current behavior favors routes without matchers, one could argue with this it also does not strictly follow the desired matching order specified in the config. But I also just noticed this after seeing the issues. I am pretty sure the cause for #238 is also the preference of routes without matchers and not the vars replacement issue.

[WeidiDeng]

I see. So if server speaks first in some protocols, it will still break if trying to multiplex this type of protocols with other protocols that clients sends data first. But then people shouldn't try to multiplex in this case. Is that right @ydylla ?

[ydylla]

So if server speaks first in some protocols, it will still break if trying to multiplex this type of protocols with other protocols that clients sends data first. But then people shouldn't try to multiplex in this case. Is that righ

Yes, mixing server speaks first and matching on client sent data is not really possible. We can not distinguish if the client did not send enough data yet or will never do so because the server speaks first. Maybe with some complicated timeout code, but that would also always artificially delay the server speaks first path. I think this was never possible with layer4 even with the old matching code.

[mholt]

(Sorry been slow to reply!)

Server-speaks-first protocols are something I hadn't considered.

I guess if we try calling Read from the client in those situations, we won't get an instant return of 0 bytes, instead it'll block forever, is that right?

If that's the case, maybe we need to advise against the use of matchers with server-speaks-first protocols in the documentation.