Select subnet based on az machine type availability #373
Conversation
lbajolet-hashicorp
force-pushed
the
select_subnet_based_on_az_machine_type_availability
branch
4 times, most recently
from
a6a1fb7 to
4f74fbc
Compare
I'm not completely sure, but it may require the |
lbajolet-hashicorp
force-pushed
the
select_subnet_based_on_az_machine_type_availability
branch
from
4f74fbc to
a43243e
Compare
When a configuration enables associate_public_ip_address, we pick a subnet from the default VPC, should one not be specified in the configs. However, based on the type on machine being created, we may not always pick the most available subnet out of all the possible ones exposed. This commit filters out the subnets based on the AZ they're linked to, and whether or not the AZ supports the instance type requested in the template.
The step_network_info step was only tested via acceptance tests, which considering the complexity of the code, is not enough. To remedy this, we add some unit tests on the functions added for filtering the subnets based on their AZ and the machine type, and for testing the step itself in this configuration.
When the associate_public_ip_address attribute is set in a template, without a VPC or a Subnet, we attempt to get the default VPC for the account. This may however fail if the `ec2:DescribeVpcs' permission is not set. This is a regression compared to older versions of the plugin, where the builds succeeded, and the associate_public_ip_address was silently ignored. To avoid breaking more clients, we decide to not treat errors when attempting to infer a default subnet or VPC from the account, and instead warn that the option will be ignored.
When selecting a default subnet, we describe the instance type offering of each AZ linked to the subnets available in the VPC we picked. This may fail if we don't have the permissions to do so, and in this case, rather than failing to pick one, and ignore the associate_public_ip_address, we continue without filtering by AZ capability, and end-up picking the most available subnet out of all the available ones. We also print a message so users know which permission may be missing from their IAM configs, and they can decide to fix that.
Since the DescribeInstanceTypeOfferings API call may require an extra action to be allowed in the IAM roles defined, we add an extra exerpt in the documentation for the `associate_public_ip_address' configuration flag, so that users understand the implication of not being able to perform this call.
lbajolet-hashicorp
force-pushed
the
select_subnet_based_on_az_machine_type_availability
branch
from
a43243e to
8f64933
Compare
| @@ -1283,14 +1283,10 @@ func TestAccBuilder_AssociatePublicIPWithoutSubnet(t *testing.T) { | |||
| return fmt.Errorf("did not change the public IP setting for the instance") | |||
| } | |||
|
|
|||
| if !strings.Contains(string(logs), "No VPC ID provided, Packer will choose one from the provided or default VPC") { | |||
| if !strings.Contains(string(logs), "No VPC ID provided, Packer will use the default VPC") { | |||
There was a problem hiding this comment.
nit: It might be worth adding the same acceptance test to the other builders
There was a problem hiding this comment.
Agreed it can't hurt, that being said, since the step is common, all the builders that import it will have the same behaviour.
And in terms of acceptance testing right now, besides ebs, the only builder that has the infra to run them is ebssurrogate, which essentially works the same as ebs, so I think for now we can consider this good to go.
Independently from this PR, we should add acceptance tests for the other builders at some point.
|
Had verbal confirmation that Wilken was OK with the current state of the PR, so I'm merging this as-is. |
lbajolet-hashicorp
deleted the
select_subnet_based_on_az_machine_type_availability
branch
This PR is an attempt to fix the problem where we pick a subnet that cannot spawn an instance of the type requested as the AZ linked to the subnet does not support it.
We also add some basic unit-tests for the step, which will be improved upon in upcoming PRs.
Related to #368
Related to #371