Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(plugins): Caddy config cache_keys json parse error #460

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dmagyari
Copy link

@dmagyari dmagyari commented Mar 1, 2024

I'm not sure, how the cache_keys with regexp should exactly work by specification, but the following example in the Caddyfile did not work.

    cache {
        cache_keys {
            /.+ {
                headers Authorization
            }
        }
    }

The json parser of the CacheKeys struct shifted directives up and the regexp become the directive form inside the block.

@darkweak
Copy link
Owner

darkweak commented Mar 1, 2024

IMHO that's not an issue caused by the JSON parser.

Comment on lines 140 to 147
resp1, _ := tester.AssertGetResponse(`http://localhost:9080/query-string?query=string`, 200, "Hello, query string!")
if resp1.Header.Get("Cache-Status") != "Souin; fwd=uri-miss; stored; key=GET-http-localhost:9080-/query-string" {
t.Errorf("unexpected Cache-Status header %v", resp1.Header)
}
resp2, _ := tester.AssertGetResponse(`http://localhost:9080/query-string?foo=bar`, 200, "Hello, query string!")
if resp2.Header.Get("Cache-Status") != "Souin; fwd=uri-miss; stored; key=GET-http-localhost:9080-/query-string?foo=bar" {
t.Errorf("unexpected Cache-Status header %v", resp2.Header)
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you disable query within the cache_key then you can expect two stored responses because it should generate a key like that GET-http-localhost:9080-/query-string for both so the first one will receive a response that is stored and the second one will get a cached response.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick reply.
If I run the new test case alone with go test -run TestCacheKeys, then it passes, but without the run filter the same test fails. Could be a persistent state between tests?

I expected the new test case to be valid because the query= regexp matches the first request and modifies the key generation to exclude the query string, and the key becomes GET-http-localhost:9080-/query-string. The second request does not match the regexp so the key generation should include the query string and the key becomes GET-http-localhost:9080-/query-string?foo=bar. Since it is a different key its a miss and store.
At least this was my understanding of the docs. What is the expected behavior in this case?

@dmagyari
Copy link
Author

dmagyari commented Mar 2, 2024

I changed the path in the test url to be unique among all tests, now running all tests passes.

@darkweak
Copy link
Owner

darkweak commented Mar 2, 2024

You were right I read to fast your PR, thank you 🙏. I still have a concern about the JSON parser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants