Merge ~jdkandersson/jenkins-agent-charm:switch-to-agent-managed-jnlp-download into jenkins-agent-charm:master

[Johann David Krister Andersson]

Use agent.jar to download jnlp file rather than download it separately

Newer versions of Jenkins no longer allow the downloading of the JNLP without authentication which is why the Jenkins agent machine charm fails to start up. This fix gets the agent.jar file to perform the download by passing an additional parameter to it.

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

This merge proposal is being monitored by mergebot. Change the status to Approved to merge.

[Arturo Enrique Seijas Fernández (arturo-seijas)]

LGTM

review: Approve

[Haw Loeung (hloeung)]

Per Mattermost, is the restriction on the ability to download JNLP's documented somewhere on the Jenkins site?

Would this continue to work or break if we were to roll out the latest charm changes to an existing environment running an older Jenkins version?

[Johann David Krister Andersson (jdkandersson)]

> Per Mattermost, is the restriction on the ability to download JNLP's
> documented somewhere on the Jenkins site?
>
> Would this continue to work or break if we were to roll out the latest charm
> changes to an existing environment running an older Jenkins version?

I found some notes in a Jira issue: https://issues.jenkins.io/browse/JENKINS-16273?page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel&showAll=true

I have tested this against all versions of Jenkins running in production (back to 2.150)

[Haw Loeung (hloeung)]

Comments inline. It's a lengthy diff. Can we split out the various whitespace and formatting changes to a different merge proposal (and make it either a pre-req. of for this or this a pre-req. for that)?

[Haw Loeung (hloeung)]

LGTM

review: Approve (+1)

[🤖 Canonical IS Merge Bot (canonical-is-mergebot)]

Change successfully merged at revision 0f5d1b9c7dbaee2ecc2fec4093ca5c53b5b44b79

diff --git a/.gitignore b/.gitignore
index ceaa0a2..7682ff5 100644
--- a/.gitignore
+++ b/.gitignore
@@ -9,3 +9,5 @@ __pycache__/
 builds/
 deps/
 revision
+.vscode
+*.charm
diff --git a/charmcraft.yaml b/charmcraft.yaml
new file mode 100644
index 0000000..b5b7ae1
--- /dev/null
+++ b/charmcraft.yaml
@@ -0,0 +1,17 @@
+type: "charm"
+bases:
+  - build-on:
+    - name: "ubuntu"
+      channel: "20.04"
+    run-on:
+    - name: "ubuntu"
+      channel: "16.04"
+    - name: "ubuntu"
+      channel: "18.04"
+    - name: "ubuntu"
+      channel: "20.04"
+parts:
+  charm:
+    source: .
+    plugin: reactive
+    build-snaps: [charm]
diff --git a/config.yaml b/config.yaml
index 50380ce..de60423 100644
--- a/config.yaml
+++ b/config.yaml
@@ -26,3 +26,12 @@ options:
     description: |
       A comma-separated list of nagios servicegroups.
       If left empty, the nagios_context will be used as the servicegroup
+  download_jnlp_file:
+    default: false
+    type: boolean
+    description: |
+      When `true`, use the DEPRECATED agent startup method that downloads the
+      JNLP file before starting the agent. The JNLP endpoint requires
+      authentication in newer versions of Jenkins meaning that the JNLP
+      download will fail. This parameter is deprecated and will be removed in
+      future versions of this charm.
diff --git a/files/download-slave.sh b/files/download-slave.sh
index dfe1462..60097e2 100755
--- a/files/download-slave.sh
+++ b/files/download-slave.sh
@@ -7,7 +7,8 @@
 SLAVE_JAR=/var/run/jenkins/slave.jar
 JENKINS_URL=$1
 JENKINS_HOSTNAME=$2
-JENKINS_JNLP=$3
+JENKINS_JNLP_FILE=$3
+DOWNLOAD_JNLP_FILE=$4
 
 if [ -z "$JENKINS_URL" ]
 then
@@ -15,20 +16,31 @@ then
     exit 1
 fi
 
+# Retrieve the JNLP file from Jenkins
+if [ "$DOWNLOAD_JNLP_FILE" = "True" ]
+then
+    echo "Downloading agent.jnlp from ${JENKINS_URL}..."
+    wget -q ${JENKINS_URL}/computer/${JENKINS_HOSTNAME}/jenkins-agent.jnlp -O ${JENKINS_JNLP_FILE}.new  || wget -q ${JENKINS_URL}/computer/${JENKINS_HOSTNAME}/slave-agent.jnlp -O ${JENKINS_JNLP_FILE}.new
+    sed -i -r "s|(<argument>-url</argument>.*</argument>)|\1<argument>-url</argument><argument>${JENKINS_URL}</argument>|g" ${JENKINS_JNLP_FILE}.new
+
+    # Check to make sure that JNLP file was downloaded
+    if [ -s ${JENKINS_JNLP_FILE}.new ]
+    then
+        mv ${JENKINS_JNLP_FILE}.new ${JENKINS_JNLP_FILE}
+    else
+        echo "Unable to download the JNLP file"
+        exit 1
+    fi
+fi
+
 # Retrieve Slave JAR from Jenkins
 echo "Downloading slave.jar from ${JENKINS_URL}..."
 wget -q -O ${SLAVE_JAR}.new ${JENKINS_URL}/jnlpJars/slave.jar
 
-# Retrive JNLP file from Jenkins
-echo "Downloading agent.jnlp from ${JENKINS_URL}..."
-wget -q ${JENKINS_URL}/computer/${JENKINS_HOSTNAME}/jenkins-agent.jnlp -O ${JENKINS_JNLP}.new  || wget -q ${JENKINS_URL}/computer/${JENKINS_HOSTNAME}/slave-agent.jnlp -O ${JENKINS_JNLP}.new
-sed -i -r "s|(<argument>-url</argument>.*</argument>)|\1<argument>-url</argument><argument>${JENKINS_URL}</argument>|g" ${JENKINS_JNLP}.new 
-
-# Check to make sure slave.jar and agent.jnlp were downloaded.
-if [ -s ${SLAVE_JAR}.new ] && [ -s ${JENKINS_JNLP}.new ]
+# Check to make sure slave.jar was downloaded.
+if [ -s ${SLAVE_JAR}.new ]
 then
     mv ${SLAVE_JAR}.new ${SLAVE_JAR}
-    mv ${JENKINS_JNLP}.new ${JENKINS_JNLP}
     exit 0
 else
     exit 1
diff --git a/files/jenkins-slave-systemd-config b/files/jenkins-slave-systemd-config
index 8148aaa..11eac18 100644
--- a/files/jenkins-slave-systemd-config
+++ b/files/jenkins-slave-systemd-config
@@ -11,7 +11,7 @@ Type=simple
 # Always be Jenkinsing!
 Restart=always
 RestartSec=3
-ExecStartPre=/bin/bash -c '[ -r /etc/default/jenkins-slave ] && . /etc/default/jenkins-slave ; [ -n "$JENKINS_URL" ] || { exit 1; };  mkdir $JENKINS_RUN  > /dev/null 2>&1  || true ; chown -R $JENKINS_USER $JENKINS_RUN || true ; /usr/local/sbin/download-slave.sh $JENKINS_URL $JENKINS_HOSTNAME $JENKINS_JNLP; chown -R $JENKINS_USER $JENKINS_HOME || true'
+ExecStartPre=/bin/bash -c '[ -r /etc/default/jenkins-slave ] && . /etc/default/jenkins-slave ; [ -n "$JENKINS_URL" ] || { exit 1; };  mkdir $JENKINS_RUN  > /dev/null 2>&1  || true ; chown -R $JENKINS_USER $JENKINS_RUN || true ; /usr/local/sbin/download-slave.sh $JENKINS_URL $JENKINS_HOSTNAME $JENKINS_JNLP_FILE $DOWNLOAD_JNLP_FILE; chown -R $JENKINS_USER $JENKINS_HOME || true'
 ExecStart=/bin/bash -c '[ -r /etc/default/jenkins-slave ] && . /etc/default/jenkins-slave ; exec start-stop-daemon --start -c $JENKINS_USER --exec $JAVA --name jenkins-slave -- $JAVA_ARGS -jar $JENKINS_RUN/slave.jar $JENKINS_ARGS'
 
 [Install]
diff --git a/reactive/jenkins_slave.py b/reactive/jenkins_slave.py
index dcb947d..91f281c 100644
--- a/reactive/jenkins_slave.py
+++ b/reactive/jenkins_slave.py
@@ -93,10 +93,10 @@ def configure_jenkins_slave():
 
     if config.get('master_url'):
         status.maintenance("Using 'master_url' to configure the slave.")
-        write_default_conf(config.get('master_url'))
+        write_default_conf(config.get('master_url'), download_jnlp_file=config.get('download_jnlp_file'))
     elif kv.get('url'):
         status.maintenance("Using url from relation as 'master_url'")
-        write_default_conf(kv.get('url'), secret=kv.get('secret'))
+        write_default_conf(kv.get('url'), secret=kv.get('secret'), download_jnlp_file=config.get('download_jnlp_file'))
     else:
         status.maintenance("No 'master_url' set; not configuring slave at this time.")
         status.blocked("requires either slave relation or 'master_url'")
@@ -191,7 +191,7 @@ def slave_relation():
         secret = hookenv.relation_get('secret')
         kv.set('url', url)
         kv.set('secret', secret)
-        write_default_conf(url, secret=secret)
+        write_default_conf(url, secret=secret, download_jnlp_file=config.get('download_jnlp_file'))
     else:
         hookenv.log("Master hasn't exported its url yet, exiting...")
         return
@@ -233,14 +233,14 @@ def file_to_units(local_path, unit_path, perms=None, owner='root', group='root')
 
 
 def write_default_conf(
-    master_url=None, owner='root', group='root', conf_path='/etc/default/jenkins-slave', secret=None
+    master_url=None, owner='root', group='root', conf_path='/etc/default/jenkins-slave', secret=None, download_jnlp_file=False
 ):
     templates_dir = os.path.join(hookenv.charm_dir(), 'templates')
     slave_host = hookenv.local_unit().replace('/', '-')
     templating.render(
         'jenkins-slave-default',
         conf_path,
-        {'master_url': master_url, 'slave_host': slave_host, 'secret': secret},
+        {'master_url': master_url, 'slave_host': slave_host, 'secret': secret, 'download_jnlp_file': download_jnlp_file},
         owner=owner,
         group=group,
         perms=0o444,
diff --git a/templates/jenkins-slave-default b/templates/jenkins-slave-default
index b3b2148..0fa9937 100644
--- a/templates/jenkins-slave-default
+++ b/templates/jenkins-slave-default
@@ -60,14 +60,20 @@ JENKINS_SLAVE_LOG=/var/log/jenkins/$NAME.log
 # to change the OS limits setup.
 MAXOPENFILES=8192
 
-# Jenkins jnlp file
-JENKINS_JNLP="${JENKINS_HOME}/agent.jnlp"
+# Jenkins jnlp URL
+DOWNLOAD_JNLP_FILE="{{ download_jnlp_file }}"
+JENKINS_JNLP_FILE="${JENKINS_HOME}/agent.jnlp"
+{% if download_jnlp_file %}
+JENKINS_JNLP_URL="file:${JENKINS_JNLP_FILE}"
+{% else %}
+JENKINS_JNLP_URL="${JENKINS_URL}/computer/${JENKINS_HOSTNAME}/slave-agent.jnlp"
+{% endif %}
 
 # Arguments to pass to jenkins slave on startup
 {% if secret %}
 JENKINS_SLAVE_SECRET="{{ secret }}"
 
-JENKINS_ARGS="-jnlpUrl file:${JENKINS_JNLP} -secret ${JENKINS_SLAVE_SECRET}"
+JENKINS_ARGS="-jnlpUrl ${JENKINS_JNLP_URL} -secret ${JENKINS_SLAVE_SECRET} -workDir ${JENKINS_HOME}"
 {% else %}
-JENKINS_ARGS="-jnlpUrl file:${JENKINS_JNLP}"
+JENKINS_ARGS="-jnlpUrl ${JENKINS_JNLP_URL} -workDir ${JENKINS_HOME}"
 {% endif %}
diff --git a/tests/unit/test_jenkins_slave.py b/tests/unit/test_jenkins_slave.py
index 1300c73..6d32481 100644
--- a/tests/unit/test_jenkins_slave.py
+++ b/tests/unit/test_jenkins_slave.py
@@ -201,10 +201,10 @@ class TestSetDefaultConf(unittest.TestCase):
     def test_configure_jenkins_slave_master_url(
         self, file_to_units, write_default_conf, unitdata_kv, config, set_flag, clear_flag
     ):
-        config.return_value = {'master_url': 'http://10.1.1.1:8080'}
+        config.return_value = {'master_url': 'http://10.1.1.1:8080', 'download_jnlp_file': False}
         unitdata_kv.return_value = {}
         jenkins_slave.configure_jenkins_slave()
-        self.assertFalse(write_default_conf.assert_called_once_with('http://10.1.1.1:8080'))
+        self.assertFalse(write_default_conf.assert_called_once_with('http://10.1.1.1:8080', download_jnlp_file=False))
         self.assertFalse(set_flag.assert_has_calls([mock.call('jenkins-slave.configured')], any_order=True))
         expected = [mock.call('jenkins-slave.active'), mock.call('nagios-nrpe.configured')]
         self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True))
@@ -218,11 +218,11 @@ class TestSetDefaultConf(unittest.TestCase):
     def test_configure_jenkins_slave_relation_url(
         self, file_to_units, write_default_conf, unitdata_kv, config, set_flag, clear_flag
     ):
-        config.return_value = {}
+        config.return_value = {'download_jnlp_file': False}
         unitdata_kv.return_value = {'url': 'http://10.22.22.22:8080'}
         jenkins_slave.configure_jenkins_slave()
         self.assertTrue(write_default_conf.called_once_with('http://10.22.22.22:8080'))
-        self.assertEqual(write_default_conf.call_args_list, [mock.call('http://10.22.22.22:8080', secret=None)])
+        self.assertEqual(write_default_conf.call_args_list, [mock.call('http://10.22.22.22:8080', secret=None, download_jnlp_file=False)])
         self.assertFalse(set_flag.assert_has_calls([mock.call('jenkins-slave.configured')], any_order=True))
         expected = [mock.call('jenkins-slave.active'), mock.call('nagios-nrpe.configured')]
         self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True))
@@ -236,10 +236,10 @@ class TestSetDefaultConf(unittest.TestCase):
     def test_configure_jenkins_slave_relation_url_with_secret(
         self, file_to_units, write_default_conf, unitdata_kv, config, set_flag, clear_flag
     ):
-        config.return_value = {}
+        config.return_value = {'download_jnlp_file': False}
         unitdata_kv.return_value = {'url': 'http://10.22.22.22:8080', 'secret': 'sekrit'}
         jenkins_slave.configure_jenkins_slave()
-        self.assertEqual(write_default_conf.call_args_list, [mock.call('http://10.22.22.22:8080', secret='sekrit')])
+        self.assertEqual(write_default_conf.call_args_list, [mock.call('http://10.22.22.22:8080', secret='sekrit', download_jnlp_file=False)])
         self.assertFalse(set_flag.assert_has_calls([mock.call('jenkins-slave.configured')], any_order=True))
         expected = [mock.call('jenkins-slave.active'), mock.call('nagios-nrpe.configured')]
         self.assertFalse(clear_flag.assert_has_calls(expected, any_order=True))