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

RFC 485: AWS Batch Constructs #484

Merged
merged 22 commits into from
Apr 19, 2023
Merged

RFC 485: AWS Batch Constructs #484

merged 22 commits into from
Apr 19, 2023

Conversation

comcalvi
Copy link
Contributor

@comcalvi comcalvi commented Feb 28, 2023

This is a request for comments on a set of Batch L2 Constructs. See #485 for
additional details.

APIs are signed off by @rix0rrr.


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

batch-l2.md Outdated Show resolved Hide resolved
batch-l2.md Outdated Show resolved Hide resolved
batch-l2.md Outdated Show resolved Hide resolved
batch-l2.md Outdated Show resolved Hide resolved
batch-l2.md Outdated
Comment on lines 357 to 358
secret: new Secret(this, 'mySecret'),
mountPath: '/Volumes/secret',

Choose a reason for hiding this comment

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

https://kubernetes.io/docs/concepts/storage/volumes/#secret
https://docs.aws.amazon.com/batch/latest/APIReference/API_EksSecret.html

Batch supports secret volumes in kube pods. That means the actual secret value is stored in Kube, life-cycled and authorized by the customer within Kube. The Batch job definition just points to that secret location; setting or specifying secret values is not support in Batch.

Copy link
Contributor Author

@comcalvi comcalvi Mar 2, 2023

Choose a reason for hiding this comment

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

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-batch-jobdefinition-eksvolume.html
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-batch-jobdefinition-secret.html

I used the Secret class here because the EKS Volume Secret requires you to specify a Secrets Manager or SSM Parameter ARN as well as the secret name. The CDK Secret construct exposes both it's ARN and name, so it seems natural to use it here.

How is the Secret ARN used by the volume? I thought that the Volume would pull the relevant information from the Secret to set it up, since you have to specify the ARN, but it sounds like that's not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline, the CFN types are wrong here

batch-l2.md Outdated
}

class SecretPathVolume extends EksVolume {
secret: ssm.Secret;

Choose a reason for hiding this comment

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

Kube (eks) secret is just a volume location pointing a a kube secret resource. SSM is not directly involved in this mapping.

customers have the ability to populate kube secrets from SSM, but those are still exposed to pods as a generic volume location and secretName string.

batch-l2.md Outdated Show resolved Hide resolved
rix0rrr
rix0rrr previously approved these changes Mar 2, 2023
Copy link
Contributor

@rix0rrr rix0rrr left a comment

Choose a reason for hiding this comment

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

I approve of most of this, couple of details to work out but otherwise good!

text/batch-l2.md Outdated Show resolved Hide resolved
text/batch-l2.md Outdated Show resolved Hide resolved
text/batch-l2.md Outdated Show resolved Hide resolved
text/batch-l2.md Outdated Show resolved Hide resolved
text/batch-l2.md Outdated Show resolved Hide resolved
text/batch-l2.md Outdated Show resolved Hide resolved
text/batch-l2.md Outdated Show resolved Hide resolved
text/batch-l2.md Outdated Show resolved Hide resolved
text/batch-l2.md Outdated Show resolved Hide resolved
text/batch-l2.md Outdated Show resolved Hide resolved
@rix0rrr rix0rrr added the pr/do-not-merge Let mergify know not to auto merge label Mar 2, 2023
@comcalvi comcalvi changed the title AWS Batch L2 RFC RFC 485: AWS Batch Constructs Mar 2, 2023
@comcalvi comcalvi marked this pull request as ready for review March 3, 2023 01:20
@mergify mergify bot dismissed rix0rrr’s stale review March 3, 2023 01:21

Pull request has been modified.

text/0485-aws-batch.md Outdated Show resolved Hide resolved
@comcalvi comcalvi removed the pr/do-not-merge Let mergify know not to auto merge label Apr 11, 2023
@comcalvi comcalvi merged commit 70a784d into master Apr 19, 2023
2 checks passed
@comcalvi comcalvi deleted the new-batch-l2 branch April 19, 2023 21:04
@evgenyka evgenyka added the l2-request request for new L2 construct label Oct 11, 2023
@mrgrain mrgrain added the status/done Implementation complete label Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
l2-request request for new L2 construct status/done Implementation complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants