Feature: Ability to save/load secrets through google secrets manager. #5791
Conversation
airbyte-jenny
changed the title
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Optional; | ||
| import java.util.Set; | ||
| import java.util.UUID; | ||
| import java.util.stream.Stream; | ||
|
|
||
| public class ConfigRepository { | ||
|
|
||
| private final ConfigPersistence persistence; |
There was a problem hiding this comment.
Nitpick. Maybe rename this variable to something like routinePersistence or nonSecrectsPersistence to distinguish it from the newly added one?
| Map<String, Stream<JsonNode>> bothConfigTypes = persistence.dumpConfigs(); | ||
| // Secrets and nonsecrets might be stored separately, and if they are, go looking for the rest. | ||
| // No need to do that if they're in the same single store, because we don't need duplicates or | ||
| // double work. |
There was a problem hiding this comment.
Nitpick. This comment contains some pronouns that take a few seconds to understand: the rest, do that. Also the structure in replaceAllConfigs looks more clear than this one.
What about following the same structure as in replaceAllConfigs?
// If secrets and nonsecrets are stored in the same persistence,
// just return configs from one persistence becase we don't need duplicates or double work.
if (secretsPersistence == persistence) {
return persistence.dumpConfigs();
}
// If secrets and nonsecrets are stored separately, return configs from both.
return MoreMaps.merge(persistence.dumpConfigs(), secretsPersistence.dumpConfigs());| * During migration, this runs through the list of connectors, determining their docker image and | ||
| * version, for purposes of skipping upgrading in-use definitions. | ||
| * | ||
| * @return |
There was a problem hiding this comment.
Nitpick. This @return doc is empty and can be removed.
| } | ||
|
|
||
| /** | ||
| * During migration, this runs through the list of connectors, determining their docker image and |
There was a problem hiding this comment.
Migration is a loaded term. It's more like "connector definition update" instead of "migration".
| * public void loadData(ConfigPersistence seedConfigPersistence) throws IOException { | ||
| * database.transaction(ctx -> { updateConfigsFromSeed(ctx, seedConfigPersistence); return null; }); | ||
| * } | ||
| */ |
| for (DestinationConnection dest : destinations) { | ||
| definitionIds.add(dest.getDestinationDefinitionId().toString()); | ||
| } | ||
| } |
There was a problem hiding this comment.
There are extra code blocks here. They can be removed.
| } | ||
| } | ||
| return definitionIds; | ||
| } |
There was a problem hiding this comment.
Same here that this method does not need to be implemented in this implementation.
| updateConfigsFromSeed(ctx, seedConfigPersistence, connectorRepositoriesInUse); | ||
| } else { | ||
| copyConfigsFromSeed(ctx, seedConfigPersistence); | ||
| } |
There was a problem hiding this comment.
The server start logic has been changed. The loadData method only updates the connector definitions now. It will only be called after the database has been initialized.
This is necessary to fix a bug during server launch. The rationale is that we cannot insert the seed or update the connector definitions as part of the initialization process. When the server launches, it can be on an old version, and the seed or the latest definitions may not be compatible with the old data structure. The server will run the migrations first, and then we call loadData to update the connectors.
So adding back the copyConfigsFromSeed is no not necessary here.
There was a problem hiding this comment.
Sorry that this change was added recently. Let me know if you want to discuss this in person for clarification.
There was a problem hiding this comment.
More context. The only thing we do to "initialize" the persistence is copying old configs from the file system if the user is running an old Airbyte version:
https://github.com/airbytehq/airbyte/blob/master/airbyte-server/src/main/java/io/airbyte/server/ServerApp.java#L187
We call loadData after file-based and database migrations here:
https://github.com/airbytehq/airbyte/blob/master/airbyte-server/src/main/java/io/airbyte/server/ServerApp.java#L248
| assertRecordCount(2); | ||
| assertHasSource(SOURCE_GITHUB); | ||
| assertHasDestination(DESTINATION_SNOWFLAKE); | ||
| verify(configPersistence, times(1)).copyConfigsFromSeed(any(DSLContext.class), any(ConfigPersistence.class)); |
There was a problem hiding this comment.
As mentioned above, the copyConfigsFromSeed won't be triggered in loadData method. So I feel that adding this test case is unnecessary.
| configPersistence.loadData(seedPersistence, new HashSet<String>()); | ||
| secretsPersistence = new FileSystemConfigPersistence(p); | ||
| configRepository = new ConfigRepository(configPersistence, secretsPersistence); | ||
| // configRepository = new ConfigRepository(configPersistence, configPersistence); |
|
|
|
We moved parts from this PR to other implementations, and do not need this particular code anymore. Closing. |
What
Moves connector config json with secrets in it out of the main database and into a dedicated secrets manager. Provides a simple wrapper on google secrets manager SDK calls to simplify those calls.
How
Modifies ConfigRepository to use two persistence stores, and separate configs by type to save in each, recombining them on retrieval. Only works if the feature flag is enabled and GCP credentials are provided in the environment. This puts the entire config for source connection or destination connection into the secrets store; it does not break out individual secrets yet.
Recommended reading order
ConfigRepository.javaGoogleSecretsManager.javaGoogleSecretsManagerConfigPersistence.java