cli: support more user friendly userfile upload semantics by adityamaru · Pull Request #51353 · cockroachdb/cockroach

adityamaru · 2020-07-12T19:23:34Z

Previously, the user would have to specify both the source and
detination args when using userfile upload. This change tweaks the
semantics to be as follows:

Users must specify a source path.
If the user does not specify a destination URI/path, we use the
default URI scheme and host, and the basename from the source arg as
the path.
If the destination is a well-formed userfile URI of the form
userfile://db.schema.tablename_prefix/path/to/file, then we use that
as the final URI.
If destination is not a well-formed userfile URI, we use the default
userfile URI schema and host, and the destination as the path.

File paths are never cleaned during upload or interaction with the
UserFileStorage. Thus, the filename is the exact string represented by
the path of the constructed userfile URI. This is in keeping with the
semantics of s3 and gcsutil. To prevent user surprises we still reject
constructs where the URI post normalization != URI pre normalization.

egs: test/./test.csv or test/../../test.csv

Informs: #51222

Release note (cli change): Improves the user semantics for userfile upload by supporting different patterns of specifying the source and
destination CLI args. The source arg is required, while the destination
arg is now optional.

cockroach-teamcity · 2020-07-12T19:23:42Z

This change is

adityamaru · 2020-07-12T19:24:43Z

Commit depends on #50981, I will rebase once merged.

miretskiy

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @dt, and @miretskiy)

pkg/cli/userfiletable_test.go, line 41 at r3 (raw file):

	// User passes no destination so we use the basename of the source as the
	// filename.
	c.Run(fmt.Sprintf("userfile upload %s", file))

any way to check if the upload path is what you expect it to be?

dt · 2020-07-14T00:09:12Z

pkg/cli/flags.go

 	clientCmds = append(clientCmds, nodeLocalCmds...)
+	clientCmds = append(clientCmds, userFileCmds...)
 	clientCmds = append(clientCmds, stmtDiagCmds...)
+	clientCmds = append(clientCmds, userFileCmds...)


i think you somehow doubled this line in a rebase since you also have an extra commit

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt and @miretskiy)

pkg/cli/flags.go, line 544 at r3 (raw file):

Previously, dt (David Taylor) wrote…

i think you somehow doubled this line in a rebase since you also have an extra commit

Done.

pkg/cli/userfiletable_test.go, line 41 at r3 (raw file):

Previously, miretskiy (Yevgeniy Miretskiy) wrote…

any way to check if the upload path is what you expect it to be?

I'm not sure I understand, the comments at the end of this test are parsed as the expected outputs.

knz

@aaron-crl I'd like to put this in your radar as well. Ideally, you'd come up with some guidelines / evaluation criteria to help Aditya assert on his own whether the path handling is sane.

Reviewed 3 of 3 files at r3, 1 of 1 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @dt, and @miretskiy)

pkg/cli/userfile.go, line 91 at r4 (raw file):

		userFileURL := url.URL{
			Scheme: defaultUserfileScheme,
			Host:   defaultQualifiedNamePrefix + user,

see my comment about the username in the other PR.

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt, @knz, and @miretskiy)

pkg/cli/userfile.go, line 91 at r4 (raw file):

Previously, knz (kena) wrote…

see my comment about the username in the other PR.

addressed in the previous PR by unescaping the URI before we use it. This allows for arbitrary Unicode characters.

adityamaru · 2020-07-16T18:01:43Z

RFAL, please

knz

Reviewed 8 of 8 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @dt, @knz, and @miretskiy)

pkg/storage/cloudimpl/file_table_storage.go, line 118 at r5 (raw file):

	if path.Clean(basename) != basename {
		return "", errors.Newf("basename %s changes to %s on normalization. "+

I'm OK with this test but I'd like a unit test that demonstrates the error: under which circumstances would client code encounter this?

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt, @knz, and @miretskiy)

pkg/storage/cloudimpl/file_table_storage.go, line 118 at r5 (raw file):

Previously, knz (kena) wrote…

I'm OK with this test but I'd like a unit test that demonstrates the error: under which circumstances would client code encounter this?

CLI related code should not encounter this because all the FileTableStorage methods are passed an empty basename, as a result of which all operations occur on the prefix.
BACKUP uses the basename when writing data, manifest files, and encryption information. I have added a specialized test to file_table_storage_test.go to ensure we're rejecting what we think we are. external_storage_test.go has tests in testExportStore which test valid basename constructs.

knz

Reviewed 2 of 2 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @dt, @knz, and @miretskiy)

pkg/cli/userfiletable_test.go, line 159 at r6 (raw file):

	}

	require.NoError(t, os.RemoveAll(dir))

Can you double-check what the cleanFn returned by TempDir does? It seems to me that you do not need this RemoveAll here.

pkg/storage/cloudimpl/file_table_storage.go, line 118 at r5 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

Client code should not encounter this because all the FileTableStorage methods are passed an empty basename, as a result of which all operations occur on the prefix.
BACKUP uses the basename when writing data, manifest files, and encryption information. I have added a specialized test to file_table_storage_test.go to ensure we're rejecting what we think we are. external_storage_test.go has tests in testExportStore which test valid basename constructs.

Yes that works thanks

adityamaru

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt, @knz, and @miretskiy)

pkg/cli/userfiletable_test.go, line 159 at r6 (raw file):

Previously, knz (kena) wrote…

Can you double-check what the cleanFn returned by TempDir does? It seems to me that you do not need this RemoveAll here.

Done.

knz

Thanks. You'll want your final PR approval from someone in your team.

Reviewed 1 of 1 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @dt, @knz, and @miretskiy)

miretskiy

Reviewed 1 of 8 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @adityamaru, @dt, and @knz)

pkg/cli/userfiletable_test.go, line 131 at r7 (raw file):

		if err != nil {
			t.Fatal(err)
		}

require.NoError(t, err) here and throughout?

pkg/storage/cloudimpl/cloudimpltests/file_table_storage_test.go, line 64 at r7 (raw file):

		require.True(t, testutils.IsError(store.WriteFile(ctx, testfile, bytes.NewReader([]byte{0})),
			"basename listing-test/../basepath changes to basepath on normalization. "+
				"userfile does not permit such constructs."))

you don't have to specify full error message. Just substring (does not permit such).
Also, I would pull out write file call and pass in err to require.True so that if the test fails, you'll see what kind of error you got (as opposed to "expected true")
err := store.WriteFile()
require.True(t, testutils.IsError(err, "changes to basepath", err))

Previously, the user would have to specify both the source and detination args when using `userfile upload`. This change tweaks the semantics to be as follows: - Users must specify a source path. - If the user does not specify a destination URI/path, we use the default URI scheme and host, and the basename from the source arg as the path. - If the destination is a well-formed userfile URI of the form userfile://db.schema.tablename_prefix/path/to/file, then we use that as the final URI. - If destination is not a well-formed userfile URI, we use the default userfile URI schema and host, and the destination as the path. File paths are never cleaned during upload or interaction with the UserFileStorage. Thus, the filename is the exact string represented by the path of the constructed userfile URI. This is in keeping with the semantics of s3 and gcsutil. To prevent user surprises we still reject constructs where the URI post normalization != URI pre normalization. egs: test/./test.csv or test/../../test.csv Release note (cli change): Improves the user semantics for `userfile upload` by supporting different patterns of specifying the source and destination CLI args. The source arg is required, while the destination arg is now optional.

adityamaru · 2020-07-22T15:42:31Z

TFTRs!

bors r+

craig · 2020-07-22T16:48:23Z

Build succeeded

GitHub CI (Cockroach)

adityamaru requested review from dt and miretskiy July 12, 2020 19:23

adityamaru requested a review from a team as a code owner July 12, 2020 19:23

adityamaru force-pushed the add-default-uribase branch from de81a03 to 925ff78 Compare July 13, 2020 13:45

miretskiy suggested changes Jul 13, 2020

View reviewed changes

miretskiy self-requested a review July 13, 2020 13:48

dt reviewed Jul 14, 2020

View reviewed changes

adityamaru force-pushed the add-default-uribase branch from 925ff78 to 0b262cb Compare July 14, 2020 02:38

adityamaru commented Jul 14, 2020

View reviewed changes

knz reviewed Jul 14, 2020

View reviewed changes

adityamaru force-pushed the add-default-uribase branch 2 times, most recently from 35c3d9d to 1990865 Compare July 16, 2020 17:57

adityamaru commented Jul 16, 2020

View reviewed changes

knz reviewed Jul 17, 2020

View reviewed changes

adityamaru force-pushed the add-default-uribase branch from 1990865 to def15e3 Compare July 17, 2020 13:46

adityamaru commented Jul 17, 2020

View reviewed changes

adityamaru force-pushed the add-default-uribase branch 2 times, most recently from 64cd0bf to 0e90fe8 Compare July 17, 2020 15:12

adityamaru requested a review from knz July 20, 2020 18:05

knz reviewed Jul 21, 2020

View reviewed changes

adityamaru force-pushed the add-default-uribase branch from 0e90fe8 to ea15683 Compare July 21, 2020 12:59

adityamaru commented Jul 21, 2020

View reviewed changes

adityamaru requested a review from knz July 21, 2020 13:00

knz reviewed Jul 22, 2020

View reviewed changes

miretskiy requested review from dt and knz July 22, 2020 13:10

miretskiy approved these changes Jul 22, 2020

View reviewed changes

adityamaru force-pushed the add-default-uribase branch from ea15683 to aeab5d3 Compare July 22, 2020 14:42

craig bot merged commit 89dda79 into cockroachdb:master Jul 22, 2020

lnhsingh mentioned this pull request Aug 26, 2020

cli: support more user friendly userfile upload semantics cockroachdb/docs#8198

Closed

Conversation

adityamaru commented Jul 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Jul 12, 2020

Uh oh!

adityamaru commented Jul 12, 2020

Uh oh!

miretskiy left a comment

Choose a reason for hiding this comment

Uh oh!

dt Jul 14, 2020

Choose a reason for hiding this comment

Uh oh!

adityamaru left a comment

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Jul 16, 2020

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru left a comment

Choose a reason for hiding this comment

Uh oh!

knz left a comment

Choose a reason for hiding this comment

Uh oh!

miretskiy left a comment

Choose a reason for hiding this comment

Uh oh!

adityamaru commented Jul 22, 2020

Uh oh!

craig bot commented Jul 22, 2020

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adityamaru commented Jul 12, 2020 •

edited

Loading

adityamaru left a comment •

edited

Loading