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

(package/tools): JSII_DEPRECATED=fail has no impact on deprecated ec2.SubnetType.ISOLATED #22066

Closed
raginjason opened this issue Sep 15, 2022 · 4 comments
Assignees
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. p1

Comments

@raginjason
Copy link

Describe the bug

It appears that JSII_DEPRECATED does not impact the CLIs behavior with certain deprecations. Per https://github.com/aws/aws-cdk/blob/v1.172.0/packages/@aws-cdk/aws-ec2/lib/vpc.ts#L166 and https://github.com/aws/aws-cdk/blob/v1-main/DEPRECATED_APIs.md, it appears that ec2.SubnetType.ISOLATED is deprecated but the CLI does not report this. It is not clear if this is a documentation error or an actual bug in the CLI.

Expected Behavior

JSII_DEPRECATED=fail cdk synth -q --strict to exit with a non-zero status when ec2.SubnetType.ISOLATED is used

Current Behavior

JSII_DEPRECATED=fail cdk synth -q --strict exits with a zero status when ec2.SubnetType.ISOLATED is used

Reproduction Steps

Running JSII_DEPRECATED=fail cdk synth -q --strict; echo $? with the following app.py exits with a status of 0 when it should be non-zero:

from aws_cdk import aws_ec2 as ec2
from aws_cdk.core import Construct, App, Stack


class TestStack(Stack):

    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        # VPC
        ec2.Vpc(
            self,
            "VPC",
            subnet_configuration=[
                ec2.SubnetConfiguration(name="isolated", subnet_type=ec2.SubnetType.ISOLATED)
            ]
        )


app = App()
TestStack(app, "Test")

app.synth()

Note that the following app.py does exit with a non-zero status which is what I would expect:

from aws_cdk import aws_ec2 as ec2
from aws_cdk import aws_ecs as ecs
from aws_cdk.core import Construct, App, Stack


class TestStack(Stack):

    def __init__(self, scope: Construct, construct_id: str, **kwargs) -> None:
        super().__init__(scope, construct_id, **kwargs)

        # VPC
        ec2.Vpc(
            self,
            "VPC",
            subnet_configuration=[
                ec2.SubnetConfiguration(name="isolated", subnet_type=ec2.SubnetType.ISOLATED)
            ]
        )
        image = ecs.ContainerImage.from_registry(
            "abcd.dkr.ecr.us-east-1.amazonaws.com/repo:latest"
        )
        task = ecs.FargateTaskDefinition(
            self,
            "Task",
        )

        task.add_container(
            "Container",
            image=image,
        )


app = App()
TestStack(app, "Test")

app.synth()

This generates an error message of:

[Warning at /Test/Task/Container] Proper policies need to be attached before pulling from ECR repository, or use 'fromEcrRepository'.

Possible Solution

No response

Additional Information/Context

This was discovered in preparation for migrating to CDKv2, so there may be other cases besides ec2.SubnetType.ISOLATED where this issue exists.

CDK CLI Version

1.172.0 (build f091202)

Framework Version

No response

Node.js Version

v14.17.6

OS

macOS Monterey 12.5.1

Language

Python

Language Version

Python 3.9.6

Other information

#17742 talks about JSII_DEPRECATED a bit, but does not seem to help here.

@raginjason raginjason added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Sep 15, 2022
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Sep 15, 2022
@corymhall
Copy link
Contributor

It looks like we are not correctly handling cases where we deprecate properties that exist in a list of intefaces.

The function in .warnings.jsii.js

function _aws_cdk_aws_ec2_SubnetConfiguration(p) {
    if (p == null)
        return;
    visitedObjects.add(p);
    try {
        if (!visitedObjects.has(p.subnetType))
            _aws_cdk_aws_ec2_SubnetType(p.subnetType);
    }
    finally {
        visitedObjects.delete(p);
    }
}

Should be something like

function _aws_cdk_aws_ec2_SubnetConfiguration(p) {
    if (p == null)
        return;
    visitedObjects.add(p);
    try {
        p.forEach(config => {
          if (!visitedObjects.has(config.subnetType))
              _aws_cdk_aws_ec2_SubnetType(config.subnetType);
        });
    }
    finally {
        visitedObjects.delete(p);
    }

cc @RomainMuller

@RomainMuller
Copy link
Contributor

Definitely a code-gen bug. I have a fix ready for review on aws/jsii. Note that this issue would also affect values nested under maps (Record<string, Foo>, a.k.a: { [key: string]: Foo }).

@corymhall
Copy link
Contributor

@raginjason this has been fixed and released in the latest version of CDK. Let us know if you still see any issues.

@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. p1
Projects
None yet
Development

No branches or pull requests

3 participants