Address Comments

README.md

@@ -38,7 +38,10 @@ Kubernetes Services of type `LoadBalancer` will be served through a [Linode Node
 
 The Linode CCM accepts several annotations which affect the properties of the underlying NodeBalancer deployment.
 
-All of the service annotation names listed below have been shortened for readability.  Each annotation **MUST** be prefixed with `service.beta.kubernetes.io/linode-loadbalancer-`.  The values, such as `http`, are case-sensitive.
+All of the Service annotation names listed below have been shortened for readability.  The values, such as `http`, are case-sensitive.
+
+Each *Service* annotation **MUST** be prefixed with:<br />
+**`service.beta.kubernetes.io/linode-loadbalancer-`**
 
 Annotation (Suffix) | Values | Default | Description
 ---|---|---|---
@@ -81,7 +84,23 @@ Key | Values | Default | Description
 `proxy-protocol` | `none`, `v1`, `v2` | `none` | Specifies whether to use a version of Proxy Protocol on the underlying NodeBalancer. Overwrites `default-proxy-protocol`.
 `tls-secret-name` | string | | Specifies a secret to use for TLS. The secret type should be `kubernetes.io/tls`.
 
-#### Example usage
+### Nodes
+
+Kubernetes Nodes can be configured with the following annotations.
+
+Each *Node* annotation **MUST** be prefixed with:<br />
+**`node.k8s.linode.com/`**
+
+Key | Values | Default | Description
+---|---|---|---
+`private-ip` | `IPv4` | `none` | Specifies the Linode Private IP overriding default detection of the Node InternalIP.<br />When using a [VLAN] or [VPC], the Node InternalIP may not be a Linode Private IP as [required for NodeBalancers] and should be specified.
+
+
+[required for NodeBalancers]: https://www.linode.com/docs/api/nodebalancers/#nodebalancer-create__request-body-schema
+[VLAN]: https://www.linode.com/products/vlan/
+[VPC]: https://www.linode.com/blog/linode/new-betas-coming-to-green-light/
+
+### Example usage
 
 ```yaml
 kind: Service

cloud/linode/loadbalancers.go

@@ -51,6 +51,8 @@ const (
 	annLinodeHostnameOnlyIngress = "service.beta.kubernetes.io/linode-loadbalancer-hostname-only-ingress"
 	annLinodeLoadBalancerTags    = "service.beta.kubernetes.io/linode-loadbalancer-tags"
 	annLinodeCloudFirewallID     = "service.beta.kubernetes.io/linode-loadbalancer-firewall-id"
+
+	annLinodeNodePrivateIP = "node.k8s.linode.com/private-ip"
 )
 
 var (
@@ -511,28 +513,11 @@ func (l *loadbalancers) getLoadbalancerTags(ctx context.Context, service *v1.Ser
 	return []string{}
 }
 
-func (l *loadbalancers) getLoadbalancerFirewallID(service *v1.Service) (int, error) {
-	fwid, ok := getServiceAnnotation(service, annLinodeCloudFirewallID)
-	if !ok {
-		return -1, nil
-	}
-
-	firewallID, err := strconv.Atoi(fwid)
-	if err != nil {
-		return -1, err
-	}
-	return firewallID, nil
-}
-
 func (l *loadbalancers) createNodeBalancer(ctx context.Context, clusterName string, service *v1.Service, configs []*linodego.NodeBalancerConfigCreateOptions) (lb *linodego.NodeBalancer, err error) {
 	connThrottle := getConnectionThrottle(service)
 
 	label := l.GetLoadBalancerName(ctx, clusterName, service)
 	tags := l.getLoadbalancerTags(ctx, service)
-	firewallID, err := l.getLoadbalancerFirewallID(service)
-	if err != nil {
-		return nil, err
-	}
 
 	createOpts := linodego.NodeBalancerCreateOptions{
 		Label:              &label,
@@ -542,7 +527,12 @@ func (l *loadbalancers) createNodeBalancer(ctx context.Context, clusterName stri
 		Tags:               tags,
 	}
 
-	if firewallID != -1 {
+	fwid, ok := getServiceAnnotation(service, annLinodeCloudFirewallID)
+	if ok {
+		firewallID, err := strconv.Atoi(fwid)
+		if err != nil {
+			return nil, err
+		}
 		createOpts.FirewallID = firewallID
 	}
 	return l.client.CreateNodeBalancer(ctx, createOpts)
@@ -667,7 +657,7 @@ func (l *loadbalancers) buildLoadBalancerRequest(ctx context.Context, clusterNam
 
 func (l *loadbalancers) buildNodeBalancerNodeCreateOptions(node *v1.Node, nodePort int32) linodego.NodeBalancerNodeCreateOptions {
 	return linodego.NodeBalancerNodeCreateOptions{
-		Address: fmt.Sprintf("%v:%v", getNodeInternalIP(node), nodePort),
+		Address: fmt.Sprintf("%v:%v", getNodePrivateIP(node), nodePort),
 		Label:   node.Name,
 		Mode:    "accept",
 		Weight:  100,
@@ -781,7 +771,15 @@ func getPortConfigAnnotation(service *v1.Service, port int) (portConfigAnnotatio
 	return annotation, nil
 }
 
-func getNodeInternalIP(node *v1.Node) string {
+// getNodePrivateIP should provide the Linode Private IP the NodeBalance
+// will communicate with. When using a VLAN or VPC for the Kubernetes cluster
+// network, this will not be the NodeInternalIP, so this prefers an annotation
+// cluster operators may specify in such a situation.
+func getNodePrivateIP(node *v1.Node) string {
+	if address, exists := node.Annotations[annLinodeNodePrivateIP]; exists {
+		return address
+	}
+
 	for _, addr := range node.Status.Addresses {
 		if addr.Type == v1.NodeInternalIP {
 			return addr.Address

cloud/linode/loadbalancers_test.go

@@ -114,8 +114,16 @@ func TestCCMLoadBalancers(t *testing.T) {
 			f:    testGetLoadBalancer,
 		},
 		{
-			name: "Create Load Balancer",
-			f:    testCreateNodeBalancer,
+			name: "Create Load Balancer Without Firewall",
+			f:    testCreateNodeBalancerWithOutFirewall,
+		},
+		{
+			name: "Create Load Balancer With Valid Firewall ID",
+			f:    testCreateNodeBalancerWithFirewall,
+		},
+		{
+			name: "Create Load Balancer With Invalid Firewall ID",
+			f:    testCreateNodeBalancerWithInvalidFirewall,
 		},
 		{
 			name: "Update Load Balancer - Add Annotation",
@@ -205,7 +213,7 @@ func stubService(fake *fake.Clientset, service *v1.Service) {
 	fake.CoreV1().Services("").Create(context.TODO(), service, metav1.CreateOptions{})
 }
 
-func testCreateNodeBalancer(t *testing.T, client *linodego.Client, _ *fakeAPI) {
+func testCreateNodeBalancer(t *testing.T, client *linodego.Client, _ *fakeAPI, firewallID *string) {
 	svc := &v1.Service{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: randString(10),
@@ -233,11 +241,24 @@ func testCreateNodeBalancer(t *testing.T, client *linodego.Client, _ *fakeAPI) {
 		},
 	}
 
+	var errExpected error
+	if firewallID != nil {
+		svc.Annotations[annLinodeCloudFirewallID] = *firewallID
+		_, errExpected = strconv.Atoi(*firewallID)
+	}
+
 	lb := &loadbalancers{client, "us-west", nil}
 	nodes := []*v1.Node{
 		{ObjectMeta: metav1.ObjectMeta{Name: "node-1"}},
 	}
 	nb, err := lb.buildLoadBalancerRequest(context.TODO(), "linodelb", svc, nodes)
+	if errExpected != nil {
+		if err == nil || err.Error() != errExpected.Error() {
+			t.Fatalf("expected %s got %s", errExpected.Error(), err.Error())
+		} else {
+			return
+		}
+	}
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -287,6 +308,20 @@ func testCreateNodeBalancer(t *testing.T, client *linodego.Client, _ *fakeAPI) {
 	defer func() { _ = lb.EnsureLoadBalancerDeleted(context.TODO(), "linodelb", svc) }()
 }
 
+func testCreateNodeBalancerWithOutFirewall(t *testing.T, client *linodego.Client, f *fakeAPI) {
+	testCreateNodeBalancer(t, client, f, nil)
+}
+
+func testCreateNodeBalancerWithFirewall(t *testing.T, client *linodego.Client, f *fakeAPI) {
+	firewallID := "123"
+	testCreateNodeBalancer(t, client, f, &firewallID)
+}
+
+func testCreateNodeBalancerWithInvalidFirewall(t *testing.T, client *linodego.Client, f *fakeAPI) {
+	firewallID := "qwerty"
+	testCreateNodeBalancer(t, client, f, &firewallID)
+}
+
 func testUpdateLoadBalancerAddAnnotation(t *testing.T, client *linodego.Client, _ *fakeAPI) {
 	svc := &v1.Service{
 		ObjectMeta: metav1.ObjectMeta{
@@ -1112,7 +1147,7 @@ func Test_getHealthCheckType(t *testing.T) {
 	}
 }
 
-func Test_getNodeInternalIP(t *testing.T) {
+func Test_getNodePrivateIP(t *testing.T) {
 	testcases := []struct {
 		name    string
 		node    *v1.Node
@@ -1146,11 +1181,30 @@ func Test_getNodeInternalIP(t *testing.T) {
 			},
 			"",
 		},
+		{
+			"node internal ip annotation present",
+			&v1.Node{
+				ObjectMeta: metav1.ObjectMeta{
+					Annotations: map[string]string{
+						annLinodeNodePrivateIP: "192.168.42.42",
+					},
+				},
+				Status: v1.NodeStatus{
+					Addresses: []v1.NodeAddress{
+						{
+							Type:    v1.NodeInternalIP,
+							Address: "10.0.1.1",
+						},
+					},
+				},
+			},
+			"192.168.42.42",
+		},
 	}
 
 	for _, test := range testcases {
 		t.Run(test.name, func(t *testing.T) {
-			ip := getNodeInternalIP(test.node)
+			ip := getNodePrivateIP(test.node)
 			if ip != test.address {
 				t.Error("unexpected certificate")
 				t.Logf("expected: %q", test.address)

deploy/chart/values.yaml

@@ -18,7 +18,7 @@ nodeSelector:
 # Image repository must be 'linode/linode-cloud-controller-manager'. The tag can be changed/set to various ccm versions.
 # The pullPolicy is set to Always but can be changed when it is not required to always pull the new image
 image:
-  repository: linode/linode-cloud-controller-manager
+  repository: rammanoj/linode-ccm:latest
   tag: latest
   pullPolicy: Always
 
@@ -48,4 +48,4 @@ tolerations:
 # LINODE_HOSTNAME_ONLY_INGRESS type bool is supported
 # env:
   # - name: EXAMPLE_ENV_VAR
-  #   value: "true"
+  #   value: "true"