-
Notifications
You must be signed in to change notification settings - Fork 425
feat: worker matching #1646
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
feat: worker matching #1646
Changes from all commits
84cd988
dbd66ec
1e8bef1
fdbee0f
b383af9
9b7ef4d
09d8da8
6acc01e
b93f261
1393be5
ff4d18c
608d175
8cda5d9
b9021fd
245ad76
9ec9560
4047b3f
03faa72
081327e
b5a1f76
6de13d4
b9adf63
847f4d5
9b92725
570069b
6dd533d
5a1ecf2
ab26c9e
cfeaf07
6062f02
e295a68
ef304a7
c9cc0c5
6367456
771099c
16d7877
94ffc39
1da86cb
0fcb920
d3d8405
23606e6
226cafe
ea9a9fe
db66a76
e634358
30427d3
1ed4504
36820c3
6833933
1ecb44c
da23e7c
d3fb25d
6e5350e
a95faf4
b8ad01a
eb9b02a
3b8c80a
48bb34c
3927399
ee2d4ac
949295b
4adfa38
fd18aa4
a11c17d
a9a1589
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| package caddy | ||
| package caddy | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,6 +69,21 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error { | |
| return fmt.Errorf(`expected ctx.App("frankenphp") to return *FrankenPHPApp, got nil`) | ||
| } | ||
|
|
||
| for i, wc := range f.Workers { | ||
|
|
||
| // make the file path absolute from the public directory | ||
| // this can only be done if the root is definied inside php_server | ||
| if !filepath.IsAbs(wc.FileName) && f.Root != "" { | ||
| wc.FileName = filepath.Join(f.Root, wc.FileName) | ||
| } | ||
|
Comment on lines
+76
to
+78
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This introduces a potential subtle bug when the root is Also, the comment above doesn't match the code :)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm yeah this code was already there, I just moved it from Parsing to Provisioning. I think the root being empty means that it is defined outside of the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the bug here - when root is "" the if statement is not entered and the worker path will later be resolved in the current working directory, just like with global workers. There's no way to resolve it to the root defined outside the php_server block, because workers are started before the first request.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah this is the currently expected behavior. A bit unfortunate, but it's not possible to always determine the root at provisioning. I guess it would also be possible to first try from the public path and if there is no script, try from the working directory, something for a future PR. |
||
|
|
||
| // Inherit environment variables from the parent php_server directive | ||
| if f.Env != nil { | ||
| wc.inheritEnv(f.Env) | ||
| } | ||
| f.Workers[i] = wc | ||
| } | ||
|
|
||
| workers, err := fapp.addModuleWorkers(f.Workers...) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -161,12 +176,11 @@ func (f *FrankenPHPModule) ServeHTTP(w http.ResponseWriter, r *http.Request, _ c | |
| } | ||
| } | ||
|
|
||
| fullScriptPath, _ := fastabs.FastAbs(documentRoot + "/" + r.URL.Path) | ||
|
|
||
| workerName := "" | ||
| for _, w := range f.Workers { | ||
| if p, _ := fastabs.FastAbs(w.FileName); p == fullScriptPath { | ||
| if w.matchesPath(r, documentRoot) { | ||
| workerName = w.Name | ||
| break | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -230,12 +244,11 @@ func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { | |
| f.ResolveRootSymlink = &v | ||
|
|
||
| case "worker": | ||
| for d.NextBlock(1) { | ||
| } | ||
| for d.NextArg() { | ||
| wc, err := parseWorkerConfig(d) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // Skip "worker" blocks in the first pass | ||
| continue | ||
| f.Workers = append(f.Workers, wc) | ||
|
|
||
| default: | ||
| allowedDirectives := "root, split, env, resolve_root_symlink, worker" | ||
|
|
@@ -244,43 +257,13 @@ func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { | |
| } | ||
| } | ||
|
|
||
| // Second pass: Parse only "worker" blocks | ||
| d.Reset() | ||
| for d.Next() { | ||
| for d.NextBlock(0) { | ||
| if d.Val() == "worker" { | ||
| wc, err := parseWorkerConfig(d) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Inherit environment variables from the parent php_server directive | ||
| if !filepath.IsAbs(wc.FileName) && f.Root != "" { | ||
| wc.FileName = filepath.Join(f.Root, wc.FileName) | ||
| } | ||
|
|
||
| if f.Env != nil { | ||
| if wc.Env == nil { | ||
| wc.Env = make(map[string]string) | ||
| } | ||
| for k, v := range f.Env { | ||
| // Only set if not already defined in the worker | ||
| if _, exists := wc.Env[k]; !exists { | ||
| wc.Env[k] = v | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check if a worker with this filename already exists in this module | ||
| for _, existingWorker := range f.Workers { | ||
| if existingWorker.FileName == wc.FileName { | ||
| return fmt.Errorf(`workers in a single "php_server" block must not have duplicate filenames: %q`, wc.FileName) | ||
| } | ||
| } | ||
|
|
||
| f.Workers = append(f.Workers, wc) | ||
| } | ||
| // Check if a worker with this filename already exists in this module | ||
| fileNames := make(map[string]struct{}, len(f.Workers)) | ||
| for _, w := range f.Workers { | ||
| if _, ok := fileNames[w.FileName]; ok { | ||
| return fmt.Errorf(`workers in a single "php_server" block must not have duplicate filenames: %q`, w.FileName) | ||
| } | ||
| fileNames[w.FileName] = struct{}{} | ||
| } | ||
|
|
||
| return nil | ||
|
|
@@ -418,6 +401,13 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) | |
| // unmarshaler can read it from the start | ||
| dispenser.Reset() | ||
|
|
||
| // the rest of the config is specified by the user | ||
| // using the php directive syntax | ||
| dispenser.Next() // consume the directive name | ||
| if err := phpsrv.UnmarshalCaddyfile(dispenser); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| if frankenphp.EmbeddedAppPath != "" { | ||
| if phpsrv.Root == "" { | ||
| phpsrv.Root = filepath.Join(frankenphp.EmbeddedAppPath, defaultDocumentRoot) | ||
|
|
@@ -433,6 +423,9 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) | |
| // set up a route list that we'll append to | ||
| routes := caddyhttp.RouteList{} | ||
|
|
||
| // prepend routes from the 'worker match *' directives | ||
| routes = prependWorkerRoutes(routes, h, phpsrv, fsrv, disableFsrv) | ||
|
|
||
| // set the list of allowed path segments on which to split | ||
| phpsrv.SplitPath = extensions | ||
|
|
||
|
|
@@ -521,15 +514,6 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) | |
| "path": h.JSON(pathList), | ||
| } | ||
|
|
||
| // the rest of the config is specified by the user | ||
| // using the php directive syntax | ||
| dispenser.Next() // consume the directive name | ||
| err = phpsrv.UnmarshalCaddyfile(dispenser) | ||
|
|
||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // create the PHP route which is | ||
| // conditional on matching PHP files | ||
| phpRoute := caddyhttp.Route{ | ||
|
|
@@ -576,3 +560,52 @@ func parsePhpServer(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) | |
| }, | ||
| }, nil | ||
| } | ||
|
|
||
| // workers can also match a path without being in the public directory | ||
| // in this case we need to prepend the worker routes to the existing routes | ||
| func prependWorkerRoutes(routes caddyhttp.RouteList, h httpcaddyfile.Helper, f FrankenPHPModule, fsrv caddy.Module, disableFsrv bool) caddyhttp.RouteList { | ||
| allWorkerMatches := caddyhttp.MatchPath{} | ||
| for _, w := range f.Workers { | ||
| for _, path := range w.MatchPath { | ||
| allWorkerMatches = append(allWorkerMatches, path) | ||
| } | ||
| } | ||
|
|
||
| if len(allWorkerMatches) == 0 { | ||
| return routes | ||
| } | ||
|
|
||
| // if there are match patterns, we need to check for files beforehand | ||
| if !disableFsrv { | ||
| routes = append(routes, caddyhttp.Route{ | ||
| MatcherSetsRaw: []caddy.ModuleMap{ | ||
| caddy.ModuleMap{ | ||
| "file": h.JSON(fileserver.MatchFile{ | ||
| TryFiles: []string{"{http.request.uri.path}"}, | ||
| Root: f.Root, | ||
| }), | ||
| "not": h.JSON(caddyhttp.MatchNot{ | ||
| MatcherSetsRaw: []caddy.ModuleMap{ | ||
| {"path": h.JSON(caddyhttp.MatchPath{"*.php"})}, | ||
| }, | ||
| }), | ||
| }, | ||
| }, | ||
| HandlersRaw: []json.RawMessage{ | ||
| caddyconfig.JSONModuleObject(fsrv, "handler", "file_server", nil), | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| // forward matching routes to the PHP handler | ||
| routes = append(routes, caddyhttp.Route{ | ||
| MatcherSetsRaw: []caddy.ModuleMap{ | ||
| caddy.ModuleMap{"path": h.JSON(allWorkerMatches)}, | ||
| }, | ||
| HandlersRaw: []json.RawMessage{ | ||
| caddyconfig.JSONModuleObject(f, "handler", "php", nil), | ||
| }, | ||
| }) | ||
|
|
||
| return routes | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,15 @@ package caddy | |
|
|
||
| import ( | ||
| "errors" | ||
| "net/http" | ||
| "path/filepath" | ||
| "strconv" | ||
|
|
||
| "github.com/caddyserver/caddy/v2" | ||
| "github.com/caddyserver/caddy/v2/caddyconfig/caddyfile" | ||
| "github.com/caddyserver/caddy/v2/modules/caddyhttp" | ||
| "github.com/dunglas/frankenphp" | ||
| "github.com/dunglas/frankenphp/internal/fastabs" | ||
| ) | ||
|
|
||
| // workerConfig represents the "worker" directive in the Caddyfile | ||
|
|
@@ -29,6 +33,8 @@ type workerConfig struct { | |
| Env map[string]string `json:"env,omitempty"` | ||
| // Directories to watch for file changes | ||
| Watch []string `json:"watch,omitempty"` | ||
| // The path to match against the worker | ||
| MatchPath []string `json:"match_path,omitempty"` | ||
| // MaxConsecutiveFailures sets the maximum number of consecutive failures before panicking (defaults to 6, set to -1 to never panick) | ||
| MaxConsecutiveFailures int `json:"max_consecutive_failures,omitempty"` | ||
| } | ||
|
|
@@ -96,6 +102,12 @@ func parseWorkerConfig(d *caddyfile.Dispenser) (workerConfig, error) { | |
| } else { | ||
| wc.Watch = append(wc.Watch, d.Val()) | ||
| } | ||
| case "match": | ||
| // provision the path so it's identical to Caddy match rules | ||
| // see: https://github.com/caddyserver/caddy/blob/master/modules/caddyhttp/matchers.go | ||
| caddyMatchPath := (caddyhttp.MatchPath)(d.RemainingArgs()) | ||
| caddyMatchPath.Provision(caddy.Context{}) | ||
| wc.MatchPath = ([]string)(caddyMatchPath) | ||
| case "max_consecutive_failures": | ||
| if !d.NextArg() { | ||
| return wc, d.ArgErr() | ||
|
|
@@ -111,7 +123,7 @@ func parseWorkerConfig(d *caddyfile.Dispenser) (workerConfig, error) { | |
|
|
||
| wc.MaxConsecutiveFailures = int(v) | ||
| default: | ||
| allowedDirectives := "name, file, num, env, watch, max_consecutive_failures" | ||
| allowedDirectives := "name, file, num, env, watch, match, max_consecutive_failures" | ||
| return wc, wrongSubDirectiveError("worker", allowedDirectives, v) | ||
| } | ||
| } | ||
|
|
@@ -126,3 +138,29 @@ func parseWorkerConfig(d *caddyfile.Dispenser) (workerConfig, error) { | |
|
|
||
| return wc, nil | ||
| } | ||
|
|
||
| func (wc workerConfig) inheritEnv(env map[string]string) { | ||
| if wc.Env == nil { | ||
| wc.Env = make(map[string]string, len(env)) | ||
| } | ||
| for k, v := range env { | ||
| // do not overwrite existing environment variables | ||
| if _, exists := wc.Env[k]; !exists { | ||
| wc.Env[k] = v | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func (wc workerConfig) matchesPath(r *http.Request, documentRoot string) bool { | ||
|
|
||
| // try to match against a pattern if one is assigned | ||
| if len(wc.MatchPath) != 0 { | ||
| return (caddyhttp.MatchPath)(wc.MatchPath).Match(r) | ||
| } | ||
|
|
||
| // if there is no pattern, try to match against the actual path (in the public directory) | ||
| fullScriptPath, _ := fastabs.FastAbs(documentRoot + "/" + r.URL.Path) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. does this work with symlinks (see #1637)?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the fallback to the previous logic, so if there was a bug with symlinks before, then it probably will still exist. In general,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When using the new matching logic though, it should always work, regardless of symlinks. |
||
| absFileName, _ := fastabs.FastAbs(wc.FileName) | ||
|
|
||
| return fullScriptPath == absFileName | ||
AlliBalliBaba marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the change here? Invisible char?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a BOM?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be a line ending, was the result of
go fmt