Merge pull request #97 from nikhiljindal/httpsforwardingRule

Adding code to create an https forwarding rule

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

@@ -25,6 +25,7 @@ type FakeForwardingRule struct {
 	IPAddress string
 	TPLink    string
 	Clusters  []string
+	IsHTTPS   bool
 }
 
 type FakeForwardingRuleSyncer struct {
@@ -50,6 +51,17 @@ func (f *FakeForwardingRuleSyncer) EnsureHttpForwardingRule(lbName, ipAddress, t
 	return nil
 }
 
+func (f *FakeForwardingRuleSyncer) EnsureHttpsForwardingRule(lbName, ipAddress, targetProxyLink string, clusters []string, forceUpdate bool) error {
+	f.EnsuredForwardingRules = append(f.EnsuredForwardingRules, FakeForwardingRule{
+		LBName:    lbName,
+		IPAddress: ipAddress,
+		TPLink:    targetProxyLink,
+		Clusters:  clusters,
+		IsHTTPS:   true,
+	})
+	return nil
+}
+
 func (f *FakeForwardingRuleSyncer) DeleteForwardingRules() error {
 	f.EnsuredForwardingRules = nil
 	return nil

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

@@ -61,13 +61,25 @@ var _ ForwardingRuleSyncerInterface = &ForwardingRuleSyncer{}
 // Does nothing if it exists already, else creates a new one.
 // Stores the given list of clusters in the description field of forwarding rule to use it to generate status later.
 func (s *ForwardingRuleSyncer) EnsureHttpForwardingRule(lbName, ipAddress, targetProxyLink string, clusters []string, forceUpdate bool) error {
-	fmt.Println("Ensuring http forwarding rule")
-	desiredFR, err := s.desiredForwardingRule(lbName, ipAddress, targetProxyLink, clusters)
+	s.namer.HttpForwardingRuleName()
+	return s.ensureForwardingRule(lbName, ipAddress, targetProxyLink, httpDefaultPortRange, "http", s.namer.HttpForwardingRuleName(), clusters, forceUpdate)
+}
+
+// EnsureHttpsForwardingRule ensures that the required https forwarding rule exists.
+// Does nothing if it exists already, else creates a new one.
+// Stores the given list of clusters in the description field of forwarding rule to use it to generate status later.
+func (s *ForwardingRuleSyncer) EnsureHttpsForwardingRule(lbName, ipAddress, targetProxyLink string, clusters []string, forceUpdate bool) error {
+	return s.ensureForwardingRule(lbName, ipAddress, targetProxyLink, httpsDefaultPortRange, "https", s.namer.HttpsForwardingRuleName(), clusters, forceUpdate)
+}
+
+// ensureForwardingRule ensures a forwarding rule exists as per the given input parameters.
+func (s *ForwardingRuleSyncer) ensureForwardingRule(lbName, ipAddress, targetProxyLink, portRange, httpProtocol, name string, clusters []string, forceUpdate bool) error {
+	fmt.Println("Ensuring", httpProtocol, "forwarding rule")
+	desiredFR, err := s.desiredForwardingRule(lbName, ipAddress, targetProxyLink, portRange, httpProtocol, name, clusters)
 	if err != nil {
 		fmt.Println("Error getting desired forwarding rule:", err)
 		return err
 	}
-	name := desiredFR.Name
 	// Check if forwarding rule already exists.
 	existingFR, err := s.frp.GetGlobalForwardingRule(name)
 	if err == nil {
@@ -94,30 +106,60 @@ func (s *ForwardingRuleSyncer) EnsureHttpForwardingRule(lbName, ipAddress, targe
 }
 
 func (s *ForwardingRuleSyncer) DeleteForwardingRules() error {
-	// TODO(nikhiljindal): Also delete the https forwarding rule when we start creating it.
+	var err error
 	name := s.namer.HttpForwardingRuleName()
-	fmt.Println("Deleting forwarding rule", name)
-	err := s.frp.DeleteGlobalForwardingRule(name)
-	if err != nil {
-		fmt.Println("error", err, "in deleting forwarding rule", name)
-		return err
+	fmt.Println("Deleting http forwarding rule", name)
+	httpErr := s.frp.DeleteGlobalForwardingRule(name)
+	if httpErr != nil {
+		if utils.IsHTTPErrorCode(httpErr, http.StatusNotFound) {
+			fmt.Println("Http forwarding rule", name, "does not exist. Nothing to delete")
+		} else {
+			httpErr := fmt.Errorf("error %s in deleting http forwarding rule %s", httpErr, name)
+			fmt.Println(httpErr)
+			err = multierror.Append(err, httpErr)
+		}
+	} else {
+		fmt.Println("Http forwarding rule", name, "deleted successfully")
 	}
-	fmt.Println("forwarding rule", name, "deleted successfully")
-	return nil
+	name = s.namer.HttpsForwardingRuleName()
+	fmt.Println("Deleting https forwarding rule", name)
+	httpsErr := s.frp.DeleteGlobalForwardingRule(name)
+	if httpsErr != nil {
+		if utils.IsHTTPErrorCode(httpsErr, http.StatusNotFound) {
+			fmt.Println("Https forwarding rule", name, "does not exist. Nothing to delete")
+		} else {
+			httpsErr := fmt.Errorf("error %s in deleting https forwarding rule %s", httpsErr, name)
+			fmt.Println(httpsErr)
+			err = multierror.Append(err, httpsErr)
+		}
+	} else {
+		fmt.Println("Https forwarding rule", name, "deleted successfully")
+	}
+	return err
 }
 
 func (s *ForwardingRuleSyncer) GetLoadBalancerStatus(lbName string) (*status.LoadBalancerStatus, error) {
 	// Fetch the http forwarding rule.
-	// TODO(nikhiljindal): Try fetching the https rule as well, once we start creating them.
-	name := s.namer.HttpForwardingRuleName()
-	fr, err := s.frp.GetGlobalForwardingRule(name)
-	if utils.IsHTTPErrorCode(err, http.StatusNotFound) {
+	httpName := s.namer.HttpForwardingRuleName()
+	httpFr, httpErr := s.frp.GetGlobalForwardingRule(httpName)
+	if httpErr == nil {
+		return getStatus(httpFr)
+	}
+	// Try fetching https forwarding rule.
+	// Ingresses with http-only annotation will not have a http forwarding rule.
+	httpsName := s.namer.HttpsForwardingRuleName()
+	httpsFr, httpsErr := s.frp.GetGlobalForwardingRule(httpsName)
+	if httpsErr == nil {
+		return getStatus(httpsFr)
+	}
+	if utils.IsHTTPErrorCode(httpErr, http.StatusNotFound) && utils.IsHTTPErrorCode(httpsErr, http.StatusNotFound) {
 		// We assume the load balancer does not exist until the forwarding rule exists.
 		return nil, fmt.Errorf("Load balancer %s does not exist", lbName)
 	}
-	if err != nil {
-		return nil, fmt.Errorf("error in fetching forwarding rule: %s. Cannot determine status without it.", err)
-	}
+	return nil, fmt.Errorf("error in fetching http forwarding rule: %s, error in fetching https forwarding rule: %s. Cannot determine status without forwarding rule", httpErr, httpsErr)
+}
+
+func getStatus(fr *compute.ForwardingRule) (*status.LoadBalancerStatus, error) {
 	status, err := status.FromString(fr.Description)
 	if err != nil {
 		return nil, fmt.Errorf("error in parsing forwarding rule description %s. Cannot determine status without it.", err)
@@ -210,11 +252,11 @@ func forwardingRuleMatches(desiredFR, existingFR *compute.ForwardingRule) bool {
 	return equal
 }
 
-func (s *ForwardingRuleSyncer) desiredForwardingRule(lbName, ipAddress, targetProxyLink string, clusters []string) (*compute.ForwardingRule, error) {
+func (s *ForwardingRuleSyncer) desiredForwardingRule(lbName, ipAddress, targetProxyLink, portRange, httpProtocol, name string, clusters []string) (*compute.ForwardingRule, error) {
 	// Sort the clusters so we get a deterministic order.
 	sort.Strings(clusters)
 	status := status.LoadBalancerStatus{
-		Description:      fmt.Sprintf("Http forwarding rule for kubernetes multicluster loadbalancer %s", lbName),
+		Description:      fmt.Sprintf("%s forwarding rule for kubernetes multicluster loadbalancer %s", httpProtocol, lbName),
 		LoadBalancerName: lbName,
 		Clusters:         clusters,
 		IPAddress:        ipAddress,
@@ -225,11 +267,11 @@ func (s *ForwardingRuleSyncer) desiredForwardingRule(lbName, ipAddress, targetPr
 	}
 	// Compute the desired forwarding rule.
 	return &compute.ForwardingRule{
-		Name:                s.namer.HttpForwardingRuleName(),
+		Name:                name,
 		Description:         desc,
 		IPAddress:           ipAddress,
 		Target:              targetProxyLink,
-		PortRange:           httpDefaultPortRange,
+		PortRange:           portRange,
 		IPProtocol:          "TCP",
 		LoadBalancingScheme: "EXTERNAL",
 	}, nil

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

@@ -79,7 +79,6 @@ func TestEnsureHttpForwardingRule(t *testing.T) {
 		clusters := []string{"cluster1", "cluster2"}
 		// Should create the forwarding rule as expected.
 		namer := utilsnamer.NewNamer("mci1", lbName)
-		frName := namer.HttpForwardingRuleName()
 		frs := NewForwardingRuleSyncer(namer, frp)
 		err := frs.EnsureHttpForwardingRule(lbName, c.ipAddr, tpLink, clusters, c.forceUpdate)
 		if (err != nil) != c.ensureErr {
@@ -100,6 +99,101 @@ func TestEnsureHttpForwardingRule(t *testing.T) {
 		if fr.Target != tpLink {
 			t.Errorf("unexpected target proxy link. expected: %s, got: %s", tpLink, fr.Target)
 		}
+		if fr.PortRange != httpDefaultPortRange {
+			t.Errorf("unexpected port range: %s, expected: %s", fr.PortRange, httpDefaultPortRange)
+		}
+		expectedDescr := status.LoadBalancerStatus{
+			LoadBalancerName: lbName,
+			Clusters:         clusters,
+			IPAddress:        c.expectedIpAddr,
+		}
+		status, err := status.FromString(fr.Description)
+		if err != nil {
+			t.Errorf("unexpected error %s in unmarshalling forwarding rule description %v", err, fr.Description)
+		}
+		if status.LoadBalancerName != expectedDescr.LoadBalancerName || !reflect.DeepEqual(status.Clusters, expectedDescr.Clusters) || status.IPAddress != expectedDescr.IPAddress {
+			t.Errorf("unexpected description: %v, expected: %v", status, expectedDescr)
+		}
+	}
+}
+
+func TestEnsureHttpsForwardingRule(t *testing.T) {
+	lbName := "lb-name"
+	namer := utilsnamer.NewNamer("mci1", lbName)
+	frName := namer.HttpsForwardingRuleName()
+	frp := ingresslb.NewFakeLoadBalancers("" /*name*/, nil /*namer*/)
+	// GET should return NotFound.
+	if _, err := frp.GetGlobalForwardingRule(frName); err == nil {
+		t.Errorf("expected NotFound error, got nil.")
+	}
+	testCases := []struct {
+		desc string
+		// In's
+		ipAddr      string
+		forceUpdate bool
+		// Out's
+		ensureErr      bool
+		expectedIpAddr string
+	}{
+		{
+			desc:           "initial write (force=false)",
+			ipAddr:         "1.2.3.4",
+			forceUpdate:    false,
+			ensureErr:      false,
+			expectedIpAddr: "1.2.3.4",
+		},
+		{
+			desc:           "write same (force=false)",
+			ipAddr:         "1.2.3.4",
+			forceUpdate:    false,
+			ensureErr:      false,
+			expectedIpAddr: "1.2.3.4",
+		},
+		{
+			desc:           "write different (force=false)",
+			ipAddr:         "2.2.2.2",
+			forceUpdate:    false,
+			ensureErr:      true,
+			expectedIpAddr: "1.2.3.4",
+		},
+		{
+			desc:           "write different (force=true)",
+			ipAddr:         "2.2.2.2",
+			forceUpdate:    true,
+			ensureErr:      false,
+			expectedIpAddr: "2.2.2.2",
+		},
+	}
+	// We want the load balancers to exist across test cases.
+	for _, c := range testCases {
+		glog.Infof("===============testing case: %s=================", c.desc)
+		tpLink := "fakeLink"
+		clusters := []string{"cluster1", "cluster2"}
+		// Should create the forwarding rule as expected.
+		namer := utilsnamer.NewNamer("mci1", lbName)
+		frs := NewForwardingRuleSyncer(namer, frp)
+		err := frs.EnsureHttpsForwardingRule(lbName, c.ipAddr, tpLink, clusters, c.forceUpdate)
+		if (err != nil) != c.ensureErr {
+			t.Errorf("in ensuring forwarding rule, expected err? %v, actual: %v", c.ensureErr, err)
+		}
+		if c.ensureErr {
+			glog.Infof("Skipping validation for this test case.")
+			continue
+		}
+		// Verify that GET does not return NotFound and the returned forwarding rule is as expected.
+		fr, err := frp.GetGlobalForwardingRule(frName)
+		if err != nil {
+			t.Errorf("expected nil error, actual: %v", err)
+		}
+		if fr.IPAddress != c.expectedIpAddr {
+			t.Errorf("unexpected ip address. expected: %s, got: %s", c.expectedIpAddr, fr.IPAddress)
+		}
+		if fr.Target != tpLink {
+			t.Errorf("unexpected target proxy link. expected: %s, got: %s", tpLink, fr.Target)
+		}
+		if fr.PortRange != httpsDefaultPortRange {
+			t.Errorf("unexpected port range: %s, expected: %s", fr.PortRange, httpsDefaultPortRange)
+		}
 		expectedDescr := status.LoadBalancerStatus{
 			LoadBalancerName: lbName,
 			Clusters:         clusters,
@@ -219,23 +313,33 @@ func TestDeleteForwardingRule(t *testing.T) {
 	lbName := "lb-name"
 	ipAddr := "1.2.3.4"
 	tpLink := "fakeLink"
-	// Should create the forwarding rule as expected.
 	frp := ingresslb.NewFakeLoadBalancers("" /*name*/, nil /*namer*/)
 	namer := utilsnamer.NewNamer("mci1", lbName)
-	frName := namer.HttpForwardingRuleName()
+	httpFrName := namer.HttpForwardingRuleName()
+	httpsFrName := namer.HttpsForwardingRuleName()
 	frs := NewForwardingRuleSyncer(namer, frp)
+	// Create both the http and https forwarding rules and verify that it deletes both.
 	if err := frs.EnsureHttpForwardingRule(lbName, ipAddr, tpLink, []string{}, false /*force*/); err != nil {
-		t.Fatalf("expected no error in ensuring forwarding rule, actual: %v", err)
+		t.Fatalf("expected no error in ensuring http forwarding rule, actual: %v", err)
+	}
+	if err := frs.EnsureHttpsForwardingRule(lbName, ipAddr, tpLink, []string{}, false /*force*/); err != nil {
+		t.Fatalf("expected no error in ensuring https forwarding rule, actual: %v", err)
+	}
+	if _, err := frp.GetGlobalForwardingRule(httpFrName); err != nil {
+		t.Fatalf("expected nil error in getting http forwarding rule, actual: %v", err)
 	}
-	if _, err := frp.GetGlobalForwardingRule(frName); err != nil {
-		t.Fatalf("expected nil error, actual: %v", err)
+	if _, err := frp.GetGlobalForwardingRule(httpsFrName); err != nil {
+		t.Fatalf("expected nil error in getting https forwarding rule, actual: %v", err)
 	}
 	// GET should return NotFound after DELETE.
 	if err := frs.DeleteForwardingRules(); err != nil {
 		t.Fatalf("unexpeted error in deleting forwarding rule: %s", err)
 	}
-	if _, err := frp.GetGlobalForwardingRule(frName); err == nil {
-		t.Fatalf("expected not found error, actual: nil")
+	if _, err := frp.GetGlobalForwardingRule(httpFrName); err == nil {
+		t.Fatalf("expected not found error in getting http forwarding rule, actual: nil")
+	}
+	if _, err := frp.GetGlobalForwardingRule(httpsFrName); err == nil {
+		t.Fatalf("expected not found error in getting https forwarding rule, actual: nil")
 	}
 }
 
@@ -249,21 +353,71 @@ func TestGetLoadBalancerStatus(t *testing.T) {
 	frp := ingresslb.NewFakeLoadBalancers("" /*name*/, nil /*namer*/)
 	namer := utilsnamer.NewNamer("mci1", lbName)
 	frs := NewForwardingRuleSyncer(namer, frp)
-	if err := frs.EnsureHttpForwardingRule(lbName, ipAddr, tpLink, clusters, false /*force*/); err != nil {
-		t.Fatalf("expected no error in ensuring forwarding rule, actual: %v", err)
-	}
-	status, err := frs.GetLoadBalancerStatus(lbName)
-	if err != nil {
-		t.Fatalf("unexpected error in getting status description for load balancer %s", lbName)
-	}
-	// Verify that status description has the expected values set.
-	if status.LoadBalancerName != lbName {
-		t.Errorf("unexpected load balancer name, expected: %s, got: %s", lbName, status.LoadBalancerName)
-	}
-	if !reflect.DeepEqual(status.Clusters, clusters) {
-		t.Errorf("unexpected list of clusters, expected: %v, got: %v", clusters, status.Clusters)
+	testCases := []struct {
+		// Should the http forwarding rule be created.
+		http bool
+		// Should the https forwarding rule be created.
+		https bool
+		// Is get status expected to return an error.
+		shouldErr bool
+	}{
+		{
+			http:      true,
+			https:     false,
+			shouldErr: false,
+		},
+		{
+			http:      false,
+			https:     true,
+			shouldErr: false,
+		},
+		{
+			http:      true,
+			https:     true,
+			shouldErr: false,
+		},
+		{
+			http:      false,
+			https:     false,
+			shouldErr: true,
+		},
 	}
-	if status.IPAddress != ipAddr {
-		t.Errorf("unpexpected ip address, expected: %s, got: %s", status.IPAddress, ipAddr)
+	for i, c := range testCases {
+		glog.Infof("===============testing case: %d=================", i)
+		if c.http {
+			// Ensure http forwarding rule.
+			if err := frs.EnsureHttpForwardingRule(lbName, ipAddr, tpLink, clusters, false /*force*/); err != nil {
+				t.Errorf("expected no error in ensuring http forwarding rule, actual: %v", err)
+			}
+		}
+		if c.https {
+			// Ensure https forwarding rule.
+			if err := frs.EnsureHttpsForwardingRule(lbName, ipAddr, tpLink, clusters, false /*force*/); err != nil {
+				t.Errorf("expected no error in ensuring https forwarding rule, actual: %v", err)
+			}
+		}
+		status, err := frs.GetLoadBalancerStatus(lbName)
+		if c.shouldErr != (err != nil) {
+			t.Errorf("unexpected error in getting status description for load balancer %s, expected err != nil: %v, actual err: %s", lbName, c.shouldErr, err)
+		}
+		if c.shouldErr {
+			continue
+		}
+		// Verify that status description has the expected values set.
+		if status.LoadBalancerName != lbName {
+			t.Errorf("unexpected load balancer name, expected: %s, got: %s", lbName, status.LoadBalancerName)
+		}
+		if !reflect.DeepEqual(status.Clusters, clusters) {
+			t.Errorf("unexpected list of clusters, expected: %v, got: %v", clusters, status.Clusters)
+		}
+		if status.IPAddress != ipAddr {
+			t.Errorf("unpexpected ip address, expected: %s, got: %s", status.IPAddress, ipAddr)
+		}
+		// Delete the forwarding rules if we created at least one.
+		if c.http || c.https {
+			if err := frs.DeleteForwardingRules(); err != nil {
+				t.Errorf("unexpeted error in deleting forwarding rules: %s", err)
+			}
+		}
 	}
 }