Merge ~smoser/cloud-init:fix/brightbox-less-matchy into cloud-init:master

[Scott Moser]

Brightbox: restrict detection to require full domain match .brightbox.com

The detection for brightbox in both ds-identify and in
identify_brightbox would incorrectly match the domain 'bobrightbox',
which is not a brightbox platform. The fix here is to restrict
matching to '*.brightbox.com' rather than '*brightbox.com'

Also, while here remove a url to bug 1661693 which added the
knowledge of brightbox.

see commit message

[Scott Moser (smoser)]

Just for reference, bug 1661693 has some real collected output that shows:

$ ( cd /sys/class/dmi/id && sudo grep -r . . ) 2>/dev/null
./product_serial:srv-fajvm.gb1.brightbox.com
./bios_vendor:Seabios
./product_version:RHEL 6.6.0 PC
./power/runtime_active_kids:0
./power/runtime_suspended_time:0
./power/runtime_enabled:disabled
./power/runtime_active_time:0
./power/control:auto
./power/async:disabled
./power/runtime_usage:0
./power/runtime_status:unsupported
./chassis_vendor:Red Hat
./modalias:dmi:bvnSeabios:bvr0.5.1:bd01/01/2007:svnRedHat:pnKVM:pvrRHEL6.6.0PC:cvnRedHat:ct1:cvr:
./product_uuid:D5BF3AE9-7D9C-2923-0B1E-B0ED967E7DF0
./bios_version:0.5.1
./sys_vendor:Red Hat
./chassis_type:1
./uevent:MODALIAS=dmi:bvnSeabios:bvr0.5.1:bd01/01/2007:svnRedHat:pnKVM:pvrRHEL6.6.0PC:cvnRedHat:ct1:cvr:
./product_name:KVM
./bios_date:01/01/2007

[Scott Moser (smoser)]

I happened to see this false positive when reviewing the add of zstack under
Bug 1841181.
  https://code.launchpad.net/~ruansx/cloud-init/+git/cloud-init/+merge/372445

[Dan Watkins (oddbloke)]

review: Approve

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

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

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

This looks good to me. One in-line question about filing a bug and updating the comment in ds-identify to reference this issue rather than the original brightbox bug.

[Neil Wilson (neil-aldur)]

Could you put actual product serials in the tests? For example

srv-otuxg.gb1.brightbox.com
srv-4mhaz.gb1.brightbox.com

It probably doesn't matter - until it does

[Scott Moser (smoser)]

> Could you put actual product serials in the tests? For example
>
> srv-otuxg.gb1.brightbox.com
> srv-4mhaz.gb1.brightbox.com
>
> It probably doesn't matter - until it does

Done!

[Scott Moser (smoser)]

Neil, thanks for the quick review.

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

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

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

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I'd like feedback from Ryan on the bug url.

I just now dropped it.
Complain about that if you'd like, or mark Approved.

Thanks.

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

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

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

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

I'd still prefer a new bug and comment.

review: Approve

[Scott Moser (smoser)]

Just for the record, I'm not at all opposed to having opened a bug that stated this match was too wide. That was a good idea.

I was opposed to updating a comment in the code that pointed to a bug. I'd prefer to *not* have comments referring to external resources in the code itself. Rather cloud-init shoudl:
 a.) have good commit messages that reference bugs
 b.) have revision control history that can show those bugs
 c.) have documentation on datasources in doc/ that has links to official external resources where available.

[Scott Moser (smoser)]

Thanks for merging.

diff --git a/cloudinit/sources/DataSourceEc2.py b/cloudinit/sources/DataSourceEc2.py
index 5c017bf..1010745 100644
--- a/cloudinit/sources/DataSourceEc2.py
+++ b/cloudinit/sources/DataSourceEc2.py
@@ -473,7 +473,7 @@ def identify_aws(data):
 
 
 def identify_brightbox(data):
-    if data['serial'].endswith('brightbox.com'):
+    if data['serial'].endswith('.brightbox.com'):
         return CloudNames.BRIGHTBOX
 
 
diff --git a/tests/unittests/test_ds_identify.py b/tests/unittests/test_ds_identify.py
index 587e699..de87be2 100644
--- a/tests/unittests/test_ds_identify.py
+++ b/tests/unittests/test_ds_identify.py
@@ -195,6 +195,10 @@ class DsIdentifyBase(CiTestCase):
         return self._check_via_dict(
             data, RC_FOUND, dslist=[data.get('ds'), DS_NONE])
 
+    def _test_ds_not_found(self, name):
+        data = copy.deepcopy(VALID_CFG[name])
+        return self._check_via_dict(data, RC_NOT_FOUND)
+
     def _check_via_dict(self, data, rc, dslist=None, **kwargs):
         ret = self._call_via_dict(data, **kwargs)
         good = False
@@ -244,9 +248,13 @@ class TestDsIdentify(DsIdentifyBase):
         self._test_ds_found('Ec2-xen')
 
     def test_brightbox_is_ec2(self):
-        """EC2: product_serial ends with 'brightbox.com'"""
+        """EC2: product_serial ends with '.brightbox.com'"""
         self._test_ds_found('Ec2-brightbox')
 
+    def test_bobrightbox_is_not_brightbox(self):
+        """EC2: bobrightbox.com in product_serial is not brightbox'"""
+        self._test_ds_not_found('Ec2-brightbox-negative')
+
     def test_gce_by_product_name(self):
         """GCE identifies itself with product_name."""
         self._test_ds_found('GCE')
@@ -724,7 +732,11 @@ VALID_CFG = {
     },
     'Ec2-brightbox': {
         'ds': 'Ec2',
-        'files': {P_PRODUCT_SERIAL: 'facc6e2f.brightbox.com\n'},
+        'files': {P_PRODUCT_SERIAL: 'srv-otuxg.gb1.brightbox.com\n'},
+    },
+    'Ec2-brightbox-negative': {
+        'ds': 'Ec2',
+        'files': {P_PRODUCT_SERIAL: 'tricky-host.bobrightbox.com\n'},
     },
     'GCE': {
         'ds': 'GCE',
diff --git a/tools/ds-identify b/tools/ds-identify
index e0d4865..2447d14 100755
--- a/tools/ds-identify
+++ b/tools/ds-identify
@@ -891,9 +891,8 @@ ec2_identify_platform() {
     local default="$1"
     local serial="${DI_DMI_PRODUCT_SERIAL}"
 
-    # brightbox https://bugs.launchpad.net/cloud-init/+bug/1661693
     case "$serial" in
-        *brightbox.com) _RET="Brightbox"; return 0;;
+        *.brightbox.com) _RET="Brightbox"; return 0;;
     esac
 
     # AWS http://docs.aws.amazon.com/AWSEC2/