feat(ecs): credentialSpecs in ContainerDefinitionOptions#29085
feat(ecs): credentialSpecs in ContainerDefinitionOptions#29085mergify[bot] merged 14 commits intoaws:mainfrom cresvi:cresvi/ecs-credentialSpecs
Conversation
…ContainerDefinitions.CredentialSpecs property
aws-cdk-automation
left a comment
There was a problem hiding this comment.
The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.
A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
| credentialSpecs: ['credentialspecdomainless:arn:aws:s3:::bucket_name/key_name'], | ||
| // Valid values are: 'credentialspecdomainless:ARN' or 'credentialspec:ARN' |
There was a problem hiding this comment.
An idea here. What if we exposed two properties, credentailSpecs and credentialSpecsDomainless, both which just takes the ARN component (to be clearly documented in the docstring). And then we can append the correct prefix before sending it to ecs. This will save users from having to do the appending themselves, right?
There was a problem hiding this comment.
Ok, I re-implemented the functionality using different specific-purpose classes, similar to other properties like environmentFiles, and at the same time I allowed for some flexibility by allowing the creation of a class where you can freely define the credentialSpec components in case new prefixes or locations are added in the future. Please take a look at the new code and let me know if you have additional comments.
There was a problem hiding this comment.
I think this is exactly what we want and this exact care to concoct purpose-driven APIs is what makes CDK L2s powerful. People who want the raw string format + other bare bones types use L1s.
Missing semicolon in doc Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>
Pull request has been modified.
| /** | ||
| * Prefix string based on the type of CredSpec. | ||
| */ | ||
| public prefixId: string; |
There was a problem hiding this comment.
| public prefixId: string; | |
| public readonly prefixId: string; |
| /** | ||
| * Location or ARN from where to retrieve the CredSpec file. | ||
| */ | ||
| public fileLocation: string; |
There was a problem hiding this comment.
| public fileLocation: string; | |
| public readonly fileLocation: string; |
| /** | ||
| * Prefix Id for this type of CredSpec. | ||
| */ | ||
| public static readonly PREFIX_ID = 'credentialspec'; |
There was a problem hiding this comment.
do we need this as a public variable? don't think it needs to be
There was a problem hiding this comment.
done, moved to constructor
| /** | ||
| * Prefix Id for this type of CredSpec. | ||
| */ | ||
| public static readonly PREFIX_ID = 'credentialspecdomainless'; |
There was a problem hiding this comment.
same here. I think we can just put the raw string in the constructor
There was a problem hiding this comment.
done, moved to constructor
| /** | ||
| * Get the ARN for an S3 object. | ||
| */ | ||
| protected static arnForS3Object(bucket: IBucket, key: string) { | ||
| if (!key) { | ||
| throw new Error('key is undefined'); | ||
| } | ||
|
|
||
| return bucket.arnForObjects(key); | ||
| } | ||
|
|
||
| /** | ||
| * Get the ARN for a SSM parameter. | ||
| */ | ||
| protected static arnForSsmParameter(parameter: IParameter) { | ||
| return parameter.parameterArn; | ||
| } |
There was a problem hiding this comment.
I was initially confused by this section. Can we document both clearly that they are only intended to be used by extensions of CredentialSpec as helper methods?
There was a problem hiding this comment.
Updated a more detailed description
| return new DomainJoinedCredentialSpec(CredentialSpec.arnForSsmParameter(parameter)); | ||
| } | ||
|
|
||
| constructor(fileLocation: string) { |
There was a problem hiding this comment.
| constructor(fileLocation: string) { | |
| public constructor(fileLocation: string) { |
| /** | ||
| * @param fileLocation Location or ARN from where to retrieve the CredSpec file | ||
| */ | ||
| constructor(prefixId: string, fileLocation: string) { |
There was a problem hiding this comment.
| constructor(prefixId: string, fileLocation: string) { | |
| public constructor(prefixId: string, fileLocation: string) { |
| credentialSpecs: ['credentialspecdomainless:arn:aws:s3:::bucket_name/key_name'], | ||
| // Valid values are: 'credentialspecdomainless:ARN' or 'credentialspec:ARN' |
There was a problem hiding this comment.
I think this is exactly what we want and this exact care to concoct purpose-driven APIs is what makes CDK L2s powerful. People who want the raw string format + other bare bones types use L1s.
| image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"), | ||
| cpu: 128, | ||
| memoryLimitMiB: 256, | ||
| credentialSpecs: [ecs.DomainlessCredentialSpec.fromS3Bucket(bucket, 'credSpec')], |
There was a problem hiding this comment.
can we mention somewhere that there are options for DomainJoinedCredentialSpec and also fromSssmParameter. Don't need examples for all, but just document their availability after this example
There was a problem hiding this comment.
Expanded documentation to give more information and show sample of the four use cases
| /** | ||
| * A list of ARNs in SSM or Amazon S3 to a credential spec (`CredSpec`) file that configures the container for Active Directory authentication. | ||
| * | ||
| * We recommend that you use this parameter instead of the `dockerSecurityOptions`. The maximum number of ARNs is 1. |
There was a problem hiding this comment.
The maximum number of ARNs is 1
I know you're pulling this directly from the docs themselves, but this is confusing to me. It sounds like the max number of ARNs this credentialSpecs property takes is 1, and then why type it as a string at all?
I see two scenarios:
a) I misunderstand. In that case, lets make the docs a bit clearer.
b) The max number of ARNs for now is 1, but might be more in the future. Lets document that if thats the decision behind the array type.
But given that we aren't doing any synth time checks for this limit of 1, I think we are in scenario a)...
There was a problem hiding this comment.
I also think the way the engineer team implemented this, but in the API the credentialSpecs accepts an array of strings. Still they only use the first one...
I updated the comment a little to make it clearer. I don't want to limit the L2 construct to only 1 CredSpec because if later the service team decide to use the rest of the entries, it will be a breaking change for customers.
Pull request has been modified.
| } | ||
| } | ||
|
|
||
| if (props.credentialSpecs) { |
There was a problem hiding this comment.
Based off of a previous comment, we should have a synth-time check (+ unit test) for ensuring that credentialSpecs.length === 1. We never want to silently ignore additional info
There was a problem hiding this comment.
I wanted to avoid the need to create a new PR if a new feature starts using more than one CredSpec in the future. But if you really insist, I made the change and added the appropriate unit test.
| /** | ||
| * A list of ARNs in SSM or Amazon S3 to a credential spec (`CredSpec`) file that configures the container for Active Directory authentication. | ||
| * | ||
| * We recommend that you use this parameter instead of the `dockerSecurityOptions`. Only the first entry on this array is used. This may be expanded in the future. |
There was a problem hiding this comment.
| * We recommend that you use this parameter instead of the `dockerSecurityOptions`. Only the first entry on this array is used. This may be expanded in the future. | |
| * We recommend that you use this parameter instead of the `dockerSecurityOptions`. | |
| * Currently, only one credential spec is allowed per container definition. |
Pull request has been modified.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
### Issue # (if applicable) Closes #N/A ### Reason for this change v2.127.0 updated the L1 construct for AWS::ECS::TaskDefinition, adding support for the property ContainerDefinitions.CredentialSpecs, [see](#29053). This PR adds support for CredentialSpecs property in the L2 construct used by `Ec2TaskDefinition.addContainer` method. ### Description of changes Added property in L2 construct, updated unit test and added integration test. ### Description of how you validated changes - [x] Unit test updated and validated - [x] Integration test added and validated ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Issue # (if applicable)
Closes #N/A
Reason for this change
v2.127.0 updated the L1 construct for AWS::ECS::TaskDefinition, adding support for the property ContainerDefinitions.CredentialSpecs, see. This PR adds support for CredentialSpecs property in the L2 construct used by
Ec2TaskDefinition.addContainermethod.Description of changes
Added property in L2 construct, updated unit test and added integration test.
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license