unable to set AssociatePublicIpAddress for network interface to false

[tuananh]

In corporate context, we often have a SCP (service control policy) to block AssociatePublicIpAddress when set to true by default.

However, in machine-api-provider-aws, we have this code path

machine-api-provider-aws/pkg/actuators/machine/instances.go

Lines 364 to 389 in 318ae2f

	var networkInterfaces = []*ec2.InstanceNetworkInterfaceSpecification{
	{
	DeviceIndex: aws.Int64(machineProviderConfig.DeviceIndex),
	SubnetId: subnetID,
	Groups: securityGroupsIDs,
	},
	}
	// Public IP assignment is different in Wavelength Zones.
	// AvailabilityZone and LocalZone uses InternetGateway.
	// WavelengthZone uses Carrier Gateway.
	if aws.BoolValue(machineProviderConfig.PublicIP) {
	zoneName, err := getAvalabilityZoneFromSubnetID(*subnetID, awsClient)
	if err != nil {
	return nil, mapierrors.InvalidMachineConfiguration("error discoverying zone type: %v", err)
	}
	zoneType, err := getAvalabilityZoneTypeFromZoneName(zoneName, awsClient)
	if err != nil {
	return nil, mapierrors.InvalidMachineConfiguration("error discoverying zone type: %v", err)
	}
	if zoneType == "wavelength-zone" {
	networkInterfaces[0].AssociateCarrierIpAddress = machineProviderConfig.PublicIP
	} else {
	networkInterfaces[0].AssociatePublicIpAddress = machineProviderConfig.PublicIP
	}
	}

where as when we create new network interface, we did not explicity set AssociatePublicIpAddress. This field, when omited, AWS will use the default value which is true as documented here

Together with the if aws.BoolValue(machineProviderConfig.PublicIP) { only check, render us unable to set this value to false even if we set machineProviderConfig.PublicIP to false.

The fix for this could be

• Default value for network interface changes to AssociatePublicIpAddress to false.
• Or Add a else clause to handle false path.

What do you think?

[tuananh]

@theobarberbany any advice on how we should proceed? I can open a PR afterward.

[tuananh]

we tested with this minimal patch (default to false) and it works well for us

diff --git a/pkg/actuators/machine/instances.go b/pkg/actuators/machine/instances.go
index 69a47dc5..6211a523 100644
--- a/pkg/actuators/machine/instances.go
+++ b/pkg/actuators/machine/instances.go
@@ -365,6 +365,7 @@ func launchInstance(machine *machinev1beta1.Machine, machineProviderConfig *mach
                {
                        DeviceIndex: aws.Int64(machineProviderConfig.DeviceIndex),
                        SubnetId:    subnetID,
+                       AssociatePublicIpAddress: aws.Bool(false),
                        Groups:      securityGroupsIDs,
                },
        }

[JoelSpeed]

Perhaps you linked the wrong thing, but, following the link, the only description I can see of the AssociatePublicIPAddress field is

Associates a public IPv4 address with eth0 for a new network interface.

AWS charges for all public IPv4 addresses, including public IPv4 addresses associated with running instances and Elastic IP addresses. For more information, see the Public IPv4 Address tab on the Amazon VPC pricing page.

There is nothing to say that it defaults to true that I can see.

Looking at the SDK reference, it does mention:

If launching into a default subnet, the default value is true.

Are you perhaps launching your instance into a default subnet?

[tuananh]

I'm pretty sure we do not use the default subnet. we delete the default vpc as part of our account provisioning process .

[tuananh]

also i corporate settings, there is usually an SCP required AssociatePublicIpAddress explicitly set to false as well

[JoelSpeed]

Hmm, double checking on that logic, I think the issue is just line 374

if aws.BoolValue(machineProviderConfig.PublicIP) {

If it were instead

if machineProviderConfig.PublicIP != nil {

The rest of the logic works just the same, except, whatever the user put in explicitly as true or false into that value, would get passed onto AWS. Meaning anyone not expressing an opinion, would still leverage AWS's default value, whatever they determine that to be.

[JoelSpeed]

Appears this is a regression as we used to set the value directly, up until #78

[tuananh]

yeah that works too. we would like to be able to explicitly set that value