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.

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

Add ENI support for nodes(for Fargate nodes) #223

Merged
merged 1 commit into from
Jun 23, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
100 changes: 100 additions & 0 deletions pkg/providers/v1/aws.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,12 @@ const (
// Number of node names that can be added to a filter. The AWS limit is 200
// but we are using a lower limit on purpose
filterNodeLimit = 150

// fargateNodeNamePrefix string is added to awsInstance nodeName and providerID of Fargate nodes.
fargateNodeNamePrefix = "fargate-"

// privateDNSNamePrefix is the prefix added to ENI Private DNS Name.
privateDNSNamePrefix = "ip-"
)

const (
Expand Down Expand Up @@ -365,6 +371,8 @@ type EC2 interface {
ModifyInstanceAttribute(request *ec2.ModifyInstanceAttributeInput) (*ec2.ModifyInstanceAttributeOutput, error)

DescribeVpcs(input *ec2.DescribeVpcsInput) (*ec2.DescribeVpcsOutput, error)

DescribeNetworkInterfaces(input *ec2.DescribeNetworkInterfacesInput) (*ec2.DescribeNetworkInterfacesOutput, error)
}

// ELB is a simple pass-through of AWS' ELB client interface, which allows for testing
Expand Down Expand Up @@ -971,6 +979,15 @@ func (s *awsSdkEC2) DescribeInstances(request *ec2.DescribeInstancesInput) ([]*e
return results, nil
}

// DescribeNetworkInterfaces describes network interface provided in the input.
func (s *awsSdkEC2) DescribeNetworkInterfaces(input *ec2.DescribeNetworkInterfacesInput) (*ec2.DescribeNetworkInterfacesOutput, error) {
requestTime := time.Now()
resp, err := s.ec2.DescribeNetworkInterfaces(input)
timeTaken := time.Since(requestTime).Seconds()
recordAWSMetric("describe_network_interfaces", timeTaken, err)
return resp, err
}

// Implements EC2.DescribeSecurityGroups
func (s *awsSdkEC2) DescribeSecurityGroups(request *ec2.DescribeSecurityGroupsInput) ([]*ec2.SecurityGroup, error) {
// Security groups are paged
Expand Down Expand Up @@ -1616,6 +1633,16 @@ func extractNodeAddresses(instance *ec2.Instance) ([]v1.NodeAddress, error) {
return addresses, nil
}

// getNodeAddressesForFargateNode generates list of Node addresses for Fargate node.
func getNodeAddressesForFargateNode(privateDNSName, privateIP string) []v1.NodeAddress {
addresses := []v1.NodeAddress{}
addresses = append(addresses, v1.NodeAddress{Type: v1.NodeInternalIP, Address: privateIP})
if privateDNSName != "" {
addresses = append(addresses, v1.NodeAddress{Type: v1.NodeInternalDNS, Address: privateDNSName})
}
return addresses
}

// NodeAddressesByProviderID returns the node addresses of an instances with the specified unique providerID
// This method will not be called from the node that is requesting this ID. i.e. metadata service
// and other local methods cannot be used here
Expand All @@ -1625,6 +1652,14 @@ func (c *Cloud) NodeAddressesByProviderID(ctx context.Context, providerID string
return nil, err
}

if isFargateNode(string(instanceID)) {
eni, err := c.describeNetworkInterfaces(string(instanceID))
if eni == nil || err != nil {
return nil, err
}
return getNodeAddressesForFargateNode(aws.StringValue(eni.PrivateDnsName), aws.StringValue(eni.PrivateIpAddress)), nil
}

instance, err := describeInstance(c.ec2, instanceID)
if err != nil {
return nil, err
Expand All @@ -1641,6 +1676,11 @@ func (c *Cloud) InstanceExistsByProviderID(ctx context.Context, providerID strin
return false, err
}

if isFargateNode(string(instanceID)) {
eni, err := c.describeNetworkInterfaces(string(instanceID))
return eni != nil, err
}

request := &ec2.DescribeInstancesInput{
InstanceIds: []*string{instanceID.awsString()},
}
Expand Down Expand Up @@ -1676,6 +1716,11 @@ func (c *Cloud) InstanceShutdownByProviderID(ctx context.Context, providerID str
return false, err
}

if isFargateNode(string(instanceID)) {
eni, err := c.describeNetworkInterfaces(string(instanceID))
return eni != nil, err
}

request := &ec2.DescribeInstancesInput{
InstanceIds: []*string{instanceID.awsString()},
}
Expand Down Expand Up @@ -1732,6 +1777,10 @@ func (c *Cloud) InstanceTypeByProviderID(ctx context.Context, providerID string)
return "", err
}

if isFargateNode(string(instanceID)) {
return "", nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason we didn't create a custom "fargate" instance type? Not saying we should but just wondering if we explored that option. It might be nice from an API consumer's perspective to not have to check the provider ID to determine why an instance type is empty. Also it could be useful as fargate evolves to be able to have different instance types, if there is any information we want to expose about the instance. Of course, that can come later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point but we don't want to populate this information as Fargate abstracts the instance types. But should be easy to add our own definition for instance-type as "fargate" if we need to. But in future it might affect if Fargate has some plan to expose the task type as "compute-optimized", "memory-optimized", etc. My suggestion is to keep it simple for now and add the type as "fargate" later if needed. Do you agree as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, exposing the task type would be be a good future reason to fill this in. In that case, setting it to "fargate" shouldn't affect anything negatively, but we can keep it simple for now and wait until someone asks for something like that.

}

instance, err := describeInstance(c.ec2, instanceID)
if err != nil {
return "", err
Expand Down Expand Up @@ -1836,6 +1885,18 @@ func (c *Cloud) GetZoneByProviderID(ctx context.Context, providerID string) (clo
if err != nil {
return cloudprovider.Zone{}, err
}

if isFargateNode(string(instanceID)) {
eni, err := c.describeNetworkInterfaces(string(instanceID))
if eni == nil || err != nil {
return cloudprovider.Zone{}, err
}
return cloudprovider.Zone{
FailureDomain: *eni.AvailabilityZone,
Region: c.region,
}, nil
}

instance, err := c.getInstanceByID(string(instanceID))
if err != nil {
return cloudprovider.Zone{}, err
Expand Down Expand Up @@ -4981,6 +5042,11 @@ func (c *Cloud) getFullInstance(nodeName types.NodeName) (*awsInstance, *ec2.Ins
return awsInstance, instance, err
}

// isFargateNode returns true if given node runs on Fargate compute
func isFargateNode(nodeName string) bool {
return strings.HasPrefix(nodeName, fargateNodeNamePrefix)
}

func (c *Cloud) nodeNameToProviderID(nodeName types.NodeName) (InstanceID, error) {
if len(nodeName) == 0 {
return "", fmt.Errorf("no nodeName provided")
Expand Down Expand Up @@ -5034,3 +5100,37 @@ func getInitialAttachDetachDelay(status string) time.Duration {
}
return volumeAttachmentStatusInitialDelay
}

// describeNetworkInterfaces returns network interface information for the given DNS name.
func (c *Cloud) describeNetworkInterfaces(nodeName string) (*ec2.NetworkInterface, error) {
eniEndpoint := strings.TrimPrefix(nodeName, fargateNodeNamePrefix)

filters := []*ec2.Filter{
newEc2Filter("attachment.status", "attached"),
newEc2Filter("vpc-id", c.vpcID),
}

// when enableDnsSupport is set to false in a VPC, interface will not have private DNS names.
if strings.HasPrefix(eniEndpoint, privateDNSNamePrefix) {
filters = append(filters, newEc2Filter("private-dns-name", eniEndpoint))
} else {
filters = append(filters, newEc2Filter("private-ip-address", eniEndpoint))
}

request := &ec2.DescribeNetworkInterfacesInput{
Filters: filters,
}

eni, err := c.ec2.DescribeNetworkInterfaces(request)
if err != nil {
return nil, err
}
if len(eni.NetworkInterfaces) == 0 {
return nil, nil
}
if len(eni.NetworkInterfaces) != 1 {
// This should not be possible - ids should be unique
return nil, fmt.Errorf("multiple interfaces found with same id %q", eni.NetworkInterfaces)
}
return eni.NetworkInterfaces[0], nil
}
27 changes: 27 additions & 0 deletions pkg/providers/v1/aws_fakes.go
Original file line number Diff line number Diff line change
Expand Up @@ -684,3 +684,30 @@ func contains(haystack []*string, needle string) bool {
}
return false
}

// DescribeNetworkInterfaces returns list of ENIs for testing
func (ec2i *FakeEC2Impl) DescribeNetworkInterfaces(input *ec2.DescribeNetworkInterfacesInput) (*ec2.DescribeNetworkInterfacesOutput, error) {
networkInterface := []*ec2.NetworkInterface{
{
PrivateIpAddress: aws.String("1.2.3.4"),
AvailabilityZone: aws.String("us-west-2c"),
},
}
for _, filter := range input.Filters {
if strings.HasPrefix(*filter.Values[0], fargateNodeNamePrefix) {
// verify filter doesn't have fargate prefix
panic(fmt.Sprintf("invalid endpoint specified for DescribeNetworkInterface call %s", *filter.Values[0]))
} else if strings.HasPrefix(*filter.Values[0], "not-found") {
// for negative testing
return &ec2.DescribeNetworkInterfacesOutput{}, nil
}

if *filter.Name == "private-dns-name" {
networkInterface[0].PrivateDnsName = aws.String("ip-1-2-3-4.compute.amazon.com")
}
}

return &ec2.DescribeNetworkInterfacesOutput{
NetworkInterfaces: networkInterface,
}, nil
}
82 changes: 82 additions & 0 deletions pkg/providers/v1/aws_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3516,3 +3516,85 @@ func Test_parseStringSliceAnnotation(t *testing.T) {
})
}
}

func TestNodeAddressesForFargate(t *testing.T) {
awsServices := newMockedFakeAWSServices(TestClusterID)
c, _ := newAWSCloud(CloudConfig{}, awsServices)

nodeAddresses, _ := c.NodeAddressesByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/fargate-ip-192.168.164.88")
verifyNodeAddressesForFargate(t, true, nodeAddresses)
}

func TestNodeAddressesForFargatePrivateIP(t *testing.T) {
awsServices := newMockedFakeAWSServices(TestClusterID)
c, _ := newAWSCloud(CloudConfig{}, awsServices)

nodeAddresses, _ := c.NodeAddressesByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/fargate-192.168.164.88")
verifyNodeAddressesForFargate(t, false, nodeAddresses)
}

func verifyNodeAddressesForFargate(t *testing.T, verifyPublicIP bool, nodeAddresses []v1.NodeAddress) {
if verifyPublicIP {
assert.Equal(t, 2, len(nodeAddresses))
assert.Equal(t, "ip-1-2-3-4.compute.amazon.com", nodeAddresses[1].Address)
assert.Equal(t, v1.NodeInternalDNS, nodeAddresses[1].Type)
} else {
assert.Equal(t, 1, len(nodeAddresses))
}
assert.Equal(t, "1.2.3.4", nodeAddresses[0].Address)
assert.Equal(t, v1.NodeInternalIP, nodeAddresses[0].Type)
}

func TestInstanceExistsByProviderIDForFargate(t *testing.T) {
awsServices := newMockedFakeAWSServices(TestClusterID)
c, _ := newAWSCloud(CloudConfig{}, awsServices)

instanceExist, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/fargate-192.168.164.88")
assert.Nil(t, err)
assert.True(t, instanceExist)
}

func TestInstanceNotExistsByProviderIDForFargate(t *testing.T) {
awsServices := newMockedFakeAWSServices(TestClusterID)
c, _ := newAWSCloud(CloudConfig{}, awsServices)

instanceExist, err := c.InstanceExistsByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/fargate-not-found")
assert.Nil(t, err)
assert.False(t, instanceExist)
}

func TestInstanceShutdownByProviderIDForFargate(t *testing.T) {
awsServices := newMockedFakeAWSServices(TestClusterID)
c, _ := newAWSCloud(CloudConfig{}, awsServices)

instanceExist, err := c.InstanceShutdownByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/fargate-192.168.164.88")
assert.Nil(t, err)
assert.True(t, instanceExist)
}

func TestInstanceShutdownNotExistsByProviderIDForFargate(t *testing.T) {
awsServices := newMockedFakeAWSServices(TestClusterID)
c, _ := newAWSCloud(CloudConfig{}, awsServices)

instanceExist, err := c.InstanceShutdownByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/fargate-not-found")
assert.Nil(t, err)
assert.False(t, instanceExist)
}

func TestInstanceTypeByProviderIDForFargate(t *testing.T) {
awsServices := newMockedFakeAWSServices(TestClusterID)
c, _ := newAWSCloud(CloudConfig{}, awsServices)

instanceType, err := c.InstanceTypeByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/fargate-not-found")
assert.Nil(t, err)
assert.Equal(t, "", instanceType)
}

func TestGetZoneByProviderIDForFargate(t *testing.T) {
awsServices := newMockedFakeAWSServices(TestClusterID)
c, _ := newAWSCloud(CloudConfig{}, awsServices)

zoneDetails, err := c.GetZoneByProviderID(context.TODO(), "aws:///us-west-2c/1abc-2def/fargate-192.168.164.88")
assert.Nil(t, err)
assert.Equal(t, "us-west-2c", zoneDetails.FailureDomain)
}
12 changes: 5 additions & 7 deletions pkg/providers/v1/instances.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ func (i InstanceID) awsString() *string {
// the following form
// * aws:///<zone>/<awsInstanceId>
// * aws:////<awsInstanceId>
// * aws:///<zone>/fargate-<eni-ip-address>
// * <awsInstanceId>
type KubernetesInstanceID string

Expand All @@ -73,17 +74,14 @@ func (name KubernetesInstanceID) MapToAWSInstanceID() (InstanceID, error) {

awsID := ""
tokens := strings.Split(strings.Trim(url.Path, "/"), "/")
if len(tokens) == 1 {
// instanceId
awsID = tokens[0]
} else if len(tokens) == 2 {
// az/instanceId
awsID = tokens[1]
// last token in the providerID is the aws resource ID for both EC2 and Fargate nodes
if len(tokens) > 0 {
awsID = tokens[len(tokens)-1]
}

// We sanity check the resulting volume; the two known formats are
// i-12345678 and i-12345678abcdef01
if awsID == "" || !awsInstanceRegMatch.MatchString(awsID) {
if awsID == "" || !(awsInstanceRegMatch.MatchString(awsID) || isFargateNode(awsID)) {
return "", fmt.Errorf("Invalid format for AWS instance (%s)", name)
}

Expand Down
8 changes: 8 additions & 0 deletions pkg/providers/v1/instances_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,14 @@ func TestMapToAWSInstanceIDs(t *testing.T) {
Kubernetes: "",
ExpectError: true,
},
{
Kubernetes: "aws:///us-west-2c/1abc-2def/fargate-ip-192-168-164-88.internal",
Aws: "fargate-ip-192-168-164-88.internal",
},
{
Kubernetes: "aws:///us-west-2c/1abc-2def/fargate-192.168.164.88",
Aws: "fargate-192.168.164.88",
},
}

for _, test := range tests {
Expand Down