fastly / fastly-magento2

Module for integrating Fastly CDN with Magento 2 installations
BSD 3-Clause "New" or "Revised" License
146 stars 120 forks source link

Enable extending VCL from Magento modules #503

Closed enl closed 2 years ago

enl commented 2 years ago

The problem we are trying to solve

We have quite a lot of projects on Magento Cloud and therefore using Fastly. These projects have some common functionality that would be nice to have as a separate packages (to support them only once). It’s all fun and games until some of these shared functionalities need to manage VCL code.

For example, we are trying to build a simple module that handles redirects:

Obvious, these features require some VCL management. Fastly module provides a way to manage VCL snippets from Magento Admin, which is awesome, but does not fully solve our problem, because the module kinda loses its purpose of being plug-n-play thing (one needs to remember to go to Admin and copy/paste these VCL snippets into Admin).

What we do now is we have an AFTER plugin that hooks into \Fastly\Cdn\Model\Config::getCustomSnippets and loads other modules VCL snippets (modules can register an implementation of special interface in a composite). This allows us to easily ADD custom snippets before they get uploaded.

But… how do I delete custom snippet?

Well, what if I first enabled “www to no www” redirect, but then I decided I want it vice-versa? In this case, extension must delete one VCL and install another one, but implementation of getCustomSnippets doesn’t call for “snippets to delete”, it calls only for “snippets to add”.



I have several options (one worse than the other)



Merge VCL snippets and build-in feature flags










declare local var.www_redirect_strategy STRING;
set var.www_redirect_strategy = “####SOME_MAGIC_REPLACEMENT####”;

If (var.www_redirect_strategy == “from_www”) {
    // handle from www redirect
} else if (var.www_redirect_strategy == “to_www”) {
    
    // handle to www redirect

}




Yes, it allows us to avoid deleting VCL snippet in this case by making one snippet out of two and making it much more clunky.

Extend a plugin so that it allows modules to delete snippets

My main concern here is it goes way beyond Fastly\Cdn\Model\Config::getCustomSnippets functionality (it not only provides a list of custom snippets but deletes unused snippets now).

Having an event after handling built-in custom snippets functionality in \Fastly\Cdn\Controller\Adminhtml\FastlyCdn\Vcl\Upload::execute would really help me :)



foreach ($customSnippets as $key => $value) {
}



$this->eventManager->dispatch('fastly_custom_snippets', [
    


    'api' => $this->api,
    'version' => $clone
]);

In this case, even subscriber would be able to call other modules to build an unit of work to be done in order to sync "module VCL snippets"

So, yeah, thanks for reading this :) I’d like to hear your concerns about the way I’m going and my suggestion.

vvuksan commented 2 years ago

Hi @enl

To generate and upload VCL snippets you do not have to use the UI. If you place them in

$MAGENTO_HOME/var/vcl_snippets_custom

directory per this document

https://github.com/fastly/fastly-magento2/blob/master/Documentation/Guides/CUSTOM-VCL-SNIPPETS.md

They will be uploaded with the stock VCL snippets.

That said the delete functionality is available only through the UI. Would this work for you?

enl commented 2 years ago

Hello @vvuksan,

There are several problems with this approach:

  1. var/ is not under VCS on Magento Cloud, so that I either have to make sure I manually copied-pasted the files to this folder, which is essentially same as using UI but with extra steps, or do some magic...
  2. We tried to make an extension that will gather vcl snippets and put them all to this var/ folder, which worked like a charm indeed until we realised that when we delete them from the folder and reupload VCLs, nothing happens.
  3. Which turns us to the root of the evil: deletion of VCL is available only from UI.

I get why it is so when you are working with files: if file was deleted, how does Fastly module know it ever existed?

This is actually a reason I started digging into fastly internals =)

So, answering your "would this work for you" question... Unfortunately, no, it would not. I'd like to have an opportunity to inject into "VCL snippets sync" process a bit more intrusively, e.g. by creating a unit of work (upload this snippet, delete this one, etc...)

vvuksan commented 2 years ago

Unfortunately the VCL snippets feature was designed for a limited set of cases. As you point out the delete functionality is tricky to achieve because you may have snippets that have dependencies between them so making you delete one snippet at a time with the UI was a "workaround". Also since this interface can coexist with the Fastly UI we didn't want to provide for the ability to just sync snippets as that could really ruin things. In addition this is not something most users do which is why we haven't spent a whole lot of time on it.

Could I suggest using the Fastly API directly to delete/upload new snippets ? I totally understand that it may be nicer to do it in the plugin. I just fear we may be adding additional complexity.

enl commented 2 years ago

Well, making it completely separate from "Upload VCL to Fastly" button in Admin is not really a good idea from both technical and UI perspectives: You now have to press two buttons AND it can break things even worse if you have to press two buttons that interact with a third-party server in a very complex way: I would need to create new version of vcl, validate it, etc...

Having an event fired right before you call validateServiceVersion for example, would solve all my issues and make sure that we do not do redundant stuffs via API. I mean I'm totally fine with calling Fastly API directly, but what I'd like to avoid is having \Fastly\Cdn\Controller\Adminhtml\FastlyCdn\Vcl\Upload::execute that creates new version of VCL, and my code that is hooked right after it that creates yet another one.

vvuksan commented 2 years ago

We'll look into addressing this.

dpotkoc commented 2 years ago

Hello @enl , we added logic for removing custom snippets. If you are adding custom snippet via plugin, then snippet name should have prefix: "magentomodule_".

Thnx Domagoj