Skip to content

Commit

Permalink
import: check permissions earlier
Browse files Browse the repository at this point in the history
Release note (enterprise change): Import now checks user permissions earlier on all files, to fail sooner if permissions are invalid.
  • Loading branch information
benbardin committed Jan 14, 2022
1 parent ac5f637 commit de4241c
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 21 deletions.
9 changes: 3 additions & 6 deletions pkg/ccl/importccl/import_stmt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7041,15 +7041,12 @@ func TestImportRowErrorLargeRows(t *testing.T) {
if r.Method != "GET" {
return
}
_, err := w.Write([]byte("firstrowvalue\nsecondrow,is,notok,"))
require.NoError(t, err)
_, _ = w.Write([]byte("firstrowvalue\nsecondrow,is,notok,"))
// Write 8MB field as the last field of the second
// row.
bigData := randutil.RandBytes(rng, 8<<20)
_, err = w.Write(bigData)
require.NoError(t, err)
_, err = w.Write([]byte("\n"))
require.NoError(t, err)
_, _ = w.Write(bigData)
_, _ = w.Write([]byte("\n"))
}))
defer srv.Close()
tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{})
Expand Down
60 changes: 45 additions & 15 deletions pkg/ccl/importccl/read_import_base.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,24 +153,54 @@ func readInputFiles(

fileSizes := make(map[int32]int64, len(dataFiles))

// Attempt to fetch total number of bytes for all files.
// "Pre-import" work.
// Validate permissions early, and attempt to fetch total number of bytes for all files to track progress.
for id, dataFile := range dataFiles {
conf, err := cloud.ExternalStorageConfFromURI(dataFile, user)
if err != nil {
return err
}
es, err := makeExternalStorage(ctx, conf)
if err != nil {
if err := func() error {
conf, err := cloud.ExternalStorageConfFromURI(dataFile, user)
if err != nil {
return err
}
es, err := makeExternalStorage(ctx, conf)
if err != nil {
return err
}
defer es.Close()

sz, err := es.Size(ctx, "")

if sz <= 0 {
// Don't log dataFile here because it could leak auth information.
log.Infof(ctx, "could not fetch file size; falling back to per-file progress: %v", err)
return nil
}
fileSizes[id] = sz

if len(dataFiles) <= 1 {
// If there's more than one file, try to read a byte from each to verify permissions.
// If there's only one file, skip that check because it provides no advantage.
return nil
}

raw, err := es.ReadFile(ctx, "")
if err != nil {
return err
}
defer raw.Close()

p := make([]byte, 1)
if _, err := raw.Read(p); err != nil && err != io.EOF {
// Check that we can read the file, for the sake of permissions.
// We don't care about content yet, so we read a single byte and we don't process it in any way.
// If the file is empty -- and we can tell that -- that also counts as readable, so don't error.
return err
}

return nil
}(); err != nil {
return err
}
sz, err := es.Size(ctx, "")
es.Close()
if sz <= 0 {
// Don't log dataFile here because it could leak auth information.
log.Infof(ctx, "could not fetch file size; falling back to per-file progress: %v", err)
break
}
fileSizes[id] = sz

}

for dataFileIndex, dataFile := range dataFiles {
Expand Down

0 comments on commit de4241c

Please sign in to comment.