add `weave dns-add/remove` commands #627

grkvlt · 2015-05-03T19:41:05Z

Added two new commands dns-add and dns-remove that call the tell_dns function method to store addresses and optionally a specific FQDN for a container ID.

This is useful for circumstances where containers may be configured with IP addresses outside weave, perhaps by some contaienr orchestration service or using a different SDN, but those addresses should still be resolvable and usable by the weaveDNS.

grkvlt · 2015-05-03T19:42:36Z

@errordeveloper Can you have a look and review this, please? it is for the use case we discussed where weaveDNS is being used without the weave SDN, but I think it might be useful in other circumstances also.

rade · 2015-05-03T20:32:37Z

Why do you want to make IP addresses for containers outside the weave network resolvable in the weave domain?

weave

 # This function is only called where we know $2 is a valid container name
 tell_dns() {
    METHOD="$1"
    CONTAINER_ID="$2"
    shift 2
-
+    collect_cidr_args "$@"
+    shift $CIDR_COUNT


rade · 2015-05-03T20:52:53Z

Is put-dns the best command name we can come up with?

Also, surely there should be an inverse operation.

rade · 2015-05-04T10:26:35Z

NB: this partially addresses #364. Note the major caveat listed there: all these "manually" inserted records are lost on weavedns restart. Nevertheless, I suggest that this PR closes #364, and that we open separate issues for the two remaining points identified there (lossy restart, and lack of support for CNAMEs).

grkvlt · 2015-05-04T19:28:43Z

@rade @errordeveloper For the inverse, I assume I just call tell_dns but with DELETE as the method. Do I need to worry about FQDNs in the delete call?

grkvlt · 2015-05-04T19:29:36Z

@rade How about add-dns and rm-dns as the two commands?

rade · 2015-05-04T19:58:44Z

For the inverse, I assume I just call tell_dns but with DELETE as the method. Do I need to worry about FQDNs in the delete call?

You should have to worry about the FQDN. After all, what if you have one IP under multiple names and want to remove just one of records? However, it looks like this functionality is missing from the DNS API.

grkvlt · 2015-05-04T20:02:38Z

You should have to worry about the FQDN

I pass in the domain name inside $MORE_DATA for the http_call in tell_dns_fqdn no matter what the $METHOD was set to. I think this means that once the DNS API supports it things will work as expected?

weave

@@ -28,6 +28,8 @@ usage() {
    echo "weave start      <cidr> [<cidr> ...] <container_id>"
    echo "weave attach     <cidr> [<cidr> ...] <container_id>"
    echo "weave detach     <cidr> [<cidr> ...] <container_id>"
+    echo "weave add-dns    <cidr> [<cidr> ...] <container_id> [-fqdn fqdn]"


rade · 2015-05-04T20:21:28Z

still not sure about the command names. add/remove-name? It's not perfect, but add-dns-A-record(s)-for-container-under-name-and-given-ip(s) is a bit long.

grkvlt · 2015-05-04T20:31:06Z

@rade happy to change to add-name and remove-name or what about rm-name instead?

rade · 2015-05-04T20:33:12Z

Let's see what other suggestions come up from interested bike shed painters.

grkvlt · 2015-05-04T20:56:37Z

OK, I have left it as add-name and rm-name for the moment. I also didn't include the ability to set an FQDN for the rm-name command, since as you pointed out it's not supported in the DNS API yet.

Thanks for taking the time to review this @rade

grkvlt · 2015-05-04T21:33:08Z

I added a test script (called 240_dns_add_name_test.sh) but was not able to run it properly, although manual testing of the add-name command works for me on a local Vargrant build of Weave. Are these tests run automatically anywhere?

awh · 2015-05-05T10:26:30Z

Are add-name etc a bit generic for the top level? How about contextualising them like this:

weave dns add
weave dns remove

awh · 2015-05-05T10:33:46Z

Just discussing with @squaremo - further to this line of thought we could contextualise the existing router commands:

weave router launch
weave router attach
weave dns launch

obviously we'd still support the existing script commands, but they would become deprecated - this would allow us to address #602 cleanly...

inercia · 2015-05-05T11:07:09Z

I like @awh proposal. I think we should move to a format like weave

grkvlt · 2015-05-05T11:23:34Z

@awh @inercia OK, but we should do that in a different PR. For this feature I think the current command names will be fine, although they could use something likr the @Beta annotation Java has?

rade · 2015-05-05T11:43:45Z

let's go with dns-add and dns-remove. This introduces an inconsistency w.r.t. other composite commands, which are "verb-noun", but all the alternatives we can think of are either longer (e.g. add-dns-record) or misleading (e.g. add-dns).

weavedns/README.md

+
+```bash
+$ docker stop $shell2
+$ weave dns-remove 10.2.1.27/24 $shell2


grkvlt · 2015-05-06T13:20:27Z

$ ./240_dns_add_name_test.sh 
Cleaning up on 192.168.48.11: removing all containers and resetting weave
Cleaning up on 192.168.48.12: removing all containers and resetting weave
Add and remove names on a single host
Weave on 192.168.48.11: launch-dns 10.2.254.1/24
9a8f3007c9166ce78815f2c2e56336fa0ebdcb9215f0c27c731f2b65fb8a1849
Weave on 192.168.48.11: run 10.2.0.34/24 -t --name=c2 ubuntu
9a8a29ee877e11acdaa4ef309c3d9344553ce941e9fee3189049b5cc3fc68296
Weave on 192.168.48.11: run --with-dns 10.2.0.78/24 -t --name=c1 aanand/docker-dnsutils /bin/sh
cf10f945925795426b5a14463fedcc4acb5a8fba9662fff39240ca42dbd0bdff
Weave on 192.168.48.11: dns-add 10.2.0.34/24 c2 -h seetwo.weave.local
Weave on 192.168.48.11: dns-remove 10.2.0.34/24 c2
all 3 tests passed in 8.012s.

grkvlt · 2015-05-06T13:29:28Z

@rade should be ready to merge, rebased and sqashed, with tests passing

test/240_dns_add_name_test.sh

+
+weave_on $HOST1 dns-remove $C2/24 c2
+
+assert_raises "exec_on $HOST1 c1 getent hosts $C2 | grep $NAME"


rade · 2015-05-06T18:08:34Z

add/remove-dns should take IP addresses (<ip_address> in weave usage terminology), not CIDRs.

I suspect this is going to make things a fair bit more involved since all the existing functions you are calling expect CIDRs. :(

test/240_dns_add_name_test.sh

+weave_on $HOST1 dns-add $C2/24 c2 -h $NAME2
+weave_on $HOST1 dns-remove $C2/24 c2
+
+assert_raises "exec_on $HOST1 c1 getent hosts $NAME2 | grep $C2" 1


grkvlt · 2015-05-06T22:11:53Z

@rade try again 🐨 this has IP addresses instead of CIDRs and improved tests as suggested.

weave

@@ -97,6 +99,32 @@ collect_cidr_args() {
    fi
 }

+is_ip() {
+    # The regexp here is far from precise, but good enough.
+    echo "$1" | grep -E '^[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}$' >/dev/null


- Commands dns-add and dns-remove added to weave script - Updated documentation for WeaveDNS - Added test script for dns-add and dns-remove - Added IP address functions matching existing CIDR ones

grkvlt · 2015-05-07T11:16:08Z

@rade we'll get there eventually... thanks again for helping me get this straightened out

add `weave dns-add/remove` commands Closes #364.

rade · 2015-05-07T12:06:12Z

@grkvlt thanks for your patience. Merged.

rade reviewed May 3, 2015
View reviewed changes

weave

# This function is only called where we know $2 is a valid container name

tell_dns() {

METHOD="$1"

CONTAINER_ID="$2"

shift 2

collect_cidr_args "$@"

shift $CIDR_COUNT

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

rade assigned errordeveloper May 4, 2015

grkvlt force-pushed the feature/add-dns-entry branch 2 times, most recently from e115e97 to 9adad5d Compare May 4, 2015 19:57

rade mentioned this pull request May 4, 2015

[dns] permit removal of individual records #631

Closed

rade reviewed May 4, 2015
View reviewed changes

grkvlt force-pushed the feature/add-dns-entry branch 2 times, most recently from 1568ddd to 46ca784 Compare May 4, 2015 21:30

rade self-assigned this May 6, 2015

rade reviewed May 6, 2015
View reviewed changes

weavedns/README.md

```bash

$ docker stop $shell2

$ weave dns-remove 10.2.1.27/24 $shell2

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

grkvlt force-pushed the feature/add-dns-entry branch 2 times, most recently from 265e1d1 to d6f9410 Compare May 6, 2015 13:27

rade changed the title ~~Add new put-dns weave command that calls tell_dns~~ add weave dns-add/remove commands May 6, 2015

rade reviewed May 6, 2015
View reviewed changes

test/240_dns_add_name_test.sh

weave_on $HOST1 dns-remove $C2/24 c2

assert_raises "exec_on $HOST1 c1 getent hosts $C2 | grep $NAME"

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

rade reviewed May 6, 2015
View reviewed changes

test/240_dns_add_name_test.sh

weave_on $HOST1 dns-add $C2/24 c2 -h $NAME2

weave_on $HOST1 dns-remove $C2/24 c2

assert_raises "exec_on $HOST1 c1 getent hosts $NAME2 | grep $C2" 1

This comment was marked as abuse.

Sign in to view

grkvlt force-pushed the feature/add-dns-entry branch 3 times, most recently from 0ec9b63 to 457866f Compare May 6, 2015 22:10

grkvlt force-pushed the feature/add-dns-entry branch from 457866f to 2fee13c Compare May 6, 2015 22:18

rade reviewed May 6, 2015
View reviewed changes

grkvlt force-pushed the feature/add-dns-entry branch from 61dabd7 to 8fad155 Compare May 7, 2015 11:10

New commands to add and remove DNS entries

633d539

- Commands dns-add and dns-remove added to weave script - Updated documentation for WeaveDNS - Added test script for dns-add and dns-remove - Added IP address functions matching existing CIDR ones

grkvlt force-pushed the feature/add-dns-entry branch from 8fad155 to 633d539 Compare May 7, 2015 11:14

rade added a commit that referenced this pull request May 7, 2015

Merge pull request #627 from grkvlt/feature/add-dns-entry

5e5e932

add `weave dns-add/remove` commands Closes #364.

rade merged commit 5e5e932 into weaveworks:master May 7, 2015

This was referenced May 7, 2015

[dns] retain extra records on restart #635

Open

[dns] support CNAMEs #636

Open

rade modified the milestone: 0.11.0 May 12, 2015

awh mentioned this pull request May 22, 2015

Inconsistent use of - vs -- for long options in weave script #602

Closed


		weave_on $HOST1 dns-remove $C2/24 c2

		assert_raises "exec_on $HOST1 c1 getent hosts $C2 \| grep $NAME"

add weave dns-add/remove commands #627

add weave dns-add/remove commands #627

Conversation

grkvlt commented May 3, 2015

grkvlt commented May 3, 2015

rade commented May 3, 2015

This comment was marked as abuse.

This comment was marked as abuse.

rade commented May 3, 2015

rade commented May 4, 2015

grkvlt commented May 4, 2015

grkvlt commented May 4, 2015

rade commented May 4, 2015

grkvlt commented May 4, 2015

This comment was marked as abuse.

rade commented May 4, 2015

grkvlt commented May 4, 2015

rade commented May 4, 2015

grkvlt commented May 4, 2015

grkvlt commented May 4, 2015

awh commented May 5, 2015

awh commented May 5, 2015

inercia commented May 5, 2015

grkvlt commented May 5, 2015

rade commented May 5, 2015

This comment was marked as abuse.

This comment was marked as abuse.

grkvlt commented May 6, 2015

grkvlt commented May 6, 2015

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

rade commented May 6, 2015

This comment was marked as abuse.

grkvlt commented May 6, 2015

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

grkvlt commented May 7, 2015

rade commented May 7, 2015

add `weave dns-add/remove` commands #627

add `weave dns-add/remove` commands #627