Merge pull request #99 from Charliekenney23/fix-generic-api-error-handling

Fix bug that caused generic API errors via InstanceExistsByProviderID to delete nodes

Author: 0xch4z

Makefile

@@ -16,6 +16,10 @@ $(GOPATH)/bin/goimports:
 $(GOPATH)/bin/ginkgo:
 	GO111MODULE=off go get -u github.com/onsi/ginkgo/ginkgo
 
+.PHONY: codegen
+codegen:
+	go generate ./...
+
 .PHONY: vet
 # lint the codebase
 vet:
@@ -29,18 +33,18 @@ fmt: vet $(GOPATH)/bin/goimports
 
 .PHONY: test
 # we say code is not worth testing unless it's formatted
-test: $(GOPATH)/bin/ginkgo fmt
+test: $(GOPATH)/bin/ginkgo fmt codegen
 	ginkgo -r --v --progress --trace --cover --skipPackage=test $(TEST_ARGS)
 
 .PHONY: build-linux
-build-linux:
+build-linux: codegen
 	echo "cross compiling linode-cloud-controller-manager for linux/amd64" && \
 		GOOS=linux GOARCH=amd64 \
 		CGO_ENABLED=0 \
 		go build -o dist/linode-cloud-controller-manager-linux-amd64 .
 
 .PHONY: build
-build:
+build: codegen
 	echo "compiling linode-cloud-controller-manager" && \
 		CGO_ENABLED=0 \
 		go build -o dist/linode-cloud-controller-manager .

cloud/linode/client.go

@@ -0,0 +1,29 @@
+package linode
+
+//go:generate go run github.com/golang/mock/mockgen -destination mock_client_test.go -package linode github.com/linode/linode-cloud-controller-manager/cloud/linode LinodeClient
+
+import (
+	"context"
+
+	"github.com/linode/linodego"
+)
+
+type LinodeClient interface {
+	GetInstance(context.Context, int) (*linodego.Instance, error)
+	ListInstances(context.Context, *linodego.ListOptions) ([]linodego.Instance, error)
+	GetInstanceIPAddresses(context.Context, int) (*linodego.InstanceIPAddressResponse, error)
+
+	CreateNodeBalancer(context.Context, linodego.NodeBalancerCreateOptions) (*linodego.NodeBalancer, error)
+	GetNodeBalancer(context.Context, int) (*linodego.NodeBalancer, error)
+	UpdateNodeBalancer(context.Context, int, linodego.NodeBalancerUpdateOptions) (*linodego.NodeBalancer, error)
+	DeleteNodeBalancer(context.Context, int) error
+	ListNodeBalancers(context.Context, *linodego.ListOptions) ([]linodego.NodeBalancer, error)
+
+	CreateNodeBalancerConfig(context.Context, int, linodego.NodeBalancerConfigCreateOptions) (*linodego.NodeBalancerConfig, error)
+	DeleteNodeBalancerConfig(context.Context, int, int) error
+	ListNodeBalancerConfigs(context.Context, int, *linodego.ListOptions) ([]linodego.NodeBalancerConfig, error)
+	RebuildNodeBalancerConfig(context.Context, int, int, linodego.NodeBalancerConfigRebuildOptions) (*linodego.NodeBalancerConfig, error)
+}
+
+// linodego.Client implements LinodeClient
+var _ LinodeClient = (*linodego.Client)(nil)

cloud/linode/cloud.go

@@ -27,7 +27,7 @@ var Options struct {
 }
 
 type linodeCloud struct {
-	client        *linodego.Client
+	client        LinodeClient
 	instances     cloudprovider.Instances
 	zones         cloudprovider.Zones
 	loadbalancers cloudprovider.LoadBalancer

cloud/linode/common.go

@@ -0,0 +1,65 @@
+package linode
+
+import (
+	"context"
+	"fmt"
+	"strconv"
+	"strings"
+
+	"github.com/linode/linodego"
+	"github.com/pkg/errors"
+	"k8s.io/apimachinery/pkg/types"
+	cloudprovider "k8s.io/cloud-provider"
+)
+
+const providerIDPrefix = "linode://"
+
+type invalidProviderIDError struct {
+	value string
+}
+
+func (e invalidProviderIDError) Error() string {
+	return fmt.Sprintf("invalid provider ID %q", e.value)
+}
+
+func parseProviderID(providerID string) (int, error) {
+	if !strings.HasPrefix(providerID, providerIDPrefix) {
+		return 0, invalidProviderIDError{providerID}
+	}
+	id, err := strconv.Atoi(strings.TrimPrefix(providerID, providerIDPrefix))
+	if err != nil {
+		return 0, invalidProviderIDError{providerID}
+	}
+	return id, nil
+}
+
+func linodeFilterListOptions(targetLabel string) *linodego.ListOptions {
+	jsonFilter := fmt.Sprintf(`{"label":%q}`, targetLabel)
+	return linodego.NewListOptions(0, jsonFilter)
+}
+
+func linodeByName(ctx context.Context, client LinodeClient, nodeName types.NodeName) (*linodego.Instance, error) {
+	linodes, err := client.ListInstances(ctx, linodeFilterListOptions(string(nodeName)))
+	if err != nil {
+		return nil, err
+	}
+
+	if len(linodes) == 0 {
+		return nil, cloudprovider.InstanceNotFound
+	} else if len(linodes) > 1 {
+		return nil, errors.New(fmt.Sprintf("Multiple instances found with name %v", nodeName))
+	}
+
+	return &linodes[0], nil
+}
+
+func linodeByID(ctx context.Context, client LinodeClient, id int) (*linodego.Instance, error) {
+	instance, err := client.GetInstance(ctx, id)
+	if err != nil {
+		return nil, err
+	}
+	if instance == nil {
+		return nil, fmt.Errorf("linode not found with id %v", id)
+	}
+	return instance, nil
+}

cloud/linode/common_test.go

@@ -0,0 +1,52 @@
+package linode
+
+import "testing"
+
+func TestParseProviderID(t *testing.T) {
+	for _, tc := range []struct {
+		name        string
+		providerID  string
+		expectedID  int
+		errExpected bool
+	}{
+		{
+			name:        "empty string is invalid",
+			providerID:  "",
+			errExpected: true,
+		},
+		{
+			name:        "malformed provider id",
+			providerID:  "invalidproviderid!",
+			errExpected: true,
+		},
+		{
+			name:        "wrong prefix",
+			providerID:  "notlinode://123",
+			errExpected: true,
+		},
+		{
+			name:       "valid",
+			providerID: "linode://123",
+			expectedID: 123,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			id, err := parseProviderID(tc.providerID)
+			if err != nil {
+				if !tc.errExpected {
+					t.Errorf("unexpected error: %v", err)
+				}
+			} else if tc.errExpected {
+				t.Error("expected an error; got nil")
+			}
+
+			if id != tc.expectedID {
+				t.Errorf("expected id to be %d; got %d", tc.expectedID, id)
+			}
+		})
+	}
+}
+
+func stringPtr(s string) *string {
+	return &s
+}

cloud/linode/fake_linode_test.go

@@ -18,12 +18,10 @@ import (
 )
 
 type fakeAPI struct {
-	t        *testing.T
-	instance *linodego.Instance
-	ips      []*linodego.InstanceIP
-	nb       map[string]*linodego.NodeBalancer
-	nbc      map[string]*linodego.NodeBalancerConfig
-	nbn      map[string]*linodego.NodeBalancerNode
+	t   *testing.T
+	nb  map[string]*linodego.NodeBalancer
+	nbc map[string]*linodego.NodeBalancerConfig
+	nbn map[string]*linodego.NodeBalancerNode
 
 	requests map[fakeRequest]struct{}
 }
@@ -40,42 +38,8 @@ type filterStruct struct {
 }
 
 func newFake(t *testing.T) *fakeAPI {
-	publicIP := net.ParseIP("45.79.101.25")
-	privateIP := net.ParseIP("192.168.133.65")
-	instanceName := "test-instance"
-	region := "us-east"
 	return &fakeAPI{
-		t: t,
-		instance: &linodego.Instance{
-			Label:      instanceName,
-			Region:     region,
-			Image:      "linode/ubuntu16.04lts",
-			Type:       "g6-standard-2",
-			Group:      "Linode-Group",
-			ID:         123,
-			Status:     "running",
-			Hypervisor: "kvm",
-			IPv4: []*net.IP{
-				&publicIP,
-				&privateIP,
-			},
-		},
-		ips: []*linodego.InstanceIP{
-			{
-				Address:  publicIP.String(),
-				Public:   true,
-				LinodeID: 123,
-				Type:     "ipv4",
-				Region:   region,
-			},
-			{
-				Address:  privateIP.String(),
-				Public:   false,
-				LinodeID: 123,
-				Type:     "ipv4",
-				Region:   region,
-			},
-		},
+		t:        t,
 		nb:       make(map[string]*linodego.NodeBalancer),
 		nbc:      make(map[string]*linodego.NodeBalancerConfig),
 		nbn:      make(map[string]*linodego.NodeBalancerNode),
@@ -112,62 +76,6 @@ func (f *fakeAPI) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	case "GET":
 		whichAPI := strings.Split(urlPath[1:], "/")
 		switch whichAPI[0] {
-		case "linode":
-			switch whichAPI[1] {
-			case "instances":
-				rx, _ := regexp.Compile("/linode/instances/[0-9]+/ips")
-				if rx.MatchString(urlPath) {
-					resp := linodego.InstanceIPAddressResponse{
-						IPv4: &linodego.InstanceIPv4Response{
-							Public:  []*linodego.InstanceIP{f.ips[0]},
-							Private: []*linodego.InstanceIP{f.ips[1]},
-						},
-					}
-					rr, _ := json.Marshal(resp)
-					_, _ = w.Write(rr)
-					return
-				}
-
-				rx, _ = regexp.Compile("/linode/instances/[0-9]+")
-				if rx.MatchString(urlPath) {
-					id := filepath.Base(urlPath)
-					if id == strconv.Itoa(f.instance.ID) {
-						rr, _ := json.Marshal(&f.instance)
-						_, _ = w.Write(rr)
-					}
-					return
-				}
-
-				rx, _ = regexp.Compile("/linode/instances")
-				if rx.MatchString(urlPath) {
-					res := 0
-					data := []linodego.Instance{}
-					filter := r.Header.Get("X-Filter")
-					if filter == "" {
-						data = append(data, *f.instance)
-					} else {
-						var fs filterStruct
-						err := json.Unmarshal([]byte(filter), &fs)
-						if err != nil {
-							f.t.Fatal(err)
-						}
-						if fs.Label == f.instance.Label {
-							data = append(data, *f.instance)
-						}
-					}
-					resp := linodego.InstancesPagedResponse{
-						PageOptions: &linodego.PageOptions{
-							Page:    1,
-							Pages:   1,
-							Results: res,
-						},
-						Data: data,
-					}
-					rr, _ := json.Marshal(resp)
-					_, _ = w.Write(rr)
-					return
-				}
-			}
 		case "nodebalancers":
 			rx, _ := regexp.Compile("/nodebalancers/[0-9]+/configs/[0-9]+/nodes/[0-9]+")
 			if rx.MatchString(urlPath) {