Skip to content

Commit c787b2f

Browse files
keyihuangkeyihuang
committed
chore: remove feature flags for secrets deletion (#13227)
1 parent 825870d commit c787b2f

File tree

7 files changed

+18
-214
lines changed

7 files changed

+18
-214
lines changed

‎airbyte-commons-server/src/main/java/io/airbyte/commons/server/handlers/DestinationHandler.java

+6-24
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@
4040
import io.airbyte.config.secrets.JsonSecretsProcessor;
4141
import io.airbyte.data.helpers.ActorDefinitionVersionUpdater;
4242
import io.airbyte.data.services.DestinationService;
43-
import io.airbyte.featureflag.DeleteSecretsWhenTombstoneActors;
4443
import io.airbyte.featureflag.FeatureFlagClient;
4544
import io.airbyte.featureflag.Workspace;
4645
import io.airbyte.persistence.job.factory.OAuthConfigSupplier;
@@ -149,31 +148,14 @@ public void deleteDestination(final DestinationRead destination)
149148
final ConnectorSpecification spec =
150149
getSpecForDestinationId(destination.getDestinationDefinitionId(), destination.getWorkspaceId(), destination.getDestinationId());
151150

152-
if (featureFlagClient.boolVariation(DeleteSecretsWhenTombstoneActors.INSTANCE, new Workspace(destination.getWorkspaceId().toString()))) {
153-
try {
154-
destinationService.tombstoneDestination(
155-
destination.getName(),
156-
destination.getWorkspaceId(),
157-
destination.getDestinationId(), spec);
158-
} catch (final io.airbyte.data.exceptions.ConfigNotFoundException e) {
159-
throw new ConfigNotFoundException(e.getType(), e.getConfigId());
160-
}
161-
} else {
162-
final JsonNode fullConfig;
163-
try {
164-
fullConfig = destinationService.getDestinationConnectionWithSecrets(destination.getDestinationId()).getConfiguration();
165-
} catch (final io.airbyte.data.exceptions.ConfigNotFoundException e) {
166-
throw new ConfigNotFoundException(e.getType(), e.getConfigId());
167-
}
168-
// persist
169-
persistDestinationConnection(
151+
// Delete secrets and config in this destination and mark it tombstoned.
152+
try {
153+
destinationService.tombstoneDestination(
170154
destination.getName(),
171-
destination.getDestinationDefinitionId(),
172155
destination.getWorkspaceId(),
173-
destination.getDestinationId(),
174-
fullConfig,
175-
true,
176-
spec);
156+
destination.getDestinationId(), spec);
157+
} catch (final io.airbyte.data.exceptions.ConfigNotFoundException e) {
158+
throw new ConfigNotFoundException(e.getType(), e.getConfigId());
177159
}
178160
}
179161

‎airbyte-commons-server/src/main/java/io/airbyte/commons/server/handlers/SourceHandler.java

+6-24
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@
5252
import io.airbyte.data.services.SecretPersistenceConfigService;
5353
import io.airbyte.data.services.SourceService;
5454
import io.airbyte.data.services.WorkspaceService;
55-
import io.airbyte.featureflag.DeleteSecretsWhenTombstoneActors;
5655
import io.airbyte.featureflag.FeatureFlagClient;
5756
import io.airbyte.featureflag.Organization;
5857
import io.airbyte.featureflag.UseRuntimeSecretPersistence;
@@ -376,31 +375,14 @@ public void deleteSource(final SourceRead source)
376375

377376
final var spec = getSpecFromSourceId(source.getSourceId());
378377

379-
if (featureFlagClient.boolVariation(DeleteSecretsWhenTombstoneActors.INSTANCE, new Workspace(source.getWorkspaceId().toString()))) {
380-
try {
381-
sourceService.tombstoneSource(
382-
source.getName(),
383-
source.getWorkspaceId(),
384-
source.getSourceId(), spec);
385-
} catch (final io.airbyte.data.exceptions.ConfigNotFoundException e) {
386-
throw new ConfigNotFoundException(e.getType(), e.getConfigId());
387-
}
388-
} else {
389-
final JsonNode fullConfig;
390-
try {
391-
fullConfig = sourceService.getSourceConnectionWithSecrets(source.getSourceId()).getConfiguration();
392-
} catch (final io.airbyte.data.exceptions.ConfigNotFoundException e) {
393-
throw new ConfigNotFoundException(e.getType(), e.getConfigId());
394-
}
395-
// persist
396-
persistSourceConnection(
378+
// Delete secrets and config in this source and mark it tombstoned.
379+
try {
380+
sourceService.tombstoneSource(
397381
source.getName(),
398-
source.getSourceDefinitionId(),
399382
source.getWorkspaceId(),
400-
source.getSourceId(),
401-
true,
402-
fullConfig,
403-
spec);
383+
source.getSourceId(), spec);
384+
} catch (final io.airbyte.data.exceptions.ConfigNotFoundException e) {
385+
throw new ConfigNotFoundException(e.getType(), e.getConfigId());
404386
}
405387
}
406388

‎airbyte-commons-server/src/test/java/io/airbyte/commons/server/handlers/DestinationHandlerTest.java

+2-59
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import static org.junit.jupiter.api.Assertions.assertEquals;
88
import static org.junit.jupiter.api.Assertions.assertTrue;
99
import static org.mockito.ArgumentMatchers.any;
10-
import static org.mockito.ArgumentMatchers.eq;
1110
import static org.mockito.Mockito.mock;
1211
import static org.mockito.Mockito.times;
1312
import static org.mockito.Mockito.verify;
@@ -47,7 +46,6 @@
4746
import io.airbyte.config.secrets.JsonSecretsProcessor;
4847
import io.airbyte.data.helpers.ActorDefinitionVersionUpdater;
4948
import io.airbyte.data.services.DestinationService;
50-
import io.airbyte.featureflag.DeleteSecretsWhenTombstoneActors;
5149
import io.airbyte.featureflag.TestClient;
5250
import io.airbyte.featureflag.Workspace;
5351
import io.airbyte.persistence.job.factory.OAuthConfigSupplier;
@@ -337,57 +335,6 @@ void testListDestinationForWorkspace() throws JsonValidationException, ConfigNot
337335
.prepareSecretsForOutput(destinationConnection.getConfiguration(), destinationDefinitionSpecificationRead.getConnectionSpecification());
338336
}
339337

340-
@Test
341-
void testDeleteDestination()
342-
throws JsonValidationException, ConfigNotFoundException, IOException, io.airbyte.data.exceptions.ConfigNotFoundException {
343-
final JsonNode newConfiguration = destinationConnection.getConfiguration();
344-
((ObjectNode) newConfiguration).put("apiKey", "987-xyz");
345-
346-
final DestinationConnection expectedSourceConnection = Jsons.clone(destinationConnection).withTombstone(true);
347-
348-
final DestinationIdRequestBody destinationIdRequestBody = new DestinationIdRequestBody().destinationId(destinationConnection.getDestinationId());
349-
final StandardSync standardSync = ConnectionHelpers.generateSyncWithDestinationId(destinationConnection.getDestinationId());
350-
standardSync.setBreakingChange(false);
351-
final ConnectionRead connectionRead = ConnectionHelpers.generateExpectedConnectionRead(standardSync);
352-
final ConnectionReadList connectionReadList = new ConnectionReadList().connections(Collections.singletonList(connectionRead));
353-
final WorkspaceIdRequestBody workspaceIdRequestBody = new WorkspaceIdRequestBody().workspaceId(destinationConnection.getWorkspaceId());
354-
355-
when(configRepository.getDestinationConnection(destinationConnection.getDestinationId()))
356-
.thenReturn(destinationConnection)
357-
.thenReturn(expectedSourceConnection);
358-
when(destinationService.getDestinationConnectionWithSecrets(destinationConnection.getDestinationId()))
359-
.thenReturn(destinationConnection)
360-
.thenReturn(expectedSourceConnection);
361-
when(oAuthConfigSupplier.maskSourceOAuthParameters(destinationDefinitionSpecificationRead.getDestinationDefinitionId(),
362-
destinationConnection.getWorkspaceId(),
363-
newConfiguration, destinationDefinitionVersion.getSpec())).thenReturn(newConfiguration);
364-
when(configRepository.getStandardDestinationDefinition(destinationDefinitionSpecificationRead.getDestinationDefinitionId()))
365-
.thenReturn(standardDestinationDefinition);
366-
when(actorDefinitionVersionHelper.getDestinationVersion(standardDestinationDefinition, destinationConnection.getWorkspaceId(),
367-
destinationConnection.getDestinationId()))
368-
.thenReturn(destinationDefinitionVersion);
369-
when(configRepository.getDestinationDefinitionFromDestination(destinationConnection.getDestinationId()))
370-
.thenReturn(standardDestinationDefinition);
371-
when(connectionsHandler.listConnectionsForWorkspace(workspaceIdRequestBody)).thenReturn(connectionReadList);
372-
when(
373-
secretsProcessor.prepareSecretsForOutput(destinationConnection.getConfiguration(),
374-
destinationDefinitionSpecificationRead.getConnectionSpecification()))
375-
.thenReturn(destinationConnection.getConfiguration());
376-
when(actorDefinitionVersionHelper.getDestinationVersionWithOverrideStatus(standardDestinationDefinition, destinationConnection.getWorkspaceId(),
377-
destinationConnection.getDestinationId())).thenReturn(destinationDefinitionVersionWithOverrideStatus);
378-
// By default feature flag is false
379-
when(featureFlagClient.boolVariation(
380-
eq(DeleteSecretsWhenTombstoneActors.INSTANCE),
381-
any(Workspace.class))).thenReturn(false);
382-
383-
destinationHandler.deleteDestination(destinationIdRequestBody);
384-
385-
verify(destinationService).getDestinationConnectionWithSecrets(any());
386-
verify(destinationService).writeDestinationConnectionWithSecrets(any(), any());
387-
verify(connectionsHandler).listConnectionsForWorkspace(workspaceIdRequestBody);
388-
verify(connectionsHandler).deleteConnection(connectionRead.getConnectionId());
389-
}
390-
391338
@Test
392339
void testDeleteDestinationAndDeleteSecrets()
393340
throws JsonValidationException, ConfigNotFoundException, IOException, io.airbyte.data.exceptions.ConfigNotFoundException {
@@ -426,14 +373,10 @@ void testDeleteDestinationAndDeleteSecrets()
426373
.thenReturn(destinationConnection.getConfiguration());
427374
when(actorDefinitionVersionHelper.getDestinationVersionWithOverrideStatus(standardDestinationDefinition, destinationConnection.getWorkspaceId(),
428375
destinationConnection.getDestinationId())).thenReturn(destinationDefinitionVersionWithOverrideStatus);
429-
// Turn on feature flag to delete secrets
430-
when(featureFlagClient.boolVariation(
431-
eq(DeleteSecretsWhenTombstoneActors.INSTANCE),
432-
any(Workspace.class))).thenReturn(true);
433376
destinationHandler.deleteDestination(destinationIdRequestBody);
434377

435-
// With the flag on, we should not no longer get secrets or write secrets anymore (since we are
436-
// deleting the destination).
378+
// We should not no longer get secrets or write secrets anymore (since we are deleting the
379+
// destination).
437380
verify(destinationService, times(0)).writeDestinationConnectionWithSecrets(expectedSourceConnection, connectorSpecification);
438381
verify(destinationService, times(0)).getDestinationConnectionWithSecrets(any());
439382
verify(destinationService).tombstoneDestination(any(), any(), any(), any());

‎airbyte-commons-server/src/test/java/io/airbyte/commons/server/handlers/SourceHandlerTest.java

+1-53
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import static io.airbyte.protocol.models.CatalogHelpers.createAirbyteStream;
88
import static org.junit.jupiter.api.Assertions.assertEquals;
99
import static org.mockito.ArgumentMatchers.any;
10-
import static org.mockito.ArgumentMatchers.eq;
1110
import static org.mockito.Mockito.doReturn;
1211
import static org.mockito.Mockito.mock;
1312
import static org.mockito.Mockito.never;
@@ -59,7 +58,6 @@
5958
import io.airbyte.data.services.SecretPersistenceConfigService;
6059
import io.airbyte.data.services.SourceService;
6160
import io.airbyte.data.services.WorkspaceService;
62-
import io.airbyte.featureflag.DeleteSecretsWhenTombstoneActors;
6361
import io.airbyte.featureflag.TestClient;
6462
import io.airbyte.featureflag.Workspace;
6563
import io.airbyte.persistence.job.factory.OAuthConfigSupplier;
@@ -462,51 +460,6 @@ void testSearchSources() throws JsonValidationException, ConfigNotFoundException
462460
assertEquals(0, actualSourceReadList.getSources().size());
463461
}
464462

465-
@Test
466-
void testDeleteSource() throws JsonValidationException, ConfigNotFoundException, IOException, io.airbyte.data.exceptions.ConfigNotFoundException {
467-
final JsonNode newConfiguration = sourceConnection.getConfiguration();
468-
((ObjectNode) newConfiguration).put("apiKey", "987-xyz");
469-
470-
final SourceConnection expectedSourceConnection = Jsons.clone(sourceConnection).withTombstone(true);
471-
472-
final SourceIdRequestBody sourceIdRequestBody = new SourceIdRequestBody().sourceId(sourceConnection.getSourceId());
473-
final StandardSync standardSync = ConnectionHelpers.generateSyncWithSourceId(sourceConnection.getSourceId());
474-
final ConnectionRead connectionRead = ConnectionHelpers.generateExpectedConnectionRead(standardSync);
475-
final ConnectionReadList connectionReadList = new ConnectionReadList().connections(Collections.singletonList(connectionRead));
476-
final WorkspaceIdRequestBody workspaceIdRequestBody = new WorkspaceIdRequestBody().workspaceId(sourceConnection.getWorkspaceId());
477-
478-
when(configRepository.getSourceConnection(sourceConnection.getSourceId()))
479-
.thenReturn(sourceConnection)
480-
.thenReturn(expectedSourceConnection);
481-
when(sourceService.getSourceConnectionWithSecrets(sourceConnection.getSourceId()))
482-
.thenReturn(sourceConnection)
483-
.thenReturn(expectedSourceConnection);
484-
when(oAuthConfigSupplier.maskSourceOAuthParameters(sourceDefinitionSpecificationRead.getSourceDefinitionId(),
485-
sourceConnection.getWorkspaceId(),
486-
newConfiguration, sourceDefinitionVersion.getSpec())).thenReturn(newConfiguration);
487-
when(configRepository.getStandardSourceDefinition(sourceDefinitionSpecificationRead.getSourceDefinitionId()))
488-
.thenReturn(standardSourceDefinition);
489-
when(actorDefinitionVersionHelper.getSourceVersion(standardSourceDefinition, sourceConnection.getWorkspaceId(), sourceConnection.getSourceId()))
490-
.thenReturn(sourceDefinitionVersion);
491-
when(configRepository.getSourceDefinitionFromSource(sourceConnection.getSourceId())).thenReturn(standardSourceDefinition);
492-
when(connectionsHandler.listConnectionsForWorkspace(workspaceIdRequestBody)).thenReturn(connectionReadList);
493-
when(
494-
secretsProcessor.prepareSecretsForOutput(sourceConnection.getConfiguration(), sourceDefinitionSpecificationRead.getConnectionSpecification()))
495-
.thenReturn(sourceConnection.getConfiguration());
496-
when(actorDefinitionVersionHelper.getSourceVersionWithOverrideStatus(standardSourceDefinition, sourceConnection.getWorkspaceId(),
497-
sourceConnection.getSourceId())).thenReturn(sourceDefinitionVersionWithOverrideStatus);
498-
// By default feature flag is false
499-
when(featureFlagClient.boolVariation(
500-
eq(DeleteSecretsWhenTombstoneActors.INSTANCE),
501-
any(Workspace.class))).thenReturn(false);
502-
503-
sourceHandler.deleteSource(sourceIdRequestBody);
504-
505-
verify(sourceService).writeSourceConnectionWithSecrets(expectedSourceConnection, connectorSpecification);
506-
verify(connectionsHandler).listConnectionsForWorkspace(workspaceIdRequestBody);
507-
verify(connectionsHandler).deleteConnection(connectionRead.getConnectionId());
508-
}
509-
510463
@Test
511464
void testDeleteSourceAndDeleteSecrets()
512465
throws JsonValidationException, ConfigNotFoundException, IOException, io.airbyte.data.exceptions.ConfigNotFoundException {
@@ -541,15 +494,10 @@ void testDeleteSourceAndDeleteSecrets()
541494
.thenReturn(sourceConnection.getConfiguration());
542495
when(actorDefinitionVersionHelper.getSourceVersionWithOverrideStatus(standardSourceDefinition, sourceConnection.getWorkspaceId(),
543496
sourceConnection.getSourceId())).thenReturn(sourceDefinitionVersionWithOverrideStatus);
544-
// Turn on feature flag to delete secrets
545-
when(featureFlagClient.boolVariation(
546-
eq(DeleteSecretsWhenTombstoneActors.INSTANCE),
547-
any(Workspace.class))).thenReturn(true);
548497

549498
sourceHandler.deleteSource(sourceIdRequestBody);
550499

551-
// With the flag on, we should not no longer get secrets or write secrets anymore (since we are
552-
// deleting the source).
500+
// We should not no longer get secrets or write secrets anymore (since we are deleting the source).
553501
verify(sourceService, times(0)).writeSourceConnectionWithSecrets(expectedSourceConnection, connectorSpecification);
554502
verify(sourceService, times(0)).getSourceConnectionWithSecrets(any());
555503
verify(sourceService).tombstoneSource(any(), any(), any(), any());

‎airbyte-config/config-secrets/src/main/kotlin/secrets/SecretsRepositoryWriter.kt

+3-7
Original file line numberDiff line numberDiff line change
@@ -8,9 +8,7 @@ import com.fasterxml.jackson.databind.JsonNode
88
import io.airbyte.commons.json.JsonPaths
99
import io.airbyte.config.secrets.persistence.RuntimeSecretPersistence
1010
import io.airbyte.config.secrets.persistence.SecretPersistence
11-
import io.airbyte.featureflag.DeleteDanglingSecrets
1211
import io.airbyte.featureflag.FeatureFlagClient
13-
import io.airbyte.featureflag.Workspace
1412
import io.airbyte.metrics.lib.MetricAttribute
1513
import io.airbyte.metrics.lib.MetricClient
1614
import io.airbyte.metrics.lib.MetricTags
@@ -136,11 +134,9 @@ open class SecretsRepositoryWriter(
136134
runtimeSecretPersistence?.write(coordinate, payload) ?: secretPersistence.write(coordinate, payload)
137135
metricClient.count(OssMetricsRegistry.UPDATE_SECRET_DEFAULT_STORE, 1)
138136
}
139-
// Delete old secrets (controlled by a workspace level feature flag).
140-
// TODO: remove this flag after testing so that by default we are always cleaning up old secrets
141-
if (featureFlagClient.boolVariation(DeleteDanglingSecrets, Workspace(workspaceId))) {
142-
deleteFromConfig(oldPartialConfig, spec, runtimeSecretPersistence)
143-
}
137+
// Delete old secrets.
138+
deleteFromConfig(oldPartialConfig, spec, runtimeSecretPersistence)
139+
144140
return updatedSplitConfig.partialConfig
145141
}
146142

‎airbyte-config/config-secrets/src/test/kotlin/secrets/SecretsRepositoryWriterTest.kt

-43
Original file line numberDiff line numberDiff line change
@@ -223,49 +223,6 @@ internal class SecretsRepositoryWriterTest {
223223
verify { metricClient.count(OssMetricsRegistry.DELETE_SECRET_DEFAULT_STORE, 1, MetricAttribute(MetricTags.SUCCESS, "true")) }
224224
}
225225

226-
@Test
227-
fun testUpdateSecretFeatureFlagFalseShouldNotDelete() {
228-
val secret = "secret-1"
229-
val oldCoordinate = "existing_coordinate_v1"
230-
secretPersistence.write(SecretCoordinate.fromFullCoordinate(oldCoordinate), secret)
231-
232-
every { featureFlagClient.boolVariation(any(), any()) } returns false
233-
234-
val updatedFullConfigNoSecretChange =
235-
Jsons.deserialize(
236-
"""
237-
{ "username": "airbyte1", "password": "$secret"}
238-
""".trimIndent(),
239-
)
240-
241-
val oldPartialConfig = injectCoordinate(oldCoordinate)
242-
val updatedPartialConfig =
243-
secretsRepositoryWriter.updateFromConfig(
244-
WORKSPACE_ID,
245-
oldPartialConfig,
246-
updatedFullConfigNoSecretChange,
247-
SPEC.connectionSpecification,
248-
null,
249-
)
250-
251-
val newCoordinate = "existing_coordinate_v2"
252-
val expPartialConfig =
253-
Jsons.deserialize(
254-
"""
255-
{"username":"airbyte1","password":{"_secret":"$newCoordinate"}}
256-
""".trimIndent(),
257-
)
258-
assertEquals(expPartialConfig, updatedPartialConfig)
259-
260-
verify(exactly = 1) { secretPersistence.write(SecretCoordinate.fromFullCoordinate(newCoordinate), secret) }
261-
assertEquals(secret, secretPersistence.read(SecretCoordinate.fromFullCoordinate(newCoordinate)))
262-
verify { metricClient.count(OssMetricsRegistry.UPDATE_SECRET_DEFAULT_STORE, 1) }
263-
264-
verify(exactly = 0) { secretPersistence.delete(SecretCoordinate.fromFullCoordinate(oldCoordinate)) }
265-
assertEquals(secret, secretPersistence.read(SecretCoordinate.fromFullCoordinate(oldCoordinate)))
266-
verify(exactly = 0) { metricClient.count(OssMetricsRegistry.DELETE_SECRET_DEFAULT_STORE, 1) }
267-
}
268-
269226
@Test
270227
fun testUpdateSecretDeleteErrorShouldNotPropagate() {
271228
secretPersistence = mockk()

‎airbyte-featureflag/src/main/kotlin/FlagDefinitions.kt

-4
Original file line numberDiff line numberDiff line change
@@ -176,10 +176,6 @@ object UseCustomK8sInitCheck : Temporary<Boolean>(key = "platform.use-custom-k8s
176176

177177
object ConnectionFieldLimitOverride : Permanent<Int>(key = "connection-field-limit-override", default = -1)
178178

179-
object DeleteDanglingSecrets : Temporary<Boolean>(key = "platform.delete-dangling-secrets", default = false)
180-
181-
object DeleteSecretsWhenTombstoneActors : Temporary<Boolean>(key = "platform.delete-secrets-when-tombstone-actors", default = false)
182-
183179
object EnableResumableFullRefresh : Temporary<Boolean>(key = "platform.enable-resumable-full-refresh", default = false)
184180

185181
object AlwaysRunCheckBeforeSync : Permanent<Boolean>(key = "platform.always-run-check-before-sync", default = false)

0 commit comments

Comments
 (0)