Fix false positives in `fastly` and `fastly-compute` deployment types #1148

marsavar · 2023-05-16T14:58:21Z

What does this change?

Fixes an issue with fastly and fastly-compute deployment types that incorrectly deploy successfully when no API token is present in the keyring.

This is a problem because you might deploy your VCL or Compute@Edge application and think everything has gone smoothly, when in reality nothing has happened because the execute function wasn't triggered.

In its current implementation, foreach is called on the optional FastlyApiClientProvider.get(keyring) function, meaning that execute will only run if it is a Some value. No implementation is defined when it's None.

How to test

Deployed to CODE Riff-Raff.

Scenario 1: Fastly API key not present in the parameter store (should fail)

Scenario 2: Invalid or expired Fastly API key present in the parameter store (should fail)

Scenario 3: Valid Fastly API key present in the parameter store (should succeed)

… retrieve API credentials

nicl · 2023-05-22T09:44:57Z

magenta-lib/src/main/scala/magenta/tasks/FastlyComputeTasks.scala

-      )
+    FastlyApiClientProvider.get(keyRing) match {
+      case None =>
+        resources.reporter.fail("Failed to fetch Fastly API credentials")


Might be worth expanding this slightly to give a clearer sense of where credentials should exist.

nicl

Nice!

Small code style advice (non-blocking though): I'd probably favour pulling the case Some(client) => ... body out into a function to improve readability - as easier to see the actual patterns being matched. And have the function return a Try[..] rather than a large try/catch - again as it makes it easier to scan quickly the actual flow. But this is somewhat subjective.

marsavar · 2023-08-18T17:21:45Z

This was closed by mistake - reopened it and refactored it with Nic's suggestion

github-actions · 2023-09-18T06:01:56Z

This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days

marsavar · 2023-09-18T14:31:22Z

This PR is stale because it has been open 30 days with no activity. Unless a comment is added or the “stale” label removed, this will be closed in 3 days

🥹

… retrieve API credentials

davidfurey

I've made a few suggestions, but this already looks like a good change that would be great to get merged.

davidfurey · 2023-10-17T16:18:28Z

magenta-lib/src/main/scala/magenta/tasks/FastlyTasks.scala

+  ): Unit = {
+    FastlyApiClientProvider.get(keyRing) match {
+      case None =>
+        resources.reporter.fail("Failed to fetch Fastly API credentials")


Suggested change

resources.reporter.fail("Failed to fetch Fastly API credentials")

resources.reporter.fail("Failed to fetch Fastly API credentials from keyring")

davidfurey · 2023-10-17T16:41:22Z

magenta-lib/src/main/scala/magenta/tasks/FastlyTasks.scala

+    FastlyApiClientProvider.get(keyRing) match {
+      case None =>
+        resources.reporter.fail("Failed to fetch Fastly API credentials")
+      case Some(client) =>
+        _execute(client, resources, stopFlag) match {
+          case Success(nextVersionNumber) =>
+            resources.reporter
+              .info(s"Fastly version $nextVersionNumber is now active")
+          case Failure(err) =>
+            resources.reporter.fail(
+              s"$err. Your API key may be invalid or it may have expired"
+            )
+        }


I think this might be slightly easier to read:

Suggested change

FastlyApiClientProvider.get(keyRing) match {

case None =>

resources.reporter.fail("Failed to fetch Fastly API credentials")

case Some(client) =>

_execute(client, resources, stopFlag) match {

case Success(nextVersionNumber) =>

resources.reporter

.info(s"Fastly version $nextVersionNumber is now active")

case Failure(err) =>

resources.reporter.fail(

s"$err. Your API key may be invalid or it may have expired"

)

}

override def execute(

resources: DeploymentResources,

stopFlag: => Boolean

): Unit = {

FastlyApiClientProvider

.get(keyRing)

.map(_execute(_, resources, stopFlag)) match {

case Some(Success(nextVersionNumber)) =>

resources.reporter

.info(s"Fastly version $nextVersionNumber is now active")

case Some(Failure(err)) =>

resources.reporter.fail(

s"$err. Your API key may be invalid or it may have expired"

)

case None =>

resources.reporter.fail("Failed to fetch Fastly API credentials")

}

}

davidfurey · 2023-10-17T16:44:21Z

magenta-lib/src/main/scala/magenta/tasks/FastlyTasks.scala

@@ -30,43 +31,73 @@ case class UpdateFastlyConfig(s3Package: S3Path)(implicit
  implicit val ec: ExecutionContextExecutorService =
    ExecutionContext.fromExecutorService(Executors.newFixedThreadPool(10))

-  override def execute(
+  private def _execute(


Is there a more meaningful name for this method? Perhaps executeWithCredentials?

magenta-lib/src/main/scala/magenta/tasks/FastlyTasks.scala

davidfurey · 2023-10-17T16:48:52Z

magenta-lib/src/main/scala/magenta/tasks/FastlyComputeTasks.scala

-        .info(
-          s"Fastly Compute@Edge service ${client.serviceId} - version $nextVersionNumber is now active"
+    FastlyApiClientProvider.get(keyRing) match {
+      case None =>


Similar flattening of pattern match could be done as for FastlyTask.scala

Thanks - yes, I'm not a big fan of nested pattern matching either. Have refactored accordingly :)

…s-fastly-fail

Flatten pattern matching to improve readability

davidfurey

Looks good to me. Might be nice to have a 👍 from a current member of devX?

jacobwinch · 2023-10-19T08:05:07Z

magenta-lib/src/main/scala/magenta/tasks/FastlyComputeTasks.scala

+          )
+      case Some((_, Failure(err))) =>
+        resources.reporter.fail(
+          s"$err. Your API key may be invalid or it may have expired"


Minor:

IIUC, this case is going to catch a number of potential errors (network problems, Fastly API downtime/errors) as well as the problems that we've already mentioned in the error message.

If we find reliability problems with this deployment type in the future, it might be worth extracting the specific error (perhaps by adding some verbose logging or a different return type to activateVersion), but I don't think that needs to block this PR (which is already a big improvement!)

Thinking about this some more, I think there is another potential false positive here. IIUC calling activateVersion can result in a successful Future (i.e. we got a Response from the Fastly API), where the response/status code indicates that the request was unsuccessful from Fastly's perspective (i.e. we got a 500 back and did not actually activate the new version). In this case we will still match on: case Some((client, Success(nextVersionNumber))).

Happy to pair on fixing that via a separate PR if helpful.

jacobwinch

Great work; thanks for improving this and documenting the testing so clearly 👍

marsavar added 4 commits May 16, 2023 15:58

Do not mark Fastly tasks as successfully completed if Riff-Raff can't…

89b3079

… retrieve API credentials

Deal with invalid key

7953f7d

Deal with invalid key

8e426b8

Fail deploy when an exception is thrown by Fastly API calls

7d01fa7

marsavar changed the title ~~Do not mark Fastly tasks as successfully completed if Riff-Raff can't…~~ Fix false positives with fastly and fastly-compute deployment types May 16, 2023

marsavar changed the title ~~Fix false positives with fastly and fastly-compute deployment types~~ Fix false positives in fastly and fastly-compute deployment types May 16, 2023

marsavar marked this pull request as ready for review May 16, 2023 16:21

marsavar requested review from a team as code owners May 16, 2023 16:21

nicl reviewed May 22, 2023

View reviewed changes

nicl approved these changes May 22, 2023

View reviewed changes

akash1810 closed this May 25, 2023

twrichards self-requested a review August 18, 2023 15:32

Extract execute logic into private function

5d208fe

marsavar reopened this Aug 18, 2023

Use correct return type

e158b20

github-actions bot added the Stale label Sep 18, 2023

github-actions bot removed the Stale label Sep 19, 2023

marsavar added 6 commits September 22, 2023 11:44

Do not mark Fastly tasks as successfully completed if Riff-Raff can't…

59e3383

… retrieve API credentials

Deal with invalid key

dc2636a

Deal with invalid key

8a1c81e

Fail deploy when an exception is thrown by Fastly API calls

12e110e

Extract execute logic into private function

568047e

Use correct return type

fe5d76d

NovemberTang force-pushed the ms-fastly-fail branch from e158b20 to fe5d76d Compare September 22, 2023 10:44

davidfurey reviewed Oct 17, 2023

View reviewed changes

marsavar added 2 commits October 18, 2023 11:15

Merge branch 'ms-fastly-fail' of github.com:guardian/riff-raff into m…

5e7e49f

…s-fastly-fail

Wrap execution in Try block for easier parsing

708478d

Flatten pattern matching to improve readability

davidfurey approved these changes Oct 18, 2023

View reviewed changes

jacobwinch reviewed Oct 19, 2023

View reviewed changes

jacobwinch approved these changes Oct 19, 2023

View reviewed changes

marsavar enabled auto-merge (squash) October 19, 2023 11:24

Merge branch 'main' into ms-fastly-fail

3a2ad05

marsavar merged commit 669ccc5 into main Nov 9, 2023
1 check passed

marsavar deleted the ms-fastly-fail branch November 9, 2023 09:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix false positives in `fastly` and `fastly-compute` deployment types #1148

Fix false positives in `fastly` and `fastly-compute` deployment types #1148

marsavar commented May 16, 2023 •

edited

Loading

nicl May 22, 2023

nicl left a comment

marsavar commented Aug 18, 2023 •

edited

Loading

github-actions bot commented Sep 18, 2023

marsavar commented Sep 18, 2023

davidfurey left a comment

davidfurey Oct 17, 2023

davidfurey Oct 17, 2023

davidfurey Oct 17, 2023

davidfurey Oct 17, 2023

marsavar Oct 18, 2023

davidfurey left a comment

jacobwinch Oct 19, 2023 •

edited

Loading

jacobwinch Oct 19, 2023

jacobwinch left a comment

	resources.reporter.fail("Failed to fetch Fastly API credentials")
	resources.reporter.fail("Failed to fetch Fastly API credentials from keyring")

Fix false positives in fastly and fastly-compute deployment types #1148

Fix false positives in fastly and fastly-compute deployment types #1148

Conversation

marsavar commented May 16, 2023 • edited Loading

What does this change?

How to test

Scenario 1: Fastly API key not present in the parameter store (should fail)

Scenario 2: Invalid or expired Fastly API key present in the parameter store (should fail)

Scenario 3: Valid Fastly API key present in the parameter store (should succeed)

nicl May 22, 2023

Choose a reason for hiding this comment

nicl left a comment

Choose a reason for hiding this comment

marsavar commented Aug 18, 2023 • edited Loading

github-actions bot commented Sep 18, 2023

marsavar commented Sep 18, 2023

davidfurey left a comment

Choose a reason for hiding this comment

davidfurey Oct 17, 2023

Choose a reason for hiding this comment

davidfurey Oct 17, 2023

Choose a reason for hiding this comment

davidfurey Oct 17, 2023

Choose a reason for hiding this comment

davidfurey Oct 17, 2023

Choose a reason for hiding this comment

marsavar Oct 18, 2023

Choose a reason for hiding this comment

davidfurey left a comment

Choose a reason for hiding this comment

jacobwinch Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

jacobwinch Oct 19, 2023

Choose a reason for hiding this comment

jacobwinch left a comment

Choose a reason for hiding this comment

Fix false positives in `fastly` and `fastly-compute` deployment types #1148

Fix false positives in `fastly` and `fastly-compute` deployment types #1148

marsavar commented May 16, 2023 •

edited

Loading

marsavar commented Aug 18, 2023 •

edited

Loading

jacobwinch Oct 19, 2023 •

edited

Loading