Merge ~smoser/cloud-init:bug/1712680-maas-use-token-for-check-instance-id into cloud-init:master

[Scott Moser]

MAAS: add check_instance_id based off oauth tokens.

This stores a hash of the OAuth tokens as an 'id' for the maas
datasource. Since new instances get new tokens created and those tokens
are written by curtin into datasource system config this will provide
a way to identify a new "instance" (install).

LP: #1712680

[Scott Moser (smoser)]

no tests, but I'm interested in thoughts.
We could make 'id_hash' a property since ds_cfg gets stored.

Chad I'm interested in your thoughts on this and the unification of storage, how did I affect that?

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:cefcd130bfb452453fbfd56c83dd175de258c620
https://jenkins.ubuntu.com/server/job/cloud-init-ci/624/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/624/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

I know this is a completely separate direction than you are taking.

It feels a bit like we are gating all cloud-init updates/runs based on whether the instance-id changes. In a world where cloud-init becomes re-entrany (or hotplug handling) should cloud-init grow the ability for people to supplement cloud-init's written content, with more information?

I'm thinking like bookend comments like this.
# begin cloud-init ignore
# end cloud-init ignore

In either case, I'll try deploying a node twice with reboots in between and make sure this only changes on node re-install. It feels like we are working around a problem by relying on instance-id as our gate for rewriting content. Should cloud-init ever accept

[Chad Smith (chad.smith)]

Another question I have is across reboots (same instance) do we worry about MAAS changing network topology between boots? If so, locking network setup permanently until re-deploy would cause breakage

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:2543c765cba201570f697ccf5d5efc5a75b65322
https://jenkins.ubuntu.com/server/job/cloud-init-ci/625/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/625/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Looking at a deployed node, the instance id is node-<uuid>;

IIUC, upon reboot, the MAAS Datasource url is re-crawled, and in that metadata url, it has an instance-id; and if that matches the 'previous-iid' that's on disk, then we assume it's a reboot of the same instance.

Then, instead of MAAS including the instance-id (node-uuid) bit in the original configuration that's written out to /etc/cloud/cloud.cfg.d/90-dpkg_maas.cfg or where-ever) we're combining the elements of the configuration into a hash.

That's pretty neat.

--
Now, w.r.t the code. My first worry was about upgrade. Since we always throw away the obj.pkl until this version; I don't think we'll have to worry about loading an object that doesn't have those internal members; however if someone were to "manually run cloud-init init" again, then I think we have that same issue here. We should handle that just because we're aware of that.

review: Needs Fixing

[Ryan Harper (raharper)]

> I know this is a completely separate direction than you are taking.
>
> It feels a bit like we are gating all cloud-init updates/runs based on whether
> the instance-id changes. In a world where cloud-init becomes re-entrany (or
> hotplug handling) should cloud-init grow the ability for people to supplement
> cloud-init's written content, with more information?
>
> I'm thinking like bookend comments like this.
> # begin cloud-init ignore
> # end cloud-init ignore

I'm not sure a book-end sort of comment is going to be sufficient; generally
there are config management engines that handle this sort of thing better.
However, I do think we can look toward creating idempotent config modules which
can check if they need to do anything w.r.t updates to files, versus blind rendering.

>
> In either case, I'll try deploying a node twice with reboots in between and
> make sure this only changes on node re-install. It feels like we are working
> around a problem by relying on instance-id as our gate for rewriting content.
> Should cloud-init ever accept

I think that's generally the design of instance-id; I don't think that's an issue.
In the event we want to react to changes in user-data/metadata then I think the
code that renders based on the metadata will need to handle that it may be called
many times, rather than once-per instance.

[Ryan Harper (raharper)]

> Another question I have is across reboots (same instance) do we worry about
> MAAS changing network topology between boots? If so, locking network setup
> permanently until re-deploy would cause breakage

MAAS , AFAIK, does not change/update metadata (including network configuration) between
deployments of a node. The node is allocated and deployed and remains the same (from a metadata perspective) until the node is released from MAAS for a new deployment.

[Scott Moser (smoser)]

@Chad, @Ryan
We do need to have way to allow user updates to a file without blowing
them away. This is non-trivial for sure.

That said, rendering some things that cloud-init "owns" on first instance
is necessary. If the user had made edits to files on an old instance,
with possibly instance-specific data inside it then it is quite likely
that those modifications are not consistent with the new instance. One
solution we had to this (/etc/hosts, /etc/apt/sources.list) is to render
the files from a template, and to allow the user to modify the template
source then if they want to make changes that persist across an instance,
they change the template.

network config wasn't really ever meant to be "multi-party updatable", so
this is non-trivial in this case. netplan has some sort of plan for that
(multiple .yaml files in /etc/netplan) but the reality is that it will
still be non-trivial. Multi-party modification of *anything* is non-
trivial.

@Ryan,
I'm not sure weather or not this hits the attribute missing-on-second-run
bug that we saw in bug 1732917. I'll check to make sure.

Over all, this is a simple-ish fix that does dramatically improve the
state of maas reboot scenario. It does come at the cost of now
requiring the user to "clean" the image if they want to take the image
out of MAAS, but that isn't actually a reality at this point that I'm
aware of.

[Scott Moser (smoser)]

I played a bit and I'm pretty sure this is safe
Heres an example which can be built on,b ut shows store and reload with changing class.

  http://paste.ubuntu.com/26177197/

[Ryan Harper (raharper)]

After discussion, the use of self.hash_id is safe because we don't expect a non-none value as we did in other cases of restored object code.

While discussing this in IRC, discovered that self.hash_id isn't set in get_data()
That needs doing and unittests.

Otherwise, it looks good.

13e000d... by Scott Moser on 2017-12-13

actually store the original value

3a3d6fc... by Scott Moser on 2017-12-21

address chad's feedback

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:ed4c0c24c1f19140432180a22e7069e2ee49f9ba
https://jenkins.ubuntu.com/server/job/cloud-init-ci/653/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/653/rebuild

review: Needs Fixing (continuous-integration)

886ad31... by Scott Moser on 2018-01-08

fix issues raised by c-i review, add some tests.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:7cef59571b99857ddb5241c5d8b4f3a982423763
https://jenkins.ubuntu.com/server/job/cloud-init-ci/671/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/671/rebuild

review: Approve (continuous-integration)

c726f64... by Scott Moser on 2018-01-08

add tests of v1 hash

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:f55dd0e0f67b1dc7fbd92c4cea7b06e312111139
https://jenkins.ubuntu.com/server/job/cloud-init-ci/672/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/672/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

review: Needs Information

b5264bd... by Scott Moser 11 hours ago

remove warning / dropping of oauth

previously if the user did not provide all required fields,
the 'get_oauth_helper' would just drop to non-oauth (and warn).
that just seems odd. now instead, that will go through to the
OauthHelper constructor and raise a ValueError.

[Server Team CI bot (server-team-bot)]

FAILED: Continuous integration, rev:cefe421e2ed7b4fcb52f10e0ce1b4d3a8c6ee1bd
https://jenkins.ubuntu.com/server/job/cloud-init-ci/681/
Executed test runs:
    SUCCESS: Checkout
    FAILED: Unit & Style Tests

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/681/rebuild

review: Needs Fixing (continuous-integration)

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:b5264bdb06669f956f6cc4fd719e38aefc6310b8
https://jenkins.ubuntu.com/server/job/cloud-init-ci/682/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/682/rebuild

review: Approve (continuous-integration)

[Chad Smith (chad.smith)]

+1 just a minor docstr fix request

[Chad Smith (chad.smith)]

review: Approve

ae66049... by Scott Moser 6 hours ago

fix docstring

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:ae66049ceee919e364b1054cdd782e41e9b68f44
https://jenkins.ubuntu.com/server/job/cloud-init-ci/684/
Executed test runs:
    SUCCESS: Checkout
    SUCCESS: Unit & Style Tests
    SUCCESS: Ubuntu LTS: Build
    SUCCESS: Ubuntu LTS: Integration
    SUCCESS: MAAS Compatability Testing
    IN_PROGRESS: Declarative: Post Actions

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/cloud-init-ci/684/rebuild

review: Approve (continuous-integration)

Unmerged commits

ae66049... by Scott Moser 6 hours ago

fix docstring

b5264bd... by Scott Moser 11 hours ago

remove warning / dropping of oauth

previously if the user did not provide all required fields,
the 'get_oauth_helper' would just drop to non-oauth (and warn).
that just seems odd. now instead, that will go through to the
OauthHelper constructor and raise a ValueError.

c726f64... by Scott Moser on 2018-01-08

add tests of v1 hash

886ad31... by Scott Moser on 2018-01-08

fix issues raised by c-i review, add some tests.

3a3d6fc... by Scott Moser on 2017-12-21

address chad's feedback

13e000d... by Scott Moser on 2017-12-13

actually store the original value

d47aedd... by Scott Moser on 2017-12-12

MAAS: add check_instance_id based off oauth tokens.

This stores a hash of the OAuth tokens as an 'id' for the maas
datasource. Since new instances get new tokens created and those tokens
are written by curtin into datasource system config this will provide
a way to identify a new "instance" (install).

LP: #1712680

diff --git a/cloudinit/sources/DataSourceMAAS.py b/cloudinit/sources/DataSourceMAAS.py
index 496bd06..6ac8863 100644
--- a/cloudinit/sources/DataSourceMAAS.py
+++ b/cloudinit/sources/DataSourceMAAS.py
@@ -8,6 +8,7 @@
 
 from __future__ import print_function
 
+import hashlib
 import os
 import time
 
@@ -41,25 +42,20 @@ class DataSourceMAAS(sources.DataSource):
     """
 
     dsname = "MAAS"
+    id_hash = None
+    _oauth_helper = None
 
     def __init__(self, sys_cfg, distro, paths):
         sources.DataSource.__init__(self, sys_cfg, distro, paths)
         self.base_url = None
         self.seed_dir = os.path.join(paths.seed_dir, 'maas')
-        self.oauth_helper = self._get_helper()
+        self.id_hash = get_id_from_ds_cfg(self.ds_cfg)
 
-    def _get_helper(self):
-        mcfg = self.ds_cfg
-        # If we are missing token_key, token_secret or consumer_key
-        # then just do non-authed requests
-        for required in ('token_key', 'token_secret', 'consumer_key'):
-            if required not in mcfg:
-                return url_helper.OauthUrlHelper()
-
-        return url_helper.OauthUrlHelper(
-            consumer_key=mcfg['consumer_key'], token_key=mcfg['token_key'],
-            token_secret=mcfg['token_secret'],
-            consumer_secret=mcfg.get('consumer_secret'))
+    @property
+    def oauth_helper(self):
+        if not self._oauth_helper:
+            self._oauth_helper = get_oauth_helper(self.ds_cfg)
+        return self._oauth_helper
 
     def __str__(self):
         root = sources.DataSource.__str__(self)
@@ -147,6 +143,36 @@ class DataSourceMAAS(sources.DataSource):
 
         return bool(url)
 
+    def check_instance_id(self, sys_cfg):
+        """locally check if the current system is the same instance.
+
+        MAAS doesn't provide a real instance-id, and if it did, it is
+        still only available over the network.  We need to check based
+        only on local resources.  So compute a hash based on Oauth tokens."""
+        if self.id_hash is None:
+            return False
+        ncfg = util.get_cfg_by_path(sys_cfg, ("datasource", self.dsname), {})
+        return (self.id_hash == get_id_from_ds_cfg(ncfg))
+
+
+def get_oauth_helper(cfg):
+    """Return an oauth helper instance for values in cfg.
+
+       @raises ValueError from OauthUrlHelper if some required fields have
+               true-ish values but others do not."""
+    keys = ('consumer_key', 'consumer_secret', 'token_key', 'token_secret')
+    kwargs = dict([(r, cfg.get(r)) for r in keys])
+    return url_helper.OauthUrlHelper(**kwargs)
+
+
+def get_id_from_ds_cfg(ds_cfg):
+    """Given a config, generate a unique identifier for this node."""
+    fields = ('consumer_key', 'token_key', 'token_secret')
+    idstr = '\0'.join([ds_cfg.get(f, "") for f in fields])
+    # store the encoding version as part of the hash in the event
+    # that it ever changed we can compute older versions.
+    return 'v1:' + hashlib.sha256(idstr.encode('utf-8')).hexdigest()
+
 
 def read_maas_seed_dir(seed_d):
     if seed_d.startswith("file://"):
@@ -322,7 +348,7 @@ if __name__ == "__main__":
             sys.stderr.write("Must provide a url or a config with url.\n")
             sys.exit(1)
 
-        oauth_helper = url_helper.OauthUrlHelper(**creds)
+        oauth_helper = get_oauth_helper(creds)
 
         def geturl(url):
             # the retry is to ensure that oauth timestamp gets fixed
diff --git a/tests/unittests/test_datasource/test_maas.py b/tests/unittests/test_datasource/test_maas.py
index 289c6a4..6e4031c 100644
--- a/tests/unittests/test_datasource/test_maas.py
+++ b/tests/unittests/test_datasource/test_maas.py
@@ -1,6 +1,7 @@
 # This file is part of cloud-init. See LICENSE file for license information.
 
 from copy import copy
+import mock
 import os
 import shutil
 import tempfile
@@ -8,15 +9,10 @@ import yaml
 
 from cloudinit.sources import DataSourceMAAS
 from cloudinit import url_helper
-from cloudinit.tests.helpers import TestCase, populate_dir
+from cloudinit.tests.helpers import CiTestCase, populate_dir
 
-try:
-    from unittest import mock
-except ImportError:
-    import mock
 
-
-class TestMAASDataSource(TestCase):
+class TestMAASDataSource(CiTestCase):
 
     def setUp(self):
         super(TestMAASDataSource, self).setUp()
@@ -159,4 +155,47 @@ class TestMAASDataSource(TestCase):
         self.assertEqual(valid['meta-data/instance-id'], md['instance-id'])
         self.assertEqual(expected_vd, vd)
 
+
[e-mail]("cloudinit.sources.DataSourceMAAS.url_helper.OauthUrlHelper")
+class TestGetOauthHelper(CiTestCase):
+    with_logs = True
+    base_cfg = {'consumer_key': 'FAKE_CONSUMER_KEY',
+                'token_key': 'FAKE_TOKEN_KEY',
+                'token_secret': 'FAKE_TOKEN_SECRET',
+                'consumer_secret': None}
+
+    def test_all_required(self, m_helper):
+        """Valid config as expected."""
+        DataSourceMAAS.get_oauth_helper(self.base_cfg.copy())
+        m_helper.assert_has_calls([mock.call(**self.base_cfg)])
+
+    def test_other_fields_not_passed_through(self, m_helper):
+        """Only relevant fields are passed through."""
+        mycfg = self.base_cfg.copy()
+        mycfg['unrelated_field'] = 'unrelated'
+        DataSourceMAAS.get_oauth_helper(mycfg)
+        m_helper.assert_has_calls([mock.call(**self.base_cfg)])
+
+
+class TestGetIdHash(CiTestCase):
+    v1_cfg = {'consumer_key': 'CKEY', 'token_key': 'TKEY',
+              'token_secret': 'TSEC'}
+    v1_id = (
+        'v1:'
+        '403ee5f19c956507f1d0e50814119c405902137ea4f8838bde167c5da8110392')
+
+    def test_v1_expected(self):
+        """Test v1 id generated as expected working behavior from config."""
+        result = DataSourceMAAS.get_id_from_ds_cfg(self.v1_cfg.copy())
+        self.assertEqual(self.v1_id, result)
+
+    def test_v1_extra_fields_are_ignored(self):
+        """Test v1 id ignores unused entries in config."""
+        cfg = self.v1_cfg.copy()
+        cfg['consumer_secret'] = "BOO"
+        cfg['unrelated'] = "HI MOM"
+        result = DataSourceMAAS.get_id_from_ds_cfg(cfg)
+        self.assertEqual(self.v1_id, result)
+
+
 # vi: ts=4 expandtab