Check that both requests and limits are set for a trusted resource co…

…nfig (#421)

* Check that both requests and limits are set for a trusted resource config

bonfire/config.py

@@ -89,11 +89,15 @@
     TRUSTED_COMPONENTS = os.getenv("BONFIRE_TRUSTED_COMPONENTS").split(",")
 
 # regexes used to check for trusted resource request/limit
-TRUSTED_REGEX_FOR_PATH = {
-    "resources.requests.cpu": r"\${(CPU_REQUEST[A-Z0-9_]*)}",
-    "resources.limits.cpu": r"\${(CPU_LIMIT[A-Z0-9_]*)}",
-    "resources.requests.memory": r"\${(MEM(?:ORY)?_REQUEST[A-Z0-9_]*)}",
-    "resources.limits.memory": r"\${(MEM(?:ORY)?_LIMIT[A-Z0-9_]*)}",
+TRUSTED_RESOURCE_REGEX = {
+    "requests": {
+        "cpu": r"\${(CPU_REQUEST[A-Z0-9_]*)}",  # noformat
+        "memory": r"\${(MEM(?:ORY)?_REQUEST[A-Z0-9_]*)}",
+    },
+    "limits": {
+        "cpu": r"\${(CPU_LIMIT[A-Z0-9_]*)}",  # noformat
+        "memory": r"\${(MEM(?:ORY)?_LIMIT[A-Z0-9_]*)}",
+    },
 }
 
 TRUSTED_CHECK_KINDS = ["ClowdApp", "ClowdJob", "ClowdJobInvocation"]

bonfire/processor.py

@@ -30,7 +30,7 @@ def _process_template(*args, **kwargs):
     return processed_template
 
 
-def _is_trusted_config(value, regex, component_params):
+def _get_trusted_config(data: dict, key1: str, key2: str, path: str, component_params: dict):
     """
     Check for presence of a trusted param being used for the value.
 
@@ -40,53 +40,84 @@ def _is_trusted_config(value, regex, component_params):
 
     Returns True if we determine this is a trusted value
     """
-    if not regex or not isinstance(regex, str):
-        raise ValueError("string value for 'regex' must be supplied")
+    regex = conf.TRUSTED_RESOURCE_REGEX.get(key1, {}).get(key2)
 
-    match = False
-    in_params = False
+    if not regex or not isinstance(regex, str):
+        raise ValueError("not able to find conf.TRUSTED_RESOURCE_REGEX[{key1}][{key2}]")
 
+    value = data.get(key1, {}).get(key2)
     if value:
+        match = False
+        in_params = False
+
         match = re.match(regex, str(value))
         if match and match.groups()[0] in component_params:
             in_params = True
 
-    log.debug(
-        "value '%s', regex r'%s', matches=%s, in params=%s", value, regex, bool(match), in_params
-    )
+        log.debug(
+            "value '%s', regex r'%s', matches=%s, in params=%s",
+            value,
+            regex,
+            bool(match),
+            in_params,
+        )
 
-    return match and in_params
+        if match and in_params:
+            return value
+        else:
+            log.debug("deleted untrusted config at '%s.%s.%s'", path, key1, key2)
+            return None
 
 
-def _remove_untrusted_configs(data, params, path="", current_dict=None, current_key=None):
+def _remove_untrusted_configs(
+    data,
+    params,
+    path="",
+    current_dict=None,
+):
     """
     Locate configurations within 'data' and remove them if not trusted.
 
-    Checks to see if any config matching a path listed in 'config.TRUSTED_PARAM_REGEX_FOR_PATH' is
-    found within 'data' dictionary and ensures the regex matches and that the parameter value is
-    set on the component's deploy config.
+    Checks to see if resource configurations match the regex specified in
+    conf.TRUSTED_RESOURCE_REGEX
+
+    Also checks that if a request is defined, a corresponding limit is defined
+    for the same resource and vice-versa.
     """
-    if isinstance(data, dict):
+    if path.endswith(".resources"):
+        resources = current_dict["resources"]
+        cpu_request = _get_trusted_config(resources, "requests", "cpu", path, params)
+        cpu_limit = _get_trusted_config(resources, "limits", "cpu", path, params)
+        mem_request = _get_trusted_config(resources, "requests", "memory", path, params)
+        mem_limit = _get_trusted_config(resources, "limits", "memory", path, params)
+
+        if any([cpu_request, cpu_limit]) and not all([cpu_request, cpu_limit]):
+            log.debug("'%s' cpu config needs both request and limit, removing cpu config", path)
+            cpu_request = None
+            cpu_limit = None
+        if any([mem_request, mem_limit]) and not all([mem_request, mem_limit]):
+            log.debug("'%s' mem config needs both request and limit, removing mem config", path)
+            mem_request = None
+            mem_limit = None
+
+        # if val is null, omit it from final dict
+        if not cpu_request:
+            del resources["requests"]["cpu"]
+        if not cpu_limit:
+            del resources["limits"]["cpu"]
+        if not mem_request:
+            del resources["requests"]["memory"]
+        if not mem_limit:
+            del resources["limits"]["memory"]
+
+    elif isinstance(data, dict):
         for key, value in copy.copy(data).items():
-            _remove_untrusted_configs(
-                value, params, path + f".{key}", current_dict=data, current_key=key
-            )
+            _remove_untrusted_configs(value, params, path + f".{key}", current_dict=data)
 
     elif isinstance(data, list):
         for index, value in enumerate(copy.copy(data)):
             _remove_untrusted_configs(value, params, path + f"[{index}]")
 
-    # check if this value is at a path where we need to see a certain parameter in use
-    # in order to preserve it
-    for path_end, regex in conf.TRUSTED_REGEX_FOR_PATH.items():
-        # only check values if the path end is listed in TRUSTED_PARAM_REGEX_FOR_PATH
-        if not path.endswith(path_end):
-            continue
-
-        if not _is_trusted_config(data, regex, params):
-            del current_dict[current_key]
-            log.debug("deleted untrusted config at '%s'", path)
-
 
 def _remove_untrusted_configs_for_template(template, params):
     """
@@ -107,7 +138,6 @@ def _remove_untrusted_configs_for_template(template, params):
 
         name = obj.get("metadata", {}).get("name")
         log.debug("checking resources on %s '%s'", kind, name)
-
         _remove_untrusted_configs(obj, params)
 
 

tests/test_processor.py

@@ -95,6 +95,70 @@ def mock_repo_file(monkeypatch):
 """
 
 
+CLOWDAPP_W_UNTRUSTED_PARAM = """
+---
+apiVersion: v1
+kind: Template
+metadata:
+  name: {name}-template
+objects:
+- apiVersion: cloud.redhat.com/v1alpha1
+  kind: ClowdApp
+  metadata:
+    name: {name}
+  spec:
+    envName: ${{ENV_NAME}}
+    dependencies: {deps}
+    optionalDependencies: {optional_deps}
+    deployments:
+    - name: deployment1
+      podSpec:
+        resources:
+          limits:
+            cpu: ${{CPU_LIMIT_DEPLOYMENT1}}
+            memory: ${{MEM_LIMIT_DEPLOYMENT1}}
+          requests:
+            cpu: ${{INVALID_PARAMETER_HERE}}
+            memory: ${{MEM_REQUEST_DEPLOYMENT1}}
+    - name: deployment2
+      podSpec:
+        resources:
+          limits:
+            cpu: ${{DEPLOYMENT2_WRONG_NAME_CPU}}
+            memory: ${{MEM_LIMIT_DEPLOYMENT2}}
+          requests:
+            cpu: ${{DEPLOYMENT2_WRONG_NAME_CPU}}
+            memory: ${{MEMORY_REQUEST_DEPLOYMENT2}}
+parameters:
+- description: Image tag
+  name: IMAGE_TAG
+  required: true
+- description: ClowdEnv Name
+  name: ENV_NAME
+  required: true
+- name: CPU_LIMIT_DEPLOYMENT1
+  value: 100m
+- name: CPU_REQUEST_DEPLOYMENT1
+  value: 1m
+- name: MEM_REQUEST_DEPLOYMENT1
+  value: 1Mi
+- name: MEM_LIMIT_DEPLOYMENT1
+  value: 100Mi
+- name: DEPLOYMENT2_WRONG_NAME_CPU
+  value: 2m
+- name: MEMORY_REQUEST_DEPLOYMENT2
+  value: 2Mi
+- name: MEM_LIMIT_DEPLOYMENT2
+  value: 200Mi
+"""
+
+
+TEMPLATES = {
+    "simple_clowdapp": SIMPLE_CLOWDAPP,
+    "clowdapp_w_untrusted_param": CLOWDAPP_W_UNTRUSTED_PARAM,
+}
+
+
 def assert_clowdapps(items, app_list):
     found_apps = AppOrComponentSelector().apps
     for i in items:
@@ -114,13 +178,16 @@ def assert_clowdapps(items, app_list):
         raise AssertionError("apps present more than once in processed output")
 
 
-def add_template(mock_repo_file, template_name, deps=None, optional_deps=None):
+def add_template(
+    mock_repo_file, template_name, deps=None, optional_deps=None, template_key="simple_clowdapp"
+):
     deps = deps or []
     optional_deps = optional_deps or []
+    template = TEMPLATES[template_key]
     mock_repo_file.add_template(
         template_name,
         uuid.uuid4().hex[0:6],
-        SIMPLE_CLOWDAPP.format(name=template_name, deps=deps, optional_deps=optional_deps),
+        template.format(name=template_name, deps=deps, optional_deps=optional_deps),
     )
 
 
@@ -702,6 +769,42 @@ def test_preserve_resources_trusted_params(mock_repo_file):
     assert "cpu" not in deployment2["podSpec"]["resources"]["limits"]
 
 
+def test_remove_resources_without_corresponding_config(mock_repo_file):
+    """
+    Test that using trusted parameters causes cpu/mem configurations to be preserved.
+
+    Ensures that a value set with an untrusted parameter name is still removed.
+    """
+    add_template(mock_repo_file, "app1-component1", template_key="clowdapp_w_untrusted_param")
+    apps_config = get_apps_config_with_params(
+        parameters={
+            "CPU_LIMIT_DEPLOYMENT1": "456m",
+            "CPU_REQUEST_DEPLOYMENT1": "123m",  # invalid param name present in template
+            "MEM_LIMIT_DEPLOYMENT1": "456Mi",
+            "MEM_REQUEST_DEPLOYMENT1": "123Mi",
+            "DEPLOYMENT2_WRONG_NAME_CPU": "1",
+            "MEM_LIMIT_DEPLOYMENT2": "910Mi",
+            "MEMORY_REQUEST_DEPLOYMENT2": "789Mi",
+        }
+    )
+    processor = get_processor(apps_config)
+    processor.requested_app_names = ["app1"]
+    result = processor.process()
+
+    deployments = result["items"][0]["spec"]["deployments"]
+    deployment1, deployment2 = deployments[0], deployments[1]
+
+    assert "cpu" not in deployment1["podSpec"]["resources"]["requests"]
+    assert deployment1["podSpec"]["resources"]["requests"]["memory"] == "123Mi"
+    assert "cpu" not in deployment1["podSpec"]["resources"]["limits"]
+    assert deployment1["podSpec"]["resources"]["limits"]["memory"] == "456Mi"
+    assert deployment2["podSpec"]["resources"]["requests"]["memory"] == "789Mi"
+    assert deployment2["podSpec"]["resources"]["limits"]["memory"] == "910Mi"
+    # deployment2 CPU param does not match trusted syntax for name
+    assert "cpu" not in deployment2["podSpec"]["resources"]["requests"]
+    assert "cpu" not in deployment2["podSpec"]["resources"]["limits"]
+
+
 @pytest.mark.parametrize(
     "no_remove_resources",
     (