Skip to content
This repository has been archived by the owner on Jul 7, 2021. It is now read-only.

Support Caddy v2 via JSON configuration #16

Closed
wants to merge 15 commits into from

Conversation

Baldinof
Copy link

@Baldinof Baldinof commented Mar 1, 2021

Hi!

Here is a first draft of a migration for Caddy v2.

Todo list:

  • Caddy module with JSON configuration
  • Caddyfile support
  • Tests
  • Updated README
  • CI / Github Actions

@mholt For the Caddyfile support I might need your expertise :)

For what I understand we will need a new class to be able to configure the supervisor top level app. How should we proceed? Would you accept a PR?

Should fix #14

@Baldinof Baldinof marked this pull request as ready for review March 2, 2021 13:52
Copy link

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Cool!

Since other app modules aren't part of the HTTP server, they don't necessarily have a place in the default HTTP Caddyfile, which is structured by site definitions. Obviously, other apps like this one don't have the concept of sites so it doesn't really fit. So, Caddyfile support can happen two ways for app modules like yours:

  1. The HTTP caddyfile has global options that appear at the top; and as of the latest commits on master, Caddy supports configuring app modules from this area. This is best for other apps that support the HTTP server, like in your case I could see users configuring running php-fpm along with their sites so they don't have to mess with php-fpm separately.

  2. The Caddyfile adapter is itself extensible; you can define your own Caddyfile syntax and have the caddyfile package do the block/token parsing for you. Then you decide what to make of them. (It's called a ServerType, kind of a holdover from Caddy v1.) These Caddyfiles stand on their own, kind of like how the HTTP Caddyfile is its own thing.

Anyway, looks like a good start!

app.go Outdated
Comment on lines 45 to 48
a.log.
With(zap.Any("supervisors", supervisors)).
With(zap.Any("definition", definition)).
Debug("Supervisors created")
Copy link

Choose a reason for hiding this comment

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

This is a weird way to emit a log; I usually only use With if I'm re-assigning it to a variable so that multiple future log entries have those fields. If I were you I'd change them to a.log.Debug("...", zap.Any(), zap.Any()).

definition.go Outdated
// Definition is the configuration for process to supervise
type Definition struct {
Command []string `json:"command,omitempty"`
Replicas int
Copy link

Choose a reason for hiding this comment

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

Make sure all user-facing fields have JSON struct tags. Also add godoc comments for everything user-facing, this will automatically be part of generated documentation on the Caddy website when you register your module. It's how people will learn how to use your module.

Copy link
Owner

@lucaslorentz lucaslorentz left a comment

Choose a reason for hiding this comment

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

@Baldinof
I wonder if after finishing the port to caddy v2 you would like to maintain this plugin in your git account, and register it in caddy v2 docs portal.
I would be happy to add a link to your repository.

{
"command": ["php-fpm"],
"dir": ".",
"env": ["ADDITIONAL_ENV=1"],
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if this would be more intuitive as a JSON object (map[string]string)

"env": {
  "ADDITIONAL_ENV": "1"
}

What do you think? Would it bring some complications to parse, or to show in caddy documentation?

Copy link
Author

Choose a reason for hiding this comment

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

👍 I just copied what v1 options was expecting, but it would be much better like this :)

@Baldinof
Copy link
Author

Baldinof commented Mar 4, 2021

Thanks @mholt for the explanation, I think I will provide the 2 options, being able to configure supervisor in an HTTP Caddyfile is very useful (php-fpm is a good example), and it makes sense to have a dedicated adatper to use caddy as supervisor.

@lucaslorentz No problem to maintain this module in my Github account. I will just finish it here, so I can have reviewers :)

@Baldinof
Copy link
Author

Hi there!

I think this can be released and I would like to move forward on this :)

I would appreciate some feedback on the Caddyfile syntax:

{
  supervisor {
    # No block prefix, whole command line on the block definition
    php-fpm --no-daemonize {
      env APP_ENV production
      
      restart_policy always
      
      redirect_stdout stdout # Should this be the default?
      redirect_stderr stderr
    }
    
    # block configuration is optional    
    node worker.js
  }
}

mysite.com

root * /var/www/html
php_fastcgi 127.0.0.1:9000
file_server

@mholt Do you have some pointers on how to make a module viewable here https://caddyserver.com/docs/modules/ ?

@lucaslorentz
Copy link
Owner

Caddyfile syntax looks good to me.

Now that caddy spits structured json logs by default. I think maybe a better default behavior would be to log stdout and stderr inside zap log messages. But that would require additional changes and can be implemented later.

So, the options for redirect_stdout and redirect_stderr would be:

  • log (default)
  • stdout (do we still need this? Or maybe only inside logs would be enough?)
  • stderr (do we still need this? Or maybe only inside logs would be enough?)
  • file
  • null

@mholt
Copy link

mholt commented May 25, 2021

@Baldinof To have your Caddy plugin show up on the Caddy website, simply go to Account, then claim the package there. It will then be available for download as well as showing documentation.

Actually, wait a few days before doing this since I'm releasing some big improvements to that system this week. Edit: All done. Go ahead.

@Baldinof
Copy link
Author

@lucaslorentz You are right, and I initially experimented a bit with logs at first. But it comes with a lot of questions, for example:

  • In which field the output should be stored so that it's clear where it comes from?
  • Supervised process may log in JSON, should it be parsed and added as a zap field for easy filtering? It would be a lot of processing for caddy (but I think just writing json encoded strings to a log field would not be good)
  • Do we need to bufferize the output? (users will expect each log entry to be an output line)

Maybe a lot of this could be configured via Caddyfile or JSON, but it's a lot of work.

So, the options for redirect_stdout and redirect_stderr would be:
* log (default)
* stdout (do we still need this? Or maybe only inside logs would be enough?)
* stderr (do we still need this? Or maybe only inside logs would be enough?)
* file
* null

I like it! I implemented it without the log supports for now.

@Baldinof
Copy link
Author

Baldinof commented Jul 7, 2021

Hey there,

The package is claimed on the Caddy website, and this branch has been merged on the master branch under my fork :)

Thank you for you help!

@Baldinof Baldinof closed this Jul 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supervisor and Caddy 2
4 participants