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

feat: use secrets instead of env vars for DSNs #1483

Merged
merged 3 commits into from
May 14, 2024
Merged

Conversation

deniseli
Copy link
Contributor

@deniseli deniseli commented May 14, 2024

Fixes #1427

database/ftl-project.toml was written using:

ftl secret set --inline database.FTL_DSN_DATABASE_TESTDB --config integration/testdata/go/database/ftl-project.toml

This PR only works thanks to this earlier PR: #1472

@deniseli deniseli requested a review from alecthomas as a code owner May 14, 2024 23:23
@deniseli deniseli requested review from a team and matt2e and removed request for a team May 14, 2024 23:23
@alecthomas alecthomas mentioned this pull request May 14, 2024
deniseli added 3 commits May 14, 2024 19:25
rest of the integration tests

unrm test

unit tests

progress

progress

progress

progress

yayyyy

yayy

cleanup
@deniseli deniseli force-pushed the dli/letstrythisagain branch from fd79ddc to 92a20c8 Compare May 14, 2024 23:26
@deniseli deniseli merged commit c3c7077 into main May 14, 2024
10 checks passed
@deniseli deniseli deleted the dli/letstrythisagain branch May 14, 2024 23:29
Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Awesome.

Please also roll this out to the PFI repo ASAP. You'll need to also change the Justfile as it is currently used to manage the DSN environment variables.

tmpDir := t.TempDir()

cwd, err := os.Getwd()
assert.NoError(t, err)

rootDir := internal.GitRoot("")

if ftlConfigPath != "" {
t.Setenv("FTL_CONFIG", filepath.Join(tmpDir, ftlConfigPath))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's use a path into the testdata directory instead of one relative to the tmpDir. Otherwise we have a chicken and egg situation where the config can't be loaded until the module is copied over, and the config itself is used by FTL during startup.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also add a comment to run describing the above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good point, done on both counts


// DatabasesFromSecrets finds DSNs in environment variables and creates a map of databases.
//
// Environment variables should be in the format FTL_POSTGRES_DSN__<MODULENAME>_<DBNAME>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is no longer accurate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, thanks for catching that

// FTL_DSN_<MODULE>_<DBNAME>
parts := strings.Split(sName, "_")
if len(parts) != 4 {
return nil, fmt.Errorf("invalid DSN secret key %q should have format FTL_DSN_MODULE_DBNAME", sName)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Use FTL_DSN_<MODULE>_<DBNAME> in the error so it's clear what parts are variable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea - done

wesbillman pushed a commit that referenced this pull request May 16, 2024
@matt2e matt2e added the approved Marks an already closed PR as approved label May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Marks an already closed PR as approved
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use FTL secrets for DSNs as an intermediate step
3 participants