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.

Sign up for GitHub

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

_bind() method for IIpv6Addresses #8

Closed
wants to merge 4 commits into from
Closed

_bind() method for IIpv6Addresses #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ca047ed
add bind mehtod to IIpv6Address
scanlonp Jan 11, 2024
08c797d
change bind to _bind
scanlonp Jan 11, 2024
c2579f2
add internal tag to context interface
scanlonp Jan 11, 2024
22949d2
add internal tag
scanlonp Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 40 additions & 15 deletions packages/aws-cdk-lib/aws-ec2/lib/ip-addresses.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,18 +139,20 @@ export interface AllocateCidrRequest {
}

/**
* Request for allocation of the VPC IPv6 CIDR.
* Internal interface to get VPC context to IpAddresses.
*
* @internal
*/
export interface AllocateVpcIpv6CidrRequest {
export interface _VpcContext {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This must not be internal, otherwise external users can never substitute their own implementation of IIpv6Addresses.

/**
* The VPC construct to attach to.
* VPC construct.
*/
readonly scope: Construct;
scope: Construct;

/**
* The id of the VPC.
* Id of the VPC.
*/
readonly vpcId: string;
vpcId: string;
}

/**
Expand Down Expand Up @@ -446,18 +448,18 @@ export class Ipv6Addresses {
*/
export interface IIpv6Addresses {
/**
* Whether the IPv6 CIDR is Amazon provided or not.
* Internal function to get scope and context from VPC.
*
* Note this is specific to the IPv6 CIDR.
* @internal
*/
amazonProvided: boolean,
_bind(context: _VpcContext): void;

/**
* Called by VPC to allocate IPv6 CIDR.
*
* Note this is specific to the IPv6 CIDR.
*/
allocateVpcIpv6Cidr(input: AllocateVpcIpv6CidrRequest): CfnVPCCidrBlock;
allocateVpcIpv6Cidr(): CfnVPCCidrBlock;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exposing L1 types at the L2 layer. Contributing guidelines say we don't do that.

Don't return the CfnVPCCidrBlock. You should return as little information as possible as what the client needs, so that the contract is as narrow as possible (and therefore, as simple to implement as possible!). The client doesn't need a construct, it needs a CIDR range. For example, it needs the string "2001:0db8:/32", so you should return that.

In our case, that probably translates to returning vpcCidrBlock.attrIpv6CidrBlock.

And wrap it in a return object (interface AllocateVpcIpv6CidrResult) so that it will be easy to add more fields to it later.


/**
* Split IPv6 CIDR block up for subnets.
Expand All @@ -483,22 +485,45 @@ class AmazonProvided implements IIpv6Addresses {
/**
* Whether the IPv6 CIDR is Amazon provided or not.
*/
amazonProvided: boolean;
private readonly amazonProvided: boolean;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is not necessary. It is always true.


/**
* Internal VPC construct.
*/
private scope?: Construct;

/**
* Internal id of the VPC.
*/
private vpcId?: string;

constructor() {
this.amazonProvided = true;
}

/**
* Internal function to get scope and context from VPC.
*
* @internal
*/
_bind(context: _VpcContext): void {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is possible, but I don't think this is better than what you used to have.

Either:

  • the IpAddresses object was stateless, and you've now made it stateful
  • OR: the IpAddresses object was already stateful, and now you've just moved parameters from 1 function call into 2 function calls.

There is no rule that says allocateVpcIpv6Cidr must take no arguments; just because it happened to be true for the IPv4 case, doesn't mean it must stay true forever. In fact, it probably shouldn't have been true for the IPv4 case either.

Background

In general, I don't like the use of self-mutating bind methods, because it means it becomes dangerous to reuse the IpAddresses object between constructs:

const ips = IpAddresses.amazonProvided();

new Vpc(this, 'Vpc1', { ips: ips });
new Vpc(this, 'Vpc2', { ips: ips });  // <-- the use of this object in `Vpc1` puts state on `ips` that may interfere with its use in Vpc2

Yes, probably in this case it will work correctly because state will be set and immediately used without the opportunity for caller code to do something else that would inappropriately reuse that state. But you can imagine a scenario in which users would do the following, and we would reuse the state, and it would break:

const ips = IpAddresses.amazonProvided();

const vpc1 = new Vpc(this, 'Vpc1', { ips: ips });
const vpc2 = new Vpc(this, 'Vpc2', { ips: ips });

vpc1.allocateOneMoreSubnet(); // Probably calls `this.ips.allocate()`; the last _bind() call was done by vpc2 so this might create objects in the wrong scope

All in all, I know we have this pattern of mutating this in bind(), but it is dangerous and I hate it.

What to do?

I would prefer one of the following:

1. Just keep the input parameter

It's great. No notes. It does prevent IpAddressses from ever becoming stateful though, OR if it does become stateful we run into the problem I described above.

2. Make bind return a state-holding object

This is a slightly more complex handshake. It separates the object that represents user intent (only holds parameters) from the object that does calculation (holds calculation state):

interface IIpv6Addresses {
  bind(context: VpcContext): IBoundIpv6Addresses;
}

interface IBoundIpv6Addresses {
  allocateVpcIpv6Cidr(): Ipv6AllocateResult;
}

The Ipv6Addresses object now doesn't hold any state relating to the VPC: we delegate that to the BoundIpv6Addresses object, which will be VPC-specific:

class Vpc {
  private ipAddresses: IBoundIpv6Addresses;
  constructor(props) {
    this.ipAddresses = props.ipAddresses.bind({ scope: this, vpcId: this.vpcId); }
  }
} 

// Note that the values from 'context' never get stored on the instance of AmazonIpv6Addresses
class AmazonIpv6Addresses implements IIpv6Addresses {
  public bind(context: VpcContext): IIpv6Addresses {
    // Return an anonymous instance of the expected interface
    return {
      allocateIpv6Addresses() {
        new CfnAmazonProvidedBlock(context.scope, 'Block', { vpcId: context.vpcId });
      }
    };
  }  
}

this.scope = context.scope;
this.vpcId = context.vpcId;
}

/**
* Called by VPC to allocate IPv6 CIDR.
*
* Creates an Amazon provided CIDR block.
*
* Note this is specific to the IPv6 CIDR.
*/
allocateVpcIpv6Cidr(input: AllocateVpcIpv6CidrRequest): CfnVPCCidrBlock {
//throw new Error(`vpcId not found, got ${(scope as any).vpcId}`);
return new CfnVPCCidrBlock(input.scope, 'ipv6cidr', {
vpcId: input.vpcId,
allocateVpcIpv6Cidr(): CfnVPCCidrBlock {
if (this.scope === undefined || this.vpcId === undefined) {
throw new Error('Context is unset when trying to allocate VPC IPv6 CIDR block');
}
return new CfnVPCCidrBlock(this.scope, 'ipv6cidr', {
vpcId: this.vpcId,
amazonProvidedIpv6CidrBlock: this.amazonProvided,
});
}
Expand Down
4 changes: 3 additions & 1 deletion packages/aws-cdk-lib/aws-ec2/lib/vpc.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1594,11 +1594,13 @@ export class Vpc extends VpcBase {
if (this.useIpv6) {
this.ipv6Addresses = props.ipv6Addresses ?? Ipv6Addresses.amazonProvided();

this.ipv6CidrBlock = this.ipv6Addresses.allocateVpcIpv6Cidr({
this.ipv6Addresses._bind({
scope: this,
vpcId: this.vpcId,
});

this.ipv6CidrBlock = this.ipv6Addresses.allocateVpcIpv6Cidr();

this.ipv6SelectedCidr = Fn.select(0, this.resource.attrIpv6CidrBlocks);
}

Expand Down
Loading