Remote Asset API: clarification about Qualifier

[sluongng]

I have been working with Remote Asset API for a bit recently and I noticed a few points:

First of all, most implementations I found in the wild do not follow this specification and won't reject qualifiers that they don't support.

// In the case of unsupported qualifiers, the server SHOULD additionally
// send a [BadRequest][google.rpc.BadRequest] error detail where, for each
// unsupported qualifier, there is a FieldViolation with a field of
// qualifiers.name and a description of "{qualifier}" not supported
// indicating the name of the unsupported qualifier.

If servers were to follow this paragraph, it would lock the implementation to a set of qualifiers and make it non-forward compatible with newer clients. So I think we should consider removing this paragraph entirely or changing the wording to COULD instead of SHOULD.

---

Another related point is that for Bazel, there are non-standard qualifiers used https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/remote/downloader/GrpcRemoteDownloader.java;l=76-85;drc=618c0abbfe518f4e29de523a2e63ca9179050e94

  // The `Qualifier::name` field uses well-known string keys to attach arbitrary
  // key-value metadata to download requests. These are the qualifier names
  // supported by Bazel.
  private static final String QUALIFIER_CHECKSUM_SRI = "checksum.sri";
  private static final String QUALIFIER_CANONICAL_ID = "bazel.canonical_id";
  
  // The `:` character is not permitted in an HTTP header name. So, we use it to
  // delimit the qualifier prefix which denotes an HTTP header qualifer from the
  // header name itself.
  private static final String QUALIFIER_HTTP_HEADER_PREFIX = "http_header:";

Should we document these in build/bazel/remote/asset/v1/qualifiers.md?

[EdSchouten]

Nitpicking: COULD is not part of RFC 2119. https://datatracker.ietf.org/doc/html/rfc2119

If we were to make a change along these lines, we should use MAY instead.

[EricBurnett]

This not being forward-compatible is correct. The client likely needs to pay attention to the version of the server it is being used with, and not pass qualifiers that are not supported.

The reason that unknown qualifiers SHOULD be rejected is that doing otherwise is a correctness issue. If a client asks for a checksum.sri and the server returns a response that doesn't match that SRI, that's a problem. If a client asks for a response at one commit, and gets a different commit back, that's a problem.

Now, if there are qualifiers that are actually benign to ignore, the server can maybe be updated to do so. But most of the time I'd expect all qualifiers to be meaningful, and so none should be ignored.

Should we document these in build/bazel/remote/asset/v1/qualifiers.md?

It'd be nice to document them there, yeah. Not strictly required - if bazel and a server agree on a qualifier, it need not be generally standardized - but still, it'd sure make interop with bazel and between other systems if there was one single place we could document the "system-specific" qualifiers, and turn them into proper standards as appropriate.