Add Cache Manager support for Accept header

[yadij]

This completes the support in cachemgr Action classes for
incremental addition of report formats other than text/plain
(historic cachemgr.cgi accepted syntax).

Reporters (class Action children) can now determine their
content-type support dynamically based on request
Accept header.

[rousskov]

This PR intentionally excludes adding special formatter class(es) for use by the report generator.

I believe that we must start with the right API, including formatting classes, instead of making actions YAML-aware and adding YAML-specific formatting code to hand-picked actions.

IIRC, the topic of introducing YAML support has been discussed on squid-dev at some point. If so, please reference that discussion in the PR description.

[rousskov]

As a workaround for a GitHub UI limitation, I am attaching two "global" change requests to the first (and innocent) line in the PR instead of listing them here, in this review comment, because change requests in such review comments are often ignored and their discussion is difficult to follow.

[rousskov]

This PR assumes that future Actions should react to Accept request header values, presumably producing output of two (or more) different Content-Types. Why is that a good idea? Should not all Actions simply respond using YAML instead, disregarding the Accept request header value (or validating it centrally, before the Action is even created and forwarded to workers)? If we have already agreed to proceed with this design, please feel free to point me to that agreement, of course.

[yadij]

The ad-hoc format accepted by cachemgr.cgi is not fully compliant YAML. So to retain support for tools expecting that syntax we should only send YAML when it is indicated by the client Accept header.

IIRC, we agreed to the principal of moving to YAML, but still have disagreement on the specifics.

[rousskov]

The ad-hoc format accepted by cachemgr.cgi is not fully compliant YAML.

Not sure why you are talking about cachemgr.cgi as if it specifies what output Squid does or should produce. That CGI script is just a hack that we fully control. It will accept whatever we make it accept. It does not and should not determine Squid output format(s). I do agree that Squid output is not YAML today, of course, but that fact has nothing to do with cachemgr.cgi.

So to retain support for tools expecting that syntax we should only send YAML when it is indicated by the client Accept header.

IMO, we do not want to add code that works towards retaining support for tools requiring non-YAML syntax because we do not want to create and maintain code that generates responses in two different formats, especially when one of those formats is an undefined/unstructured/ad-hoc/custom/informal format. We want to move towards YAML, not towards YAML+informal. We cannot get to YAML immediately, of course, but I see no value (and quite a bit of harm) in enabling this kind of Action code:

if (clientAcceptsYaml)
    generateYamlResponse();
else
    generateInformalResponse();

And if the above is not what we want, then I see no value in telling Actions what formats the client accepts.

IIRC, we agreed to the principal of moving to YAML

Yes, that matches my recollection as well.

but still have disagreement on the specifics.

That is probably true, but this change request disagreement appears to be about the very target/direction of that move rather than any move specifics. If we target just one format, then Actions do not need to know what formats the client accepts.

[rousskov]

I do not understand the current description¹ of the feature label, but perhaps this PR does not need that mysterious label?

Footnotes

1. maintainer needs documentation update for merge: "For merge" of which PR? Where? It does not sound like we are talking about merging the labeled PR into its target branch... Please update the label name and description to clarify. If this label is about the need to update Squid release notes and such (after the PR is merged), then let's say that explicitly, preferably with a common reusable prefix! For example, todo-release-notes or todo-update-release-docs. ↩

[yadij]

Cache manager is no longer planned to present anything other than YAML. Closing as unneeded