Merge pull request 2i2c-org#1042 from sgibson91/fix-secret-helm-chart…

…-values-files

deployer/__main__.py

@@ -13,7 +13,7 @@
 from auth import KeyProvider
 from hub import Cluster
 from utils import (
-    verify_and_decrypt_file,
+    get_decrypted_file,
     print_colour,
     find_absolute_path_to_cluster_file,
 )
@@ -93,14 +93,8 @@ def deploy_grafana_dashboards(cluster_name):
         "enc-grafana-token.secret.yaml"
     )
 
-    # Check the secret file exists before continuing
-    if not os.path.exists(grafana_token_file):
-        raise FileExistsError(
-            f"File does not exist! Please create it and try again: {grafana_token_file}"
-        )
-
-    # Read the cluster specific secret config file
-    with verify_and_decrypt_file(grafana_token_file) as decrypted_file_path:
+    # Read the cluster specific secret grafana token file
+    with get_decrypted_file(grafana_token_file) as decrypted_file_path:
         with open(decrypted_file_path) as f:
             config = yaml.load(f)
 
@@ -175,7 +169,7 @@ def deploy(cluster_name, hub_name, skip_hub_health_test, config_path):
     # Validate our config with JSON Schema first before continuing
     validate(cluster_name)
 
-    with verify_and_decrypt_file(config_path) as decrypted_file_path:
+    with get_decrypted_file(config_path) as decrypted_file_path:
         with open(decrypted_file_path) as f:
             config = yaml.load(f)
 

deployer/hub.py

@@ -14,9 +14,9 @@
 from ruamel.yaml import YAML
 
 from utils import (
-    verify_and_decrypt_file,
+    get_decrypted_file,
     print_colour,
-    check_file_exists,
+    get_decrypted_files,
 )
 
 # Without `pure=True`, I get an exception about str / byte issues
@@ -127,7 +127,7 @@ def deploy_support(self):
         subprocess.check_call(["helm", "dep", "up", support_dir])
 
         support_secrets_file = support_dir.joinpath("enc-support.secret.yaml")
-        with tempfile.NamedTemporaryFile(mode="w") as f, verify_and_decrypt_file(
+        with tempfile.NamedTemporaryFile(mode="w") as f, get_decrypted_file(
             support_secrets_file
         ) as secret_file:
             yaml.dump(self.support.get("config", {}), f)
@@ -160,7 +160,7 @@ def auth_kubeconfig(self):
         config = self.spec["kubeconfig"]
         config_path = self.config_path.joinpath(config["file"])
 
-        with verify_and_decrypt_file(config_path) as decrypted_key_path:
+        with get_decrypted_file(config_path) as decrypted_key_path:
             # FIXME: Unset this after our yield
             os.environ["KUBECONFIG"] = decrypted_key_path
             yield
@@ -189,7 +189,7 @@ def auth_aws(self):
             orig_access_key_id = os.environ.get("AWS_ACCESS_KEY_ID", None)
             orig_secret_access_key = os.environ.get("AWS_SECRET_ACCESS_KEY", None)
             try:
-                with verify_and_decrypt_file(key_path) as decrypted_key_path:
+                with get_decrypted_file(key_path) as decrypted_key_path:
 
                     decrypted_key_abspath = os.path.abspath(decrypted_key_path)
                     if not os.path.isfile(decrypted_key_abspath):
@@ -252,7 +252,7 @@ def auth_azure(self):
             try:
                 os.environ["KUBECONFIG"] = kubeconfig.name
 
-                with verify_and_decrypt_file(key_path) as decrypted_key_path:
+                with get_decrypted_file(key_path) as decrypted_key_path:
 
                     decrypted_key_abspath = os.path.abspath(decrypted_key_path)
                     if not os.path.isfile(decrypted_key_abspath):
@@ -311,7 +311,7 @@ def auth_gcp(self):
             orig_kubeconfig = os.environ.get("KUBECONFIG")
             try:
                 os.environ["KUBECONFIG"] = kubeconfig.name
-                with verify_and_decrypt_file(key_path) as decrypted_key_path:
+                with get_decrypted_file(key_path) as decrypted_key_path:
                     subprocess.check_call(
                         [
                             "gcloud",
@@ -540,38 +540,16 @@ def deploy(self, auth_provider, secret_key, skip_hub_health_test=False):
         if "domain_override_file" in self.spec.keys():
             domain_override_file = self.spec["domain_override_file"]
 
-            check_file_exists(self.cluster.config_path.joinpath(domain_override_file))
-
-            if domain_override_file.startswith("enc-") or (
-                "secret" in domain_override_file
-            ):
-                with verify_and_decrypt_file(
-                    self.cluster.config_path.joinpath(domain_override_file)
-                ) as decrypted_path:
-                    with open(decrypted_path) as f:
-                        domain_override_config = yaml.load(f)
-            else:
-                with open(self.cluster.config_path.joinpath(domain_override_file)) as f:
+            with get_decrypted_file(
+                self.cluster.config_path.joinpath(domain_override_file)
+            ) as decrypted_path:
+                with open(decrypted_path) as f:
                     domain_override_config = yaml.load(f)
 
             self.spec["domain"] = domain_override_config["domain"]
 
         generated_values = self.get_generated_config(auth_provider, secret_key)
 
-        # Find helm chart values files
-        values_files = []
-        for values_file in self.spec["helm_chart_values_files"]:
-            check_file_exists(self.cluster.config_path.joinpath(values_file))
-            if values_file.startswith("enc-") or ("secret" in values_file):
-                with verify_and_decrypt_file(
-                    self.cluster.config_path.joinpath(values_file)
-                ) as decrypted_file:
-                    values_files.append(f"--values={decrypted_file}")
-            else:
-                values_files.append(
-                    f"--values={self.cluster.config_path.joinpath(values_file)}"
-                )
-
         # Ensure helm charts are up to date
         helm_charts_dir = (Path(__file__).parent.parent).joinpath("helm-charts")
         subprocess.check_call(
@@ -582,7 +560,11 @@ def deploy(self, auth_provider, secret_key, skip_hub_health_test=False):
                 ["helm", "dep", "up", helm_charts_dir.joinpath("daskhub")]
             )
 
-        with tempfile.NamedTemporaryFile(mode="w") as generated_values_file:
+        with tempfile.NamedTemporaryFile(
+            mode="w"
+        ) as generated_values_file, get_decrypted_files(
+            self.spec["helm_chart_values_files"], self.cluster.config_path
+        ) as values_files:
             json.dump(generated_values, generated_values_file)
             generated_values_file.flush()
 
@@ -602,7 +584,8 @@ def deploy(self, auth_provider, secret_key, skip_hub_health_test=False):
             ]
 
             # Add on the values files
-            cmd.extend(values_files)
+            for values_file in values_files:
+                cmd.append(f"--values={values_file}")
 
             # join method will fail on the PosixPath element if not transformed
             # into a string first

deployer/utils.py

@@ -4,22 +4,22 @@
 import subprocess
 from ruamel.yaml import YAML
 from ruamel.yaml.scanner import ScannerError
-from contextlib import contextmanager
+from contextlib import contextmanager, ExitStack
 from pathlib import Path
 import warnings
 
 yaml = YAML(typ="safe", pure=True)
 
 
-def check_file_exists(filepath):
-    """Check a filepath exists, raise an error if not. This function is to be used for
+def assert_file_exists(filepath):
+    """Assert a filepath exists, raise an error if not. This function is to be used for
     files that *absolutely have to exist* in order to successfully complete deployment,
     such as, files listed in the `helm_chart_values_file` key in the `cluster.yaml` file
 
     Args:
-        filepath (str): Absolute path to the file that is to be checked for existence
+        filepath (str): Absolute path to the file that is to be asserted for existence
     """
-    if not os.path.exists(filepath):
+    if not os.path.isfile(filepath):
         raise FileNotFoundError(
             f"""
             File Not Found at following location! Have you checked it's the correct path?
@@ -68,47 +68,54 @@ def find_absolute_path_to_cluster_file(cluster_name: str):
 
 
 @contextmanager
-def verify_and_decrypt_file(encrypted_path):
+def get_decrypted_file(original_filepath):
     """
-    Provide secure, temporarily decrypted contents of a given file. We verify the file
-    is sops-encrypted and raise an error if we do not find the sops key when we expect
-    to, in case the decrypted contents have been leaked via version control.
+    Assert that a given file exists. If the file is sops-encryped, we provide secure,
+    temporarily decrypted contents of the file. We raise an error if we do not find the
+    sops key when we expect to, in case the decrypted contents have been leaked via
+    version control. We expect to find the sops key in a file if the filename begins
+    with "enc-" or contains the word "secret". If the file is not encrypted, we return
+    the original filepath.
 
     Args:
-        encrypted_path (path object): Absolute path to an encrypted file to perform
-            checks on and decrypt
+        original_filepath (path object): Absolute path to a file to perform checks on
+            and decrypt if it's encrypted
 
     Yields:
-        decrypted_path (path object): Abolute path to a tempfile containing the
-            decrypted contents. Unless the file is not valid JSON/YAML or does not have
-            the prefix `enc-`, then we return the original, encrypted path.
+        (path object): EITHER the absolute path to a tempfile containing the
+            decrypted contents, OR the original filepath. The original filepath is
+            yielded if the file is not valid JSON/YAML, or does not have the prefix
+            'enc-' or contain 'secret'.
     """
-    filename = os.path.basename(encrypted_path)
+    assert_file_exists(original_filepath)
+    filename = os.path.basename(original_filepath)
     _, ext = os.path.splitext(filename)
 
-    # Our convention is that encrypted secrets in the repository begin with "enc-",
-    # so first we check for that
-    if filename.startswith("enc-"):
+    # Our convention is that encrypted secrets in the repository begin with "enc-" and include
+    # "secret" in the filename, so first we check for that. We use an 'or' conditional here since
+    # we want to catch files that contain "secret" but do not have the "enc-" prefix and ensure
+    # they are encrypted, raising an error if not.
+    if filename.startswith("enc-") or ("secret" in filename):
         # We must then determine if the file is using sops
         # sops files are JSON/YAML with a `sops` key. So we first check
         # if the file is valid JSON/YAML, and then if it has a `sops` key
-        with open(encrypted_path) as f:
+        with open(original_filepath) as f:
 
             # Support the (clearly wrong) people who use .yml instead of .yaml
             if ext == ".yaml" or ext == ".yml":
                 try:
-                    encrypted_data = yaml.load(f)
+                    content = yaml.load(f)
                 except ScannerError:
-                    yield encrypted_path
+                    yield original_filepath
                     return
             elif ext == ".json":
                 try:
-                    encrypted_data = json.load(f)
+                    content = json.load(f)
                 except json.JSONDecodeError:
-                    yield encrypted_path
+                    yield original_filepath
                     return
 
-        if "sops" not in encrypted_data:
+        if "sops" not in content:
             raise KeyError(
                 "Expecting to find the `sops` key in this encrypted file - but it "
                 + "wasn't found! Please regenerate the secret in case it has been "
@@ -118,18 +125,29 @@ def verify_and_decrypt_file(encrypted_path):
         # If file has a `sops` key, we assume it's sops encrypted
         with tempfile.NamedTemporaryFile() as f:
             subprocess.check_call(
-                ["sops", "--output", f.name, "--decrypt", encrypted_path]
+                ["sops", "--output", f.name, "--decrypt", original_filepath]
             )
             yield f.name
 
     else:
-        # Hmmm. A file has been passed to this function but does not have the `enc-`
-        # prefix. What is the correct thing to do here? For now, we do what has been
-        # done before and return the path to the encrypted file
-        yield encrypted_path
+        # For a file that does not match our naming conventions for secrets, yield the
+        # original path
+        yield original_filepath
         return
 
 
+@contextmanager
+def get_decrypted_files(files, abspath):
+    """
+    This is a context manager that combines multiple `get_decrypted_file`
+    context managers that open and/or decrypt the files in `files`.
+    """
+    with ExitStack() as stack:
+        yield [
+            stack.enter_context(get_decrypted_file(abspath.joinpath(f))) for f in files
+        ]
+
+
 def print_colour(msg: str):
     """Print messages in colour to be distinguishable in CI logs