internal/config: export the Config type

In preparation for eliminating the cfg singleton, export the Config type
and add a load function that resolves a Config from the environment.

The Init method is kept around temporarily, but modified to return the
Config that was loaded. This allows for incrementally deprecating
dependence on the config singleton. In the future, the Init function
will be deprecated and Load will be exported, making the package
stateless.

Configuration values that were only used in main.go are now read
directly from the newly available Config instance. This allowed deleting
some functions in the config package.

Updates b/145301722

Change-Id: I98e00971fad95cc4d69b371d815de5f5498b04db
Reviewed-on: https://team-review.git.corp.google.com/c/golang/discovery/+/609591
CI-Result: Cloud Build <devtools-proctor-result-processor@system.gserviceaccount.com>
Reviewed-by: Jonathan Amsterdam <jba@google.com>

cmd/etl/main.go

@@ -46,12 +46,13 @@ func main() {
 
 	ctx := context.Background()
 
-	if err := config.Init(ctx); err != nil {
+	cfg, err := config.Init(ctx)
+	if err != nil {
 		log.Fatal(ctx, err)
 	}
-	config.Dump(os.Stderr)
+	cfg.Dump(os.Stderr)
 
-	if config.UseProfiler() {
+	if cfg.UseProfiler {
 		if err := profiler.Start(profiler.Config{}); err != nil {
 			log.Fatalf(ctx, "profiler.Start: %v", err)
 		}
@@ -64,24 +65,24 @@ func main() {
 	if err != nil {
 		log.Fatalf(ctx, "unable to register the ocsql driver: %v\n", err)
 	}
-	ddb, err := database.Open(driverName, config.DBConnInfo())
+	ddb, err := database.Open(driverName, cfg.DBConnInfo())
 	if err != nil {
 		log.Fatalf(ctx, "database.Open: %v", err)
 	}
 	db := postgres.New(ddb)
 	defer db.Close()
 
-	indexClient, err := index.New(config.IndexURL())
+	indexClient, err := index.New(cfg.IndexURL)
 	if err != nil {
 		log.Fatal(ctx, err)
 	}
-	proxyClient, err := proxy.New(config.ProxyURL())
+	proxyClient, err := proxy.New(cfg.ProxyURL)
 	if err != nil {
 		log.Fatal(ctx, err)
 	}
 	fetchQueue := queue(ctx, proxyClient, db)
 	reportingClient := reportingClient(ctx)
-	redisClient := getRedis(ctx)
+	redisClient := getRedis(ctx, cfg)
 	server, err := etl.NewServer(db, indexClient, proxyClient, redisClient, fetchQueue, reportingClient, *staticPath)
 	if err != nil {
 		log.Fatal(ctx, err)
@@ -130,10 +131,10 @@ func queue(ctx context.Context, proxyClient *proxy.Client, db *postgres.DB) etl.
 	return etl.NewGCPQueue(client, queueName)
 }
 
-func getRedis(ctx context.Context) *redis.Client {
-	if config.RedisHAHost() != "" {
+func getRedis(ctx context.Context, cfg *config.Config) *redis.Client {
+	if cfg.RedisHAHost != "" {
 		return redis.NewClient(&redis.Options{
-			Addr: config.RedisHAHost() + ":" + config.RedisHAPort(),
+			Addr: cfg.RedisHAHost + ":" + cfg.RedisHAPort,
 			// We update completions with one big pipeline, so we need long write
 			// timeouts. ReadTimeout is increased only to be consistent with
 			// WriteTimeout.

cmd/frontend/main.go

@@ -41,12 +41,13 @@ func main() {
 
 	ctx := context.Background()
 
-	if err := config.Init(ctx); err != nil {
+	cfg, err := config.Init(ctx)
+	if err != nil {
 		log.Fatal(ctx, err)
 	}
-	config.Dump(os.Stderr)
+	cfg.Dump(os.Stderr)
 
-	if config.UseProfiler() {
+	if cfg.UseProfiler {
 		if err := profiler.Start(profiler.Config{}); err != nil {
 			log.Fatalf(ctx, "profiler.Start: %v", err)
 		}
@@ -69,7 +70,7 @@ func main() {
 		if err != nil {
 			log.Fatalf(ctx, "unable to register the ocsql driver: %v\n", err)
 		}
-		ddb, err := database.Open(ocDriver, config.DBConnInfo())
+		ddb, err := database.Open(ocDriver, cfg.DBConnInfo())
 		if err != nil {
 			log.Fatalf(ctx, "database.Open: %v", err)
 		}
@@ -79,9 +80,9 @@ func main() {
 		exp = db
 	}
 	var haClient *redis.Client
-	if config.RedisHAHost() != "" {
+	if cfg.RedisHAHost != "" {
 		haClient = redis.NewClient(&redis.Options{
-			Addr: config.RedisHAHost() + ":" + config.RedisHAPort(),
+			Addr: cfg.RedisHAHost + ":" + cfg.RedisHAPort,
 		})
 	}
 	server, err := frontend.NewServer(ds, haClient, *staticPath, *reloadTemplates)
@@ -90,9 +91,9 @@ func main() {
 	}
 	router := dcensus.NewRouter(frontend.TagRoute)
 	var cacheClient *redis.Client
-	if config.RedisHost() != "" {
+	if cfg.RedisCacheHost != "" {
 		cacheClient = redis.NewClient(&redis.Options{
-			Addr: config.RedisHost() + ":" + config.RedisPort(),
+			Addr: cfg.RedisCacheHost + ":" + cfg.RedisCachePort,
 		})
 	}
 	server.Install(router.Handle, cacheClient)
@@ -129,7 +130,7 @@ func main() {
 
 	mw := middleware.Chain(
 		middleware.RequestLog(requestLogger),
-		middleware.Quota(config.Quota()),
+		middleware.Quota(cfg.Quota),
 		middleware.SecureHeaders(),                     // must come before any caching for nonces to work
 		middleware.LatestVersion(server.LatestVersion), // must come before caching for version badge to work
 		middleware.Panic(panicHandler),

cmd/prober/main.go

@@ -216,19 +216,17 @@ func main() {
 	}
 	log.Infof(ctx, "base URL %s", baseURL)
 
-	if err := config.Init(ctx); err != nil {
+	cfg, err := config.Init(ctx)
+	if err != nil {
 		log.Fatal(ctx, err)
 	}
-	config.Dump(os.Stderr)
+	cfg.Dump(os.Stderr)
 
 	if _, err := log.UseStackdriver(ctx, "prober-log"); err != nil {
 		log.Fatal(ctx, err)
 	}
 
-	var (
-		jsonCreds []byte
-		err       error
-	)
+	var jsonCreds []byte
 
 	if *credsFile != "" {
 		jsonCreds, err = ioutil.ReadFile(*credsFile)

internal/config/config.go

@@ -55,16 +55,6 @@ func GetEnv(key, fallback string) string {
 	return fallback
 }
 
-// IndexURL returns the URL of the Go module index.
-func IndexURL() string {
-	return cfg.IndexURL
-}
-
-// ProxyURL returns the URL of the Go module proxy.
-func ProxyURL() string {
-	return cfg.ProxyURL
-}
-
 // ServiceID returns a the name of the current application.
 func ServiceID() string {
 	return cfg.ServiceID
@@ -80,37 +70,6 @@ func LocationID() string {
 	return cfg.LocationID
 }
 
-// RedisHost returns the hostname or IP address of the redis instance to
-// be used for page caching.
-// TODO(b/143370178): rename to RedisCacheHost
-func RedisHost() string {
-	return cfg.RedisCacheHost
-}
-
-// RedisPort returns the port of the redis instance to be used for page
-// caching.
-// TODO(b/143370178): rename to RedisCachePort
-func RedisPort() string {
-	return cfg.RedisCachePort
-}
-
-// RedisHAHost returns the hostname or IP address of the redis instance
-// to be used for auto-completion.
-func RedisHAHost() string {
-	return cfg.RedisHAHost
-}
-
-// RedisHAPort returns the port of the redis instance to be used for
-// auto-completion.
-func RedisHAPort() string {
-	return cfg.RedisHAPort
-}
-
-// Quota returns the settings for the quota middleware.
-func Quota() QuotaSettings {
-	return cfg.Quota
-}
-
 // AppVersionLabel returns the version label for the current instance.  This is
 // the AppVersionID available, otherwise a string constructed using the
 // timestamp of process start.
@@ -126,11 +85,6 @@ func AppVersionID() string {
 	return cfg.VersionID
 }
 
-// UseProfiler specifies whether to enable Stackdriver Profiler.
-func UseProfiler() bool {
-	return cfg.UseProfiler
-}
-
 // AppVersionFormat is the expected format of the app version timestamp.
 const AppVersionFormat = "20060102t150405"
 
@@ -185,7 +139,7 @@ const StatementTimeout = 10 * time.Minute
 
 // DBConnInfo returns a PostgreSQL connection string constructed from
 // environment variables.
-func DBConnInfo() string {
+func (c *Config) DBConnInfo() string {
 	// For the connection string syntax, see
 	// https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNSTRING.
 
@@ -193,10 +147,12 @@ func DBConnInfo() string {
 	// See https://www.postgresql.org/docs/current/runtime-config-client.html.
 	timeoutOption := fmt.Sprintf("-c statement_timeout=%d", StatementTimeout/time.Millisecond)
 	return fmt.Sprintf("user='%s' password='%s' host='%s' port=%s dbname='%s' sslmode=disable options='%s'",
-		cfg.DBUser, cfg.DBPassword, cfg.DBHost, cfg.DBPort, cfg.DBName, timeoutOption)
+		c.DBUser, c.DBPassword, c.DBHost, c.DBPort, c.DBName, timeoutOption)
 }
 
-type config struct {
+// Config holds shared configuration values used in instantiating our server
+// components.
+type Config struct {
 	// Discovery environment variables
 	ProxyURL, IndexURL string
 
@@ -248,14 +204,32 @@ type QuotaSettings struct {
 	RecordOnly *bool
 }
 
-var cfg config
+var cfg Config
 
 const overrideBucket = "go-discovery"
 
 // Init resolves all configuration values provided by the config package. It
 // must be called before any configuration values are used.
-func Init(ctx context.Context) (err error) {
+func Init(ctx context.Context) (_ *Config, err error) {
 	defer derrors.Add(&err, "config.Init(ctx)")
+	cfg2, err := load(ctx)
+	if err != nil {
+		return nil, err
+	}
+	cfg = *cfg2
+	return cfg2, nil
+}
+
+// load builds a Config from the execution environment, loading some values
+// from envvars and others from remote services.
+func load(ctx context.Context) (_ *Config, err error) {
+	defer derrors.Add(&err, "config.Load(ctx)")
+
+	// TODO(b/145301722): remove this comment.
+	// This variable shadowing is temporary, as this package is being made
+	// stateless. Init is being incrementally deprecated in favor of an exported
+	// Load function.
+	cfg := &Config{}
 
 	// Resolve client/server configuration from the environment.
 	cfg.IndexURL = GetEnv("GO_MODULE_INDEX_URL", "https://index.golang.org/index")
@@ -279,7 +253,7 @@ func Init(ctx context.Context) (err error) {
 		// Zone is not available in the environment but can be queried via the metadata API.
 		zone, err := gceMetadata(ctx, "instance/zone")
 		if err != nil {
-			return err
+			return nil, err
 		}
 		cfg.ZoneID = zone
 	}
@@ -301,7 +275,7 @@ func Init(ctx context.Context) (err error) {
 	cfg.DBPassword = os.Getenv("GO_DISCOVERY_DATABASE_PASSWORD")
 	cfg.DBHost, err = chooseOne(GetEnv("GO_DISCOVERY_DATABASE_HOST", "localhost"))
 	if err != nil {
-		return err
+		return nil, err
 	}
 	cfg.DBPort = GetEnv("GO_DISCOVERY_DATABASE_PORT", "5432")
 	cfg.DBName = GetEnv("GO_DISCOVERY_DATABASE_NAME", "discovery-database")
@@ -311,7 +285,7 @@ func Init(ctx context.Context) (err error) {
 		var err error
 		cfg.DBPassword, err = secrets.Get(ctx, cfg.DBSecret)
 		if err != nil {
-			return fmt.Errorf("could not get database password secret: %v", err)
+			return nil, fmt.Errorf("could not get database password secret: %v", err)
 		}
 	}
 
@@ -340,10 +314,10 @@ func Init(ctx context.Context) (err error) {
 			log.Print(err)
 		} else {
 			log.Printf("processing overrides from gs://%s/%s", overrideBucket, overrideObj)
-			processOverrides(&cfg, overrideBytes)
+			processOverrides(cfg, overrideBytes)
 		}
 	}
-	return nil
+	return cfg, nil
 }
 
 func readOverrideFile(ctx context.Context, bucketName, objName string) (_ []byte, err error) {
@@ -362,7 +336,7 @@ func readOverrideFile(ctx context.Context, bucketName, objName string) (_ []byte
 	return ioutil.ReadAll(r)
 }
 
-func processOverrides(cfg *config, bytes []byte) {
+func processOverrides(cfg *Config, bytes []byte) {
 	var ov configOverride
 	if err := yaml.Unmarshal(bytes, &ov); err != nil {
 		log.Printf("processOverrides: %v", err)
@@ -398,7 +372,7 @@ func overrideBool(name string, field **bool, val *bool) {
 }
 
 // Dump outputs the current config information to the given Writer.
-func Dump(w io.Writer) error {
+func (c *Config) Dump(w io.Writer) error {
 	fmt.Fprint(w, "config: ")
 	enc := json.NewEncoder(w)
 	enc.SetIndent("", "    ")

internal/config/config_test.go

@@ -58,7 +58,7 @@ func TestChooseOne(t *testing.T) {
 func TestProcessOverrides(t *testing.T) {
 	tr := true
 	f := false
-	cfg := config{
+	cfg := Config{
 		DBHost: "origHost",
 		DBName: "origName",
 		Quota:  QuotaSettings{QPS: 1, Burst: 2, MaxEntries: 3, RecordOnly: &tr},
@@ -71,7 +71,7 @@ func TestProcessOverrides(t *testing.T) {
     `
 	processOverrides(&cfg, []byte(ov))
 	got := cfg
-	want := config{
+	want := Config{
 		DBHost: "newHost",
 		DBName: "origName",
 		Quota:  QuotaSettings{QPS: 1, Burst: 2, MaxEntries: 17, RecordOnly: &f},

internal/postgres/benchmarks_test.go

@@ -34,10 +34,11 @@ var testQueries = []string{
 
 func BenchmarkSearch(b *testing.B) {
 	ctx := context.Background()
-	if err := config.Init(ctx); err != nil {
+	cfg, err := config.Init(ctx)
+	if err != nil {
 		b.Fatal(err)
 	}
-	ddb, err := database.Open("pgx", config.DBConnInfo())
+	ddb, err := database.Open("pgx", cfg.DBConnInfo())
 	if err != nil {
 		b.Fatal(err)
 	}