etcdctl: add etcdctl snapshot pipe command

[Ais8Ooz8]

To improve the security of etcdctl. Added the ability to write snapshots to stdout without writing data to disk. Useful for read-only file systems.

Solves #16242

[jmhbnz]

Thanks for putting this proposal forward @Ais8Ooz8.

My initial feeling is that there is now unnecessary duplication here. Just as an idea we could maybe mostly refactor the SaveWithVersion and PipeWithVersion functions into a single new SaveSnapshotWithVersion function that accepts an additional io writer / file parameter?

This way, we could reuse most of the logic while providing flexibility in saving the snapshot to different destinations, such as an actual file or os.Stdout.

The implementations for Save vs Pipe would then become something roughly like this perhaps:

func SaveWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config, dbPath string) (string, error) {
	file, err := os.OpenFile(dbPath+".part", os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fileutil.PrivateFileMode)
	if err != nil {
		return "", fmt.Errorf("could not open %s (%v)", dbPath+".part", err)
	}
	defer os.RemoveAll(dbPath + ".part")
	defer file.Close()

	return SaveSnapshotWithVersion(ctx, lg, cfg, file)
}

func PipeWithVersion(ctx context.Context, lg *zap.Logger, cfg clientv3.Config) (string, error) {
	return SaveSnapshotWithVersion(ctx, lg, cfg, os.Stdout)
}

Just an idea and not tested at all so feel free to hold off until more people review.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 21 days if no further activity occurs. Thank you for your contributions.

[siyuanfoundation]

This way, we could reuse most of the logic while providing flexibility in saving the snapshot to different destinations, such as an actual file or os.Stdout.

I agree with James. Much of the logic can be refactored by passing a file writer into SaveWithVersion.

@Ais8Ooz8 can you followup on this? Thanks!

[jmhbnz]

/ok-to-test

[siyuanfoundation]

/ok-to-test

[siyuanfoundation]

@Ais8Ooz8 can you fix the fmt error? It is a nitty one about an empty line. Thanks!

[Ais8Ooz8]

/retest

[siyuanfoundation]

/lgtm

[jmhbnz]

Overall looks good to me - Thanks @Ais8Ooz8. A couple small suggestions below if you have time.

Additionally - Given this is user facing new feature I would suggest we add an entry to https://github.com/etcd-io/etcd/blob/main/CHANGELOG/CHANGELOG-3.6.md#etcdctl-v3.

[jmhbnz]

Discussed during sig-etcd triage - this is looking good, @ahrtr could you please take a look for final review?

[ahrtr]

Is there a real use case for the etcdctl snapshot pipe command? @Ais8Ooz8

[serathius]

QQ, isn't the UNIX convention to use - as a file name to mean writing to pipe? At least tar does it.
So that would be etcdctl snapshot safe -.

High level, I don't think we need a separate command, it can be just a flag.

[Ais8Ooz8]

/retest

[Ais8Ooz8]

@ahrtr Since most encryption and compression utilities work with standard streams, and s3 utilities have subcommands such as aws s3 cp - or mc pipe, it seems like a real use case because it can be used in a single pipeline.

@serathius I think tar uses - along as --files-from=- for stdin and --to-stdout for stdout. In general, I agree that we can use a flag instead of a subcommand. Let's discuss which solution would be the most elegant.

[Ais8Ooz8]

/retest

[ahrtr]

LGTM

Thanks @Ais8Ooz8 for your first contribution!

[ahrtr]

Since @serathius has a comment on adding a flag instead of a new command, so leave to @serathius to take a second look.

Either a flag or a new command works for me. A new command is slight clearer, but not a big deal in this case.

[k8s-ci-robot]

PR needs rebase.

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-sigs/prow repository.