Fix: fronting_provider_id omitted from proxy broker requests

- Related test case was also broken

- Refactor: now metrics such as fronting_provider_id are sent through
  MetricsForBrokerRequests, and bundled into requests within the `inproxy`
  package in both the client and proxy cases

psiphon/common/api.go

@@ -24,6 +24,17 @@ package common
 // values are of varying types: strings, ints, arrays, structs, etc.
 type APIParameters map[string]interface{}
 
+// Add copies API parameters from b to a, skipping parameters which already
+// exist, regardless of value, in a.
+func (a APIParameters) Add(b APIParameters) {
+	for name, value := range b {
+		_, ok := a[name]
+		if !ok {
+			a[name] = value
+		}
+	}
+}
+
 // APIParameterValidator is a function that validates API parameters
 // for a particular request or context.
 type APIParameterValidator func(APIParameters) error

psiphon/common/inproxy/client.go

@@ -356,7 +356,11 @@ func dialClientWebRTCConn(
 
 	// Send the ClientOffer request to the broker
 
-	packedBaseParams, err := protocol.EncodePackedAPIParameters(config.BaseAPIParameters)
+	apiParams := common.APIParameters{}
+	apiParams.Add(config.BaseAPIParameters)
+	apiParams.Add(common.APIParameters(brokerCoordinator.MetricsForBrokerRequests()))
+
+	packedParams, err := protocol.EncodePackedAPIParameters(apiParams)
 	if err != nil {
 		return nil, false, errors.Trace(err)
 	}
@@ -372,7 +376,7 @@ func dialClientWebRTCConn(
 		ctx,
 		&ClientOfferRequest{
 			Metrics: &ClientMetrics{
-				BaseAPIParameters:    packedBaseParams,
+				BaseAPIParameters:    packedParams,
 				ProxyProtocolVersion: proxyProtocolVersion,
 				NATType:              config.WebRTCDialCoordinator.NATType(),
 				PortMappingTypes:     config.WebRTCDialCoordinator.PortMappingTypes(),

psiphon/common/inproxy/coordinator.go

@@ -23,6 +23,8 @@ import (
 	"context"
 	"net"
 	"time"
+
+	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
 )
 
 // RoundTripper provides a request/response round trip network transport with
@@ -170,6 +172,13 @@ type BrokerDialCoordinator interface {
 	// proxies are not well balanced across brokers.
 	BrokerClientNoMatch(roundTripper RoundTripper)
 
+	// MetricsForBrokerRequests returns the metrics, associated with the
+	// broker client instance, which are to be added to the base API
+	// parameters included in client and proxy requests sent to the broker.
+	// This includes fronting_provider_id, which varies depending on the
+	// broker client dial and isn't a fixed base API parameter value.
+	MetricsForBrokerRequests() common.LogFields
+
 	SessionHandshakeRoundTripTimeout() time.Duration
 	AnnounceRequestTimeout() time.Duration
 	AnnounceDelay() time.Duration

psiphon/common/inproxy/coordinator_test.go

@@ -46,6 +46,7 @@ type testBrokerDialCoordinator struct {
 	brokerClientRoundTripperSucceeded func(RoundTripper)
 	brokerClientRoundTripperFailed    func(RoundTripper)
 	brokerClientNoMatch               func(RoundTripper)
+	metricsForBrokerRequests          common.LogFields
 	sessionHandshakeRoundTripTimeout  time.Duration
 	announceRequestTimeout            time.Duration
 	announceDelay                     time.Duration
@@ -124,6 +125,12 @@ func (t *testBrokerDialCoordinator) BrokerClientNoMatch(roundTripper RoundTrippe
 	t.brokerClientNoMatch(roundTripper)
 }
 
+func (t *testBrokerDialCoordinator) MetricsForBrokerRequests() common.LogFields {
+	t.mutex.Lock()
+	defer t.mutex.Unlock()
+	return t.metricsForBrokerRequests
+}
+
 func (t *testBrokerDialCoordinator) SessionHandshakeRoundTripTimeout() time.Duration {
 	t.mutex.Lock()
 	defer t.mutex.Unlock()

psiphon/common/inproxy/proxy.go

@@ -565,7 +565,8 @@ func (p *Proxy) proxyOneClient(
 	// returned in the proxy announcment response are associated and stored
 	// with the original network ID.
 
-	metrics, tacticsNetworkID, err := p.getMetrics(webRTCCoordinator)
+	metrics, tacticsNetworkID, err := p.getMetrics(
+		brokerCoordinator, webRTCCoordinator)
 	if err != nil {
 		return backOff, errors.Trace(err)
 	}
@@ -962,7 +963,9 @@ func (p *Proxy) proxyOneClient(
 	return backOff, err
 }
 
-func (p *Proxy) getMetrics(webRTCCoordinator WebRTCDialCoordinator) (*ProxyMetrics, string, error) {
+func (p *Proxy) getMetrics(
+	brokerCoordinator BrokerDialCoordinator,
+	webRTCCoordinator WebRTCDialCoordinator) (*ProxyMetrics, string, error) {
 
 	// tacticsNetworkID records the exact network ID that corresponds to the
 	// tactics tag sent in the base parameters, and is used when applying any
@@ -972,13 +975,17 @@ func (p *Proxy) getMetrics(webRTCCoordinator WebRTCDialCoordinator) (*ProxyMetri
 		return nil, "", errors.Trace(err)
 	}
 
-	packedBaseParams, err := protocol.EncodePackedAPIParameters(baseParams)
+	apiParams := common.APIParameters{}
+	apiParams.Add(baseParams)
+	apiParams.Add(common.APIParameters(brokerCoordinator.MetricsForBrokerRequests()))
+
+	packedParams, err := protocol.EncodePackedAPIParameters(apiParams)
 	if err != nil {
 		return nil, "", errors.Trace(err)
 	}
 
 	return &ProxyMetrics{
-		BaseAPIParameters:             packedBaseParams,
+		BaseAPIParameters:             packedParams,
 		ProxyProtocolVersion:          proxyProtocolVersion,
 		NATType:                       webRTCCoordinator.NATType(),
 		PortMappingTypes:              webRTCCoordinator.PortMappingTypes(),

psiphon/dialParameters.go

@@ -1660,18 +1660,6 @@ func (dialParams *DialParameters) GetInproxyMetrics() common.LogFields {
 	return inproxyMetrics
 }
 
-func (dialParams *DialParameters) GetInproxyBrokerMetrics() common.LogFields {
-	inproxyMetrics := common.LogFields{}
-
-	if !dialParams.inproxyDialInitialized {
-		return inproxyMetrics
-	}
-
-	inproxyMetrics.Add(dialParams.inproxyBrokerDialParameters.GetBrokerMetrics())
-
-	return inproxyMetrics
-}
-
 func (dialParams *DialParameters) Succeeded() {
 
 	// When TTL is 0, don't store dial parameters.

psiphon/inproxy.go

@@ -987,6 +987,11 @@ func (b *InproxyBrokerClientInstance) BrokerClientNoMatch(roundTripper inproxy.R
 	}
 }
 
+// Implements the inproxy.BrokerDialCoordinator interface.
+func (b *InproxyBrokerClientInstance) MetricsForBrokerRequests() common.LogFields {
+	return b.brokerDialParams.GetMetricsForBrokerRequests()
+}
+
 // Implements the inproxy.BrokerDialCoordinator interface.
 func (b *InproxyBrokerClientInstance) AnnounceRequestTimeout() time.Duration {
 	return b.announceRequestTimeout
@@ -1145,9 +1150,9 @@ func (brokerDialParams *InproxyBrokerDialParameters) prepareDialConfigs(
 	return nil
 }
 
-// GetBrokerMetrics returns  dial parameter log fields to be reported to a
-// broker.
-func (brokerDialParams *InproxyBrokerDialParameters) GetBrokerMetrics() common.LogFields {
+// GetMetricsForBroker returns broker client dial parameter log fields to be
+// reported to a broker.
+func (brokerDialParams *InproxyBrokerDialParameters) GetMetricsForBrokerRequests() common.LogFields {
 
 	logFields := common.LogFields{}
 

psiphon/server/server_test.go

@@ -1057,10 +1057,10 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 			if !ok {
 				t.Errorf("missing inproxy_broker.broker_event")
 			}
-			if event == "client_offer" || event == "proxy_announce" {
+			if event == "client-offer" || event == "proxy-announce" {
 				fronting_provider_id, ok := logFields["fronting_provider_id"].(string)
 				if !ok || fronting_provider_id != inproxyTestConfig.brokerFrontingProviderID {
-					t.Errorf("unexpected inproxy_broker.fronting_provider_id")
+					t.Errorf("unexpected inproxy_broker.fronting_provider_id for %s", event)
 				}
 			}
 		}

psiphon/serverApi.go

@@ -1014,7 +1014,7 @@ func (serverContext *ServerContext) getBaseAPIParameters(
 // included with each Psiphon API request. These common parameters are used
 // for metrics.
 //
-// The input dialPatrams may be nil when the filter has
+// The input dialParams may be nil when the filter has
 // baseParametersNoDialParameters.
 func getBaseAPIParameters(
 	filter baseParametersFilter,

psiphon/tunnel.go

@@ -1550,8 +1550,6 @@ func dialInproxy(
 	params := getBaseAPIParameters(
 		baseParametersNoDialParameters, true, config, nil)
 
-	common.LogFields(params).Add(dialParams.GetInproxyBrokerMetrics())
-
 	// The debugLogging flag is passed to both NoticeCommonLogger and to the
 	// inproxy package as well; skipping debug logs in the inproxy package,
 	// before calling into the notice logger, avoids unnecessary allocations