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

Extend surrogate keys instead of replacing them #719

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

Conversation

herzog31
Copy link

  • Update logic in fetch to extend surrogate keys instead of replacing them if the origin returns any.
  • This is required for origins outside of Commerce which depend on surrogate keys for cache invalidation (e.g. Adobe Experience Manager).

I open this pull request as draft as this is untested for now.

@herzog31 herzog31 marked this pull request as ready for review November 21, 2024 14:32
@sirugh
Copy link

sirugh commented Nov 21, 2024

FYI I have applied this directly to https://manage.fastly.com/configure/services/EOYnA7LoQmtBNB2xROd8C3/versions/57/snippets?customerId=3KO4CgMCsDyiIhA2EHU6Vw in the magentomodule_fetch snippet. We are validating now.

@rcaril
Copy link
Contributor

rcaril commented Dec 12, 2024

Hi @herzog31 - we had some concerns with extending the keys, especially around Fastly limits. Can you further explain why this functionality is needed for cache invalidation?

@herzog31
Copy link
Author

Hi @rcaril,

the use case here is primarily about integrating different backends/origins which rely on surrogate keys for cache invalidation. For example Adobe Experience Manager (AEM).

For example you add AEM as origin in addition to Commerce to Fastly to deliver content pages. AEM will tag these pages with surrogate keys. When an author changes one of these pages, AEM will send an invalidation request with the corresponding surrogate key to Fastly to purge the cache. If the fastly-magento2 module overwrites all surrogate keys of paths coming from AEM with the value text, this mechanism breaks. Ideally the module does not touch the surrogate keys of AEM at all, but this would potentially require more changes.

You can run this command to see some example surrogate keys of AEM pages:

curl -Is -H 'Fastly-Debug: 1' https://main--aem-boilerplate-commerce--hlxsites.hlx.live | grep surrogate-key

surrogate-key: main--aem-boilerplate-commerce--hlxsites eXX8Ith1EDHl8Z1h 40b5fa1953019ef8aae1d581ed6fec610198c806f67247ec96653904ccf_metadata main--aem-boilerplate-commerce--hlxsites_head 40b5fa1953019ef8aae1d581ed6fec610198c806f67247ec96653904ccf main--aem-boilerplate-commerce--hlxsites.hlx.live

In our current setup, we solve this issue by skipping the VCLs of the module entirely. This makes the whole configuration overly complex though as you can see in the following documentation:

@kpfleming
Copy link

That sounds like a reasonable thing to do, but we need to ensure that the implementation won't duplicate the keys when a request goes through a Delivery service that is using shielding. At the moment the duplication happens on both the edge and the shield, which causes the list of keys to grow rapidly. If the code can be improve to only manipulate the key list at the edge (and not at the shield), at least that issue would be resolved.

@herzog31
Copy link
Author

@kpfleming Could you provide some guidance on what condition we would need to add so that the key change only happens on edge, but not at the shield? Thank you.

@kpfleming
Copy link

@herzog31
Copy link
Author

Thanks @kpfleming, I updated the code accordingly.

@sirugh could you please test the change on our environment? I couldn't test it with Fastly fiddle, as the value for fastly.ff.visits_this_service there is always 1.

@herzog31
Copy link
Author

We tested the change again on our environment (aemshop.net) and confirmed that the surrogate keys still work.

Co-authored-by: Kevin P. Fleming <kpfleming@users.noreply.github.com>
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.

4 participants