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(lambda-python): support setting environment vars for bundling #18635

Merged
merged 4 commits into from
Jan 28, 2022

Conversation

setu4993
Copy link
Contributor

@setu4993 setu4993 commented Jan 25, 2022

While using the Python Lambda with Code Artifact, discovered that Code Artifact was still inaccessible because bundling occurs at run time, which can only access env vars, not build args.

This is not a security issue because bundled output doesn't contain any of the secret values.

Note: Without this, using Code Artifact (or any other private packaging for Python Lambdas) is currently broken.


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

@gitpod-io
Copy link

gitpod-io bot commented Jan 25, 2022

Comment on lines 6 to 8
# Upgrade pip (required by cryptography v3.4 and above, which is a dependency of poetry)
RUN pip install --upgrade pip
RUN pip install pipenv poetry
Copy link
Contributor Author

@setu4993 setu4993 Jan 25, 2022

Choose a reason for hiding this comment

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

Moved this up since if using Code Artifact, this install uses the private repo, which forces rebuilds of the Docker image every time (and skipping Docker cache).

@setu4993 setu4993 force-pushed the fix/lambda-python/bundling-arg-env branch from 1847f89 to 2e8ba56 Compare January 25, 2022 05:49
Comment on lines 233 to 250
test('Bundling with custom build arg for `PIP_EXTRA_INDEX_URL`', () => {
const entry = path.join(__dirname, 'lambda-handler');
const testPypi = 'https://test.pypi.org/simple/';
Bundling.bundle({
entry: entry,
runtime: Runtime.PYTHON_3_7,
buildArgs: { PIP_EXTRA_INDEX_URL: testPypi },
});

expect(DockerImage.fromBuild).toHaveBeenCalledWith(expect.stringMatching(path.join(__dirname, '../lib')), expect.objectContaining({
buildArgs: expect.objectContaining({
PIP_EXTRA_INDEX_URL: testPypi,
}),
}));
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added this to pacify the validate-pr CI. Not sure if there's a way to validate env vars in Docker images (the change we are making here).

@setu4993 setu4993 changed the title fix(lambda-python): set env vars for bundling fix(lambda-python): set env vars from build args for bundling Jan 25, 2022
@setu4993
Copy link
Contributor Author

@corymhall : Any feedback on this? Tiny fix, so would be great to get into the next release.

@corymhall
Copy link
Contributor

@setu4993

Alternatively, this could be passed as environment variables to the BundlingOptions, but that'd require quite a bit more of refactoring to modify the current DockerImage.fromBuild(...) call, which doesn't support the environment prop.

I think it should be pretty easy to add environment variable support. You just need to add environment to BundlingOptions and then assign it to a public environment property on Bundling. Those env vars then get added to the Docker environment.

public readonly environment?: { [key: string]: string };

this.environment = props.environment;

@setu4993
Copy link
Contributor Author

I think it should be pretty easy to add environment variable support. You just need to add environment to BundlingOptions and then assign it to a public environment property on Bundling. Those env vars then get added to the Docker environment.

Neat! I thought I was missing something obvious. Thanks for the pointer, updating the PR.

@setu4993 setu4993 force-pushed the fix/lambda-python/bundling-arg-env branch from 2e8ba56 to 220d089 Compare January 26, 2022 07:39
@setu4993 setu4993 changed the title fix(lambda-python): set env vars from build args for bundling fix(lambda-python): support setting environment vars for bundling Jan 26, 2022
@setu4993 setu4993 force-pushed the fix/lambda-python/bundling-arg-env branch from 00005df to a8dbafe Compare January 26, 2022 08:32
@setu4993 setu4993 changed the title fix(lambda-python): support setting environment vars for bundling feat(lambda-python): support setting environment vars for bundling Jan 26, 2022
Copy link
Contributor Author

@setu4993 setu4993 left a comment

Choose a reason for hiding this comment

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

@corymhall : Thanks for the tip! Updated the PR and docs.

});
```

The index URL or the token are only used during bundling and thus not included in the final asset.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added note about tokens not being persisted in the final asset.

@@ -172,4 +199,33 @@ new lambda.PythonFunction(this, 'function', {
});
```

This type of an example should work for `pip` and `poetry` based dependencies, but will not work for `pipenv`.
**Note:** Setting custom build args for bundling will force the base bundling image to be rebuilt every time (i.e. skip the Docker cache).
Copy link
Contributor Author

@setu4993 setu4993 Jan 26, 2022

Choose a reason for hiding this comment

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

And about rebuilding Docker images for bundling if using build args.

Setting only environment variable should work for `pip` and `poetry` based dependencies, whereas `pipenv` based dependencies will require **both** build args and environment variables to be set.


Example for using Code Artifact with `pipenv`-based dependencies:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Running into this also made clear how we could use pipenv with Code Artifact.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does pipenv require both env vars and build args?

Copy link
Contributor Author

@setu4993 setu4993 Jan 28, 2022

Choose a reason for hiding this comment

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

Thanks for asking, @corymhall. That was an incorrect note on my part stemming from how bundling was occurring earlier (in the Docker build step), and I didn't completely understand it since I don't use pipenv.

Over the last couple days, I created a test project with pipenv and validated installing a package into it that is only on Test PyPI. It works with the PIP_INDEX_URL environment variable set (same as for pip and poetry), which it'll be when the Docker run step occurs. I updated the documentation now to reflect that.

packages/@aws-cdk/aws-lambda-python/test/bundling.test.ts Outdated Show resolved Hide resolved
Setting only environment variable should work for `pip` and `poetry` based dependencies, whereas `pipenv` based dependencies will require **both** build args and environment variables to be set.


Example for using Code Artifact with `pipenv`-based dependencies:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does pipenv require both env vars and build args?

@mergify mergify bot dismissed corymhall’s stale review January 28, 2022 08:27

Pull request has been modified.

@setu4993 setu4993 force-pushed the fix/lambda-python/bundling-arg-env branch from 3a75397 to 1a7fc45 Compare January 28, 2022 08:33
Copy link
Contributor

@corymhall corymhall left a comment

Choose a reason for hiding this comment

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

Looks good!

@mergify
Copy link
Contributor

mergify bot commented Jan 28, 2022

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: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 90fe3ad
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 30e2233 into aws:master Jan 28, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 28, 2022

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).

@setu4993 setu4993 deleted the fix/lambda-python/bundling-arg-env branch January 28, 2022 21:01
TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
…ws#18635)

While using the Python Lambda with Code Artifact, discovered that Code Artifact was still inaccessible because bundling occurs at _run_ time, which can only access env vars, not build args.

This is not a security issue because bundled output doesn't contain any of the secret values.

**Note:** Without this, using Code Artifact (or any other private packaging for Python Lambdas) is currently broken.

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants