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

aws-ec2: Using IPV6 CIDR as input to Security Group IPeer results in parsing error #28213

Open
juinquok opened this issue Dec 1, 2023 · 3 comments
Labels
@aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud bug This issue is a bug. effort/medium Medium work item – several days of effort p1

Comments

@juinquok
Copy link
Contributor

juinquok commented Dec 1, 2023

Describe the bug

Using the aws-ec2.SecurityGroup's addIngressRule method to add a rule allowing an IPV6 CIDR block results in a Token error if the input for the aws-ec2.Peer.ipv6() function is a IPV6 CIDR block derived from aws_ec2.CfnVPCCidrBlock. The error I received is Error: Invalid IPv6 CIDR: "#{Token[TOKEN.53]}"

Expected Behavior

Since aws_ec2.CfnVPCCidrBlock only gets the CIDR block at deploy time as its AWS-assigned, we should allow the address range here to be interoperable with existing Security Groups. Otherwise, we will not be able to add ingress rules in code when we use IPV6.

Current Behavior

A token error is thrown when trying to synth the stack as the Peer.ipv6() is expecting a string but a token is provided in this case.

Reproduction Steps

const vpc = aws_ec2.vpc(......)
const ipv6CfnCidrBlock = new aws_ec2.CfnVPCCidrBlock(this, 'Ipv6CfnCidrBlock', {
      vpcId: vpc.vpcId,
      amazonProvidedIpv6CidrBlock: true,
    });

// Extract ipv6 cidr block from cfn output
const ipv6CidrBlock = Fn.select(0, this.vpcIpv6CidrBlocks);

// Create 256 /64 subnets from the /56 cidr block
const ipv6SubnetCidrBlocks = Fn.cidr(ipv6CidrBlock, 256, '64');

// Create SG
const sg = new ec2.SecurityGroup(this, 'sg', {
      vpc: vpc
      allowAllOutbound: true,
      allowAllIpv6Outbound: true,
    });

// Grant entire VPC access to the Port 80 of this application
vpc.vpcIpv6CidrBlocks.forEach((ipv6CidrBlock) => {
      this.albSecurityGroup.addIngressRule(
        ec2.Peer.ipv6(ipv6CidrBlock),
        ec2.Port.tcp(80),
      );
    });

Possible Solution

Ideally, Peer needs to implement IResolvable so that it can be parsed at runtime
Edit: See last comment below for the possible workarounds

Additional Information/Context

Related to #894

CDK CLI Version

2.103.1 (build 3bb19ac)

Framework Version

No response

Node.js Version

v18.16.0

OS

Mac OS 13.6

Language

TypeScript

Language Version

No response

Other information

No response

@juinquok juinquok added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Dec 1, 2023
@github-actions github-actions bot added the @aws-cdk/aws-ec2 Related to Amazon Elastic Compute Cloud label Dec 1, 2023
@pahud
Copy link
Contributor

pahud commented Dec 5, 2023

Yes this could relate to #894. I am making it p1.

@pahud pahud added p1 needs-review effort/medium Medium work item – several days of effort and removed needs-triage This issue or PR still needs to be triaged. labels Dec 5, 2023
@juinquok
Copy link
Contributor Author

juinquok commented Dec 6, 2023

Found a temporary workaround for anyone who might want to implement this in the time being whilst this change is being implemented:

for (let i = 0; i < context.vpc.vpcIpv6CidrBlocks.length; i += 1) {
      this.albSecurityGroup.addIngressRule(
        ec2.Peer.ipv6(Fn.select(i, context.vpc.vpcIpv6CidrBlocks)),
        ec2.Port.tcp(accessPort),
        'Allow inbound traffic from VPC IPV6 CIDR block' ,
      );
    }

@pahud pahud removed the needs-review label Dec 6, 2023
@juinquok
Copy link
Contributor Author

Took a deeper look into this issue as we are trying to fully implement ipv6 across our entire stack.

Using the following code:

vpc.vpcIpv6CidrBlocks.forEach((ipv6CidrBlock) => {
      this.albSecurityGroup.addIngressRule(
        ec2.Peer.ipv6(ipv6CidrBlock),
        ec2.Port.tcp(80),
      );
    });

doesn't work because of the code here is doing a lookup of the Token.

This should work in theory because the type of vpcIpv6CidrBlocks is string[] but this is actually a tad misleading. At synth time, this doesn't actually give you an array of IPv6 CIDRs which you might be expecting from the type. It is actually an array of ListToken.

Here's what happens when its parsed:

// Peer.ts calls this function to check if the token is valid
export function unresolved(obj: any): boolean {
  if (typeof(obj) === 'string') {
    return TokenString.forString(obj).test();
  } else if (typeof obj === 'number') {
    return extractTokenDouble(obj) !== undefined;
  } else if (Array.isArray(obj) && obj.length === 1) {
    return typeof(obj[0]) === 'string' && TokenString.forListToken(obj[0]).test();
  } else {
    return isResolvableObject(obj);
  }
}

Since the typeof cidrIpv6 in Peer is string, it will create a TokenString.forString(cidrIpv6) which will then be evaluate as a StringToken instead of a list token. In TokenString.test() method, it will then do a regex check using a StringToken regex instead of a ListToken regex which causes the error to be thrown. This can be seen in the output for the error as it will say something like Error: Invalid IPv6 CIDR: "#{Token[TOKEN.18]}" at new CidrIPv6 where #{Token[TOKEN.18]} is a ListToken but ${Token[TOKEN.18]} would be a StringToken.

Given this, I guess there is 2 ways which cdk can help make this easier for devs looking to implement this.

  1. CDK can add more information to the error Invalid IPv6 CIDR: such as suggesting to the dev to try using Fn.select if they are using the VPC's properties to add the IPV6 address to the ingress rule

  2. Change the way which vpcIpv6CidrBlocks in aws_ec2.Vpc provides the reference to the IPv6 CIDR block. I'm not entirely sure if this possible but when assigning it to the L2 construct in vpc.ts. This can probably be implemented using the Fn.select method (since this seems to be the only way to manipulate ListTokens) to iterate through the list of ListTokens by index before storing them into the array.

this.vpcIpv6CidrBlocks = this.resource.attrIpv6CidrBlocks.map((block, index, attrIpv6CidrBlocks)=> {
      return Fn.select(index, attrIpv6CidrBlocks);
    });

Not sure which would be more ideal...

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. effort/medium Medium work item – several days of effort p1
Projects
None yet
Development

No branches or pull requests

2 participants