Skip to content
This repository has been archived by the owner on Apr 18, 2024. It is now read-only.

adds support for varnish_ban_key #29

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

larsboldt
Copy link
Contributor

VARNISH_BAN_KEY lets you specify a unique id which you can pick up in your VCL and issue the correct bans. Very useful when you have multiple sites hosted on the same backends and you want to control what to ban.

@ostark
Copy link
Owner

ostark commented Mar 13, 2019

I thought this is solved by the keyprefix already, isn't it?

@larsboldt
Copy link
Contributor Author

Key Prefix prevents key collisions when you have multisites in CraftCMS or more CraftCMS installations on same varnish server.

Ban key lets you know which domains to ban (based on your own logic in regards to the ban key) when you want to clear the entire cache for a certain domain.

So they are different things, but both needed in a multisite setup.

@larsboldt
Copy link
Contributor Author

The BAN is issued when using ./craft clear-caches/upper-purge-all at which point the key prefix is not in the picture. All it does now is issue a ban request to varnish, but you have no way of knowing what to ban, that is what the ban key lets you determine.

@larsboldt
Copy link
Contributor Author

any chance this is getting in soon, need it in production asap? if not I'll just redirect to my fork

@ostark
Copy link
Owner

ostark commented Mar 18, 2019

@larsboldt
Depending on your vcl it should not hurt to include the Varnish_Ban_Key to any request, right?

// config
// ...
'varnish'    => [
            'tagHeaderName'   => 'XKEY',
            'purgeHeaderName' => 'XKEY-PURGE',
            'purgeUrl'        => getenv('VARNISH_URL') ?: 'http://127.0.0.1:80/',
            'headers'         =>['Host' => getenv('VARNISH_HOST'), 'Varnish_Ban_Key' =>getenv('VARNISH_BAN_KEY')] : []
        ],

@larsboldt
Copy link
Contributor Author

@ostark
I guess not, but it makes only sense when issuing the ban?

@larsboldt
Copy link
Contributor Author

Would love to get this merged so I could stick to the official instead of a fork - what do you think?

Copy link
Owner

@ostark ostark left a comment

Choose a reason for hiding this comment

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

Wrap it in a if() block and I'll merge it

@timkelty
Copy link
Collaborator

@larsboldt Seems like this should be a config var, not an explicit env check.

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

Successfully merging this pull request may close these issues.

3 participants