[proxy] More flexible container name -> hostname derivation #1126

2opremio · 2015-07-13T17:44:47Z

Provides a new "--hostname=substitution" flag to WeaveProxy, where
'substitution' is a sed-style substitution command. 'substitution' will be
applied to container names at launch-time, as a means to control what hostnames will
be registered in WeaveDNS.

The substitution is implemented with
https://golang.org/pkg/regexp/#Regexp.ReplaceAllString . Thus, references to
regexp substitution groups should be prepended with a dollar sign.

For instance, if we provide "--hostname='/aws-[0-9]+-(.*)/my-app-$1/'", running
a container named 'aws-12798186823-foo' through WeaveProxy will lead to WeaveDNS
registering 'my-app-foo'.

Closes #1018

tomwilkie · 2015-07-13T20:51:18Z

we'll need a few lines in the docs too, please.

2opremio · 2015-07-13T22:02:27Z

@tomwilkie Sure, I considered it but it wasn't clear to me where to include the documentation. https://github.com/weaveworks/weave/tree/master/docs only seems to include architectural stuff.

2opremio · 2015-07-13T22:19:13Z

OK, I found /site which I hadn't payed attention to until now. I will add something there first thing tomorrow.

2opremio · 2015-07-14T09:20:06Z

@tomwilkie Done, I will squash the commit once the PR is good to go.

2opremio · 2015-07-14T11:09:07Z

Rebased with master (and squashed the documentation commit) to make coveralls happy

tomwilkie · 2015-07-14T11:45:05Z

You should fetch the coverage.txt from your branch and diff it with master to see where this -5% is coming from.

weave

@@ -36,7 +36,7 @@ weave launch-router [--password <password>] [--nickname <nickname>]
                      [--no-discovery] [--init-peer-count <count>] <peer> ...
 weave launch-dns    [<addr>]
 weave launch-proxy  [-H <endpoint>] [--with-dns | --without-dns]
-                      [--no-default-ipalloc] [--no-rewrite-hosts]
+                      [--no-default-ipalloc] [--no-rewrite-hosts] [--hostname]


2opremio · 2015-07-14T11:55:18Z

@tomwilkie Thanks, will do. I suspect it's coming from proxy/ not having any tests until now.

proxy/proxy.go

-	execCreateRegexp      = regexp.MustCompile("^(/v[0-9\\.]*)?/containers/[^/]*/exec$")
+	containerCreateRegexp = regexp.MustCompile(`^(/v[0-9\.]*)?/containers/create$`)
+	containerStartRegexp  = regexp.MustCompile(`^(/v[0-9\.]*)?/containers/[^/]*/(re)?start$`)
+	execCreateRegexp      = regexp.MustCompile(`^(/v[0-9\.]*)?/containers/[^/]*/exec$`)


tomwilkie · 2015-07-14T13:20:11Z

Alternatively, instead of requireing a sed-style regex in the for /foo/bar/, how about we just have --hostname-match=foo and --hostname-replacement=bar? It would save us a bunch of code (using regexes to parse regexes...) Default for --hostname-match should be (.*) and --hostname-replacement should be $1

Also, I wouldn't bother including this in the weave usage text; this feature is meant for integrators only. Just include it in the site/ docs.

2opremio · 2015-07-14T13:28:28Z

@tomwilkie I personally like the /regexp/replacement/ syntax better, @rade also seems to agree as per #1018

Also, I am not sure it makes sense not to document it in the usage if it's in the docs. If we want it to be hidden from normal users then it shouldn't appear on the site.

tomwilkie · 2015-07-14T13:32:47Z

@rade what do you think? I suggested this as I'm not sure the parsing of the sed-style regexps is worth the extra code.

2opremio · 2015-07-14T13:46:37Z

I pushed a commit which should address all the code comments (except for @tomwilkie's design remark). Also, I pushed a change to surround relevant regexps with ^ and $.

Please make sure to squash the changes before merging.

test/680_proxy_hostname_derivation.sh

+
+check() {
+  assert "proxy exec_on $HOST1 $1 getent hosts $2 | tr -s ' '" "$3 $2"
+}


rade · 2015-07-14T13:49:52Z

I'm not sure the parsing of the sed-style regexps is worth the extra code.

Agreed, and I am not sold on the UI benefits either.

So two options it is, with the defaults as you suggest. But I do think they should be documented in the usage.

2opremio · 2015-07-14T13:55:15Z

OK, I will rework the PR to use those two options.

Provides to new two new flags: `--hostname-match <regexp>` and `--hostname-replacement <replacement>`. When launching a container, its name matched against regular expression `<regexp>`. Then, based on that match, `<replacement>` will be used to generate a hostname, which will ultimately be handed over to weaveDNS for registration. The substitution is implemented with https://golang.org/pkg/regexp/#Regexp.ReplaceAllString . Thus, references to regexp substitution groups should be prepended with a dollar sign. For instance, if we provide `--hostname-match '^aws-[0-9]+-(.*)$'` and `--hostname-replacement 'my-app-$1'`, running a container named `aws-12798186823-foo` through WeaveProxy will lead to WeaveDNS registering `my-app-foo`. Closes #1018

2opremio · 2015-07-14T14:44:18Z

@rade @paulbellamy @tomwilkie please review

proxy/proxy.go

+	dockerSock                  = "/var/run/docker.sock"
+	dockerSockUnix              = "unix://" + dockerSock
+	substitutionPatternName     = "pattern"
+	substitutionReplacementName = "replacement"


2opremio · 2015-07-14T15:15:36Z

@tomwilkie It seems there are GCP quota problems in circleci https://circleci.com/gh/weaveworks/weave/1835

tomwilkie · 2015-07-14T15:25:07Z

Yes I've just cleared out some stale VMs. VMs aren't cleaned up if you
click cancel.

On Tue, Jul 14, 2015 at 4:15 PM, Alfonso Acosta [email protected]
wrote:

@tomwilkie https://github.com/tomwilkie It seems there are GCP quota
problems in circleci https://circleci.com/gh/weaveworks/weave/1835

—
Reply to this email directly or view it on GitHub
#1126 (comment).

weave

@@ -37,6 +37,7 @@ weave launch-router [--password <password>] [--nickname <nickname>]
 weave launch-dns    [<addr>]
 weave launch-proxy  [-H <endpoint>] [--with-dns | --without-dns]
                      [--no-default-ipalloc] [--no-rewrite-hosts]
+                      [--hostname-match <regexp>] [--hostname-replacement <replacement>]


…ivation [proxy] More flexible container name -> hostname derivation Closes #1018.

This wasn't spotted because #1065, which removed launch-dns, got merged just before #1126, which introduced this test.

These tests were introduced in #1065 and #1126.

2opremio · 2015-07-14T20:11:23Z

@rade Thanks. Out of curiosity, is there a reason why fix-commits are not squashed before merging? (Commits like ded3d6b don't make any sense out of context) I added extra commits to make clear what was being changed, with the intention of squashing them before merging. Should I had kept amending instead?

rade · 2015-07-14T20:19:43Z

I didn't realise you wanted to squash some commits. Better leave a note in the PR next time.

2opremio · 2015-07-14T21:10:02Z

@rade Sorry, I was expecting the merger to check the commits before going ahead. This is probably not the forum for this discussion but, in order to avoid these situations, wouldn't it make sense to let the author merge the code himself after getting a "go ahead"?

rade · 2015-07-14T21:15:50Z

I was expecting the merger to check the commits before going ahead.

I did. I'm not bothered by the non-squashed commits; There is enough context to trace them back to this PR.

This is probably not the forum for this discussion

Correct.

2opremio · 2015-07-14T21:53:05Z

@rade ok, fair enough :)

2opremio force-pushed the issues/1018-more-flexible-hostname-derivation branch 2 times, most recently from 95e7663 to 32a24b0 Compare July 14, 2015 11:01

rade reviewed Jul 14, 2015
View reviewed changes

2opremio force-pushed the issues/1018-more-flexible-hostname-derivation branch from 32a24b0 to 1c543a2 Compare July 14, 2015 13:42

rade reviewed Jul 14, 2015
View reviewed changes

test/680_proxy_hostname_derivation.sh

check() {

assert "proxy exec_on $HOST1 $1 getent hosts $2 | tr -s ' '" "$3 $2"

}

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

2opremio force-pushed the issues/1018-more-flexible-hostname-derivation branch from 1c543a2 to 4f82dc9 Compare July 14, 2015 14:43

paulbellamy reviewed Jul 14, 2015
View reviewed changes

proxy/proxy.go

dockerSock = "/var/run/docker.sock"

dockerSockUnix = "unix://" + dockerSock

substitutionPatternName = "pattern"

substitutionReplacementName = "replacement"

This comment was marked as abuse.

Sign in to view

Address PR comments

ded3d6b

2opremio force-pushed the issues/1018-more-flexible-hostname-derivation branch from 0777fce to ded3d6b Compare July 14, 2015 15:31

rade reviewed Jul 14, 2015
View reviewed changes

Make weave usage fit into 80 columns

dfcc967

rade added a commit that referenced this pull request Jul 14, 2015

Merge pull request #1126 from /issues/1018-more-flexible-hostname-der…

7ae06ef

…ivation [proxy] More flexible container name -> hostname derivation Closes #1018.

rade merged commit 7ae06ef into master Jul 14, 2015

rade deleted the issues/1018-more-flexible-hostname-derivation branch July 14, 2015 16:32

rade added a commit that referenced this pull request Jul 14, 2015

fix breakage

86448a1

This wasn't spotted because #1065, which removed launch-dns, got merged just before #1126, which introduced this test.

rade added a commit that referenced this pull request Jul 14, 2015

rename tests so they actually run

3629221

These tests were introduced in #1065 and #1126.

rade modified the milestones: current, 1.1.0 Jul 15, 2015

2opremio mentioned this pull request Sep 3, 2015

Obtain container hostname from specified Docker container label #1381

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[proxy] More flexible container name -> hostname derivation #1126

[proxy] More flexible container name -> hostname derivation #1126

2opremio commented Jul 13, 2015

tomwilkie commented Jul 13, 2015

2opremio commented Jul 13, 2015

2opremio commented Jul 13, 2015

2opremio commented Jul 14, 2015

2opremio commented Jul 14, 2015

tomwilkie commented Jul 14, 2015

This comment was marked as abuse.

2opremio commented Jul 14, 2015

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

tomwilkie commented Jul 14, 2015

2opremio commented Jul 14, 2015

tomwilkie commented Jul 14, 2015

2opremio commented Jul 14, 2015

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Jul 14, 2015

2opremio commented Jul 14, 2015

2opremio commented Jul 14, 2015

This comment was marked as abuse.

2opremio commented Jul 14, 2015

tomwilkie commented Jul 14, 2015

This comment was marked as abuse.

2opremio commented Jul 14, 2015

rade commented Jul 14, 2015

2opremio commented Jul 14, 2015

rade commented Jul 14, 2015

2opremio commented Jul 14, 2015

[proxy] More flexible container name -> hostname derivation #1126

[proxy] More flexible container name -> hostname derivation #1126

Conversation

2opremio commented Jul 13, 2015

tomwilkie commented Jul 13, 2015

2opremio commented Jul 13, 2015

2opremio commented Jul 13, 2015

2opremio commented Jul 14, 2015

2opremio commented Jul 14, 2015

tomwilkie commented Jul 14, 2015

This comment was marked as abuse.

2opremio commented Jul 14, 2015

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

tomwilkie commented Jul 14, 2015

2opremio commented Jul 14, 2015

tomwilkie commented Jul 14, 2015

2opremio commented Jul 14, 2015

This comment was marked as abuse.

This comment was marked as abuse.

rade commented Jul 14, 2015

2opremio commented Jul 14, 2015

2opremio commented Jul 14, 2015

This comment was marked as abuse.

2opremio commented Jul 14, 2015

tomwilkie commented Jul 14, 2015

This comment was marked as abuse.

2opremio commented Jul 14, 2015

rade commented Jul 14, 2015

2opremio commented Jul 14, 2015

rade commented Jul 14, 2015

2opremio commented Jul 14, 2015