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

(#128) Throw an error when bash is not installed in the target container #129

Merged
merged 9 commits into from
Feb 7, 2020

Conversation

mdelapenya
Copy link
Collaborator

@mdelapenya mdelapenya commented Jan 7, 2020

What does this PR do?

It throws an error when the evaluation of the '/bin/bash' command used to
check the host/port in the target container fails because 'bash' is not
present in the system (as it happens in most Alpine-based images).

Why is it important?

It will make the checks more reliable, as the implementation will raise an
error that could be handled by client code.

Before this change, the client code waited and a timeout happened. Now a more
accurate error is raised so it's easier to handle.

Related issues

Closes #128

@mdelapenya mdelapenya closed this Jan 7, 2020
@mdelapenya
Copy link
Collaborator Author

reopening for travis to rebuild

@mdelapenya mdelapenya reopened this Jan 7, 2020
@mdelapenya mdelapenya closed this Jan 8, 2020
@mdelapenya mdelapenya reopened this Jan 8, 2020
@@ -88,13 +88,15 @@ func (hp *HostPortStrategy) WaitUntilReady(ctx context.Context, target StrategyT
//internal check
command := buildInternalCheckCommand(hp.Port.Int())
for {
exitCode, err := target.Exec(ctx, []string{"/bin/bash", "-c", command})
exitCode, err := target.Exec(ctx, []string{"/bin/sh", "-c", command})

Choose a reason for hiding this comment

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

it was a bug, yes, sorry for that

@@ -105,7 +107,7 @@ func buildInternalCheckCommand(internalPort int) string {
command := `(
cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :%x ||
nc -vz -w 1 localhost %d ||
/bin/bash -c '</dev/tcp/localhost/%d'
/bin/sh -c '</dev/tcp/localhost/%d'

Choose a reason for hiding this comment

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

this is unnecessary, I believe. External command fix should be enough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hey, sorry for the late response. I'll revert this change and resend. TBH, not sure why the travis tests fail tough.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mmmm, thinking about it, the internal command should be sh, because it's very likely that bash is not present in the container (i.e. Alpine-based images)

@nikolayk812
Copy link

@mdelapenya are you planning to fix this branch?

@mdelapenya mdelapenya closed this Feb 5, 2020
@mdelapenya mdelapenya reopened this Feb 5, 2020
@mdelapenya
Copy link
Collaborator Author

@mdelapenya are you planning to fix this branch?

Yes, the thing is I'm not sure why the tests are failing on Travis 🤷‍♀ I'm debugging locally trying to find the issue

@mdelapenya
Copy link
Collaborator Author

After debugging this before the sh replacement (37e3ccb) and after it (8e056de) I can see that the execution of the buildInternalCheckCommand is causing the problem.

The below output comes from debugging TestContainerCreationAndWaitForListeningPortLongEnough from VSCode before replacing BASH, adding a breakpoint at host_port.go:

exitCode, err := target.Exec(ctx, []string{"/bin/bash", "-c", command})

The generated command is:

true && (
					cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :50 ||
					nc -vz -w 1 localhost 80 ||
					/bin/bash -c '</dev/tcp/localhost/80'
				)

Entering into the created container (docker exec -ti), I can execute each of those subcommands:

root@16589f2fd2e4:/# cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :50                                               
root@16589f2fd2e4:/# cat /proc/net/tcp{,6} | awk '{print $2}'              
local_address
00000000:0050
0100007F:8200
0100007F:81F4
local_address
root@16589f2fd2e4:/# cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :0050
00000000:0050 

BTW, nc is probably not installed in the target container although not executed if the previous piped commands succeed.

The weirdest thing here is that running the tests at this point, they all pass. Replacing BASH with SH, then the command exits with errorLevel 2:

Exit code 2        Misuse of shell builtins (according to Bash documentation)        Example: empty_function() {}

@nikolayk812 could you assist me here? Thanks!

@mdelapenya
Copy link
Collaborator Author

More on this. If I run the container used in tests:

docker run --name nginx --rm -d menedev/delayed-nginx:1.15.2

And execute the internal command from my local terminal:

With /bin/bash

$ docker exec nginx /bin/bash -c "true && (cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :50)"
$ docker exec nginx /bin/bash -c "true && (cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :0050)"
   0: 00000000:0050 00000000:0000 0A 00000000:00000000 00:00000000 00000000     0        0 6218691 1 0000000000000000 100 0 0 10 0 

With /bin/sh

$ docker exec nginx /bin/sh -c "true && (cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :50)"  
cat: /proc/net/tcp{,6}: No such file or directory
$ docker exec nginx /bin/sh -c "true && (cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :0050)"
cat: /proc/net/tcp{,6}: No such file or directory

I'm going to check how to use /bin/sh properly

@mdelapenya
Copy link
Collaborator Author

More on this: trying to escape awk quotes properly:

$ docker exec nginx /bin/bash -c 'true && (cat /proc/net/tcp6 | awk "\"{print $2}"\" | grep -i :0050)'
$ docker exec nginx /bin/sh -c 'true && (cat /proc/net/tcp{,6} | awk "\"{print $2}"\" | grep -i :0050)' 
cat: /proc/net/tcp{,6}: No such file or directory

I see differences in how the sh command sees the tcp files.

Entering the container used in tests:

root@8da5c254bfe3:/# /bin/bash -c 'true && (cat /proc/net/tcp{,6} | awk "\"{print $2}"\" | grep -i :0050)'
   0: 00000000:0050 00000000:0000 0A 00000000:00000000 00:00000000 00000000     0        0 6218691 1 0000000000000000 100 0 0 10 0                   
root@8da5c254bfe3:/# echo $?
0
root@8da5c254bfe3:/# /bin/sh -c 'true && (cat /proc/net/tcp{,6} | awk "\"{print $2}"\" | grep -i :0050)'
cat: /proc/net/tcp{,6}: No such file or directory
root@8da5c254bfe3:/# echo $?
1

I see slight differences in how the command behaves.

As described in https://www.kernel.org/doc/Documentation/networking/proc_net_tcp.txt
the format of /proc/net/tcp is using 4 digits for the hex port:

46: 010310AC:9C4C 030310AC:1770 01
|      |      |      |      |   |--> connection state
|      |      |      |      |------> remote TCP port number
|      |      |      |-------------> remote IPv4 address
|      |      |--------------------> local TCP port number
|      |---------------------------> local IPv4 address
|----------------------------------> number of entry

So this string pattern is more accurate
@@ -109,7 +109,7 @@ func (hp *HostPortStrategy) WaitUntilReady(ctx context.Context, target StrategyT

func buildInternalCheckCommand(internalPort int) string {
command := `(
cat /proc/net/tcp{,6} | awk '{print $2}' | grep -i :%04x ||
cat /proc/net/tcp* | awk '{print $2}' | grep -i :%04x ||
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was the clue!! At least, WOMC! Let's 🤞 for Travis :)

@mdelapenya
Copy link
Collaborator Author

mdelapenya commented Feb 6, 2020

Tests fixed! @gianarb please take a look 🙏, this is already fixed

image

BTW, using SH instead of BASH is what the Java impl is doing (https://github.com/testcontainers/testcontainers-java/blob/23e9718a56eeac3ca2ec87c8a2525fa8e4bf76df/core/src/main/java/org/testcontainers/containers/wait/internal/InternalCommandPortListeningCheck.java#L42), so without knowing it before, it turned out we are in the same page 😄

@gianarb
Copy link
Collaborator

gianarb commented Feb 7, 2020

Good job!! Thanks @mdelapenya

@gianarb gianarb merged commit 0a03bcf into testcontainers:master Feb 7, 2020
@mdelapenya mdelapenya deleted the 128-bash-host-port branch February 7, 2020 16:41
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 this pull request may close these issues.

WaitForHostPort for depends on bash being installed in the target service
4 participants