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(ecs): credentialSpecs in ContainerDefinitionOptions #29085

Merged

mergify merged 14 commits into aws:main from cresvi:cresvi/ecs-credentialSpecs

Feb 16, 2024

Contributor

cresvi commented Feb 13, 2024

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.addContainer method.

Description of changes

Added property in L2 construct, updated unit test and added integration test.

Description of how you validated changes

Unit test updated and validated
Integration test added and validated

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

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

cresvi added 2 commits

February 12, 2024 22:47


          Adding L2 construct support for setting the AWS::ECS::TaskDefinition …

ae681a2

…ContainerDefinitions.CredentialSpecs property


          Added integration test for previous commit

6a21674

github-actions bot added the p2 label

aws-cdk-automation requested a review from a team

February 13, 2024 05:17

github-actions bot added the beginning-contributor label

aws-cdk-automation previously requested changes

View reviewed changes

Collaborator

aws-cdk-automation left a 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.

cresvi added 2 commits

February 14, 2024 12:27


          Added Readme update

2b49d5c


          Added integration test snapshot

e0b3e1e

aws-cdk-automation dismissed their stale review

February 14, 2024 18:33

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

kaizencc changed the title ~~feat(ecs): support for credentialSpecs in ContainerDefinitionOptions~~ feat(ecs): credentialSpecs in ContainerDefinitionOptions

kaizencc previously requested changes

View reviewed changes

Contributor

kaizencc left a comment

Hi @cresvi, thanks for opening this PR. I have a slight change in the API that I think would help. Lemme know what you think

packages/aws-cdk-lib/aws-ecs/README.md Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-ecs/README.md Outdated

Comment on lines 641 to 642

		credentialSpecs: ['credentialspecdomainless:arn:aws:s3:::bucket_name/key_name'],
		// Valid values are: 'credentialspecdomainless:ARN' or 'credentialspec:ARN'

Contributor

kaizencc Feb 14, 2024

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?

Contributor Author

cresvi Feb 14, 2024 •

edited

Loading

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.

Contributor

kaizencc Feb 16, 2024

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.


          Update packages/aws-cdk-lib/aws-ecs/README.md

7edc755

Missing semicolon in doc

Co-authored-by: Kaizen Conroy <36202692+kaizencc@users.noreply.github.com>

mergify bot dismissed kaizencc’s stale review

February 14, 2024 20:07

Pull request has been modified.

cresvi and others added 2 commits

February 14, 2024 22:58


          Fixes error in name for integration tests

cde5c1a


          Fixe example on Readme

077781c

aws-cdk-automation added the pr/needs-community-review label

cresvi added 4 commits

February 16, 2024 01:15


          Updated credential spec implementation. Untested

b6a4cc0


          UPdated credential spec code and added additional unit tests

55dd28a


          Updated integration test with new credSpec API

b1c92a6


          Removed versioning from s3 objects

8f565ba

kaizencc previously requested changes

View reviewed changes

Contributor

kaizencc left a comment

Hi @cresvi, I think what you've got now is much, much better than typing things as string[]. Just a few more comments here and there.

packages/aws-cdk-lib/aws-ecs/lib/credential-spec.ts Outdated

+                /**
+                 * Prefix string based on the type of CredSpec.
+                 */
+                public prefixId: string;

Contributor

kaizencc Feb 16, 2024

Suggested change

      
              public prefixId: string;
          
              public readonly prefixId: string;

Contributor Author

cresvi Feb 16, 2024

done

packages/aws-cdk-lib/aws-ecs/lib/credential-spec.ts Outdated

+                /**
+                 * Location or ARN from where to retrieve the CredSpec file.
+                 */
+                public fileLocation: string;

Contributor

kaizencc Feb 16, 2024

Suggested change

      
              public fileLocation: string;
          
              public readonly fileLocation: string;

Contributor Author

cresvi Feb 16, 2024

done

packages/aws-cdk-lib/aws-ecs/lib/credential-spec.ts Outdated

+                /**
+                 * Prefix Id for this type of CredSpec.
+                 */
+                public static readonly PREFIX_ID = 'credentialspec';

Contributor

kaizencc Feb 16, 2024

do we need this as a public variable? don't think it needs to be

Contributor Author

cresvi Feb 16, 2024

done, moved to constructor

packages/aws-cdk-lib/aws-ecs/lib/credential-spec.ts Outdated

+                /**
+                 * Prefix Id for this type of CredSpec.
+                 */
+                public static readonly PREFIX_ID = 'credentialspecdomainless';

Contributor

kaizencc Feb 16, 2024

same here. I think we can just put the raw string in the constructor

Contributor Author

cresvi Feb 16, 2024

done, moved to constructor

packages/aws-cdk-lib/aws-ecs/lib/credential-spec.ts Outdated

Comment on lines 8 to 24

+                /**
+                 * 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;
+                }

Contributor

kaizencc Feb 16, 2024

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?

Contributor Author

cresvi Feb 16, 2024

Updated a more detailed description

packages/aws-cdk-lib/aws-ecs/lib/credential-spec.ts Outdated

+                  return new DomainJoinedCredentialSpec(CredentialSpec.arnForSsmParameter(parameter));
+                }
+                constructor(fileLocation: string) {

Contributor

kaizencc Feb 16, 2024

Suggested change

      
              constructor(fileLocation: string) {
          
              public constructor(fileLocation: string) {

Contributor Author

cresvi Feb 16, 2024

done

packages/aws-cdk-lib/aws-ecs/lib/credential-spec.ts Outdated

+                /**
+                 * @param fileLocation Location or ARN from where to retrieve the CredSpec file
+                 */
+                constructor(prefixId: string, fileLocation: string) {

Contributor

kaizencc Feb 16, 2024

Suggested change

      
              constructor(prefixId: string, fileLocation: string) {
          
              public constructor(prefixId: string, fileLocation: string) {

Contributor Author

cresvi Feb 16, 2024

done

packages/aws-cdk-lib/aws-ecs/README.md Outdated

Comment on lines 641 to 642

		credentialSpecs: ['credentialspecdomainless:arn:aws:s3:::bucket_name/key_name'],
		// Valid values are: 'credentialspecdomainless:ARN' or 'credentialspec:ARN'

Contributor

kaizencc Feb 16, 2024

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.

packages/aws-cdk-lib/aws-ecs/README.md

+                image: ecs.ContainerImage.fromRegistry("amazon/amazon-ecs-sample"),
+                cpu: 128,
+                memoryLimitMiB: 256,
+                credentialSpecs: [ecs.DomainlessCredentialSpec.fromS3Bucket(bucket, 'credSpec')],

Contributor

kaizencc Feb 16, 2024

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

Contributor Author

cresvi Feb 16, 2024

Expanded documentation to give more information and show sample of the four use cases

packages/aws-cdk-lib/aws-ecs/lib/container-definition.ts Outdated

+                /**
+                 * 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.

Contributor

kaizencc Feb 16, 2024

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

Contributor Author

cresvi Feb 16, 2024

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.

aws-cdk-automation removed the pr/needs-community-review label


          Attended comments; Updated integ test

73ea435

mergify bot dismissed kaizencc’s stale review

February 16, 2024 15:59

Pull request has been modified.

aws-cdk-automation added the pr/needs-community-review label

kaizencc previously requested changes

View reviewed changes

Contributor

kaizencc left a comment

One last comment @cresvi :)

packages/aws-cdk-lib/aws-ecs/lib/container-definition.ts

@@ @@ -538,6 +553,14 @@ export class ContainerDefinition extends Construct { @@
                     }
                   }
+                  if (props.credentialSpecs) {

Contributor

kaizencc Feb 16, 2024

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

Contributor Author

cresvi Feb 16, 2024

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.

packages/aws-cdk-lib/aws-ecs/lib/container-definition.ts Outdated

+                /**
+                 * 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.

Contributor

kaizencc Feb 16, 2024

Suggested change

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

Contributor Author

cresvi Feb 16, 2024

done

aws-cdk-automation removed the pr/needs-community-review label


          Added logic to validate only 1 credspec is provided

67a97ae

mergify bot dismissed kaizencc’s stale review

February 16, 2024 17:30

Pull request has been modified.

aws-cdk-automation added the pr/needs-community-review label

kaizencc approved these changes

View reviewed changes

Contributor

mergify bot commented Feb 16, 2024

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-cdk-automation removed the pr/needs-community-review label


          Merge branch 'main' into cresvi/ecs-credentialSpecs

8698eb2

Collaborator

aws-cdk-automation commented Feb 16, 2024

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 8698eb2
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify bot merged commit e92dbec into aws:main

9 checks passed

Contributor

mergify bot commented Feb 16, 2024

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

GavinZZ pushed a commit that referenced this pull request


          feat(ecs): credentialSpecs in ContainerDefinitionOptions (#29085)

e197371

### 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*

github-actions bot mentioned this pull request

Monthly PR metrics report - Feb 2023 #29328

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beginning-contributor p2