Intrinsics support #1261

viksrivat · 2019-07-09T17:19:02Z

Issue #, if available:
#528
Description of changes:
#1260

Checklist:

Write Design Document (Do I need to write a design document?)
Write unit tests
Write/update functional tests
Write/update integration tests
make pr passes
Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

jfuss

I didn't review any of the actually intrinsic resolver functions. This PR is pretty big and has lots of logic. It may be work breaking this up into a short form to make this easier to review and consume.

samcli/commands/local/lib/intrinsic_resolver/intrinsic_property_resolver.py

jfuss · 2019-07-15T23:39:24Z

samcli/commands/local/lib/intrinsic_resolver/intrinsic_property_resolver.py

+        """
+        processed_template = {}
+        for key, val in self.resources.items():
+            processed_key = self.symbol_resolver.get_translation(key, IntrinsicResolver.REF) or key


IntrinsicResolver.REF isn't need here, get_translation already defaults to this.

What does get_translation do here? It's not clear what the translation is referring too.

get_translation simplifies something like to RestApi.Deployment -> NewRestApi.

jfuss · 2019-07-15T23:45:36Z

samcli/commands/local/lib/intrinsic_resolver/intrinsic_property_resolver.py

+            processed_key = self.symbol_resolver.get_translation(key, IntrinsicResolver.REF) or key
+            try:
+                processed_resource = self.intrinsic_property_resolver(val)
+                processed_template[processed_key] = processed_resource


Not sure I follow what is going on here. Isn't processed_key always equal to key (aka. the logical id)?

There are cases when processed_key != key. The example in the SAM Ref code was RestApi.Deployment -> NewRestApi. This will be updated based on parameters to the logical_id_translator.

jfuss

This looks really good. Love how deep you went in unit testing. Make understand and verification easier now and in the future.

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py

jfuss · 2019-07-30T21:31:38Z

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py

+        -------
+        An InvalidIntrinsicException
+        """
+        raise InvalidIntrinsicException("Fn::ImportValue is currently not supported by IntrinsicResolver")


One thing we need to make sure is that we log debug messages in these cases (maybe in info level) so customers understand what we support or don't support.

Sidenote: We will not want to fail resolving intrinsics initially, or at least until we know we have all things covered correctly. Otherwise, this will become a blocker for customers when they try invoking/building/etc.

Currently, I was handling for that in resolve_template() function. This has a property called ignore_errors, which we can keep on till we have covered everything. If ignore_errors is turned on, then it will run LOG.error("Unable to process properties of %s.%s", key, resource_type) and keep the current resource unmodified.
Ex Code around line 170:
except (InvalidIntrinsicException, InvalidSymbolException) as e:
resource_type = val.get("Type", "")
if ignore_errors:
LOG.error("Unable to process properties of %s.%s", key, resource_type)
processed_template[key] = val

It would be helpful if these logs were only printed if the --debug flag is set, or if the log only printed if the property was one that should be possible to process but for whatever reasons couldn't be. Currently there is a lot of noise in terminal for unprocessed properties that SAM CLI can not process i.e. ImportValue

jfuss · 2019-07-30T21:34:49Z

samcli/lib/intrinsic_resolver/intrinsics_symbol_table.py

+        -------
+        This resolves the attribute
+        """
+        # pylint: disable-msg=too-many-return-statements


This is an indication that we should split this function out or find a better way to manage the control flows to reduce return statements (in some sane way).

jfuss · 2019-07-30T21:39:11Z

samcli/lib/intrinsic_resolver/intrinsics_symbol_table.py

+        function_name = logical_id
+        function_name = self.logical_id_translator.get(function_name) or function_name
+        str_format = "arn:{partition_name}:{service_name}:{aws_region}:{account_id}:{function_name}"
+        if service_name == "lambda":


I wonder what other services have similar (custom) things in their arns.

Can we look at a few other services? This function looks like it should be generic but has function_name in all arns, so it reads a little off. Also, I have the general concern that Lambda isn't the only special arn one here. Keeping this updated with every service, could be a big pain.

The arn types are
arn:partition:service:region:account-id:resource
arn:partition:service:region:account-id:resourcetype/resource
arn:partition:service:region:account-id:resourcetype/resource/qualifier
arn:partition:service:region:account-id:resourcetype/resource:qualifier
arn:partition:service:region:account-id:resourcetype:resource
arn:partition:service:region:account-id:resourcetype:resource:qualifier

The ones I looked at sns/sqs followed the default
str_format = "arn:{partition_name}:{service_name}:{aws_region}:{account_id}:{function_name}"

Before, we also only supported dealing with Lambda Functions. This function will also only be called as a default case if they do not specify and override for resolving GetAtt for Arns.

That's fine but then we shouldn't have the str_format be tied to function_name. My callout is an arn for sqs doesn't have a function_name, it has a queue_name. Since this is broad we need to make sure these reads as such. Reading it as it is written, makes it seems like lambda is only supported.

tests/unit/lib/intrinsic_resolver/test_intrinsic_resolver.py

The json might get outdated and tools like cfn-lint are handling it.

jfuss

Overall looking good.

Can we do better on covering unit tests coverage here?

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py      264     15     53      3    93%   83, 156, 169, 245, 247, 468, 488-501, 244->245, 246->247, 467->468
samcli/lib/intrinsic_resolver/intrinsics_symbol_table.py          115     12     32      6    86%   198, 204, 211-215, 271, 289, 337, 366, 378, 390, 197->198, 203->204, 208->213, 209->211, 242->245, 270->271

jfuss · 2019-08-07T03:41:39Z

samcli/lib/intrinsic_resolver/invalid_intrinsic_exception.py

@@ -0,0 +1,71 @@
+"""
+A custom Exception to display Invalid Intrinsics and Symbol Table format.
+It also has a list of helper functions that cleanup the processing in IntrinsicResolver and IntrinsicSymbolTable


You should separate out the two concrete things that are in this file. "It also has" indicates this is not scoped. Or you can rename this file to be more encompassing of the exceptions and the validation methods.

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py

jfuss · 2019-08-07T04:19:56Z

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py

+            self._symbol_resolver = symbol_resolver
+
+        processed_template = {}
+        for key, val in self._resources.items():


This and any of the other _ properties (self._mapping, self._parameters, or self._conditions) can result in an AttributeError due to the early exit if the template is None or empty (line 95). This is also why it's important not to ignore the attribute-defined-outside-init that pylint throws.

All of these properties are now initialized before calling this to prevent an AttributeError

jfuss · 2019-08-07T04:23:22Z

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py

+        template: dict
+            A dictionary containing all the resources and the attributes of the CloudFormation template. The class does
+            not mutate the template.
+        ignore_errors: bool


Should ignore_errors ignore any and all errors/exceptions or only certain ones?

I updated the docstring to reflect that ignore_errors only ignores InvalidIntrinsicExceptions

jfuss · 2019-08-07T04:25:41Z

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py

+
+        processed_template = {}
+        for key, val in self._resources.items():
+            processed_key = self._symbol_resolver.get_translation(key) or key


Should self._symbol_resolver.get(key) just default to returning the key back if one can't be found?

The reason I think we should continue to return None for self._symbol_resolver.get(key) is because we may want to differentiate between keys that don't exist and symbols that references to a logical_id in the future. This also makes it feel like dict.get(), which returns a default of None.

jfuss · 2019-08-07T04:31:49Z

samcli/lib/intrinsic_resolver/intrinsics_symbol_table.py

+        function_name = logical_id
+        function_name = self.logical_id_translator.get(function_name) or function_name
+        str_format = "arn:{partition_name}:{service_name}:{aws_region}:{account_id}:{function_name}"
+        if service_name == "lambda":


Can we look at a few other services? This function looks like it should be generic but has function_name in all arns, so it reads a little off. Also, I have the general concern that Lambda isn't the only special arn one here. Keeping this updated with every service, could be a big pain.

samcli/lib/intrinsic_resolver/intrinsics_symbol_table.py

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py

samcli/lib/intrinsic_resolver/intrinsics_symbol_table.py

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py

tests/unit/lib/intrinsic_resolver/test_intrinsic_resolver.py

sriram-mv · 2019-08-07T18:25:31Z

One general feedback is to submit smaller PRs, for eg: instrintic property resolver with one just one instrinsic tested, and to build on them.

jfuss

Almost there! flake8 is failing, make sure you run it locally and address the failures.

jfuss · 2019-08-08T19:48:10Z

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py

+
+    CONDITIONAL_FUNCTIONS = [FN_AND, FN_OR, FN_IF, FN_EQUALS, FN_NOT]
+
+    def __init__(self, template=None, symbol_resolver=None):


Should this require a template?

jfuss · 2019-08-08T19:53:05Z

samcli/lib/intrinsic_resolver/intrinsic_property_resolver.py

+        -----------
+        ignore_errors: bool
+            An option to ignore errors that are InvalidIntrinsicException and InvalidSymbolException
+        symbol_resolver: IntrinsicSymbolTable


I find it a little strange that after creating the object, you would want to override this on the resolve itself. What is the reasoning behind this, vs the caller creating another object with the new symbol_resolver?

jfuss · 2019-08-08T21:04:15Z

samcli/lib/intrinsic_resolver/intrinsics_symbol_table.py

+                "Arn:": arn_resolver
+            }
+        """
+        if template is None:


You don't do anything be set template to self._template so this check can be wrapped into that assignment.

jfuss · 2019-08-08T21:07:58Z

samcli/lib/intrinsic_resolver/intrinsics_symbol_table.py

+        function_name = logical_id
+        function_name = self.logical_id_translator.get(function_name) or function_name
+        str_format = "arn:{partition_name}:{service_name}:{aws_region}:{account_id}:{function_name}"
+        if service_name == "lambda":


That's fine but then we shouldn't have the str_format be tied to function_name. My callout is an arn for sqs doesn't have a function_name, it has a queue_name. Since this is broad we need to make sure these reads as such. Reading it as it is written, makes it seems like lambda is only supported.

…om:viksrivat/aws-sam-cli into feature/cloud_formation_intrinsics_support

jfuss · 2019-08-09T03:14:42Z

tests/unit/lib/intrinsic_resolver/test_intrinsic_resolver.py

@@ -0,0 +1,1347 @@
+import json
+from copy import deepcopy
+from pathlib import Path


pathlib is not in the Python2.7 standard library. You will need to do

try: import pathlib except ImportError: import pathlib2 as pathlib

to make the Py2.7 tests pass.

viksrivat added 6 commits July 9, 2019 10:18

Add intrinsics support

0ebcb82

Add __init__.py to intrinsic_resolver folder

d298e96

Fix style issues with make pr

11911ec

Fix python2 style issue

e77a0fa

Fix python2 support bug

3aefa5d

Cleanup imports

ccc4d4b

jfuss self-assigned this Jul 15, 2019

jfuss requested changes Jul 15, 2019

View reviewed changes

Update with comments

8427dde

jfuss mentioned this pull request Jul 22, 2019

fix(layers): show warning if !Sub function is used in Layers section of template.yaml (#948) #949

Closed

6 tasks

jfuss requested changes Jul 30, 2019

View reviewed changes

jfuss requested a review from sriram-mv July 30, 2019 22:51

viksrivat added 4 commits July 31, 2019 09:22

Remove CloudFormation Resource specification.json

477336a

The json might get outdated and tools like cfn-lint are handling it.

Add deep copy to template

0bac081

Update code to reflect intrinsics within delimeter

18ad1ff

Expose private variables to improve flexibility

3d40403

jfuss requested changes Aug 7, 2019

View reviewed changes

viksrivat added 2 commits August 7, 2019 09:55

Fix Template init and Psuedo region

d9139bd

Move intrinsic_property tests from 94% -> 100%

26dde18

sriram-mv reviewed Aug 7, 2019

View reviewed changes

viksrivat and others added 4 commits August 7, 2019 11:43

Update symbol resolver from 88% -> 100%

719cb8a

Update ith position to handle first,second,third

27d3d9b

Move large test to test_data

2fa053a

Merge branch 'develop' into feature/cloud_formation_intrinsics_support

6cf4839

jfuss requested changes Aug 8, 2019

View reviewed changes

viksrivat added 2 commits August 8, 2019 15:58

Update with comments

49ad1af

Merge branch 'feature/cloud_formation_intrinsics_support' of github.c…

847a2b7

…om:viksrivat/aws-sam-cli into feature/cloud_formation_intrinsics_support

jfuss reviewed Aug 9, 2019

View reviewed changes

Fix pathlib2

508fcd0

Update Style

315bd8b

jfuss approved these changes Aug 9, 2019

View reviewed changes

Merge branch 'develop' into feature/cloud_formation_intrinsics_support

ea02fbd

jfuss merged commit ca93e83 into aws:develop Aug 9, 2019

viksrivat mentioned this pull request Aug 12, 2019

Integrate Intrinsics #1333

Merged

6 tasks

michaelj-smith mentioned this pull request Oct 17, 2019

Default parameter value of Empty String is not recognized in processing of Intrinsic function #1476

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intrinsics support #1261

Intrinsics support #1261

viksrivat commented Jul 9, 2019 •

edited

Loading

jfuss left a comment

jfuss Jul 15, 2019

viksrivat Jul 16, 2019

jfuss Jul 15, 2019

viksrivat Jul 16, 2019

jfuss left a comment

jfuss Jul 30, 2019

viksrivat Jul 31, 2019

daveykane Sep 9, 2019

jfuss Jul 30, 2019

jfuss Jul 30, 2019

jfuss Aug 7, 2019

viksrivat Aug 7, 2019

jfuss Aug 8, 2019

jfuss left a comment

jfuss Aug 7, 2019

jfuss Aug 7, 2019

viksrivat Aug 8, 2019

jfuss Aug 7, 2019

viksrivat Aug 8, 2019

jfuss Aug 7, 2019

viksrivat Aug 7, 2019

jfuss Aug 7, 2019

sriram-mv commented Aug 7, 2019

jfuss left a comment

jfuss Aug 8, 2019

jfuss Aug 8, 2019

jfuss Aug 8, 2019

jfuss Aug 8, 2019

jfuss Aug 9, 2019


		CONDITIONAL_FUNCTIONS = [FN_AND, FN_OR, FN_IF, FN_EQUALS, FN_NOT]

		def __init__(self, template=None, symbol_resolver=None):

Intrinsics support #1261

Intrinsics support #1261

Conversation

viksrivat commented Jul 9, 2019 • edited Loading

jfuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sriram-mv commented Aug 7, 2019

jfuss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viksrivat commented Jul 9, 2019 •

edited

Loading