Migrate iptables and ebtables usages to nftables#2561
Conversation
|
|
||
| handle_file="${cuttlefish_run_dir}/${type}-${id}.handle" | ||
|
|
||
| if [ "$op" = "add" ]; then |
There was a problem hiding this comment.
Can this also use a case statement?
| elif [ "$op" = "delete" ]; then | ||
| if [ -f "$handle_file" ]; then | ||
| # Find the handle file and use it to clean things up. | ||
| handle=$(cat "$handle_file") |
| return 1 | ||
| fi | ||
| # Add the rule and store the handle. | ||
| handle=$(nft -a -e add rule "$family" "$table" "$chain" "$contents" | awk '/# handle/ {print $NF}') |
There was a problem hiding this comment.
Please add Depends entries for newly added tools, in this case nft and awk:
android-cuttlefish/base/debian/control
Lines 36 to 40 in 40a03a3
| local handle_file="" | ||
| local handle="" |
There was a problem hiding this comment.
Can the variable declarations for handle and handle_file be combined with the first assignment, or at least declared more closely?
https://google.github.io/styleguide/shellguide.html#s7.5-use-local-variables does mention though that
Declaration and assignment must be separate statements when the assignment value is provided by a command substitution; as the
localbuiltin does not propagate the exit code from the command substitution.
There was a problem hiding this comment.
my first instinct says that if all of the possible variables are declared at the top of the function, in the case of a language like bash, it would make it easier to reason about the function body (since then you already know all of the variables you're going to see), what do you think?
| // Discover an IP address of the host that the guest can ping to verify default routing. | ||
| func discoverPingableHostIP(t *testing.T) string { |
There was a problem hiding this comment.
What do you think of pinging a standard publicly available IP address like 8.8.8.8?
There was a problem hiding this comment.
I chose this IP because it'd be pingable even if the host's network is down but tbh internet access is a prerequisite so 8.8.8.8 would let us skip this entire step and simplify the test case so i might as well do that
| }{ | ||
| { | ||
| branch: "aosp-android-latest-release", | ||
| target: "aosp_cf_x86_64_only_phone-userdebug", | ||
| }, | ||
| } |
There was a problem hiding this comment.
Please add a test case for git_main as well.
| return 1 | ||
| fi | ||
| # Add the rule and store the handle. | ||
| handle=$(nft -a -e add rule "$family" "$table" "$chain" "$contents" | awk '/# handle/ {print $NF}') |
There was a problem hiding this comment.
An alternative to awk is using nft -j to output JSON and parsing it. This has the advantage of of relying on the documented json output format, rather than the current output format, which is more of an implementation detail. JSON parsing is easier with a tool like jq, but using that has the downside of a new package depenency.
There was a problem hiding this comment.
yeah it's a bit fragile like this. i'll weigh how to do that..
3 participants
Also adds a basic networking test and a workaround bazel arg to disable sandboxing to run locally (since some of the upstream images don't have the fix yet)
b/500704721