Use Souin cache system instead of reinventing the wheel

[darkweak]

Delegate the cache management to a RFC compliant system

[francislavoie]

I appreciate all the work you've put into this, but this will definitely need to be discussed. It would've been better if you had first opened an issue to talk about this work instead of directly opening a pull request.

I think I see where you're going with this, i.e. having a more generalized interface rather than using Olric directly, but I have a lot of concerns about this approach.

I've only spent ~15 minutes or so trying to understand what's going on, but immediately my biggest concern is configuration. It looks like the approach you're taking is to directly parse the Caddyfile tokens as YAML config for Souin. That's not okay. Caddy's actual config language is JSON, and the Caddyfile is an adapter. What you're doing currently is very much a hack and I have to give an immediate :-1: based on that. This approach doesn't fit the Caddy vision, and therefore I don't agree that it makes sense to merge in this state, especially for a project under the caddyserver organization.

[darkweak]

Hi, thank for your answer and taking time to do that. The fact, this repository is not maintained, there are no real commit since November 2020, so making an "official cache system" not maintained, non-evolutive is really sad (I know maintainers doesn't have to work on this project only, I don't blame anyone). Then, if caddyserver organization doesn't want to allow external maintained modules I take that as brake of open-source linking (but that's a choice and I respect that (that's why I made this PR)) Using olric only doesn't fit with all needs, I doesn't make sense for 60% of projects using caddy as reverse-proxy.

Then, the configuration initialization is such a pain for caddymodule. It could be great to provide a function to be able to pass a struct and caddy will return an hydrated one with the configuration. Then it can avoid long files like this one https://github.com/greenpau/caddy-auth-portal/blob/main/caddyfile.go to parse caddy configuration to fit to our configuration structure.

Using interfaces instead of implementation itself is better to extend and use any providers depending the need.

[francislavoie]

The fact, this repository is not maintained, there are no real commit since November 2020, so making an "official cache system" not maintained, non-evolutive is really sad (I know maintainers doesn't have to work on this project only, I don't blame anyone).

I know that @dunglas is going to come back to this when he can. He's already pretty heavily bought-in on Caddy so it's only an issue of time. He's split between being a core maintainer of one of the biggest PHP frameworks, and being the primary maintainer of another, plus maintaining a bunch of other projects that are tools to support those.

Then, if caddyserver organization doesn't want to allow external maintained modules I take that as brake of open-source linking (but that's a choice and I respect that (that's why I made this PR) Using olric only doesn't fit with all needs, I doesn't make sense for 60% of projects using caddy as reverse-proxy.

We definitely want help maintaining this project, but the issue here was communication. It's poor Github etiquette to open a PR without prior discussion, and in this case without more detail in the PR description. Here's an article that explains: https://eli.thegreenplace.net/2019/how-to-send-good-pull-requests-on-github/. I had to make assumptions of your intentions because there was a lack of information up-front. Ideally you would've described in the an issue or the PR description everything we needed to know about your proposed approach.

to parse caddy configuration to fit to our configuration structure

This is the problem though - you're going about it the wrong way. You need to think about how it's configured in Caddy first. The important part is to think about how a user would want to configure Caddy such that it's made easy for them. That's the point of the Caddyfile, it's a UX layer on top of the underlying JSON config.

In the example in your PR, I see things such as ssl_providers which don't seem to make sense in the context of Caddy. Caddy is the "SSL provider" (also, SSL is an outdated term, it's now called TLS). Also you're using [ ] braces for lists. This is not Caddyfile syntax. The Caddyfile also does not support ' single quote strings, it's either " or backticks. The urls config also doesn't make sense to me, since Caddy is a server. Caching should be implemented as middleware, similar to the encode module.

[darkweak]

I know that @dunglas is going to come back to this when he can

I work with him, he started this project and thought there will be contributors (as he said during the conference about the future of API Platform). Then as I said to him I was afraid about merging my work into your organisation for these mindsets that's "all should be approved by the caddyserver core team" (and everything is centralized which is the opposite of the cache storage you promote). And merging external work to caddy would mean that caddy is the owner of the code and that's not the philosophy of open-source. The bridge can be but the core module system should be own by the creator especially when the core wasn't written only for Caddy.

We definitely want help maintaining this project, but the issue here was communication

I totally agree I should have to link the discussion with dunglas on this PR description, it would be clearer.

This is the problem though - you're going about it the wrong way. You need to think about how it's configured in Caddy first.

You missed something in my previous answer imo, I said it could be good that Caddy provide an exported function to parse the configuration file (Caddyfile, json, or whatever in the universe) to be able to pass only the struct and hydrate it. Then the developer shouldn't have to manipulate a caddyfile.Dispenser object.

also, SSL is an outdated term, it's now called TLS

We can talk about bad naming in caddy too but imo that's not the point :)

Also you're using [ ] braces for lists. This is not Caddyfile syntax.

I probably missed something in the documentation but didn't found something that's working so I had to find another way.

The urls config also doesn't make sense to me

Maybe to you but for real it's usefull to manage your ttl depending the regex matching because you don't wan't to always cache your data with the same ttl.