Skip to content
This repository has been archived by the owner on Mar 31, 2024. It is now read-only.

Caddy v2 version? #4

Closed
codeagencybe opened this issue Mar 24, 2020 · 22 comments
Closed

Caddy v2 version? #4

codeagencybe opened this issue Mar 24, 2020 · 22 comments

Comments

@codeagencybe
Copy link

Hello

Is there any Caddy v2 version yet or in progress? Any ETA?

Thanks!

@gamalan
Copy link
Owner

gamalan commented Mar 29, 2020

Next week, probably.

@gamalan
Copy link
Owner

gamalan commented Apr 9, 2020

@codeagencybe kindly check version v0.2.0-beta.1

@frenchvandal
Copy link

Thank you @gamalan
I have tried to make it work by adding the module to the Caddy Docker builder image but when I reload the new config I get:

caddy    | {"level":"info","ts":1586883503.061408,"msg":"using provided configuration","config_file":"/etc/caddy/Caddyfile","config_adapter":"caddyfile"}
caddy    | run: adapting config using caddyfile: storage: /etc/caddy/Caddyfile:8 - Error during parsing: storage module 'caddy.storage.redis' is not a Caddyfile unmarshaler

@mholt
Copy link
Contributor

mholt commented Apr 14, 2020

Ah, yeah, if you want to use it with the Caddyfile, the same module that implements StorageConverter needs to implement the caddyfile.Unmarshaler interface too.

It's pretty simple, a great example is in the default FileStorage converter: https://github.com/caddyserver/caddy/blob/ec456811bb6d61ce32dbe6e4b7580d383f8a4adf/modules/filestorage/filestorage.go#L46

@gamalan
Copy link
Owner

gamalan commented Apr 14, 2020

ah, i thought it was not necessary. @mholt does it need to implement even if it doesn't take any parameter from caddyfile? or how does unmarshaler works, especially for storage part?

@mholt
Copy link
Contributor

mholt commented Apr 14, 2020

@gamalan It's not strictly required, but it is if you want people to be able to use your module from the Caddyfile, i.e. to have a Caddyfile interface.

All it has to do is iterate the tokens and unpack them into your struct for config.

For example, your JSON interface is currently something like:

{
    "module": "redis",
    "Options": {
        "Host": "localhost",
        "Port": "1234"
    }
}

(see my PS below for suggestions to improve this)

Then if people want to use the Caddyfile to configure it instead, they might do:

storage redis localhost:1234

So you'd simply iterate the next token and assign their value to the Host and Port fields on your struct. That's all! See how the example I linked to above does it, it's just a few lines of code:

https://github.com/caddyserver/caddy/blob/ec456811bb6d61ce32dbe6e4b7580d383f8a4adf/modules/filestorage/filestorage.go#L46-L60

PS. You might consider simplifying the JSON interface if you can, i.e. Host and Port could be combined and you can then use net.SplitHostPort if you need them separate (or caddy.ParseNetworkAddress, if possible!). And do you really need a separate struct for Options? Also, you should put json struct tags on these fields to be consistent with the rest of Caddy's config conventions, which is snake_case field names:

type RedisStorage struct {
Client *redis.Client
ClientLocker *redislock.Client
Options *Options
locks map[string]*redislock.Lock
}
-- I'll try to make this clearer in the docs!

@gamalan
Copy link
Owner

gamalan commented Apr 15, 2020

Well, it's because caddy v1 doesn't have standard storage directive yet, afaik. Assuming if i don't use the caddyfile yet, i can just add the function and skip it right?

func (rd *RedisStorage) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
	return nil
}

@mholt
Copy link
Contributor

mholt commented Apr 15, 2020

@gamalan To clarify, you don't need to add that method at all -- might as well not add it if it will be empty. The point of adding it is to let people configure your module from the Caddyfile. If you add it but leave it empty, they will think they've configured it (they won't get an error) but in reality it will have all empty values.

You're definitely not obligated to implement it yourself; anyone could spend a few minutes and submit a pull request, I suppose.

When you are ready, let me help you finish the transition to v2! 👍 I can give you some pointers and review code, no problem.

@gamalan
Copy link
Owner

gamalan commented Apr 15, 2020

but @mholt your previous comment

Ah, yeah, if you want to use it with the Caddyfile, the same module that implements StorageConverter needs to implement the caddyfile.Unmarshaler interface too.

means, it is necessary to be added, for now(?)

@mholt
Copy link
Contributor

mholt commented Apr 15, 2020

means, it is necessary to be added, for now(?)

Necessary for what, precisely?

  • It is not necessary to simply get it working with Caddy 2.
  • It is necessary if you want it to work with the Caddyfile adapter in Caddy 2.

@gamalan
Copy link
Owner

gamalan commented Apr 15, 2020

ah I see, so this error won't happen if running caddy v2 without adapt ?

caddy    | {"level":"info","ts":1586883503.061408,"msg":"using provided configuration","config_file":"/etc/caddy/Caddyfile","config_adapter":"caddyfile"}
caddy    | run: adapting config using caddyfile: storage: /etc/caddy/Caddyfile:8 - Error during parsing: storage module 'caddy.storage.redis' is not a Caddyfile unmarshaler

@mholt
Copy link
Contributor

mholt commented Apr 15, 2020

Ahh... you are not aware yet how Caddy 2 config works. Caddy 2's native config is JSON. The Caddyfile is simply a wrapper ("config adapter") over it that converts the Caddyfile into JSON: https://caddyserver.com/docs/config-adapters

Making plugins work with the Caddyfile is totally optional, since Caddy 2 does not use it for config.

You only have to run caddy adapt if you want to convert your Caddyfile (or whatever other format) into JSON. caddy run and caddy start will do this for you automatically. See command line docs and the Getting Started guide

To summarize:

  • You do not need to implement UnmarshalCaddyfile() but then your plugin cannot be configured with the Caddyfile.
  • You do need to implement UnmarshalCaddyfile() if you want your plugin to be configurable via the Caddyfile.

@gamalan
Copy link
Owner

gamalan commented Apr 15, 2020

I see, I've read but not in full, and want to make sure. Let me update it then.

@mholt
Copy link
Contributor

mholt commented Apr 15, 2020

Thanks for building and maintaining this plugin! I think it'll be useful to a lot of people as v2 gains more steam. 😃

@gamalan
Copy link
Owner

gamalan commented Apr 15, 2020

@mholt based on my understanding of how parsing caddyfile works, i could use something like this to looping the Caddyfile directive

for example

storage redis {
   host localhost
   port 1234
   password ******
   db 12
}
func (rd *RedisStorage) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
	for d.Next() {
		args := d.RemainingArgs()
		if len(args) > 1 {
			switch args[0] {
			case "host":
				break
			case "port":
				break
			case "db":
				break
			}
		} else {
			continue
		}
	}
	return nil
}

just making sure

@mholt
Copy link
Contributor

mholt commented Apr 15, 2020

Something like that, yep -- I think some improvements could be made to the API/structure here (again, something like combining the host and port, and making password optional so that it can be specified via env variables or something) -- but that's the right idea. Instead of just break though you'd actually set rd.Host = args[1] or whatever.

@gamalan
Copy link
Owner

gamalan commented Apr 15, 2020

Yep, because the Redis client actually take host:port as a single address input. In the previous version, all parameter have default value, and can be set through env variable, so I will keep it like that while supporting Caddyfile format.

@frenchvandal
Copy link

Just a little feedback:

  • I do not normally use the Caddyfile by default (my config is done via JSON) but for testing purpose (and a bit out of laziness), I used a simple Caddyfile. Hence the error I reported above.
  • I went bold and tried this module directly in production by modifying the TLS policy in my JSON file:
                        "storage": {
                            "module": "redis"
                        }

I used the Docker builder image to install the module with the RC3 release.

The Caddy TLS Redis storage works just fine. I got new certificates generated for my domains and they are stored in Redis. So far, after a couple of days, I do not have noticed any issues or errors on the served sites.

Thanks to @gamalan for making this module possible with v2 and to @mholt for giving the author some helping hints.

I see that this pull request caddyserver/certmagic#66 will introduce some breaking changes in certmagic. I will keep an eye close, but I still have my certs in the local file storage in case I need to revert.

Thank you once again!

@mholt
Copy link
Contributor

mholt commented Apr 18, 2020

Awesome! Great report, thanks for the update.

(That breaking change is easy to accommodate, I'll personally submit PRs to all repos I know it will affect, so you won't have much work to do!)

@gamalan
Copy link
Owner

gamalan commented Apr 21, 2020

Already update the implementation for v0.2.0-beta.2

@frenchvandal
Copy link

Thank you @gamalan

I updated my custom Caddy image with the v0.2.0-beta.2 release. All good for me:

caddy    | {"level":"info","ts":1587457624.7980285,"msg":"using provided configuration","config_file":"/etc/caddy/caddy.json","config_adapter":""}
caddy    | {"level":"info","ts":1587457624.7992222,"logger":"admin","msg":"admin endpoint started","address":"tcp/0.0.0.0:2019","enforce_origin":false,"origins":["0.0.0.0:2019"]}
caddy    | {"level":"info","ts":1587457624.799707,"logger":"caddy.storage.redis","msg":"TLS Storage are using Redis, on redis:6379"}
caddy    | 2020/04/21 08:27:04 [INFO][cache:0xc0004a9360] Started certificate maintenance routine
caddy    | {"level":"info","ts":1587457624.8014157,"logger":"http","msg":"enabling automatic HTTP->HTTPS redirects","server_name":"srv0"}
caddy    | {"level":"info","ts":1587457624.80857,"logger":"tls","msg":"cleaned up storage units"}
caddy    | {"level":"info","ts":1587457624.8088355,"logger":"http","msg":"enabling experimental HTTP/3 listener","addr":":443"}
caddy    | {"level":"debug","ts":1587457624.8090024,"logger":"http","msg":"starting server loop","address":"[::]:443","http3":true,"tls":true}
caddy    | {"level":"debug","ts":1587457624.8091228,"logger":"http","msg":"starting server loop","address":"[::]:80","http3":false,"tls":false}
caddy    | {"level":"info","ts":1587457624.8091328,"logger":"http","msg":"enabling automatic TLS certificate management","domains":["www.itsallsotireso.me","www.normco.re","itsallsotireso.me","normco.re"]}
caddy    | {"level":"info","ts":1587457624.850657,"msg":"autosaved config","file":"/config/caddy/autosave.json"}
caddy    | {"level":"info","ts":1587457624.8506882,"msg":"serving initial configuration"}

@gamalan
Copy link
Owner

gamalan commented Apr 21, 2020

Thanks for confirming @frenchvandal

@gamalan gamalan closed this as completed Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants