(aws_ec2): Dummy Security Group id has changed

[mickael-caro-sonarsource]

Describe the bug

With the new 2.55.0 version, all "dummy" security group ids that are generated from synth for our tests have changed, from sg-12345 to sg-12345678, breaking all of our tests suddenly. Is it a breaking change or something introduced by mistake ?

Expected Behavior

"Type": "AWS::EC2::SecurityGroupIngress",
                "Properties": {
                    "IpProtocol": "tcp",
                    "Description": f"some description",
                    "FromPort": 443,
                    "GroupId": "sg-12345678",
                    "SourceSecurityGroupId": {
                        "Fn::GetAtt": [
                            "dummysecgroup",
                            "GroupId",
                        ]
                    },
                    "ToPort": 443,

Current Behavior

GroupId here has changed. (same for DestinationGroupId in SecurityGroupEgress

"Type": "AWS::EC2::SecurityGroupIngress",
                "Properties": {
                    "IpProtocol": "tcp",
                    "Description": f"some description",
                    "FromPort": 443,
                    "GroupId": "sg-12345",
                    "SourceSecurityGroupId": {
                        "Fn::GetAtt": [
                            "dummysecgroup",
                            "GroupId",
                        ]
                    },
                    "ToPort": 443,

Reproduction Steps

Create a SG egress rule in Python :

    def _create_security_group(self):
            security_group.add_egress_rule(
                peer=alb_sg,
                connection=aws_ec2.Port(
                    protocol=aws_ec2.Protocol.TCP,
                    from_port=443,
                    to_port=443,
                   string_representation="Allow connection on default HTTPS port",
            ),
                description="Enables communication on HTTPS port.",
        )

Add a snapshot testing :

def test_validate_security_group_egress(
   template: assertions.Template
) -> None:
    template.has_resource(
        "AWS::EC2::SecurityGroupEgress",
        assertions.Match.object_equals(
            {
                "Type": "AWS::EC2::SecurityGroupEgress",
                "Properties": {
                    "GroupId": {"Fn::GetAtt": ["secu_group", "GroupId"]},
                    "IpProtocol": "tcp",
                    "Description": "Enables communication on HTTPS port.",
                    "DestinationSecurityGroupId": "sg-12345",
                    "FromPort": 443,
                    "ToPort": 443,
                },
            }
        ),
    )

DestinationSecurityGroupId was sg-12345 until 2.54.0 whereas it's not sg-12345678.

Possible Solution

No response

Additional Information/Context

No response

CDK CLI Version

2.55.0

Framework Version

No response

Node.js Version

LTS

OS

MacOS 13

Language

Python

Language Version

3.9.13

Other information

No response

[peterwoodworth]

Apologies for this breaking your test @mickael-caro-sonarsource,

This was a deliberate change to avoid a specific issue, see the PR for more information #22859

CC @mrgrain

[rix0rrr]

Yep, this is by design, and we will not change this. Thanks for letting us know that this happened though, it's a good reminder that every part of our API that people should not be relying on, they do in practice anyway.

While we're at it: I strongly recommend against snapshot tests at every occasion for exactly this reason. They always end up testing more than you intended, and therefore end up breaking for unrelated reasons. I'm pretty sure the functional part of your test didn't really care about the security group ID, but you were forced to put sg-12345 in there anyway because of the nature of a snapshot test -- and now your test fails for reasons outside your control.

Snapshot tests sound like a great idea in theory but in practice require constant human attention, and cause more problems than they solve (in my opinion, that is, I'm sure reasonable people may disagree).

[github-actions]

⚠️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.

[olivier-schmitt-sonarsource]

There was also another breaking change with this release:

The way the list of subnets ids is generated in the SSM parameter resource has also changed and it broke many tests.

@rix0rrr I understand your point but how would we know given the huge scope of the CDK and all the possible use cases that X is not supposed to be tested and Y should?

To me, it's not a big deal if my tests fail: it's a way to understand changes that happened on the resources I'm using.

It might be nothing or something bigger and I prefer to fail fast and be alerted than to have the impression that the constructs I'm using have not been touched by a release, and then discovering bigger problems later (like a change that looks minor but trigger a resource replacement in Prod).