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

etcd: don't confuse prefixes during migration #4299

Merged
merged 4 commits into from
Sep 14, 2020
Merged

Conversation

awly
Copy link
Contributor

@awly awly commented Sep 9, 2020

The prefix fetching logic has a bug: it treats everything starting with
/teleport as the legacy prefix data, even if it's /teleport-foo/bar.
This is an issue if user specifies /teleport-foo as their custom
prefix. Each restart will copy the data from /teleport-foo/... to
/teleport-foo-foo/....

Set the legacy prefix const to /teleport/ instead. This avoids
excessive copying during startup.

Fixes #4312

@russjones
Copy link
Contributor

@awly Can we make a backup of everything under /teleport/ before we kick off the migration?

The prefix fetching logic has a bug: it treats everything starting with
`/teleport` as the legacy prefix data, even if it's `/teleport-foo/bar`.
This is an issue if user specifies `/teleport-foo` as their custom
prefix. Each restart will copy the data from `/teleport-foo/...` to
`/teleport-foo-foo/...`.

Set the legacy prefix const to `/teleport/` instead. This avoids
excessive copying during startup.

Prefixes can still be confused later on, with `Watch` and `GetRange`,
but this is harder to migrate with backwards-compatibility.
@awly awly force-pushed the andrew/etcd-prefix-handling branch from a623683 to 12de0b0 Compare September 9, 2020 20:31
@awly awly requested a review from benarent as a code owner September 9, 2020 20:31
@awly
Copy link
Contributor Author

awly commented Sep 9, 2020

@russjones we can probably backup the data we delete, under the custom prefix (not /teleport/).
I'll add that.

In case the migration kicks in by mistake, or ends up deleting more data
than expected, a backup will be very handy.
@awly
Copy link
Contributor Author

awly commented Sep 9, 2020

@russjones PTAL

When custom prefix is `/foo/`, the migration would move `/teleport/a` to
`/foo/a`, but the backend later tries to read `/foo//a`.

Also added tests to cover these edge cases.
Copy link
Contributor

@fspmarshall fspmarshall left a comment

Choose a reason for hiding this comment

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

I don't like the idea of having the logic be sensitive to the differences in proceeding/trailing slashes. Seems like an unnecessary footgun. Ideally prefix, /prefix, and /prefix/ would all mean the same thing from the user's perspective.

Otherwise, LGTM.

@awly
Copy link
Contributor Author

awly commented Sep 10, 2020

@fspmarshall yeah, I agree.
We made /prefix and prefix the same, but not with trailing slashes. Now some users have data under /prefix//foo so we'll need to be careful when fixing this. That might need another migration :(

@benarent
Copy link
Contributor

When we update the docs, I'll come back and update a setup guide for etcd, this does seem to follow the slightly confusing prefix logic of etcd as Forrest has pointed out https://etcd.io/docs/v3.3.12/op-guide/authentication/

btw, we should update #2883 to make it clear that it needs the trailing slash incase anyone else finds that ticket. ( or is this only an issue during a migration )

@fspmarshall
Copy link
Contributor

We made /prefix and prefix the same, but not with trailing slashes. Now some users have data under /prefix//foo so we'll need to be careful when fixing this. That might need another migration :(

Huh. That is tricky. Its too bad we don't have a more well-defined format for the keys used by the Backend interface. It would be nice to be able to unambiguously separate path sections.

I suppose that while /prefix//foo looks a bit silly, at least it is unambiguous 🤷

@awly
Copy link
Contributor Author

awly commented Sep 10, 2020

@benarent actually, all our backed keys start with a / so the trailing slash in prefix is not needed.
Updated the description.

Still need approval from @webvictim or @russjones for /examples/

@awly awly force-pushed the andrew/etcd-prefix-handling branch from f6e68ea to a667162 Compare September 11, 2020 16:31
@awly awly merged commit 396812c into master Sep 14, 2020
@awly awly deleted the andrew/etcd-prefix-handling branch September 14, 2020 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

etcd prefix data migration
5 participants