Implement issue #6296 passing FDs / socket activation #6573

MayCXC · 2024-09-15T04:20:55Z

third time's the charm ☘️

example Caddyfile:

{
	auto_https disable_redirects
	admin off
}

http://localhost {
	bind fd/{env.CADDY_HTTP_FD} {
		protocols h1
	}
	log
	respond "Hello, HTTP!"
}

https://localhost {
	bind fd/{env.CADDY_HTTPS_FD} {
		protocols h1 h2
	}
	bind fdgram/{env.CADDY_HTTP3_FD} {
		protocols h3
	}
	log
	respond "Hello, HTTPS!"
}

as seen above, two reserved networks fd and fdgram have been added. fd/3 creates a FileListener from fd 3, fdgram/4 creates a FilePacketConn from fd 4. this simplifies configuration compared to #6543 , but the "actual" host addresses are no longer visible. so, for health checks and the admin interface, fds are treated similarly to unix sockets.

this is a good thing i think, fds should be decoupled from the address they are bound to. separate config options can be added to override the origin address for health checks / the admin interface if needed.

link to issue: #6296

…ten logic

MayCXC · 2024-09-25T22:23:04Z

Sorry for the merge conflict. I'm about ready to review this one now 😅 I'll make sure it gets in the 2.9 beta 1.

no problem. the only conflict that this branch actually touched was the protocol function, i just pulled in everything else from upstream.

wplinge · 2024-09-26T11:27:38Z

I'm really looking forward to this one and have been trying it out.

I listen for multiple hosts so was trying to use default_bind instead. I'd normally assume I was holding it wrong and/or file another bug after this landed, but it led to a panic around HTTP/3 handling rather than a normal error so might be a more fundamental type problem.

With Caddyfile

{
	auto_https disable_redirects
	admin off
	default_bind fd/3 {
		protocols h1
	}
	default_bind fd/4 {
		protocols h1 h2
	}
	default_bind fdgram/5 {
		protocols h3
	}
}
https://localhost {
	log
	respond "Wibble"
}

I get the backtrace

Sep 26 12:21:17 sandbox caddy[131974]: panic: interface conversion: caddy.deletePacketConn is not net.Listener: missing method Accept
Sep 26 12:21:17 sandbox caddy[131974]:
Sep 26 12:21:17 sandbox caddy[131974]: goroutine 1 [running]:
Sep 26 12:21:17 sandbox caddy[131974]: github.com/caddyserver/caddy/v2/modules/caddyhttp.(*App).Start(0xc000bce300)
Sep 26 12:21:17 sandbox caddy[131974]:         github.com/caddyserver/caddy/[email protected]/modules/caddyhttp/app.go:533 +0x14f4
Sep 26 12:21:17 sandbox caddy[131974]: github.com/caddyserver/caddy/v2.run.func1({{0x2051c50, 0xc00088d9a0}, 0xc000b9f890, 0xc000e80180, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}, ...})
Sep 26 12:21:17 sandbox caddy[131974]:         github.com/caddyserver/caddy/[email protected]/caddy.go:420 +0x111
Sep 26 12:21:17 sandbox caddy[131974]: github.com/caddyserver/caddy/v2.run(0x1000100?, 0x1)
Sep 26 12:21:17 sandbox caddy[131974]:         github.com/caddyserver/caddy/[email protected]/caddy.go:436 +0x198
Sep 26 12:21:17 sandbox caddy[131974]: github.com/caddyserver/caddy/v2.unsyncedDecodeAndRun({0xc0001029a0, 0x156, 0x160}, 0x1)
Sep 26 12:21:17 sandbox caddy[131974]:         github.com/caddyserver/caddy/[email protected]/caddy.go:343 +0x145
Sep 26 12:21:17 sandbox caddy[131974]: github.com/caddyserver/caddy/v2.changeConfig({0x1af13ed, 0x4}, {0x1af8455, 0x7}, {0xc000102840, 0x156, 0x160}, {0x0, 0x0}, 0x1)
Sep 26 12:21:17 sandbox caddy[131974]:         github.com/caddyserver/caddy/[email protected]/caddy.go:234 +0x6b6
[... other functions that look like generic startup stuff ...]

Let me know if you'd prefer a separate issue (or different channel entirely) for this.

MayCXC · 2024-09-26T15:58:00Z

I'm really looking forward to this one and have been trying it out.

I listen for multiple hosts so was trying to use default_bind instead. I'd normally assume I was holding it wrong and/or file another bug after this landed, but it led to a panic around HTTP/3 handling rather than a normal error so might be a more fundamental type problem.

With Caddyfile

{
	auto_https disable_redirects
	admin off
	default_bind fd/3 {
		protocols h1
	}
	default_bind fd/4 {
		protocols h1 h2
	}
	default_bind fdgram/5 {
		protocols h3
	}
}
https://localhost {
	log
	respond "Wibble"
}

I get the backtrace

Sep 26 12:21:17 sandbox caddy[131974]: panic: interface conversion: caddy.deletePacketConn is not net.Listener: missing method Accept
Sep 26 12:21:17 sandbox caddy[131974]:
Sep 26 12:21:17 sandbox caddy[131974]: goroutine 1 [running]:
Sep 26 12:21:17 sandbox caddy[131974]: github.com/caddyserver/caddy/v2/modules/caddyhttp.(*App).Start(0xc000bce300)
Sep 26 12:21:17 sandbox caddy[131974]:         github.com/caddyserver/caddy/[email protected]/modules/caddyhttp/app.go:533 +0x14f4
Sep 26 12:21:17 sandbox caddy[131974]: github.com/caddyserver/caddy/v2.run.func1({{0x2051c50, 0xc00088d9a0}, 0xc000b9f890, 0xc000e80180, {0x0, 0x0, 0x0}, {0x0, 0x0, 0x0}, ...})
Sep 26 12:21:17 sandbox caddy[131974]:         github.com/caddyserver/caddy/[email protected]/caddy.go:420 +0x111
Sep 26 12:21:17 sandbox caddy[131974]: github.com/caddyserver/caddy/v2.run(0x1000100?, 0x1)
Sep 26 12:21:17 sandbox caddy[131974]:         github.com/caddyserver/caddy/[email protected]/caddy.go:436 +0x198
Sep 26 12:21:17 sandbox caddy[131974]: github.com/caddyserver/caddy/v2.unsyncedDecodeAndRun({0xc0001029a0, 0x156, 0x160}, 0x1)
Sep 26 12:21:17 sandbox caddy[131974]:         github.com/caddyserver/caddy/[email protected]/caddy.go:343 +0x145
Sep 26 12:21:17 sandbox caddy[131974]: github.com/caddyserver/caddy/v2.changeConfig({0x1af13ed, 0x4}, {0x1af8455, 0x7}, {0xc000102840, 0x156, 0x160}, {0x0, 0x0}, 0x1)
Sep 26 12:21:17 sandbox caddy[131974]:         github.com/caddyserver/caddy/[email protected]/caddy.go:234 +0x6b6
[... other functions that look like generic startup stuff ...]

Let me know if you'd prefer a separate issue (or different channel entirely) for this.

that for sure looks like an issue with this branch. thank you for testing it, I can take a look later today.

MayCXC · 2024-09-27T09:25:15Z

@wplinge HEAD treats default_bind like bind now, so you can add multiple options with protocol blocks like in your example. if you can try running it again, it should create the expected listeners.

@mholt this was an additional feature, but it is compatible with every existing Caddyfile and config JSON. I also added a clearer error message for the case in the log.

wplinge · 2024-09-27T09:49:31Z

@MayCXC Thanks, it works perfectly for me with that update. I'd noticed only one default_bind was being used after posting, but still slightly surprised it led to that failure mode.

MayCXC · 2024-09-27T19:04:26Z

@MayCXC Thanks, it works perfectly for me with that update. I'd noticed only one default_bind was being used after posting, but still slightly surprised it led to that failure mode.

great, yeah what happened was only that last default_bind would end up in the config, and the protocols sub option was ignored for it. so instead of working like bind, the default h1/h2 protos were served with your datagram fd, but it definitely should have worked the way you expected. b98bf1a adds a clearer error message for the failure mode you saw, similar to the one you see when you try to serve h3 with a stream fd.

mholt · 2024-09-27T22:34:38Z

Well, impressive.

This change is too big for me to fully understand and thoroughly review. I appreciate the field testing, and think we could benefit from more of it. So let's go ahead and merge this, if you're ready, so it goes in Caddy 2.9 beta releases.

MayCXC · 2024-09-27T22:52:05Z

thanks, and yes I'm ready now. I will be one of the beta testers with production traffic :)

mholt

🤞

francislavoie · 2024-10-01T02:16:49Z

Unfortunately I think this broke integration tests. The annoying thing is the integration tests are horribly flaky, but with this commit on master I get 100% fail rate on a dozen retries instead of a pass in two tries on the prior commit in master.

Specifically, this is the failing test:

--- FAIL: TestHTTPRedirectWrapperWithLargeUpload (0.01s)
    listener_test.go:74: failed to post: Post "https://localhost:9443": write tcp 127.0.0.1:33560->127.0.0.1:9443: write: connection reset by peer
    caddytest.go:134: ----------- failed with config -----------
        {
          "admin": {
            "listen": "localhost:2999"
          },
          "apps": {
            "http": {
              "http_port": 9080,
              "https_port": 9443,
              "servers": {
                "srv0": {
                  "listen": [
                    ":9443"
                  ],
                  "listener_wrappers": [
                    {
                      "wrapper": "http_redirect"
                    },
                    {
                      "wrapper": "tls"
                    }
                  ],
                  "routes": [
                    {
                      "handle": [
                        {
                          "handler": "subroute",
                          "routes": [
                            {
                              "handle": [
                                {
                                  "handler": "reverse_proxy",
                                  "upstreams": [
                                    {
                                      "dial": "127.0.0.1:42089"
                                    }
                                  ]
                                }
                              ]
                            }
                          ]
                        }
                      ],
                      "match": [
                        {
                          "host": [
                            "localhost"
                          ]
                        }
                      ],
                      "terminal": true
                    }
                  ]
                }
              }
            },
            "pki": {
              "certificate_authorities": {
                "local": {
                  "install_trust": false
                }
              }
            },
            "tls": {
              "automation": {
                "policies": [
                  {
                    "issuers": [
                      {
                        "module": "internal"
                      }
                    ]
                  }
                ]
              }
            }
          }
        }

MayCXC · 2024-10-01T04:33:15Z

Unfortunately I think this broke integration tests. The annoying thing is the integration tests are horribly flaky, but with this commit on master I get 100% fail rate on a dozen retries instead of a pass in two tries on the prior commit in master.

Specifically, this is the failing test:

[...]

investigating this, it looks like I broke http_redirect. in this Caddyfile based off of func setupListenerWrapperTest in listener_test.go, commenting it out lets my browser connect to https://localhost:9443, but leaving it uncommented shows me a ERR_HTTP2_PROTOCOL_ERROR :

{
	skip_install_trust
	admin localhost:2999
	http_port     9080
	https_port    9443
	local_certs
	servers :9443 {
		listener_wrappers {
#			http_redirect
#			tls
		}
	}
}

localhost {
	log
	respond "tlspls"
}

so next I will debug http_redirect to see what is going wrong.

MayCXC · 2024-10-01T05:11:20Z

the above config with http_redirect uncommented works with just app.go reset to https://github.com/caddyserver/caddy/blob/1a345b4fa620dfb0909a3b086bd76e35dfdbefa5/modules/caddyhttp/app.go , so the bug is in very likely in func Start . bisecting from here should be straightforward.

MayCXC · 2024-10-01T05:41:45Z

fix is here: #6599

MayCXC added 30 commits August 23, 2024 08:49

caddy adapt for listen_protocols

c1da1df

adapt listen_socket

8c5705b

allow multiple listen sockets for port ranges and readd socket fd lis…

071679e

…ten logic

readd logic to start servers according to listener protocols

e1e4a11

gofmt

256eee1

adapt caddytest

9d66463

Merge branch 'caddyserver:master' into master

152c42c

gosec

e20a504

fmt and rename listen to listenWithSocket

b34b11a

fmt and rename listen to listenWithSocket

6e9001d

more consistent error msg

db0d780

non unix listenReusableWithSocketFile

f4bdfd1

remove unused func

5b339c6

Merge branch 'master' into master

6db7264

doc comment typo

3ba23c7

Merge branch 'master' of github.com:MayCXC/caddy

646a0b3

nonosec

bce0058

commit

86c39ba

Merge branch 'master' into master

b006895

doc comments

dc89c77

Merge branch 'master' of github.com:MayCXC/caddy

8a9aed3

more doc comments

d6d6e6c

Merge branch 'master' into master

af576b1

Merge branch 'master' into master

8ba019f

Merge branch 'master' into master

891499b

comment was misleading, cardinality did not change

7217db8

Merge branch 'master' into master

7852daf

addressesWithProtocols

1086e61

Merge branch 'master' of github.com:MayCXC/caddy

d01cafc

update test

f7ea6cf

MayCXC added 2 commits September 25, 2024 20:03

key -> parsedKey

87ba4d0

osx

c078da7

MayCXC marked this pull request as draft September 26, 2024 17:52

MayCXC force-pushed the master branch from aade720 to c078da7 Compare September 27, 2024 08:49

MayCXC added 2 commits September 27, 2024 04:50

multiple default_bind with protocols

04bb55c

Merge branch 'master' into master

70e06c9

MayCXC marked this pull request as ready for review September 27, 2024 08:52

MayCXC added 2 commits September 27, 2024 05:23

check for h1 and h2 listener netw

b98bf1a

Merge branch 'master' of github.com:MayCXC/caddy

39d876a

Merge branch 'master' into master

5d2d245

mholt approved these changes Sep 30, 2024

View reviewed changes

mholt merged commit 4b1a9b6 into caddyserver:master Sep 30, 2024
33 checks passed

mholt added the feature ⚙️ New feature or request label Sep 30, 2024

mholt added this to the v2.9.0 milestone Sep 30, 2024

francislavoie pushed a commit that referenced this pull request Oct 1, 2024

caddyhttp: Fix listener wrapper regression from #6573 (#6599)

0e829bc

This was referenced Oct 12, 2024

core: addresses.go funcs renames #6622

Merged

docs for PR caddyserver/caddy#6573 caddyserver/website#424

Open

MayCXC mentioned this pull request Oct 21, 2024

core: Change getListenerFromPlugin signature #6651

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement issue #6296 passing FDs / socket activation #6573

Implement issue #6296 passing FDs / socket activation #6573

MayCXC commented Sep 15, 2024 •

edited

Loading

MayCXC commented Sep 25, 2024

wplinge commented Sep 26, 2024 •

edited

Loading

MayCXC commented Sep 26, 2024

MayCXC commented Sep 27, 2024

wplinge commented Sep 27, 2024

MayCXC commented Sep 27, 2024 •

edited

Loading

mholt commented Sep 27, 2024

MayCXC commented Sep 27, 2024

mholt left a comment

francislavoie commented Oct 1, 2024

MayCXC commented Oct 1, 2024 •

edited

Loading

MayCXC commented Oct 1, 2024 •

edited

Loading

MayCXC commented Oct 1, 2024 •

edited

Loading

Implement issue #6296 passing FDs / socket activation #6573

Implement issue #6296 passing FDs / socket activation #6573

Conversation

MayCXC commented Sep 15, 2024 • edited Loading

MayCXC commented Sep 25, 2024

wplinge commented Sep 26, 2024 • edited Loading

MayCXC commented Sep 26, 2024

MayCXC commented Sep 27, 2024

wplinge commented Sep 27, 2024

MayCXC commented Sep 27, 2024 • edited Loading

mholt commented Sep 27, 2024

MayCXC commented Sep 27, 2024

mholt left a comment

Choose a reason for hiding this comment

francislavoie commented Oct 1, 2024

MayCXC commented Oct 1, 2024 • edited Loading

MayCXC commented Oct 1, 2024 • edited Loading

MayCXC commented Oct 1, 2024 • edited Loading

MayCXC commented Sep 15, 2024 •

edited

Loading

wplinge commented Sep 26, 2024 •

edited

Loading

MayCXC commented Sep 27, 2024 •

edited

Loading

MayCXC commented Oct 1, 2024 •

edited

Loading

MayCXC commented Oct 1, 2024 •

edited

Loading

MayCXC commented Oct 1, 2024 •

edited

Loading