reviewer comments

Author: ohnorobo

x/examples/smart-proxy/config_broken.yaml

@@ -26,7 +26,7 @@ fallback:
   # Nonexistent Outline Server
   - ss://Y2hhY2hhMjAtaWV0Zi1wb2x5MTMwNTprSzdEdHQ0MkJLOE9hRjBKYjdpWGFK@1.2.3.4:9999/?outline=1
   # Nonexistant Psiphon Config JSON. Not yet supported
-  - {
+  - psiphon: {
       "LocalHttpProxyPort":8081,
       "LocalSocksProxyPort":1081,
       "PropagationChannelId":"FFFFFFFFFFFFFFFF",

x/smart/stream_dialer.go

@@ -27,6 +27,8 @@ import (
 	"time"
 
 	"github.com/goccy/go-yaml"
+	"github.com/goccy/go-yaml/ast"
+	"github.com/goccy/go-yaml/parser"
 	"github.com/Jigsaw-Code/outline-sdk/dns"
 	"github.com/Jigsaw-Code/outline-sdk/transport"
 	"github.com/Jigsaw-Code/outline-sdk/x/configurl"
@@ -95,12 +97,13 @@ type dnsEntryConfig struct {
 	TCP    *tcpEntryConfig   `yaml:"tcp,omitempty"`
 }
 
-type psiphonEntryConfig struct {
-	// Don't verify the psiphon config format here, just pass it forward
-	Psiphon 	map[string]interface{} 	`yaml:"psiphon,omitempty"`
+type fallbackEntryStructConfig struct {
+	Psiphon any	`yaml:"psiphon,omitempty"`
+	// As we allow more fallback types beyond psiphon they will be added here
 }
 
-// can be either a configurl string or a psiphon config
+// This contains either a configURL string or a fallbackEntryStructConfig
+// It is parsed into the correct type later
 type fallbackEntryConfig any
 
 type configConfig struct {
@@ -331,22 +334,23 @@ func (f *StrategyFinder) findFallback(ctx context.Context, testDomains []string,
 	fallback, err := raceTests(ctx, 250*time.Millisecond, fallbackConfigs, func(fallbackConfig fallbackEntryConfig) (*SearchResult, error) {
 		switch v := fallbackConfig.(type) {
 		case string:
-			fallbackUrl := v
-			dialer, err := configModule.NewStreamDialer(ctx, fallbackUrl)
+			configUrl := v
+			dialer, err := configModule.NewStreamDialer(ctx, configUrl)
 			if err != nil {
 				return nil, fmt.Errorf("getStreamDialer failed: %w", err)
 			}
 
-			err = f.testDialer(ctx, dialer, testDomains, fallbackUrl)
+			err = f.testDialer(ctx, dialer, testDomains, configUrl)
 			if err != nil {
 				return nil, err
 			}
 
 			return &SearchResult{dialer, fallbackConfig}, nil
-		case map[string]interface{}:
-			psiphonJSON, err := json.Marshal(v)
+		case fallbackEntryStructConfig:
+			psiphonCfg := v.Psiphon
+			psiphonJSON, err := json.Marshal(psiphonCfg)
 			if err != nil {
-				f.logCtx(ctx, "Error marshaling to JSON: %v, %v", v, err)
+				f.logCtx(ctx, "Error marshaling to JSON: %v, %v", psiphonCfg, err)
 			}
 
 			// TODO(laplante): pass this forward into psiphon.go, which takes raw json
@@ -355,10 +359,8 @@ func (f *StrategyFinder) findFallback(ctx context.Context, testDomains []string,
 		default:
 			return nil, fmt.Errorf("unknown fallback type: %v", v)
 		}
-
-		// If neither, it's an unknown type
-		return nil, fmt.Errorf("unknown fallback type: %v", fallbackConfig)
 	})
+
 	if err != nil {
 		return nil, fmt.Errorf("could not find a working fallback: %w", err)
 	}
@@ -392,14 +394,58 @@ func (f *StrategyFinder) newProxylessDialer(ctx context.Context, testDomains []s
 	return f.findTLS(ctx, testDomains, dnsDialer, config.TLS)
 }
 
+func (f *StrategyFinder) parseConfig(configBytes []byte) (configConfig, error) {
+	parsedBytes, err := parser.ParseBytes(configBytes, 0)
+	if err != nil {
+		return configConfig{}, fmt.Errorf("failed to parse config: %v", err)
+	}
+
+	var parsedConfig configConfig
+	err = yaml.NodeToValue(parsedBytes.Docs[0].Body, &parsedConfig)
+	if err != nil {
+		return configConfig{}, fmt.Errorf("failed to parse config: %v", err)
+	}
+
+	// Parse fallback list
+	mapping, ok := parsedBytes.Docs[0].Body.(*ast.MappingNode)
+	if !ok {
+		return configConfig{}, fmt.Errorf("failed to parse config: root is not a mapping")
+	}
+	for _, value := range mapping.Values {
+		if key, ok := value.Key.(*ast.StringNode); ok && key.Value == "fallback" {
+			sequence, ok := value.Value.(*ast.SequenceNode)
+			if !ok {
+				return configConfig{}, fmt.Errorf("failed to parse config: fallback is not a sequence")
+			}
+			for i, fallback := range sequence.Values {
+				switch v := fallback.(type) {
+				case *ast.StringNode:
+					parsedConfig.Fallback[i] = v.Value
+				case *ast.MappingNode:
+					var fallbackEntry fallbackEntryStructConfig
+					err := yaml.NodeToValue(v, &fallbackEntry)
+					if err != nil {
+						return configConfig{}, fmt.Errorf("failed to parse config as fallbackEntryStructConfig: %v", err)
+					}
+					parsedConfig.Fallback[i] =  fallbackEntry
+				default:
+					return configConfig{}, fmt.Errorf("unknown fallback type: %v", v)
+				}
+			}
+		}
+	}
+
+	return parsedConfig, nil
+}
+
 // NewDialer uses the config in configBytes to search for a strategy that unblocks DNS and TLS for all of the testDomains, returning a dialer with the found strategy.
 // It returns an error if no strategy was found that unblocks the testDomains.
 // The testDomains must be domains with a TLS service running on port 443.
 func (f *StrategyFinder) NewDialer(ctx context.Context, testDomains []string, configBytes []byte) (transport.StreamDialer, error) {
 	var parsedConfig configConfig
-	err := yaml.Unmarshal(configBytes, &parsedConfig)
+	parsedConfig, err := f.parseConfig(configBytes)
 	if err != nil {
-		return nil, fmt.Errorf("failed to parse config: %v", err)
+		return nil, err
 	}
 
 	// Make domain fully-qualified to prevent confusing domain search.