From 6797bad1d1dcd688d273e0503bb08b76e569a49b Mon Sep 17 00:00:00 2001 From: Pawel Pasterz Date: Wed, 1 Jul 2020 11:27:07 +0200 Subject: [PATCH 01/15] Enhace permission denied execption logs --- .../src/main/kotlin/ftl/gc/GcToolResults.kt | 18 +++++++++++++++--- .../main/kotlin/ftl/http/ExecuteWithRetry.kt | 9 ++++++--- .../src/main/kotlin/ftl/util/FlankException.kt | 8 ++++++++ 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt b/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt index 1a1d4f7153..ca35f1e2c9 100644 --- a/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt +++ b/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt @@ -15,6 +15,8 @@ import ftl.config.FtlConstants.applicationName import ftl.config.FtlConstants.httpCredential import ftl.config.FtlConstants.httpTransport import ftl.http.executeWithRetry +import ftl.util.FlankCommonException +import ftl.util.ProjectPermissionDeniedError object GcToolResults { @@ -114,8 +116,18 @@ object GcToolResults { ).executeWithRetry() } - fun getDefaultBucket(projectId: String): String? { - val response = service.Projects().initializeSettings(projectId).executeWithRetry() - return response.defaultBucket + fun getDefaultBucket(projectId: String): String? = try { + service.Projects().initializeSettings(projectId).executeWithRetry().defaultBucket + } catch (ex: ProjectPermissionDeniedError) { + // flank need to rewrap exception with additional info about project + throw FlankCommonException(enhancedErrorMessage(projectId, ex.message)) } } + +private val enhancedErrorMessage = { projectId: String, message: String? -> + """Flank encountered a 403 error when running on project $projectId. Please verify this credential is authorized for the project. +Consider authentication with Service Account https://github.com/Flank/flank#authenticate-with-a-service-account +or with Google account https://github.com/Flank/flank#authenticate-with-a-google-account + +$message""".trimIndent() +} diff --git a/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt b/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt index 9bf865cfbe..7bd85873e6 100644 --- a/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt +++ b/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt @@ -3,6 +3,7 @@ package ftl.http import com.google.api.client.googleapis.services.json.AbstractGoogleJsonClientRequest import com.google.api.client.http.HttpResponseException import ftl.config.FtlConstants +import ftl.util.ProjectPermissionDeniedError import kotlinx.coroutines.delay import kotlinx.coroutines.runBlocking import java.io.IOException @@ -25,9 +26,11 @@ private inline fun withRetry(crossinline block: () -> T): T = runBlocking { FtlConstants.bugsnag?.notify(FlankGoogleApiError(err)) lastErr = err - // HttpStatusCodes from google api client does not have 429 code - if (err is HttpResponseException && err.statusCode == 429) { - return@repeat + if (err is HttpResponseException) { + when (err.statusCode) { + 429 -> return@repeat + 403 -> throw ProjectPermissionDeniedError("Caused by: $err") + } } delay(exp(it - 1.0).roundToInt().toLong()) } diff --git a/test_runner/src/main/kotlin/ftl/util/FlankException.kt b/test_runner/src/main/kotlin/ftl/util/FlankException.kt index 2b7386c251..da3f1ea26a 100644 --- a/test_runner/src/main/kotlin/ftl/util/FlankException.kt +++ b/test_runner/src/main/kotlin/ftl/util/FlankException.kt @@ -60,3 +60,11 @@ class FlankFatalError(message: String) : FlankException(message) * @param message [String] message to be printed to [System.err] */ class FlankCommonException(message: String) : FlankException(message) + +/** + * Custom exception to handle only permission denied errors. + * Should be caught and rewrap to FlankCommonException with project id info attached. + * + * @param message [String] message to be printed + */ +class ProjectPermissionDeniedError(message: String) : FlankException(message) From 38deff70d435990d2ab73c33f8705f6fa566cd46 Mon Sep 17 00:00:00 2001 From: Pawel Pasterz Date: Wed, 1 Jul 2020 14:21:48 +0200 Subject: [PATCH 02/15] Add 404 error support --- .../src/main/kotlin/ftl/gc/GcToolResults.kt | 19 +++++++++++++++---- .../main/kotlin/ftl/http/ExecuteWithRetry.kt | 7 +++++-- .../main/kotlin/ftl/util/FlankException.kt | 9 ++++++--- 3 files changed, 26 insertions(+), 9 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt b/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt index ca35f1e2c9..ff7bb31fa4 100644 --- a/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt +++ b/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt @@ -16,7 +16,9 @@ import ftl.config.FtlConstants.httpCredential import ftl.config.FtlConstants.httpTransport import ftl.http.executeWithRetry import ftl.util.FlankCommonException -import ftl.util.ProjectPermissionDeniedError +import ftl.util.FTLProjectError +import ftl.util.PermissionDenied +import ftl.util.ProjectNotFound object GcToolResults { @@ -118,16 +120,25 @@ object GcToolResults { fun getDefaultBucket(projectId: String): String? = try { service.Projects().initializeSettings(projectId).executeWithRetry().defaultBucket - } catch (ex: ProjectPermissionDeniedError) { + } catch (ex: FTLProjectError) { // flank need to rewrap exception with additional info about project - throw FlankCommonException(enhancedErrorMessage(projectId, ex.message)) + when (ex) { + is PermissionDenied -> throw FlankCommonException(permissionDeniedErrorMessage(projectId, ex.message)) + is ProjectNotFound -> throw FlankCommonException(projectNotFoundErrorMessage(projectId, ex.message)) + } } } -private val enhancedErrorMessage = { projectId: String, message: String? -> +private val permissionDeniedErrorMessage = { projectId: String, message: String? -> """Flank encountered a 403 error when running on project $projectId. Please verify this credential is authorized for the project. Consider authentication with Service Account https://github.com/Flank/flank#authenticate-with-a-service-account or with Google account https://github.com/Flank/flank#authenticate-with-a-google-account $message""".trimIndent() } + +private val projectNotFoundErrorMessage = { projectId: String, message: String? -> + """Flank was unable to find project $projectId. Please verify project id. + +$message""".trimIndent() +} diff --git a/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt b/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt index 7bd85873e6..cd2146aad5 100644 --- a/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt +++ b/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt @@ -3,7 +3,8 @@ package ftl.http import com.google.api.client.googleapis.services.json.AbstractGoogleJsonClientRequest import com.google.api.client.http.HttpResponseException import ftl.config.FtlConstants -import ftl.util.ProjectPermissionDeniedError +import ftl.util.PermissionDenied +import ftl.util.ProjectNotFound import kotlinx.coroutines.delay import kotlinx.coroutines.runBlocking import java.io.IOException @@ -27,9 +28,11 @@ private inline fun withRetry(crossinline block: () -> T): T = runBlocking { lastErr = err if (err is HttpResponseException) { + // we want to handle some FTL errors with special care when (err.statusCode) { 429 -> return@repeat - 403 -> throw ProjectPermissionDeniedError("Caused by: $err") + 403 -> throw PermissionDenied(err) + 404 -> throw ProjectNotFound(err) } } delay(exp(it - 1.0).roundToInt().toLong()) diff --git a/test_runner/src/main/kotlin/ftl/util/FlankException.kt b/test_runner/src/main/kotlin/ftl/util/FlankException.kt index da3f1ea26a..554ad7b83b 100644 --- a/test_runner/src/main/kotlin/ftl/util/FlankException.kt +++ b/test_runner/src/main/kotlin/ftl/util/FlankException.kt @@ -1,6 +1,7 @@ package ftl.util import ftl.json.SavedMatrix +import java.io.IOException /** * Base class for all custom flank's exceptions @@ -62,9 +63,11 @@ class FlankFatalError(message: String) : FlankException(message) class FlankCommonException(message: String) : FlankException(message) /** - * Custom exception to handle only permission denied errors. + * Base class for project related exceptions. * Should be caught and rewrap to FlankCommonException with project id info attached. * - * @param message [String] message to be printed + * @param exc [IOException] */ -class ProjectPermissionDeniedError(message: String) : FlankException(message) +sealed class FTLProjectError(exc: IOException) : FlankException("Caused by: $exc") +class PermissionDenied(exc: IOException) : FTLProjectError(exc) +class ProjectNotFound(exc: IOException) : FTLProjectError(exc) From ee298ed55236b7e39c9b986f9d2056f87a3d31b5 Mon Sep 17 00:00:00 2001 From: Adam Date: Wed, 1 Jul 2020 15:55:28 +0200 Subject: [PATCH 03/15] Update GcToolResultsTest.kt --- .../test/kotlin/ftl/gc/GcToolResultsTest.kt | 122 +++++++++++++++++- 1 file changed, 118 insertions(+), 4 deletions(-) diff --git a/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt b/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt index 2fcf1549b8..1b6b5f00fd 100644 --- a/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt +++ b/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt @@ -1,29 +1,41 @@ package ftl.gc +import com.google.api.client.googleapis.json.GoogleJsonResponseException +import com.google.api.client.http.HttpResponseException import com.google.api.services.testing.model.ToolResultsHistory import com.google.common.truth.Truth.assertThat import ftl.args.AndroidArgs +import ftl.config.FtlConstants import ftl.test.util.FlankTestRunner +import ftl.test.util.TestHelper.getThrowable +import ftl.util.FlankCommonException +import ftl.util.PermissionDenied +import ftl.util.ProjectNotFound import io.mockk.every import io.mockk.mockk +import io.mockk.mockkObject import io.mockk.unmockkAll import org.junit.After +import org.junit.Assert.assertEquals import org.junit.Test import org.junit.runner.RunWith +import java.io.IOException @RunWith(FlankTestRunner::class) class GcToolResultsTest { + private val projectId = "123" + @After fun tearDown() = unmockkAll() @Test fun `createToolResultsHistory null succeeds`() { val args = mockk() - every { args.project } returns "123" + every { args.project } returns projectId every { args.resultsHistoryName } returns null - val expected = ToolResultsHistory().setProjectId("123") + val expected = ToolResultsHistory().setProjectId(projectId) assertThat(GcToolResults.createToolResultsHistory(args)).isEqualTo(expected) } @@ -31,13 +43,115 @@ class GcToolResultsTest { @Test fun `createToolResultsHistory succeeds`() { val args = mockk() - every { args.project } returns "123" + every { args.project } returns projectId every { args.resultsHistoryName } returns "custom history" val expected = ToolResultsHistory() - .setProjectId("123") + .setProjectId(projectId) .setHistoryId("mockId") assertThat(GcToolResults.createToolResultsHistory(args)).isEqualTo(expected) } + + @Test + fun `getDefaultBucket on 403 error should throw exception with specific message`() { + val expected = """ + Flank encountered a 403 error when running on project $projectId. Please verify this credential is authorized for the project. + Consider authentication with Service Account https://github.com/Flank/flank#authenticate-with-a-service-account + or with Google account https://github.com/Flank/flank#authenticate-with-a-google-account + + Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 403 Forbidden + { + "code" : 403, + "errors" : [ { + "domain" : "global", + "message" : "The caller does not have permission", + "reason" : "forbidden" + } ], + "message" : "The caller does not have permission", + "status" : "PERMISSION_DENIED" + } + """.trimIndent() + mockkObject(GcToolResults) { + every { GcToolResults.service.applicationName } returns projectId + + val exceptionBuilder = mockk() + every { exceptionBuilder.message } returns """ + 403 Forbidden + { + "code" : 403, + "errors" : [ { + "domain" : "global", + "message" : "The caller does not have permission", + "reason" : "forbidden" + } ], + "message" : "The caller does not have permission", + "status" : "PERMISSION_DENIED" + } + """.trimIndent() + val mockJSonException = GoogleJsonResponseException(exceptionBuilder, null) + every { GcToolResults.service.Projects().initializeSettings(projectId) } throws PermissionDenied(mockJSonException) + val exception = getThrowable { GcToolResults.getDefaultBucket(projectId) } + assertEquals(expected, exception.message) + } + } + + @Test(expected = FlankCommonException::class) + fun `getDefaultBucket on PermissionDenied error should throw FlankCommonException`() { + mockkObject(GcToolResults) { + every { GcToolResults.service.applicationName } returns projectId + every { GcToolResults.service.Projects().initializeSettings(projectId) } throws PermissionDenied(IOException()) + GcToolResults.getDefaultBucket(projectId) + } + } + + @Test(expected = FlankCommonException::class) + fun `getDefaultBucket on ProjectNotFound error should throw FlankCommonException`() { + mockkObject(GcToolResults) { + every { GcToolResults.service.applicationName } returns projectId + every { GcToolResults.service.Projects().initializeSettings(projectId) } throws ProjectNotFound(IOException()) + GcToolResults.getDefaultBucket(projectId) + } + } + + @Test + fun `getDefaultBucket on 404 error should throw exception with specific message`() { + val expected = """ + Flank was unable to find project $projectId. Please verify project id. + + Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 404 Not Found + { + "code" : 404, + "errors" : [ { + "domain" : "global", + "message" : "Project not found: $projectId", + "reason" : "notFound" + } ], + "message" : "Project not found: $projectId", + "status" : "NOT_FOUND" + } + """.trimIndent() + mockkObject(GcToolResults) { + every { GcToolResults.service.applicationName } returns FtlConstants.applicationName + + val exceptionBuilder = mockk() + every { exceptionBuilder.message } returns """ + 404 Not Found + { + "code" : 404, + "errors" : [ { + "domain" : "global", + "message" : "Project not found: $projectId", + "reason" : "notFound" + } ], + "message" : "Project not found: $projectId", + "status" : "NOT_FOUND" + } + """.trimIndent() + val mockJSonException = GoogleJsonResponseException(exceptionBuilder, null) + every { GcToolResults.service.Projects().initializeSettings(projectId) } throws ProjectNotFound(mockJSonException) + val exception = getThrowable { GcToolResults.getDefaultBucket(projectId) } + assertEquals(expected, exception.message) + } + } } From 3a237d777b82bd3d17a9cdf4ac6ca966772e9e23 Mon Sep 17 00:00:00 2001 From: Adam Date: Wed, 1 Jul 2020 15:58:22 +0200 Subject: [PATCH 04/15] Update release_notes.md --- release_notes.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/release_notes.md b/release_notes.md index ab8fb5dfe5..9a581342e4 100644 --- a/release_notes.md +++ b/release_notes.md @@ -6,7 +6,8 @@ - [#865](https://github.com/Flank/flank/pull/865) Flank needs to respect the timeout value as that's a cap for billing purposes. ([adamfilipow92](https://github.com/adamfilipow92), [pawelpasterz](https://github.com/pawelpasterz)) - [#866](https://github.com/Flank/flank/pull/866) Fix printing all matrix links. ([piotradamczyk5](https://github.com/piotradamczyk5)) - [#862](https://github.com/Flank/flank/pull/862) Added printing outcome details. ([piotradamczyk5](https://github.com/piotradamczyk5), [jan-gogo](https://github.com/jan-gogo)) -- [#876](https://github.com/Flank/flank/pull/876) Added --directories-to-pull validation and avoid making request with empty toolStepResult. ([piotradamczyk5](https://github.com/piotradamczyk5)) +- [#876](https://github.com/Flank/flank/pull/876) Added --directories-to-pull validation and avoid making request with empty toolStepResult. ([piotradamczyk5](https://github.com/piotradamczyk5) +- [#875](https://github.com/Flank/flank/pull/875) Enhance permission denied exception logs. ([adamfilipow92](https://github.com/adamfilipow92), [pawelpasterz](https://github.com/pawelpasterz)) - - @@ -21,7 +22,7 @@ - [#831](https://github.com/Flank/flank/pull/831) Refactor config entities and arguments. ([jan-gogo](https://github.com/jan-gogo)) - [#817](https://github.com/Flank/flank/pull/817) Add AndroidTestContext as base data for dump shards & test execution. ([jan-gogo](https://github.com/jan-gogo)) -- [#801](https://github.com/Flank/flank/pull/801) Omit missing app apk if additional-app-test-apks specified. ([jan-gogo](https://github.com/jan-gogo)) +- [#801](https://github.com/Flank/flank/pull/801) Omit missing app apk if additional-app-test-apks specified. ([jan-gogo](https://github.com/jan-gogo)) - [#784](https://github.com/Flank/flank/pull/784) Add output-style option. ([jan-gogo](https://github.com/jan-gogo)) - [#779](https://github.com/Flank/flank/pull/779) Print retries & display additional info. ([jan-gogo](https://github.com/jan-gogo)) - [#793](https://github.com/Flank/flank/issues/793) Better error message on file not found. ([adamfilipow92](https://github.com/adamfilipow92)) From 6db347cd4ab9ce59aa5eb61583e9e7eeeaf27bc3 Mon Sep 17 00:00:00 2001 From: Adam Date: Wed, 1 Jul 2020 18:29:52 +0200 Subject: [PATCH 05/15] Refractor --- .../src/main/kotlin/ftl/gc/GcToolResults.kt | 8 +-- .../main/kotlin/ftl/http/ExecuteWithRetry.kt | 62 ++++++++++++------- 2 files changed, 43 insertions(+), 27 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt b/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt index ff7bb31fa4..a1c630e2f6 100644 --- a/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt +++ b/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt @@ -120,11 +120,11 @@ object GcToolResults { fun getDefaultBucket(projectId: String): String? = try { service.Projects().initializeSettings(projectId).executeWithRetry().defaultBucket - } catch (ex: FTLProjectError) { + } catch (ftlProjectError: FTLProjectError) { // flank need to rewrap exception with additional info about project - when (ex) { - is PermissionDenied -> throw FlankCommonException(permissionDeniedErrorMessage(projectId, ex.message)) - is ProjectNotFound -> throw FlankCommonException(projectNotFoundErrorMessage(projectId, ex.message)) + when (ftlProjectError) { + is PermissionDenied -> throw FlankCommonException(permissionDeniedErrorMessage(projectId, ftlProjectError.message)) + is ProjectNotFound -> throw FlankCommonException(projectNotFoundErrorMessage(projectId, ftlProjectError.message)) } } } diff --git a/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt b/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt index cd2146aad5..c547fb9329 100644 --- a/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt +++ b/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt @@ -2,6 +2,7 @@ package ftl.http import com.google.api.client.googleapis.services.json.AbstractGoogleJsonClientRequest import com.google.api.client.http.HttpResponseException +import com.google.api.client.http.HttpStatusCodes import ftl.config.FtlConstants import ftl.util.PermissionDenied import ftl.util.ProjectNotFound @@ -9,37 +10,52 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.runBlocking import java.io.IOException import kotlin.math.exp -import kotlin.math.roundToInt +import kotlin.math.roundToLong // Only use on calls that don't change state. // Fetching status is safe to retry on timeout. Creating a matrix is not. fun AbstractGoogleJsonClientRequest.executeWithRetry(): T = withRetry { this.execute() } private inline fun withRetry(crossinline block: () -> T): T = runBlocking { - var lastErr: IOException? = null - - repeat(4) { - try { - return@runBlocking block() - } catch (err: IOException) { - // We want to send every occurrence of Google API error for statistic purposes - // https://github.com/Flank/flank/issues/701 - FtlConstants.bugsnag?.notify(FlankGoogleApiError(err)) - - lastErr = err - if (err is HttpResponseException) { - // we want to handle some FTL errors with special care - when (err.statusCode) { - 429 -> return@repeat - 403 -> throw PermissionDenied(err) - 404 -> throw ProjectNotFound(err) - } - } - delay(exp(it - 1.0).roundToInt().toLong()) - } + var lastError: IOException? = null + repeat(maxTries) { repeatCounter -> + val executionResult = tryExecuteBlock(block) + if (executionResult.isSuccess()) return@runBlocking executionResult.success() + + val error = executionResult.error() + // We want to send every occurrence of Google API error for statistic purposes + // https://github.com/Flank/flank/issues/701 + FtlConstants.bugsnag?.notify(FlankGoogleApiError(error)) + lastError = error + if (handleFtlError(error)) return@repeat + delay(calculateDelayTime(repeatCounter)) } + throw IOException("Request failed", lastError) +} +private const val maxTries = 4 + +private inline fun tryExecuteBlock(block: () -> T): ExecutionBlockResult = try { + ExecutionBlockResult(block(), null) +} catch (ioError: IOException) { + ExecutionBlockResult(null, ioError) +} - throw IOException("Request failed", lastErr) +private data class ExecutionBlockResult(private val success: T?, private val error: IOException?) { + fun isSuccess() = success != null + fun success() = success!! + fun error() = error!! } +private fun handleFtlError(error: IOException) = (error as? HttpResponseException)?.let { + // we want to handle some FTL errors with special care + when (it.statusCode) { + HttpStatusCodes.STATUS_CODE_FORBIDDEN -> throw PermissionDenied(it) + HttpStatusCodes.STATUS_CODE_NOT_FOUND -> throw ProjectNotFound(it) + 429 -> true + else -> false + } +} ?: false + +private fun calculateDelayTime(repeatCounter: Int) = exp(repeatCounter - 1.0).roundToLong() + private class FlankGoogleApiError(exception: Throwable) : Error(exception) From 9f43e8fc9dadfc3287667c9d965172ad39518efa Mon Sep 17 00:00:00 2001 From: Adam Date: Wed, 1 Jul 2020 18:32:45 +0200 Subject: [PATCH 06/15] Detekt --- test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt b/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt index c547fb9329..6e7426d207 100644 --- a/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt +++ b/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt @@ -18,7 +18,7 @@ fun AbstractGoogleJsonClientRequest.executeWithRetry(): T = withRetry { t private inline fun withRetry(crossinline block: () -> T): T = runBlocking { var lastError: IOException? = null - repeat(maxTries) { repeatCounter -> + repeat(MAX_TRIES) { repeatCounter -> val executionResult = tryExecuteBlock(block) if (executionResult.isSuccess()) return@runBlocking executionResult.success() @@ -32,7 +32,7 @@ private inline fun withRetry(crossinline block: () -> T): T = runBlocking { } throw IOException("Request failed", lastError) } -private const val maxTries = 4 +private const val MAX_TRIES = 4 private inline fun tryExecuteBlock(block: () -> T): ExecutionBlockResult = try { ExecutionBlockResult(block(), null) From da0d51232014c4b35d9f0432aac2f329507c0bf2 Mon Sep 17 00:00:00 2001 From: Pawel Pasterz Date: Wed, 1 Jul 2020 20:53:13 +0200 Subject: [PATCH 07/15] Change wording --- test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt b/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt index a1c630e2f6..9d5f335ef5 100644 --- a/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt +++ b/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt @@ -121,7 +121,7 @@ object GcToolResults { fun getDefaultBucket(projectId: String): String? = try { service.Projects().initializeSettings(projectId).executeWithRetry().defaultBucket } catch (ftlProjectError: FTLProjectError) { - // flank need to rewrap exception with additional info about project + // flank needs to rewrap the exception with additional info about project when (ftlProjectError) { is PermissionDenied -> throw FlankCommonException(permissionDeniedErrorMessage(projectId, ftlProjectError.message)) is ProjectNotFound -> throw FlankCommonException(projectNotFoundErrorMessage(projectId, ftlProjectError.message)) @@ -131,14 +131,14 @@ object GcToolResults { private val permissionDeniedErrorMessage = { projectId: String, message: String? -> """Flank encountered a 403 error when running on project $projectId. Please verify this credential is authorized for the project. -Consider authentication with Service Account https://github.com/Flank/flank#authenticate-with-a-service-account -or with Google account https://github.com/Flank/flank#authenticate-with-a-google-account +Consider authentication with a Service Account https://github.com/Flank/flank#authenticate-with-a-service-account +or with a Google account https://github.com/Flank/flank#authenticate-with-a-google-account $message""".trimIndent() } private val projectNotFoundErrorMessage = { projectId: String, message: String? -> - """Flank was unable to find project $projectId. Please verify project id. + """Flank was unable to find project $projectId. Please verify the project id. $message""".trimIndent() } From 0b9d4d01fd33140f3c6f4a1b56c43a39a9ec3a4a Mon Sep 17 00:00:00 2001 From: Adam Date: Thu, 2 Jul 2020 13:10:26 +0200 Subject: [PATCH 08/15] Revert changes on ExecuteWithRetry, update tests --- .../main/kotlin/ftl/http/ExecuteWithRetry.kt | 58 +++++++------------ .../test/kotlin/ftl/gc/GcToolResultsTest.kt | 6 +- 2 files changed, 23 insertions(+), 41 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt b/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt index 6e7426d207..64f699b609 100644 --- a/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt +++ b/test_runner/src/main/kotlin/ftl/http/ExecuteWithRetry.kt @@ -2,7 +2,6 @@ package ftl.http import com.google.api.client.googleapis.services.json.AbstractGoogleJsonClientRequest import com.google.api.client.http.HttpResponseException -import com.google.api.client.http.HttpStatusCodes import ftl.config.FtlConstants import ftl.util.PermissionDenied import ftl.util.ProjectNotFound @@ -10,7 +9,7 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.runBlocking import java.io.IOException import kotlin.math.exp -import kotlin.math.roundToLong +import kotlin.math.roundToInt // Only use on calls that don't change state. // Fetching status is safe to retry on timeout. Creating a matrix is not. @@ -18,44 +17,27 @@ fun AbstractGoogleJsonClientRequest.executeWithRetry(): T = withRetry { t private inline fun withRetry(crossinline block: () -> T): T = runBlocking { var lastError: IOException? = null - repeat(MAX_TRIES) { repeatCounter -> - val executionResult = tryExecuteBlock(block) - if (executionResult.isSuccess()) return@runBlocking executionResult.success() - - val error = executionResult.error() - // We want to send every occurrence of Google API error for statistic purposes - // https://github.com/Flank/flank/issues/701 - FtlConstants.bugsnag?.notify(FlankGoogleApiError(error)) - lastError = error - if (handleFtlError(error)) return@repeat - delay(calculateDelayTime(repeatCounter)) + repeat(4) { + try { + return@runBlocking block() + } catch (err: IOException) { + // We want to send every occurrence of Google API error for statistic purposes + // https://github.com/Flank/flank/issues/701 + FtlConstants.bugsnag?.notify(FlankGoogleApiError(err)) + + lastError = err + if (err is HttpResponseException) { + // we want to handle some FTL errors with special care + when (err.statusCode) { + 429 -> return@repeat + 403 -> throw PermissionDenied(err) + 404 -> throw ProjectNotFound(err) + } + } + delay(exp(it - 1.0).roundToInt().toLong()) + } } throw IOException("Request failed", lastError) } -private const val MAX_TRIES = 4 - -private inline fun tryExecuteBlock(block: () -> T): ExecutionBlockResult = try { - ExecutionBlockResult(block(), null) -} catch (ioError: IOException) { - ExecutionBlockResult(null, ioError) -} - -private data class ExecutionBlockResult(private val success: T?, private val error: IOException?) { - fun isSuccess() = success != null - fun success() = success!! - fun error() = error!! -} - -private fun handleFtlError(error: IOException) = (error as? HttpResponseException)?.let { - // we want to handle some FTL errors with special care - when (it.statusCode) { - HttpStatusCodes.STATUS_CODE_FORBIDDEN -> throw PermissionDenied(it) - HttpStatusCodes.STATUS_CODE_NOT_FOUND -> throw ProjectNotFound(it) - 429 -> true - else -> false - } -} ?: false - -private fun calculateDelayTime(repeatCounter: Int) = exp(repeatCounter - 1.0).roundToLong() private class FlankGoogleApiError(exception: Throwable) : Error(exception) diff --git a/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt b/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt index 1b6b5f00fd..083d408a46 100644 --- a/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt +++ b/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt @@ -57,8 +57,8 @@ class GcToolResultsTest { fun `getDefaultBucket on 403 error should throw exception with specific message`() { val expected = """ Flank encountered a 403 error when running on project $projectId. Please verify this credential is authorized for the project. - Consider authentication with Service Account https://github.com/Flank/flank#authenticate-with-a-service-account - or with Google account https://github.com/Flank/flank#authenticate-with-a-google-account + Consider authentication with a Service Account https://github.com/Flank/flank#authenticate-with-a-service-account + or with a Google account https://github.com/Flank/flank#authenticate-with-a-google-account Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 403 Forbidden { @@ -117,7 +117,7 @@ class GcToolResultsTest { @Test fun `getDefaultBucket on 404 error should throw exception with specific message`() { val expected = """ - Flank was unable to find project $projectId. Please verify project id. + Flank was unable to find project $projectId. Please verify the project id. Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 404 Not Found { From 1acd3031b5369cfd0f776d6aa3eac9e7fe6d57ad Mon Sep 17 00:00:00 2001 From: Adam Date: Thu, 2 Jul 2020 14:04:28 +0200 Subject: [PATCH 09/15] Create permissions_denied_behavior.md --- docs/permissions_denied_behavior.md | 53 +++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 docs/permissions_denied_behavior.md diff --git a/docs/permissions_denied_behavior.md b/docs/permissions_denied_behavior.md new file mode 100644 index 0000000000..1b24d7d892 --- /dev/null +++ b/docs/permissions_denied_behavior.md @@ -0,0 +1,53 @@ +# Flank permissions denied behavior + +Reported on: [Clean flank not authorized error messages #874](https://github.com/Flank/flank/issues/874) + +Changed on: [Enhance permission denied exception logs #875](https://github.com/Flank/flank/pull/875) + +## 1. User don't have permission to project (403) + +When user don't have permission to project Flank should returns message like: + +```json + +Flank encountered a 403 error when running on project $project_name. Please verify this credential is authorized for the project. +Consider authentication a with Service Account https://github.com/Flank/flank#authenticate-with-a-service-account +or with a Google account https://github.com/Flank/flank#authenticate-with-a-google-account + +Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 403 Forbidden +{ + "code" : 403, + "errors" : [ { + "domain" : "global", + "message" : "The caller does not have permission", + "reason" : "forbidden" + } ], + "message" : "The caller does not have permission", + "status" : "PERMISSION_DENIED" +} + +``` + +## 2. Project not found (404) + +When project not found on firebase Flank should return message like: + +```json + +Flank was unable to find project $project_name. Please verify the project id. + +Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 404 Not Found +{ + "code" : 404, + "errors" : [ { + "domain" : "global", + "message" : "Project not found: $project_name", + "reason" : "notFound" + } ], + "message" : "Project not found: $project_name", + "status" : "NOT_FOUND" +} + +``` + +## 3. On this two cases Flank throws FlankCommonException and exit with code: 1 From 4ac26780c9e214c87339537c7107d9b95f6a82a8 Mon Sep 17 00:00:00 2001 From: Adam Date: Thu, 2 Jul 2020 14:23:37 +0200 Subject: [PATCH 10/15] Update permissions_denied_behavior.md --- docs/permissions_denied_behavior.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/permissions_denied_behavior.md b/docs/permissions_denied_behavior.md index 1b24d7d892..9e917bec13 100644 --- a/docs/permissions_denied_behavior.md +++ b/docs/permissions_denied_behavior.md @@ -28,6 +28,8 @@ Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 40 ``` +You can reproduce it by set PROJECT_ID to project where your current firebase account doesn't have permission. + ## 2. Project not found (404) When project not found on firebase Flank should return message like: @@ -50,4 +52,6 @@ Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 40 ``` +You can reproduce it by set PROJECT_ID to not existing project. + ## 3. On this two cases Flank throws FlankCommonException and exit with code: 1 From 88132fd9b4bf08707bfc62d36415d93817b48632 Mon Sep 17 00:00:00 2001 From: adamfilipow92 <64852261+adamfilipow92@users.noreply.github.com> Date: Thu, 2 Jul 2020 17:12:59 +0200 Subject: [PATCH 11/15] Update docs/permissions_denied_behavior.md Co-authored-by: bootstraponline --- docs/permissions_denied_behavior.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/permissions_denied_behavior.md b/docs/permissions_denied_behavior.md index 9e917bec13..7522b1b565 100644 --- a/docs/permissions_denied_behavior.md +++ b/docs/permissions_denied_behavior.md @@ -28,7 +28,7 @@ Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 40 ``` -You can reproduce it by set PROJECT_ID to project where your current firebase account doesn't have permission. +You can reproduce the error by setting PROJECT_ID to a project that the firebase account doesn't have permission to access. ## 2. Project not found (404) From 33d75b74d40aaecf395546e3f2366b78743b5da6 Mon Sep 17 00:00:00 2001 From: adamfilipow92 <64852261+adamfilipow92@users.noreply.github.com> Date: Thu, 2 Jul 2020 17:13:08 +0200 Subject: [PATCH 12/15] Update docs/permissions_denied_behavior.md Co-authored-by: bootstraponline --- docs/permissions_denied_behavior.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/permissions_denied_behavior.md b/docs/permissions_denied_behavior.md index 7522b1b565..2c9f69ab48 100644 --- a/docs/permissions_denied_behavior.md +++ b/docs/permissions_denied_behavior.md @@ -52,6 +52,6 @@ Caused by: com.google.api.client.googleapis.json.GoogleJsonResponseException: 40 ``` -You can reproduce it by set PROJECT_ID to not existing project. +You can reproduce the error by setting PROJECT_ID to a project that doesn't exist. ## 3. On this two cases Flank throws FlankCommonException and exit with code: 1 From dd5a7dd29a648bfff9c09e06e14604eb84986226 Mon Sep 17 00:00:00 2001 From: Pawel Pasterz Date: Fri, 3 Jul 2020 20:46:28 +0200 Subject: [PATCH 13/15] Review changes --- test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt | 2 +- test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt b/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt index 9d5f335ef5..7e62f0f8cf 100644 --- a/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt +++ b/test_runner/src/main/kotlin/ftl/gc/GcToolResults.kt @@ -130,7 +130,7 @@ object GcToolResults { } private val permissionDeniedErrorMessage = { projectId: String, message: String? -> - """Flank encountered a 403 error when running on project $projectId. Please verify this credential is authorized for the project. + """Flank encountered a 403 error when running on project $projectId. Please verify this credential is authorized for the project and has the required permissions. Consider authentication with a Service Account https://github.com/Flank/flank#authenticate-with-a-service-account or with a Google account https://github.com/Flank/flank#authenticate-with-a-google-account diff --git a/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt b/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt index 083d408a46..b1726e6f37 100644 --- a/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt +++ b/test_runner/src/test/kotlin/ftl/gc/GcToolResultsTest.kt @@ -56,7 +56,7 @@ class GcToolResultsTest { @Test fun `getDefaultBucket on 403 error should throw exception with specific message`() { val expected = """ - Flank encountered a 403 error when running on project $projectId. Please verify this credential is authorized for the project. + Flank encountered a 403 error when running on project $projectId. Please verify this credential is authorized for the project and has the required permissions. Consider authentication with a Service Account https://github.com/Flank/flank#authenticate-with-a-service-account or with a Google account https://github.com/Flank/flank#authenticate-with-a-google-account From c2546acb6b8000d627facc1c97c188be37c807a5 Mon Sep 17 00:00:00 2001 From: Pawel Pasterz Date: Fri, 3 Jul 2020 20:50:27 +0200 Subject: [PATCH 14/15] Update release notes --- release_notes.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/release_notes.md b/release_notes.md index 9a581342e4..540b3b005d 100644 --- a/release_notes.md +++ b/release_notes.md @@ -6,9 +6,13 @@ - [#865](https://github.com/Flank/flank/pull/865) Flank needs to respect the timeout value as that's a cap for billing purposes. ([adamfilipow92](https://github.com/adamfilipow92), [pawelpasterz](https://github.com/pawelpasterz)) - [#866](https://github.com/Flank/flank/pull/866) Fix printing all matrix links. ([piotradamczyk5](https://github.com/piotradamczyk5)) - [#862](https://github.com/Flank/flank/pull/862) Added printing outcome details. ([piotradamczyk5](https://github.com/piotradamczyk5), [jan-gogo](https://github.com/jan-gogo)) +<<<<<<< HEAD - [#876](https://github.com/Flank/flank/pull/876) Added --directories-to-pull validation and avoid making request with empty toolStepResult. ([piotradamczyk5](https://github.com/piotradamczyk5) - [#875](https://github.com/Flank/flank/pull/875) Enhance permission denied exception logs. ([adamfilipow92](https://github.com/adamfilipow92), [pawelpasterz](https://github.com/pawelpasterz)) - +======= +- [#875](https://github.com/Flank/flank/pull/875) Enhance permission denied exception logs. ([adamfilipow92](https://github.com/adamfilipow92), [pawelpasterz](https://github.com/pawelpasterz)) +>>>>>>> Update release notes - ## v20.06.2 From b8c085672040d39fc9fc8026f286b2b03f19c25d Mon Sep 17 00:00:00 2001 From: Pawel Pasterz Date: Mon, 6 Jul 2020 21:13:29 +0200 Subject: [PATCH 15/15] Update relese notes --- release_notes.md | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/release_notes.md b/release_notes.md index 540b3b005d..4363c3c982 100644 --- a/release_notes.md +++ b/release_notes.md @@ -6,14 +6,9 @@ - [#865](https://github.com/Flank/flank/pull/865) Flank needs to respect the timeout value as that's a cap for billing purposes. ([adamfilipow92](https://github.com/adamfilipow92), [pawelpasterz](https://github.com/pawelpasterz)) - [#866](https://github.com/Flank/flank/pull/866) Fix printing all matrix links. ([piotradamczyk5](https://github.com/piotradamczyk5)) - [#862](https://github.com/Flank/flank/pull/862) Added printing outcome details. ([piotradamczyk5](https://github.com/piotradamczyk5), [jan-gogo](https://github.com/jan-gogo)) -<<<<<<< HEAD -- [#876](https://github.com/Flank/flank/pull/876) Added --directories-to-pull validation and avoid making request with empty toolStepResult. ([piotradamczyk5](https://github.com/piotradamczyk5) +- [#876](https://github.com/Flank/flank/pull/876) Added --directories-to-pull validation and avoid making request with empty toolStepResult. ([piotradamczyk5](https://github.com/piotradamczyk5)) - [#875](https://github.com/Flank/flank/pull/875) Enhance permission denied exception logs. ([adamfilipow92](https://github.com/adamfilipow92), [pawelpasterz](https://github.com/pawelpasterz)) - -======= -- [#875](https://github.com/Flank/flank/pull/875) Enhance permission denied exception logs. ([adamfilipow92](https://github.com/adamfilipow92), [pawelpasterz](https://github.com/pawelpasterz)) ->>>>>>> Update release notes -- ## v20.06.2 - [#853](https://github.com/Flank/flank/pull/853) Store @Ignore tests in the JUnit XML without sending ignored tests to FTL. ([piotradamczyk5](https://github.com/piotradamczyk5), [adamfilipow92](https://github.com/adamfilipow92))