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.

Sign up for GitHub

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

Jump to bottom

feat: cache asm secrets #1832

Merged
merged 14 commits into from
Jun 25, 2024
Merged

feat: cache asm secrets #1832

merged 14 commits into from
Jun 25, 2024

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Jun 19, 2024 •
edited
Loading

closes #1782
ASM changes:

  • ASM uses leader package to coordinate between leader and followers
  • The leader syncs secrets from ASM
    • Only requests secrets that have been updated since last sync
  • Followers always uses admin service to get the secrets from the leader
  • Both leader and follower cache secrets using secretsCache

Other changes:

  • Remove additional db connection pools created at start up. Now a single db pool is created along with a dal at start up, which is then passed into the controller service and anything else that needs it

@ftl-robot ftl-robot mentioned this pull request Jun 19, 2024
Open
@alecthomas
Copy link
Collaborator

Wasn't this going to be two separate PRs?

@alecthomas
Copy link
Collaborator

Can you split it up please?

@matt2e matt2e force-pushed the matt2e/secrets-cache branch 4 times, most recently from b4bb735 to 1133952 Compare June 20, 2024 01:17
@matt2e matt2e marked this pull request as ready for review June 20, 2024 01:51
@matt2e matt2e requested a review from alecthomas as a code owner June 20, 2024 01:51
@matt2e matt2e requested review from a team and worstell and removed request for a team June 20, 2024 01:51
@matt2e
Copy link
Collaborator Author

matt2e commented Jun 20, 2024

I'll add some more tests to this PR too

@matt2e matt2e force-pushed the matt2e/secrets-cache branch from 5630f53 to 70ad963 Compare June 20, 2024 01:57
@matt2e matt2e requested review from gak and removed request for worstell June 20, 2024 02:04
gak
gak reviewed Jun 20, 2024
View reviewed changes
common/configuration/asm_follower.go Outdated
}
entries := []Entry{}
for _, s := range resp.Msg.Secrets {
components := strings.Split(s.RefPath, ".")
Copy link
Contributor

Choose a reason for hiding this comment

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

I've seen this pattern a few times, maybe a fn (rp *) ToRef (Ref, error) {} ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll make a helper func for this 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ParseRef already exists 🤦‍♂️

@matt2e
Copy link
Collaborator Author

matt2e commented Jun 20, 2024

I'll add some more tests to this PR too

added integration test for syncing secrets

@matt2e matt2e requested a review from gak June 20, 2024 05:15
@matt2e matt2e force-pushed the matt2e/secrets-cache branch from 815b466 to 9b0726e Compare June 20, 2024 05:35
alecthomas
alecthomas approved these changes Jun 20, 2024
View reviewed changes
cmd/ftl-controller/main.go Outdated Show resolved Hide resolved
common/configuration/asm_leader.go Outdated
Comment on lines 72 to 74
go func() {
l.watchForUpdates(ctx, clock)
}()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
go func() {
l.watchForUpdates(ctx, clock)
}()
go l.watchForUpdates(ctx, clock)

common/configuration/asm_leader.go Outdated

err := l.sync(ctx)
if err == nil {
backOff = syncInitialBackoff
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should log this error, even if at debug.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This case is for when there is no error and is resetting the backoff. The error is logged below

common/configuration/asm_leader.go Outdated Show resolved Hide resolved
common/configuration/asm_leader.go Outdated

// sync retrieves all secrets from ASM and updates the cache
func (l *asmLeader) sync(ctx context.Context) error {
previous := map[Ref]asmSecretValue{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see a cache for the followers?

common/configuration/asm_leader.go Outdated
client *secretsmanager.Client

// indicates if the initial sync with ASM has finished
loaded atomic.Value[bool]
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be more idiomatic to use a channel here, closing it once the value is available. This makes it simpler to do timeouts too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh nice

@matt2e matt2e force-pushed the matt2e/secrets-cache branch 2 times, most recently from 4253681 to a58886d Compare June 21, 2024 04:05
@matt2e
Copy link
Collaborator Author

matt2e commented Jun 21, 2024

Updated:

  • added secretsCache which asmLeader and asmFollower use
  • made the integration test where set secrets via ftl and ASM directly handle both cases:
    • setting/getting via leader which calls ASM
    • setting/getting via a follower, which calls a leader, which calls ASM

@matt2e matt2e force-pushed the matt2e/secrets-cache branch from 3c767ad to 2374dae Compare June 21, 2024 06:43
@matt2e matt2e force-pushed the matt2e/secrets-cache branch from 4f6c5c2 to 775ae73 Compare June 23, 2024 23:22
@matt2e matt2e merged commit c7d272c into main Jun 25, 2024
43 checks passed
@matt2e matt2e deleted the matt2e/secrets-cache branch June 25, 2024 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alecthomas alecthomas alecthomas approved these changes

@gak gak gak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add caching SecretsResolver/SecretsProvider
3 participants
@matt2e @alecthomas @gak