Make VPN-only subnets private

[lblackstone]

The previous logic did not properly detect VPN-only subnets,
and erroneously assigned them to the public group. This fix
properly detects if the CIDR block is in a private range, and
adds that subnet to the private list.

Fixes #151

[metral]

Thanks for the PR. Some notes:

• The netmask package has not been updated in 3 years, so this worries me a bit - though we're not doing anything complex with it, theoretically it could be fine for this use case.
• Is detecting if the route is in a private CIDR range the only qualifier we can use here? cc @lukehoban @pgavlin
• Can we add any supporting tests for this?

[lblackstone]

• Just took another look at the netmask package, and it's currently in use by ~170k repos. Considering that we're just using it for some fairly simple network address calculations, I think it's acceptable.
• I didn't see another way to detect a VPN-only subnet from the official docs.
• Yes; any suggestions on what kind of test coverage you'd like to see here? (integration, unit, etc)

[metral]

I didn't see another way to detect a VPN-only subnet from the official docs.

Agreed, based on docs I've seen I don't see what else we leverage to detect this, but I don't know enough about vpc personal gateways to gleam if only detecting private CIDR's is enough. Hoping Luke or Pat can add some more insight.

any suggestions on what kind of test coverage you'd like to see here? (integration, unit, etc)

Yes - an integration test in examples/test would be great - use any existing one in there as a basis. Note, given that this is an a VPN based issue, we're going to need a test that has a bastion host in the same VPC to hit the apiserver, as well as to issue pulumi updates from the subnets since its private subnets + VPN. This could be involved but I don't see any other way to test it - fwiw, we're up against the same needs in #154 (comment).

[lukehoban]

Slightly separate from this immediate PR - I do wonder if we can/should get rid of all this logic that tries to "guess" the subset of the subnets to use. It seems we should just ask users to tell us. And in general, users already do today either pass vpc.privateSubents or vpc.publicSubnets depending on what they are trying to do. From the new awsx.ec2.Vpc we don't even expose an "all subnets" option, so it's rare to even end up passing a mixed set of subnets here which would need subsetting.

I really can't recall why we added all this complexity. I think it was because the old awsinfra.Network returned a .subnets property with all the subnets, and we wanted to support that here? I'd be inclined to try to deprecate this and have users be more explicit (which I believe in practice they already are).

[pgavlin]

I really can't recall why we added all this complexity. I think it was because the old awsinfra.Network returned a .subnets property with all the subnets, and we wanted to support that here? I'd be inclined to try to deprecate this and have users be more explicit (which I believe in practice they already are).

Yes, this was essentially a consequence of the awsinfra.Network API. Is there any way we can do a quick poll to find out how many folks are passing on a mixed set of subnets here?

[metral]

@lblackstone any updates by chance per Luke's comments?

[lblackstone]

@metral No, I was partway through figuring out if there was a way to remove this logic and then got sidetracked.

Do we want to go ahead and merge this change, and then figure out how to simplify the process separately?

[metral]

Do we want to go ahead and merge this change, and then figure out how to simplify the process separately?

If this will be short-lived anyways, then we should probably just skip on this PR, and follow Luke's suggestion to be reliant on the user deliberately specify which subnet they want to run in, versus us guessing as the logic is currently doing.

[lblackstone]

@lukehoban WDYT about deprecating subnetIds

pulumi-eks/nodejs/eks/cluster.ts

Lines 336 to 342 in 19c8dfb

	/**
	* The subnets to attach to the EKS cluster. If either vpcId or subnetIds is unset, the cluster will use the
	* default VPC's subnets. If the list of subnets includes both public and private subnets, the Kubernetes API
	* server and the worker nodes will only be attached to the private subnets. See
	* https://docs.aws.amazon.com/eks/latest/userguide/network_reqs.html for more details.
	*/
	subnetIds?: pulumi.Input<pulumi.Input<string>[]>;

and creating two new variables, publicSubnetIds and privateSubnetIds?

[lukehoban]

I actually do think we should take this fix as is.

And then open a new issue to track making a breaking change to simplify this. There is some subtlety to that discussion - so should probably have it separately of this tactical issue.

[lblackstone]

Will do; thanks for clarifying.

[metral]

LGTM