-
Notifications
You must be signed in to change notification settings - Fork 672
add support for ips
in ipam config
#3754
base: master
Are you sure you want to change the base?
Conversation
Thanks for the PR! Would you consider adding a test, e.g. extending https://github.com/weaveworks/weave/blob/master/test/830_cni_plugin_test.sh (the integration tests at CircleCI always fail for 3rd-party branches, but I've pushed your branch to this repo so they will run this time) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some points to consider.
plugin/ipam/cni.go
Outdated
@@ -37,7 +38,15 @@ func (i *Ipam) Allocate(args *skel.CmdArgs) (types.Result, error) { | |||
} | |||
var ipnet *net.IPNet | |||
|
|||
if conf.Subnet == "" { | |||
if len(conf.IPs) == 1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when 2 or more IPs are supplied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how we can assign multiple IP addresses to the same interface we pass as part of RuntimeConf
, when calling AddNetworkList
.
In a Kubernetes context it won't make any difference - that parameter is only used when Weave Net is configured to talk to Docker. |
@bboreham thank you for the review. I will make sure I add a test and address all your comments and questions in the coming days! |
Thanks for the test; there appear to be a couple of points still outstanding from my first review - could you respond to those? |
@bboreham sorry for the delay :( I am trying to prioritise this alongside other work. I will definitely address the rest of the issues, and ping you, when this is ready for another review! |
Hey, no apology required! I've re-pushed the branch so your new test should run in CI. |
b12d4fd
to
08afded
Compare
ipnet, err = i.weave.AllocateIP(containerID, false) | ||
var ipconfigs []*current.IPConfig | ||
|
||
if len(conf.IPs) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bboreham I am a bit confused with how to support >1 IPs passed to the CNI via a NetworkConfigList
, since the signature of AddNetworkList
is:
func (c *CNIConfig) AddNetworkList(ctx context.Context, list *NetworkConfigList, rt *RuntimeConf) (types.Result, error) {
and RuntimeConf
includes:
type RuntimeConf struct {
ContainerID string
NetNS string
IfName string
...
Can a given interface (i.e. eth1
) have multiple IP addresses? I am assuming we set IfName
to eth1
for example.
I hope that makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, an interface can have many IP addresses. These days one IPv4 and one IPv6 is quite common (though not in Weave Net).
What you did is OK, though the Weave Net CNI plugin will currently only use the first address:
Lines 85 to 86 in b04217d
// Only expecting one address | |
ip := result.IPs[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this; sorry I didn't get back to you. Some small notes below.
I was going to ask about handling IPs
in DEL, but it makes me realise that DEL is already bugged for the case that ADD is called twice (for eth0 and eth1, say), because it will release all IPs on the first DEL. I guess that should at least be filed as an issue to be addressed later.
ipnet, err = i.weave.AllocateIP(containerID, false) | ||
var ipconfigs []*current.IPConfig | ||
|
||
if len(conf.IPs) > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, an interface can have many IP addresses. These days one IPv4 and one IPv6 is quite common (though not in Weave Net).
What you did is OK, though the Weave Net CNI plugin will currently only use the first address:
Lines 85 to 86 in b04217d
// Only expecting one address | |
ip := result.IPs[0] |
No worries! I think I addressed all comments. Let me know if you think this PR should include anything else. I'd be very happy if this gets into Weave at some point, so that we can stop using a fork on Testground! |
It looks like your |
Any chance we can allow this PR to run these tests on CI? I didn't manage to get the test setup to run locally last time, and it seems like it will be fairly easy to update the code and test on CI. |
We have no automated system to allow 3rd-party PRs access to the secrets; what I generally do is manually pull your branch and push it to the Note that your test doesn't need to duplicate all the twists and turns of test 830; it just needs to make one or two calls with the IPs set and check the result. The tests do assume they can ssh into another host where Docker is running with open TCP socket. diff --git test/830_cni_plugin_test.sh test/830_cni_plugin_test.sh
index e53e988c..a8d93cc0 100755
--- test/830_cni_plugin_test.sh
+++ test/830_cni_plugin_test.sh
@@ -131,9 +131,9 @@ assert_raises "exec_on $HOST1 c1 $PING $C3IP"
# CH0_HAIRPIN=$($SSH $HOST1 cat /sys/devices/virtual/net/weave/brif/$CH0_PEER/hairpin_mode)
# assert_raises "[ $CH0_HAIRPIN == 1 ]"
#
-assert "$SSH $HOST1 cat /sys/devices/virtual/net/weave/brif/vethwepl${CH0:0:7}/hairpin_mode" "1"
-assert "$SSH $HOST1 cat /sys/devices/virtual/net/weave/brif/vethwepl${CH1:0:7}/hairpin_mode" "0"
-assert "$SSH $HOST1 cat /sys/devices/virtual/net/weave/brif/vethwepl${CH2:0:7}/hairpin_mode" "1"
+assert "cat /sys/devices/virtual/net/weave/brif/vethwepl${CH0:0:7}/hairpin_mode" "1"
+assert "cat /sys/devices/virtual/net/weave/brif/vethwepl${CH1:0:7}/hairpin_mode" "0"
+assert "cat /sys/devices/virtual/net/weave/brif/vethwepl${CH2:0:7}/hairpin_mode" "1"
@@ -141,13 +141,13 @@ assert "$SSH $HOST1 cat /sys/devices/virtual/net/weave/brif/vethwepl${CH2:0:7}/h
stop_weave_on $HOST1
# Ensure no warning is printed to the standard error:
-ACTUAL_OUTPUT=$(CHECKPOINT_DISABLE="$CHECKPOINT_DISABLE" DOCKER_HOST=tcp://$HOST1:$DOCKER_PORT $WEAVE launch 2>&1)
-EXPECTED_OUTPUT=$($SSH $HOST1 docker inspect --format="{{.Id}}" weave)
+ACTUAL_OUTPUT=$(CHECKPOINT_DISABLE="$CHECKPOINT_DISABLE" $WEAVE launch 2>&1)
+EXPECTED_OUTPUT=$(docker inspect --format="{{.Id}}" weave)
assert_raises "[ $EXPECTED_OUTPUT == $ACTUAL_OUTPUT ]"
-assert "$SSH $HOST1 \"curl -s -X GET 127.0.0.1:6784/ip/$C1 | grep -o -E '[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}'\"" "$C1IP"
-assert "$SSH $HOST1 \"curl -s -X GET 127.0.0.1:6784/ip/$C3 | grep -o -E '[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}'\"" "$C3IP"
+assert "curl -s -X GET 127.0.0.1:6784/ip/$C1 | grep -o -E '[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}'" "$C1IP"
+assert "curl -s -X GET 127.0.0.1:6784/ip/$C3 | grep -o -E '[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}'" "$C3IP"
end_suite
diff --git test/config.sh test/config.sh
index 3b599ffa..76483877 100644
--- test/config.sh
+++ test/config.sh
@@ -101,7 +101,7 @@ run_on() {
host=$1
shift 1
[ -z "$DEBUG" ] || greyly echo "Running on $host: $@" >&2
- remote $host $SSH $host "$@"
+ "$@"
}
get_command_output_on() {
@@ -115,7 +115,7 @@ docker_on() {
host=$1
shift 1
[ -z "$DEBUG" ] || greyly echo "Docker on $host:$DOCKER_PORT: $@" >&2
- docker -H tcp://$host:$DOCKER_PORT "$@"
+ docker "$@"
}
docker_api_on() {
@@ -136,7 +136,7 @@ weave_on() {
host=$1
shift 1
[ -z "$DEBUG" ] || greyly echo "Weave on $host:$DOCKER_PORT: $@" >&2
- CHECKPOINT_DISABLE="$CHECKPOINT_DISABLE" DOCKER_HOST=tcp://$host:$DOCKER_PORT $WEAVE "$@"
+ CHECKPOINT_DISABLE="$CHECKPOINT_DISABLE" $WEAVE "$@"
}
stop_weave_on() {
@@ -151,7 +151,7 @@ exec_on() {
host=$1
container=$2
shift 2
- docker -H tcp://$host:$DOCKER_PORT exec $container "$@"
+ docker exec $container "$@"
}
# Look through 'docker run' args and try to make the hostname match the name Then the run looks a little untidy because the CNI results are going to stdout, but you can see it worked:
|
@bboreham OK, thanks for the pointers, will do that and ping you when ready. |
9f20d40
to
d14fa9e
Compare
@bboreham thanks to the tips on how to run tests locally, I am able to run the test suite now! I started adding a test for this functionality, but when running
Not sure why we are not able to claim that address. I checked the following places:
I also rebased the PR onto latest |
It shows that all but 1 are available ("1 active").
|
d14fa9e
to
cc28030
Compare
case c.ident == "weave:expose": | ||
alloc.debugln("Ignoring weave:expose") | ||
c.sendResult(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bboreham I am not entirely sure what's going on here. Without this case that ignores weave:expose
, this is what I see in the weave
logs:
DEBU: 2021/01/25 17:56:56.533494 [http] PUT /ip/99cb8ae4fe35aa7992bc1ae439fcd07bc464b9ccc846ec6140d3860665adedaf/10.32.1.30/8
INFO: 2021/01/25 17:56:56.533603 Assuming quorum size of 1
DEBU: 2021/01/25 17:56:56.533612 [allocator 32:7d:23:67:8a:ff] Paxos proposing
DEBU: 2021/01/25 17:56:56.533644 [allocator 32:7d:23:67:8a:ff]: Paxos consensus: [32:7d:23:67:8a:ff]
DEBU: 2021/01/25 17:56:56.536546 [allocator 32:7d:23:67:8a:ff]: Claimed 10.32.1.30/8 for 99cb8ae4fe35aa7992bc1ae439fcd07bc464b9ccc846ec6140d3860665adedaf
DEBU: 2021/01/25 17:56:56.537963 [ring 32:7d:23:67:8a:ff]: ReportFree token=10.32.0.0 peer=32:7d:23:67:8a:ff version=1
DEBU: 2021/01/25 17:56:56.538721 [http] PUT /ip/weave:expose/10.32.1.30/8
WARN: 2021/01/25 17:56:56.540951 [allocator]: Unable to claim: address 10.32.1.30/8 is already owned by 99cb8ae4fe35aa7992bc1ae439fcd07bc464b9ccc846ec6140d3860665adedaf ; i am weave:expose
I added the i am weave:expose
part, which corresponds to c.ident
. I am not sure why func (c* claim) Try
is called with weave:expose
for ident, after an IP has been allocated to the container.
Looks like PUT /ip/weave:expose/10.32.1.30/8
is entering a check for the claim on the IP, and then complains that a container owns the IP already.
cc28030
to
32445eb
Compare
I've added a test and it is passing locally. @bboreham could you also trigger the remote CI, just to make sure that I haven't messed up the setup phase while testing locally? The test creates 3 containers with static IPs, and makes sure that these containers can see each other, and that they can't access the open internet. For some reason |
I think your main problem is you left in all sorts of logic from the other test, including Strip it down to just what you mean to test. |
Sorry, changed my mind, that line wouldn't do a I do recommend you strip down the test, but you'd need to find out where that |
Could you check if you have a pre-existing address on the Like You may want to do |
I am getting the same behaviour after doing a This happens at:
It is reproducible only with one container and no bridge. |
@@ -128,6 +128,9 @@ func (c *claim) Try(alloc *Allocator) bool { | |||
// but we also cannot prove otherwise, so we let it reclaim the address: | |||
alloc.debugln("Re-Claimed", c.cidr, "for ID", c.ident, "having existing ID as", existingIdent) | |||
c.sendResult(nil) | |||
case c.ident == "weave:expose": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bboreham Is weave:expose
even a valid identifier for this function? AFAICT this is supposed to be a container ID, so it seems like there is a bug where weave:expose
calls this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The weave expose
command allocates an address for the local machine on the Weave network.
Rather than have a whole different path for that, we pretend we are allocating an address for a container with ID weave:expose
.
I suspect the problem is happening here: Line 98 in 34de0b1
This part of the CNI plugin wants to set up a gateway address on the bridge so that egress traffic can be routed back into the Weave network. So it calls the IP allocator, with the same CNI config, so it will get an address in the correct subnet. Your new code sees I cannot see a simple fix retaining all existing behaviour; that code is deliberately not parsing the config because it wants to be independent of whatever ipam the caller has configured. Maybe we could add another ipam config to be used for gateway address allocation? I can get your test to otherwise pass by adding |
for j := range ips { | ||
ipnet := &net.IPNet{ | ||
IP: ips[j], | ||
Mask: ips[j].DefaultMask(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems wrong; the IPs are coming back as 10.32.1.42/8
rather than 10.32.1.42/12
which would match the default subnet used by Weave Net.
This PR is adding an additional field in the IPAM config for Weave -
ips
.It allows for specifying an IP address through the CNI plugin, similar to
weave attach
and claim that IP.I am not sure if it makes sense to configure also
checkAlive
or to set it totrue
at all times - currently it is set tofalse
.Since the IPAM configuration is specific to each plugin, I think this is allowed per the CNI spec. I am not sure if the choice for
ips
type is right though - this type is used in theResult
, but it seemed appropriate for the input values of the IPAM as well.TODO:
880_cni_plugin_ip_address_assignment_test.sh.sh
tests to test new functionality