✨ Cache middleware: Store e2e headers. #1807

thylong · 2022-03-05T15:23:33Z

Please provide enough information so that others can review your pull request:

As defined in RFC2616 - section-13.5.1, shared caches should store end-to-end headers from backend response and MUST be transmitted in any response formed from a cache entry.

This commit ensures a stronger consistency between responses served from the handlers & from the cache middleware. It also tries to not generate many more memory allocations and ops complexity.

Explain the details for making this change. What existing problem does the pull request solve?

The current cache middleware is minimalist and really fast but doesn't respect several aspects of HTTP/1.1 caching RFCs (RFC2616 & RFC7234).

Non-exhaustive list of unsupported features:

The Vary header
The Warning header
Cache entry containing additional headers

This causes response consistency issues such as the one described in the following scenario :

You have a simple endpoint with a cache middleware instantiated :

package main

import (
        "github.com/gofiber/fiber/v2"
        "github.com/gofiber/fiber/v2/middleware/cache"
)

func main() {
        app := fiber.New()
	app.Use(cache.New())

	app.Get("/", func(c *fiber.Ctx) error {
		c.Response().Header.Add("X-Foobar", "foobar")
		return c.SendString("hi")
	})

        # [...]
}

If you curl -D - http://localhost:8080/ this endpoint, your response will look like the following:

HTTP/1.1 200 OK
Date: Sat, 05 Mar 2022 15:12:26 GMT
Content-Type: text/plain; charset=utf-8
Content-Length: 6548
X-Cache: miss
X-Foobar: foobar

Now if you execute the curl command again, the second response will look like the following:

HTTP/1.1 200 OK
Date: Sat, 05 Mar 2022 15:12:27 GMT
Content-Type: text/plain; charset=utf-8
Content-Length: 6548
X-Cache: hit

⚠️ ☝️ The X-Foobar response header gets stripped as it's not store in the cache entry.

Note

@codemicro @ReneWerner87 @Fenny , do you think this should be optional as it changes the default behaviour of the cache middleware?
I'm hesitating to let this PR setting the new default cache middleware behaviour to make it RFC compliant but it could also catch off guard people already using the middleware. WDYT?

codemicro · 2022-03-05T15:42:04Z

I seem to recall that when we were first merging this into Fiber core, we thought about implementing such a feature and decided against it for whatever reason (I don't remember what exactly).

While I'm not on the maintainers team anymore, I think this would be a good addition to the cache middleware.

do you think this should be optional as it changes the default behaviour of the cache middleware?

I think it's a good idea to put this behind a flag of some sorts, yes, since this is changing observable behaviour of the cache middleware. I'm struggling to find a situation where it'd be a problem if more headers were preserved from the original request, but I'm think there probably are some.

Were this not the case, and it wasn't placed behind a flag, I'd suggest removing explicit caching of the Content-Type and Content-Encoding headers in favour of having it handled by the logic in this PR.

thylong · 2022-03-05T15:50:51Z

Thanks for your prompt answer on this @codemicro 🙏

I think it's a good idea to put this behind a flag of some sorts, yes, since this is changing observable behaviour of the cache middleware. I'm struggling to find a situation where it'd be a problem if more headers were preserved from the original request, but I'm think there probably are some.

IMHO, the risks reside in client identity specific headers (such as Set-Cookie) and backend application maintainers' potential mistakes. This could lead to security issues where the cache serves the same private cookie content to multiple clients.

Were this not the case, and it wasn't placed behind a flag, I'd suggest removing explicit caching of the Content-Type and Content-Encoding headers in favour of having it handled by the logic in this PR.

I was thinking about it as well and preferred to assess if this PR generates interest from the maintainers and you before going further.

I'll create a E2EHeaders flag that defaults to false to make this new feature safe for everyone 👍

ReneWerner87 · 2022-03-05T16:07:11Z

Thanks for the work,
Maybe you can provide a method for filtering the headers, which by default removes the cookie and security related header
Otherwise I think we could make this the default behavior

Maybe we should do some more research on what were the reasons for not saving and delivering all headers

I will have a look at the pull-requests later or tomorrow

thylong · 2022-03-05T16:11:22Z

Thanks for answer @ReneWerner87 !

Maybe you can provide a method for filtering the headers, which by default removes the cookie and security related header
Otherwise I think we could make this the default behavior

I introduced the E2EHeaders flag to turn off straight the feature but if you prefer to make the default behavio, I can provide a simple mechanism to exclude certain headers in this PR instead 👍

Maybe we should do some more research on what were the reasons for not saving and delivering all headers
I will have a look at the pull-requests later or tomorrow

Sounds good to me, let me know what you decide and I'll re-adjust if necessary 😊

P.S: I would be very interested to participate in cache middleware improvement efforts as I'm currently building several projects on-top of Fiber (closed sources for the moment, but will open them at some point)

ReneWerner87 · 2022-03-05T16:19:46Z

Gladly any pull request and improvements both functional and in the area of speed or tests are welcome

Of course this also applies to all other parts of the software

codemicro · 2022-03-05T16:31:22Z

the risks reside in client identity specific headers (such as Set-Cookie)

I agree, though if an endpoint is setting cookies, I'd argue that most of the time it's a bad idea to be caching any request to that endpoint.

Maybe you can provide a method for filtering the headers, which by default removes the cookie and security related header

Typically, I'd structure my application so that I'm specifically including endpoints to be cached as opposed to specifically excluding endpoints. I wonder if this is an approach should be encouraged, as opposed to implementing a feature to remove headers automatically like this?

Otherwise I think we could make this the default behavior

I think this still has the potential to break things in bizzare edge-cases or when this is used in an unusual manner.

Thanks for your prompt answer on this @codemicro 🙏

You're welcome!

thylong · 2022-03-05T18:15:30Z

In case you believe the flag doesn't offer enough flexibility for the end users, I might have 2 different suggestions that satisfies your two perspectives:

First (simple but whitelisting only)

The E2EHeaders can be a func(c *fiber.Ctx) map[string][]byte instead of just a bool (similar to Next() func).
This would handle all the headers we want to store in the cache.

Use cases

By default we would have a function returning an empty map and consequently it won't activate the feature
If a map contains headers, they will be stored in the cache
If the map contains a "*": []byte{} entry, we could allow all headers

Benefits

The default behaviour is safe for everyone as it doesn't change at all
We introduce only 1 flag instead of a boolean + a custom config to pass
It's possible to take a conservative approach through whitelist
⚠️ It's not possible to take a liberal approach through blacklisting
BUT possible to be very liberal through "*"

Second (more complex but blacklisting and/or whitelisting)

The E2EHeaders can be a func(c *fiber.Ctx) [2]map[string][]byte instead of just a bool.
This would handle all the headers we want to store in the cache and:

The first slice map would be a whitelist (to store)
The second slice map would be a blacklist of headers (to not store)

Use cases

By default we would have a function returning an empty map and consequently it won't activate the feature
If the whitelist map contains headers, they will be stored in the cache
If the blacklist map contains headers, they won't be stored in the cache
⚠️ ~~If the map contains a "*": []byte{} entry, we could allow all headers~~ would be confusing in this case
Blacklisting > whitelist in case of conflicts

Benefits

The default behaviour is safe for everyone as it doesn't change at all
We introduce only 1 flag instead of a boolean + a custom config to pass
It's possible to take a conservative approach through whitelist
It's possible to take a liberal approach through blacklisting

It feels like we would provide maximum flexibility to the end user without impacting current users.

codemicro · 2022-03-06T15:50:49Z

Personally, I'm not sure there needs to be flexibility in relation to what headers are included/excluded. Both your proposals here seem a little overcomplicated for this middleware, whereas the current state of this PR with a simple boolean flag looks alright to me.

ReneWerner87 · 2022-03-06T16:12:09Z

I will have a look at the pull-requests later or tomorrow

Look at it tomorrow, today i have unfortunately no time

thylong · 2022-03-06T17:37:26Z

Personally, I'm not sure there needs to be flexibility in relation to what headers are included/excluded. Both your proposals here seem a little overcomplicated for this middleware, whereas the current state of this PR with a simple boolean flag looks alright to me.

I understand your standpoint and starting with this simple flag instead of increasing the complexity suddenly could be indeed a good option to start with and satisfy the current need 👍

ReneWerner87

please adjust the readme
please create a pull request for the documentation repository - https://github.com/gofiber/docs/blob/master/api/middleware/cache.md

ReneWerner87 · 2022-03-07T07:27:22Z

middleware/cache/cache.go

@@ -133,6 +149,20 @@ func New(config ...Config) fiber.Handler {
 		e.status = c.Response().StatusCode()
 		e.ctype = utils.CopyBytes(c.Response().Header.ContentType())
 		e.cencoding = utils.CopyBytes(c.Response().Header.Peek(fiber.HeaderContentEncoding))
+		e.e2eHeaders = make(map[string][]byte)


why convert to string? that would take time

As it's impossible to determine both header key and value ahead of time, it feels like a map is needed to compare ignoreHeaders with response headers with a O(1) complexity.

If you have another approach to recommend, I take it 🙏

middleware/cache/cache_test.go

middleware/cache/config.go

As defined in RFC2616 - section-13.5.1, shared caches MUST store end-to-end headers from backend response and MUST be transmitted in any response formed from a cache entry. This commit ensures a stronger consistency between responses served from the handlers & from the cache middleware.

Set flag to prevent e2e headers caching to be the default behavior of the cache middleware. This would otherwise change quite a lot the experience for cache middleware current users.

E2E is an acronym commonly associated with test. While in the present case it refers to end-to-end HTTP headers (by opposition to hop-by-hop), this still remains confusing. This commits renames it to a more generic name.

middleware/cache/cache.go

ReneWerner87 · 2022-03-07T14:34:57Z

@thylong i added 2 more comments for performance improvements stuff

This will prevent an extra memory allocation for users not interested in this feature.

…rs is disabled.

thylong · 2022-03-07T15:32:26Z

Thanks for your review @ReneWerner87 !

Feedbacks:

✅ I renamed the option from E2EHeaders to StoreResponseHeaders and adjusted the rest of the codebase accordingly
✅ A new benchmark test has been added as requested
✅ You'll find the PR for the documentation here
✅ Fixed the 2 allocations outside of if statements
⚠️ Not quite sure how to solve this the best way

Requesting your review again 🙂

ReneWerner87 · 2022-03-07T17:12:18Z

I will do the last point.

- use set instead of add for the headers - copy value from the headers -> prevent problems with mutable values

ReneWerner87 · 2022-03-08T08:53:39Z

the last point is not so easy

have added something anyway

thylong · 2022-03-08T09:02:24Z

the last point is not so easy

Indeed... Thanks for the the help though ! I'll keep that in mind for later contributions, in case they generate opportunities to make this better.

efectn added the ✏️ Feature label Mar 5, 2022

ReneWerner87 requested changes Mar 7, 2022

View reviewed changes

thylong added 3 commits March 7, 2022 14:47

✨ Cache middleware: Add flag for e2e headers.

ca2305d

Set flag to prevent e2e headers caching to be the default behavior of the cache middleware. This would otherwise change quite a lot the experience for cache middleware current users.

✨ Cache middleware: Add Benchmark for additionalHeaders feature.

583802e

thylong force-pushed the feat/cache-middleware-e2e-headers branch from 751553a to 583802e Compare March 7, 2022 13:56

thylong added 2 commits March 7, 2022 15:10

✨ Cache middleware: Update README

d803a05

thylong mentioned this pull request Mar 7, 2022

✨ Cache middleware: Document new StoreResponseHeaders feature. gofiber/docs#247

Merged

ReneWerner87 reviewed Mar 7, 2022

View reviewed changes

middleware/cache/cache.go Outdated Show resolved Hide resolved

ReneWerner87 reviewed Mar 7, 2022

View reviewed changes

middleware/cache/cache.go Outdated Show resolved Hide resolved

thylong added 2 commits March 7, 2022 15:38

✨ Cache middleware: Move map instanciation.

699f70a

This will prevent an extra memory allocation for users not interested in this feature.

✨ Cache middleware: Prevent memory allocation when StoreResponseHeade…

c8371eb

…rs is disabled.

thylong requested a review from ReneWerner87 March 7, 2022 15:33

✨ Cache middleware: Store e2e headers. gofiber#1807

87099e5

- use set instead of add for the headers - copy value from the headers -> prevent problems with mutable values

ReneWerner87 approved these changes Mar 8, 2022

View reviewed changes

ReneWerner87 merged commit 1cddc56 into gofiber:master Mar 8, 2022

thylong deleted the feat/cache-middleware-e2e-headers branch March 8, 2022 11:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Cache middleware: Store e2e headers. #1807

✨ Cache middleware: Store e2e headers. #1807

thylong commented Mar 5, 2022 •

edited

codemicro commented Mar 5, 2022

thylong commented Mar 5, 2022

ReneWerner87 commented Mar 5, 2022

thylong commented Mar 5, 2022 •

edited

ReneWerner87 commented Mar 5, 2022

codemicro commented Mar 5, 2022 •

edited

thylong commented Mar 5, 2022

codemicro commented Mar 6, 2022 •

edited

ReneWerner87 commented Mar 6, 2022 •

edited

thylong commented Mar 6, 2022

ReneWerner87 left a comment •

edited

ReneWerner87 Mar 7, 2022

thylong Mar 7, 2022

ReneWerner87 commented Mar 7, 2022

thylong commented Mar 7, 2022

ReneWerner87 commented Mar 7, 2022

ReneWerner87 commented Mar 8, 2022

thylong commented Mar 8, 2022

✨ Cache middleware: Store e2e headers. #1807

✨ Cache middleware: Store e2e headers. #1807

Conversation

thylong commented Mar 5, 2022 • edited

Note

codemicro commented Mar 5, 2022

thylong commented Mar 5, 2022

ReneWerner87 commented Mar 5, 2022

thylong commented Mar 5, 2022 • edited

ReneWerner87 commented Mar 5, 2022

codemicro commented Mar 5, 2022 • edited

thylong commented Mar 5, 2022

First (simple but whitelisting only)

Use cases

Benefits

Second (more complex but blacklisting and/or whitelisting)

Use cases

Benefits

codemicro commented Mar 6, 2022 • edited

ReneWerner87 commented Mar 6, 2022 • edited

thylong commented Mar 6, 2022

ReneWerner87 left a comment • edited

Choose a reason for hiding this comment

ReneWerner87 Mar 7, 2022

Choose a reason for hiding this comment

thylong Mar 7, 2022

Choose a reason for hiding this comment

ReneWerner87 commented Mar 7, 2022

thylong commented Mar 7, 2022

ReneWerner87 commented Mar 7, 2022

ReneWerner87 commented Mar 8, 2022

thylong commented Mar 8, 2022

thylong commented Mar 5, 2022 •

edited

thylong commented Mar 5, 2022 •

edited

codemicro commented Mar 5, 2022 •

edited

codemicro commented Mar 6, 2022 •

edited

ReneWerner87 commented Mar 6, 2022 •

edited

ReneWerner87 left a comment •

edited