(FOR REVIEW) (SERVER-299) Craft custom 404 error message #3532
Conversation
| @@ -5,6 +5,10 @@ module Puppet::Network::HTTP | |||
| MASTER_URL_PREFIX = "/puppet" | |||
| CA_URL_PREFIX = "/puppet-ca" | |||
|
|
|||
| NOT_FOUND_ERROR_MESSAGE = 'This master is running Puppet 4, and the URL format has changed. ' + | |||
| 'This request was likely made with a Puppet 3 Agent, as it used an invalid ' + | |||
There was a problem hiding this comment.
Should the message report PUPPETVERSION rather than a fixed string of "Puppet 4"? E.g. if someone has puppet 5 this would be confusing. But I could also be persuaded that we can cross that bridge when we come to it.
There was a problem hiding this comment.
Starting with puppet 3.3.1, we have X-Puppet-Version which we could use for more accurate reporting of the agent version.
There was a problem hiding this comment.
I have the same comment as @kylog made here and @nfagerlund expanded on, with detail, at https://github.com/puppetlabs/puppet-server/pull/371/files#r23735044
|
Can you create a PUP ticket for this and reference it in the commit message? We have a release script that expect PUP-xxx. |
|
CLA signed by all contributors. |
Craft a custom 404 error message indicating to the user that the request has failed, and stating that this may be because they are running a Puppet 3 agent and if so they should upgrade to Puppet 4.
MSLilah
force-pushed
the
SERVER-299-404-error-message
branch
from
8cad433
to
23b5268
Compare
| def self.not_found | ||
| Puppet::Network::HTTP::Route. | ||
| path(/.*/). | ||
| any(lambda do |req, res| | ||
| raise Puppet::Network::HTTP::Error::HTTPNotFoundError.new("No route for #{req.method} #{req.path}", Puppet::Network::HTTP::Issues::HANDLER_NOT_FOUND) | ||
| raise Puppet::Network::HTTP::Error::HTTPNotFoundError.new("No route for #{req.method} #{req.path} #{Puppet::Network::HTTP::NOT_FOUND_ERROR_MESSAGE}", | ||
| Puppet::Network::HTTP::Issues::HANDLER_NOT_FOUND) |
There was a problem hiding this comment.
So with this change, anything that uses this helper will have this error message. So, for example, a request to /puppet/v2.0/unknown would get this message, even though that does have the prefix and version string, so is probably not coming from a Puppet 3 agent. Given that we're removing the v2.0 endpoints, I'm not actually sure for right now that there will be anywhere that want to be using a not_found helper that wouldn't want this custom error message, but I feel like it might be good to have a general not_found helper and then a more specific one with this error message?
There was a problem hiding this comment.
I think @rlinehan has it right, this error message should only be displayed to agents using old URL's that were previously valid, but are not in later versions.
There should be a generic error message for generic resource not found errors. If there isn't a generic 404 handler, then at the very least the specific upgrade helper message should fire only when an agent that needs to be upgraded is making requests.
| @@ -47,7 +47,9 @@ def respond(text) | |||
| req = Puppet::Network::HTTP::Request.from_hash(:path => "/unknown") | |||
| res = {} | |||
| handler.process(req, res) | |||
| res_body = JSON(res[:body]) | |||
There was a problem hiding this comment.
Generally not a good idea to use JSON as a method like this. Are you trying to serialize to JSON, or deserialize from JSON? I suggest changing to JSON.parse to be a bit more clear and also make it easier to intercept the explicit message sent to JSON, rather than an implicit message.
|
Closing in favor of #3554 |
Craft a custom 404 error message indicating to the user that
the request has failed, and stating that this may be because they
are running a Puppet 3 agent and if so they should upgrade to
Puppet 4.
This change corresponds to the change made to Puppet Server in puppetlabs/puppetserver#371. As in that PR, there is going to need to be some discussion as to what the new 404 message should actually say.