Preserve custom etcd prefix compatibility for etcd3

[liggitt]

Fixes #42505

restored normalization of custom `--etcd-prefix` when `--storage-backend` is set to etcd3

[k8s-reviewable]

This change is 

[timothysc]

iirc @hongchaodeng made that change.

[hongchaodeng]

@timothysc Feel free to assign to you

[hongchaodeng]

/lgtm

[enj]

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

[hongchaodeng]

/foo//bar

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

[enj]

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

[timothysc]

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

[liggitt]

rebased and added unit tests

[liggitt]

#39975 flake
@k8s-bot bazel test this

[smarterclayton]

/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

[k8s-ci-robot]

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

In response to this comment:

/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

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[smarterclayton]

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)?

[hongchaodeng]

/lgtm

[k8s-github-robot]

[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-github-robot]

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

[liggitt]

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.

[smarterclayton]

Ok, thanks lgtm as well.

[liggitt]

opened cherry-pick for 1.5.x

[k8s-ci-robot]

@liggitt: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins GCE e2e	6853e4d	link	@k8s-bot cvm gce e2e test this

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.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

[k8s-github-robot]

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

[enj]

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)
}

[liggitt]

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