fix: handle exceptions when adding a secret version (#14962)

airbytehq · Jan 9, 2025 · 0c2bb4f · 0c2bb4f
1 parent f0d7bcb
commit 0c2bb4f
Show file tree

Hide file tree

Showing 2 changed files with 96 additions and 5 deletions.
diff --git a/...nfig/config-secrets/src/main/kotlin/secrets/persistence/GoogleSecretManagerPersistence.kt b/...nfig/config-secrets/src/main/kotlin/secrets/persistence/GoogleSecretManagerPersistence.kt
@@ -106,15 +106,38 @@ class GoogleSecretManagerPersistence(
         }
 
         metricClient.count(OssMetricsRegistry.CREATE_SECRET_DEFAULT_STORE, 1, *expTag.toTypedArray())
-        client.createSecret(ProjectName.of(gcpProjectId), coordinate.fullCoordinate, secretBuilder.build())
-      }
 
-      val name = SecretName.of(gcpProjectId, coordinate.fullCoordinate)
-      val secretPayload = SecretPayload.newBuilder().setData(ByteString.copyFromUtf8(payload)).build()
-      client.addSecretVersion(name, secretPayload)
+        logger.info { "GoogleSecretManagerPersistence createSecret coordinate=$coordinate expiry=$expiry" }
+
+        val secret = client.createSecret(ProjectName.of(gcpProjectId), coordinate.fullCoordinate, secretBuilder.build())
+        try {
+          addSecretVersion(client, coordinate, payload)
+        } catch (e: Exception) {
+          client.deleteSecret(secret.name)
+          throw e
+        }
+      } else {
+        addSecretVersion(client, coordinate, payload)
+        logger.warn {
+          "Added a new version to a secret with existing versions name=${SecretName.of(
+            gcpProjectId,
+            coordinate.fullCoordinate,
+          )} coordinate=$coordinate"
+        }
+      }
     }
   }
 
+  fun addSecretVersion(
+    client: SecretManagerServiceClient,
+    coordinate: SecretCoordinate,
+    payload: String,
+  ) {
+    val name = SecretName.of(gcpProjectId, coordinate.fullCoordinate)
+    val secretPayload = SecretPayload.newBuilder().setData(ByteString.copyFromUtf8(payload)).build()
+    client.addSecretVersion(name, secretPayload)
+  }
+
   override fun delete(coordinate: SecretCoordinate) {
     googleSecretManagerServiceClient.createClient().use { client ->
       val secretName = SecretName.of(gcpProjectId, coordinate.fullCoordinate)

diff --git a/.../config-secrets/src/test/kotlin/secrets/persistence/GoogleSecretManagerPersistenceTest.kt b/.../config-secrets/src/test/kotlin/secrets/persistence/GoogleSecretManagerPersistenceTest.kt
@@ -20,12 +20,14 @@ import io.airbyte.config.secrets.persistence.GoogleSecretManagerPersistence.Comp
 import io.airbyte.metrics.lib.MetricClient
 import io.grpc.Status
 import io.mockk.Runs
+import io.mockk.coEvery
 import io.mockk.every
 import io.mockk.just
 import io.mockk.mockk
 import io.mockk.verify
 import org.junit.jupiter.api.Assertions
 import org.junit.jupiter.api.Test
+import org.junit.jupiter.api.assertThrows
 import java.time.Duration
 import java.time.Instant
 
@@ -178,6 +180,72 @@ class GoogleSecretManagerPersistenceTest {
     verify { mockGoogleClient.addSecretVersion(any<SecretName>(), any<SecretPayload>()) }
   }
 
+  @Test
+  fun `test exception when adding a secret version deletes new secret`() {
+    val secret = "secret value"
+    val projectId = "test"
+    val coordinate = SecretCoordinate.fromFullCoordinate("secret_coordinate_v1")
+    val mockClient: GoogleSecretManagerServiceClient = mockk()
+    val mockGoogleClient: SecretManagerServiceClient = mockk()
+    val mockResponse: AccessSecretVersionResponse = mockk()
+    val mockPayload: SecretPayload = mockk()
+    val mockMetric: MetricClient = mockk()
+    val persistence = GoogleSecretManagerPersistence(projectId, mockClient, mockMetric)
+    val mockSecret: Secret = mockk()
+
+    every { mockPayload.data } returns ByteString.copyFromUtf8(secret)
+    every { mockResponse.payload } returns mockPayload
+    every { mockResponse.payload.data.toStringUtf8() } returns ""
+    every { mockGoogleClient.accessSecretVersion(ofType(SecretVersionName::class)) } returns mockResponse
+    every { mockSecret.name } returns secret
+    coEvery {
+      mockGoogleClient.createSecret(
+        any<ProjectName>(),
+        any<String>(),
+        any<Secret>(),
+      )
+    } returns mockSecret
+    every { mockGoogleClient.addSecretVersion(any<SecretName>(), any<SecretPayload>()) } throws RuntimeException()
+    every { mockGoogleClient.deleteSecret(secret) } just Runs
+    every { mockGoogleClient.close() } returns Unit
+    every { mockClient.createClient() } returns mockGoogleClient
+    every { mockMetric.count(any(), any(), any()) } returns Unit
+
+    assertThrows<RuntimeException> { persistence.writeWithExpiry(coordinate, secret) }
+
+    verify {
+      mockGoogleClient.addSecretVersion(any<SecretName>(), any<SecretPayload>())
+      mockGoogleClient.deleteSecret(secret)
+    }
+  }
+
+  @Test
+  fun `test exception when adding a secret version does not delete a pre-existing secret`() {
+    val secret = "secret value"
+    val projectId = "test"
+    val coordinate = SecretCoordinate.fromFullCoordinate("secret_coordinate_v1")
+    val mockClient: GoogleSecretManagerServiceClient = mockk()
+    val mockGoogleClient: SecretManagerServiceClient = mockk()
+    val mockResponse: AccessSecretVersionResponse = mockk()
+    val mockPayload: SecretPayload = mockk()
+    val mockMetric: MetricClient = mockk()
+    val persistence = GoogleSecretManagerPersistence(projectId, mockClient, mockMetric)
+
+    every { mockPayload.data } returns ByteString.copyFromUtf8(secret)
+    every { mockResponse.payload } returns mockPayload
+    every { mockResponse.payload.data.toStringUtf8() } returns "mocked-secret-value"
+    every { mockGoogleClient.accessSecretVersion(ofType(SecretVersionName::class)) } returns mockResponse
+    every { mockGoogleClient.addSecretVersion(any<SecretName>(), any<SecretPayload>()) } throws RuntimeException()
+    every { mockGoogleClient.close() } returns Unit
+    every { mockClient.createClient() } returns mockGoogleClient
+    every { mockMetric.count(any(), any(), any()) } returns Unit
+
+    assertThrows<RuntimeException> { persistence.writeWithExpiry(coordinate, secret) }
+
+    verify { mockGoogleClient.addSecretVersion(any<SecretName>(), any<SecretPayload>()) }
+    verify(exactly = 0) { mockGoogleClient.deleteSecret(any<SecretName>()) }
+  }
+
   @Test
   fun `test deleting a secret via the client deletes the secret`() {
     val secret = "secret value"