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(proxy-cache): keep cache_method same with nginx's proxy_cache_methods #4814

Merged
merged 2 commits into from
Aug 15, 2021

Conversation

agile6v
Copy link
Member

@agile6v agile6v commented Aug 12, 2021

What this PR does / why we need it:

Nginx's proxy_cache_methods directive only allows to specify GETPOST and HEAD, so we keep same with Nginx.

#4776

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@@ -596,7 +596,7 @@ http {
proxy_cache $upstream_cache_zone;
proxy_cache_valid any {% if proxy_cache.cache_ttl then %} {* proxy_cache.cache_ttl *} {% else %} 10s {% end %};
proxy_cache_min_uses 1;
proxy_cache_methods GET HEAD;
proxy_cache_methods GET HEAD POST;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nginx by default doesn't enable the cache for POST. Why change this behavior here?

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't affect the default behavior of APISIX, which means that user can specify the POST method on APISIX.

Copy link
Member

Choose a reason for hiding this comment

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

@agile6v
Could you sync it to APISIX.pm?

Copy link
Member

Choose a reason for hiding this comment

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

Is it necessary to cache a POST request?

The only I can think of for caching POST requests is that some internal specification forces the consistent use of POST requests……

Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't affect the default behavior of APISIX, which means that user can specify the POST method on APISIX.

Here you change the Nginx template, without a configurable setting. This is a change for default havior.

Copy link
Member Author

Choose a reason for hiding this comment

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

@agile6v
Could you sync it to APISIX.pm?

done

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it necessary to cache a POST request?

The only I can think of for caching POST requests is that some internal specification forces the consistent use of POST requests……

Yes. This is a very rare scenario. The users don't enable the method on APISIX, it doesn't take effect. If users enable it, they need to know what they are doing.

Copy link
Member Author

Choose a reason for hiding this comment

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

It doesn't affect the default behavior of APISIX, which means that user can specify the POST method on APISIX.

Here you change the Nginx template, without a configurable setting. This is a change for default havior.

Whether to enable is here (cache_method)

@spacewander spacewander merged commit ca5ea1f into apache:master Aug 15, 2021
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

4 participants