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

rootlessport: set source IP to slirp4netns device #9052

Merged

Conversation

giuseppe
Copy link
Member

set the source IP to the slirp4netns address instead of 127.0.0.1 when
using rootlesskit.

Closes: #5138

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 21, 2021
@rhatdan
Copy link
Member

rhatdan commented Jan 21, 2021

@giuseppe You have to add a new test for this.
LGTM except for the missing test.

@AkihiroSuda
Copy link
Collaborator

CNI-in-slirp4netns mode would need be updated to use container IP as the RootlessKit child IP

@AkihiroSuda
Copy link
Collaborator

Also it should be documented that apps will always see 10.0.2.100 (or CNI IP) as the source address.

defer GinkgoRecover()
defer wg.Done()

// wait 2 seconds to be sure the container is running
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ouch, I'm super uncomfortable with this type of test (the "sleep and hope we don't race" thing). Please stand by, I'm working on an alternative quicker safer test.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@giuseppe please try
this test instead:

diff --git a/test/system/500-networking.bats b/test/system/500-networking.bats
index a824ebcd7..b4548c78a 100644
--- a/test/system/500-networking.bats
+++ b/test/system/500-networking.bats
@@ -65,8 +65,13 @@ load helpers
     myport=54321

     # Container will exit as soon as 'nc' receives input
+    # We use '-n -v' to give us log messages showing an incoming connection
+    # and its IP address; the purpose of that is guaranteeing that the
+    # remote IP is not 127.0.0.1 (podman PR #9052).
+    # We could get more parseable output by using $NCAT_REMOTE_ADDR,
+    # but busybox nc doesn't support that.
     run_podman run -d --userns=keep-id -p 127.0.0.1:$myport:$myport \
-               $IMAGE nc -l -p $myport
+               $IMAGE nc -l -n -v -p $myport
     cid="$output"

     # emit random string, and check it
@@ -74,7 +79,20 @@ load helpers
     echo "$teststring" | nc 127.0.0.1 $myport

     run_podman logs $cid
-    is "$output" "$teststring" "test string received on container"
+    # High-level overview of received output. We also check it line by line
+    # but this is a basic test; if it fails, we can see full output, which
+    # is helpful because failure here indicates something is VERY wrong.
+    is "$output" "listening on .*:$myport .*connect to .*$teststring" \
+       "Basic check on received output"
+
+    # Line-by-line output check. If any of these fail, we will not see
+    # the full output of 'nc'. The most important check here is the
+    # second line, in which we check for a 10.X remote IP (not 127.*)
+    is "${lines[0]}" "listening on \[::\]:$myport ..." "First line of output"
+    is "${lines[1]}" \
+       "connect to \[::ffff:10\..*\]:$myport from \[::ffff:10\..*\]:" \
+       "Second output line from nc"
+    is "${lines[2]}" "$teststring" "test string received on container"

     # Clean up
     run_podman rm $cid

@giuseppe
Copy link
Member Author

CNI-in-slirp4netns mode would need be updated to use container IP as the RootlessKit child IP

any pointer on how that must be done?

@AkihiroSuda
Copy link
Collaborator

CNI-in-slirp4netns mode would need be updated to use container IP as the RootlessKit child IP

any pointer on how that must be done?

This line would need to be changed to pass child IP from CNI result:

return r.setupRootlessPortMappingViaRLK(ctr, netnsPath)

@edsantiago
Copy link
Collaborator

Error looks like a flake:

[+0998s] not ok 119 podman pod create - hashtag AllTheOptions
...
         # # podman-remote --url unix:/tmp/podman.CqfZh5 build -t infra_gyst8obgpd -
         # time="2021-01-21T18:46:25Z" level=error msg="cannot tar container entries [/var/tmp/buildah169026310/Dockerfile /var/tmp/buildah169026310] error: 0 errors occurred:\n\t\n\n"
         # Error: 0 errors occurred:
         # 	
         # [ rc=125 (** EXPECTED 0 **) ]

Will restart.

@rhatdan
Copy link
Member

rhatdan commented Jan 21, 2021

@AkihiroSuda PTAL

@edsantiago
Copy link
Collaborator

ARGH! My test is bad: output from 'nc' is not ordered reliably:

[+0759s] # #/vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
[+0759s] # #|     FAIL: Basic check on received output
[+0759s] # #| expected: 'listening on .*:54321 .*connect to .*jufhtVkETQ58fsP96HFZF7RU1vnAmG'
[+0759s] # #|   actual: 'listening on [::]:54321 ...'
[+0759s] # #|         > 'jufhtVkETQ58fsP96HFZF7RU1vnAmG'
[+0759s] # #|         > 'connect to [::ffff:10.88.0.132]:54321 from [::ffff:10.88.0.1]:51352 ([::ffff:10.88.0.1]:51352)'
[+0759s] # #\^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I need to rethink this. Will offer a fix soon.

@edsantiago
Copy link
Collaborator

Replacement test (with downloadable patch):

diff --git a/test/system/500-networking.bats b/test/system/500-networking.bats
index b4548c78a..bcc6737b7 100644
--- a/test/system/500-networking.bats
+++ b/test/system/500-networking.bats
@@ -79,20 +79,17 @@ load helpers
     echo "$teststring" | nc 127.0.0.1 $myport

     run_podman logs $cid
-    # High-level overview of received output. We also check it line by line
-    # but this is a basic test; if it fails, we can see full output, which
-    # is helpful because failure here indicates something is VERY wrong.
-    is "$output" "listening on .*:$myport .*connect to .*$teststring" \
-       "Basic check on received output"
-
-    # Line-by-line output check. If any of these fail, we will not see
-    # the full output of 'nc'. The most important check here is the
-    # second line, in which we check for a 10.X remote IP (not 127.*)
-    is "${lines[0]}" "listening on \[::\]:$myport ..." "First line of output"
-    is "${lines[1]}" \
-       "connect to \[::ffff:10\..*\]:$myport from \[::ffff:10\..*\]:" \
-       "Second output line from nc"
-    is "${lines[2]}" "$teststring" "test string received on container"
+    # Sigh. We can't check line-by-line, because 'nc' output order is
+    # unreliable. We usually get the 'connect to' line before the random
+    # string, but sometimes we get it after. So, just do substring checks.
+    is "$output" ".*listening on \[::\]:$myport .*" "nc -v shows right port"
+
+    # This is the truly important check: make sure the remote IP is
+    # in the 10.X range, not 127.X.
+    is "$output" \
+       ".*connect to \[::ffff:10\..*\]:$myport from \[::ffff:10\..*\]:.*" \
+       "nc -v shows remote IP address in 10.X space (not 127.0.0.1)"
+    is "$output" ".*${teststring}.*" "test string received on container"

     # Clean up
     run_podman rm $cid

@giuseppe
Copy link
Member Author

Replacement test (with downloadable patch):

thanks! Just pushed the new version

@edsantiago
Copy link
Collaborator

Yay, tests are green. LGTM, but I'll let someone more network-knowledgeable do the approving. Thanks @giuseppe, and sorry for my botched first attempt.

go.mod Outdated Show resolved Hide resolved
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
set the source IP to the slirp4netns address instead of 127.0.0.1 when
using rootlesskit.

Closes: containers#5138

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@rhatdan
Copy link
Member

rhatdan commented Jan 22, 2021

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 22, 2021
@openshift-merge-robot openshift-merge-robot merged commit f02aba6 into containers:master Jan 22, 2021
0 of 2 checks passed
@AkihiroSuda
Copy link
Collaborator

CNI support seems unfixed: #9065

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source IP always 127.0.0.1 in rootless Podman 1.8.0
6 participants