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

(pipelines): Various pipeline-related constructs not visited by aspect #18440

Closed
stekern opened this issue Jan 14, 2022 · 5 comments
Closed

(pipelines): Various pipeline-related constructs not visited by aspect #18440

stekern opened this issue Jan 14, 2022 · 5 comments
Labels
@aws-cdk/pipelines CDK Pipelines library guidance Question that needs advice or information.

Comments

@stekern
Copy link

stekern commented Jan 14, 2022

What is the problem?

I noticed this issue when trying to tag all resources that are created when using thepipelines.CodePipeline construct. Some constructs (e.g., <...>/UpdatePipeline/SelfMutate/CodePipelineActionRole/Resource) were created and available in my cloud assembly, but never visited by my aspect (and thus not tagged).

I've observed the following behavior:

  • various pipeline-related constructs are not visited by an aspect when using an existing CodePipeline to create a CDK Pipeline and not manually calling buildPipeline.
  • All pipeline-related constructs seem to be visited when not using an existing CodePipeline and/or manually calling buildPipeline.

Reproduction Steps

  1. Synthesize the application defined by the code below.
  2. Verify that constructs such as Pipeline/CodePipeline/UpdatePipeline/SelfMutate/CodePipelineActionRole/Resource are not logged to standard out (i.e., not visited by the aspect), but exist in the generated Cloud assembly cdk.out/Pipeline.template.json.
  3. Manually call buildPipeline and/or remove the use of an existing CodePipeline, synthesize the application and verify that more constructs are visited by the aspect now than previously.
#!/usr/bin/env node

import * as cdk from "@aws-cdk/core"
import * as codepipeline from "@aws-cdk/aws-codepipeline"
import * as pipelines from "@aws-cdk/pipelines"

const app = new cdk.App()

export class MyStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props)
    new cdk.CfnOutput(this, "Output", {
      value: "Hello World",
    })
  }
}

class MyStage extends cdk.Stage {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StageProps) {
    super(scope, id, props)
    new MyStack(this, "stack")
  }
}

export class PipelineStack extends cdk.Stack {
  constructor(scope: cdk.Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props)
    const codePipeline = new codepipeline.Pipeline(this, "CodePipeline")
    const cdkPipeline = new pipelines.CodePipeline(this, "CdkPipeline", {
      synth: new pipelines.ShellStep("Synth", {
        input: pipelines.CodePipelineSource.connection(
          "my-org/my-app",
          "main",
          {
            connectionArn:
              "arn:aws:codestar-connections:us-east-1:222222222222:connection/7d2469ff-514a-4e4f-9003-5ca4a43cdc41",
          },
        ),
        commands: ["npm ci", "npm run build", "npx cdk synth"],
      }),
      codePipeline: codePipeline,
    })

    cdkPipeline.addStage(new MyStage(this, "example"))
    // Note: If the following line is uncommented, all expected construct paths are logged during synthesis.
    // cdkPipeline.buildPipeline()
  }
}

new PipelineStack(app, "Pipeline")

cdk.Aspects.of(app).add({
  visit(construct: cdk.IConstruct) {
    console.log(construct.node.path)
  },
})

What did you expect to happen?

  • I would expect the aspect to visit all constructs inside the defined application.
  • I would expect the usage of an existing CodePipeline to not affect this.
  • I would expect to be able to not be required to manually call buildPipeline in order to have the aspect visit all constructs.

What actually happened?

Only a subset of the expected constructs were visited by the aspect.

CDK CLI Version

1.139.0

Framework Version

No response

Node.js Version

14.17.6

OS

MacOS

Language

Typescript

Language Version

4.5.4

Other information

No response

@stekern stekern added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jan 14, 2022
@github-actions github-actions bot added the @aws-cdk/pipelines CDK Pipelines library label Jan 14, 2022
@ryparker ryparker added p2 and removed needs-triage This issue or PR still needs to be triaged. labels Jan 14, 2022
@stekern
Copy link
Author

stekern commented Jan 18, 2022

Seeing similar behavior in CDK v2.8.0 as well.

@stekern
Copy link
Author

stekern commented Feb 1, 2022

In this specific case the consequences aren't very severe (a handful of missing tags), but it may be an indicator that there is a more general issue wrt. synthesizing and the usage of aspects.

In general I'd expect there to be a guarantee that any construct in a given scope will be visited by an aspect. If there is a bug that breaks this guarantee, it may have more severe implications for other use-cases (e.g., if you're using aspects for compliance checking, security rules, etc.).

@rix0rrr
Copy link
Contributor

rix0rrr commented Feb 8, 2022

This is unfortunately true. For example, constructs generated during aspect visiting time are not guaranteed to be visited again. It is not a bug, it's a limitation.

In the cdk lifecycle, only constructs that are added in the "construction" phase are guaranteed to be visited. By calling buildPipeline() you force the pipeline construct creation forward.

It is not safe to use Aspects as the sole defense for compliance or security checking; not just because of this behavior, but more generically because the CDK employs a general-purpose programming language where anyone could write any code to do anything (also overwrite the output of the synth directory with arbitrary contents after your validation has run).

To get any guarantees at all, you must do security analysis on the artifacts produced by a CDK application (CloudFormation templates etc), after the user code is definitely done running.

You can use Aspects for early warning/convenience, not for security.

@rix0rrr rix0rrr closed this as completed Feb 8, 2022
@rix0rrr rix0rrr added guidance Question that needs advice or information. and removed bug This issue is a bug. p2 labels Feb 8, 2022
@rix0rrr rix0rrr removed their assignment Feb 8, 2022
@github-actions
Copy link

github-actions bot commented Feb 8, 2022

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

@stekern
Copy link
Author

stekern commented Feb 8, 2022

@rix0rrr I fully agree that one should not use Aspects as a single line of defense for security and compliance checking. I brought it up as a more "severe" example compared to the scenario of missing a handful of tags (and also because I've been looking at https://github.com/cdklabs/cdk-nag lately which uses Aspects specifically for compliance and security checks).

I do, however, think it could be helpful to highlight this behavior/limitation in the official pipelines documentation, or in the documentation of Aspects itself, especially if there are other official APIs that work similar to buildPipeline.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/pipelines CDK Pipelines library guidance Question that needs advice or information.
Projects
None yet
Development

No branches or pull requests

3 participants