Extract secrets handling out of ConfigRepository #8898
Conversation
github-actions
bot
added
area/platform
...fig/persistence/src/test/java/io/airbyte/config/persistence/SecretsRepositoryReaderTest.java
Show resolved
Hide resolved
...fig/persistence/src/test/java/io/airbyte/config/persistence/SecretsRepositoryWriterTest.java
Show resolved
Hide resolved
...-config/persistence/src/main/java/io/airbyte/config/persistence/SecretsRepositoryReader.java
Outdated
Show resolved
Hide resolved
...-config/persistence/src/main/java/io/airbyte/config/persistence/SecretsRepositoryReader.java
Outdated
Show resolved
Hide resolved
| return splitSecretConfig.getPartialConfig(); | ||
| } | ||
| } else { | ||
| return fullConfig; |
There was a problem hiding this comment.
Does this have the potential to break the guarantee stated in the javadoc?
never returns a secrets as return values (even the ones that are passed in as arguments)
There was a problem hiding this comment.
fair. moved this method to be private. added a todo to get rid of the case where a secret can be returned. the root problem here is that we handle secrets so differently when a user does or does not provide a secrets store. fixing that is out of scope for this PR, but once we fix that, then we can get get the guarantee we are looking for.
| } | ||
|
|
||
| public void loadData(final ConfigPersistence seedPersistence) throws IOException { | ||
| configRepository.loadDataNoSecrets(seedPersistence); |
There was a problem hiding this comment.
Do we not need to split out any secrets here before calling loadDataNoSecrets()?
There was a problem hiding this comment.
good call. in practice, this doesn't matter. the seedPersistence will never have secrets in it. that said, it is misleading. i'm going to move this back into config repository to avoid confusion.
airbyte-server/src/main/java/io/airbyte/server/ServerFactory.java
Outdated
Show resolved
Hide resolved
airbyte-server/src/main/java/io/airbyte/server/ConfigDumpImporter.java
Outdated
Show resolved
Hide resolved
| connectorSpecification); | ||
| final SourceConnection partialSource = Jsons.clone(source).withConfiguration(partialConfig); | ||
|
|
||
| // validate partial to avoid secret leak issues. |
There was a problem hiding this comment.
So this would make the cloud version not include the coordinate, but only *****? Seems fine for now, but we will likely want to change that in the future when we support user secret stores on cloud.
There was a problem hiding this comment.
i don't think i follow. this is just doing a validation.
| private final ConfigRepository configRepository; | ||
| private final SecretsHydrator secretsHydrator; | ||
|
|
||
| public SecretsRepositoryReader(final ConfigRepository configRepository, |
There was a problem hiding this comment.
Nit: You can use @AllArgConstructor here.
| */ | ||
| public class SecretsRepositoryReader { | ||
|
|
||
| private static final Logger LOGGER = LoggerFactory.getLogger(SecretsRepositoryReader.class); |
There was a problem hiding this comment.
Nit: you can use @Slf4j to instantiate the logger. The const will be named log instead of LOGGER unfortunately.
| } | ||
|
|
||
| public SourceConnection getSourceConnectionWithSecrets(final UUID sourceId) throws JsonValidationException, IOException, ConfigNotFoundException { | ||
| final var source = configRepository.getSourceConnection(sourceId); |
There was a problem hiding this comment.
nit: val should work here instead of final var. I have the same comments for the other final var variables.
...-config/persistence/src/main/java/io/airbyte/config/persistence/SecretsRepositoryReader.java
Outdated
Show resolved
Hide resolved
| try { | ||
| return Optional.of(configRepository.getSourceConnection(sourceId)); | ||
| } catch (final ConfigNotFoundException e) { | ||
| return Optional.empty(); |
There was a problem hiding this comment.
Why this is not an error? Is there a valid use case where a source don't have a config?
There was a problem hiding this comment.
it is possible that it hasn't been persisted to the db yet. if it hasn't then this returns empty.
cgardens
force-pushed
the
cgardens/secrets-repository
branch
from
8dcf003
to
aa854e7
Compare
What
ConfigRepositoryit hard for me to track where we are actually using secrets. It also means that plenty places that need not know about connector secrets handling currently need to, because it's all in that one class.How
SecretsRepositoryReaderandSecretsRepositoryWriter. These new classes give better ability to audit where secrets are, remove secrets from classes that have a lot of other responsibilities, and help us have clearer contracts on secrets in our java classes. I split into Reader and Writer so we have crystal clear visibility into what classes read versus write secrets.Recommended reading order
SecretsRepositoryReader.javaSecretsRepositoryWriter.javaConfigRepository.java🚨 User Impact 🚨
None. All internal refactor.
This change is