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

(ElasticLoadBalancingV2): Security group missing when specifying Connections rules #31644

Open
1 task done
adam-clauss opened this issue Oct 3, 2024 · 5 comments · May be fixed by #31704
Open
1 task done

(ElasticLoadBalancingV2): Security group missing when specifying Connections rules #31644

adam-clauss opened this issue Oct 3, 2024 · 5 comments · May be fixed by #31704
Assignees
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. p3 potential-regression Marking this issue as a potential regression to be checked by team member

Comments

@adam-clauss
Copy link

adam-clauss commented Oct 3, 2024

Describe the bug

A network load balancer with its connectivity specified using Connections does not get a security group specified in the synthesized yml.

Regression Issue

  • Select this option if this issue appears to be a regression.

Last Known Working CDK Version

Unknown - I assume it worked on some point since the behavior was explicitly added in #28494.

Expected Behavior

Expected a security group to be defined and applied to the NLB allowing the specified traffic.

Current Behavior

The load balancer gets created, but without a security group.

Reproduction Steps

public class NlbStack : Stack
{
    internal NlbStack(Construct scope, string id, IStackProps props = null) : base(scope, id, props)
    {
        //CreateWithSecurityGroup();
        CreateWithConnections();
    }

    private void CreateWithSecurityGroup()
    {
        IVpc vpc = Vpc.FromLookup(this, "VPC", new VpcLookupOptions { IsDefault = true });

        ISecurityGroup sg = new SecurityGroup(this, "sg", new SecurityGroupProps
        {
            Vpc = vpc
        });
        sg.AddIngressRule(Peer.AnyIpv4(), Port.HTTPS);

        NetworkLoadBalancer nlb = new NetworkLoadBalancer(this, "nlb", new NetworkLoadBalancerProps
        {
            Vpc = vpc,
            InternetFacing = true,
            IpAddressType = IpAddressType.IPV4,
            VpcSubnets = new SubnetSelection { SubnetType = SubnetType.PUBLIC, OnePerAz = true },
            SecurityGroups = new[] { sg }
        });
    }

    private void CreateWithConnections()
    {
        IVpc vpc = Vpc.FromLookup(this, "VPC", new VpcLookupOptions { IsDefault = true });

        NetworkLoadBalancer nlb = new NetworkLoadBalancer(this, "nlb", new NetworkLoadBalancerProps
        {
            Vpc = vpc,
            InternetFacing = true,
            IpAddressType = IpAddressType.IPV4,
            VpcSubnets = new SubnetSelection { SubnetType = SubnetType.PUBLIC, OnePerAz = true },
        });

        nlb.Connections.AllowFromAnyIpv4(Port.HTTPS);
        nlb.Connections.AllowToAnyIpv4(Port.AllTraffic());
    }
}

Possible Solution

No response

Additional Information/Context

As per the sample code, two ways to create a load balancer. If a SecurityGroup is explicitly specified, the resulting network load balanacer has a security group created for it and it is fine.

If instead the Connections property is used to define the allowed connectivity, this appears to be completely omitted from the resulting YML.

In our case, we initially had a stack defined using explicit security groups, but are attempting to convert to using the Connections property as that appears to be the recommended best practice. Attempting to deploy our stack with these changes fails on the NLB resource with the error:
Resource handler returned message: "1 validation error detected: Value null at 'securityGroups' failed to satisfy constraint: Member must not be null (Service: ElasticLoadBalancingV2, Status Code: 400, Request ID: 4582c208-2aa9-44e3-b606-21a00ef2f0a4)" (RequestToken: 67f14885-5b87-0c95-7e53-5687c9b9cf5c, HandlerErrorCode: InvalidRequest)

We then tracked down that this was occurring because Connections did not create a security group, which meant this deploy was attempting to remove the only existing SG for the NLB, which is apparently not allowed.

CDK CLI Version

2.160.0 (build 7a8ae02)

Framework Version

No response

Node.js Version

20.17.0

OS

Windows 11

Language

.NET

Language Version

.NET 6.0

Other information

YML for the sample code when using Connections:

Resources:
  nlbC39469D4:
    Type: AWS::ElasticLoadBalancingV2::LoadBalancer
    Properties:
      IpAddressType: ipv4
      LoadBalancerAttributes:
        - Key: deletion_protection.enabled
          Value: "false"
      Scheme: internet-facing
      Subnets:
        - subnet-08d9c3055d5b61288
        - subnet-000fa1a98d78d8527
        - subnet-071c4e1ea6385b60e
        - subnet-0e61c25b31ec03fca
        - subnet-0a1b16563016fd90a
        - subnet-031852040ec63f0d8
      Type: network
    Metadata:
      aws:cdk:path: NlbStack/nlb/Resource
  CDKMetadata:
    Type: AWS::CDK::Metadata
    Properties:
      Analytics: v2:deflate64:H4sIAAAAAAAA/02MsQ6CMBRFv8W9fRRI2BwU3QwDGlfzKE9TqX2kLTAQ/t2gMTF3ODlnuBmkhQK1wSlI3XbSmgbmc0TdiZoCD16TwCncyGKIRlvGtkGLThv3GDOYK4oT++7E2O4/nbwo7+7flzX8zhZx4FhRTApQkOcJVMdLyZ52fS+v5INhtx1zSJMU1LrNMxgj/eCieRHUX74BXuPQ5LUAAAA=
    Metadata:
      aws:cdk:path: NlbStack/CDKMetadata/Default
Parameters:
  BootstrapVersion:
    Type: AWS::SSM::Parameter::Value<String>
    Default: /cdk-bootstrap/hnb659fds/version
    Description: Version of the CDK Bootstrap resources in this environment, automatically retrieved from SSM Parameter Store. [cdk:skip]
@adam-clauss adam-clauss added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Oct 3, 2024
@github-actions github-actions bot added @aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 potential-regression Marking this issue as a potential regression to be checked by team member labels Oct 3, 2024
@ashishdhingra
Copy link
Contributor

ashishdhingra commented Oct 3, 2024

Per Connections, it allows manage the allowed network connections for constructs with Security Groups. This class makes it easy to allow network connections to and from security groups, and between security groups individually. When used by itself, one needs to specify at least one security group per example:

// The code below shows an example of how to instantiate this type.
// The values are placeholders you should change.
import { aws_ec2 as ec2 } from 'aws-cdk-lib';

declare const peer: ec2.IPeer;
declare const port: ec2.Port;
declare const securityGroup: ec2.SecurityGroup;
const connections = new ec2.Connections(/* all optional props */ {
  defaultPort: port,
  peer: peer,
  securityGroups: [securityGroup],
});

When used as convenience as property connections as part of NLB, it provides an interface to the security groups associated with NLB (refer here). The tests in associated PR 28494 demonstrate this behavior.

Essentially, you could create a NLB without defining security groups using code below:

const vpc = ec2.Vpc.fromLookup(this, 'DefaultVpc', { isDefault: true });
    
new elbv2.NetworkLoadBalancer(this, 'TestNLB', {
  vpc
}); 

So to use EC2 Connections, you need to specify at least one security group to act on via Connections convenience class.

I'm unsure if this issue is a regression, since per commit history,connections property would not create security group if not specified.

Thanks,
Ashish

@ashishdhingra ashishdhingra removed the needs-triage This issue or PR still needs to be triaged. label Oct 3, 2024
@ashishdhingra ashishdhingra self-assigned this Oct 3, 2024
@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. p3 labels Oct 3, 2024
@adam-clauss
Copy link
Author

If true, that only appears to be true for NLB?

For example, this code to create an instance DOES create a security group automatically:
var instance = new Instance_(this, "instance", new InstanceProps
{
Vpc = vpc,
InstanceType = InstanceType.Of(InstanceClass.M7I_FLEX, InstanceSize.XLARGE2),
MachineImage = MachineImage.LatestAmazonLinux2(),
VpcSubnets = new SubnetSelection { SubnetType = SubnetType.PUBLIC, OnePerAz = true },
});

instance.Connections.AllowFromAnyIpv4(Port.HTTPS);
instance.Connections.AllowToAnyIpv4(Port.AllTraffic());

I've also tested similar Client VPN Endpoints, ECS Services, even an ALB instead of NLB. All created a security group without a SecurityGroup being constructed in our CDK code.

@pahud
Copy link
Contributor

pahud commented Oct 4, 2024

The reason an Instance has its connection with security group is because it "place" a security group in its "Connection" container:

this.connections = new Connections({ securityGroups: [this.securityGroup] });

while in NLB if you leave props.securityGroups undefined it won't add any security group in that Connection.

this.connections = new ec2.Connections({ securityGroups: props.securityGroups });

@ashishdhingra
Copy link
Contributor

@pahud Thanks for the follow up explanation.

@ashishdhingra ashishdhingra added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Oct 4, 2024
@adam-clauss
Copy link
Author

Yes, thanks for tracking that @pahud . I'll try explicitly adding an (empty?) security group in the meantime, but hopefully finding that should help resolve the bug faster.

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/aws-elasticloadbalancingv2 Related to Amazon Elastic Load Balancing V2 bug This issue is a bug. p3 potential-regression Marking this issue as a potential regression to be checked by team member
Projects
None yet
3 participants