kubemci list command: look for relevant global forwarding rules. #91

G-Harmon · 2017-12-06T00:47:50Z

This change is

k8s-ci-robot · 2017-12-06T00:47:57Z

Hi @G-Harmon. 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.

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.

G-Harmon · 2017-12-06T00:48:39Z

This can't be merged until kubernetes/ingress-gce#78 is merged,
AND we re-vendor that commit into this repo.

nikhiljindal · 2017-12-06T00:49:15Z

/ok-to-test

G-Harmon · 2017-12-06T01:25:40Z

The test failure is expected here, since it doesn't have the updates I made to ingress-gce.

nikhiljindal · 2017-12-09T00:33:49Z

app/kubemci/pkg/gcp/forwardingrule/fake_forwardingrulesyncer.go

@@ -67,3 +68,8 @@ func (f *FakeForwardingRuleSyncer) GetLoadBalancerStatus(lbName string) (*status
 	}
 	return nil, fmt.Errorf("load balancer %s does not exist", lbName)
 }
+
+func (f *FakeForwardingRuleSyncer) ListForwardingRules() (string, error) {
+	glog.Errorf("ListForwardingRules not implemented.")


Add a TODO to implement it?
Or just return the list of load balancer names from EnsuredForwardingRules?

done. (implemented)

nikhiljindal · 2017-12-09T01:12:12Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go

+		return "[No loadbalancers found]", nil
+	}
+	var result string = "List of multicluster loadbalancers created:\nName; IP; Clusters\n"
+	for _, item := range rules.Items {


not in this PR, but eventually we should reuse kubectl printers: https://github.com/kubernetes/kubernetes/blob/44f24d219f9d28121eb1f16863312f790e144882/pkg/printers/printers.go#L123:13

added a TODO.

nikhiljindal · 2017-12-09T01:13:44Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go

+	var result string = "List of multicluster loadbalancers created:\nName; IP; Clusters\n"
+	for _, item := range rules.Items {
+		if strings.HasPrefix(item.Name, "mci1") {
+			result += "Name: " + item.Name


This will print the forwarding rule name. User will expect to see the load balancer name (what they can use with create, delete, get-status)

nikhiljindal · 2017-12-09T01:15:47Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go

+
+		}
+	}
+	result = strings.Trim(result, "\n")


It can still happen that there were forwarding rules in the project, but none with mci1 prefix.
The output for no forwarding rules and that should be same.

good point, done.

nikhiljindal · 2017-12-09T01:18:11Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go

+			if lbStatus, err := status.FromString(item.Description); err != nil {
+				fmt.Println("Error decoding load balancer status:", err)
+			} else {
+				result += "\tIP: " + lbStatus.IPAddress


I would suggest splitting this into 2 methods:

One that lists the forwarding rules, checks the prefix, parses status and computes the list.

Second that formats the given list for printing. This can later be replaced by directly calling kubectl printer.

This will enable easy unit testing of the first method

Also to clarify, the public method will still be one - which will call these 2 private members

as we discussed, done.

nikhiljindal · 2017-12-09T01:19:10Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go

+		t.Fatalf("expected no error in ensuring forwarding rule, actual: %v", err)
+	}
+
+	list, err := frs.ListForwardingRules()


If we split the code as suggested above, I would love to see more test cases such as:

No forwarding rule exists

Forwarding rules exist but none with the right prefix

okay, I added those 2 test cases.

nikhiljindal · 2017-12-09T01:21:23Z

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go

@@ -383,7 +387,7 @@ PortLoop:
 	}

 	if port == nil {
-		return ingressbe.ServicePort{}, fmt.Errorf("could not find matching nodeport for backend %v and service %s/%s. Looking for port %v in %v", be, namespace, be.ServiceName, be.ServicePort, svc.Spec.Ports)
+		return ingressbe.ServicePort{}, fmt.Errorf("Could not find matching nodeport for backend %+v and service %s/%s. Looking for port %+v in %v", be, namespace, be.ServiceName, be.ServicePort, svc.Spec.Ports)


Libraries are supposed to return errors starting with smaller case so that the caller can prepend context

ah, I didn't realize. done.

nikhiljindal · 2017-12-09T01:26:45Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go

+func (s *ForwardingRuleSyncer) ListForwardingRules() (string, error) {
+	var rules *compute.ForwardingRuleList
+	var err error
+	if rules, err = s.frp.ListGlobalForwardingRules(); err != nil {


Depending on which PR goes in first, we will need to update this to handle HTTPS ingresses.
HTTPS ingresses create 2 forwarding rules: one for HTTP and one for HTTPS.

okay, added a TODO! let's see which one goes first.

nikhiljindal · 2017-12-09T01:29:31Z

app/kubemci/pkg/gcp/forwardingrule/interfaces.go

@@ -28,4 +28,6 @@ type ForwardingRuleSyncerInterface interface {
 	DeleteForwardingRules() error
 	// GetLoadBalancerStatus returns the struct describing the status of the given load balancer.
 	GetLoadBalancerStatus(lbName string) (*status.LoadBalancerStatus, error)
+	// ListForwardingRules returns all forwarding rules used by MCI.
+	ListForwardingRules() (string, error)


Comparing it with GetLoadBalancerStatus, I see that this assumes that the result is to be printed and hence formats it for printing. GetLoadBalancerStatus, on the other hand, just returns structured data and then its upto the caller on how to use it.

Wondering if we should do the same here?
(am still thinking about it, so feel free to push back or discuss before making changes)

nikhiljindal · 2017-12-09T01:32:39Z

Thanks @G-Harmon
looks great, main comment is about who should do formatting for printing.

Also needs rebase with #93

coveralls · 2017-12-12T01:56:12Z

Changes Unknown when pulling c0505dd on G-Harmon:mciList into ** on GoogleCloudPlatform:master**.

nikhiljindal · 2017-12-12T06:03:42Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go

+	for _, item := range rules.Items {
+		if strings.HasPrefix(item.Name, "mci1") {
+			if lbStatus, err := status.FromString(item.Description); err != nil {
+				fmt.Println("Error decoding load balancer status:", err)


Shouldnt we return this error as well?

nikhiljindal · 2017-12-12T06:04:35Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go

+	result := []status.LoadBalancerStatus{}
+	// TODO: When we have HTTPS ingresses, check for those as well.
+	if rules, err = s.frp.ListGlobalForwardingRules(); err != nil {
+		fmt.Println("Error getting global forwarding rules:", err)


Returned error and printed error should be same?

err = fmt.Errorf("Error getting global forwarding rules: %s", err) fmt.Println(err) return err

nikhiljindal · 2017-12-12T06:05:26Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go

+		fmt.Println("Error getting global forwarding rules:", err)
+		return result, err
+	}
+	// TODO: Should we check for nil rules?


What do you mean? Filter out nil rules?

nikhiljindal · 2017-12-12T06:06:17Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go

@@ -115,6 +115,106 @@ func TestEnsureHttpForwardingRule(t *testing.T) {
 	}
 }

+func TestListLoadBalancers(t *testing.T) {


You meant TestGetForwardingRules?

nikhiljindal · 2017-12-12T06:06:57Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go

+	}
+	for _, rule := range list {
+		if rule.LoadBalancerName != lbName {
+			t.Errorf("Bad name.")


"unexpected name, expected: %s, actual: %s"

nikhiljindal · 2017-12-12T06:10:16Z

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer_test.go

+		t.Errorf("unexpected error listing load balancers:%v", err)
+	}
+	if len(list) != 0 {
+		t.Errorf("Expected no fowarding rules. Actual:%v", list)


nit: add space after :

nikhiljindal · 2017-12-12T06:11:21Z

app/kubemci/pkg/gcp/forwardingrule/interfaces.go

@@ -28,4 +28,6 @@ type ForwardingRuleSyncerInterface interface {
 	DeleteForwardingRules() error
 	// GetLoadBalancerStatus returns the struct describing the status of the given load balancer.
 	GetLoadBalancerStatus(lbName string) (*status.LoadBalancerStatus, error)
+	// GetForwardingRules returns all forwarding rules used by MCI.
+	GetForwardingRules() ([]status.LoadBalancerStatus, error)


How about calling this ListLoadBalancerStatuses?
From GetForwardingRules, I expect it to return compute.ForwardingRule.

nikhiljindal · 2017-12-12T06:14:23Z

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go

@@ -219,6 +220,29 @@ func (l *LoadBalancerSyncer) PrintStatus() (string, error) {
 	return fmt.Sprintf("Load balancer %s has IPAddress %s and is spread across %d clusters (%s)", l.lbName, sd.IPAddress, len(sd.Clusters), strings.Join(sd.Clusters, ",")), nil
 }

+func formatForwardingRules(rules []status.LoadBalancerStatus) (string, error) {


how about calling this formatLoadBalancersList? This method does not know that load balancer statuses came from forwarding rules and hence should have nothing to do with forwarding rules.

nikhiljindal · 2017-12-12T06:14:30Z

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go

+	// TODO: Should reuse printers in kubernetes/pkg/printers/printers.go
+	var result string = "List of multicluster loadbalancers created:\nName; IP; Clusters\n"
+	for _, lbStatus := range rules {
+		fmt.Println("name:", lbStatus.LoadBalancerName)


unintended?

nikhiljindal · 2017-12-12T06:15:19Z

app/kubemci/pkg/gcp/loadbalancer/loadbalancersyncer.go

@@ -219,6 +220,29 @@ func (l *LoadBalancerSyncer) PrintStatus() (string, error) {
 	return fmt.Sprintf("Load balancer %s has IPAddress %s and is spread across %d clusters (%s)", l.lbName, sd.IPAddress, len(sd.Clusters), strings.Join(sd.Clusters, ",")), nil
 }

+func formatForwardingRules(rules []status.LoadBalancerStatus) (string, error) {
+	// TODO: Should reuse printers in kubernetes/pkg/printers/printers.go
+	var result string = "List of multicluster loadbalancers created:\nName; IP; Clusters\n"


call it multicluster ingresses instead of multicluster loadbalancers (since we call them mci's everywhere)?

G-Harmon · 2017-12-12T16:18:18Z

PTAL. sorry it was a little messy, I was a bit rushed last night. I wanted to get your review comments, so thanks for looking at it!

Review status: 0 of 10 files reviewed at latest revision, 12 unresolved discussions.

app/kubemci/pkg/gcp/forwardingrule/forwardingrulesyncer.go, line 133 at r1 (raw file):