Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add the ability to use a secret persistence #6415

Merged
merged 37 commits into from
Sep 29, 2021

Conversation

jrhizor
Copy link
Contributor

@jrhizor jrhizor commented Sep 23, 2021

This PR is meant to enable external secret persistences and optional coordinate-based secret access throughout the system.

It's probably most useful to start by looking at ConfigRepository and following through execution paths for syncs and check/discover workflows.

There are a few things I'd like feedback on here:

  • Is there a better way of optionally offering secret access?
  • How should we handle the migration process? (see comment)
  • Check/discover workflows don't store a config with a set id but they do need to store their secrets outside of temporal. Is there a better alternative to using a TTL?

If you want to play around with it locally I recommend setting SECRET_PERSISTENCE to TESTING_CONFIG_DB_TABLE so you don't need to deal with GCP secrets.

todos:

  • offer the option to expose partial configs and do combining at the worker layer
  • verify that updating connection information works
  • handle making ephemeral secrets in check/discover
  • make sure migrations work as expected (open question in todo still)
  • figure out why the acceptance tests create superfluous secret versions
  • add real feature flagging

out of scope for this pr (create tickets):

  • adding to cloud
  • add automated testing to verify the secrets aren't stored in the db
  • add automated testing to verify the secrets aren't stored in temporal
  • removing oauth secrets from temporal
  • ticket to track number of secrets (ephemeral and permanent) that we have in our system
  • create a debug docker-compose extension file that exposes db ports and also gives the temporal ui

@github-actions github-actions bot added the area/platform issues related to the platform label Sep 23, 2021
@jrhizor jrhizor temporarily deployed to more-secrets September 23, 2021 23:08 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 23, 2021 23:27 Inactive
@jrhizor
Copy link
Contributor Author

jrhizor commented Sep 23, 2021

For step 1, just running a sync from the config repository while accessing a db-based secrets layer is running!
Secrets are still being passed to temporal and I haven't tested import/export functionality, but it's looking on the right track!

@@ -23,6 +23,8 @@ services:
logging: *default-logging
container_name: airbyte-db
restart: unless-stopped
ports:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: remove this before merging

@github-actions github-actions bot added the area/worker Related to worker label Sep 24, 2021
@jrhizor jrhizor temporarily deployed to more-secrets September 24, 2021 22:16 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 25, 2021 05:21 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 25, 2021 05:58 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 25, 2021 06:29 Inactive
@@ -198,7 +201,7 @@ public static void init() throws URISyntaxException, IOException, InterruptedExc
}

// by default use airbyte deployment governed by a test container.
if (System.getenv("USE_EXTERNAL_DEPLOYMENT") == null || !System.getenv("USE_EXTERNAL_DEPLOYMENT").equalsIgnoreCase("true")) {
if (!USE_EXTERNAL_DEPLOYMENT) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

todo: fix

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@jrhizor jrhizor temporarily deployed to more-secrets September 25, 2021 07:46 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 25, 2021 07:50 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 25, 2021 08:01 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 28, 2021 18:00 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 28, 2021 18:16 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 29, 2021 15:53 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 29, 2021 16:03 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 29, 2021 16:06 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 29, 2021 16:15 Inactive
Copy link
Contributor

@cgardens cgardens left a comment

Choose a reason for hiding this comment

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

looks good! I think this iteration makes some nice improvements.

there were a couple questions and naming things i pointed out that i think didn't get covered. it is fine if the answer is that you disagree and are going to skip them. just want to make sure they were seen.

@jrhizor jrhizor temporarily deployed to more-secrets September 29, 2021 16:51 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 29, 2021 17:14 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 29, 2021 17:18 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 29, 2021 18:07 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets September 29, 2021 18:16 Inactive
@jrhizor jrhizor merged commit f88b831 into master Sep 29, 2021
@jrhizor jrhizor deleted the jrhizor/secrets-in-configrepo branch September 29, 2021 18:53
@jrhizor jrhizor temporarily deployed to more-secrets September 29, 2021 18:54 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/worker Related to worker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants