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.

Sign up for GitHub

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

Jump to bottom

feat(batch): add default AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set #21041

Merged
merged 41 commits into from
Jul 21, 2022
Merged

feat(batch): add default AWS_ACCOUNT and AWS_REGION to Batch container, if they are not explicitly set #21041

Show file tree
Hide file tree
Changes from 39 commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
fb6de3d
Merge remote-tracking branch 'remote/master'
Dzhuneyt Jan 31, 2022
c038921
fix(ec2): `UserData.addSignalOnExitCommand` does not work in combinat…
corymhall Jan 31, 2022
6a35cbd
Merge branch 'aws:master' into master
Dzhuneyt Feb 1, 2022
f2958c6
Merge branch 'aws:master' into master
Dzhuneyt Mar 17, 2022
5531ac1
Merge branch 'aws:master' into master
Dzhuneyt Mar 17, 2022
846ecd3
Merge branch 'aws:master' into master
Dzhuneyt Mar 23, 2022
ee14ff1
Merge branch 'aws:master' into master
Dzhuneyt May 2, 2022
2720ad1
Merge branch 'aws:main' into master
Dzhuneyt Jul 6, 2022
7fef2a2
feat: Pass default AWS_REGION and AWS_ACCOUNT
Dzhuneyt Jul 7, 2022
e726152
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 7, 2022
4fbcfcb
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 7, 2022
cb8e781
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 8, 2022
cf07efd
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 12, 2022
248319c
chore: Update README.md
Dzhuneyt Jul 12, 2022
364c74a
feat: Pass default environment variables
Dzhuneyt Jul 12, 2022
d488b86
feat: Pass default environment variables
Dzhuneyt Jul 12, 2022
c8bd318
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 12, 2022
52a9b4c
chore: Fix tests
Dzhuneyt Jul 12, 2022
66d92aa
chore: Restore original formatting
Dzhuneyt Jul 13, 2022
ccd5dc8
chore: Update README.md
Dzhuneyt Jul 13, 2022
f8f765e
chore: Update README.md
Dzhuneyt Jul 13, 2022
658bfaf
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 13, 2022
c5b0106
test: Try creating an integration test
Dzhuneyt Jul 13, 2022
6c7d0e4
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 13, 2022
285e3de
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
TheRealAmazonKendra Jul 14, 2022
9dd3c6b
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 14, 2022
552e393
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 14, 2022
aa4478e
chore: Update integration test snapshots
Dzhuneyt Jul 14, 2022
c2f1ac2
chore: Update integration test snapshots
Dzhuneyt Jul 14, 2022
66dc657
chore: Update integration test snapshots
Dzhuneyt Jul 14, 2022
6968942
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 14, 2022
420eba8
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 18, 2022
85fd70a
chore: Update integration test snapshots
Dzhuneyt Jul 18, 2022
d3d3323
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 18, 2022
752defa
chore: Update integration test snapshot of deps
Dzhuneyt Jul 18, 2022
d828c92
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 19, 2022
963aa6e
chore: Update integration test snapshots
Dzhuneyt Jul 19, 2022
2a025dd
chore: Update integration test snapshots
Dzhuneyt Jul 19, 2022
2366c3f
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 19, 2022
0eab1f6
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
Dzhuneyt Jul 21, 2022
be06bff
Merge branch 'main' into dzhuneyt/issue-10952-aws-batch-env-name
mergify[bot] Jul 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/@aws-cdk/aws-batch/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,8 @@ new batch.JobDefinition(this, 'batch-job-def-secrets', {
});
```

It is common practice to invoke other AWS services from within AWS Batch jobs (e.g. using the AWS SDK). For this reason, the AWS_ACCOUNT and AWS_REGION environments are always provided by default to the JobDefinition construct with the values inferred from the current context. You can always overwrite them by setting these environment variables explicitly though.

### Importing an existing Job Definition

#### From ARN
Expand Down
26 changes: 22 additions & 4 deletions packages/@aws-cdk/aws-batch/lib/job-definition.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,11 +476,11 @@ export class JobDefinition extends Resource implements IJobDefinition {
this.jobDefinitionName = this.getResourceNameAttribute(jobDef.ref);
}

private deserializeEnvVariables(env?: { [name: string]: string }): CfnJobDefinition.EnvironmentProperty[] | undefined {
private deserializeEnvVariables(env?: { [name: string]: string }): CfnJobDefinition.EnvironmentProperty[] {
const vars = new Array<CfnJobDefinition.EnvironmentProperty>();

if (env === undefined) {
return undefined;
return vars;
}

Object.keys(env).map((name: string) => {
Expand Down Expand Up @@ -519,9 +519,27 @@ export class JobDefinition extends Resource implements IJobDefinition {
return undefined;
}

// If the AWS_*** environment variables are not explicitly set to the container, infer them from the current environment.
// This makes the usage of tools like AWS SDK inside the container frictionless

const environment = this.deserializeEnvVariables(container.environment);

if (!environment.find((x) => x.name === 'AWS_REGION')) {
environment.push({
name: 'AWS_REGION',
value: Stack.of(this).region,
});
}
if (!environment.find((x) => x.name === 'AWS_ACCOUNT')) {
environment.push({
name: 'AWS_ACCOUNT',
value: Stack.of(this).account,
});
}

return {
command: container.command,
environment: this.deserializeEnvVariables(container.environment),
environment,
Copy link
Contributor

Choose a reason for hiding this comment

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

So, I'm wondering if always having values here is a breaking change for customers currently using this. Breaking changes are OK in an experimental module, but I'm wondering if we should make setting these defaults an option in the props instead of just doing it 100% of the time. I'm also wondering if there are use cases where a user would want this undefined. I'm not super familiar with AWS Batch so I think I need to consult with others on this.

Copy link
Contributor Author

@Dzhuneyt Dzhuneyt Jul 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not 100% sure about this specific scenario, but generally speaking, from my experience - when we talk about breaking change in the code of some library - it's usually the removal of parameters, functions, classes, or a modification of their existing behavior, whereas additions of fields, parameters, methods are not considered a breaking change.

But again, that's just my perspective, looking at other libraries I've consumed in my projects as a developer. With CDK it's slightly different since we are considering not just the code but also things that users might have deployed using that code, and I realize it's a slightly more flavored and opinionated discussion.

secrets: container.secrets
? Object.entries(container.secrets).map(([key, value]) => {
return {
Expand Down Expand Up @@ -584,4 +602,4 @@ export class JobDefinition extends Resource implements IJobDefinition {
};
});
}
}
}
1 change: 1 addition & 0 deletions packages/@aws-cdk/aws-batch/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
"@aws-cdk/assertions": "0.0.0",
"@aws-cdk/cdk-build-tools": "0.0.0",
"@aws-cdk/integ-runner": "0.0.0",
"@aws-cdk/integ-tests": "0.0.0",
"@aws-cdk/cfn2ts": "0.0.0",
"@aws-cdk/pkglint": "0.0.0",
"@types/jest": "^27.5.2",
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-batch/test/batch.integ.snapshot/batch-stack.assets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
{
"version": "20.0.0",
"files": {
"d3685c79f9ec67f5dd6fda839a136b079f201b3d72695fe0ea3b3788c3471cc8": {
"dd24d4801d80a639d9d7dcd67e4caa9af438a4af95a6243cdebbe188e79ba312": {
"source": {
"path": "batch-stack.template.json",
"packaging": "file"
},
"destinations": {
"current_account-current_region": {
"bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}",
"objectKey": "d3685c79f9ec67f5dd6fda839a136b079f201b3d72695fe0ea3b3788c3471cc8.json",
"objectKey": "dd24d4801d80a639d9d7dcd67e4caa9af438a4af95a6243cdebbe188e79ba312.json",
"assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}"
}
}
Expand Down
42 changes: 42 additions & 0 deletions packages/@aws-cdk/aws-batch/test/batch.integ.snapshot/batch-stack.template.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1659,6 +1659,20 @@
"Properties": {
"Type": "container",
"ContainerProperties": {
"Environment": [
{
"Name": "AWS_REGION",
"Value": {
"Ref": "AWS::Region"
}
},
{
"Name": "AWS_ACCOUNT",
"Value": {
"Ref": "AWS::AccountId"
}
}
],
"Image": {
"Fn::Join": [
"",
Expand Down Expand Up @@ -1735,6 +1749,20 @@
"Properties": {
"Type": "container",
"ContainerProperties": {
"Environment": [
{
"Name": "AWS_REGION",
"Value": {
"Ref": "AWS::Region"
}
},
{
"Name": "AWS_ACCOUNT",
"Value": {
"Ref": "AWS::AccountId"
}
}
],
"Image": "docker/whalesay",
"Privileged": false,
"ReadonlyRootFilesystem": false,
Expand Down Expand Up @@ -1806,6 +1834,20 @@
"Properties": {
"Type": "container",
"ContainerProperties": {
"Environment": [
{
"Name": "AWS_REGION",
"Value": {
"Ref": "AWS::Region"
}
},
{
"Name": "AWS_ACCOUNT",
"Value": {
"Ref": "AWS::AccountId"
}
}
],
"ExecutionRoleArn": {
"Fn::GetAtt": [
"executionroleD9A39BE6",
Expand Down
30 changes: 30 additions & 0 deletions packages/@aws-cdk/aws-batch/test/batch.integ.snapshot/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,36 @@
"data": "batchspotcomputeenv2CE4DFD9"
}
],
"/batch-stack/batch-demand-compute-env-launch-template-2/Resource-Security-Group/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "batchdemandcomputeenvlaunchtemplate2ResourceSecurityGroupBEA8DDD5"
}
],
"/batch-stack/batch-demand-compute-env-launch-template-2/Ecs-Instance-Role/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "batchdemandcomputeenvlaunchtemplate2EcsInstanceRoleEE146754"
}
],
"/batch-stack/batch-demand-compute-env-launch-template-2/Instance-Profile": [
{
"type": "aws:cdk:logicalId",
"data": "batchdemandcomputeenvlaunchtemplate2InstanceProfileC5A36CBC"
}
],
"/batch-stack/batch-demand-compute-env-launch-template-2/Resource-Service-Instance-Role/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "batchdemandcomputeenvlaunchtemplate2ResourceServiceInstanceRole41CADAC1"
}
],
"/batch-stack/batch-demand-compute-env-launch-template-2/Resource": [
{
"type": "aws:cdk:logicalId",
"data": "batchdemandcomputeenvlaunchtemplate2E12D5CBC"
}
],
"/batch-stack/batch-job-queue/Resource": [
{
"type": "aws:cdk:logicalId",
Expand Down
Loading