Problem/Motivation

We have the Fastly module configured via config split to be enabled only in our production environment. We also have the Fastly API key set via an environment variable so that even if the module is enabled in a non-production environment it is not able to talk to Fastly.

When we import production database dumps to other environments, the Fastly module is enabled until a config-import has been completed. During this time, the module will attempt to purge keys from Fastly. Since the API key is not set, it results errors on the command-line like these:

[error]  Unable to purge following key(s) **** from Fastly. Purge Method: soft. 
 [error]  Client error: `POST https://api.fastly.com/service/********************/purge` resulted in a `401 Unauthorized` response:
@"msg":"Provided credentials are missing or invalid"

Steps to reproduce

There may be other ways to reproduce this scenario but this is ours:

  1. Add Fastly to a config split so it is only enabled in production, but disabled in other environments.
  2. Set Fastly API key only in the production environment via an environment variable or other method.
  3. Import a production database dump to your local environment.
  4. Run config import which will disable Fastly via Config Split, but not before Fastly attempts to purge some keys.

Proposed resolution

Add an additional check that an API key is present before making a request to the Fastly client/API.

CommentFileSizeAuthor
#7 Screenshot from 2020-10-16 10-51-46.png12.73 KBdanielveza
#2 3176963-2.patch991 bytesmarkdorison

Comments

markdorison created an issue. See original summary.

markdorison’s picture

markdorison
he/him
commented
Status: Active » Needs review
StatusFileSize
new 3176963-2.patch991 bytes
markdorison’s picture

markdorison
he/him
commented
Issue summary: View changes
adamzimmermann’s picture

adamzimmermann commented

I just tested this by downloading a fresh production database and importing configuration. I saw no Fastly API errors, while I previously would have seen many. So the patch is working great.

One question though. Previously malformed or incorrect API credentials would fail loudly with logging in the catch statement. This patch will bypass the code block that has the logging. The admin form calls the API credential validation function, but if settings.php overrides are used it wouldn't be caught. Should we add logging for invalid credentials? I can see both sides, as it would get noisy and they should already be validated, but sometimes noisy is good.

markdorison’s picture

markdorison
he/him
commented

@adamzimmermann Maybe this would benefit from a bigger re-structure. I feel pretty strongly that the module should do its best to make sure that it is not calling the Fastly library/API with invalid credentials, but I do see your point. I think the question it raises would be when is the time that the module should complain loudly about missing/invalid credentials?

adamzimmermann’s picture

adamzimmermann commented
Status: Needs review » Reviewed & tested by the community

when is the time that the module should complain loudly about missing/invalid credentials?

Exactly. That is really a question that is out of scope for this issue, but this patch does alter when it complains, hence why I even brought it up.

I think it's potentially fine as is, so marking it RTBC.

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented
Issue summary: View changes
StatusFileSize
new Screenshot from 2020-10-16 10-51-46.png12.73 KB

I was just having this exact issue and looking at potential solutions when I found this. Very cool.

I reckon what this module needs is something like the SMTP module.

SMTP configuration screen

danielveza’s picture

danielveza
he/him
Location Brisbane, AU
commented

Also - Applied patch and all is good now. So another RTBC for the patch! I would still recommend the SMTP approach, and happy to make a patch if the maintainer agrees. But this one is good too and is working!

wells’s picture

wells
he/him/his
Primary language English
Location Seattle, WA
commented

+1 for RTBC. Also applied patch with success on an application using config sync and this prevents a lot of chatter when performing sync operations in drush. Thanks!

kay_v’s picture

kay_v commented

looks like there's overlap with this issue, for what it's worth: #3099119: Add setting that can be overridden to disable API purge for local environments

luigimannoni’s picture

luigimannoni commented

We got an issue raised 3 years ago here #2908228 which overlaps this one, the patch in the issue seem to work without adding extra settings

luksak’s picture

luksak
he/him
commented
Status: Reviewed & tested by the community » Needs work
Related issues: +#3099119: Add setting that can be overridden to disable API purge for local environments

Adding the related issue. This one makes sense to get committed.

Regarding #2908228: Fastly purge gets initiated without api_key value, causes 404 errors: The two patches basically have the same approach by calling Api::validatePurgeCredentials() I prefer to have a critical log regarding this, which #2908228: Fastly purge gets initiated without api_key value, causes 404 errors does. On the other hand, I guess it makes more sense to have this on the API level as this issue does.

Hm... How can we move on from there? The first step would be to close one of the two issues and consolidate the best of the two worlds in one patch :)

Reverting to needs work.

luksak’s picture

luksak
he/him
commented
Status: Needs work » Closed (duplicate)

Closing in favor of #2908228: Fastly purge gets initiated without api_key value, causes 404 errors