Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,22 @@ public interface Configs {

/**
* Defines the Secret Persistence type. None by default. Set to GOOGLE_SECRET_MANAGER to use Google
* Secret Manager. Set to TESTING_CONFIG_DB_TABLE to use the database as a test. Alpha support.
* Undefined behavior will result if this is turned on and then off.
* Secret Manager. Set to TESTING_CONFIG_DB_TABLE to use the database as a test. Set to VAULT to use
* Hashicorp Vault. Alpha support. Undefined behavior will result if this is turned on and then off.
*/
SecretPersistenceType getSecretPersistenceType();

/**
* Define the vault address to read/write Airbyte Configuration to Hashicorp Vault. Alpha Support.
*/
String getVaultAddress();

/**
* Define the vault path prefix to read/write Airbyte Configuration to Hashicorp Vault. Empty by
* default. Alpha Support.
*/
String getVaultPrefix();

// Database
/**
* Define the Jobs Database user.
Expand Down Expand Up @@ -535,7 +546,8 @@ enum DeploymentMode {
enum SecretPersistenceType {
NONE,
TESTING_CONFIG_DB_TABLE,
GOOGLE_SECRET_MANAGER
GOOGLE_SECRET_MANAGER,
VAULT
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ public class EnvConfigs implements Configs {
private static final String DEFAULT_JOB_KUBE_CURL_IMAGE = "curlimages/curl:7.83.1";
private static final int DEFAULT_DATABASE_INITIALIZATION_TIMEOUT_MS = 60 * 1000;

private static final String VAULT_ADDRESS = "VAULT_ADDRESS";
private static final String VAULT_PREFIX = "VAULT_PREFIX";

public static final long DEFAULT_MAX_SPEC_WORKERS = 5;
public static final long DEFAULT_MAX_CHECK_WORKERS = 5;
public static final long DEFAULT_MAX_DISCOVER_WORKERS = 5;
Expand Down Expand Up @@ -328,6 +331,16 @@ public SecretPersistenceType getSecretPersistenceType() {
return SecretPersistenceType.valueOf(secretPersistenceStr);
}

@Override
public String getVaultAddress() {
return getEnv(VAULT_ADDRESS);
}

@Override
public String getVaultPrefix() {
return getEnvOrDefault(VAULT_PREFIX, "");
}

// Database
@Override
public String getDatabaseUser() {
Expand Down
2 changes: 2 additions & 0 deletions airbyte-config/config-persistence/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@ dependencies {

implementation 'commons-io:commons-io:2.7'
implementation 'com.google.cloud:google-cloud-secretmanager:2.0.5'
implementation 'com.bettercloud:vault-java-driver:5.1.0'

testImplementation 'org.hamcrest:hamcrest-all:1.3'
testImplementation libs.platform.testcontainers.postgresql
testImplementation libs.flyway.core
testImplementation project(':airbyte-test-utils')
testImplementation "org.testcontainers:vault:1.17.2"
integrationTestJavaImplementation project(':airbyte-config:config-persistence')
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ static Optional<SecretPersistence> getLongLived(final DSLContext dslContext, fin
case GOOGLE_SECRET_MANAGER -> {
return Optional.of(GoogleSecretManagerPersistence.getLongLived(configs.getSecretStoreGcpProjectId(), configs.getSecretStoreGcpCredentials()));
}
case VAULT -> {
return Optional.of(new VaultSecretPersistence(configs.getVaultAddress(), configs.getVaultPrefix()));
}
default -> {
return Optional.empty();
}
Expand All @@ -56,6 +59,9 @@ static Optional<SecretPersistence> getEphemeral(final DSLContext dslContext, fin
case GOOGLE_SECRET_MANAGER -> {
return Optional.of(GoogleSecretManagerPersistence.getEphemeral(configs.getSecretStoreGcpProjectId(), configs.getSecretStoreGcpCredentials()));
}
case VAULT -> {
return Optional.of(new VaultSecretPersistence(configs.getVaultAddress(), configs.getVaultPrefix()));
}
default -> {
return Optional.empty();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
/*
* Copyright (c) 2022 Airbyte, Inc., all rights reserved.
*/

package io.airbyte.config.persistence.split_secrets;

import com.bettercloud.vault.Vault;
import com.bettercloud.vault.VaultConfig;
import com.bettercloud.vault.VaultException;
import io.airbyte.commons.lang.Exceptions;
import java.util.HashMap;
import java.util.Optional;
import lombok.extern.slf4j.Slf4j;

@Slf4j
final public class VaultSecretPersistence implements SecretPersistence {

private final String secretKey = "value";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it should be static and names SECRET_KEY

private final Vault vault;
private final String pathPrefix;

public VaultSecretPersistence(final String address, final String prefix) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove that method. If the vault mode is wanted, they have to specify a token in their env. This will make it explicit for someone reading the code that the identification is token based. getEnsureEnv in the Configs has a precondition for that.

this.vault = Exceptions.toRuntime(() -> getVaultClient(address));
this.pathPrefix = prefix;
}

/**
* Constructor for testing
*/
protected VaultSecretPersistence(final String address, final String prefix, final String token) {
this.vault = Exceptions.toRuntime(() -> getVaultClient(address, token));
this.pathPrefix = prefix;
}

@Override
public Optional<String> read(final SecretCoordinate coordinate) {
try {
final var response = vault.logical().read(pathPrefix + coordinate.getFullCoordinate());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: all the final var van be replace by val

final var restResponse = response.getRestResponse();
final var responseCode = restResponse.getStatus();
if (responseCode != 200) {
log.error("failed on read. Response code: " + responseCode);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the error message includes vault to be more comprehensive

return Optional.empty();
}
final var data = response.getData();
return Optional.of(data.get(secretKey));
} catch (final VaultException e) {
return Optional.empty();
}
}

@Override
public void write(final SecretCoordinate coordinate, final String payload) {
try {
final var newSecret = new HashMap<String, Object>();
newSecret.put(secretKey, payload);
vault.logical().write(pathPrefix + coordinate.getFullCoordinate(), newSecret);
} catch (final VaultException e) {
log.error("failed on write", e);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment about mentioning vault in the log.

}
}

/**
* This creates a vault client using a vault agent which uses AWS IAM for auth using engine version 2.
*/
private static Vault getVaultClient(final String address) throws VaultException {
final var config = new VaultConfig()
.address(address)
.engineVersion(2)
.build();
return new Vault(config);
}

/**
* Vault client for testing
*/
private static Vault getVaultClient(final String address, final String token) throws VaultException {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be made available for the non-test mode? It looks like it will only required to add one env variable.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should. Would this actually be 2 env vars? One to indicate the auth type and the other to pass in a token?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think this should be it.

final var config = new VaultConfig()
.address(address)
.token(token)
.engineVersion(2)
.build();
return new Vault(config);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
package io.airbyte.config.persistence.split_secrets;

import org.apache.commons.lang3.RandomUtils;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.testcontainers.vault.VaultContainer;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

public class VaultSecretPersistenceTest {
private VaultSecretPersistence persistence;
private String baseCoordinate;

private VaultContainer vaultContainer;

@BeforeEach
void setUp() {
vaultContainer = new VaultContainer("vault").withVaultToken("vault-dev-token-id");
vaultContainer.start();

final var vaultAddress = "http://" + vaultContainer.getHost() + ":" + vaultContainer.getFirstMappedPort();

persistence = new VaultSecretPersistence(vaultAddress, "secret/testing", "vault-dev-token-id");
baseCoordinate = "VaultSecretPersistenceIntegrationTest_coordinate_" + RandomUtils.nextInt() % 20000;
}

@AfterEach
void tearDown() {
vaultContainer.stop();
}

@Test
void testReadWriteUpdate() {
final var coordinate1 = new SecretCoordinate(baseCoordinate, 1);

// try reading non-existent value
final var firstRead = persistence.read(coordinate1);
assertTrue(firstRead.isEmpty());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit/Not needed for this review: We have started using assertJ that provides more meaningful failure reason. this could be written like: assertThat(firstRead).isTrue.


// write
final var firstPayload = "abc";
persistence.write(coordinate1, firstPayload);
final var secondRead = persistence.read(coordinate1);
assertTrue(secondRead.isPresent());
assertEquals(firstPayload, secondRead.get());

// update
final var secondPayload = "def";
final var coordinate2 = new SecretCoordinate(baseCoordinate, 2);
persistence.write(coordinate2, secondPayload);
final var thirdRead = persistence.read(coordinate2);
assertTrue(thirdRead.isPresent());
assertEquals(secondPayload, thirdRead.get());
}
}

4 changes: 3 additions & 1 deletion docs/operator-guides/configuring-airbyte.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,9 @@ The following variables are relevant to both Docker and Kubernetes.
#### Secrets
1. `SECRET_STORE_GCP_PROJECT_ID` - Defines the GCP Project to store secrets in. Alpha support.
2. `SECRET_STORE_GCP_CREDENTIALS` - Define the JSON credentials used to read/write Airbyte Configuration to Google Secret Manager. These credentials must have Secret Manager Read/Write access. Alpha support.
3. `SECRET_PERSISTENCE` - Defines the Secret Persistence type. Defaults to NONE. Set to GOOGLE_SECRET_MANAGER to use Google Secret Manager. Set to TESTING_CONFIG_DB_TABLE to use the database as a test. Alpha support. Undefined behavior will result if this is turned on and then off.
3. `VAULT_ADDRESS` - Define the vault address to read/write Airbyte Configuration to Hashicorp Vault. Alpha Support.
4. `VAULT_PREFIX` - Define the vault path prefix. Empty by default. Alpha Support.
5. `SECRET_PERSISTENCE` - Defines the Secret Persistence type. Defaults to NONE. Set to GOOGLE_SECRET_MANAGER to use Google Secret Manager. Set to TESTING_CONFIG_DB_TABLE to use the database as a test. Set to VAULT to use Hashicorp Vault. Alpha support. Undefined behavior will result if this is turned on and then off.

#### Database
1. `DATABASE_USER` - Define the Jobs Database user.
Expand Down