Rename config metadata key and fix config logic. (#411)

The metadata key `google-instance-configs` is used during instance setup to generate the instance configs file.

4 files changed, 72 insertions, 19 deletions

diff --git a/google_compute_engine/instance_setup/instance_config.py b/google_compute_engine/instance_setup/instance_config.py
index 07cd01e..703009e 100644
--- a/google_compute_engine/instance_setup/instance_config.py
+++ b/google_compute_engine/instance_setup/instance_config.py
@@ -21,6 +21,7 @@ Note that the configurations in
 overridden during package upgrade.
 """
 
+import logging
 import os
 
 from google_compute_engine import config_manager
@@ -76,7 +77,7 @@ class InstanceConfig(config_manager.ConfigManager):
       },
   }
 
-  def __init__(self, instance_config_metadata=None):
+  def __init__(self, logger=logging, instance_config_metadata=None):
     """Constructor.
 
     Inherit from the ConfigManager class. Read the template for instance
@@ -84,8 +85,10 @@ class InstanceConfig(config_manager.ConfigManager):
     updates from overriding user set defaults.
 
     Args:
+      logger: logger object, used to write to SysLog and serial port.
       instance_config_metadata: string, a config file specified in metadata.
     """
+    self.logger = logger
     self.instance_config_metadata = instance_config_metadata
     self.instance_config_header %= (
         self.instance_config_script, self.instance_config_template)
@@ -102,15 +105,23 @@ class InstanceConfig(config_manager.ConfigManager):
     config_defaults = []
     if self.instance_config_metadata:
       config = parser.SafeConfigParser()
-      config.readfp(stringio.StringIO(self.instance_config_metadata))
-      config_defaults.append(
-          dict((s, dict(config.items(s))) for s in config.sections()))
+      try:
+        config.readfp(stringio.StringIO(self.instance_config_metadata))
+      except parser.Error as e:
+        self.logger.error('Error parsing metadata configs: %s', str(e))
+      else:
+        config_defaults.append(
+            dict((s, dict(config.items(s))) for s in config.sections()))
     for config_file in config_files:
       if os.path.exists(config_file):
         config = parser.SafeConfigParser()
-        config.read(config_file)
-        config_defaults.append(
-            dict((s, dict(config.items(s))) for s in config.sections()))
+        try:
+          config.read(config_file)
+        except parser.Error as e:
+          self.logger.error('Error parsing config file: %s', str(e))
+        else:
+          config_defaults.append(
+              dict((s, dict(config.items(s))) for s in config.sections()))
     config_defaults.append(self.instance_config_options)
 
     for defaults in config_defaults:
diff --git a/google_compute_engine/instance_setup/instance_setup.py b/google_compute_engine/instance_setup/instance_setup.py
index 56aa241..1677fc7 100755
--- a/google_compute_engine/instance_setup/instance_setup.py
+++ b/google_compute_engine/instance_setup/instance_setup.py
@@ -46,11 +46,13 @@ class InstanceSetup(object):
         name='instance-setup', debug=self.debug, facility=facility)
     self.watcher = metadata_watcher.MetadataWatcher(logger=self.logger)
     self.metadata_dict = None
-    self.instance_config = instance_config.InstanceConfig()
+    self.instance_config = instance_config.InstanceConfig(logger=self.logger)
 
     if self.instance_config.GetOptionBool('InstanceSetup', 'network_enabled'):
       self.metadata_dict = self.watcher.GetMetadata()
-      self.instance_config = self._GetInstanceConfig()
+      instance_config_metadata = self._GetInstanceConfig()
+      self.instance_config = instance_config.InstanceConfig(
+          logger=self.logger, instance_config_metadata=instance_config_metadata)
       if self.instance_config.GetOptionBool('InstanceSetup', 'set_host_keys'):
         self._SetSshHostKeys()
       if self.instance_config.GetOptionBool('InstanceSetup', 'set_boto_config'):
@@ -83,8 +85,8 @@ class InstanceSetup(object):
       project_data = {}
       self.logger.warning('Project attributes were not found.')
 
-    return (instance_data.get('instance-configs') or
-            project_data.get('instance-configs'))
+    return (instance_data.get('google-instance-configs') or
+            project_data.get('google-instance-configs'))
 
   def _RunScript(self, script):
     """Run a script and log the streamed script output.
diff --git a/google_compute_engine/instance_setup/tests/instance_config_test.py b/google_compute_engine/instance_setup/tests/instance_config_test.py
index b53f813..a40dcbf 100644
--- a/google_compute_engine/instance_setup/tests/instance_config_test.py
+++ b/google_compute_engine/instance_setup/tests/instance_config_test.py
@@ -23,6 +23,7 @@ from google_compute_engine.test_compat import unittest
 class InstanceConfigTest(unittest.TestCase):
 
   def setUp(self):
+    self.mock_logger = mock.Mock()
     instance_config.InstanceConfig.instance_config = 'config'
     instance_config.InstanceConfig.instance_config_distro = 'distro'
     instance_config.InstanceConfig.instance_config_template = 'template'
@@ -50,9 +51,10 @@ class InstanceConfigTest(unittest.TestCase):
     mocks.attach_mock(mock_init, 'init')
     mocks.attach_mock(mock_set, 'set')
     mocks.attach_mock(mock_exists, 'exists')
+    mocks.attach_mock(self.mock_logger, 'logger')
     mock_exists.return_value = False
 
-    instance_config.InstanceConfig()
+    instance_config.InstanceConfig(logger=self.mock_logger)
     expected_calls = [
         mock.call.init(
             config_file='template', config_header='/tmp/test.py template'),
@@ -88,9 +90,11 @@ class InstanceConfigTest(unittest.TestCase):
       mocks.attach_mock(mock_parser, 'parser')
       mocks.attach_mock(mock_config, 'config')
       mocks.attach_mock(mock_exists, 'exists')
+      mocks.attach_mock(self.mock_logger, 'logger')
       mock_exists.return_value = True
 
-      instance_config.InstanceConfig(instance_config_metadata=config_metadata)
+      instance_config.InstanceConfig(
+          logger=self.mock_logger, instance_config_metadata=config_metadata)
       expected_calls = [
           mock.call.init(
               config_file='template', config_header='/tmp/test.py template'),
@@ -116,6 +120,40 @@ class InstanceConfigTest(unittest.TestCase):
       ]
       self.assertEqual(mocks.mock_calls, expected_calls)
 
+  @mock.patch('google_compute_engine.instance_setup.instance_config.parser.SafeConfigParser.read')
+  @mock.patch('google_compute_engine.instance_setup.instance_config.os.path.exists')
+  @mock.patch('google_compute_engine.instance_setup.instance_config.config_manager.ConfigManager.SetOption')
+  @mock.patch('google_compute_engine.instance_setup.instance_config.config_manager.ConfigManager.__init__')
+  def testInstanceConfigError(self, mock_init, mock_set, mock_exists, mock_read):
+    mock_read.side_effect = instance_config.parser.ParsingError('Error')
+    mocks = mock.Mock()
+    mocks.attach_mock(mock_init, 'init')
+    mocks.attach_mock(mock_set, 'set')
+    mocks.attach_mock(mock_read, 'read')
+    mocks.attach_mock(mock_exists, 'exists')
+    mocks.attach_mock(self.mock_logger, 'logger')
+    mock_exists.return_value = True
+
+    instance_config.InstanceConfig(
+        logger=self.mock_logger, instance_config_metadata='Error')
+    expected_calls = [
+        mock.call.init(
+            config_file='template', config_header='/tmp/test.py template'),
+        mock.call.logger.error(mock.ANY, mock.ANY),
+        mock.call.exists('config'),
+        mock.call.read('config'),
+        mock.call.logger.error(mock.ANY, mock.ANY),
+        mock.call.exists('distro'),
+        mock.call.read('distro'),
+        mock.call.logger.error(mock.ANY, mock.ANY),
+        mock.call.set('first', 'a', 'false', overwrite=False),
+        mock.call.set('second', 'b', 'true', overwrite=False),
+        mock.call.set('third', 'c', '1', overwrite=False),
+        mock.call.set('third', 'd', '2', overwrite=False),
+        mock.call.set('third', 'e', '3', overwrite=False)
+    ]
+    self.assertEqual(mocks.mock_calls, expected_calls)
+
   @mock.patch('google_compute_engine.instance_setup.instance_config.config_manager.ConfigManager.WriteConfig')
   def testWriteConfig(self, mock_write):
     mock_config = instance_config.InstanceConfig()
diff --git a/google_compute_engine/instance_setup/tests/instance_setup_test.py b/google_compute_engine/instance_setup/tests/instance_setup_test.py
index 572a26e..1de8134 100644
--- a/google_compute_engine/instance_setup/tests/instance_setup_test.py
+++ b/google_compute_engine/instance_setup/tests/instance_setup_test.py
@@ -50,7 +50,7 @@ class InstanceSetupTest(unittest.TestCase):
     mock_config_instance = mock.Mock()
     mock_config_instance.GetOptionBool.return_value = True
     mock_config.InstanceConfig.return_value = mock_config_instance
-    mock_setup._GetInstanceConfig.return_value = mock_config_instance
+    mock_setup._GetInstanceConfig.return_value = 'config'
 
     instance_setup.InstanceSetup.__init__(mock_setup)
     expected_calls = [
@@ -58,13 +58,15 @@ class InstanceSetupTest(unittest.TestCase):
         mock.call.logger.Logger(
             name=mock.ANY, debug=False, facility=mock.ANY),
         mock.call.watcher.MetadataWatcher(logger=mock_logger_instance),
-        mock.call.config.InstanceConfig(),
+        mock.call.config.InstanceConfig(logger=mock_logger_instance),
         # Check network access for reaching the metadata server.
         mock.call.config.InstanceConfig().GetOptionBool(
             'InstanceSetup', 'network_enabled'),
         mock.call.watcher.MetadataWatcher().GetMetadata(),
         # Get the instance config if specified in metadata.
         mock.call.setup._GetInstanceConfig(),
+        mock.call.config.InstanceConfig(
+            logger=mock_logger_instance, instance_config_metadata='config'),
         # Setup for SSH host keys if necessary.
         mock.call.config.InstanceConfig().GetOptionBool(
             'InstanceSetup', 'set_host_keys'),
@@ -109,7 +111,7 @@ class InstanceSetupTest(unittest.TestCase):
         mock.call.logger.Logger(
             name=mock.ANY, debug=False, facility=mock.ANY),
         mock.call.watcher.MetadataWatcher(logger=mock_logger_instance),
-        mock.call.config.InstanceConfig(),
+        mock.call.config.InstanceConfig(logger=mock_logger_instance),
         mock.call.config.InstanceConfig().GetOptionBool(
             'InstanceSetup', 'network_enabled'),
         mock.call.config.InstanceConfig().GetOptionBool(
@@ -127,12 +129,12 @@ class InstanceSetupTest(unittest.TestCase):
     self.mock_setup.metadata_dict = {
         'instance': {
             'attributes': {
-                'instance-configs': instance_config,
+                'google-instance-configs': instance_config,
             }
         },
         'project': {
             'attributes': {
-                'instance-configs': 'Unused config.',
+                'google-instance-configs': 'Unused config.',
             }
         }
     }
@@ -149,7 +151,7 @@ class InstanceSetupTest(unittest.TestCase):
         },
         'project': {
             'attributes': {
-                'instance-configs': instance_config,
+                'google-instance-configs': instance_config,
             }
         }
     }