services/horizon: Add deprecation warning for using command-line flags (

#5051)

* Add deprecated warning

* Add deprecated warning tests

* add test for env vars

* Dynamically print deprecated message

* Update flags.go

* Update parameters_test.go

* Update parameters_test.go

* Update parameters_test.go

* Update parameters_test.go

* Shift environment.go to horizon package

* Move environment.go to test dir

* Update flags_test.go

* Update flags_test.go

* Update integration.go

* Nit changes - 1

* Update CHANGELOG.md

* Nit changes - 2

* Update flags_test.go

services/horizon/CHANGELOG.md

@@ -15,6 +15,7 @@ file. This project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 - The same slippage calculation from the [`v2.26.1`](#2261) hotfix now properly excludes spikes for smoother trade aggregation plots ([4999](https://github.com/stellar/go/pull/4999)).
+- Add a deprecation warning for using command-line flags when running Horizon ([5051](https://github.com/stellar/go/pull/5051))
 
 
 ## 2.26.1

services/horizon/cmd/root.go

@@ -16,7 +16,11 @@ var (
 		Short:         "client-facing api server for the Stellar network",
 		SilenceErrors: true,
 		SilenceUsage:  true,
-		Long:          "Client-facing API server for the Stellar network. It acts as the interface between Stellar Core and applications that want to access the Stellar network. It allows you to submit transactions to the network, check the status of accounts, subscribe to event streams and more.",
+		Long: "Client-facing API server for the Stellar network. It acts as the interface between Stellar Core " +
+			"and applications that want to access the Stellar network. It allows you to submit transactions to the " +
+			"network, check the status of accounts, subscribe to event streams and more.\n" +
+			"DEPRECATED - the use of command-line flags has been deprecated in favor of environment variables. Please" +
+			"consult our Configuring section in the developer documentation on how to use them - https://developers.stellar.org/docs/run-api-server/configuring",
 		RunE: func(cmd *cobra.Command, args []string) error {
 			app, err := horizon.NewAppFromFlags(config, flags)
 			if err != nil {

services/horizon/internal/flags.go

@@ -238,11 +238,10 @@ func Flags() (*Config, support.ConfigOptions) {
 
 				if val := viper.GetString(opt.Name); val != "" {
 					stdLog.Printf(
-						"DEPRECATED - No ingestion filter rules are defined by default, which equates to no filtering " +
-							"of historical data. If you have never added filter rules to this deployment, then nothing further needed. " +
-							"If you have defined ingestion filter rules prior but disabled filtering overall by setting this flag " +
-							"disabled with --exp-enable-ingestion-filtering=false, then you should now delete the filter rules using " +
-							"the Horizon Admin API to achieve the same no-filtering result. Remove usage of this flag in all cases.",
+						"DEPRECATED - No ingestion filter rules are defined by default, which equates to " +
+							"no filtering of historical data. If you have never added filter rules to this deployment, then no further action is needed. " +
+							"If you have defined ingestion filter rules previously but disabled filtering overall by setting the env variable EXP_ENABLE_INGESTION_FILTERING=false, " +
+							"then you should now delete the filter rules using the Horizon Admin API to achieve the same no-filtering result. Remove usage of this variable in all cases.",
 					)
 				}
 				return nil
@@ -801,10 +800,19 @@ func setCaptiveCoreConfiguration(config *Config, options ApplyOptions) error {
 
 // ApplyFlags applies the command line flags on the given Config instance
 func ApplyFlags(config *Config, flags support.ConfigOptions, options ApplyOptions) error {
+	// Check if the user has passed any flags and if so, print a DEPRECATED warning message.
+	flagsPassedByUser := flags.GetCommandLineFlagsPassedByUser()
+	if len(flagsPassedByUser) > 0 {
+		result := fmt.Sprintf("DEPRECATED - the use of command-line flags: [%s], has been deprecated in favor of environment variables. "+
+			"Please consult our Configuring section in the developer documentation on how to use them - https://developers.stellar.org/docs/run-api-server/configuring", "--"+strings.Join(flagsPassedByUser, ",--"))
+		stdLog.Println(result)
+	}
+
 	// Verify required options and load the config struct
 	if err := flags.RequireE(); err != nil {
 		return err
 	}
+
 	if err := flags.SetValues(); err != nil {
 		return err
 	}

services/horizon/internal/flags_test.go

@@ -2,8 +2,12 @@ package horizon
 
 import (
 	"fmt"
+	"os"
 	"testing"
 
+	"github.com/spf13/cobra"
+	"github.com/stellar/go/services/horizon/internal/test"
+
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -201,3 +205,59 @@ func Test_createCaptiveCoreConfig(t *testing.T) {
 		})
 	}
 }
+
+func TestEnvironmentVariables(t *testing.T) {
+	environmentVars := map[string]string{
+		"INGEST":                        "false",
+		"HISTORY_ARCHIVE_URLS":          "http://localhost:1570",
+		"DATABASE_URL":                  "postgres://postgres@localhost/test_332cb65e6b00?sslmode=disable&timezone=UTC",
+		"STELLAR_CORE_URL":              "http://localhost:11626",
+		"NETWORK_PASSPHRASE":            "Standalone Network ; February 2017",
+		"APPLY_MIGRATIONS":              "true",
+		"ENABLE_CAPTIVE_CORE_INGESTION": "false",
+		"CHECKPOINT_FREQUENCY":          "8",
+		"MAX_DB_CONNECTIONS":            "50",
+		"ADMIN_PORT":                    "6060",
+		"PORT":                          "8001",
+		"CAPTIVE_CORE_BINARY_PATH":      os.Getenv("HORIZON_INTEGRATION_TESTS_CAPTIVE_CORE_BIN"),
+		"CAPTIVE_CORE_CONFIG_PATH":      "../docker/captive-core-classic-integration-tests.cfg",
+		"CAPTIVE_CORE_USE_DB":           "true",
+	}
+
+	envManager := test.NewEnvironmentManager()
+	defer func() {
+		envManager.Restore()
+	}()
+	if err := envManager.InitializeEnvironmentVariables(environmentVars); err != nil {
+		fmt.Println(err)
+	}
+
+	config, flags := Flags()
+	horizonCmd := &cobra.Command{
+		Use:           "horizon",
+		Short:         "Client-facing api server for the Stellar network",
+		SilenceErrors: true,
+		SilenceUsage:  true,
+		Long:          "Client-facing API server for the Stellar network.",
+	}
+	if err := flags.Init(horizonCmd); err != nil {
+		fmt.Println(err)
+	}
+	if err := ApplyFlags(config, flags, ApplyOptions{RequireCaptiveCoreFullConfig: true, AlwaysIngest: false}); err != nil {
+		fmt.Println(err)
+	}
+	assert.Equal(t, config.Ingest, false)
+	assert.Equal(t, config.HistoryArchiveURLs, []string{"http://localhost:1570"})
+	assert.Equal(t, config.DatabaseURL, "postgres://postgres@localhost/test_332cb65e6b00?sslmode=disable&timezone=UTC")
+	assert.Equal(t, config.StellarCoreURL, "http://localhost:11626")
+	assert.Equal(t, config.NetworkPassphrase, "Standalone Network ; February 2017")
+	assert.Equal(t, config.ApplyMigrations, true)
+	assert.Equal(t, config.EnableCaptiveCoreIngestion, false)
+	assert.Equal(t, config.CheckpointFrequency, uint32(8))
+	assert.Equal(t, config.MaxDBConnections, 50)
+	assert.Equal(t, config.AdminPort, uint(6060))
+	assert.Equal(t, config.Port, uint(8001))
+	assert.Equal(t, config.CaptiveCoreBinaryPath, os.Getenv("HORIZON_INTEGRATION_TESTS_CAPTIVE_CORE_BIN"))
+	assert.Equal(t, config.CaptiveCoreConfigPath, "../docker/captive-core-classic-integration-tests.cfg")
+	assert.Equal(t, config.CaptiveCoreConfigUseDB, true)
+}

services/horizon/internal/integration/parameters_test.go

@@ -431,12 +431,6 @@ func TestDisableTxSub(t *testing.T) {
 		test.Shutdown()
 	})
 	t.Run("horizon starts successfully when DISABLE_TX_SUB=true and INGEST=true", func(t *testing.T) {
-		//localParams := integration.MergeMaps(networkParamArgs, map[string]string{
-		//	//horizon.NetworkFlagName:           "testnet",
-		//	horizon.IngestFlagName:            "true",
-		//	horizon.DisableTxSubFlagName:      "true",
-		//	horizon.StellarCoreBinaryPathName: "/usr/bin/stellar-core",
-		//})
 		testConfig := integration.GetTestConfig()
 		testConfig.HorizonIngestParameters = map[string]string{
 			"disable-tx-sub": "true",
@@ -450,40 +444,93 @@ func TestDisableTxSub(t *testing.T) {
 	})
 }
 
-func TestDeprecatedOutputForIngestionFilteringFlag(t *testing.T) {
-	originalStderr := os.Stderr
-	r, w, _ := os.Pipe()
-	os.Stderr = w
-	stdLog.SetOutput(os.Stderr)
+func TestDeprecatedOutputs(t *testing.T) {
+	t.Run("deprecated output for ingestion filtering", func(t *testing.T) {
+		originalStderr := os.Stderr
+		r, w, _ := os.Pipe()
+		os.Stderr = w
+		stdLog.SetOutput(os.Stderr)
 
-	testConfig := integration.GetTestConfig()
-	testConfig.HorizonIngestParameters = map[string]string{"exp-enable-ingestion-filtering": "false"}
-	test := integration.NewTest(t, *testConfig)
-	err := test.StartHorizon()
-	assert.NoError(t, err)
-	test.WaitForHorizon()
+		testConfig := integration.GetTestConfig()
+		testConfig.HorizonIngestParameters = map[string]string{"exp-enable-ingestion-filtering": "false"}
+		test := integration.NewTest(t, *testConfig)
+		err := test.StartHorizon()
+		assert.NoError(t, err)
+		test.WaitForHorizon()
 
-	// Use a wait group to wait for the goroutine to finish before proceeding
-	var wg sync.WaitGroup
-	wg.Add(1)
-	go func() {
-		defer wg.Done()
-		if err := w.Close(); err != nil {
-			t.Errorf("Failed to close Stdout")
-			return
+		// Use a wait group to wait for the goroutine to finish before proceeding
+		var wg sync.WaitGroup
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			if err := w.Close(); err != nil {
+				t.Errorf("Failed to close Stdout")
+				return
+			}
+		}()
+
+		outputBytes, _ := io.ReadAll(r)
+		wg.Wait() // Wait for the goroutine to finish before proceeding
+		_ = r.Close()
+		os.Stderr = originalStderr
+
+		assert.Contains(t, string(outputBytes), "DEPRECATED - No ingestion filter rules are defined by default, which equates to "+
+			"no filtering of historical data. If you have never added filter rules to this deployment, then no further action is needed. "+
+			"If you have defined ingestion filter rules previously but disabled filtering overall by setting the env variable EXP_ENABLE_INGESTION_FILTERING=false, "+
+			"then you should now delete the filter rules using the Horizon Admin API to achieve the same no-filtering result. Remove usage of this variable in all cases.")
+	})
+	t.Run("deprecated output for command-line flags", func(t *testing.T) {
+		originalStderr := os.Stderr
+		r, w, _ := os.Pipe()
+		os.Stderr = w
+		stdLog.SetOutput(os.Stderr)
+
+		config, flags := horizon.Flags()
+
+		horizonCmd := &cobra.Command{
+			Use:           "horizon",
+			Short:         "Client-facing api server for the Stellar network",
+			SilenceErrors: true,
+			SilenceUsage:  true,
+			Long:          "Client-facing API server for the Stellar network.",
+			RunE: func(cmd *cobra.Command, args []string) error {
+				_, err := horizon.NewAppFromFlags(config, flags)
+				if err != nil {
+					return err
+				}
+				return nil
+			},
 		}
-	}()
 
-	outputBytes, _ := io.ReadAll(r)
-	wg.Wait() // Wait for the goroutine to finish before proceeding
-	_ = r.Close()
-	os.Stderr = originalStderr
+		horizonCmd.SetArgs([]string{"--disable-tx-sub=true"})
+		if err := flags.Init(horizonCmd); err != nil {
+			fmt.Println(err)
+		}
+		if err := horizonCmd.Execute(); err != nil {
+			fmt.Println(err)
+		}
+
+		// Use a wait group to wait for the goroutine to finish before proceeding
+		var wg sync.WaitGroup
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+			if err := w.Close(); err != nil {
+				t.Errorf("Failed to close Stdout")
+				return
+			}
+		}()
 
-	assert.Contains(t, string(outputBytes), "DEPRECATED - No ingestion filter rules are defined by default, which equates to "+
-		"no filtering of historical data. If you have never added filter rules to this deployment, then nothing further needed. "+
-		"If you have defined ingestion filter rules prior but disabled filtering overall by setting this flag disabled with "+
-		"--exp-enable-ingestion-filtering=false, then you should now delete the filter rules using the Horizon Admin API to achieve "+
-		"the same no-filtering result. Remove usage of this flag in all cases.")
+		outputBytes, _ := io.ReadAll(r)
+		wg.Wait() // Wait for the goroutine to finish before proceeding
+		_ = r.Close()
+		os.Stderr = originalStderr
+
+		assert.Contains(t, string(outputBytes), "DEPRECATED - the use of command-line flags: "+
+			"[--disable-tx-sub], has been deprecated in favor of environment variables. Please consult our "+
+			"Configuring section in the developer documentation on how to use them - "+
+			"https://developers.stellar.org/docs/run-api-server/configuring")
+	})
 }
 
 func TestHelpOutput(t *testing.T) {
@@ -505,7 +552,7 @@ func TestHelpOutput(t *testing.T) {
 	}
 
 	var writer io.Writer = &bytes.Buffer{}
-	horizonCmd.SetOutput(writer)
+	horizonCmd.SetOut(writer)
 
 	horizonCmd.SetArgs([]string{"-h"})
 	if err := flags.Init(horizonCmd); err != nil {

services/horizon/internal/test/environment.go

@@ -0,0 +1,65 @@
+package test
+
+import (
+	"fmt"
+	"os"
+	"strings"
+
+	"github.com/stellar/go/support/errors"
+)
+
+type EnvironmentManager struct {
+	oldEnvironment, newEnvironment map[string]string
+}
+
+func NewEnvironmentManager() *EnvironmentManager {
+	env := &EnvironmentManager{}
+	env.oldEnvironment = make(map[string]string)
+	env.newEnvironment = make(map[string]string)
+	return env
+}
+
+func (envManager *EnvironmentManager) InitializeEnvironmentVariables(environmentVars map[string]string) error {
+	var env strings.Builder
+	for key, value := range environmentVars {
+		env.WriteString(fmt.Sprintf("%s=%s ", key, value))
+	}
+
+	// prepare env
+	for key, value := range environmentVars {
+		innerErr := envManager.Add(key, value)
+		if innerErr != nil {
+			return errors.Wrap(innerErr, fmt.Sprintf(
+				"failed to set envvar (%s=%s)", key, value))
+		}
+	}
+	return nil
+}
+
+// Add sets a new environment variable, saving the original value (if any).
+func (envManager *EnvironmentManager) Add(key, value string) error {
+	// If someone pushes an environmental variable more than once, we don't want
+	// to lose the *original* value, so we're being careful here.
+	if _, ok := envManager.newEnvironment[key]; !ok {
+		if oldValue, ok := os.LookupEnv(key); ok {
+			envManager.oldEnvironment[key] = oldValue
+		}
+	}
+
+	envManager.newEnvironment[key] = value
+	return os.Setenv(key, value)
+}
+
+// Restore restores the environment prior to any modifications.
+//
+// You should probably use this alongside `defer` to ensure the global
+// environment isn't modified for longer than you intend.
+func (envManager *EnvironmentManager) Restore() {
+	for key := range envManager.newEnvironment {
+		if oldValue, ok := envManager.oldEnvironment[key]; ok {
+			os.Setenv(key, oldValue)
+		} else {
+			os.Unsetenv(key)
+		}
+	}
+}