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

feat(codepipeline): use a special bootstrapless synthesizer for cross-region support Stacks #8091

Merged

Conversation

skinny85
Copy link
Contributor

Fixes #8082


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

@skinny85 skinny85 requested a review from rix0rrr May 20, 2020 00:20
@skinny85 skinny85 self-assigned this May 20, 2020
@skinny85 skinny85 marked this pull request as draft May 20, 2020 00:20
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label May 20, 2020
@skinny85
Copy link
Contributor Author

Hey Rico,

this is a draft of the solution to #8082 . I'm putting it as a draft, as I have 2 remaining questions:

  1. Does this approach make sense? I'm not sure this example asset declaration (for the template upload location):
{
  "version": "0.0.0",
  "files": {
    "4a751a2697187d165e0f7e6be2e8b8ae26fd67588a03cc3d88b673618201c552": {
      "source": {
        "path": "CrossRegionAndAccountCfnPipelineStack-support-eu-central-1.template.json",
        "packaging": "file"
      },
      "destinations": {
        "828671620168-eu-central-1": {
          "bucketName": "cdk-hnb659fds-assets-828671620168-us-west-2",
          "objectKey": "4a751a2697187d165e0f7e6be2e8b8ae26fd67588a03cc3d88b673618201c552",
          "region": "eu-central-1",
          "assumeRoleArn": "arn:${AWS::Partition}:iam::828671620168:role/cdk-hnb659fds-publishing-role-828671620168-us-west-2"
        }
      }
    }
  },
  "dockerImages": {}
}

is correct (the bucket is cdk-hnb659fds-assets-828671620168-us-west-2, obviously in us-west-2, but the region is eu-central-1, so not sure this works...)

  1. Do we have any existing tests that are checking the contents of the generated Cloud Assembly, that I could base the unit tests for this one? Not sure how to test this...

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 50d903f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rix0rrr
Copy link
Contributor

rix0rrr commented May 20, 2020

In fact I think a custom subclass might make more sense. For example, this stack needs to be deployable without a bucket in the target location (as you already noted), so it definitely can't support assets.

So I'm thinking of a subclass that'll throw when addFileAsset or addDockerAsset are called.

I did write that in the original ticket, though only in passing:

We need a new Synthesizer which [...] won't support assets...

Not supporting assets also means that is should also not "upload its own template" the way the default synthesizer does. It can be like the legacy synthesizer in that way, and basically not do anything special there -- the CLI will ultimately deploy the template.

Another restriction we could encode into the synthesizer is, since we know it needs to be deployable without a bootstrap environment, the template itself needs to be <40k (or whatever the size limit is again), so that it goes into one CFN call.

Maybe the specific subclass needs to be a BootstraplessSynthesizer, that could live in core?

@rix0rrr
Copy link
Contributor

rix0rrr commented May 20, 2020

Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

See el above-o

@skinny85
Copy link
Contributor Author

skinny85 commented May 20, 2020

That's exactly why I wanted to post this quickly as a draft, this is great Rico!

I'm not sure about this part though:

Maybe the specific subclass needs to be a BootstraplessSynthesizer, that could live in core?

I wonder why you want to put it in core - why not in the codepipeline package?

@skinny85 skinny85 added the pr-linter/exempt-readme The PR linter will not require README changes label May 20, 2020
@skinny85 skinny85 force-pushed the feat/cross-region-codepipeline-replication-buckets branch from 50d903f to 4eb2637 Compare May 20, 2020 20:40
@skinny85
Copy link
Contributor Author

@rix0rrr submitted a new version. The result that I now see (with my manual test) is:

    "CrossRegionAndAccountCfnPipelineStack-support-eu-central-1": {
      "type": "aws:cloudformation:stack",
      "environment": "aws://828671620168/eu-central-1",
      "properties": {
        "templateFile": "CrossRegionAndAccountCfnPipelineStack-support-eu-central-1.template.json",
        "assumeRoleArn": "arn:${AWS::Partition}:iam::828671620168:role/cdk-hnb659fds-deploy-role-828671620168-us-west-2",
        "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::828671620168:role/cdk-hnb659fds-cfn-exec-role-828671620168-us-west-2",
        "requiresBootstrapStackVersion": 1
      },
    },

And there is no CrossRegionAndAccountCfnPipelineStack-support-eu-central-1.assets anymore.

Let me know if this version makes sense, and if "yes", I'll add unit tests for it.

@skinny85 skinny85 requested a review from rix0rrr May 20, 2020 20:47
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 4eb2637
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@skinny85 skinny85 changed the title feat(codepipeline): have the same synthesizer for cross-region support Stacks feat(codepipeline): use a special bootstrapless synthesizer for cross-region support Stacks May 20, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented May 21, 2020

Let me know if this version makes sense, and if "yes", I'll add unit tests for it.

Yep, I believe that looks good.

public synthesizeStackArtifacts(session: ISynthesisSession): void {
assertBound(this.stack);

// do _not_ treat the template as an asset,
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably worth doing a size check on the template here as well.

Copy link
Contributor Author

@skinny85 skinny85 May 21, 2020

Choose a reason for hiding this comment

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

Can we do it as a quick follow-up to this issue? <bats eyelashes>

@@ -71,6 +71,8 @@ export interface CrossRegionSupportStackProps {
* @example '012345678901'
*/
readonly account: string;

readonly synthesizer: cdk.IStackSynthesizer | undefined;
Copy link
Contributor

Choose a reason for hiding this comment

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

Using a ? is the more usual way we express that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I actually wanted to make the synthesizer a required property.

I don't think it matters much, this is all private to the codepipeline module anyway.

@skinny85 skinny85 force-pushed the feat/cross-region-codepipeline-replication-buckets branch from 4eb2637 to 0c3c530 Compare May 21, 2020 21:46
@skinny85 skinny85 marked this pull request as ready for review May 21, 2020 21:49
@skinny85 skinny85 requested a review from rix0rrr May 21, 2020 21:49
@skinny85
Copy link
Contributor Author

@rix0rrr included your comments, and added a unit test. This is now ready to graduate from a draft PR.

Let me know if you have any more comments!

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0c3c530
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

*/
export interface BootstraplessSynthesizerProps {
/** The deploy Role ARN to use. */
readonly deployRoleArn: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think make these optional and you've got yourself an approved!

@rix0rrr rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label May 25, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 7040340
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rix0rrr rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label May 25, 2020
@mergify
Copy link
Contributor

mergify bot commented May 25, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 2a2834b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented May 25, 2020

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit 575f1db into aws:master May 25, 2020
@skinny85 skinny85 deleted the feat/cross-region-codepipeline-replication-buckets branch May 27, 2020 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cross-region support stacks need bootstrappin'
3 participants