-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
fix(ecs): new arn format not supported (under feature flag) #18140
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for contributing this fix, @jumic! This looks great, I just have a couple questions/comments.
@@ -32,22 +37,38 @@ export function fromServiceAtrributes(scope: Construct, id: string, attrs: Servi | |||
throw new Error('You can only specify either serviceArn or serviceName.'); | |||
} | |||
|
|||
const featureConstruct = new CoreConstruct(scope, randomString()); | |||
const newArnFormat = FeatureFlags.of(featureConstruct).isEnabled(ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you create a new construct to check the feature flags on here? Shouldn't it be possible to just do this?
const newArnFormat = FeatureFlags.of(featureConstruct).isEnabled(ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME); | |
const newArnFormat = FeatureFlags.of(scope).isEnabled(ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried FeatureFlags.of(scope)
first because it should be the preferred solution. Unfortunately, it causes this error:
Argument of type 'import("/Users/julian/dev/aws-cdk/node_modules/constructs/lib/construct").Construct' is not assignable to parameter of type 'import("/Users/julian/dev/aws-cdk/packages/@aws-cdk/core/lib/construct-compat").Construct'.
Type 'Construct' is missing the following properties from type 'Construct': node, validate, prepare, synthesizets(2345)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since FeatureFlags expects a Construct
from the core library, I think we can just switch to using that one in the fromServiceAttributes()
function. I pushed this change to your branch, waiting for the build to see if this has other impacts.
if (Token.isUnresolved(ec2ServiceArn)) { | ||
if (FeatureFlags.of(this).isEnabled(ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME)) { | ||
const components = Fn.split(':', ec2ServiceArn); | ||
const lastComponents = Fn.split('/', Fn.select(5, components)); | ||
this.serviceName = Fn.select(2, lastComponents); | ||
} else { | ||
this.serviceName = resourceName; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is repeated (or at least extremely similar) here, in fargate-service.ts and in from-service-attributes.ts. We should factor this out to one place. It could probably be shared from from-service-attributes.ts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the code to a new function extractServiceNameFromArn
in from-service-attributes.ts
.
const resourceNameSplit = resourceName.split('/'); | ||
this.serviceName = resourceNameSplit.length === 1 ? resourceName : resourceNameSplit[1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be factored out to one place as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved the code to a new function extractServiceNameFromArn
in from-service-attributes.ts
.
} | ||
return new Import(scope, id); | ||
return new Import(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be missing something here, but I don't understand this. Can you explain why is this changing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, passing the parameters was necessary because they were passed to the super class. The new implementation contains a constructor. In this case, the super()
call has to be implemented in the constructor. Therefore, the parameters are not necessary anymore.
If you prefer the existing code (return new Import(scope, id);
), I could implement it too. For example:
constructor(importScope: Construct, importId: string) {
super(importScope, importId);
// ...
}
}
return new Import(scope, id);
However, we have to use new attribute names. If I use scope
instead of importScope
, the following error is displayed:'scope' is already declared in the upper scope.eslint@typescript-eslint/no-shadow
.
Which option do you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No constructor is used anymore --> done.
'Fn::Select': [ | ||
1, | ||
{ | ||
'Fn::Split': [ | ||
'/', | ||
{ | ||
'Fn::Select': [ | ||
5, | ||
{ | ||
'Fn::Split': [ | ||
':', | ||
{ Ref: 'ARN' }, | ||
], | ||
}, | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oof, this is very hard to determine if this is correct (although I am pretty sure it is). And unfortunately, I don't think there is another way to implement this to make the resulting template more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I add integration tests that show the result using CloudFormation Outputs? I tested the changes using this code:
const cluster = new ecs.Cluster(this, 'Cluster', {
vpc,
});
const param = new CfnParameter(this, 'Param', {
default: 'arn:aws:ecs:us-west-2:123456789012:service/my-http-service',
})
const fromFargateServiceArn = ecs.FargateService.fromFargateServiceArn(this, 'fromFargateServiceArn', param.valueAsString);
new CfnOutput(this, 'fromFargateServiceArnOutput', {
value: fromFargateServiceArn.serviceName,
});
const fromEc2ServiceArn = ecs.Ec2Service.fromEc2ServiceArn(this, 'fromEc2ServiceArn', param.valueAsString);
new CfnOutput(this, 'fromEc2ServiceArnOutPut', {
value: fromEc2ServiceArn.serviceName,
});
const fromEc2ServiceAttributes = ecs.Ec2Service.fromEc2ServiceAttributes(this, 'fromEc2ServiceAttributes', {
cluster,
serviceArn: param.valueAsString,
});
new CfnOutput(this, 'fromEc2ServiceAttributesOutput', {
value: fromEc2ServiceAttributes.serviceName,
});
const fromFargateServiceAttributes = ecs.FargateService.fromFargateServiceAttributes(this, 'fromFargateServiceAttributes', {
cluster,
serviceArn: param.valueAsString,
});
new CfnOutput(this, 'fromFargateServiceAttributesOutput', {
value: fromFargateServiceAttributes.serviceName,
});
New ARN Format and feature flag enabled (CfnParameter
: arn:aws:ecs:us-west-2:123456789012:service/cluster-name/my-http-service
) - Output:
MyStack.fromEc2ServiceArnOutPut = my-http-service
MyStack.fromEc2ServiceAttributesOutput = my-http-service
MyStack.fromFargateServiceArnOutput = my-http-service
MyStack.fromFargateServiceAttributesOutput = my-http-service
Old ARN Format and feature flag disabled (CfnParameter
: arn:aws:ecs:us-west-2:123456789012:service/my-http-service
) - Output:
MyStack.fromEc2ServiceArnOutPut = my-http-service
MyStack.fromEc2ServiceAttributesOutput = my-http-service
MyStack.fromFargateServiceArnOutput = my-http-service
MyStack.fromFargateServiceAttributesOutput = my-http-service
Implementing this as a feature flag impacts both existing and new services. The use case would be if you have an service which was created with the old Arn format and are extending your CDK to create a addition new ECS service which would use the new Arn format. The feature flag would need to be set for the new ECS service with the new Arn Format. But it would impact the existing service as well. Maybe a property would be more desirable? |
Pull request has been modified.
You're right. The solution in this PR doesn't provide an option to use the new and the old format in a single stack at the same time. I also proposed a new property in my original comment. @tobytipton Do you have the requirement to mix the old and the new format in a single stack for tokenized ARNs? In this case, we could think about implementing a property. If there is no requirement, I wouldn't support this because we don't know if somebody will ever use it. In any case, the feature flag should be implemented. This simplifies the use of the method, since the |
The current code for creating the code which would be generating the tokenized ARN has the clusterName and the physicalName in the ARN, which means the token would in theory be the new format. That has issues as well though, I have created #18246 for that. Here is the scenario I can think of for having new and old formats in the token. I see in your tests you are having the token coming from a CFN parameter, but still you could have a new and old format at the same time. So possibly there does need to be both a feature flag and a property? |
I totally agree. In this case, we need both. A property and a feature flag. @madeline-k Should I add a property to support parsing the old and the new format from a tokenized ARN in a single stack? |
Hey @tobytipton and @jumic, thanks for working on this issue! There is currently some overlap between this PR and #18382. What I'm thinking is this PR should fix the issues with |
@jumic, regarding:
I'd say if this is something that we can support later, and this implementation is not going to be closing a door to that, then we should no implement the property in this PR, and keep the scope smaller. |
@madeline-k Yes, this makes totally sense. Keeping the pull requests small was also my original idee when I asked to create a separate issue for the region/account topic.
We can add this feature later if it will be requested. The current logic must not be changed. Therefore, we are free to add it later if necessary. |
…ries should be using Construct from core
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making this contribution @jumic! Sorry for the delay in getting back to reviewing this. To summarize what has already been said on this thread: this change will use the new ARN format when the ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME
feature flag is enabled. Since feature flags operate at the stack level, this means the old and new ARN formats cannot both be used in a single stack. If there is a need to be able to use both the old and new ARN format in a single stack, then we can open an issue for that and add a property to control the ARN format at the construct level.
The last change that is needed here is an update to the README to explain the feature flag, and explain the restriction about not being able to use the old and new formats within one stack. @jumic, if you are able to include that that would be great!
This change is motivated by #18140, where we are running into trouble because the current scope is a `constructs.Construct` but `FeatureFlags.of` required a `core.construct`. ---- ### All Submissions: * [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md) ### Adding new Unconventional Dependencies: * [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/master/CONTRIBUTING.md/#adding-new-unconventional-dependencies) ### New Features * [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/master/INTEGRATION_TESTS.md)? * [ ] Did you use `cdk-integ` to deploy the infrastructure and generate the snapshot (i.e. `cdk-integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Pull request has been modified.
@jumic I tried to do some quick fixes because of the branch change but there are still some test failures to look at. Could you please take a look at those? |
@TheRealAmazonKendra I analyzed the test failures. The test are not executed correctly because there is an issue with the feature flag logic in CDK v2. I opened issue #20890. I will check this PR again after the issue is resolved. |
Ah, the feature flag issue strikes again. I'm actually working on this issue. In the meantime, the tests can still be done by manually adding the feature flag to the app in the tests instead of using these testing functions. If you feel any urgency about getting this merged, that would be my suggestion as a path forward. Otherwise, I'll update when we have a path forward for the feature flag fix. |
Just in case you don't want to wait on the feature flag changes, here is a model for testing with and without the feature flag enabled https://github.com/aws/aws-cdk/pull/21021/files. |
This PR has been in the BUILD FAILING state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
@TheRealAmazonKendra I added the suggested fix for the current feature flag issue. |
|
||
Since AWS has changed the [ARN format for ECS](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-account-settings.html#ecs-resource-ids), | ||
feature flag `@aws-cdk/aws-ecs:arnFormatIncludesClusterName` must be enabled to use the new ARN format. | ||
The feature flag changes behavior for the entire CDK project. Therefore it is not possible to mix the old and the new format in one CDK project. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not strictly true, but we generally recommend to not do this inline and have zero (I think?) instructions on how to do it so I don't think I mind this note here.
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). |
Thanks for all your effort on this and for bearing with us on the feature flag issues! Glad we could get this one merged! |
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). |
AWS has changed the [ARN format for ECS](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-account-settings.html#ecs-resource-ids). Currently, CDK doesn't return the correct values/ARNs if the new ARN format is used in ECS. Changed methods: - `Ec2Service.fromEc2ServiceAttributes()` - `Ec2Service.fromEc2ServiceArn()` - `FargateService.fromFargateServiceAttributes()` - `FargateService.fromFargateServiceArn()` The logic automatically detects whether the old or new format is used. The format cannot be recognized automatically for tokenized values. Therefore the feature flag `ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME` is introduced, which controls whether the old or the new format should be used. In `Ec2Service.fromEc2ServiceAttributes()` and `FargateService.fromFargateServiceAttributes()` an ARN is created. In these methods the feature flag be considered to construct the ARN in the correct format. Closes aws#16634. Closes aws#18137. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* cr: https://code.amazon.com/reviews/CR-73367009
AWS has changed the [ARN format for ECS](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/ecs-account-settings.html#ecs-resource-ids). Currently, CDK doesn't return the correct values/ARNs if the new ARN format is used in ECS. Changed methods: - `Ec2Service.fromEc2ServiceAttributes()` - `Ec2Service.fromEc2ServiceArn()` - `FargateService.fromFargateServiceAttributes()` - `FargateService.fromFargateServiceArn()` The logic automatically detects whether the old or new format is used. The format cannot be recognized automatically for tokenized values. Therefore the feature flag `ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME` is introduced, which controls whether the old or the new format should be used. In `Ec2Service.fromEc2ServiceAttributes()` and `FargateService.fromFargateServiceAttributes()` an ARN is created. In these methods the feature flag be considered to construct the ARN in the correct format. Closes aws#16634. Closes aws#18137. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
AWS has changed the ARN format for ECS. Currently, CDK doesn't return the correct values/ARNs if the new ARN format is used in ECS.
Changed methods:
Ec2Service.fromEc2ServiceAttributes()
Ec2Service.fromEc2ServiceArn()
FargateService.fromFargateServiceAttributes()
FargateService.fromFargateServiceArn()
The logic automatically detects whether the old or new format is used. The format cannot be recognized automatically for tokenized values. Therefore the feature flag
ECS_ARN_FORMAT_INCLUDES_CLUSTER_NAME
is introduced, which controls whether the old or the new format should be used.In
Ec2Service.fromEc2ServiceAttributes()
andFargateService.fromFargateServiceAttributes()
an ARN is created. In these methods the feature flag be considered to construct the ARN in the correct format.Closes #16634.
Closes #18137.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license