Ignore value_prefix

[pberkel]

Based on feedback from francislavoie in issue #42 this PR introduces backwards-compatible code changes deprecating the "value_prefix" configuration item.

When storing data in Redis, the plugin now simply ignores the contents of "value_prefix", thus reducing the amount of storage required. I commented out the single line of code in case it is required again in the future it however it can be removed entirely at the discretion of the code maintainer.

When retrieving data from Redis, the plugin checks if the data is prefixed by "value_prefix" and removes it if detected, which means that existing users should be able to upgrade their version of this plugin without breaking their system.

I did not make changes to the README.md however it might be worthwhile noting that this change deprecates "value_prefix" configuration item.

[francislavoie]

My only concern about making this change is that if someone users are running a cluster of Caddy instances, then they would need to be upgraded all at once, otherwise one upgraded later might read data with the prefix already removed and would throw invalid data format errors. The problem could be compounded if an error leads to a request to delete the data as being invalid, in which case it would wipe out legitimate data by accident.

The answer is that we might need to make multiple releases in sequence, i.e. first release just stops expecting the prefix and simply strips it when reading even if it doesn't exist, then next release stops writing the prefix. That way transitioning to the release with the change to writing is safe as long as all instances were on the release with the read change.

But informing users of this change is difficult because they might not necessarily actually look at the release notes on this repo to recognize they can't just use the latest blindly.

[pberkel]

My only concern about making this change is that if someone users are running a cluster of Caddy instances, then they would need to be upgraded all at once, otherwise one upgraded later might read data with the prefix already removed and would throw invalid data format errors. The problem could be compounded if an error leads to a request to delete the data as being invalid, in which case it would wipe out legitimate data by accident.

Valid concern @francislavoie on reflection, the main problem is that the current code does not provide a way to "unset" value_prefix in the config since UnmarshalCaddyfile() will set the default value if it encounters and empty string.

https://github.com/gamalan/caddy-tlsredis/blob/1d90bd3f80cb3c600e4c756d431cfb831f6a1c10/storageredis.go#L234

So perhaps the short term (backwards-compatible) solution is to update the code in UnmarshalCaddyfile() to explicitly allow setting value_prefix (and perhaps other configuration parameters?) to an empty string AND stop expecting the prefix and simply strip it when reading even if it doesn't exist rather than throwing an error? This will allow existing users to continue using the plugin as-is and new users can explicitly prevent an value_prefix from being set by configuring it to be an empty string.

I guess this might still break compatibility with anyone who already sets value_prefix to empty string in their configuration, not sure if this is a significant edge case worth considering?

[pberkel]

Going to close this particular PR and re-think the approach.

[francislavoie]

FWIW I was kinda thinking of writing a fork/rewrite of this module (to solve the issues in #42), and recommending users run caddy storage export to migrate the data to the new module. But I haven't had the time to work on that. If that's something you're interested in doing, I think it's a valid approach because it gives a safer migration path.

[pberkel]

After trying to refactor the Provision() code in this plugin, I have come to the same conclusion. Interestingly, having tested caddy storage export I believe https://github.com/gamalan/caddy-tlsredis/issues/46 is a legitimate issue and will see if I can resolve that first before moving forward.