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

Implement issue #6296 passing FDs / socket activation #6507

Closed
wants to merge 0 commits into from

Conversation

MayCXC
Copy link
Contributor

@MayCXC MayCXC commented Aug 11, 2024

This PR enables socket activation by adding a global server option that can be used to open the fd of an existing socket, instead of binding a new socket, for any network address. Example Caddyfile:

{
	servers :80 {
		socket {env.CADDY_HTTP_FD}
		protocols h1
	}
	servers :443 {
		socket {env.CADDY_HTTPS_FD}
		protocols h1 h2
	}
	servers udp/:443 {
		socket {env.CADDY_HTTP3_FD}
		protocols h3
	}
	auto_https disable_redirects
	admin off
}

http://localhost {
	bind tcp/
	log
	respond "Hello, HTTP!"
}

https://localhost {
	bind tcp/
	bind udp/
	log
	respond "Hello, HTTPS!"
}

@CLAassistant
Copy link

CLAassistant commented Aug 11, 2024

CLA assistant check
All committers have signed the CLA.

@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 11, 2024

link to issue: #6296

@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 11, 2024

caddy adapt produces the expected config, and allows env placeholders in server/socket

@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 12, 2024

It works for a local HTTP and HTTPS connections with this Caddyfile:

{
	servers :80 {
		socket {env.CADDY_HTTP_FD}
		protocols h1
	}
	servers :443 {
		socket {env.CADDY_HTTPS_FD}
		protocols h1 h2
	}
#	servers udp/:443 {
#		socket {env.CADDY_HTTP3_FD}
#		protocols h3
#	}
	auto_https disable_redirects
	admin off
}

http://localhost {
	bind tcp/
	log
	respond "Hello, HTTP!"
}

https://localhost {
	bind tcp/
#	bind udp/
	log
	respond "Hello, HTTPS!"
}

this caddy-socket-activate script:

#!/usr/bin/env -S sh -e
CADDY_HTTP_FD=3 CADDY_HTTPS_FD=4 exec "${@}"

and this test command: systemd-socket-activate -l 0.0.0.0:80 -l 0.0.0.0:443 ./caddy-socket-activate ./caddy run -c ./Caddyfile

I have not gotten h3 to work alongside h1 and h2 with systemd-socket-activate -l 0.0.0.0:80 -l 0.0.0.0:443 -l 0.0.0.0:443 -d and the udp bind directive enabled. I think systemd-socket-activate is part of the issue, because I can only get it to activate with UDP when only the udp socket is listening. I'll also have to make some changes to app.Start() to allow for udp addresses in Server.[]Listen.

@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 13, 2024

Ok, I think it works with h3 now. I split Server.Protocols into a parallel list of lists, for each listen address for each server. h3 is still enabled for TCP sockets by binding to the UDP socket with the same address and port, but it can now be disabled per address rather than per server. It uses the same socket activation fd global options as h1 and h2, so it can use systemd socket activation with ListenDatagram. here is the config I tested with:

Caddyfile:

{
	servers :80 {
		socket {env.CADDY_HTTP_FD}
		protocols h1
	}
	servers :443 {
		socket {env.CADDY_HTTPS_FD}
		protocols h1 h2
	}
	servers udp/:443 {
		socket {env.CADDY_HTTP3_FD}
		protocols h3
	}
	auto_https disable_redirects
	admin off
}

http://localhost {
	bind tcp/
	log
	respond "Hello, HTTP!"
}

https://localhost {
	bind tcp/
	bind udp/
	log
	respond "Hello, HTTPS!"
}

caddy.service:

[Unit]
Description=Caddy
Documentation=https://caddyserver.com/docs/
After=network.target network-online.target
Requires=network-online.target

[Service]
Type=notify
User=caddy
Group=caddy
Environment=CADDY_HTTP_FD=3 CADDY_HTTPS_FD=4 CADDY_HTTP3_FD=5
ExecStart=/path/to/caddy run --environ --config /path/to/Caddyfile
ExecReload=/path/to/caddy reload --config /path/to/Caddyfile --force
TimeoutStopSec=5s
LimitNOFILE=1048576
PrivateTmp=true
ProtectSystem=full
AmbientCapabilities=CAP_NET_ADMIN CAP_NET_BIND_SERVICE

[Install]
WantedBy=multi-user.target

caddy.socket:

[Socket]
ListenStream=80
ListenStream=443
ListenDatagram=443

[Install]
WantedBy = sockets.target

I need to double check that the h3 socket is actually working, because my browser doesn't like it. But http/https on h1/h2 is definitely working for any combination of binds, tls, inet and unix domain sockets I have tried.

@MayCXC MayCXC marked this pull request as ready for review August 13, 2024 11:40
modules/caddyhttp/app.go Outdated Show resolved Hide resolved
caddy.go Outdated
Comment on lines 878 to 886
// sliceContains returns true if needle is in haystack.
func SliceContains(haystack []string, needle string) bool {
for _, s := range haystack {
if s == needle {
return true
}
}
return false
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need this function be exposed outside Caddy. Let's move it to internal. Better now than later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we could use this new generic function: https://pkg.go.dev/slices#Contains

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, looks like slices is used throughout caddy already. I'll replace SliceContains with slices.Contains.

Comment on lines 218 to 219
// Default: `[[h1 h2 h3]...]`
Protocols [][]string `json:"protocols,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a breaking change. It breaks existing configuration.

Copy link
Contributor Author

@MayCXC MayCXC Aug 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, it is hard to determine how to create each listener if they all share one list of protocols for each server. One alternative could be to leave Protocols alone and add a new list, ListenProtocols, which contains a sublist of Protocols for each address in Listen.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can't be changed in this way; but do we need to at all? Protocols are more of a server configuration than having to do with the individual listeners.

Copy link
Contributor Author

@MayCXC MayCXC Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the main case it handles is that with h3 enabled, a server with a tcp listen address will also bind an additional socket to the udp address with the same host:port. but if you are using socket activation, you will have two separate fds for that tcp and udp address, and not want one to be bound implicitly. so separating the protocols lets you disable h3 for the tcp address and enable h3 for the udp address explicitly. and with the change you can enable h1/h2 for a unix socket and h3 for a unixgram socket on the same server, which was not allowed before even though caddy can handle it just fine.

so for these cases, the network of an individual listener does change how its protocols are handled quite a bit. currently, the only place where Server.protocol is called to do something other than set up a listener, is to check if h2 was enabled without h1.

@MayCXC MayCXC requested review from mholt and mohammed90 August 13, 2024 13:42
@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 14, 2024

ok, this puts the config provisioning/defaults for Server.Socket and Server.Protocols where I think they need to be. I can make Server.Protocols a []string again to lessen the breaking config change, but the individual protocol global server options will still have to be stored separately somewhere, say Server.ListenProtocols. This is for fd passing, I need a way to tell apart an h1/h2 TCP socket with an implicit h3 UDP socket bound to the same host:port, vs. two separate h1/h2 TCP and h3 UDP sockets each with their own fds. This way h3 can still be enabled for a TCP socket to indicate the first case, but it can be disabled if that TCP socket is passed an fd so it isn't bound twice.

@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 16, 2024

is it ok to change the shape of Server.Protocols? it just isn't very straightforward to enable h3 for a whole server, that can bind to addresses with both tcp and udp networks. each server needs to know which http should serve which socket with which fd, not the union of protocols enabled for all its listen addresses. the other change this causes is that an h3 UDS socket will have to use the correct unixgram network in its listen address, rather than unix. but I think this is expected behavior.

the best way to do this is probably to add Server.Bind as a []struct{Listen: string, Socket: *string, Protocols: []string}, and just remove/ignore Server.Listen and Server.Protocols. or they could be provisioned together and converted to a Server.Bind array, where each address has the same protocols, and display a deprecation warning.

@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 17, 2024

another way to approach config would be to add protocol flags to the bind directive, bind h1+h2+tcp/ h3+udp/, with h1+h2+h3+ as a default determined by Server.protocols. This way the shape of Server.Protocols can be left alone, and its behavior stays the same. Then I could move socket addr fd to a toplevel global config, to differentiate it from servers and emphasize that a server can have its config affected by more than one socket directive, and not just the first that matches. This is more precise as well, because one site block can potentially serve a listen address with h1 while another serves it with h1+h2.

I like this config more, so I'll update the PR with it if we all agree on how bind can be changed instead:

  • bind tcp/ unchanged, implicitly bind a udp address if h3 is in Server.protocols and port is not the default http port, then serve it with h3
  • bind h1+h2+tcp/ do not implicitly bind a udp address
  • bind h3+udp/ this explicitly adds a udp address to Listen, so that it can receive a fd from the socket udp/:443 fd global option, then serve it with h3, etc.

and this also allows a h1+h2+h3 site to be served entirely with uds's:

  • bind h1+h2+unix/path/to/stream/uds
  • bind h3+unixgram/path/to/dgram/uds

@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 17, 2024

here is an example Caddyfile:

{
	socket :80 {env.CADDY_HTTP_FD}
	socket :443 {env.CADDY_HTTPS_FD}
	socket udp/:443 {env.CADDY_HTTP3_FD}

	servers :80 {
		# protocols are not needed here for socket activation, they set Sever.protocols to a []string like before
	}
#	servers :443 {
#		# ...
#	}
#	servers udp/:443 {
#		# ...
#	}
	auto_https disable_redirects
	admin off
}

http://localhost {
	bind tcp/
	log
	respond "Hello, HTTP!"
}

https://localhost {
	bind h1+h2+tcp/ # overrides Server.protocols with ["h1","h2"] for listener config
	bind h3+udp/ # same but with ["h3"]
	log
	respond "Hello, HTTPS!"
}

@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 17, 2024

or I could add a new directive serve instead of changing bind, then do serve tcp/ h1 h2 instead of bind h1+h2+tcp/.

@mholt
Copy link
Member

mholt commented Aug 23, 2024

Did you mean to close this? Sorry I've been absent. Very busy with a lot to catch up on.

@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 23, 2024

Did you mean to close this? Sorry I've been absent. Very busy with a lot to catch up on.

no problem. I'm going to reopen or make a new PR with a separate listen_protocols config key, an optional block after bind to specify which protocols to serve, and a separate socket global option. This will have the same functionality as this PR, but avoid making breaking config changes. It might be ready today, the branch is here: master...MayCXC:caddy:master

@mholt
Copy link
Member

mholt commented Aug 23, 2024

Ah neat. Sounds good, will be happy to review when you're ready!

@MayCXC
Copy link
Contributor Author

MayCXC commented Aug 27, 2024

closed in favor of #6543
this PR was for master...MayCXC:caddy:socket-activation-with-global-server-options

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.

4 participants