This change is 

iirc @hongchaodeng made that change.

@timothysc Feel free to assign to you

/lgtm

Also need upgrade tests with custom prefixes that normalize differently (foo, /foo, /foo//bar, etc).

/foo//bar

This is already invalid path. Starting apiserver like this should reject it directly.

I do not know if we do any validation to the --etcd-prefix parameter but path.Join will normalize /foo//bar to /foo/bar.

/cc @mml @wojtek-t there is a gap in your upgrade test coverage.

rebased and added unit tests

#39975 flake
@k8s-bot bazel test this

/approve

This is a very serious issue that could cause data loss for an upgrading cluster if the administrator had custom prefixes and wasn't paying attention.

/lgtm

@smarterclayton: you can't LGTM a PR unless you are an assignee.

Actually, quick question. What if an admin had previously configured a prefix of /foo//bar? We use path.join elsewhere when building keys? Could an admin have // in practice (I believe etcd normalizes this via the golang server library, just want to be sure)?

/lgtm

[APPROVALNOTIFIER] This PR is APPROVED

The following people have approved this PR: hongchaodeng, liggitt, smarterclayton

Needs approval from an approver in each of these OWNERS Files:

• staging/src/k8s.io/apiserver/pkg/storage/OWNERS [liggitt,smarterclayton]

We suggest the following people:
cc
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

etcd2 used path.join on the prefix and would have normalized it away (// and trailing slashes are normalized away), so they couldn't have a prefix containing // or ending in / on etcd2.

we don't use path join in the keyfuncs.

Ok, thanks lgtm as well.

opened cherry-pick for 1.5.x

@liggitt: The following test(s) failed:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Automatic merge from submit-queue (batch tested with PRs 42506, 42585, 42596, 42584)

we don't use path join in the keyfuncs.

Both of them use path.Join to merge the root prefix with the keyFunc's output. So now the etcd key is guaranteed to start with a / and be a fully normalized path.

etcd2:

func (h *etcdHelper) prefixEtcdKey(key string) string {
	if strings.HasPrefix(key, h.pathPrefix) {
		return key
	}
	return path.Join(h.pathPrefix, key)
}

etcd3:

func keyWithPrefix(prefix, key string) string {
	if strings.HasPrefix(key, prefix) {
		return key
	}
	return path.Join(prefix, key)
}

those don't exist any more (b6e3296), but yes, the etcd prefix and the key are joined that way. I was referring to the actual key functions in the registries