aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.5k stars 3.84k forks source link

(ec2): Fails to create subnet with `fromSubnetAttributes` without `routeTableId` #19786

Open nomike opened 2 years ago

nomike commented 2 years ago

Describe the bug

When I create a Subnet using ec2.Subnet.from_subnet_attributes and don't specify a routeTableId, I'm getting the following warning:

[Warning at /euc1-dv-rest-alb-alb/subnet-0] No routeTableId was provided to the subnet at 'euc1-dv-rest-alb-alb/subnet-0'. Attempting to read its .routeTable.routeTableId will return null/undefined. (More info: https://github.com/aws/aws-cdk/pull/3171)

While this warning definitely serves it's purpose, I'm missing the option to turn it off or acknowledge it when for example I anyway don't have the intention to read the routeTableId of the subnet.

It is just spamming the console when running synth but also prevents me from using --strict.

Expected Behavior

This warning shouldn't be there. at all.

Current Behavior

cdk synth produces said warning, regardless of whether you're actually trying to read the routeTableId parameter or not.

Reproduction Steps

Just do this in one of your stacks.

ec2.Subnet.from_subnet_attributes(self, id="subnet", subnet_id='subnet-0123456789abcdef0')

Possible Solution

Either the routeTableId Parameter should be mandatory for ec2.Subnet.from_subnet_attributes or if it stays optional (which is probably the better solution) it should not automatically trigger a warning. Instead synth should fail if you omit the routeTableId parameter and try to read it later. Maybe an error message with similar content could be created if this happens.

Additional Information/Context

The line of code where this happens is https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-ec2/lib/vpc.ts#L2103.

CDK CLI Version

2.19.0 (build e0d3e62)

Framework Version

No response

Node.js Version

v14.19.0

OS

Ubuntu 20.04.3 LTS, Linux VIE-NOMPOS-MOB 5.10.60.1-microsoft-standard-WSL2 #1 SMP Wed Aug 25 23:20:18 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux

Language

Typescript, Python, .NET, Java, Go

Language Version

not relevant

Other information

No response

NGL321 commented 2 years ago

Hey @nomike,

Thanks for reporting this. I have converted to a feature-request for now since it is intended behavior, but I wonder if this brings up a more large-scale question about disabling warning conditionally (or only enabling them on conditions). This would likely be a larger-scale endeavor though.

@rix0rrr I will pass it off to you since I am uncertain about what this would entail.

rix0rrr commented 2 years ago

Only adding the warning when routeTableId is read sounds good. Happy to accept a PR that changes this behavior.

jordan-brough commented 2 years ago

This happens with ec2.Subnet.fromSubnetId as well, which doesn't even have an option to specify a routeTableId.

My current terrible workaround to avoid warning spam:

const subnet = cdk.aws_ec2.Subnet.fromSubnetId(this, 'Subnet', 'subnet-XXX');
// hack to avoid warning spam
subnet.node._metadata = subnet.node._metadata.filter((item) =>
  item.type !== 'aws:cdk:warning' || !/No routeTableId was provided to the subnet/.test(item.data)
);

It would be great to have the warning (or an exception) only when routeTableId is read.

nomike commented 2 years ago

I don't see the point in this waning at all. If i don't specify the routeTableID it will be None when I read it. So be it. I can check the variable contents and see that it is None. I don't need a warning to tell me. Not 500 lines of code earlier, not when I actually try to read the variable.

Sent from MailDroid

-----Original Message----- From: Jordan Brough @.> To: aws/aws-cdk @.> Cc: nomike @.>, Mention @.> Sent: Tue, 14 Jun 2022 20:11 Subject: Re: [aws/aws-cdk] (ec2): Fails to create subnet with fromSubnetAttributes without routeTableId (Issue #19786)

This happens with ec2.Subnet.fromSubnetId as well, which doesn't even have an option to specify a routeTableId.

My current terrible workaround to avoid warning spam:

const subnet = cdk.aws_ec2.Subnet.fromSubnetId(this, 'Subnet', 'subnet-XXX');
// hack to avoid warning spam
subnet.node._metadata = subnet.node._metadata.filter((item) =>
  item.type !== 'aws:cdk:warning' || !/No routeTableId was provided to the subnet/.test(item.data)
);

It would be great to have the warning (or an exception) when routeTableId is read.

-- Reply to this email directly or view it on GitHub: https://github.com/aws/aws-cdk/issues/19786#issuecomment-1155530398 You are receiving this because you were mentioned.

Message ID: @.***>

DesAWSume commented 2 years ago

This happens with ec2.Subnet.fromSubnetId as well, which doesn't even have an option to specify a routeTableId.

My current terrible workaround to avoid warning spam:

const subnet = cdk.aws_ec2.Subnet.fromSubnetId(this, 'Subnet', 'subnet-XXX');
// hack to avoid warning spam
subnet.node._metadata = subnet.node._metadata.filter((item) =>
  item.type !== 'aws:cdk:warning' || !/No routeTableId was provided to the subnet/.test(item.data)
);

It would be great to have the warning (or an exception) only when routeTableId is read.

Do you have a Python way to hack this?

Doesn't seem like I can modify the metadata

peterwoodworth commented 2 years ago

I don't see the point in this waning at all

Lots of customers frequently get tripped up by their imported resources having undefined values, usually due to expecting CDK to look the values up itself.

This warning has been here for a long time, and we have other warnings similar to this - but usually only when the value would be read. Personally I'd be perfectly ok with removing this warning altogether, because we're actively working on documentation which will reduce confusion surrounding imported resources. Once that new documentation is live we could remove this warning. In the meantime, we'd definitely still accept a PR which only throws this warning when the routeTableId is read

@DesAWSume Check out our Python examples of escape hatches

petejewels commented 2 years ago

Using dotnet , having same issue:

TaskSubnets = new SubnetSelection
                    {
                        Subnets = new[] {
                           Subnet.FromSubnetId(this, "subnet1", "subnet-64564"),
                            Subnet.FromSubnetId(this, "subnet2", "subnet-987987")
                        },

                    },

If we are given warning we should have ability to add a route table id to theFromSubnetId these subnets use....

entest-hai commented 1 year ago

Having the same issue when specify subnets from theirs ids

aws_ec2.Subnet.fromSubnetId
TakaakiFuruse commented 1 year ago

I just got the warning by simply doing this.

ec2.Subnet.from_subnet_id(
    scope=scope,
    id="my_subnet1",
    subnet_id="subnet-xxxxx",
)

Getting and ignoring any kind of warning is not good for our health, so I am very happy to create a pull request.

How should I fix it?

  1. How about suggesting developers to use fromSubnetAttributes not fromSubnetId through new documentation or new warning(or info?), since fromSubnetId is just a wrapper of fromSubnetAttributes ?

  2. Rather than sending warning, why not send info instead? We can create the resource but just get null/undefined, if no routeTableId was not supplied, so info sounds better for me.

Any ideas?

whereisaaron commented 1 year ago

we'd definitely still accept a PR which only throws this warning when the routeTableId is read

Both Subnet.FromSubnetAttributes and Subnet.FromSubnetId generate the same warning unless you supply an unnecessary route table id. I agree referencing the route table id should be the warning.

dontirun commented 11 months ago

Better "workaround" with the recent addition of addWarningsV2

    const subnet = ec2.Subnet.fromSubnetId(
      this,
      'subnet',
      'subnet-01234567890abcdef'),
    );
    cdk.Annotations.of(subnet).acknowledgeWarning('@aws-cdk/aws-ec2:noSubnetRouteTableId');
guidospadavecchia commented 7 months ago

Better "workaround" with the recent addition of addWarningsV2

    const subnet = ec2.Subnet.fromSubnetId(
      this,
      'subnet',
      'subnet-01234567890abcdef'),
    );
    cdk.Annotations.of(subnet).acknowledgeWarning('@aws-cdk/aws-ec2:noSubnetRouteTableId');

Any chance to translate this for .NET?

github-actions[bot] commented 5 months ago

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

WhiteAutumn commented 5 months ago

This issue was created from missing an option to acknowledge the warning. With the addition of addWarningsV2 @dontirun mentioned, this issue can probably be closed.