Support for external hostname for localstack #1422

CauchyPeano · 2019-04-24T15:35:33Z

Was not sure whether it's good idea to introduce new dependency for testing, but it's so far needed to prove that feature works.

Fixes #1102

kiview

Thanks a lot for the PR, I 've added some remarks and questions.

...s/localstack/src/main/java/org/testcontainers/containers/localstack/LocalStackContainer.java

modules/localstack/build.gradle

...s/localstack/src/main/java/org/testcontainers/containers/localstack/LocalStackContainer.java

bsideup

Please do not set HOSTNAME_EXTERNAL by default, it will break scenarios when LocalStack is being used inside a network, container-to-container style

rnorth · 2019-05-08T05:11:25Z

Good point @bsideup. We’ll need to think about that.

This is going to get tricky if some services (inc S3, it seems?) need it to be set, but if there’s no one value that works for both access from host and access from other containers.

_{Sent with GitHawk}

rnorth · 2019-05-28T19:46:36Z

...s/localstack/src/main/java/org/testcontainers/containers/localstack/LocalStackContainer.java

@@ -51,6 +52,10 @@ protected void configure() {

        withEnv("SERVICES", services.stream().map(Service::getLocalStackName).collect(Collectors.joining(",")));

+        if (hostnameExternal != null) {
+            withEnv("HOSTNAME_EXTERNAL", hostnameExternal);
+        }


FYI @bsideup, @CauchyPeano, @kiview I've removed the else clause that would set HOSTNAME_EXTERNAL by default.

I'd like to progress this further and have been thinking about how to do it.

We could leave as-is, but the downside is that everyone using SQS has to remember to use withHostnameExternal( DockerClientFactory.instance().dockerHostIpAddress()). That's an OK workaround, but I'd prefer to go with something more ergonomic...

So how about this:

If withHostnameExternal or the env var has been set, always respect that

Otherwise:

if the container is not connected to a Network and doesn't have a network alias, then use dockerHostIpAddress

if the container is connected to a Network and has at least one network alias, then use the first network alias

I think that this would work nicely out of the box.

And it should also should be backward compatible with anyone currently using HOSTNAME_EXTERNAL manually, which is the only way that anyone can currently be using SQS in either mode.

FWIW, it's already impossible to use the SQS mock from both the host and over a docker network because HOSTNAME_EXTERNAL can only take one value. The above approach doesn't help in that situation, but at least doesn't make it worse.

WDYT @bsideup?

that might work! 👍

We could improve it later and start a proxy on localhost (so that HOSTNAME_EXTERNAL is always localhost) but that will require a bit of work and can be done later if the proposed solution will cause any problems

Yeah, I was thinking about that too, but yes we have the potential to add that later should the need arise.

Cheers - I'll implement tonight.

I've implemented, with one slight difference: it is taking the last network alias, rather than the first. I've done this because:

we have a default tc-***** alias generated for every container, and users can add an arbitrary number of their own aliases

it's fiddly, and creates implicit coupling across classes, to try and pick 'the first alias that is not the tc-***** one'.

given that all we really need is for the behaviour to be predictable and reasonable, it seems like picking the last is meets that criteria just as well as picking the first.

rnorth · 2019-05-28T19:48:23Z

.../localstack/src/test/java/org/testcontainers/containers/localstack/SimpleLocalstackTest.java

+    }
+
+    @Test
+    public void hostnameSqsTest() {


It would be nice if we could make this test verify correct end-to-end behaviour rather than just check that the URL reflects the provided hostname.

Is there an easy to test scenario where the external hostname matters?

Have added some tests using AWS CLI.

bsideup · 2019-06-21T08:53:57Z

.../localstack/src/test/java/org/testcontainers/containers/localstack/SimpleLocalstackTest.java


+        CreateQueueResult queueResult = sqs.createQueue("baz");
+        String fooQueueUrl = queueResult.getQueueUrl();
+        assertThat("Created queue has external hostname URL", fooQueueUrl, containsString(TEST_HOSTNAME));


not critical, but would be better to parse the returned URL and compare the host, otherwise result like foobar.localhost will pass too, but fail in real world

Now comparing with the full URL; still using the slightly simplistic string contains mechanism though.

rnorth · 2019-09-19T08:44:11Z

It looks like my commits were disappearing into the ether (or at least not landing on this PR), so I've created #1891 to replace it.

CauchyPeano · 2019-09-19T08:44:42Z

Yeah, sorry, I've completely lost track of this discussion :)

Support for external hostname for localstack (testcontainers#1102)

1e32b7b

CauchyPeano requested review from bsideup, kiview and rnorth as code owners April 24, 2019 15:35

kiview requested changes Apr 24, 2019

View reviewed changes

CauchyPeano added 2 commits May 2, 2019 16:55

Merge branch 'master' into master

6443e51

Support for external hostname for localstack (testcontainers#1102)

ea82b0d

rnorth mentioned this pull request May 7, 2019

Enable new Localstack services #1413

Closed

rnorth requested a review from kiview May 7, 2019 11:34

rnorth reviewed May 7, 2019

View reviewed changes

...s/localstack/src/main/java/org/testcontainers/containers/localstack/LocalStackContainer.java Outdated Show resolved Hide resolved

bsideup requested changes May 7, 2019

View reviewed changes

Don't set HOSTNAME_EXTERNAL by default

0a536ee

rnorth reviewed May 28, 2019

View reviewed changes

bsideup reviewed Jun 21, 2019

View reviewed changes

This was referenced Sep 16, 2019

Added HOSTNAME_EXTERNAL env property to local stack container (AWS) #1864

Closed

Localstack support for HOSTNAME_EXTERNAL #1891

Merged

rnorth closed this Sep 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for external hostname for localstack #1422

Support for external hostname for localstack #1422

CauchyPeano commented Apr 24, 2019 •

edited by bsideup

Loading

kiview left a comment

bsideup left a comment

rnorth commented May 8, 2019

rnorth May 28, 2019

rnorth Sep 17, 2019

bsideup Sep 17, 2019

rnorth Sep 17, 2019

rnorth Sep 19, 2019

rnorth May 28, 2019

rnorth Sep 19, 2019

bsideup Jun 21, 2019 •

edited

Loading

rnorth Sep 19, 2019

rnorth commented Sep 19, 2019

CauchyPeano commented Sep 19, 2019

Support for external hostname for localstack #1422

Support for external hostname for localstack #1422

Conversation

CauchyPeano commented Apr 24, 2019 • edited by bsideup Loading

kiview left a comment

Choose a reason for hiding this comment

bsideup left a comment

Choose a reason for hiding this comment

rnorth commented May 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsideup Jun 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnorth commented Sep 19, 2019

CauchyPeano commented Sep 19, 2019

CauchyPeano commented Apr 24, 2019 •

edited by bsideup

Loading

bsideup Jun 21, 2019 •

edited

Loading