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.

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

Ensure unique resources ID are created #118

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fasatrix
Copy link

@fasatrix fasatrix commented Sep 8, 2021

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

While deploying a multi pipeline that will independently deploy FE and BE ECS services under the same ALB
This reverts commit cac1627.
@fasatrix
Copy link
Author

@hupe1980 any chances for this to be reviewed? Thanks

@fasatrix
Copy link
Author

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

@cao2504
Copy link

cao2504 commented Oct 6, 2021

@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
Copy link
Member

hupe1980 commented Oct 9, 2021

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
Copy link
Author

fasatrix commented Oct 12, 2021

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
Copy link
Author

@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
Copy link
Member

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

recordNames.forEach((recordName): void => {

@fasatrix
Copy link
Author

fasatrix commented Oct 14, 2021

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

recordNames.forEach((recordName): void => {

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
Copy link
Member

Ah okay. I understand it :

The problem is the singleton 'getOrCreate':

const serviceToken = CustomResourceProvider.getOrCreate(this, 'Custom::EcsDeploymentGroup', {

We have to replace the singleton with a normal construct

@fasatrix
Copy link
Author

Ah okay. I understand it :

The problem is the singleton 'getOrCreate':

const serviceToken = CustomResourceProvider.getOrCreate(this, 'Custom::EcsDeploymentGroup', {

We have to replace the singleton with a normal construct

Nice thx

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants