Repository: onicagroup/runway

Issue:
update `runway.cfngin.lookups.handlers.kms` to use the standardized query syntax
Subtask of #1184 

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/onicagroup/runway/pull/1316?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup) Report
> Merging [#1316](https://codecov.io/gh/onicagroup/runway/pull/1316?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup) (e37c024) into [master](https://codecov.io/gh/onicagroup/runway/commit/f23756f0578b15bd5e75479db6f4ee5b28dc8734?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup) (f23756f) will **increase** coverage by `0.00%`.
> The diff coverage is `94.11%`.

[![Impacted file tree graph](https://codecov.io/gh/onicagroup/runway/pull/1316/graphs/tree.svg?width=650&height=150&src=pr&token=Ku28I0RY80&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup)](https://codecov.io/gh/onicagroup/runway/pull/1316?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup)

```diff
@@           Coverage Diff           @@
##           master    #1316   +/-   ##
=======================================
  Coverage   90.13%   90.14%           
=======================================
  Files         247      247           
  Lines       15412    15419    +7     
=======================================
+ Hits        13892    13899    +7     
  Misses       1520     1520           
```


| [Impacted Files](https://codecov.io/gh/onicagroup/runway/pull/1316?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup) | Coverage Δ | |
|---|---|---|
| [runway/cfngin/lookups/handlers/kms.py](https://codecov.io/gh/onicagroup/runway/pull/1316/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup#diff-cnVud2F5L2Nmbmdpbi9sb29rdXBzL2hhbmRsZXJzL2ttcy5weQ==) | `96.29% <93.33%> (+1.29%)` | :arrow_up: |
| [runway/cfngin/lookups/handlers/output.py](https://codecov.io/gh/onicagroup/runway/pull/1316/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup#diff-cnVud2F5L2Nmbmdpbi9sb29rdXBzL2hhbmRsZXJzL291dHB1dC5weQ==) | `96.42% <100.00%> (ø)` | |
| [runway/cfngin/lookups/handlers/rxref.py](https://codecov.io/gh/onicagroup/runway/pull/1316/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup#diff-cnVud2F5L2Nmbmdpbi9sb29rdXBzL2hhbmRsZXJzL3J4cmVmLnB5) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/onicagroup/runway/pull/1316?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/onicagroup/runway/pull/1316?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup). Last update [f23756f...e37c024](https://codecov.io/gh/onicagroup/runway/pull/1316?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=onicagroup).


Patch:
diff --git a/docs/source/cfngin/lookups/kms.rst b/docs/source/cfngin/lookups/kms.rst
index 40684191..88eca43c 100644
--- a/docs/source/cfngin/lookups/kms.rst
+++ b/docs/source/cfngin/lookups/kms.rst
@@ -2,12 +2,27 @@
 kms
 ###
 
-:Query Syntax: ``[<region>@]<encrypted-blob>``
+:Query Syntax: ``<encrypted-blob>[::region=<region>, ...]``
 
 
 The kms_ lookup type decrypts its input value.
 
-As an example, if you have a database and it has a parameter called ``DBPassword`` that you don't want to store in plain text in your config (maybe because you want to check it into your version control system to share with the team), you could instead encrypt the value using ``kms``.
+As an example, if you have a database and it has a parameter called ``DBPassword`` that you don't want to store in plain text in your config (maybe because you want to check it into your version control system to share with the team), you could instead en
+crypt the value using ``kms``.
+
+
+.. versionchanged:: 2.7.0
+  The ``[<region>@]<encrypted-blob>`` syntax is deprecated to comply with Runway's lookup syntax.
+
+
+
+*********
+Arguments
+*********
+
+This Lookup supports all :ref:`Common Lookup Arguments` but, the following have limited or no effect:
+
+- default
 
 
 
@@ -15,28 +30,39 @@ As an example, if you have a database and it has a parameter called ``DBPassword
 Example
 *******
 
-.. code-block:: shell
+We use can use the aws cli to get the encrypted value for the string "PASSWORD" using the master key called 'myKey' in us-east-1.
+
+.. code-block:: console
 
-  # We use the aws cli to get the encrypted value for the string
-  # "PASSWORD" using the master key called 'myKey' in us-east-1
   $ aws --region us-east-1 kms encrypt --key-id alias/myKey \
       --plaintext "PASSWORD" --output text --query CiphertextBlob
 
   CiD6bC8t2Y<...encrypted blob...>
 
-  # With CFNgin we would reference the encrypted value like:
-  DBPassword: ${kms us-east-1@CiD6bC8t2Y<...encrypted blob...>}
+.. code-block:: yaml
 
-  # The above would resolve to
-  DBPassword: PASSWORD
+  namespace: example
 
-This requires that the person using CFNgin has access to the master key used to encrypt the value.
+  stacks:
+    - ...
+      variables:
+        # With CFNgin we would reference the encrypted value like:
+        DBPassword: ${kms CiD6bC8t2Y<...encrypted blob...>::region=us-east-1}
+        # The above would resolve to:
+        DBPassword: PASSWORD
+
+This requires that the credentials used by CFNgin have access to the master key used to encrypt the value.
 
 It is also possible to store the encrypted blob in a file (useful if the value is large) using the ``file://`` prefix, ie:
 
 .. code-block:: yaml
 
-  DockerConfig: ${kms file://dockercfg}
+  namespace: example
+
+  stacks:
+    - ...
+      variables:
+        DockerConfig: ${kms file://dockercfg}
 
 .. note::
-  Lookups resolve the path specified with ``file://`` relative to the location of the config file, not where the CFNgin command is run.
+  Lookups resolve the path specified with ``file://`` relative to the location of the config file, not the current working directory.
diff --git a/runway/cfngin/lookups/handlers/kms.py b/runway/cfngin/lookups/handlers/kms.py
index 193186f1..20a63519 100644
--- a/runway/cfngin/lookups/handlers/kms.py
+++ b/runway/cfngin/lookups/handlers/kms.py
@@ -3,23 +3,45 @@
 from __future__ import annotations
 
 import codecs
-from typing import TYPE_CHECKING, Any, BinaryIO, Union, cast
+import logging
+from typing import TYPE_CHECKING, Any, BinaryIO, Dict, Tuple, Union, cast
 
 from typing_extensions import Final, Literal
 
 from ....lookups.handlers.base import LookupHandler
+from ....utils import DOC_SITE
 from ...utils import read_value_from_path
 
 if TYPE_CHECKING:
     from ....context import CfnginContext
 
+LOGGER = logging.getLogger(__name__)
+
 
 class KmsLookup(LookupHandler):
     """AWS KMS lookup."""
 
+    DEPRECATION_MSG = (
+        'lookup query syntax "<region>@<encrypted-blob>" has been deprecated; '
+        "to learn how to use the new lookup query syntax visit "
+        f"{DOC_SITE}/page/cfngin/lookups/kms.html"
+    )
     TYPE_NAME: Final[Literal["kms"]] = "kms"
     """Name that the Lookup is registered as."""
 
+    @classmethod
+    def legacy_parse(cls, value: str) -> Tuple[str, Dict[str, str]]:
+        """Retain support for legacy lookup syntax.
+
+        Format of value::
+
+            <region>@<encrypted-blob>
+
+        """
+        LOGGER.warning("${%s %s}: %s", cls.TYPE_NAME, value, cls.DEPRECATION_MSG)
+        region, value = read_value_from_path(value).split("@", 1)
+        return value, {"region": region}
+
     @classmethod
     def handle(  # pylint: disable=arguments-differ
         cls, value: str, context: CfnginContext, **_: Any
@@ -30,57 +52,20 @@ class KmsLookup(LookupHandler):
             value: Parameter(s) given to this lookup.
             context: Context instance.
 
-        ``value`` should be in the following format:
-
-            [<region>@]<base64 encrypted value>
-
-        .. note: The region is optional, and defaults to the environment's
-                 ``AWS_DEFAULT_REGION`` if not specified.
-
-        Example:
-            ::
-
-                # We use the aws cli to get the encrypted value for the string
-                # "PASSWORD" using the master key called "myKey" in
-                # us-east-1
-                $ aws --region us-east-1 kms encrypt --key-id alias/myKey \
-                        --plaintext "PASSWORD" --output text --query CiphertextBlob
-
-                CiD6bC8t2Y<...encrypted blob...>
-
-                # With CFNgin we would reference the encrypted value like:
-                conf_key: ${kms us-east-1@CiD6bC8t2Y<...encrypted blob...>}
-
-            You can optionally store the encrypted value in a file, ie::
-
-                kms_value.txt
-                us-east-1@CiD6bC8t2Y<...encrypted blob...>
-
-            and reference it within CFNgin (NOTE: the path should be relative
-            to the CFNgin config file)::
-
-                conf_key: ${kms file://kms_value.txt}
-
-                # Both of the above would resolve to
-                conf_key: PASSWORD
-
         """
-        value = read_value_from_path(value)
-
-        region = None
         if "@" in value:
-            region, value = value.split("@", 1)
-
-        kms = context.get_session(region=region).client("kms")
+            query, args = cls.legacy_parse(value)
+        else:
+            query, args = cls.parse(value)
 
-        # get raw but still encrypted value from base64 version.
-        decoded = codecs.decode(value.encode(), "base64")
+        kms = context.get_session(region=args.get("region")).client("kms")
 
-        # decrypt and return the plain text raw value.
         decrypted = cast(
             Union[BinaryIO, bytes],
-            kms.decrypt(CiphertextBlob=decoded).get("Plaintext", b""),
+            kms.decrypt(CiphertextBlob=codecs.decode(query.encode(), "base64")).get(
+                "Plaintext", b""
+            ),
         )
         if isinstance(decrypted, bytes):
-            return decrypted.decode()
-        return decrypted.read().decode()
+            return cls.format_results(decrypted.decode(), **args)
+        return cls.format_results(decrypted.read().decode(), **args)
diff --git a/runway/cfngin/lookups/handlers/output.py b/runway/cfngin/lookups/handlers/output.py
index 521fe82b..e414ed00 100644
--- a/runway/cfngin/lookups/handlers/output.py
+++ b/runway/cfngin/lookups/handlers/output.py
@@ -46,7 +46,7 @@ class OutputLookup(LookupHandler):
             <relative-stack-name>::<OutputName>
 
         """
-        LOGGER.warning("${output %s}: %s", value, cls.DEPRECATION_MSG)
+        LOGGER.warning("${%s %s}: %s", cls.TYPE_NAME, value, cls.DEPRECATION_MSG)
         return deconstruct(value), {}
 
     @classmethod
diff --git a/runway/cfngin/lookups/handlers/rxref.py b/runway/cfngin/lookups/handlers/rxref.py
index 6ea4b544..a6fcaa6a 100644
--- a/runway/cfngin/lookups/handlers/rxref.py
+++ b/runway/cfngin/lookups/handlers/rxref.py
@@ -38,7 +38,7 @@ class RxrefLookup(LookupHandler):
             <relative-stack-name>::<OutputName>
 
         """
-        LOGGER.warning("${rxref %s}: %s", value, cls.DEPRECATION_MSG)
+        LOGGER.warning("${%s %s}: %s", cls.TYPE_NAME, value, cls.DEPRECATION_MSG)
         return deconstruct(value), {}
 
     @classmethod


Test patch:
diff --git a/tests/unit/cfngin/lookups/handlers/test_kms.py b/tests/unit/cfngin/lookups/handlers/test_kms.py
index 4091cb52..2b175341 100644
--- a/tests/unit/cfngin/lookups/handlers/test_kms.py
+++ b/tests/unit/cfngin/lookups/handlers/test_kms.py
@@ -4,8 +4,11 @@
 from __future__ import annotations
 
 import codecs
+import string
 from typing import TYPE_CHECKING
 
+import pytest
+
 from runway.cfngin.lookups.handlers.kms import KmsLookup
 
 if TYPE_CHECKING:
@@ -17,8 +20,8 @@ SECRET = "my secret"
 class TestKMSHandler:
     """Tests for runway.cfngin.lookups.handlers.kms.KmsLookup."""
 
-    def test_kms_handler(self, cfngin_context: MockCFNginContext) -> None:
-        """Test kms handler."""
+    def test_handle(self, cfngin_context: MockCFNginContext) -> None:
+        """Test handle."""
         stubber = cfngin_context.add_stubber("kms")
         stubber.add_response(
             "decrypt",
@@ -30,9 +33,15 @@ class TestKMSHandler:
             assert KmsLookup.handle(SECRET, context=cfngin_context) == SECRET
             stubber.assert_no_pending_responses()
 
-    def test_kms_handler_with_region(self, cfngin_context: MockCFNginContext) -> None:
-        """Test kms handler with region."""
+    @pytest.mark.parametrize(
+        "template", ["${region}@${blob}", "${blob}::region=${region}"]
+    )
+    def test_handle_with_region(
+        self, cfngin_context: MockCFNginContext, template: str
+    ) -> None:
+        """Test handle with region."""
         region = "us-west-2"
+        query = string.Template(template).substitute({"blob": SECRET, "region": region})
         stubber = cfngin_context.add_stubber("kms", region=region)
 
         stubber.add_response(
@@ -42,7 +51,12 @@ class TestKMSHandler:
         )
 
         with stubber:
-            assert (
-                KmsLookup.handle(f"{region}@{SECRET}", context=cfngin_context) == SECRET
-            )
+            assert KmsLookup.handle(query, context=cfngin_context) == SECRET
             stubber.assert_no_pending_responses()
+
+    def test_legacy_parse(self) -> None:
+        """Test legacy_parse."""
+        assert KmsLookup.legacy_parse("us-east-1@foo") == (
+            "foo",
+            {"region": "us-east-1"},
+        )