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

Attempt local asset bundling before using Docker. #320

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

dfkunstler
Copy link
Contributor

Issue #, if available: N/A

Description of changes:
Implemented local asset bundling, which will be used instead of Docker when possible. Local bundling is preferred over DIND for performance and security reasons. This is the same approach already used to bundle node.js dependencies in the UI construct.

Problem: Docker-in-Docker execution is often blocked in regulated environments. CDK uses Docker for S3 asset bundling by default. cdk synth fails if Docker is not available, making it impossible to build or deploy the solution.

Solution: This implementation of local bundling should succeed if zip and python3.11 (with pip) are available on the local path. If not, CDK falls back to Docker, so worst case we're back to the stated problem. Note that the required version of python is determined by the Lambda runtime specified in lib/shared/index.ts. Hard-coding python3 instead would probably make this work for more people, but matching the Lambda runtime version seemed a safer choice. I'll leave it to reviewers to make the final call.

Due to limits in CDK (and perhaps my knowledge of Javascript), I had to add similar code to two different classes. Currently, the CDK ILocalBundling.tryBundle(...) interface definition does not include the path to be bundled. An inline anonymous class implementation is the only solution I could get to work.

I've tested this on multiple versions of Amazon Linux and on Mac OS, but nowhere else.

Bundling discussion in CDK docs:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Copy link
Collaborator

@massi-ang massi-ang left a comment

Choose a reason for hiding this comment

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

I think this is good change. Please see the few comments I have regarding naming and more importantly on using a singleton implementation to avoid trying to rebuild the container at every use of the SharedAssetBundler.

I would remove local bundling for the Lambda layers since it will create issues with dependencies that are platform specific, such as many ML libraries.

lib/shared/shared-asset-bundler.ts Outdated Show resolved Hide resolved
lib/shared/shared-asset-bundler.ts Outdated Show resolved Hide resolved
lib/shared/shared-asset-bundler.ts Outdated Show resolved Hide resolved
lib/layer/index.ts Outdated Show resolved Hide resolved
dfkunstler and others added 6 commits January 22, 2024 11:15
Copy link
Collaborator

@massi-ang massi-ang left a comment

Choose a reason for hiding this comment

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

Nice work. Still need to fix the issue with the bundling image.

lib/shared/shared-asset-bundler.ts Show resolved Hide resolved
massi-ang
massi-ang previously approved these changes Feb 2, 2024
@massi-ang massi-ang dismissed their stale review February 5, 2024 14:09

I am getting an error when deploying the same stack once more

@massi-ang
Copy link
Collaborator

Hi, I am getting an issue with the code, and need to investigate the underlying reason

@dfkunstler
Copy link
Contributor Author

dfkunstler commented Feb 5, 2024

Hi, I am getting an issue with the code, and need to investigate the underlying reason.

Coincidentally, i've just tried a clean build of 4.0.2 and am getting an error related to this import in the react app:
import * as InfraConfig from '../../../../bin/config.json';

Build output makes it seem like a bundling error, but i think the bundling error is a consequence of the compilation failure. Is this the same issue you're seeing? If so, keep in mind that the local bundling used in the UI construct was implemented previously and is not related to this PR.

@massi-ang
Copy link
Collaborator

massi-ang commented Feb 5, 2024

Hi, I am getting an issue with the code, and need to investigate the underlying reason.

Coincidentally, i've just tried a clean build of 4.0.2 and am getting an error related to this import in the react app: import * as InfraConfig from '../../../../bin/config.json';

Build output makes it seem like a bundling error, but i think the bundling error is a consequence of the compilation failure. Is this the same issue you're seeing? If so, keep in mind that the local bundling used in the UI construct was implemented previously and is not related to this PR.

This might be an unrelated issue. Try running npm run build && npm run create to create a new config.json and then try again.

@dfkunstler
Copy link
Contributor Author

dfkunstler commented Feb 5, 2024

Hi, I am getting an issue with the code, and need to investigate the underlying reason.

Coincidentally, i've just tried a clean build of 4.0.2 and am getting an error related to this import in the react app: import * as InfraConfig from '../../../../bin/config.json';
Build output makes it seem like a bundling error, but i think the bundling error is a consequence of the compilation failure. Is this the same issue you're seeing? If so, keep in mind that the local bundling used in the UI construct was implemented previously and is not related to this PR.

This might be an unrelated issue. Try running npm run build && npm run create to create a new config.json and then try again.

i'm confused about the decision to handle import that way; will open a separate issue (#360) to ask about it.

@massi-ang
Copy link
Collaborator

The solution with the static class member does not work for me. The only solution that seems to work is to use this pattern:

class BuildImageProvider extends Construct {
  public static getOrCreate(scope: Construct): DockerImage {
    const stack = Stack.of(scope);
    const id = "build-image-provider";
    const x =
      (Node.of(stack).tryFindChild(id) as BuildImageProvider) ||
      new BuildImageProvider(stack, id);
    return x.buildImage;
  }

  private readonly buildImage: DockerImage;

  constructor(scope: Construct, id: string) {
    super(scope, id);

    this.buildImage = DockerImage.fromBuild(
      path.posix.join(__dirname, "alpine-zip")
    );
  }
}

and then retrieve the image builder with

bundling: {
          image: BuildImageProvider.getOrCreate(this),

@dfkunstler
Copy link
Contributor Author

dfkunstler commented Feb 13, 2024

@massi-ang sorry just seeing your latest comment above. i'm happy to change it as you suggested, but can you please elaborate on what isn't working? are there specific errors or build failures that you're seeing? the current implementation is working fine in my environment, so i'd like to understand what's causing the trouble.

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

Successfully merging this pull request may close these issues.

3 participants