The Wayback Machine - https://web.archive.org/web/20220223042211/https://github.com/etcd-io/etcd/issues/13705
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

More warnings around "etcdctl del" #13705

Open
mac-chaffee opened this issue Feb 17, 2022 · 3 comments · May be fixed by #13729
Open

More warnings around "etcdctl del" #13705

mac-chaffee opened this issue Feb 17, 2022 · 3 comments · May be fixed by #13729

Comments

@mac-chaffee
Copy link

@mac-chaffee mac-chaffee commented Feb 17, 2022

What happened?

Not really a bug, but maybe unexpected behavior. Since etcdctl del accepts an optional second parameter which deletes all keys in a range, these are very easy mistakes to make:

# Oh no, there was a space in the key I copied!
etcdctl del /registry/persistentvolumes/pvc-eef4ec4b-326d-47e6- f11c-6474a5fd4d89
# <deletes thousands of keys>
etcdctl put 'key space' somevalue
# Oh no! I forgot to quote my key that intentionally contains a space!
etcdctl del key space
# <deletes thousands of keys>
# Oh no! I forgot to add -- to my flag!
etcdctl del a-key cacert=/some/file
# <deletes hundreds of keys>

What did you expect to happen?

Of course we could just tell people to pay super close attention when they run etcdctl del, but there's lots of advice scattered around the internet for fixing broken Kubernetes clusters that involves etcdctl del. Here's one example: kubernetes/kubernetes#90585 (comment)

Here's more: https://www.google.com/search?q=etcdctl+del+site%3Agithub.com%2Fkubernetes

As a result, I think having that optional parameter to delete a range of keys might constitute a footgun. Perhaps we should require an extra flag like --range or something so that there's never an implicit deletion of more than 1 key. Other options include placing a warning/confirmation if the command would result in more than one key being deleted (may be performance-intensive).

How can we reproduce it (as minimally and precisely as possible)?

docker run -d --name etcd quay.io/coreos/etcd:v3.5.0
docker exec -it etcd sh
<run etcdctl put and del commands from example>

Anything else we need to know?

No response

Etcd version (please run commands below)

v3.5.0

Etcd configuration (command line flags or environment variables)

n/a

Etcd debug information (please run commands blow, feel free to obfuscate the IP address or FQDN in the output)

n/a

Relevant log output

No response

@ptabor
Copy link
Contributor

@ptabor ptabor commented Feb 21, 2022

I think it's a valid concern. If not an issue with backward compatibility, it would be a definitely right thing to fix.

  • In 3.5 we could add a warning on stderr.
  • In 3.6 we could add support for --range and sleep (2s) if the --range is not given:
    a) let the operation be cancelled by human operator
    b) trigger attention of the script owners to start using the new flag.
@kkkkun
Copy link
Contributor

@kkkkun kkkkun commented Feb 22, 2022

Let me have a try.
/assign

@kkkkun kkkkun linked a pull request that will close this issue Feb 22, 2022
@ahrtr
Copy link
Collaborator

@ahrtr ahrtr commented Feb 23, 2022

I think it's a valid concern. If not an issue with backward compatibility, it would be a definitely right thing to fix.

* In 3.5 we could add a warning on stderr.

* In 3.6 we could add support for `--range` and sleep (2s) if the `--range` is not given:
  a) let the operation be cancelled by human operator
  b) trigger attention of the script owners to start using the new flag.

I would propose to deprecate the existing range delete operation, and add a new flag "--to-key" to explicitly state that it's a range delete operation (if both --from-key and --to-key are set), otherwise raise an error. Since it's not backward compatible, so we can raise a warning for now and sleep 3 seconds so that users have a change to cancel it. cc @ptabor @serathius @spzala WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment