Relaxed Vary header defaults

[seun-ja]

Motivation

Fixes #539

Relaxes the default Vary header to an empty list.

Solution

Instead of the custom Default implementation adding some header values, it now simply returns the out-of-the-box Default, which is an empty Vector.

Also, I altered the test to reflect the new change and an additional test.

[jplatte]

This is worse than the current state of things IMO. Has the current default caused some specific issue for you?

[seun-ja]

No @jplatte.

I haven't had any issue using it in this context.

I'm just fixing because of the issue raised in #539 and seems it was ok to fix as you were ok with PR on it

[jlizen]

++ to @jplatte 's feedback that this will break a lot of users since we are shifting from "conservative" defaults (include vary headers unnecessarily), to empty as a default, meaning we force anybody with dynamic CORS to manually set the header or risk caching correctness problems. Definitely a breaking change, probably not a justifiable one.

Would it work to instead have permissive() implicitly set .vary(Vary::list([])) rather than shifting defaults globally?

Or are there other overly conservative cases you are concerned with @seun-ja ?

[jplatte]

So, what I was thinking of when writing the referenced comment was having the various methods like allow_origin subtract from the vary list (only if not customized), if and only if the input is one that doesn't result in different header values for different requests. For AllowOrigin this would be any and exact (AllowOriginInner::Const). For AllowHeaders it would be any and list (AllowHeadersInner::Const).

[jlizen]

The approach of eagerly stripping the vary headers based on permissive() creates a problem:

CorsLayer::permissive().allow_origin(AllowOrigin::predicate(...))

This would produce no vary: origin header, when in fact we might have dynamic CORS that has multiple allowed origins. That could result in a stale cache improperly rejecting origins that DO match the predicate.

The proper way to handle this would be, like jplatte@ suggested, doing the stripping on the explicit allow_* methods.

And then, we also would need an extra Is_custom_vary parameter in the builder (or something), to make sure that we don't overwrite the user's explicit configuration.

[jplatte]

Instead of this complicated logic to add back the vary header if it has been removed from the default list, I think we could just do the checks to omit some vary headers in the Layer impl for CorsLayer (and additionally the Service impl of Cors, since that one also has the builder-style methods, though they are probably much less frequently used). What do you think?

[seun-ja]

Yeah this makes more sense.

It's a lot cleaner and maintainable

[jlizen]

Coming together! Down to just small tweaks.

[jlizen]

Can we extract this logic into a shared helper that can be used across both Layer::layer() and Cors::map_layer()

[jlizen]

This logic looks fine in that it won't ever remove Vary headers from potentially dynamic CORS configurations.

But, it could be broadened to also include eg: AllowOrigin::exact("http://example.com").

Also the list method for headers/methods are constant (return all values), only the origin will vary what is returned per request.

The current approach is fine as a first step, optionally you could leave a TODO to sweep up the rest, or just add the others in now.

[jlizen]

I don't think we need these clippy annotations on this test. Can we flip them over to expect() for all usages in this file to avoid future buildup?

[jlizen]

Can we also have tests for:

• "mixed" case where eg origin is a wildcard, but headers/method include vary headers
• very_permissive emitting vary headers
• at least a couple smoke tests on the Cors::map_layer test with and without vary headers

[jlizen]

This should be updated to explain that the default is derived from which other configuration is set, and that setting it explicitly will pin it regardless of other configuration.