fix(capture): ignore known copy failure and fix iptables issue #903
Conversation
Signed-off-by: Qingchuan Hao <[email protected]>
mainred
force-pushed
the
capture/fix-iptables-error
branch
from
5834c7f to
b95d2e4
Compare
|
We install iptables legacy on Mariner image when building the image, but when it runs on ubuntu host, the command returns empty result, which works fine on iptables nft mode. |
| nftIptablesMode iptablesMode = "nft" | ||
| ) | ||
|
|
||
| func obtainIptablesMode(l *log.ZapLogger) iptablesMode { |
There was a problem hiding this comment.
I think you should return unhandled errors along with mode.
There was a problem hiding this comment.
The ideas in my mind is the failed iptables rules does not break the whole capture.
mainred
changed the title
…a into capture/fix-iptables-error
mainred
force-pushed
the
capture/fix-iptables-error
branch
from
30b2834 to
8e7d3b2
Compare
Signed-off-by: Qingchuan Hao <[email protected]>
mainred
force-pushed
the
capture/fix-iptables-error
branch
from
8e7d3b2 to
b173ff7
Compare
|
ping @timraymond and @anubhabMajumdar for another look. |
|
LGTM on @anubhabMajumdar 's approval |
|
This PR will be closed in 7 days due to inactivity. |
github-actions
bot
added
the
meta/waiting-for-author
|
Pull request closed due to inactivity. |
|
Reopening and bringing it back to light @anubhabMajumdar @mainred |
github-actions
bot
removed
the
meta/waiting-for-author
| legacySaveOut, err := exec.Command("iptables-legacy-save").CombinedOutput() | ||
| if err != nil { | ||
| nftIptablesModeAvaiable = false | ||
| logger.Error("Failed to run iptables-legacy-save", zap.Error(err)) |
There was a problem hiding this comment.
I think we should make this more verbose. This error doesn't indicate the tool failed to collect capture; it just indicates the command may not be available.
There was a problem hiding this comment.
@anubhabMajumdar It doesn't even really indicate that since weird things can go wrong when exec'ing under a stressed-out system. If we want to definitively test whether the command exists or not, the correct way to do that is with exec.LookPath. We should preflight each of these invocations to rule out the command not existing first (which can be done without exec()ing too). Then if we hit this arm of the error, we know definitively that it was something unusual in the actual execution of the command (probably OOM or something).
Also, in general, if it's happy-path control flow that the command does not exist (which we could be certain of with exec.LookPath, we don't need to log the error (since it's not an error). Once we are sure that it's an unusual error, we should return the error from this func, wrapped with context about what we were trying to do when the error was produced.
| } | ||
| return "nft" | ||
| return nftIptablesMode |
There was a problem hiding this comment.
I want to highlight this again - we are returning nftIptablesMode even if we know that is not available. This assumes the behavior of the caller and shouldn't be done. We should return error if no mode is available and let the caller handle the error.
| name: "cp", | ||
| args: []string{"-r", "/proc/sys/net", filepath.Join(ncp.TmpCaptureDir, "proc-sys-net")}, | ||
| description: "kernel networking configuration", | ||
| ignoreFailure: true, |
There was a problem hiding this comment.
Can we add a comment as to why this is optional? Also, maybe log this as a warn so that user doesn't go looking for this information in the capture.
Description
Related Issue
If this pull request is related to any issue, please mention it here. Additionally, make sure that the issue is assigned to you before submitting this pull request.
Checklist
git commit -S -s ...). See this documentation on signing commits.Screenshots (if applicable) or Testing Completed
Additional Notes
None
Please refer to the CONTRIBUTING.md file for more information on how to contribute to this project.