(aws_servicecatalog): `cdk synth` already create a new logicID for resource [AWS::ServiceCatalog::PortfolioPrincipalAssociation]

UlaJiang commented 1 year ago

Describe the bug

cdk synth already create a new logicID for resource AWS::ServiceCatalog::PortfolioPrincipalAssociation if using value_for_string_parameter() and from_portfolio_arn() method.

Expected Behavior

cdk synth should generate the same template (same logicID of resources)

Current Behavior

cdk synth generatedifferent logicID of resource [AWS::ServiceCatalog::PortfolioPrincipalAssociation]

Reproduction Steps

1. Use case:

we have an existing IAM Role, and Service Catalog Portfolio in AWS account.
We store the Service Catalog Portfolio ARN into the SSM Parameter Store,
```
parameter name = PortfolioARN
parameter value = ARN of SC profile
```
we use value_for_string_parameter to get the parameter value () from SSM Parameter Store
we use from_role_name to import the IAM role to our CDK project

we use from_portfolio_arn to import the SC profile to our CDK project

2. Code Replication:


from constructs import Construct
from aws_cdk import (
Duration,
Stack,
aws_servicecatalog as sc,
aws_s3 as s3,
aws_iam as iam,
aws_ssm as ssm,

) import aws_cdk as cdk

class CdkWorkshopStack(Stack):

def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
    super().__init__(scope, construct_id, **kwargs)

    s3CopyRole = iam.Role.from_role_name(self, "s3CopyRole", "testS3bucketempty-role-caldj1zo")
    myPortfolioArn = ssm.StringParameter.value_for_string_parameter(self,"PortfolioARN");
    myPortfolio = sc.Portfolio.from_portfolio_arn(self,"test",myPortfolioArn)
    myPortfolio.give_access_to_role(s3CopyRole);

-------

- when we run `cdk synth` several times, we can confirm the generated CFN template will already create a new new LogicalID of `AWS::ServiceCatalog::PortfolioPrincipalAssociation`. this will cause the error:

port-xxxxxx|arn:aws:iam:::role/ already exists in stack



- If we hardcode the `myPortfolioArn`, and re-run the `cdk synth`, LogicalID of `AWS::ServiceCatalog::PortfolioPrincipalAssociation` will keep the same.

### Possible Solution

CfnPortfolioPrincipalAssociation L1 construct works all good. 

### Additional Information/Context

N/A

### CDK CLI Version

$ cdk --version 2.59.0 (build b24095d)

### Framework Version

_No response_

### Node.js Version

$ npm --v 8.15.0

### OS

Cloud9 Linux

### Language

Python

### Language Version

Python (3.7)

### Other information

N/A

peterwoodworth commented 1 year ago

I can confirm this behavior, thanks for reporting.

This is happening because a unique hash is being generated and attached to the logical id: https://github.com/aws/aws-cdk/blob/c164a49c67def802a84f37b6ad779a78442e5a53/packages/%40aws-cdk/aws-servicecatalog/lib/portfolio.ts#L236-L246

I believe the root cause of this issue is that using a string parameter will make it so that the portfolio arn value is a token, which the token itself can have varying values between different synths (e.g. I printed ${Token[TOKEN.578]}, ${Token[TOKEN.580]} between different synths without changing the code at all). This differing value is taken into account when the hash is created, causing the logical ID to change.

You can work around this by using an escape hatch to override the logical ID, or as you said, dropping down to the L1 resource works as well.

I am marking this issue as p2, which means that we are unable to work on this immediately.

We use +1s to help prioritize our work, and are happy to revaluate this issue based on community feedback. You can reach out to the cdk.dev community on Slack to solicit support for reprioritization.

jeffb4 commented 1 year ago

@peterwoodworth my team is hitting this issue and I would like to pursue resolving this. My guess is that the correct way to fix the problem is to use a Lazy string for the tag option identifier around https://github.com/aws/aws-cdk/blob/main/packages/aws-cdk-lib/aws-servicecatalog/lib/tag-options.ts#L55C1-L60C12 , so that the sha hashing can be done correctly at synth time. Does that approach sound reasonable for a start, to you?

aws / aws-cdk