Merge pull request #8189 from mgugino-upstream-stage/search-master-config-paths

Add master config filepath checking

playbooks/init/basic_facts.yml

@@ -70,6 +70,15 @@
         - l_atomic_docker_version.stdout | replace('"', '') is version_compare('1.12','>=')
         msg: Installation on Atomic Host requires Docker 1.12 or later. Please upgrade and restart the Atomic Host.
 
+- name: Retrieve existing master configs and validate
+  hosts: oo_masters_to_config
+  gather_facts: no
+  any_errors_fatal: true
+  tasks:
+  - import_role:
+      name: openshift_control_plane
+      tasks_from: check_existing_config.yml
+
 - name: Initialize special first-master variables
   hosts: oo_first_master
   roles:
@@ -77,36 +86,15 @@
   tasks:
   - when: not (osm_default_node_selector is defined)
     block:
-    - stat:
-        path: "/etc/origin/master/master-config.yaml"
-      register: master_config_path_check
-
-    - slurp:
-        src: "/etc/origin/master/master-config.yaml"
-      register: openshift_master_config_encoded_contents
-      when: master_config_path_check.stat.exists
-
     - set_fact:
+        # l_existing_config_master_config is set in openshift_control_plane/tasks/check_existing_config.yml
         openshift_master_config_node_selector: "{{ l_existing_config_master_config.projectConfig.defaultNodeSelector }}"
-      vars:
-        l_existing_config_master_config: "{{ (openshift_master_config_encoded_contents.content | b64decode | from_yaml) }}"
       when:
-      - master_config_path_check.stat.exists
+      - l_existing_config_master_config is defined
       - l_existing_config_master_config.projectConfig is defined
       - l_existing_config_master_config.projectConfig.defaultNodeSelector is defined
       - l_existing_config_master_config.projectConfig.defaultNodeSelector != ''
 
-    - set_fact:
-        # Take list of existing IDProviders so we can use/modify them later,
-        # especially for migrating htpasswd file.
-        openshift_master_existing_idproviders: "{{ l_existing_config_master_config.oauthConfig.identityProviders }}"
-      vars:
-        l_existing_config_master_config: "{{ (openshift_master_config_encoded_contents.content | b64decode | from_yaml) }}"
-      when:
-      - master_config_path_check.stat.exists
-      - l_existing_config_master_config.oauthConfig is defined
-      - l_existing_config_master_config.oauthConfig.identityProviders is defined
-
   - set_fact:
       # We need to setup openshift_client_binary here for special uses of delegate_to in
       # later roles and plays.

playbooks/openshift-master/private/config.yml

@@ -76,14 +76,14 @@
       local_facts:
         bootstrapped: true
 
-- name: Inspect state of first master config settings
+- name: Generate or retrieve existing session secrets
   hosts: oo_first_master
   roles:
   - role: openshift_facts
   tasks:
   - import_role:
       name: openshift_control_plane
-      tasks_from: check_existing_config
+      tasks_from: generate_session_secrets.yml
 
 - name: Configure masters
   hosts: oo_masters_to_config

roles/lib_utils/action_plugins/master_check_paths_in_config.py

@@ -0,0 +1,129 @@
+"""
+Ansible action plugin to ensure inventory variables are set
+appropriately and no conflicting options have been provided.
+"""
+import collections
+import six
+
+from ansible.plugins.action import ActionBase
+from ansible import errors
+
+
+FAIL_MSG = """A string value that appears to be a file path located outside of
+{} has been found in /etc/origin/master/master-config.yaml.
+In 3.10 and newer, all files needed by the master must reside inside of
+those directories or a subdirectory or it will not be readable by the
+master process. Please migrate all files needed by the master into
+one of {} or a subdirectory and update your master configs before
+proceeding. The string found was: {}
+***********************
+NOTE: the following items do not need to be migrated, they will be migrated
+for you: {}"""
+
+
+ITEMS_TO_POP = (
+    ('oauthConfig', 'identityProviders'),
+)
+# Create csv string of dot-separated dictionary keys:
+# eg: 'oathConfig.identityProviders, something.else.here'
+MIGRATED_ITEMS = ", ".join([".".join(x) for x in ITEMS_TO_POP])
+
+ALLOWED_DIRS = (
+    '/etc/origin/master/',
+    '/var/lib/origin',
+    '/etc/origin/cloudprovider',
+)
+
+ALLOWED_DIRS_STRING = ', '.join(ALLOWED_DIRS)
+
+
+def pop_migrated_fields(mastercfg):
+    """Some fields do not need to be searched because they will be migrated
+    for users automatically"""
+    # Walk down the tree and pop the specific item we migrate / don't care about
+    for item in ITEMS_TO_POP:
+        field = mastercfg
+        for sub_field in item:
+            parent_field = field
+            field = field[sub_field]
+        parent_field.pop(item[len(item) - 1])
+
+
+def do_item_check(val, strings_to_check):
+    """Check type of val, append to strings_to_check if string, otherwise if
+    it's a dictionary-like object call walk_mapping, if it's a list-like
+    object call walk_sequence, else ignore."""
+    if isinstance(val, six.string_types):
+        strings_to_check.append(val)
+    elif isinstance(val, collections.Sequence):
+        # A list-like object
+        walk_sequence(val, strings_to_check)
+    elif isinstance(val, collections.Mapping):
+        # A dictionary-like object
+        walk_mapping(val, strings_to_check)
+    # If it's not a string, list, or dictionary, we're not interested.
+
+
+def walk_sequence(items, strings_to_check):
+    """Walk recursively through a list, items"""
+    for item in items:
+        do_item_check(item, strings_to_check)
+
+
+def walk_mapping(map_to_walk, strings_to_check):
+    """Walk recursively through map_to_walk dictionary and add strings to
+    strings_to_check"""
+    for _, val in map_to_walk.items():
+        do_item_check(val, strings_to_check)
+
+
+def check_strings(strings_to_check):
+    """Check the strings we found to see if they look like file paths and if
+    they are, fail if not start with /etc/origin/master"""
+    for item in strings_to_check:
+        if item.startswith('/') or item.startswith('../'):
+            matches = 0
+            for allowed in ALLOWED_DIRS:
+                if item.startswith(allowed):
+                    matches += 1
+            if matches == 0:
+                raise errors.AnsibleModuleError(
+                    FAIL_MSG.format(ALLOWED_DIRS_STRING,
+                                    ALLOWED_DIRS_STRING,
+                                    item, MIGRATED_ITEMS))
+
+
+# pylint: disable=R0903
+class ActionModule(ActionBase):
+    """Action plugin to validate no files are needed by master that reside
+    outside of /etc/origin/master as masters will now run as pods and cannot
+    utilize files outside of that path as they will not be mounted inside the
+    containers."""
+    def run(self, tmp=None, task_vars=None):
+        """Run this action module"""
+        result = super(ActionModule, self).run(tmp, task_vars)
+
+        # self.task_vars holds all in-scope variables.
+        # Ignore settting self.task_vars outside of init.
+        # pylint: disable=W0201
+        self.task_vars = task_vars or {}
+
+        # mastercfg should be a dictionary from scraping an existing master's
+        # config yaml file.
+        mastercfg = self._task.args.get('mastercfg')
+
+        # We migrate some paths for users automatically, so we pop those.
+        pop_migrated_fields(mastercfg)
+
+        # Create an empty list to append strings from our config file to to check
+        # later.
+        strings_to_check = []
+
+        walk_mapping(mastercfg, strings_to_check)
+
+        check_strings(strings_to_check)
+
+        result["changed"] = False
+        result["failed"] = False
+        result["msg"] = "Aight, configs looking good"
+        return result

roles/lib_utils/test/test_master_check_paths_in_config.py

@@ -0,0 +1,82 @@
+'''
+ Unit tests for the master_check_paths_in_config action plugin
+'''
+import os
+import sys
+
+from ansible import errors
+import pytest
+
+
+MODULE_PATH = os.path.realpath(os.path.join(__file__, os.pardir, os.pardir, 'action_plugins'))
+sys.path.insert(1, MODULE_PATH)
+
+# pylint: disable=import-error,wrong-import-position,missing-docstring
+# pylint: disable=invalid-name,redefined-outer-name
+import master_check_paths_in_config  # noqa: E402
+
+
+@pytest.fixture()
+def loaded_config():
+    """return testing master config"""
+    data = {
+        'apiVersion': 'v1',
+        'oauthConfig':
+        {'identityProviders':
+            ['1', '2', '/this/will/fail']},
+        'fake_top_item':
+        {'fake_item':
+            {'fake_item2':
+                ["some string",
+                    {"fake_item3":
+                        ["some string 2",
+                            {"fake_item4":
+                                {"some_key": "deeply_nested_string"}}]}]}}
+    }
+    return data
+
+
+def test_pop_migrated(loaded_config):
+    """Params:
+
+    * `loaded_config` comes from the `loaded_config` fixture in this file
+    """
+    # Ensure we actually loaded a valid config
+    assert loaded_config['apiVersion'] == 'v1'
+
+    # Test that migrated key is removed
+    master_check_paths_in_config.pop_migrated_fields(loaded_config)
+    assert loaded_config['oauthConfig'] is not None
+    assert loaded_config['oauthConfig'].get('identityProviders') is None
+
+
+def test_walk_mapping(loaded_config):
+    """Params:
+    * `loaded_config` comes from the `loaded_config` fixture in this file
+    """
+    # Ensure we actually loaded a valid config
+    fake_top_item = loaded_config['fake_top_item']
+    stc = []
+    expected_keys = ("some string", "some string 2", "deeply_nested_string")
+
+    # Test that we actually extract all the strings from complicated nested
+    # structures
+    master_check_paths_in_config.walk_mapping(fake_top_item, stc)
+    assert len(stc) == 3
+    for item in expected_keys:
+        assert item in stc
+
+
+def test_check_strings():
+    stc_good = ('/etc/origin/master/good', 'some/child/dir')
+    # This should not raise
+    master_check_paths_in_config.check_strings(stc_good)
+
+    # This is a string we should alert on
+    stc_bad = ('goodfile.txt', '/root/somefile')
+    with pytest.raises(errors.AnsibleModuleError):
+        master_check_paths_in_config.check_strings(stc_bad)
+
+    stc_bad_relative = ('goodfile.txt', '../node/otherfile')
+    with pytest.raises(errors.AnsibleModuleError):
+        master_check_paths_in_config.check_strings(stc_bad_relative)

roles/openshift_control_plane/tasks/check_existing_config.yml

@@ -1,57 +1,32 @@
 ---
-- name: Check for existing configuration
-  stat:
-    path: /etc/origin/master/master-config.yaml
-  register: master_config_stat
-
-- name: Set clean install fact
-  set_fact:
-    l_clean_install: "{{ not master_config_stat.stat.exists | bool }}"
-
-- name: Determine if etcd3 storage is in use
-  command: grep  -Pzo  "storage-backend:\n.*etcd3" /etc/origin/master/master-config.yaml -q
-  register: etcd3_grep
-  failed_when: false
-  changed_when: false
-
-- name: Set etcd3 fact
-  set_fact:
-    l_etcd3_enabled: "{{ etcd3_grep.rc == 0 | bool }}"
-
-- name: Check if atomic-openshift-master sysconfig exists yet
-  stat:
-    path: /etc/sysconfig/atomic-openshift-master
-  register: l_aom_exists
-
-- name: Preserve OPENSHIFT_DEFAULT_REGISTRY master parameter if present
-  command: awk '/^OPENSHIFT_DEFAULT_REGISTRY/' /etc/sysconfig/atomic-openshift-master
-  register: l_default_registry_defined
-  when: l_aom_exists.stat.exists | bool
-
-- name: Check if atomic-openshift-master-api sysconfig exists yet
-  stat:
-    path: /etc/sysconfig/atomic-openshift-master-api
-  register: l_aom_api_exists
-
-- name: Preserve OPENSHIFT_DEFAULT_REGISTRY master-api parameter if present
-  command: awk '/^OPENSHIFT_DEFAULT_REGISTRY/' /etc/sysconfig/atomic-openshift-master-api
-  register: l_default_registry_defined_api
-  when: l_aom_api_exists.stat.exists | bool
-
-- name: Check if atomic-openshift-master-controllers sysconfig exists yet
-  stat:
-    path: /etc/sysconfig/atomic-openshift-master-controllers
-  register: l_aom_controllers_exists
-
-- name: Preserve OPENSHIFT_DEFAULT_REGISTRY master-controllers parameter if present
-  command: awk '/^OPENSHIFT_DEFAULT_REGISTRY/' /etc/sysconfig/atomic-openshift-master-controllers
-  register: l_default_registry_defined_controllers
-  when: l_aom_controllers_exists.stat.exists | bool
-
-- name: Update facts with OPENSHIFT_DEFAULT_REGISTRY value
-  set_fact:
-    l_default_registry_value: "{{ l_default_registry_defined.stdout | default('') }}"
-    l_default_registry_value_api: "{{ l_default_registry_defined_api.stdout | default('') }}"
-    l_default_registry_value_controllers: "{{ l_default_registry_defined_controllers.stdout | default('') }}"
-
-- import_tasks: generate_session_secrets.yml
+# We need to scrape existing config and check some items.
+- stat:
+    path: "/etc/origin/master/master-config.yaml"
+  register: master_config_path_check
+
+- slurp:
+    src: "/etc/origin/master/master-config.yaml"
+  register: openshift_master_config_encoded_contents
+  when: master_config_path_check.stat.exists
+
+# The existing config will also be used to determine first node selector on
+# first master.
+- set_fact:
+    l_existing_config_master_config: "{{ (openshift_master_config_encoded_contents.content | b64decode | from_yaml) }}"
+  when:
+  - openshift_master_config_encoded_contents is defined
+  - openshift_master_config_encoded_contents.content is defined
+
+- name: Check for file paths outside of /etc/origin/master in master's config
+  master_check_paths_in_config:
+    mastercfg: "{{ l_existing_config_master_config }}"
+  when: l_existing_config_master_config is defined
+
+- set_fact:
+    # Take list of existing IDProviders so we can use/modify them later,
+    # especially for migrating htpasswd file.
+    openshift_master_existing_idproviders: "{{ l_existing_config_master_config.oauthConfig.identityProviders }}"
+  when:
+  - l_existing_config_master_config is defined
+  - l_existing_config_master_config.oauthConfig is defined
+  - l_existing_config_master_config.oauthConfig.identityProviders is defined