Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 12 commits into from
Mar 30, 2021
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b088af7
refactor: Extract logics into _locate_layer_from_ref()
aahung Mar 24, 2021
4a5b6da
providers no longer extract functions/layers with s3 uri
aahung Mar 24, 2021
67c61f9
Fix grammar issues
aahung Mar 24, 2021
f13ad32
Unit tests and integ tests
aahung Mar 24, 2021
21183f0
Replace unsupported with more concrete words
aahung Mar 25, 2021
5313334
Refine types
aahung Mar 25, 2021
10182dc
Merge branch 'develop' into skip-s3-in-providers
aahung Mar 25, 2021
980d895
Merge branch 'develop' into skip-s3-in-providers
aahung Mar 26, 2021
880f057
Merge branch 'develop' into skip-s3-in-providers
aahung Mar 30, 2021
4a0cd6c
Update integ tests
aahung Mar 30, 2021
1ce6126
Add doc about skipping invoking
aahung Mar 30, 2021
8632564
_is_code_uri_s3 -> _is_s3_location
aahung Mar 30, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
providers no longer extract functions/layers with s3 uri
  • Loading branch information
@aahung
aahung committed Mar 24, 2021
commit 4a5b6dadcc1f1f2d26513059f78ddedecd7514ae
66 changes: 24 additions & 42 deletions samcli/lib/providers/sam_base_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ class SamBaseProvider:
CLOUDFORMATION_STACK = AWS_CLOUDFORMATION_STACK
DEFAULT_CODEURI = "."

CODE_PROPERTY_KEYS = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea

Copy link
Contributor Author

@aahung aahung Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LAMBDA_FUNCTION: "Code",
SERVERLESS_FUNCTION: "CodeUri",
LAMBDA_LAYER: "Content",
SERVERLESS_LAYER: "ContentUri",
}

def get(self, name: str) -> Optional[Any]:
"""
Given name of the function, this method must return the Function object
Expand All @@ -45,9 +52,9 @@ def get_all(self) -> Iterable:
raise NotImplementedError("not implemented")

@staticmethod
def _extract_lambda_function_code(resource_properties: Dict, code_property_key: str) -> str:
def _extract_codeuri(resource_properties: Dict, code_property_key: str) -> str:
"""
Extracts the Lambda Function Code from the Resource Properties
Extracts the Function/Layer code path from the Resource Properties

Parameters
----------
Expand All @@ -61,14 +68,28 @@ def _extract_lambda_function_code(resource_properties: Dict, code_property_key:
str
Representing the local code path
"""

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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@qingchm qingchm Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@aahung aahung Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

yeah right.

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

return (isinstance(codeuri, dict) and ("S3Bucket" in codeuri or "Bucket" in codeuri)) or (
isinstance(codeuri, str) and codeuri.startswith("s3://")
)

@staticmethod
def _warn_code_extraction(resource_type: str, resource_name: str, code_property: str) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@hoffa hoffa Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

LOG.warning(
"%s '%s' has specified S3 location for %s which is unsupported.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

resource_type,
resource_name,
code_property,
)

@staticmethod
def _extract_lambda_function_imageuri(resource_properties: Dict, code_property_key: str) -> Optional[str]:
"""
Expand Down Expand Up @@ -107,45 +128,6 @@ def _extract_sam_function_imageuri(resource_properties: Dict, code_property_key:
"""
return resource_properties.get(code_property_key, None)

@staticmethod
def _extract_sam_function_codeuri(
name: str,
resource_properties: Dict[str, Any],
code_property_key: str,
ignore_code_extraction_warnings: bool = False,
) -> str:
"""
Extracts the SAM Function CodeUri from the Resource Properties

Parameters
----------
name str
LogicalId of the resource
resource_properties dict
Dictionary representing the Properties of the Resource
code_property_key str
Property Key of the code on the Resource
ignore_code_extraction_warnings
Boolean to ignore log statements on code extraction from Resources.

Returns
-------
str
Representing the local code path
"""
codeuri = resource_properties.get(code_property_key, SamBaseProvider.DEFAULT_CODEURI)
# CodeUri can be a dictionary of S3 Bucket/Key or a S3 URI, neither of which are supported
if isinstance(codeuri, dict) or (isinstance(codeuri, str) and codeuri.startswith("s3://")):
if not ignore_code_extraction_warnings:
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
Comment on lines -140 to -146
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return cast(str, codeuri)

@staticmethod
def get_template(template_dict: Dict, parameter_overrides: Optional[Dict[str, str]] = None) -> Dict:
"""
Expand Down
39 changes: 21 additions & 18 deletions samcli/lib/providers/sam_function_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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://")
    )

# CodeUri can be a dictionary of S3 Bucket/Key or a S3 URI, neither of which are supported
if not ignore_code_extraction_warnings:
SamFunctionProvider._warn_code_extraction(resource_type, name, code_property_key)
continue

if resource_type == SamFunctionProvider.SERVERLESS_FUNCTION:
layers = SamFunctionProvider._parse_layer_info(
stack,
Expand All @@ -142,7 +150,6 @@ def _extract_functions(
resource_properties,
layers,
use_raw_codeuri,
ignore_code_extraction_warnings=ignore_code_extraction_warnings,
)
result[function.full_path] = function

Expand All @@ -169,7 +176,6 @@ def _convert_sam_function_resource(
resource_properties: Dict,
layers: List[LayerVersion],
use_raw_codeuri: bool = False,
ignore_code_extraction_warnings: bool = False,
) -> Function:
"""
Converts a AWS::Serverless::Function resource to a Function configuration usable by the provider.
Expand Down Expand Up @@ -197,12 +203,7 @@ def _convert_sam_function_resource(
LOG.debug("Found Serverless function with name='%s' and InlineCode", name)
codeuri = None
else:
codeuri = SamFunctionProvider._extract_sam_function_codeuri(
name,
resource_properties,
"CodeUri",
ignore_code_extraction_warnings=ignore_code_extraction_warnings,
)
codeuri = SamBaseProvider._extract_codeuri(resource_properties, "CodeUri")
LOG.debug("Found Serverless function with name='%s' and CodeUri='%s'", name, codeuri)
elif packagetype == IMAGE:
imageuri = SamFunctionProvider._extract_sam_function_imageuri(resource_properties, "ImageUri")
Expand Down Expand Up @@ -252,7 +253,7 @@ def _convert_lambda_function_resource(
LOG.debug("Found Lambda function with name='%s' and Code ZipFile", name)
codeuri = None
else:
codeuri = SamFunctionProvider._extract_lambda_function_code(resource_properties, "Code")
codeuri = SamBaseProvider._extract_codeuri(resource_properties, "Code")
LOG.debug("Found Lambda function with name='%s' and CodeUri='%s'", name, codeuri)
elif packagetype == IMAGE:
imageuri = SamFunctionProvider._extract_lambda_function_imageuri(resource_properties, "Code")
Expand Down Expand Up @@ -390,7 +391,8 @@ def _parse_layer_info(
found_layer = SamFunctionProvider._locate_layer_from_ref(
stack, layer, use_raw_codeuri, ignore_code_extraction_warnings
)
layers.append(found_layer)
if found_layer:
layers.append(found_layer)
else:
LOG.debug(
'layer "%s" is not recognizable, '
Expand All @@ -403,7 +405,7 @@ def _parse_layer_info(
@staticmethod
def _locate_layer_from_ref(
stack: Stack, layer: Dict, use_raw_codeuri: bool = False, ignore_code_extraction_warnings: bool = False
) -> LayerVersion:
) -> Optional[LayerVersion]:
layer_logical_id = cast(str, layer.get("Ref"))
layer_resource = stack.resources.get(layer_logical_id)
if not layer_resource or layer_resource.get("Type", "") not in (
Expand All @@ -417,13 +419,14 @@ def _locate_layer_from_ref(
compatible_runtimes = layer_properties.get("CompatibleRuntimes")
codeuri: Optional[str] = None

if resource_type == SamFunctionProvider.LAMBDA_LAYER:
codeuri = SamFunctionProvider._extract_lambda_function_code(layer_properties, "Content")

if resource_type == SamFunctionProvider.SERVERLESS_LAYER:
codeuri = SamFunctionProvider._extract_sam_function_codeuri(
layer_logical_id, layer_properties, "ContentUri", ignore_code_extraction_warnings
)
if resource_type in [SamFunctionProvider.LAMBDA_LAYER, SamFunctionProvider.SERVERLESS_LAYER]:
code_property_key = SamBaseProvider.CODE_PROPERTY_KEYS[resource_type]
if SamBaseProvider._is_code_uri_s3(layer_properties.get(code_property_key)):
# Content can be a dictionary of S3 Bucket/Key or a S3 URI, neither of which are supported
if not ignore_code_extraction_warnings:
SamFunctionProvider._warn_code_extraction(resource_type, layer_logical_id, code_property_key)
return None
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some grammar issue on this sentence.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch

Expand Down
60 changes: 41 additions & 19 deletions samcli/lib/providers/sam_layer_provider.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,38 +81,60 @@ def _extract_layers(self) -> List[LayerVersion]:
layers = []
for stack in self._stacks:
for name, resource in stack.resources.items():
if resource.get("Type") in [self.LAMBDA_LAYER, self.SERVERLESS_LAYER]:
layers.append(self._convert_lambda_layer_resource(stack, name, resource))
# In the list of layers that is defined within a template, you can reference a LayerVersion resource.
# When running locally, we need to follow that Ref so we can extract the local path to the layer code.
resource_type = resource.get("Type")
resource_properties = resource.get("Properties", {})

if resource_type in [SamBaseProvider.LAMBDA_LAYER, SamBaseProvider.SERVERLESS_LAYER]:
code_property_key = SamBaseProvider.CODE_PROPERTY_KEYS[resource_type]
if SamBaseProvider._is_code_uri_s3(resource_properties.get(code_property_key)):
# Content can be a dictionary of S3 Bucket/Key or a S3 URI, neither of which are supported
SamBaseProvider._warn_code_extraction(resource_type, name, code_property_key)
continue
codeuri = SamBaseProvider._extract_codeuri(resource_properties, code_property_key)

compatible_runtimes = resource_properties.get("CompatibleRuntimes")
metadata = resource.get("Metadata", None)
layers.append(
self._convert_lambda_layer_resource(stack, name, codeuri, compatible_runtimes, metadata)
)
return layers

def _convert_lambda_layer_resource(self, stack: Stack, layer_logical_id: str, layer_resource: Dict) -> LayerVersion:
def _convert_lambda_layer_resource(
self,
stack: Stack,
layer_logical_id: str,
codeuri: str,
compatible_runtimes: Optional[List[str]],
metadata: Optional[Dict],
) -> LayerVersion:
"""
Convert layer resource into {LayerVersion} object.
Parameters
----------
layer_logical_id: LogicalID of resource.
layer_resource: resource in template.
stack
layer_logical_id
LogicalID of resource.
codeuri
codeuri of the layer
compatible_runtimes
list of compatible runtimes
metadata
dictionary of layer metadata
Returns
-------
LayerVersion
The layer object
"""
# In the list of layers that is defined within a template, you can reference a LayerVersion resource.
# When running locally, we need to follow that Ref so we can extract the local path to the layer code.
layer_properties = layer_resource.get("Properties", {})
resource_type = layer_resource.get("Type")
compatible_runtimes = layer_properties.get("CompatibleRuntimes")
codeuri = None

if resource_type == self.SERVERLESS_LAYER:
codeuri = SamLayerProvider._extract_sam_function_codeuri(layer_logical_id, layer_properties, "ContentUri")
if resource_type == self.LAMBDA_LAYER:
codeuri = SamLayerProvider._extract_lambda_function_code(layer_properties, "Content")

if codeuri and not self._use_raw_codeuri:
LOG.debug("--base-dir is presented not, adjusting uri %s relative to %s", codeuri, stack.location)
LOG.debug("--base-dir is not presented, adjusting uri %s relative to %s", codeuri, stack.location)
codeuri = SamLocalStackProvider.normalize_resource_path(stack.location, codeuri)

return LayerVersion(
layer_logical_id,
codeuri,
compatible_runtimes,
layer_resource.get("Metadata", None),
metadata,
stack_path=stack.stack_path,
)