Add a firestore key migration to Teleport 17 #46472

tigrato · 2024-09-11T11:46:14Z

Throughout the lifespan of the Firestore backend, the type of the key parameter has evolved from string, to []byte, to backend.Key, and eventually back to []byte. With each new type, documents were added without migrating the existing ones. As a result, every time a new key type was introduced, an additional step was required during key iteration because Firestore maps each of these key types to a distinct database type.

backend.Key is mapped to an Array in Firestore.
[]byte is mapped to Bytes in Firestore.
string is mapped to a String in Firestore.

When searching for a specific key, the Go type is converted into a proto type and encoded in a particular way. During the database's range query, all keys with a different type than the requested one are ignored. This allows us to retrieve only the relevant keys and convert them to []byte by updating the database.

Throughout the lifespan of the Firestore backend, the type of the key parameter has evolved from `string`, to `[]byte`, to `backend.Key`, and eventually back to `[]byte`. With each new type, documents were added without migrating the existing ones. As a result, every time a new key type was introduced, an additional step was required during key iteration because Firestore maps each of these key types to a distinct database type. - `backend.Key` is mapped to an Array in Firestore. - `[]byte` is mapped to Bytes in Firestore. - `string` is mapped to a String in Firestore. When searching for a specific key, the Go type is converted into a proto type and encoded in a particular way. During the database's range query, all keys with a different type than the requested one are ignored. This allows us to retrieve only the relevant keys and convert them to `[]byte` by updating the database. Signed-off-by: Tiago Silva <tiago.silva@goteleport.com>

lib/backend/firestore/migration.go

github-actions · 2024-09-12T09:59:37Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

rosstimothy · 2024-09-12T19:10:35Z

lib/auth/migration/0002_firestore.go

+type migrateFirestoreKeys struct {
+}
+
+func (d migrateFirestoreKeys) Version() int64 {
+	return 2
+}
+
+func (d migrateFirestoreKeys) Name() string {
+	return "migrate_firestore_keys"
+}
+
+// Up scans the backend for keys that are stored as strings or backend.Key types
+// and converts them to the correct type (bytes).
+func (d migrateFirestoreKeys) Up(ctx context.Context, b backend.Backend) error {
+	ctx, span := tracer.Start(ctx, "migrateFirestoreKeys/Up")
+	defer span.End()
+
+	// if the backend is not firestore, skip this migration
+	if b.GetName() != firestore.GetName() {
+		return nil
+	}
+
+	// migrate firestore keys
+	return trace.Wrap(firestore.MigrateIncorrectKeyTypes(ctx, b))
+}
+
+// Down is a no-op for this migration.
+func (d migrateFirestoreKeys) Down(ctx context.Context, _ backend.Backend) error {
+	_, span := tracer.Start(ctx, "migrateFirestoreKeys/Down")
+	defer span.End()
+	return nil
+}


I don't know that a one-shot migration will work. If we assume that a new Firestore cluster was created on v16.2.0, the release the introduced the latest broken key, and stays on that version until upgrading to v17, then there is a chance the upgrade isn't performed according to our guidelines and an auth at 16.2.0 and 17 are running simultaneously. In that case, the migration may complete successfully, however, it may also be immediately undone by the old auth instance.

If we are to proceed with a one-shot migration I don't know that we can safely do it until v18. I think the safest migration strategy might be to have the firestore backend always spin up a background goroutine that performs MigrateIncorrectKeyTypes. In that case though, I think we need to rate limit the migration in order to prevent having any impact on cluster reads and writes.

cc @fspmarshall

My thinking is that during a migration, the older version will simply upsert the heartbeats, which will reset once authentication restarts. However, there’s a possibility that someone might try to modify static objects during the migration process.

I don’t believe we can handle everything in a background goroutine as well, at least not without optimistic locking. There’s nothing stopping a role from being altered between the read and put operations, especially since authentication will remain fully functional. We also can’t enforce this during startup because it could take a long time, and backend creation might fail.

We’d need to restart the conversion loop whenever a conditional update issue occurs if we use bulk writing.

Given this, I think we should postpone it to v18 and treat it as part of the migration but this is becoming terrible as read range operations take tons of time.

What are your thoughts?

The older version of Auth may still be processing and handling traffic from users, which means any tctl create operation could land on the wrong Auth server during a migration.

As you mentioned, the background migration is a perpetual clean up operation. However, since in v17 all Auth servers are capable of reading the correct, legacy, and broken key types, if a v16 Auth server undoes a migration it's not the end of the world. By the time the migration was removed in v19, there would be no possibility of any new keys being stored in the wrong format.

@rosstimothy c2f93ec applied the changes. I also start the migration after a few minutes to avoid interfering with cache loads

rosstimothy · 2024-09-20T13:32:28Z

lib/backend/firestore/firestorebk.go

@@ -190,6 +190,7 @@ func newRecord(from backend.Item, clock clockwork.Clock) record {
 	return r
 }

+// TODO(tigrato|rosstimothy): Simplify this function by removing the brokenRecord and legacyRecord struct


Can you add a note indicating which version that it's safe to do this?

Added in f74a94d

fspmarshall

IMO it would be preferable to make this an infrequent periodic rather than only run once on startup in order to be absolutely certain that we eventually migrate everything even if a bugged auth server is accidentally kept live longer than intended and auths with the migration aren't restarted.

My suggestion would be to set this up to run on an infrequent jittered interval with a short first duration. Ex:

migrationInterval := interval.New(interval.Config{
    Duration:      time.Hour * 12,
    FirstDuration: utils.FullJitter(time.Minute *5),
    Jitter:        retryutils.NewSeventhJitter(),
})
defer migrationInterval.Stop()
for {
    select {
    case <-migrationInterval.Next():
        doMigration()
    case <-ctx.Done():
        return
    }
}

fspmarshall · 2024-09-20T16:03:54Z

lib/backend/firestore/firestorebk.go

@@ -413,6 +414,10 @@ func New(ctx context.Context, params backend.Params, options Options) (*Backend,
 		go RetryingAsyncFunctionRunner(b.clientContext, linearConfig, b.Logger, b.purgeExpiredDocuments, "purgeExpiredDocuments")
 	}

+	// Migrate incorrect key types to the correct type.
+	// Start the migration after a delay to allow the backend to start up and won't be affected by the migration.
+	_ = b.clock.AfterFunc(5*time.Minute, b.migrateIncorrectKeyTypes)


Probably worth using a FullJitter on this delay to ensure that it still eventually gets run even if auth is being frequently restarted. That will mean that some small portion of the time it'll interfere a bit with startup, but IMO it's worth it in order to be confident that it'll eventually get run in the context of frequent restarts.

Switched to interval.

lib/backend/firestore/migration.go

rosstimothy

Thanks Tiago!

tigrato requested a review from rosstimothy September 11, 2024 11:46

tigrato force-pushed the tigrato/firestore-migration branch from b7943fe to 030072c Compare September 11, 2024 12:14

rosstimothy reviewed Sep 11, 2024

View reviewed changes

lib/backend/firestore/migration.go Outdated Show resolved Hide resolved

force key to be array

a4e45a9

tigrato marked this pull request as ready for review September 12, 2024 09:59

github-actions bot added the size/md label Sep 12, 2024

github-actions bot requested review from camscale and smallinsky September 12, 2024 09:59

tigrato added the no-changelog Indicates that a PR does not require a changelog entry label Sep 12, 2024

rosstimothy reviewed Sep 12, 2024

View reviewed changes

rosstimothy requested a review from fspmarshall September 12, 2024 19:11

add rate limited bucket

ac792a4

tigrato force-pushed the tigrato/firestore-migration branch from 75bc4bc to ac792a4 Compare September 13, 2024 12:37

tigrato added 2 commits September 13, 2024 13:42

reduce limit

29d14b7

add migration in its own goroutine and use optimistic locking

c2f93ec

rosstimothy reviewed Sep 20, 2024

View reviewed changes

add deprecation release

f74a94d

fspmarshall reviewed Sep 20, 2024

View reviewed changes

retry migration every 12h (max)

3fadc99

fspmarshall approved these changes Sep 24, 2024

View reviewed changes

rosstimothy approved these changes Sep 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a firestore key migration to Teleport 17 #46472

Add a firestore key migration to Teleport 17 #46472

tigrato commented Sep 11, 2024

github-actions bot commented Sep 12, 2024

rosstimothy Sep 12, 2024 •

edited

Loading

rosstimothy Sep 12, 2024

tigrato Sep 13, 2024

rosstimothy Sep 18, 2024

tigrato Sep 18, 2024

rosstimothy Sep 20, 2024

tigrato Sep 20, 2024

fspmarshall left a comment

fspmarshall Sep 20, 2024

tigrato Sep 24, 2024

rosstimothy left a comment

Add a firestore key migration to Teleport 17 #46472

Are you sure you want to change the base?

Add a firestore key migration to Teleport 17 #46472

Conversation

tigrato commented Sep 11, 2024

github-actions bot commented Sep 12, 2024

rosstimothy Sep 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fspmarshall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rosstimothy left a comment

Choose a reason for hiding this comment

rosstimothy Sep 12, 2024 •

edited

Loading