fix: Skip functions/layers pointing to S3 location in providers #2756

aahung · 2021-03-22T22:53:38Z

Which issue(s) does this change fix?

Why is this change necessary?

Use the newly added integration test template file as example, this template should be totally buildable:

"tests/integration/testdata/buildcmd/template-with-s3-code.yaml"

AWSTemplateFormatVersion : '2010-09-09'
Transform: AWS::Serverless-2016-10-31


Resources:
  IgnoreServerlessFunction:
    Type: AWS::Serverless::Function
    Properties:
      Handler: app.handler
      Runtime: python3.8
      CodeUri: s3://some/path
      Timeout: 600

  IgnoreServerlessFunction2:
    Type: AWS::Serverless::Function
    Properties:
      Handler: app.handler
      Runtime: python3.8
      CodeUri:
        Bucket: some-bucket
        Key: some-key
      Timeout: 600

  IgnoreLambdaFunction:
    Type: AWS::Lambda::Function
    Properties:
      Handler: app.handler
      Runtime: python3.8
      Code: s3://some/path
      Timeout: 600

  IgnoreLambdaFunction2:
    Type: AWS::Lambda::Function
    Properties:
      Handler: app.handler
      Runtime: python3.8
      Code:
        S3Bucket: some-bucket
        S3Key: some-key
      Timeout: 600

  IgnoreServerlessLayer:
    Type: AWS::Serverless::LayerVersion
    Properties:
      ContentUri: s3://some/path
      CompatibleRuntimes:
        - python3.8

  IgnoreServerlessLayer2:
    Type: AWS::Serverless::LayerVersion
    Properties:
      ContentUri:
        Bucket: some-bucket
        Key: some-key
      CompatibleRuntimes:
        - python3.8

  IgnoreLambdaLayer:
    Type: AWS::Lambda::LayerVersion
    Properties:
      Content: s3://some/path
      CompatibleRuntimes:
        - python3.8

  IgnoreLambdaLayer2:
    Type: AWS::Lambda::LayerVersion
    Properties:
      Content:
        S3Bucket: some-bucket
        S3Key: some-key
      CompatibleRuntimes:
        - python3.8

  # the two functions below should still be built even all layers are not available
  ServerlessFunction:
    Type: AWS::Serverless::Function
    Properties:
      Handler: main.handler
      Runtime: python3.8
      CodeUri: Python
      Timeout: 600
      Layers:
        - !Ref IgnoreServerlessLayer
        - !Ref IgnoreServerlessLayer2
        - !Ref IgnoreLambdaLayer
        - !Ref IgnoreLambdaLayer2

  LambdaFunction:
    Type: AWS::Lambda::Function
    Properties:
      Handler: main.handler
      Runtime: python3.8
      Code: Python
      Timeout: 600
      Layers:
        - !Ref IgnoreServerlessLayer
        - !Ref IgnoreServerlessLayer2
        - !Ref IgnoreLambdaLayer
        - !Ref IgnoreLambdaLayer2

However, currently sam will quit with this error because it will still try to build functions with S3 uri:

$ sam build --use-container --skip-pull-image --cached --parallel -t tests/integration/testdata/buildcmd/template-with-s3-code.yaml

...

Building codeuri: /Users/~/Projects/aws-sam-cli/tests/integration/testdata/buildcmd runtime: python3.8 metadata: {} functions: ['IgnoreServerlessFunction', 'IgnoreServerlessFunction2', 'IgnoreLambdaFunction2']
Requested to skip pulling images ...

Mounting /Users/xinhol/Projects/aws-sam-cli/tests/integration/testdata/buildcmd as /tmp/samcli/source:ro,delegated inside runtime container

Build Failed
Running PythonPipBuilder:ResolveDependencies
Error: PythonPipBuilder:ResolveDependencies - Requirements file not found: /tmp/samcli/source/requirements.txt

How does it address the issue?

When Code, CodeUri, Content, or ContentUri has string value like s3://... or dict value like {"Bucket":.. or {"S3Bucket..., it will be ignored in providers.

Also, I used a single _extract_codeuri() to replace both _extract_lambda_function_code() and _extract_sam_function_codeuri(), which are not consistent regarding the behaviors (one print out the warning while the other does not).

What side effects does this change have?

N/A

Checklist

Add input/output type hints to new functions/methods
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
make update-reproducible-reqs if dependencies were changed
Write documentation

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

aahung · 2021-03-22T23:40:11Z

samcli/lib/providers/sam_function_provider.py

+    @staticmethod
+    def _locate_layer_from_ref(
+        stack: Stack, layer: Dict, use_raw_codeuri: bool = False, ignore_code_extraction_warnings: bool = False
+    ) -> Optional[LayerVersion]:


explain: pylint complains about _parse_layer_info() being too complex, here I extract some logic to _locate_layer_from_ref()

aahung · 2021-03-22T23:40:58Z

samcli/lib/providers/sam_base_provider.py

-                LOG.warning(
-                    "Lambda function '%s' has specified S3 location for CodeUri which is unsupported. "
-                    "Using default value of '%s' instead",
-                    name,
-                    SamBaseProvider.DEFAULT_CODEURI,
-                )
-            return SamBaseProvider.DEFAULT_CODEURI


explain: now the warning is being handled by SamFunctionProvider and SamLayerProviders.

sriram-mv · 2021-03-23T00:13:22Z

samcli/lib/providers/sam_base_provider.py

@@ -27,6 +27,13 @@ class SamBaseProvider:
    CLOUDFORMATION_STACK = AWS_CLOUDFORMATION_STACK
    DEFAULT_CODEURI = "."

+    CODE_PROPERTY_KEYS = {


sam package already has a bunch of these constants for package-able code paths, would it make sense to refactor that, so thats its usable across the code base?

I have another PR ready for this purpose (coming right after this PR is merged)

sriram-mv · 2021-03-23T00:21:40Z

samcli/lib/providers/sam_function_provider.py

+            codeuri = SamBaseProvider._extract_codeuri(layer_properties, code_property_key)
+
+        if codeuri and not use_raw_codeuri:
+            LOG.debug("--base-dir is presented not, adjusting uri %s relative to %s", codeuri, stack.location)


There's some grammar issue on this sentence.

sriram-mv · 2021-03-23T00:27:24Z

tests/integration/buildcmd/test_build_cmd.py

+        "requirements.txt",
+    }
+
+    # @pytest.mark.flaky(reruns=3)


comment that might need to be removed?

sriram-mv · 2021-03-23T00:30:32Z

tests/integration/buildcmd/test_build_cmd.py

+            )
+
+            overrides = self._make_parameter_override_arg({})
+            self._verify_invoke_built_functions(


The template is buildable because s3 based locations are explicitly skipped, what does the local invoke do here?

~~good point. I don't think functions containing these remote layers should be local invocable at all. What do you think, disallow their invocation? @sriram-mv~~ in #2760 I tried to prevent these functions from being invoked, but after discussing with Mehmet, that change might be risky. So I removed it. these two functions are still invocable so we should still test invoke them.

~~I implemented this integ test base in #2760, will rebase this PR once #2760 is merged.~~

maybe only test building? I feel that testing invoke might be unnecessary here

no strong opinion here, the invoke is added based on the current sam-cli behavior, we might not need this in the future. here removed

does the invoke just skip? what is our current behavior? could we add a comment on what the behavior we are trying to test. This will be useful for future deep dives.

Documented!

samcli/lib/providers/sam_function_provider.py

qingchm

Open question: should we add any special handling in local invoke/start api if the template contains s3 locations? I mean, any warnings?

aahung · 2021-03-25T00:25:22Z

Open question: should we add any special handling in local invoke/start api if the template contains s3 locations? I mean, any warnings?

good question. I think we should. As right now, customers will see a universal warning "AWS::Serverless::LayerVersion XXX has specified S3 location for ContentUri which is unsupported." I think it is good enough to raise the concern.

But for other cases (like #2760 pass-up example), we don't have the warning yet, but we have LOG.debug in place.

aws-sam-cli/samcli/lib/providers/sam_function_provider.py

Lines 424 to 428 in a8be3ae

    
           else: 
        
               LOG.debug( 
        
                   'layer "%s" is not recognizable, ' 
        
                   "it might be using intrinsic functions that we don't support yet. Skipping.", 
        
                   str(layer),

qingchm · 2021-03-25T00:32:56Z

#2760

To me the easier solution here would be during the build time if we have the is_s3 function returned true, we can log a message warning about local invoke might fail. The more complex approach would be to add the message at invoke time so that they know why the invoke didn't work. For me personally I prefer the first option (kind of aligns with the layer behaviour), but as I said this is an open question so more opinions are strongly welcomed

hoffa · 2021-03-25T00:27:32Z

samcli/lib/providers/sam_base_provider.py

        codeuri = resource_properties.get(code_property_key, SamBaseProvider.DEFAULT_CODEURI)

        if isinstance(codeuri, dict):
            return SamBaseProvider.DEFAULT_CODEURI

        return cast(str, codeuri)

+    @staticmethod
+    def _is_code_uri_s3(codeuri: Optional[Any]) -> bool:


nit: isn't Optional[Any] == Any?

Can codeuri be anything other than None, dict, str? Shouldn't we use something like Optional[Union[str, Dict]]?

nit: isn't Optional[Any] == Any?

yeah right.

I changed to Optional[Union[str, Dict]]

hoffa · 2021-03-25T00:35:20Z

samcli/lib/providers/sam_base_provider.py

+        )
+
+    @staticmethod
+    def _warn_code_extraction(resource_type: str, resource_name: str, code_property: str) -> None:


Feels slightly weird to have the logging specifically refactored out. Doesn't seem very meaningful on its own.

Would it make sense to (e.g.) instead have a method whose sole purpose is to check whether a resource uses an S3-based CodeUri? Something like that should also allow simplifying the 3 very similar use cases in this PR.

I think we should do something like we did in #2691 (we had similar discussion before).

Yeah so I think the whole flow in these methods feels weird as we're essentially dealing with two very similar abstractions (AWS::Serverless::Function and AWS::Lambda::Function) with vaguely-defined operations (e.g. methods that deal with both abstractions) but still keep repeating similar logic for both.

Not in this scope so I'm happy with this for now, but ideally I'm thinking we would have a well-defined abstraction to deal with functions, e.g. by wrapping them or performing a translation beforehand so we only deal with a subset of well-defined resource types with well-defined operations, so that we don't need all this if and isinstance ceremony scattered across the codebase.

aahung · 2021-03-25T00:42:48Z

#2760

To me the easier solution here would be during the build time if we have the is_s3 function returned true, we can log a message warning about local invoke might fail. The more complex approach would be to add the message at invoke time so that they know why the invoke didn't work. For me personally I prefer the first option (kind of aligns with the layer behaviour), but as I said this is an open question so more opinions are strongly welcomed

That's a good point. For this PR, it is a bit out of the scope.

qingchm · 2021-03-25T00:48:14Z

samcli/lib/providers/sam_base_provider.py

+    @staticmethod
+    def _warn_code_extraction(resource_type: str, resource_name: str, code_property: str) -> None:
+        LOG.warning(
+            "%s '%s' has specified S3 location for %s which is unsupported.",


This warning feels a bit ambiguous here. Shouldn't we specify the scope of this "unsupported"?

sriram-mv · 2021-03-30T19:07:18Z

samcli/lib/providers/sam_function_provider.py

@@ -129,6 +129,14 @@ def _extract_functions(
                if resource_metadata:
                    resource_properties["Metadata"] = resource_metadata

+                if resource_type in [SamFunctionProvider.SERVERLESS_FUNCTION, SamFunctionProvider.LAMBDA_FUNCTION]:
+                    code_property_key = SamBaseProvider.CODE_PROPERTY_KEYS[resource_type]
+                    if SamBaseProvider._is_code_uri_s3(resource_properties.get(code_property_key)):


should we rename this method? _is_code_uri_s3, we explicitly take in the key to look up in the function. it could be CodeUri, ContentUri, Code, Content.

good catch. Renamed to

@staticmethod def _is_s3_location(location: Optional[Union[str, Dict]]) -> bool: """ the input could be: - CodeUri of Serverless::Function - Code of Lambda::Function - ContentUri of Serverless::LayerVersion - Content of Lambda::LayerVersion """ return (isinstance(location, dict) and ("S3Bucket" in location or "Bucket" in location)) or ( isinstance(location, str) and location.startswith("s3://") )

…2756) * refactor: Extract logics into _locate_layer_from_ref() * providers no longer extract functions/layers with s3 uri

aahung force-pushed the skip-s3-in-providers branch 3 times, most recently from da68d27 to 7ab4bb2 Compare March 22, 2021 23:26

aahung marked this pull request as ready for review March 22, 2021 23:34

aahung requested a review from sriram-mv March 22, 2021 23:34

aahung changed the title ~~fix: Skip functions/layers pointing to s3 location in providers~~ fix: Skip functions/layers pointing to S3 location in providers Mar 22, 2021

aahung requested review from jfuss and mndeveci March 22, 2021 23:34

aahung force-pushed the skip-s3-in-providers branch from 7ab4bb2 to c1b072c Compare March 22, 2021 23:36

aahung added the pr/internal label Mar 22, 2021

aahung commented Mar 22, 2021

View reviewed changes

aahung force-pushed the skip-s3-in-providers branch from c1b072c to 76395ec Compare March 23, 2021 00:10

sriram-mv suggested changes Mar 23, 2021

View reviewed changes

aahung commented Mar 24, 2021

View reviewed changes

samcli/lib/providers/sam_function_provider.py Show resolved Hide resolved

aahung requested review from CoshUS and qingchm March 24, 2021 21:57

aahung added 4 commits March 24, 2021 16:10

refactor: Extract logics into _locate_layer_from_ref()

b088af7

providers no longer extract functions/layers with s3 uri

4a5b6da

Fix grammar issues

67c61f9

Unit tests and integ tests

f13ad32

aahung force-pushed the skip-s3-in-providers branch from b425b03 to f13ad32 Compare March 24, 2021 23:15

qingchm reviewed Mar 25, 2021

View reviewed changes

hoffa reviewed Mar 25, 2021

View reviewed changes

qingchm reviewed Mar 25, 2021

View reviewed changes

aahung added 2 commits March 24, 2021 17:56

Replace unsupported with more concrete words

21183f0

Refine types

5313334

hoffa approved these changes Mar 25, 2021

View reviewed changes

Merge branch 'develop' into skip-s3-in-providers

10182dc

Base automatically changed from develop to elbayaaa-develop March 25, 2021 21:28

Base automatically changed from elbayaaa-develop to develop March 25, 2021 21:38

aahung and others added 3 commits March 25, 2021 17:44

Merge branch 'develop' into skip-s3-in-providers

980d895

Merge branch 'develop' into skip-s3-in-providers

880f057

Update integ tests

4a0cd6c

qingchm approved these changes Mar 30, 2021

View reviewed changes

sriram-mv suggested changes Mar 30, 2021

View reviewed changes

aahung added 2 commits March 30, 2021 12:18

Add doc about skipping invoking

1ce6126

_is_code_uri_s3 -> _is_s3_location

8632564

sriram-mv approved these changes Mar 30, 2021

View reviewed changes

aahung merged commit fd25093 into aws:develop Mar 30, 2021

jfuss mentioned this pull request Apr 14, 2021

The resource AWS::Lambda::Function 'xxx4FF3C67E' has specified S3 location for Code. It will not be built and SAM CLI does not support invoking it locally. #2802

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Skip functions/layers pointing to S3 location in providers #2756

fix: Skip functions/layers pointing to S3 location in providers #2756

aahung commented Mar 22, 2021 •

edited

Loading

aahung Mar 22, 2021 •

edited

Loading

aahung Mar 22, 2021

sriram-mv Mar 23, 2021

aahung Mar 23, 2021

aahung Mar 24, 2021 •

edited

Loading

sriram-mv Mar 23, 2021

aahung Mar 23, 2021

sriram-mv Mar 23, 2021

aahung Mar 23, 2021

sriram-mv Mar 23, 2021

aahung Mar 23, 2021 •

edited

Loading

aahung Mar 23, 2021 •

edited

Loading

qingchm Mar 26, 2021

aahung Mar 30, 2021

sriram-mv Mar 30, 2021

aahung Mar 30, 2021

qingchm left a comment •

edited

Loading

aahung commented Mar 25, 2021

qingchm commented Mar 25, 2021 •

edited

Loading

hoffa Mar 25, 2021

qingchm Mar 25, 2021 •

edited

Loading

aahung Mar 25, 2021 •

edited

Loading

hoffa Mar 25, 2021

aahung Mar 25, 2021

hoffa Mar 25, 2021 •

edited

Loading

aahung commented Mar 25, 2021 •

edited

Loading

qingchm Mar 25, 2021

sriram-mv Mar 30, 2021

aahung Mar 30, 2021

fix: Skip functions/layers pointing to S3 location in providers #2756

fix: Skip functions/layers pointing to S3 location in providers #2756

Conversation

aahung commented Mar 22, 2021 • edited Loading

Which issue(s) does this change fix?

Why is this change necessary?

How does it address the issue?

What side effects does this change have?

Checklist

aahung Mar 22, 2021 • edited Loading

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

aahung Mar 24, 2021 • edited Loading

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

aahung Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

aahung Mar 23, 2021 • edited Loading

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

qingchm left a comment • edited Loading

Choose a reason for hiding this comment

aahung commented Mar 25, 2021

qingchm commented Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

qingchm Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

aahung Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoffa Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

aahung commented Mar 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aahung commented Mar 22, 2021 •

edited

Loading

aahung Mar 22, 2021 •

edited

Loading

aahung Mar 24, 2021 •

edited

Loading

aahung Mar 23, 2021 •

edited

Loading

aahung Mar 23, 2021 •

edited

Loading

qingchm left a comment •

edited

Loading

qingchm commented Mar 25, 2021 •

edited

Loading

qingchm Mar 25, 2021 •

edited

Loading

aahung Mar 25, 2021 •

edited

Loading

hoffa Mar 25, 2021 •

edited

Loading

aahung commented Mar 25, 2021 •

edited

Loading