_bind() method for IIpv6Addresses

[scanlonp]

Currently, IIpv6Addresses.allocateVpcIpv6Cidr() takes arguments to pass the scope of the VPC construct and the vpcId. These are needed to create a CfnVPCCidrBlock. However, passing this context in separately through a _bind() method allows allocateVpcIpv6Cidr() to need no arguments, making its implementation flexible and extensible for each class that implements IIpv6Addresses.

Still a couple design decisions I would like consulting on:

• bind() or _bind()
  • along with generally needing the _ in names
• should bind() take the context variables as input, or should the context variables be non-constant and set
  • or bind() could call a private constructor where the context variables are directly set. This would avoid optional class variables and undefined checks

Tested the integ tests locally, and they succeed.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[rix0rrr]

This property is not necessary. It is always true.

[rix0rrr]

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 });
      }
    };
  }  
}

[rix0rrr]

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

[rix0rrr]

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.

[scanlonp]

Thanks for your comments Rico. Good to hear your reasoning about the pitfalls of bind / stateful objects. If allocateVpcIpv6Cidr can take arguments, this solution has little purpose.

We decided to not go with this approach.