implement secret handling for workspace_service_account table #11946
Conversation
| private static TextNode getOrThrowSecretValueNode(final ReadOnlySecretPersistence secretPersistence, final SecretCoordinate coordinate) { | ||
| private static JsonNode getOrThrowSecretValueNode(final ReadOnlySecretPersistence secretPersistence, | ||
| final SecretCoordinate coordinate, | ||
| final boolean asTextNode) { |
There was a problem hiding this comment.
Nitpick. Whether the secret value is a text or json depends on the type of the secret. The asTextNode parameter makes it sound like the client can choose between the two types at will, which is slightly misleading. Maybe rename it to something like isTextSecret?
Since there are only two use cases of this method, this is probably over-optimization. So feel free to ignore.
There was a problem hiding this comment.
would it make sense to just have an inner and outter method here and get rid of the boolean? i think it would be a lot clearer what's happening.
JsonNode getOrThrowSecretValueJsonNode(...) {
...
}
TextNode getOrThrowSecretValueTextNode(...) {
return getOrThrowSecretValueJsonNode(...).asTextNode();
}
There was a problem hiding this comment.
Alternatively, getOrThrowSecretValueNode is called two times, once to get the text node, and once to get the json node. So this helper method is not that useful. It's also fine to just remove it.
| final SecretsRepositoryReader secretsRepositoryReader = | ||
| spy(new SecretsRepositoryReader(configRepository, realSecretsHydrator)); | ||
|
|
||
| final UUID workspaceId = UUID.fromString("13fb9a84-6bfa-4801-8f5e-ce717677babf"); |
There was a problem hiding this comment.
Many of the UUIDs and constants are reused in the two test files, like this workspace ID, the service account ID, and HMAC key. They can be moved to MockData.java for clarity.
...ig/persistence/src/main/java/io/airbyte/config/persistence/split_secrets/SecretsHelpers.java
Show resolved
Hide resolved
...ig/persistence/src/main/java/io/airbyte/config/persistence/split_secrets/SecretsHelpers.java
Outdated
Show resolved
Hide resolved
...ig/persistence/src/main/java/io/airbyte/config/persistence/split_secrets/SecretsHelpers.java
Outdated
Show resolved
Hide resolved
...g/persistence/src/main/java/io/airbyte/config/persistence/split_secrets/SecretsHydrator.java
Outdated
Show resolved
Hide resolved
| private static TextNode getOrThrowSecretValueNode(final ReadOnlySecretPersistence secretPersistence, final SecretCoordinate coordinate) { | ||
| private static JsonNode getOrThrowSecretValueNode(final ReadOnlySecretPersistence secretPersistence, | ||
| final SecretCoordinate coordinate, | ||
| final boolean asTextNode) { |
There was a problem hiding this comment.
would it make sense to just have an inner and outter method here and get rid of the boolean? i think it would be a lot clearer what's happening.
JsonNode getOrThrowSecretValueJsonNode(...) {
...
}
TextNode getOrThrowSecretValueTextNode(...) {
return getOrThrowSecretValueJsonNode(...).asTextNode();
}
...g/persistence/src/main/java/io/airbyte/config/persistence/split_secrets/SecretsHydrator.java
Outdated
Show resolved
Hide resolved
| clonedWorkspaceServiceAccount.setJsonCredential(jsonCredSecretCoordinateToPayload.secretCoordinateForDB()); | ||
| } | ||
|
|
||
| if (workspaceServiceAccount.getHmacKey() != null) { |
There was a problem hiding this comment.
i'm lost here. i understood lines 277-191 are storing a service account. from here down i'm not quite sure what this key is. i think we need the code to explain that somewhere.
There was a problem hiding this comment.
thanks. is all of this specific to a GCP service account (as opposed to an AWS one?)? If they are different how will we tell in the future? As long as we have answer to this, then all good. If it GCP specific now, worth adding a comment mentioning that as well and that we plan for it to evolve.
…nto secret-management-for-service-account-creds
| */ | ||
| private static TextNode getOrThrowSecretValueNode(final ReadOnlySecretPersistence secretPersistence, final SecretCoordinate coordinate) { | ||
| private static String getOrThrowSecretValueNode(final ReadOnlySecretPersistence secretPersistence, |
There was a problem hiding this comment.
getOrThrowSecretValueNode => getOrThrowSecretValue or even getOrThrowSecret. It's not a "node" anymore, right?
| clonedWorkspaceServiceAccount.setJsonCredential(jsonCredSecretCoordinateToPayload.secretCoordinateForDB()); | ||
| } | ||
|
|
||
| if (workspaceServiceAccount.getHmacKey() != null) { |
There was a problem hiding this comment.
thanks. is all of this specific to a GCP service account (as opposed to an AWS one?)? If they are different how will we tell in the future? As long as we have answer to this, then all good. If it GCP specific now, worth adding a comment mentioning that as well and that we plan for it to evolve.
subodh1810
merged commit c4d1837
into
introduce-persistence-code-for-service-account-table
* implement persistence code for workspace_service_account table * update yaml * implement secret handling for workspace_service_account table (#11946) * implement secret handling for workspace_service_account table * add new line to the mock json * get rid of file * address review comments * update method name and add comment
* implement migration to create workspace_service_account table * make all columns non nullable * introduce persistence code for service account table (#11944) * implement persistence code for workspace_service_account table * update yaml * implement secret handling for workspace_service_account table (#11946) * implement secret handling for workspace_service_account table * add new line to the mock json * get rid of file * address review comments * update method name and add comment
* implement migration to create workspace_service_account table * make all columns non nullable * introduce persistence code for service account table (#11944) * implement persistence code for workspace_service_account table * update yaml * implement secret handling for workspace_service_account table (#11946) * implement secret handling for workspace_service_account table * add new line to the mock json * get rid of file * address review comments * update method name and add comment
Issue : #11968