feat: support for cross-account CodePipelines

[skinny85]

This change adds support for expressing CodePipelines that are cross-account in the CDK.

There is a lot of things happening in this PR, as in order to support this functionality, changes were needed at the framework level. A summary:

1. CrossEnvironmentToken and ResourceIdentifiers

I've added a new Token, CrossEnvironmentToken, that allows references across accounts and/or regions. I've also added a helper class, ResourceIdentifiers, that encapsulates using the new Token. ResourceIdentifiers are mean to be used inside our L2s. Both of these live in @aws-cdk/cdk.

2. Added the concept of a physical name

I've created a new class, PhysicalName, that encompasses our strategies for assigning physical names. I've added the option to pass a physical name when creating S3 Buckets, IAM Roles and CodeBuild Projects (if the API is deemed acceptable, we would need to make changes to other L2s as well of course).

The "old" ways of passing a physical name (bucketName, roleName, etc.) have been kept for now, but we might want to consider removing them in favor of physicalName.

3. Added facilities for automatically generating physical names

Added a new class, PhysicalNameGenerator, that creates physical names at synthesis time. Added it to the Resource class, so that particular resources can override the strategy (used for S3 Buckets, for example). As both IResource and Resource were previously empty, this uncovered a lot of incorrect inheritance hierarchies in our Constructs, where an interface extended IResource, but the implementation class extended Construct, which now failed when IResource has members - fixed those as well.

4. Changed some implicit sub resources to the new physical naming scheme

To facilitate cross-environment usage, the Bucket for a CodePipeline and a Role for CodeBuild Project are now created with PhysicalName.deployTimeOrAssigned().

5. Changes in the CodePipeline Construct

The CodePipeline Construct detects if a resource is from a different account, and automatically generates a bootstrap Role with the correct permissions if it is.

---

All in all, the changes allow for the following cross-environment customer experience with CodePipeline:

export class CrossAccountPipelineForCodeBuildGood extends cdk.Stack {
    constructor(app: cdk.App, mainAccount: string, buildAccount: string) {
        super(app, 'CrossAccountPipelineForCodeBuildGood', {
            env: {
                account: mainAccount,
            },
        });

        /*************** the CodeBuild Project Stack *************************/

        const buildStack = new cdk.Stack(app, 'CrossAccountPipelineForCodeBuildGoodBuildStack', {
            env: {
                account: buildAccount,
            },
        });

        const project = new codebuild.PipelineProject(buildStack, 'Project', {
            projectName: 'CrossAccountPipelineForCodeBuildGoodProject',
            environment: {
                buildImage: codebuild.LinuxBuildImage.UBUNTU_14_04_OPEN_JDK_8,
            },
        });

        /*************** the Pipeline Stack *************************/

        const repository = codecommit.Repository.import(this, 'Repository', {
            repositoryName: 'ManualTestRepo',
        });
        const sourceOutput = new codepipeline.Artifact();
        new codepipeline.Pipeline(this, 'CrossAccountPipelineForCodeBuildGood', {
            pipelineName: 'CrossAccountPipelineForCodeBuildGood',
            stages: [
                {
                    name: 'Source',
                    actions: [
                        new cpactions.CodeCommitSourceAction({
                            actionName: 'CodeCommit',
                            repository,
                            output: sourceOutput,
                        }),
                    ],
                },
                {
                    name: 'Build',
                    actions: [
                        new cpactions.CodeBuildAction({
                            actionName: 'CodeBuild',
                            project,
                            input: sourceOutput,
                            output: new codepipeline.Artifact(),
                        }),
                    ],
                },
            ],
        });
    }
}

which is exactly the same as creating a non-cross account Pipeline.

---

Pull Request Checklist

• Testing
  • Unit test added (prefer not to modify an existing test, otherwise, it's probably a breaking change)
  • CLI change?: coordinate update of integration tests with team
  • cdk-init template change?: coordinated update of integration tests with team
• Docs
  • jsdocs: All public APIs documented
  • README: README and/or documentation topic updated
• Title and Description
  • Change type: title prefixed with fix, feat will appear in changelog
  • Title: use lower-case and doesn't end with a period
  • Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
  • Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
• Sensitive Modules (requires 2 PR approvers)
  • IAM Policy Document (in @aws-cdk/aws-iam)
  • EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
  • Grant APIs (only if not based on official documentation with a reference)

---

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

[eladb]

A few comments following an initial pass.

[rix0rrr]

I don't understand why this has to be a different class from CrossAccountRegionPhysicalArnToken. Is the difference that the second one knows that obj.physicalName exists, but the first one doesn't? Because that doesn't seem strong enough of a reason to make two classes to me, especially given that the second one seems like it could be written as:

new cdk.CrossEnvironmentIdentifier(resource.projectName, () => this.physicalName).toString();

[skinny85]

Updated the PR with the proposed vision. This is still an early version - I'm very keen on the feedback for the names I chose, as well as the API for Construct Library authors.

@eladb and @rix0rrr , I would appreciate a re-review. Thanks!

[eladb]

Initial partial pass. Let's do another iteration and I'll continue the review.

[skinny85]

Thanks for the review @eladb !

One question: how do you see the migration to the 'new' way of specifying a physical name (new s3.Bucket(this, 'B', { physicalName: PhysicalName.of('my-bucket') });? Should we deprecate the 'old' attributes like bucketName, roleName, etc., but still handle them in the L2s, like I did here, or should we "rip the bandaid" and remove them? That might be quite a gigantic change...

Also, should we do it for all L2s at the same time, or should we migrate gradually, module-by-module?

[eladb]

Something I forgot to comment about: I think the physical name prop should be “bucketName” and not “physicalName”. The type denotes that it’s a physical name so there is no additional benefit in adding another prop.

Which I believe answers your question: this is our last chance to “rip the bandaid”, and we should do it across the entire library through an awslint rule.

[eladb]

(But I would still do it in a subsequent PR)

[skinny85]

I'm actually not a huge fan of the bucketName convention. I feel like it only came about because we used to call the logical ID name, and we wanted to distinguish clearly between the two. Now that we switched to calling logical IDs id, I feel like repeating the name of the class in the property is not great.

"Physical names" is a concept in our domain, it's present in the CDK docs, etc. I feel like calling the property that way has value (I think a name is much more important than the type, especially in dynamically-typed languages) - it reinforces that the concepts and code use a shared terminology.

Just my two cents. If you feel strongly that it should remain bucketName, I'll live with it, just wanted to lay out my reasoning.

EDIT: it also make it trivial to answer the question "Which property is the physical name?" for every resource (and a related one, which is: what resources don't have a physical name?).

[skinny85]

Implemented some (not all) of Elad's comments.

[skinny85]

Should this also be this.node.uniqueId instead of just this.node.id?

[eladb]

probably.

[skinny85]

Hmmm... on second thought, these are per-resource Stack unique, and the Pipeline ID makes it per-Pipeline unique... maybe leave it as this.node.id?

[eladb]

What if there are two pipelines with the same id?

[skinny85]

The latest batch of changes should be close to merge ready, the significant part that is missing are the name changes of the static factory methods of PhysicalName.

[eladb]

why are these called "simple"?

[skinny85]

Those are the non-cross environments variants (so, those that result in Ref and/or Fn::GetAtt).

I'm open to having a better name for them.

[eladb]

I think just arn and name are sufficient. These are the only ARNs and names in this Props...

[eladb]

I do have a feeling that since there will be cases where users won't have control over how underlying resources are configured, and those will be configured without cross: true, then they would want to be able to override the default "auto" behavior. That's why I am proposing to merge deployTime and deployTimeOrAssigned into a single method with config. This will enable us to provide facilities for users to basically modify the default behavior of some scope:

PhysicalName.allowCrossEnvWithinThisScope(this); // this is "Construct"

new Bucket(this, 'MyBucket'); // implies `deploy-time-or-assigned`.

[eladb]

I'm actually not a huge fan of the bucketName convention. I feel like it only came about because we used to call the logical ID name, and we wanted to distinguish clearly between the two. Now that we switched to calling logical IDs id, I feel like repeating the name of the class in the property is not great.
"Physical names" is a concept in our domain, it's present in the CDK docs, etc. I feel like calling the property that way has value (I think a name is much more important than the type, especially in dynamically-typed languages) - it reinforces that the concepts and code use a shared terminology.
Just my two cents. If you feel strongly that it should remain bucketName, I'll live with it, just wanted to lay out my reasoning.
EDIT: it also make it trivial to answer the question "Which property is the physical name?" for every resource (and a related one, which is: what resources don't have a physical name?).

AWS resources are messy. Their intrinsic physical name has different names for different resources (usually either "name" or "ID"). I want to make sure that the prop that assigns this name is the sameas the attribute property that can be used to retrieve it, and there are sometimes many attributes (could be bucketName, bucketArn, securityGroupId (boom "id"), etc.

I feel strongly that we should use consistent terminology between the prop and the attribute and we've settled on a pretty strict convention for attribute names (always start with the type name, etc), so I think we should stay with bucketName as the physical name.

As for discovery: we now have your beautiful nice type PhysicalName, so it's pretty easy to identify which prop is the physical name of the resource. Moreover, we are going to include this information in the AWS model and our documentation will call this out explicitly thanks to the type.

[rix0rrr]

Sorry for derailing the discussion further, but I just had a stroke of insight leading me to no longer agree to do fully automatic auto-naming. I think it's too risky. I forgot whether we already discussed this, and rejected the concerns?

Good news is, it would get rid of the token magic, simplifying the whole thing a lot.

[rix0rrr]

FWIW, Aspects could totally cover the use case you're thinking of there, Elad.