fix: duplicate CDK resource ID

[WinterYukky]

Which issue(s) does this change fix?

#4034 https://github.com/aws/aws-cdk/issues/21134

Why is this change necessary?

Currently, SAM CLI extract SamResourceId from last second string of aws:cdk:path value. But, CDK possibly generate aws:cdk:path like next.

• Stack/aaa/Fn/Resource -> SamResouceId: Fn
• Stack/bbb/Fn/Resource -> SamResouceId: Fn
• Stack/ccc/Fn/Resource -> SamResouceId: Fn

In such cases, the SAM CLI considers these resources to have the same SamResouceId (i.e., Fn) and treats only the last lambda function found as an executable lambda function. In other words, it will generate lambda functions that are not found.

How does it address the issue?

See https://github.com/aws/aws-sam-cli/pull/4064#issuecomment-1187724150

What side effects does this change have?

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• [x] Add input/output type hints to new functions/methods
• [ ] Write design document if needed (Do I need to write a design document?)
• [x] Write/update unit tests
• [x] Write/update integration tests
• [ ] Write/update functional tests if needed
• [x] 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.

[jfuss]

Left a comment in https://github.com/aws/aws-sam-cli/issues/4034 but putting the important details here too for visibility with the team.

We need to understand if this is the right approach. Combining the two fields seems reasonable but I need to do a deeper check in what we support now and why before I can understand the right approach (allowing the logical id and the cdk path, using "_" as a delimiter, etc).

Again thanks to @WinterYukky for catching this and @sinokuma for the initial report.

[WinterYukky]

Thanks @jfuss for checking the PR. I was wondering why you use part of the cdk path (i.e Fn) for the ID value, but your explanation helped me understand. This PR works by using underscores as delimiters, but may not be a good solution for users. It is better to use slashes as delimiters than to use underscores, at least.

Currently, SAM CLI find functions by function_id name functionname. https://github.com/aws/aws-sam-cli/blob/develop/samcli/lib/providers/sam_function_provider.py#L110-L112

Consider the following example.

{
  "Resources": {
    "aaaFn67B877B1": {
      "Type": "AWS::Lambda::Function",
      "Properties": {},
      "Metadata": {
        "aws:cdk:path": "Stack/aaa/Fn/Resource",
        "aws:asset:path": "/workspaces/cdk-sam-local/lib/handler",
        "aws:asset:is-bundled": false,
        "aws:asset:property": "Code"
      }
    },
    "bbbFn9F2D7876": {
      "Type": "AWS::Lambda::Function",
      "Properties": {},
      "Metadata": {
        "aws:cdk:path": "Stack/bbb/Fn/Resource",
        "aws:asset:path": "/workspaces/cdk-sam-local/lib/handler",
        "aws:asset:is-bundled": false,
        "aws:asset:property": "Code"
      }
    },
    "cccFn256E40E6": {
      "Type": "AWS::Lambda::Function",
      "Properties": {},
      "Metadata": {
        "aws:cdk:path": "Stack/ccc/Fn/Resource",
        "aws:asset:path": "/workspaces/cdk-sam-local/lib/handler",
        "aws:asset:is-bundled": false,
        "aws:asset:property": "Code"
      }
    },
    "CDKMetadata": {
      "Type": "AWS::CDK::Metadata",
      "Properties": {
        "Analytics": "v2:deflate64:H4sIAAAAAAAA/zWNXQqDMBCEz+J73FqFvtdCD2APIGvcyvqTgJu0DyF3b6IUFr7ZYZipoamhKvArpR6XcuUBwsuhXlSy+rDiNowI4emNdmyNerzNX0fFuEHo7ErZzoxKmh5FyAncM9IPofV6IdeikDpljp8qRnXk0uTEZjp6SKzfdeoydiSY5fK53iBdVczCXO7eON4IupM/SkZ1q8EAAAA="
      },
      "Metadata": {
        "aws:cdk:path": "Stack/CDKMetadata/Default"
      }
    }
  }
}

In this case, the SAM CLI resolves the functions into the following objects.

LogicalId	function_id(SamResourceId)	name	functionname
aaaFn67B877B1	Fn	aaaFn67B877B1	aaaFn67B877B1
bbbFn9F2D7876	Fn	bbbFn9F2D7876	bbbFn9F2D7876
cccFn256E40E6	Fn	cccFn256E40E6	cccFn256E40E6

How about changing it as follows.

LogicalId	function_id(SamResourceId)	name	functionname
aaaFn67B877B1	aaaFn67B877B1	Fn	aaaFn67B877B1
bbbFn9F2D7876	bbbFn9F2D7876	Fn	bbbFn9F2D7876
cccFn256E40E6	cccFn256E40E6	Fn	cccFn256E40E6

When change to the proposed solution, then I think SAM CLI allows use these lambda functions. When call by aaaFn67B877B1 (i.e LogicalId) then invoke the found function and when call by Fn then print warning log "Multiple functions found with keyword {name}! Function ... ".

[jfuss]

@WinterYukky I chatted with the team. Instead of concatenating parts of the cdk:path, we want to ensure that the LogicalId is callable. Concatenating, still requires the customer to understand how the cdk:path works and map it to their code. Instead, we feel that supporting the LogicalId should always work. We know this does not solve the how we handle the cdk:path but want to start from the LogicalId as a first step.

Would you be willing to update this PR to support this?

[WinterYukky]

@jfuss Of course! I'll try it.

[jfuss]

@WinterYukky I should be able to get you a review this week. Sorry for the delay.

[WinterYukky]

I left a couple comments but think I need to do a fresh pass when my brain is fresh. Just having a hard time following how name and layer_id fit into everything. Ideally, we could follow the pattern we have for functions but maybe there is a reason they need to be different that I don't quite get yet.

I at least wanted to get some feedback out to you now.

Thanks for reviewing @jfuss! I recheck the my change and will reply.

[moelasmar]

Thanks @WinterYukky for your effort. I have some concern about the solution you suggested to fix this problem. My concern is it could not be extend to other IaC frameworks. The full_name property you added to the lambda functions, and layers seems is only applicable for CDK use case, and this breaks our current implementation to support other IaC frameworks like Terraform.

As per Jacob's comment in the opened issue. The solution we are looking for is to enable the customer to use logical id for search, and not to fix the CDK id extraction.So, I would suggest to define two ids for Functions, Layers, and Stacks (Logical Id, and Custom Id), and to have full_logical_id which will be <parent stack full_logical_id>/<resource logical id> and full_custom_id which will be <parent stack full_custom_id>/<resource custom id>. So, the customer can use either the a path of logical ids or a path of custom ids to define a resource, as I do not think that it is a valid use case for the customer to mix between the 2 id types to define a resource.

I also suggest to keep ResourceMetadataNormalizer.get_resource_id as it is, and remove ResourceMetadataNormalizer.get_resource_name. As, the custom logic to determine the resource custom id (CDK id) is only applicable for CDK use case, and this id is always equivalent to the custom id. The best solution here is to add a change to CDK so during synthesize step, it adds the SamResourceId metadata, and set its value to the resource CDK Id.

[WinterYukky]

Thanks for reviewing @moelasmar! Your propose that using new properties custom_id and full_custom_id looks like nice. I also understand your thoughts on other IaC frameworks.

However, I don't agree to keep ResourceMetadataNormalizer.get_resource_id as it is. Because CDK ID (which SAM expects) can be duplicates. The solution can't fix this bug.

So I thought I should use the IaCPluginInterface to implement per-IaC behavior, but I don't know if that is appropriate. Can you help me?

[moelasmar]

Sorry @WinterYukky for my slow reply. I want to leave the ResourceMetadataNormalizer.get_resource_id for now as in my opinion we should fix it from the CDK side, we need to push a PR in the CDK repo to add a new Metadata property to get the resource CDK id. Any fix we are going to do in SAM CLI is a workaround in my opinion, that is why I am thinking to leave it as is right now, till we can do the CDK change.

For this PR, we can change its purpose to enable the customers to use the Logical Ids to invoke a lambda function in case if there is any issue with using the CDK ID, so the customers can unblock themselves.

[mndeveci]

Thanks for your contribution @WinterYukky ! I will be converting this PR into draft until you will have some time to address comments from second part of @moelasmar's suggestion.

[WinterYukky]

@mndeveci I'm sorry. I'm busy now but working this issue slowly. I'm never abandonment. When I'm ready let you know.

[jfuss]

Closing as this appears stale. If you are still interesting in contribution this, feel free to reopen or create a new PR.