improve parsing and handling of ServiceControlUrl (#800)

- Support parsing IPv6 addresses.
- Remove use of global variable: ServiceControlUrl should be parsed in `ServiceInfo`, not `cluster_generator`.

Signed-off-by: Teju Nareddy <[email protected]>

examples/grpc_dynamic_routing/envoy_config.json

@@ -347,7 +347,7 @@
                         "serviceControlUri": {
                           "cluster": "service-control-cluster",
                           "timeout": "30s",
-                          "uri": "https://servicecontrol.googleapis.com/v1/services"
+                          "uri": "https://servicecontrol.googleapis.com:443/v1/services"
                         },
                         "services": [
                           {

examples/service_control/envoy_config.json

@@ -164,7 +164,7 @@
                         "serviceControlUri": {
                           "cluster": "service-control-cluster",
                           "timeout": "30s",
-                          "uri": "https://servicecontrol.googleapis.com/v1/services"
+                          "uri": "https://servicecontrol.googleapis.com:443/v1/services"
                         },
                         "services": [
                           {

src/go/configgenerator/cluster_generator.go

@@ -16,6 +16,7 @@ package configgenerator
 
 import (
 	"fmt"
+	"strconv"
 	"time"
 
 	"github.com/GoogleCloudPlatform/esp-v2/src/go/options"
@@ -30,7 +31,6 @@ import (
 )
 
 // MakeClusters provides dynamic cluster settings for Envoy
-// This must be called before MakeListeners.
 func MakeClusters(serviceInfo *sc.ServiceInfo) ([]*clusterpb.Cluster, error) {
 	var clusters []*clusterpb.Cluster
 	backendCluster, err := makeLocalBackendCluster(serviceInfo)
@@ -79,9 +79,6 @@ func MakeClusters(serviceInfo *sc.ServiceInfo) ([]*clusterpb.Cluster, error) {
 		clusters = append(clusters, iamCluster)
 	}
 
-	// Note: makeServiceControlCluster should be called before makeListener
-	// as makeServiceControlFilter is using m.serviceControlURI assigned by
-	// makeServiceControlCluster
 	scCluster, err := makeServiceControlCluster(serviceInfo)
 	if err != nil {
 		return nil, err
@@ -346,37 +343,23 @@ func makeLocalBackendCluster(serviceInfo *sc.ServiceInfo) (*clusterpb.Cluster, e
 }
 
 func makeServiceControlCluster(serviceInfo *sc.ServiceInfo) (*clusterpb.Cluster, error) {
-	uri := serviceControlURL(serviceInfo, serviceInfo.Options)
-	if uri == "" {
-		return nil, nil
-	}
-
-	// The assumption about control.environment field. Its format:
-	//   [scheme://] +  host + [:port]
-	// * It should not have any path part
-	// * If scheme is missed, https is the default
-
-	scheme, hostname, port, path, err := util.ParseURI(uri)
+	port, err := strconv.Atoi(serviceInfo.ServiceControlURI.Port())
 	if err != nil {
-		return nil, err
-	}
-	if path != "" {
-		return nil, fmt.Errorf("error parsing service control URI: should not have path part: %s, %s", uri, path)
+		return nil, fmt.Errorf("failed to parse port from url %+v: %v", serviceInfo.ServiceControlURI, err)
 	}
 
 	connectTimeoutProto := ptypes.DurationProto(5 * time.Second)
-	serviceInfo.ServiceControlURI = scheme + "://" + hostname + "/v1/services"
 	c := &clusterpb.Cluster{
 		Name:                 util.ServiceControlClusterName,
 		LbPolicy:             clusterpb.Cluster_ROUND_ROBIN,
 		ConnectTimeout:       connectTimeoutProto,
 		DnsLookupFamily:      clusterpb.Cluster_V4_ONLY,
-		ClusterDiscoveryType: &clusterpb.Cluster_Type{clusterpb.Cluster_LOGICAL_DNS},
-		LoadAssignment:       util.CreateLoadAssignment(hostname, port),
+		ClusterDiscoveryType: &clusterpb.Cluster_Type{Type: clusterpb.Cluster_LOGICAL_DNS},
+		LoadAssignment:       util.CreateLoadAssignment(serviceInfo.ServiceControlURI.Hostname(), uint32(port)),
 	}
 
-	if scheme == "https" {
-		transportSocket, err := util.CreateUpstreamTransportSocket(hostname, serviceInfo.Options.SslSidestreamClientRootCertsPath, "", nil, "")
+	if serviceInfo.ServiceControlURI.Scheme == "https" {
+		transportSocket, err := util.CreateUpstreamTransportSocket(serviceInfo.ServiceControlURI.Hostname(), serviceInfo.Options.SslSidestreamClientRootCertsPath, "", nil, "")
 		if err != nil {
 			return nil, fmt.Errorf("error marshaling tls context to transport_socket config for cluster %s, err=%v",
 				c.Name, err)
@@ -387,14 +370,6 @@ func makeServiceControlCluster(serviceInfo *sc.ServiceInfo) (*clusterpb.Cluster,
 	return c, nil
 }
 
-func serviceControlURL(serviceInfo *sc.ServiceInfo, opts options.ConfigGeneratorOptions) string {
-	if uri := opts.ServiceControlURL; uri != "" {
-		// Ignore value from ServiceConfig if flag is set
-		return uri
-	}
-	return serviceInfo.ServiceConfig().GetControl().GetEnvironment()
-}
-
 func makeRemoteBackendClusters(serviceInfo *sc.ServiceInfo) ([]*clusterpb.Cluster, error) {
 	var brClusters []*clusterpb.Cluster
 

src/go/configgenerator/filtergen/service_control.go

@@ -121,7 +121,7 @@ func (g *ServiceControlGenerator) GenFilterConfig(serviceInfo *ci.ServiceInfo) (
 		Services:        []*scpb.Service{service},
 		ScCallingConfig: makeServiceControlCallingConfig(serviceInfo.Options),
 		ServiceControlUri: &commonpb.HttpUri{
-			Uri:     serviceInfo.ServiceControlURI,
+			Uri:     serviceInfo.ServiceControlURI.String() + "/v1/services",
 			Cluster: util.ServiceControlClusterName,
 			Timeout: ptypes.DurationProto(serviceInfo.Options.HttpRequestTimeout),
 		},

src/go/configinfo/service_info.go

@@ -17,6 +17,7 @@ package configinfo
 import (
 	"fmt"
 	"math"
+	"net/url"
 	"strconv"
 	"strings"
 	"time"
@@ -57,9 +58,8 @@ type ServiceInfo struct {
 	// Stores all the query parameters to be ignored for json-grpc transcoder.
 	AllTranscodingIgnoredQueryParams map[string]bool
 
-	AllowCors bool
-	// TODO(nareddyt): Fix immutability, this var is updated by consumers
-	ServiceControlURI string
+	AllowCors         bool
+	ServiceControlURI url.URL
 	// TODO(nareddyt): Fix immutability, this var is updated by consumers
 	GcpAttributes *scpb.GcpAttributes
 	// Keep a pointer to original service config. Should always process rules
@@ -163,6 +163,9 @@ func NewServiceInfoFromServiceConfig(serviceConfig *confpb.Service, id string, o
 	if err := serviceInfo.processAuthRequirement(); err != nil {
 		return nil, err
 	}
+	if err := serviceInfo.processServiceControlURL(); err != nil {
+		return nil, err
+	}
 
 	return serviceInfo, nil
 }
@@ -427,6 +430,7 @@ func (s *ServiceInfo) processHttpRule() error {
 		if err != nil {
 			return fmt.Errorf("error processing http rule for operation (%v): %v", rule.Selector, err)
 		}
+
 		if err := s.addHttpRule(method, rule, addedRouteMatchWithOptionsSet, s.Options.DisallowColonInWildcardPathSegment); err != nil {
 			return err
 		}
@@ -698,9 +702,7 @@ func (s *ServiceInfo) processAllBackends() error {
 		backendInfo := method.BackendInfo
 		if backendInfo == nil {
 			return fmt.Errorf("all the methods should have an un-empty BackendInfo")
-
 		}
-
 		if err := s.applyGlobalBackendInfoOverride(backendInfo); err != nil {
 			return err
 		}
@@ -984,10 +986,6 @@ func (s *ServiceInfo) LocalBackendClusterName() string {
 	return util.BackendClusterName(fmt.Sprintf("%s_local", s.Name))
 }
 
-func (s *ServiceInfo) LocalHttpBackendClusterName() string {
-	return util.BackendClusterName(fmt.Sprintf("%s_local_http", s.Name))
-}
-
 func (s *ServiceInfo) shouldSkipDiscoveryAPI(operation string) bool {
 	return IsDiscoveryAPI(operation) && !s.Options.AllowDiscoveryAPIs
 }
@@ -1042,3 +1040,34 @@ func parseRetriableStatusCodes(statusCodes string) ([]uint32, error) {
 	}
 	return codes, nil
 }
+
+func (s *ServiceInfo) processServiceControlURL() error {
+	uri := getServiceControlURL(s.serviceConfig, s.Options)
+	if uri == "" {
+		return nil
+	}
+
+	// The assumption about control.environment field. Its format:
+	//   [scheme://] +  host + [:port]
+	// * It should not have any path part
+	// * If scheme is missed, https is the default
+
+	url, err := util.ParseURIIntoURL(uri)
+	if err != nil {
+		return fmt.Errorf("failed to parse uri %q into url: %v", uri, err)
+	}
+	if url.Path != "" {
+		return fmt.Errorf("error parsing service control url %+v: should not have path part: %s", url, url.Path)
+	}
+	s.ServiceControlURI = url
+	return nil
+}
+
+func getServiceControlURL(serviceConfig *confpb.Service, opts options.ConfigGeneratorOptions) string {
+	// Ignore value from ServiceConfig if flag is set
+	if uri := opts.ServiceControlURL; uri != "" {
+		return uri
+	}
+
+	return serviceConfig.GetControl().GetEnvironment()
+}

src/go/configinfo/service_info_test.go

@@ -19,6 +19,7 @@ import (
 	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"reflect"
 	"sort"
 	"strings"
@@ -3991,6 +3992,113 @@ func TestProcessUsageRule(t *testing.T) {
 	}
 }
 
+func TestProcessServiceControlURL(t *testing.T) {
+	testData := []struct {
+		desc                  string
+		serviceConfigIn       *confpb.Service
+		optionsIn             options.ConfigGeneratorOptions
+		wantServiceControlURI url.URL
+	}{
+		{
+			desc: "URL from service config by default",
+			serviceConfigIn: &confpb.Service{
+				Control: &confpb.Control{
+					Environment: "https://staging-servicecontrol.sandbox.googleapis.com",
+				},
+			},
+			wantServiceControlURI: url.URL{
+				Scheme: "https",
+				Host:   "staging-servicecontrol.sandbox.googleapis.com:443",
+			},
+		},
+		{
+			desc: "option overrides service config",
+			serviceConfigIn: &confpb.Service{
+				Control: &confpb.Control{
+					// not used due to non-empty option
+					Environment: "https://staging-servicecontrol.sandbox.googleapis.com",
+				},
+			},
+			optionsIn: options.ConfigGeneratorOptions{
+				ServiceControlURL: "https://servicecontrol.googleapis.com",
+			},
+			wantServiceControlURI: url.URL{
+				Scheme: "https",
+				Host:   "servicecontrol.googleapis.com:443",
+			},
+		},
+		{
+			desc:                  "Empty inputs results in empty URL",
+			serviceConfigIn:       &confpb.Service{},
+			wantServiceControlURI: url.URL{},
+		},
+	}
+
+	for _, tc := range testData {
+		t.Run(tc.desc, func(t *testing.T) {
+			// Fill in required fields not relevant to test.
+			tc.serviceConfigIn.Apis = []*apipb.Api{
+				{
+					Name: testApiName,
+				},
+			}
+
+			serviceInfo, err := NewServiceInfoFromServiceConfig(tc.serviceConfigIn, testConfigID, tc.optionsIn)
+			if err != nil {
+				t.Fatalf("processServiceControlURL(...) has wrong error, got: %v, want no error", err)
+			}
+
+			if diff := cmp.Diff(tc.wantServiceControlURI, serviceInfo.ServiceControlURI); diff != "" {
+				t.Errorf("processServiceControlURL(...) has unexpected diff for ServiceControlURI (-want +got):\n%s", diff)
+			}
+		})
+	}
+}
+
+func TestProcessServiceControlURL_BadInput(t *testing.T) {
+	testData := []struct {
+		desc            string
+		serviceConfigIn *confpb.Service
+		optionsIn       options.ConfigGeneratorOptions
+		wantErr         string
+	}{
+		{
+			desc: "url parsing fails",
+			serviceConfigIn: &confpb.Service{
+				Control: &confpb.Control{
+					Environment: "https://[::1:80",
+				},
+			},
+			wantErr: `parse "https://[::1:80": missing ']' in host`,
+		},
+		{
+			desc: "url should not have path segment",
+			serviceConfigIn: &confpb.Service{
+				Control: &confpb.Control{
+					Environment: "https://servicecontrol.googleapis.com/v1/services",
+				},
+			},
+			wantErr: `should not have path part: /v1/services`,
+		},
+	}
+
+	for _, tc := range testData {
+		t.Run(tc.desc, func(t *testing.T) {
+			// Fill in required fields not relevant to test.
+			tc.serviceConfigIn.Apis = []*apipb.Api{
+				{
+					Name: testApiName,
+				},
+			}
+
+			_, err := NewServiceInfoFromServiceConfig(tc.serviceConfigIn, testConfigID, tc.optionsIn)
+			if err == nil || !strings.Contains(err.Error(), tc.wantErr) {
+				t.Fatalf("processServiceControlURL(...) has wrong error, got: %v, want: %q", err, tc.wantErr)
+			}
+		})
+	}
+}
+
 func parseUriTemplate(input string) *httppattern.UriTemplate {
 	u, _ := httppattern.ParseUriTemplate(input)
 	return u

src/go/configmanager/testdata/test_fetch_listeners.go

@@ -1621,7 +1621,7 @@ var (
                   "serviceControlUri": {
                     "cluster": "service-control-cluster",
                     "timeout": "30s",
-                    "uri": "https://servicecontrol.googleapis.com/v1/services"
+                    "uri": "https://servicecontrol.googleapis.com:443/v1/services"
                   },
                   "services": [
                     {
@@ -2489,7 +2489,7 @@ var (
                   "serviceControlUri": {
                     "cluster": "service-control-cluster",
                     "timeout": "30s",
-                    "uri": "https://servicecontrol.googleapis.com/v1/services"
+                    "uri": "https://servicecontrol.googleapis.com:443/v1/services"
                   },
                   "services": [
                     {

src/go/util/url_util.go

@@ -75,6 +75,31 @@ func ParseURI(uri string) (string, string, uint32, string, error) {
 	return u.Scheme, u.Hostname(), uint32(portVal), pathNoTrailingSlash, nil
 }
 
+// ParseURIIntoURL is the same as ParseURI, but it returns the URL in a
+// standard struct.
+func ParseURIIntoURL(uri string) (url.URL, error) {
+	scheme, hostname, port, path, err := ParseURI(uri)
+	if err != nil {
+		return url.URL{}, err
+	}
+
+	host := fmt.Sprintf("%v:%v", hostname, port)
+	if isIpv6Hostname(hostname) {
+		// IPv6 hostname should be embraced by brackets
+		host = fmt.Sprintf("[%v]:%d", hostname, port)
+	}
+
+	return url.URL{
+		Scheme: scheme,
+		Host:   host,
+		Path:   path,
+	}, nil
+}
+
+func isIpv6Hostname(hostname string) bool {
+	return strings.Contains(hostname, ":")
+}
+
 // ParseBackendProtocol parses a scheme string and http protocol string into BackendProtocol and UseTLS bool.
 func ParseBackendProtocol(scheme string, httpProtocol string) (BackendProtocol, bool, error) {
 	scheme = strings.ToLower(scheme)