upgrade_cni_plugin upgrades symlinks which are already in the correct form

[stevenjohnstone]

What you expected to happen?

What happened?

The weave script function "upgrade_cni_plugin" will attempt to upgrade symlinks even if they exist already in the correct form due to buggy use of readlink: https://github.com/weaveworks/weave/blob/master/weave#L710 and https://github.com/weaveworks/weave/blob/master/weave#L713.

The comparison should probably be something like

    if [ "$(readlink -f $1/weave-net)" != "$1/$CNI_PLUGIN_NAME" ]; then

because readlink will return an absolute path e.g. /host/opt/cni/bin/$CNI_PLUGIN_NAME.

I think this would go unnoticed if /opt/cni/bin has write permissions (most use cases, probably). I have a read-only root-filesystem with all the moving parts of weave pre-installed. I run "kubectl apply..." to setup weave. Fails due to script "weave" attempting to update the symlinks weave-ipam etc although they already exist.

How to reproduce it?

The problem lies with how "readlink" is used in upgrade_cni_plugin. Here's a minimal example with the relevant functions copied into a test script:

test.sh

#!/bin/sh
set -ex

upgrade_cni_plugin_symlink() {
    # Remove potential temporary symlink from previous failed upgrade:
    rm -f $1/$2.tmp
    # Atomically create a symlink to the plugin:
    ln -s "$CNI_PLUGIN_NAME" $1/$2.tmp && mv -f $1/$2.tmp $1/$2
}

upgrade_cni_plugin() {
    # Check if weave-net and weave-ipam are (legacy) copies of the plugin, and
    # if so remove these so symlinks can be used instead from now onwards.
    if [ -f $1/weave-net  -a ! -L $1/weave-net  ];  then rm $1/weave-net;   fi
    if [ -f $1/weave-ipam -a ! -L $1/weave-ipam ];  then rm $1/weave-ipam;  fi

    # Create two symlinks to the plugin, as it has different
    # behaviour depending on its name:
    if [ "$(readlink -f $1/weave-net)" != "$CNI_PLUGIN_NAME" ]; then
        upgrade_cni_plugin_symlink $1 weave-net
    fi
    if [ "$(readlink -f $1/weave-ipam)" != "$CNI_PLUGIN_NAME" ]; then
        upgrade_cni_plugin_symlink $1 weave-ipam
    fi
}

CNI_PLUGIN_NAME=foo
upgrade_cni_plugin /tmp

$ . test.sh 
+ . test.sh
++ set -ex
++ CNI_PLUGIN_NAME=foo
++ upgrade_cni_plugin /tmp
++ '[' -f /tmp/weave-net -a '!' -L /tmp/weave-net ']'
++ '[' -f /tmp/weave-ipam -a '!' -L /tmp/weave-ipam ']'
+++ readlink -f /tmp/weave-net
++ '[' /tmp/foo '!=' foo ']'
++ upgrade_cni_plugin_symlink /tmp weave-net
++ rm -f /tmp/weave-net.tmp
++ ln -s foo /tmp/weave-net.tmp
++ mv -f /tmp/weave-net.tmp /tmp/weave-net
+++ readlink -f /tmp/weave-ipam
++ '[' /tmp/foo '!=' foo ']'
++ upgrade_cni_plugin_symlink /tmp weave-ipam
++ rm -f /tmp/weave-ipam.tmp
++ ln -s foo /tmp/weave-ipam.tmp
++ mv -f /tmp/weave-ipam.tmp /tmp/weave-ipam

Symlinks are created at /tmp/weave-ipam and /tmp/weave-net, as expected. Run the script again:

$ . test.sh 
+ . test.sh
++ set -ex
++ CNI_PLUGIN_NAME=foo
++ upgrade_cni_plugin /tmp
++ '[' -f /tmp/weave-net -a '!' -L /tmp/weave-net ']'
++ '[' -f /tmp/weave-ipam -a '!' -L /tmp/weave-ipam ']'
+++ readlink -f /tmp/weave-net
++ '[' /tmp/foo '!=' foo ']'
++ upgrade_cni_plugin_symlink /tmp weave-net
++ rm -f /tmp/weave-net.tmp
++ ln -s foo /tmp/weave-net.tmp
++ mv -f /tmp/weave-net.tmp /tmp/weave-net
+++ readlink -f /tmp/weave-ipam
++ '[' /tmp/foo '!=' foo ']'
++ upgrade_cni_plugin_symlink /tmp weave-ipam
++ rm -f /tmp/weave-ipam.tmp
++ ln -s foo /tmp/weave-ipam.tmp
++ mv -f /tmp/weave-ipam.tmp /tmp/weave-ipam

The symlinks are recreated erroneously. With a slight modification, things behave better:

@@ -16,10 +16,10 @@
 
     # Create two symlinks to the plugin, as it has different
     # behaviour depending on its name:
-    if [ "$(readlink -f $1/weave-net)" != "$CNI_PLUGIN_NAME" ]; then
+    if [ "$(readlink -f $1/weave-net)" != "$1/$CNI_PLUGIN_NAME" ]; then
         upgrade_cni_plugin_symlink $1 weave-net
     fi
-    if [ "$(readlink -f $1/weave-ipam)" != "$CNI_PLUGIN_NAME" ]; then
+    if [ "$(readlink -f $1/weave-ipam)" != "$1/$CNI_PLUGIN_NAME" ]; then
         upgrade_cni_plugin_symlink $1 weave-ipam
     fi
 }

Running the script:

$ rm /tmp/weave-*
$ . test.sh 
++ CNI_PLUGIN_NAME=foo
++ upgrade_cni_plugin /tmp
++ '[' -f /tmp/weave-net -a '!' -L /tmp/weave-net ']'
++ '[' -f /tmp/weave-ipam -a '!' -L /tmp/weave-ipam ']'
+++ readlink -f /tmp/weave-net
++ '[' /tmp/weave-net '!=' /tmp/foo ']'
++ upgrade_cni_plugin_symlink /tmp weave-net
++ rm -f /tmp/weave-net.tmp
++ ln -s foo /tmp/weave-net.tmp
++ mv -f /tmp/weave-net.tmp /tmp/weave-net
+++ readlink -f /tmp/weave-ipam
++ '[' /tmp/weave-ipam '!=' /tmp/foo ']'
++ upgrade_cni_plugin_symlink /tmp weave-ipam
++ rm -f /tmp/weave-ipam.tmp
++ ln -s foo /tmp/weave-ipam.tmp
++ mv -f /tmp/weave-ipam.tmp /tmp/weave-ipam

Symlinks created as expected. Run again:

$ . test.sh 
+ . test.sh
++ set -ex
++ CNI_PLUGIN_NAME=foo
++ upgrade_cni_plugin /tmp
++ '[' -f /tmp/weave-net -a '!' -L /tmp/weave-net ']'
++ '[' -f /tmp/weave-ipam -a '!' -L /tmp/weave-ipam ']'
+++ readlink -f /tmp/weave-net
++ '[' /tmp/foo '!=' /tmp/foo ']'
+++ readlink -f /tmp/weave-ipam
++ '[' /tmp/foo '!=' /tmp/foo ']'

Symlinks are detected correctly.

Anything else we need to know?

Versions:

Latest

Logs:

n/a

Network:

n/a

[brb]

@stevenjohnstone Thanks for the detailed issue! It seems to be buggy indeed. Mind creating a PR?