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

pod read unable to distinguish between empty result and missing file #4846

Closed
shawkins opened this issue Feb 7, 2023 · 6 comments · Fixed by #4842
Closed

pod read unable to distinguish between empty result and missing file #4846

shawkins opened this issue Feb 7, 2023 · 6 comments · Fixed by #4842
Milestone

Comments

@shawkins
Copy link
Contributor

shawkins commented Feb 7, 2023

Is your task related to a problem? Please describe

using pod read or copy will treat a missing file as an empty result.

Describe the solution you'd like

An exception when missing.

It would be nice if we could just turn on terminateOnError on the underlying exec, but this does not work because the tar command will complain about stripping the leading / to stdErr. The tar command either needs sanitized (which my quick attempt didn't seem to do) or both the readTar and the read commands need to include an explicit existence check that returns an error exit code:

test -e file && ... regular command ... || exit 1

Describe alternatives you've considered

No response

Additional context

No response

@manusa
Copy link
Member

manusa commented Feb 8, 2023

I'm assuming (since you mention readTar) that this only affects the read when reading directories and not files.
Our current readFile approach should fail since catting a missing file should return a non-zero exit code.

Regarding the empty-file, we could do a subsequent check in case the downloaded file/stream is empty and verify that the container file/directory actually exists. (i.e. if byteCount == 0 then checkFileExists())

@shawkins
Copy link
Contributor Author

shawkins commented Feb 8, 2023

I'm assuming (since you mention readTar) that this only affects the read when reading directories and not files.

Detecting a missing target is a problem for both. What I mean is that when you do something like "cat %s | base64" it will naturally emit something to stdErr about the file not existing, but the exitCode is 0. We have an option to terminateOnError that could pickup that stdErr message, but that doesn't work for directories because the tar command is also creating an error message - even for successful execution. If we have some way or at least preserving the non-zero exit code then we'll see the situation as an error. It's not great without the corresponding message (which you may only see in debug), but it's a start.

Instead of the file test approach we could also just to "base64 %s" - that will preserve the non-zero exit code as well.

@manusa
Copy link
Member

manusa commented Feb 8, 2023

OK, you're right, the pipe to base64 swallows the exit code.

What about test -e $file && (cat $file | base64)?

@shawkins
Copy link
Contributor Author

shawkins commented Feb 8, 2023

What about test -e $file && (cat $file | base64)?

Do you know the rationale behind using cat? It looks like "base64 %s" works just fine by itself and preserves the exit code.

@manusa
Copy link
Member

manusa commented Feb 8, 2023

I can't remember, not sure if all base64 versions have the same behavior (they probably do, but different behavior is the only reason I can think of)

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 8, 2023
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 8, 2023
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 9, 2023
@shawkins
Copy link
Contributor Author

shawkins commented Feb 9, 2023

I can't remember, not sure if all base64 versions have the same behavior (they probably do, but different behavior is the only reason I can think of)

Looking at things more the use of base64 seems unnecessary for both regular files and directories. The go client for example does not use base64 - and always uses tar even for single files. I've updated the debugging pr to be for this refinement instead. One small oddity is that the copy directory won't fail until after some of the initial stdOut has been read - I did not try to address that.

shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 22, 2023
shawkins added a commit to shawkins/kubernetes-client that referenced this issue Feb 22, 2023
@manusa manusa added this to the 6.5.0 milestone Feb 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants