blobs: enforce disabled external-io-dir #45858
Conversation
|
This change is |
There was a problem hiding this comment.
LGTM but I'd like to see a test that COPY does the right thing now too.
Reviewed 2 of 2 files at r1, 4 of 4 files at r2.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ajwerner and @dt)
pkg/blobs/local_storage.go, line 58 at r2 (raw file):
func (l *LocalStorage) prependExternalIODir(path string) (string, error) { if l == nil { return "", errors.Errorf("local file access is disabled")
might be worth defining the error object as a global var so as to enable errors.Is() elsewhere.
pkg/sql/copy_file_upload.go, line 146 at r1 (raw file):
} // Issue a final zero-byte write to ensure we observe any errors in the pipe.
This feels like it could use a test.
There was a problem hiding this comment.
I would also recommend a Release note (security update): ... which would outline the overall security rules for nodelocal/COPY. That would ensure it lands in the right place in docs.
Reviewable status:
There was a problem hiding this comment.
Reviewable status:
pkg/sql/copy_file_upload.go, line 146 at r1 (raw file):
Previously, knz (kena) wrote…
This feels like it could use a test.
that's what the empty file case is above:
cockroach/pkg/cli/nodelocal_test.go
Line 62 in b0f4571
|
Oh I see I had missed that |
dt
force-pushed
the
disable-nodelocal
branch
2 times, most recently
from
6de9f19
to
9d67c71
Compare
The row processing function in the COPY machine is where we actually check for some error cases so it is important to call it at least on the final batch, even if there are no pending rows. Additional, in the file-upload machine, that function needs to include a final zero-byte Write to its pipe to ensure there isn’t an error in that pipe from the reader side that was set since the last call to Write (which could be never for zero-byte files). Release note: none.
The `nodelocal` cloud storage implementation previously did its own
file IO, and checked for and enforced the ExternalIODirectory, including
checking when it was disabled completely ("").
However after the switch to backing `nodelocal` with the local-or-remote
blob service, this enforcement now needs to be done in that service,
when it actually goes to do local IO, as it is a per-node decision if
and where to allow IO.
Release note (security update): ensure --external-io-dir=disabled applies to `nodelocal upload` requests as well.
|
bors r=knz |
Build failed |
|
bors r+ |
Build succeeded |
The
nodelocalcloud storage implementation previously did its ownfile IO, and checked for and enforced the ExternalIODirectory, including
checking when it was disabled completely ("").
However after the switch to backing
nodelocalwith the local-or-remoteblob service, this enforcement now needs to be done in that service,
when it actually goes to do local IO, as it is a per-node decision if
and where to allow IO.
Release note (security update): ensure --external-io-dir=disabled applies to
nodelocal uploadrequests as well.Release justification: Bug fix.
Separately: add a zero-byte write at the end of upload to ensure any errors in the pipe are flushed (otherwise zero-byte files never saw errors).