Don't cache private content in Varnish, only 200 and 404 responses. A…

[hostep]

…lso fixes some whitespace inconsistencies.

Description (*)

https://github.com/magento/magento2/pull/28927 accidentally added a bug, where it allows any response that doesn't have a 200 or 404 status code to become cached. Unless the Cache-Control header contains private.

This is incorrect, it should only cache 200 or 404 responses, unless they contains Cache-Control header private, not any other status codes.

I've also noticed some whitespace inconsistencies where indentation used 3 spaces instead of 4, so those got fixed as well here. Append ?w=1 to the url of this PR to see only the relevant changes.

Related Pull Requests

Fixed Issues (if relevant)

1. Fixes magento/magento2#36492

Manual testing scenarios (*)

1. Setup Magento with Varnish and configure Varnish with the VCL file that Magento generates in the backoffice
2. Test if a response with HTTP status 200 or 404 gets cached
3. Test if a response with HTTP status 301 or 302 does not get cached
4. Test if a response which contains header Cache-Control: private and HTTP status 200 or 404 does not get cached
5. Test if a response which contains header Cache-Control: private and HTTP status 301 or 302 does not get cached

Questions or comments

@icecactus: I hope I understood your report correctly, feel free to correct me or give better steps of how to test this fix

Contribution checklist (*)

• [x] Pull request has a meaningful description of its purpose
• [x] All commits are accompanied by meaningful commit messages
• [ ] All new or changed code is covered with unit/integration tests (if applicable)
• [ ] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• [ ] All automated tests passed successfully (all builds are green)

[m2-assistant[bot]]

Hi @hostep. Thank you for your contribution Here are some useful tips how you can test your changes using Magento test environment. Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

:exclamation: Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here

:information_source: Run only required test builds during development. Run all test builds before sending your pull request for review.

For more details, review the Magento Contributor Guide documentation.

:warning: According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

:clock10: You can find the schedule on the Magento Community Calendar page.

:telephone_receiver: The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, join the Community Contributions Triage session to discuss the appropriate ticket.

:pencil2: Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[hostep]

It probably does not make sense to run tests for this PR, not sure if there are automated tests that use Varnish?

But let me trigger it just once ...

@magento run all tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[hostep]

This PR will have some conflicts once https://github.com/magento/magento2/pull/38052 and https://github.com/magento/magento2/pull/36796 get approved and merged, but let's see which one will make it first (probably none of the 3 ones, if we see how Adobe has handled similar PR's in the past)

[engcom-Echo]

@magento run all tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Echo]

@magento run Unit Tests,Static Tests,Functional Tests EE

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Echo]

@hostep ,

Thank you for your contribution. We have tried to reproduce the issue but the issue is not reproducible on 2.4-develop.

Below are the steps we have followed.

1. Setup clean Magento 2.4-develop instance with Varnish and configure Varnish with the VCL file that Magento generates in the backoffice
2. Test - response with HTTP status 200 or 404 gets cached
3. Test - response with HTTP status 301 does not get cached
4. Test - response with Cache-Control header private with HTTP status 301 does not get cached

Can you please let us know if the issue still exists or we are missing any step.

[hostep]

@engcom-Echo: it sounds like you only tested 3 out of the 4 cases. You forgot to test:

• Test if a response which contains header Cache-Control: private and HTTP status 200 or 404 does not get cached

Can you try that one as well?

If that still works as expected, I think we'll need to pull in the help of a Varnish expert (which I'm certainly not myself unfortunately) to verify if there is still a bug or not in the VCL file.

[icecactus]

@engcom-Echo, @hostep is correct, you missed the test for the private header. Anything with a private header should not be cached.

[engcom-Echo]

If that still works as expected, I think we'll need to pull in the help of a Varnish expert (which I'm certainly not myself unfortunately) to verify if there is still a bug or not in the VCL file.

@hostep We have verify on 2.4-develop and PR branch but the behavior is same for below case

• Test if a response which contains header Cache-Control: private and HTTP status 200 or 404 get cached

Let us know if we are missing something here.

[engcom-Echo]

@hostep can you please have a look on above comment and suggest us if we have missed any steps?

[hostep]

I'm not an Varnish expert, so maybe we can pull in one here.

@gquintard: what do you think about this fix? For context, this originates from this discussion here: https://github.com/magento/magento2/issues/36492#issuecomment-1321206478

Do the changes in this PR look good to you? (use this link to view them without whitespace changes). Are my testing scenario's described correctly? Anything else that can help @engcom-Echo further?

[gquintard]

hi @hostep!

I think that PR is correct, I did break that caching behavior in https://github.com/magento/magento2/pull/28927 . Let me get you a vtc going so we can lock that down and avoid manual testing

[engcom-Echo]

@gquintard Have you had an opportunity to review the mentioned comment ?

[gquintard]

the one I answered to? Yes. I will write the varnishtest case when I have some time, hopefully this week

[gquintard]

@hostep, I've opened https://github.com/hostep/magento2/pull/2 on your fork. I just adds a vtc that, like for #38211 and #28944, you can run with:

varnishtest dev/tests/varnish/*.vtc

You'll notice the test failing before your patch is applied, and passing after.

The test case will be cleaner if/once #28944 is merged, but it will do for now.

[hostep]

Awesome!

I've cherry-picked your commit and added it to this PR here.

I can confirm the test passes with the fixes provided here and fails without them.

[engcom-Echo]

@magento run all tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Echo]

:heavy_check_mark: QA Passed

Before: :heavy_multiplication_x:

After: :heavy_check_mark:

As some builds are failing hence moving it to Extended Testing

[engcom-Echo]

@magento run Functional Tests EE,Functional Tests B2B,Integration Tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Echo]

Functional Tests B2B failures are different on last two run on same commit. Run1

Run2 StorefrontPrintListItemsFromRequisitionListTest

Hence Moving it to Merge in progress