instances: initial implementation of instancesV2 interface #131

nicolehanjing · 2020-09-25T22:51:11Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Take over the initial work here: #127
This is a first pass at implementing Instances.
Some TODOs:

add more unit tests
support node naming policy other than private DNS names

Which issue(s) this PR fixes:

Part of #125

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Add initial implementation of Instances for the v2 provider.

k8s-ci-robot · 2020-09-25T22:51:19Z

Hi @nicolehanjing. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/providers/v2/instances_test.go

nicolehanjing · 2020-09-25T22:53:38Z

pkg/providers/v2/instances.go

+// parseInstanceIDFromProviderID parses the node's instance ID based on the well-known provider ID format:
+//   * aws://<availability-zone>/<instance-id>
+// This function always assumes a valid providerID format was provided.
+func parseInstanceIDFromProviderID(providerID string) (string, error) {


@andrewsykim is that intended to only parse well-formatted providerID? Should I take care of invalid cases?

nicolehanjing · 2020-09-25T22:55:01Z

pkg/providers/v2/instances.go

+
+	if ec2Instance.State != nil {
+		state := aws.StringValue(ec2Instance.State.Name)
+		if state == ec2.InstanceStateNameTerminated || state == ec2.InstanceStateNameStopping || state == ec2.InstanceStateNameStopped {


@andrewsykim A little confused on how we define "shutdown"
I feel that states after "shutting down" (terminated, stopping, stopped) should all be considered as a "shutdown" state, is that right? 🤔

I think "shutdown" is really referring to "stopped" here. The key difference from "terminated" is that a stopped instance can go back to running state, where-as terminated instances are gone for good.

So we should remove the check for ec2.InstanceStateNameTerminated here.

gotcha, thanks for the explanation!

pkg/providers/v2/instances.go

nicolehanjing · 2020-09-25T22:58:21Z

pkg/providers/v2/instances.go

+	var err error
+	var ec2Instance *ec2.Instance
+	if node.Spec.ProviderID == "" {
+		//  TODO: support node name policy other than private DNS names


not sure if I have enough contexts here, can you share more inputs? :) @andrewsykim

In the existing implementation, only private DNS is allowed for a node's name (see kubernetes/kubernetes#52241). We should allow other naming policies. I think a reasonable starting point is allowing the node name to be either the private DNS or the instance name

I'm thinking that maybe we need a ConfigMap or something to store this information. For this PR maybe just implementing with private DNS is sufficient.

gotcha, thanks for the contexts!

After looking into kops and EKS in some detail, I don't think we would want "arbitrary" names, or at least if we do, it would have to come with huge caveats around node security.

In particular, EKS and kops-controller match a instance ID attestation with the privateDNSName, via an STS based authentication webhook in EKS using aws-iam-authenticator, or via issuance of a certificate with the privateDNSname as the node name with presentation of the AWS instance identity document in kops-controller's case (CAPI will likely also implement the latter).

In theory, this means we can securely support instanceID, privateDNSName or other unique identifiers on the EC2 DescribeInstances, but beyond that, it would have to come with big "you don't want to use this" warnings.

cc @nckturner @justinsb

Also, why would we use a ConfigMap vs. say ComponentConfig for the controller?

EKS currently has a lot of built-in assumptions on private DNS. That being said, instance ID is attractive because its guaranteed to be unique. I'm ok with making it possible for other names, but we will need to be able to restrict it for EKS to stuff available to DescribeInstances, like @randomvariable said. I don't think a ConfigMap is a good idea though. Its seems like that suggests we want to allow on-the-fly reconfiguration, which for EKS we definitely wouldn't want. Guessing, but if we allowed customers to make changes here then we'd probably want it protected behind an EKS API, which would be easier to do if it was configuration passed in via flags/ComponentConfig file on disk, and required a restart.

Also, why would we use a ConfigMap vs. say ComponentConfig for the controller?

I'm not particularly tied to using ConfigMap here, but if we use a config file it should ideally be yaml/json and not INI. I'm not sure ComponentConfig is relevant here since this is a config file read by aws-cloud-controller-manager, not a config for any of its options/flags.

EKS currently has a lot of built-in assumptions on private DNS. That being said, instance ID is attractive because its guaranteed to be unique. I'm ok with making it possible for other names, but we will need to be able to restrict it for EKS to stuff available to DescribeInstances

Sounds like there needs to be a broader discussion on this topic for sure. @nicolehanjing since we know for sure that we still want to support private DNS, let's get this PR only working with that and make sure we have a follow-up PR to support instance ID and other naming policies.

gotcha! thanks for the info!

pkg/providers/v2/instances.go

andrewsykim · 2020-09-28T16:43:20Z

pkg/providers/v2/instances.go

+	var err error
+	var ec2Instance *ec2.Instance
+	if node.Spec.ProviderID == "" {
+		//  TODO: support node name policy other than private DNS names


In the existing implementation, only private DNS is allowed for a node's name (see kubernetes/kubernetes#52241). We should allow other naming policies. I think a reasonable starting point is allowing the node name to be either the private DNS or the instance name

andrewsykim · 2020-09-28T16:44:24Z

pkg/providers/v2/instances.go

+	var err error
+	var ec2Instance *ec2.Instance
+	if node.Spec.ProviderID == "" {
+		//  TODO: support node name policy other than private DNS names


I'm thinking that maybe we need a ConfigMap or something to store this information. For this PR maybe just implementing with private DNS is sufficient.

pkg/providers/v2/instances.go

andrewsykim · 2020-09-28T16:47:14Z

pkg/providers/v2/instances.go

+
+	if ec2Instance.State != nil {
+		state := aws.StringValue(ec2Instance.State.Name)
+		if state == ec2.InstanceStateNameTerminated || state == ec2.InstanceStateNameStopping || state == ec2.InstanceStateNameStopped {


I think "shutdown" is really referring to "stopped" here. The key difference from "terminated" is that a stopped instance can go back to running state, where-as terminated instances are gone for good.

andrewsykim · 2020-09-28T16:47:28Z

pkg/providers/v2/instances.go

+
+	if ec2Instance.State != nil {
+		state := aws.StringValue(ec2Instance.State.Name)
+		if state == ec2.InstanceStateNameTerminated || state == ec2.InstanceStateNameStopping || state == ec2.InstanceStateNameStopped {


So we should remove the check for ec2.InstanceStateNameTerminated here.

pkg/providers/v2/instances_test.go

nicolehanjing · 2020-09-29T05:00:42Z

pkg/providers/v2/instances_test.go

+	}
+}
+
+func TestInstanceExists(t *testing.T) {


@andrewsykim Updated the unit tests, let me know if you have any other suggestions :)

randomvariable · 2020-09-29T10:46:04Z

We'll need some follow up issues to do the following, but not blocking this PR:

Add rate limiters and metrics collection to the AWS request handlers
Check if there's anything needed to support custom EC2 endpoints (for GovCloud/Secret Region and Outposts setups)

randomvariable · 2020-09-29T10:46:23Z

/ok-to-test

andrewsykim · 2020-09-29T13:59:34Z

We'll need some follow up issues to do the following, but not blocking this PR:
Add rate limiters and metrics collection to the AWS request handlers
Check if there's anything needed to support custom EC2 endpoints (for airgapped and Outposts setups)

+1, we should definitely add the first one before removing the alpha gating env var (but in a follow-up PR):

cloud-provider-aws/cmd/aws-cloud-controller-manager/main.go

Lines 104 to 108 in 4eef54c

    
           if cloudProvider == awsv2.ProviderName { 
        
           	if v2Enabled := os.Getenv(enableAlphaV2EnvVar); v2Enabled != "true" { 
        
           		klog.Fatalf("aws/v2 cloud provider requires environment variable ENABLE_ALPHA_V2=true to be set") 
        
           	} 
        
           }

nckturner · 2020-10-13T22:29:59Z

pkg/providers/v2/instances.go

+func (i *instances) InstanceExists(ctx context.Context, node *v1.Node) (bool, error) {
+	var err error
+	if node.Spec.ProviderID == "" {
+		_, err = i.getInstanceByPrivateDNSName(ctx, node.Name)


I wouldn't mind some level 4 (or maybe higher) logging here that printed a line "looking for node X by private DNS name".

gotcha, will add!

nckturner · 2020-10-13T22:30:29Z

pkg/providers/v2/instances.go

+		}
+	}
+
+	_, err = i.getInstanceByProviderID(ctx, node.Spec.ProviderID)


Another log line at 4 or higher for "looking for node by provider ID".

nckturner · 2020-10-13T22:35:33Z

pkg/providers/v2/instances.go

+		if err != nil {
+			return false, err
+		}
+	}


If the instance does exist by private DNS name, and the provider ID is empty, we are still falling through this block and calling getInstanceByProviderID()? Should we have an else statement for the provider ID check?

updated! I wrap the logic in an if-else block so that ensures we only call getInstanceByProviderID() when the providerID is not empty

nckturner · 2020-10-13T22:38:03Z

pkg/providers/v2/instances.go

+	if node.Spec.ProviderID == "" {
+		_, err = i.getInstanceByPrivateDNSName(ctx, node.Name)
+		if err == cloudprovider.InstanceNotFound {
+			return false, nil


If we group this entire thing into an if else, then we can also add a single err != nil and err == cloudprovider.InstanceNotFound at the end of the function, and add logs (maybe at level 6?) for not found instances.

nckturner · 2020-10-13T22:43:37Z

pkg/providers/v2/instances.go

+	var ec2Instance *ec2.Instance
+	if node.Spec.ProviderID == "" {
+		//  TODO: support node name policy other than private DNS names
+		ec2Instance, err = i.getInstanceByPrivateDNSName(ctx, node.Name)


Instead of having the getInstanceByPrivateDNSName and then getInstanceByProviderID logic in multiple places, can we put all that logic into a getInstance(ctx context.Context, node *Node) (*ec2.Instance, error) function? That way it will be easier to refactor or add other forms of getInstance in the future.

the only difference in these two functions is the ec2 request, I think we could put the logic into a getInstance(ctx context.Context, node *Node) (*ec2.Instance, error) function but inside that function we need to differentiate the request based on the type of given node info

nckturner · 2020-10-13T23:23:57Z

/cc @micahhausler @wongma7

nicolehanjing · 2020-10-14T00:16:55Z

pkg/providers/v2/instances.go

+
+// getInstance returns the instance if the instance with the given node info still exists.
+// If false an error will be returned, the instance will be immediately deleted by the cloud controller manager.
+func (i *instances) getInstance(ctx context.Context, node *v1.Node) (*ec2.Instance, error) {


@nckturner Updated!

added the logs

unified two functions getInstanceByProviderID and getInstanceByPrivateDNSName into getInstance and the only difference is the ec2 request input

likely unified two functions InstanceShutdownByProviderID and InstanceShutdownByPrivateDNSName into InstanceShutdown
PTAL! :)

wongma7 · 2020-10-14T02:15:45Z

pkg/providers/v2/instances.go

+		request = &ec2.DescribeInstancesInput{
+			InstanceIds: []*string{aws.String(instanceID)},
+			Filters: []*ec2.Filter{
+				newEc2Filter("instance-state-name", aliveFilter...),


It may be better to describe without this aliveFilter and instead filter out terminated instances locally like v1 did. I do not have numbers or documentation but my understanding is that filters can affect describeinstances performance. cc kubernetes/kubernetes#78140

gotcha, will update! Thanks for the contexts

andrewsykim

Really minor comments

andrewsykim · 2020-10-19T21:34:01Z

pkg/providers/v2/instances.go

+	_, err := i.getInstance(ctx, node)
+
+	if err == cloudprovider.InstanceNotFound {
+		klog.V(6).Infof("instance not found")


Log the node name here for the instance:

klog.V(6).Infof("instance not found for node: %s", node.Name)

pkg/providers/v2/instances.go

andrewsykim · 2020-10-19T21:37:27Z

pkg/providers/v2/instances_test.go

+
+	nodeName := "ip-192-168-0-1.ec2.internal"
+
+	tests := []struct {


These tests are looking great :)

andrewsykim · 2020-10-19T21:38:52Z

pkg/providers/v2/instances_test.go

+	tests := []struct {
+		name              string
+		node              *v1.Node
+		expectedEc2Output *ec2.DescribeInstancesOutput


I would call this field mockedEC2Output instead, since we are not actually validating against this as an "expected" valuie.

same applies for other tests

gotcha, will update!

nicolehanjing · 2020-10-20T00:02:54Z

/retest

nicolehanjing · 2020-10-20T17:01:44Z

@andrewsykim updated! PTAL :)

andrewsykim · 2020-10-20T17:27:19Z

pkg/providers/v2/instances.go

+
+		if err == ErrInstanceTerminated {
+			klog.V(6).Infof("instance terminated for node: %s", node.Name)
+			return true, nil


ErrInstanceTerminated is for terminated instances which I don't think apply for the "shutdown" case. Checking ec2.InstanceStateNameStopped might be enough for checking shutdown.

Oh I see you're already checking that below. In that case for terminated state this should return false, nil or false, err

I think this whole block can be simplified actually to:

ec2Instance, err := i.getInstance(ctx, node) if err != nil { return false, err }

gotcha, updated!

nicolehanjing · 2020-10-20T21:01:24Z

pkg/providers/v2/instances.go

+func (i *instances) InstanceShutdown(ctx context.Context, node *v1.Node) (bool, error) {
+	ec2Instance, err := i.getInstance(ctx, node)
+	if err != nil {
+		return false, err


@andrewsykim Updated the shutdown checks, PTAL!
Let me know if you have any other suggestions :)

andrewsykim · 2020-10-21T01:20:23Z

pkg/providers/v2/instances.go

+	}
+
+	state := instances[0].State.Name
+	if *state == ec2.InstanceStateNameTerminated {


If DescribeInstances returns only 1 instance and that instance has state InstanceStateNameTerminated, then I think we should treat it similar to the case of len(instances) == 0 and here return nil, cloudprovider.InstanceNotFound.

gotcha, updated!
I was thinking to have a different log for terminated instances, but as "terminated" is just an intermediate state I agree that we should treat it the same as 'not exist'

andrewsykim · 2020-10-21T01:22:08Z

pkg/providers/v2/instances.go

+	}
+
+	if len(instances) > 1 {
+		return nil, fmt.Errorf("getInstance: multiple instances found")


nit: this should use errors.New

andrewsykim

I think this is a reasonable starting point. Nothing added here is binding so we can revisit some decisions in a follow-up PR. Some functionality we need to revisit is:

Cluster tagging support
Support instance ID as the node name
Configuration API to store the various thing that are configurable.

Thanks @nicolehanjing

/approve
/lgtm

k8s-ci-robot · 2020-10-21T21:04:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, nicolehanjing

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andrewsykim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 25, 2020

k8s-ci-robot requested review from andrewsykim and justinsb September 25, 2020 22:51

nicolehanjing commented Sep 25, 2020

View reviewed changes

pkg/providers/v2/instances_test.go Outdated Show resolved Hide resolved

nicolehanjing commented Sep 25, 2020

View reviewed changes

pkg/providers/v2/instances.go Show resolved Hide resolved

nicolehanjing force-pushed the nicoleh-instances-v2 branch from f82b19d to 0d1ef93 Compare September 25, 2020 22:57

nicolehanjing commented Sep 25, 2020

View reviewed changes

pkg/providers/v2/instances.go Show resolved Hide resolved

nicolehanjing force-pushed the nicoleh-instances-v2 branch from 0d1ef93 to ad6dcd6 Compare September 25, 2020 23:22

andrewsykim mentioned this pull request Sep 28, 2020

[WIP] instances: add initial implementation of instancesV2 interface #127

Closed

andrewsykim reviewed Sep 28, 2020

View reviewed changes

nicolehanjing force-pushed the nicoleh-instances-v2 branch 3 times, most recently from a14a944 to 72c968f Compare September 29, 2020 04:58

nicolehanjing commented Sep 29, 2020

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 29, 2020

nicolehanjing changed the title ~~[WIP] instances: initial implementation of instancesV2 interface~~ instances: initial implementation of instancesV2 interface Sep 29, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 29, 2020

This was referenced Oct 12, 2020

cloud provider: fix the fake cloud provider kubernetes/kubernetes#95499

Merged

zones: initial implementation of Zones interface #143

Closed

nckturner reviewed Oct 13, 2020

View reviewed changes

k8s-ci-robot requested review from micahhausler and wongma7 October 13, 2020 23:23

nicolehanjing force-pushed the nicoleh-instances-v2 branch from 94c075e to 7ca94a4 Compare October 14, 2020 00:13

nicolehanjing commented Oct 14, 2020

View reviewed changes

wongma7 reviewed Oct 14, 2020

View reviewed changes

nicolehanjing force-pushed the nicoleh-instances-v2 branch from 7ca94a4 to c7dafbc Compare October 14, 2020 05:16

andrewsykim reviewed Oct 19, 2020

View reviewed changes

nicolehanjing force-pushed the nicoleh-instances-v2 branch from c7dafbc to 1c1ac06 Compare October 19, 2020 23:57

nicolehanjing force-pushed the nicoleh-instances-v2 branch 2 times, most recently from ccc5ee3 to 278380e Compare October 20, 2020 03:48

andrewsykim reviewed Oct 20, 2020

View reviewed changes

nicolehanjing force-pushed the nicoleh-instances-v2 branch from 278380e to 9945452 Compare October 20, 2020 17:47

nicolehanjing commented Oct 20, 2020

View reviewed changes

andrewsykim reviewed Oct 21, 2020

View reviewed changes

instances: initial implementation of Instances interface

c71bade

nicolehanjing force-pushed the nicoleh-instances-v2 branch from 9945452 to c71bade Compare October 21, 2020 04:24

andrewsykim reviewed Oct 21, 2020

View reviewed changes

k8s-ci-robot assigned andrewsykim Oct 21, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 21, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 21, 2020

k8s-ci-robot merged commit 805fde8 into kubernetes:master Oct 21, 2020

nicolehanjing mentioned this pull request Oct 28, 2020

tags: initial implementation of tags #149

Merged


		nodeName := "ip-192-168-0-1.ec2.internal"

		tests := []struct {

instances: initial implementation of instancesV2 interface #131

instances: initial implementation of instancesV2 interface #131

Conversation

nicolehanjing commented Sep 25, 2020

k8s-ci-robot commented Sep 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randomvariable Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

randomvariable commented Sep 29, 2020 • edited Loading

randomvariable commented Sep 29, 2020

andrewsykim commented Sep 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolehanjing Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

nckturner commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolehanjing commented Oct 20, 2020

nicolehanjing commented Oct 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Oct 21, 2020

randomvariable Oct 6, 2020 •

edited

Loading

randomvariable commented Sep 29, 2020 •

edited

Loading

andrewsykim commented Sep 29, 2020 •

edited

Loading

nicolehanjing Oct 13, 2020 •

edited

Loading