Skip to content
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

How about more mature deprecation flow? #5050

Closed
nsauk opened this issue Sep 19, 2022 · 16 comments
Closed

How about more mature deprecation flow? #5050

nsauk opened this issue Sep 19, 2022 · 16 comments
Labels
discussion 💬 The right solution needs to be found

Comments

@nsauk
Copy link

nsauk commented Sep 19, 2022

I used caddyserver v2.4.3 for about a year, then I've upgraded to v2.5.2 and noticed that server just can't use old config because of #4148.

I've keep an eye on your project since v0.10 or so, and I was pretty sure that now it's a serious project that could be used in the production without any doubts. I even recommended it to other people. But how could you deprecate and delete a setting within 1 year? Since I did not update between v2.4.3 and v2.5.2, I did not even have a chance to see the warning (if there was one).

https://caddy.community/t/error-module-not-registered-caddy-logging-encoders-single-field-encountered-after-upgrading-to-2-5-0/15763

Yes, it was removed. It was deprecated for many versions.

Hey, that's many versions for you (developers), but I as a user expect from a newer v2.x.x to be fully compatible with an older v2.x.x.

Please consider changing your way from 'move fast and break things' to something more suitable for a web server.

@francislavoie
Copy link
Member

francislavoie commented Sep 19, 2022

We're not really doing semver right now. "v2" is the "product name" for us and "2.x" you can consider major versions where deprecations are removed. Our "2.x.x" minor versions often contains new features and bug fixes, but we try to avoid breakages in those versions (as much as we can, but we do sometimes break things when we need to if we deem a feature to have a very low % userbase based on what we hear from the community).

We had deprecated common_log like a year prior, and there was a warning at startup in your logs. It's your responsibility to look at your logs and find alternatives to deprecations when you see them, so you don't get yourself stuck during an upgrade.

@mholt
Copy link
Member

mholt commented Sep 19, 2022

(FYI, I'll chime in too when I'm back at the office tomorrow)

@nsauk
Copy link
Author

nsauk commented Sep 19, 2022

@francislavoie there is no deprecation warning about common_log in v2.4.3. Warnings have been added in v2.4.4 released on 31 Aug 2021. The only warnings I saw with v2.4.3 are:

  • WARN Caddyfile input is not formatted; run the 'caddy fmt' command to fix inconsistencies (idk why it's considered a reason to warn me; I use spaces for indentation and feels good)
  • WARN exiting; byeee!! 👋 (no comments)

So from my point of view you just break things in unexpected manner. Do you imply that all users of web server should upgrade it every month? Why should I update the web server often if it works? If there are security issues, I prefer to update as needed rather than on a schedule. I don't understand why you think removing the CLF is such an urgent change that it can't wait for years or for v3.

@francislavoie
Copy link
Member

Warnings have been added in v2.4.4 released on 31 Aug 2021.

Exactly, that's over a year ago. Running security critical software that's over a year old is a bad idea.

Do you imply that all users of web server should upgrade it every month?

Yes, you should continually update to the latest version regularly, and as soon as possible.

Why should I update the web server often if it works?

Your perspective of "works" may not be accurate - it may have bugs that go unnoticed because they only happen in edgecases, or there may be security vulnerabilities that we patched, etc.

It's very important to have processes in place to keep your software up to date, for the safety of you and your site's users.

I don't understand why you think removing the CLF is such an urgent change that it can't wait for years or for v3.

Because it adds a lot of extra bytes to the logs that are redundant information (the structured part of the logs already contained all the same information). For users of the JSON logs, that adds up quickly when you're logging hundreds of thousands or more requests per day.

We announced our intent to remove it a very long time ago. It's your responsibility as a user to keep yourself informed by reading the release notes and announcements we make. You can't blame us if you didn't read the information we put out.

@mholt
Copy link
Member

mholt commented Sep 19, 2022

First, thanks for being such a long-time user and follower of the Caddy project!

Sorry for the inconvenience about the deprecation of CLF support. Here's the full background on that:

Ever since v2 launched, our upgrade guide always recommended against using Common Log in the long term:

we recommend this only for transitioning while your legacy systems still require CLF.

because we knew that support was a temporary hack that was inefficient and cumbersome, and that CLF itself was insufficient for v2's requirements.

Issue #4148 that you've read explains why it was broken. As is the nature of open source, we opened this for feedback and discussion a long time ago. You could have participated in that; open source is largely influenced by its community and sponsors. With the information and feedback we had, we decided deprecating it at that time was the best decision.

Thus the single_field module was deprecated in August 2021, a good ~9 months before it was removed: https://github.com/caddyserver/website/blob/3dac36ebc856dbe4351f91330370f8507ede6a46/src/docs/markdown/caddyfile/directives/log.md

We prominently logged it at the WARN level (which we reserve only for very important, non-error logs) since v2.4.4. I'm sorry you didn't see the warning:

caddy.Log().Named("caddy.logging.encoders.single_field").Warn("the 'single_field' encoder is deprecated and will be removed soon!")

Our release notes highlighted with a ⚠️ by the time it was finally removed (on a minor release, not a patch release, which we carefully aimed for):

⚠️ Logging: Removed the deprecated common_log field from HTTP access logs, and the single_field encoder. If you relied on this, you may use the transform encoder plugin to encode logs in Common Log format.

I'm not sure what a more "mature" deprecation flow you are expecting? Did you just want us to wait longer?

I as a user expect from a newer v2.x.x to be fully compatible with an older v2.x.x.

We do strive for this, and 99% of the time this is the case. We generally only intentionally break in the case of bugs, security issues, experimental features, or odd corners of the software that we are fairly certain nobody is using. The CLF removal sits squarely in between all of those:

  • CLF is broken, redundant, and noisy; essentially a bugged design
  • CLF is bad for security because log alterations such as redactions didn't affect CLF
  • CLF was never intended to be in Caddy 2 for the long-term; only to help people transition from v1 to v2 (we could have noted this more clearly, I guess - sorry about that)
  • Based on the discussion, interest in the feature, and without the telemetry that we once had, our best knowledge was that CLF did not have compelling use cases these days (which we still stand by)

Please consider changing your way from 'move fast and break things' to something more suitable for a web server.

We're open to this! But catering to old, broken features can be a lot of work. This is the kind of request I think we would be very happy to satisfy for a sponsor at a sufficient tier.

So from my point of view you just break things in unexpected manner. Do you imply that all users of web server should upgrade it every month?

We do expect that users keep their server software mostly up-to-date, yes. We can cater to users of older versions through sufficiently-tiered sponsorships to cover the cost and complexity of that.

I don't understand why you think removing the CLF is such an urgent change that it can't wait for years or for v3.

Hopefully I've explained that by now: it was a bug, it was a security flaw waiting to happen, etc. As you've hopefully noticed by now, you can still retain CLF if you really want to by using the transform encoder!

@mholt mholt added the discussion 💬 The right solution needs to be found label Sep 19, 2022
@simaotwx
Copy link
Contributor

We're not really doing semver right now. "v2" is the "product name" for us and "2.x" you can consider major versions where deprecations are removed. Our "2.x.x" minor versions often contains new features and bug fixes, but we try to avoid breakages in those versions (as much as we can, but we do sometimes break things when we need to if we deem a feature to have a very low % userbase based on what we hear from the community).

I was actually assuming that Caddy uses semver – seems like I was wrong about this.

Some projects deprecate functionality and remove it after a significant amount of time without incrementing the major version.

So from my point of view you just break things in unexpected manner. Do you imply that all users of web server should upgrade it every month? Why should I update the web server often if it works? If there are security issues, I prefer to update as needed rather than on a schedule. I don't understand why you think removing the CLF is such an urgent change that it can't wait for years or for v3.

As someone who writes, maintains and deploys software it is my (or the organization's) responsibility to keep software up-to-date and read important announcements. If this feature was removed due to a security issue, you'd have to have checked the news anyway. "Updating as needed" requires checking in on changes regularly to find relevant issues. If you haven't looked at new versions for months then there is something wrong there. Maybe it's time to automate your deployment.

Regardless, I don't think we need to add this back. As @mholt said, a custom encoder can be used.

@francislavoie
Copy link
Member

(Oops, I accidentally linked a PR to this issue, ignore that)

I was actually assuming that Caddy uses semver – seems like I was wrong about this.

Part of this issue IMO is that Go makes it hard to actually follow semver properly because of how it deals with module paths. Notice that the module in v2 needs to have /v2 in it

caddy/go.mod

Line 1 in e3d04ff

module github.com/caddyserver/caddy/v2
see https://go.dev/blog/v2-go-modules for an explanation.

So if we made a v3, all plugins would have to explicitly update their code to use the new /v3 module path. For now, we're going to avoid as much as possible major API breaks that would require changes to all plugins.

See the commit e43b6d8 for an example, Matt wanted to deprecate passing an argument to ctx.Logger() because we have a smarter way of doing it now, but he was able to find a workaround using a variadic arg to essentially ignore the argument for now "until v3".

But right now, we have no actual plans for a v3. The core design is "fine". We have some small gripes, but nothing that would warrant actually drawing a hard line in the sand, breaking the whole v2 ecosystem, forcing everyone to update their plugins to be compatible with v3.

In other languages like PHP, it's possible to define a version constraint that supports multiple major versions, e.g. with ^2.1 || ^3.0 or simply >=2.1 to avoid actually specifying a possible upcoming major version. But I'm not aware of a way to do this in Go in a way that a plugin can support multiple major versions, especially being implicitly forwards-compatible with new major versions.

@simaotwx
Copy link
Contributor

So if we made a v3, all plugins would have to explicitly update their code to use the new /v3 module path. For now, we're going to avoid as much as possible major API breaks that would require changes to all plugins.

You have a really good point, IMO. Overall, I agree.
If Go 2 changes how this works, maybe we could think about moving to v3 and start adhering to semver rules.

@mholt
Copy link
Member

mholt commented Sep 20, 2022

On the topic of semantic versioning:

We do follow semver, mostly. We do fudge slightly on deprecations sometimes, as mentioned, but do it as gracefully and carefully as possible, since truly following semver would break WAY more than not as Francis mentioned.

But patch releases are usually just patches or very minor enhancements, and minor versions have the most changes.

We also try to be really clear about bigger things in release notes and logs if we ever tag a release that may be less conventional or more surprising.

Also, Caddy has a lot of surface area. Web clients, config API, Caddyfile, logs and metrics, so many inputs and outputs. It's inevitable that some of them will change on a living, breathing, moving Internet with evolving standards and best practices. In fact, not adapting would be even more breaking for people or put people at risk.

I have mixed feelings about absolute/pure semver because of that. It's like, "semver was made for man, not man for semver," kind of thing. Does it apply to exported Go identifiers? Caddyfile, JSON config? HTTP behavior? TLS behavior? All of it? To truly follow semver would presume no external systems, factors, or best practices change. But there's a lot of moving parts that Caddy brings together!

Part of the point of Caddy is to not be Apache or Nginx, which we know have not aged well to the modern Internet. So we follow semver in spirit and, the vast majority of the time, we follow it precisely. Most people's configs will not break from release to release, at least not without substantial notice beforehand.

So our approach is less, "how do we worship the one and only semver" and more, "how do we best serve our users and the wider Internet that Caddy touches"? And sometimes that means changing things. So we strive to do that in the best way possible.

As this is open source, it's a symbiotic relationship, and it really helps if people keep their software up to date, stay involved, participate in the development and feedback process, and in return we'll provide great software you can use freely. 😃

@francislavoie
Copy link
Member

With all that said, I'm not seeing anything actionable here, so I'll close this issue. I hope we've clarified enough why we do what we do.

@nsauk
Copy link
Author

nsauk commented Sep 20, 2022

Part of the point of Caddy is to not be Apache or Nginx, which we know have not aged well to the modern Internet.

Huh, nginx is reliable software that doesn't break things on upgrade. Every old valid nginx config I can find in the internet is still pretty valid. Moreover, a link to nginx docs posted somewhere on internet in 2012 still leads to the right page because of responsible redirects of nginx team. With caddy, from first link by request caddy cors I can go to broken page of your docs. Built-in upgrade command that replaces binary with a new one, that incompatible with the current config, is just a part of the big picture.

I liked caddy because it's a small portable thing that just works, I even never installed it using package manager, I just download/copy the file. But you think that I necessarily have to use JSON logs (and ELK stack on a big tech company's cloud, right? because I don't want to break my eyes with reading JSON), kubernetes and so on. I'm not an enterprise user of this web server, but as an employee of a company with a lot of web servers deployed, I can say that noone will use caddy instead of nginx until it will be reliable as nginx. Have a good luck to grow from 0.2% with such ideology.

@mohammed90
Copy link
Member

mohammed90 commented Sep 20, 2022

But you think that I necessarily have to use JSON logs (and ELK stack on a big tech company's cloud, right? because I don't want to break my eyes with reading JSON)

For the record, you don't have to use JSON. The human-readable console format is available.

@nsauk
Copy link
Author

nsauk commented Sep 20, 2022

@mohammed90 I got it. But this doesn't matter and isn't a topic of this issue. I claimed that you break working Caddyfile for no good reason. I see it as a sign of 'perpetual beta' and felt it important to convey my pain.

@AD-Wright
Copy link

@nsauk - I just went through implementing the workaround myself, it's actually become a lot more streamlined since the initial deprecation.

Workaround is as follows:

  • Replace all instances of format single_field common_log in your Caddyfile with format transform "{common_log}"
  • Add the transform encoder through the command caddy add-package github.com/caddyserver/transform-encoder
  • Verify proper operation through sudo systemctl restart caddy and testing
  • Upgrade Caddy to newest release through caddy upgrade and sudo systemctl restart caddy

The transform encoder module should remain loaded as long as subsequent upgrades are through caddy upgrade, if you use sudo apt upgrade you may have to add back the module. If, as you mentioned, you prefer to download a pre-compiled binary, you can go to https://caddyserver.com/download?package=github.com%2Fcaddyserver, select the transform-encoder package, and download a caddy binary with the package already included.

@francislavoie
Copy link
Member

francislavoie commented Oct 13, 2022

@AD-Wright if you're using the apt repo, consider following these steps to divert the binary so you don't risk overwriting it, and still getting updated to the support files (systemd config, bash completions, manpage, etc) https://caddyserver.com/docs/build#package-support-files-for-custom-builds-for-debianubunturaspbian

@AD-Wright
Copy link

Rather than diverting the binary, a simple sudo apt-mark hold caddy seems to still allow sudo caddy upgrade to do its thing, and prevent an accident sudo apt upgrade from breaking caddy.

Are there additional benefits to diverting the binary?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 The right solution needs to be found
Projects
None yet
Development

No branches or pull requests

6 participants