cloudcomponents / cdk-constructs

A collection of higher-level reusable cdk constructs
MIT License
625 stars 104 forks source link

Ensure unique resources ID are created #118

Open fasatrix opened 3 years ago

fasatrix commented 3 years ago

Context: While deploying a multi pipeline with a single stack that will independently deploy FE and BE ECS services (under the same ALB) we noted the following issue.

Issues: Resources IDs aren't unique while being created as part of a single CDK stack.

image

image

Architectural Diagram of the target state image

fasatrix commented 3 years ago

@hupe1980 any chances for this to be reviewed? Thanks

fasatrix commented 3 years ago

@hupe1980 any change for this to be looked at? It should be quick as doesn't change anything major. Thanks

cao2504 commented 3 years ago

@hupe1980 Hi, I also come across this issue if I try to deploy multiple services in one stack, is there any change for you to look at this PR

hupe1980 commented 2 years ago

Actually, the cdk framework already attaches an id to the logical name. Even the stdlib does not take any further steps. See: https://github.com/aws/aws-cdk/blob/ae840ff1abb8283a1290dae5859f5729a9cf72b1/packages/%40aws-cdk/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts#L160

How is your stack structured exactly?

fasatrix commented 2 years ago

Actually, the cdk framework already attaches an id to the logical name. Even the stdlib does not take any further steps. See: https://github.com/aws/aws-cdk/blob/ae840ff1abb8283a1290dae5859f5729a9cf72b1/packages/%40aws-cdk/aws-ecs-patterns/lib/fargate/application-load-balanced-fargate-service.ts#L160

How is your stack structured exactly?

Of course it does , but if in the same stack you call that CDK construct twice (e.g. you deploy two services at once, for instance fe and be), CDK will complain as that resource exists a ready.

We have a stack like this

new CDPipelineStack(app, 'StackName', {
    certificateArn: values.certificateArn,
    url: `som- URL`,
    pipelines: [
      {
        cpu: 2048,
        memory: 4096,
        imageRepoName: 'image-be',
        imageRepoTag: values.tag,
        nextImageRepoTag: values.nextTag,
        path: '/api/*',
        containerEnvironment: [
          { name: 'ENVIRONMENT_NAME', value: values.environmentName },
          { name: 'FRONTEND_HOST', value: 'some-host-name' },
        ],
        containerSecrets: [
          { name: 'OAUTH_GITHUB_CLIENT_ID', valueFrom: `/service-catalogue/${values.parameterStoreKey}/OAUTH_GITHUB_CLIENT_ID` },
          { name: 'OAUTH_GITHUB_CLIENT_SECRET', valueFrom: `/service-catalogue/${values.parameterStoreKey}/OAUTH_GITHUB_CLIENT_SECRET` },
          { name: 'GIT_SVC_ACCOUNT_TOKEN', valueFrom: `/${values.parameterStoreKey}/GIT_SVC_ACCOUNT_TOKEN` },
        ],
        numberOfTasks: 2,
        // Due to session state being application server local, we need stickiness on the ALB
        sessionStickinessDuration: Duration.days(7),
      },
      {
        url: 'someurl',
        imageRepoName: 'image-fe,
        imageRepoTag: values.tag,
        nextImageRepoTag: values.nextTag,
        containerEnvironment: [
          { name: 'ENVIRONMENT_NAME', value: values.environmentName },
          { name: 'FE_HOST', value: `somehost` },
        ]
      }
    ],
  });

The construct then will loop through the above array of pipelines and creates independent codePipleine/CodeDeploy

fasatrix commented 2 years ago

@hupe1980 as said in my PR we have already forked this and made the changes ourselves as per this PR and it works, problem is we don't w3ant to maintain the fork each time you change this project hence why we are requiring the change to be approved. I can walk you through our use case (which I believe it is quite unique) if you can let me know abut your availability in NZT (we can zeoom call)

hupe1980 commented 2 years ago

Do you set always new IDs during the loop? About as: https://github.com/cloudcomponents/cdk-constructs/blob/0ff07cbce1fe25b5ea6d57ce08553f4b1fff90c8/packages/cdk-static-website/src/website-alias-record.ts#L47

fasatrix commented 2 years ago

Do you set always new IDs during the loop? About as:

https://github.com/cloudcomponents/cdk-constructs/blob/0ff07cbce1fe25b5ea6d57ce08553f4b1fff90c8/packages/cdk-static-website/src/website-alias-record.ts#L47

HI @hupe1980 we definitely do when we create our own resources however as you can see from the above error stack the problem is with a resource we have no control of Custom:EcsDeploymentGroup as it is part of your lib. This is the one that fails because of a not unique name while called in parallel as part of multiple pipelines within the same stack, and hence why we asking for this PR. As you can see from the PR it is not the only one the affected recourse, and you could use the same strategy as in the example you indicate above to make it unique by using a has instead of passing the id as I do (this is unique though as it is carried over from the constract that instantiate the resource). Please try to create a deployment similar to the one I have explained above (two pipelines in a common stack, classic front and backend architecture which will create two separate CodePipeline and CodeDeploy) and hopefully you will witness it too.

Look at the following example from my PR, if I try to create those resources as part of the same stack twice (two different pipelines) AWS will complain as I have already one resource with that Name. You can use hash if you like instead of id I do not mind as long as it is unique and fixes the problem.

image

I am still keen to show while it is happening if you indicate a time suitable to attend a zoom?

hupe1980 commented 2 years ago

Ah okay. I understand it :

The problem is the singleton 'getOrCreate': https://github.com/cloudcomponents/cdk-constructs/blob/0ff07cbce1fe25b5ea6d57ce08553f4b1fff90c8/packages/cdk-blue-green-container-deployment/src/ecs-deployment-group.ts#L108

We have to replace the singleton with a normal construct

fasatrix commented 2 years ago

Ah okay. I understand it :

The problem is the singleton 'getOrCreate':

https://github.com/cloudcomponents/cdk-constructs/blob/0ff07cbce1fe25b5ea6d57ce08553f4b1fff90c8/packages/cdk-blue-green-container-deployment/src/ecs-deployment-group.ts#L108

We have to replace the singleton with a normal construct

Nice thx