Refactor target allocator config handling (#1957)

* Refactor target allocator config handling

* Move TargetAllocator flags to a separate file

* Fix yaml annotation on the TA config struct

* Clearly separate loading config from CLI and file

* Use an explicit flag set for target allocator flags

* Add changelog entry

* Pass config by value to PrometheusCRWatcher

.chloggen/feat_ta-cliconfig.yaml

@@ -0,0 +1,16 @@
+# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
+change_type: enhancement
+
+# The name of the component, or a single word describing the area of concern, (e.g. operator, target allocator, github action)
+component: target allocator
+
+# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
+note: Allow target allocator to be completely configured via the config file
+
+# One or more tracking issues related to the change
+issues: [2129]
+
+# (Optional) One or more lines of additional information to render under the primary note.
+# These lines will be padded with 2 spaces and then inserted directly into the document.
+# Use pipe (|) for multiline entries.
+subtext:

cmd/otel-allocator/config/config.go

@@ -16,11 +16,9 @@ package config
 
 import (
 	"errors"
-	"flag"
 	"fmt"
 	"io/fs"
 	"os"
-	"path/filepath"
 	"time"
 
 	"github.com/go-logr/logr"
@@ -31,7 +29,6 @@ import (
 	"gopkg.in/yaml.v2"
 	"k8s.io/client-go/rest"
 	"k8s.io/client-go/tools/clientcmd"
-	"k8s.io/client-go/util/homedir"
 	"k8s.io/klog/v2"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/log/zap"
@@ -42,8 +39,12 @@ const DefaultConfigFilePath string = "/conf/targetallocator.yaml"
 const DefaultCRScrapeInterval model.Duration = model.Duration(time.Second * 30)
 
 type Config struct {
+	ListenAddr             string             `yaml:"listen_addr,omitempty"`
+	KubeConfigFilePath     string             `yaml:"kube_config_file_path,omitempty"`
+	ClusterConfig          *rest.Config       `yaml:"-"`
+	RootLogger             logr.Logger        `yaml:"-"`
 	LabelSelector          map[string]string  `yaml:"label_selector,omitempty"`
-	Config                 *promconfig.Config `yaml:"config"`
+	PromConfig             *promconfig.Config `yaml:"config"`
 	AllocationStrategy     *string            `yaml:"allocation_strategy,omitempty"`
 	FilterStrategy         *string            `yaml:"filter_strategy,omitempty"`
 	PrometheusCR           PrometheusCRConfig `yaml:"prometheus_cr,omitempty"`
@@ -52,6 +53,7 @@ type Config struct {
 }
 
 type PrometheusCRConfig struct {
+	Enabled        bool           `yaml:"enabled,omitempty"`
 	ScrapeInterval model.Duration `yaml:"scrape_interval,omitempty"`
 }
 
@@ -69,26 +71,45 @@ func (c Config) GetTargetsFilterStrategy() string {
 	return ""
 }
 
-type PrometheusCRWatcherConfig struct {
-	Enabled *bool
+func LoadFromFile(file string, target *Config) error {
+	return unmarshal(target, file)
 }
 
-type CLIConfig struct {
-	ListenAddr     *string
-	ConfigFilePath *string
-	ClusterConfig  *rest.Config
-	// KubeConfigFilePath empty if in cluster configuration is in use
-	KubeConfigFilePath string
-	RootLogger         logr.Logger
-	PromCRWatcherConf  PrometheusCRWatcherConfig
-}
+func LoadFromCLI(target *Config, flagSet *pflag.FlagSet) error {
+	var err error
+	// set the rest of the config attributes based on command-line flag values
+	target.RootLogger = zap.New(zap.UseFlagOptions(&zapCmdLineOpts))
+	klog.SetLogger(target.RootLogger)
+	ctrl.SetLogger(target.RootLogger)
+
+	target.KubeConfigFilePath, err = getKubeConfigFilePath(flagSet)
+	if err != nil {
+		return err
+	}
+	clusterConfig, err := clientcmd.BuildConfigFromFlags("", target.KubeConfigFilePath)
+	if err != nil {
+		pathError := &fs.PathError{}
+		if ok := errors.As(err, &pathError); !ok {
+			return err
+		}
+		clusterConfig, err = rest.InClusterConfig()
+		if err != nil {
+			return err
+		}
+	}
+	target.ClusterConfig = clusterConfig
 
-func Load(file string) (Config, error) {
-	cfg := createDefaultConfig()
-	if err := unmarshal(&cfg, file); err != nil {
-		return Config{}, err
+	target.ListenAddr, err = getListenAddr(flagSet)
+	if err != nil {
+		return err
+	}
+
+	target.PrometheusCR.Enabled, err = getPrometheusCREnabled(flagSet)
+	if err != nil {
+		return err
 	}
-	return cfg, nil
+
+	return nil
 }
 
 func unmarshal(cfg *Config, configFile string) error {
@@ -103,52 +124,47 @@ func unmarshal(cfg *Config, configFile string) error {
 	return nil
 }
 
-func createDefaultConfig() Config {
+func CreateDefaultConfig() Config {
 	return Config{
 		PrometheusCR: PrometheusCRConfig{
 			ScrapeInterval: DefaultCRScrapeInterval,
 		},
 	}
 }
 
-func ParseCLI() (CLIConfig, error) {
-	opts := zap.Options{}
-	opts.BindFlags(flag.CommandLine)
-	cLIConf := CLIConfig{
-		ListenAddr:     pflag.String("listen-addr", ":8080", "The address where this service serves."),
-		ConfigFilePath: pflag.String("config-file", DefaultConfigFilePath, "The path to the config file."),
-		PromCRWatcherConf: PrometheusCRWatcherConfig{
-			Enabled: pflag.Bool("enable-prometheus-cr-watcher", false, "Enable Prometheus CRs as target sources"),
-		},
+func Load() (*Config, string, error) {
+	var err error
+
+	flagSet := getFlagSet(pflag.ExitOnError)
+	err = flagSet.Parse(os.Args)
+	if err != nil {
+		return nil, "", err
 	}
-	kubeconfigPath := pflag.String("kubeconfig-path", filepath.Join(homedir.HomeDir(), ".kube", "config"), "absolute path to the KubeconfigPath file")
-	pflag.Parse()
 
-	cLIConf.RootLogger = zap.New(zap.UseFlagOptions(&opts))
-	klog.SetLogger(cLIConf.RootLogger)
-	ctrl.SetLogger(cLIConf.RootLogger)
+	config := CreateDefaultConfig()
 
-	clusterConfig, err := clientcmd.BuildConfigFromFlags("", *kubeconfigPath)
-	cLIConf.KubeConfigFilePath = *kubeconfigPath
+	// load the config from the config file
+	configFilePath, err := getConfigFilePath(flagSet)
 	if err != nil {
-		pathError := &fs.PathError{}
-		if ok := errors.As(err, &pathError); !ok {
-			return CLIConfig{}, err
-		}
-		clusterConfig, err = rest.InClusterConfig()
-		if err != nil {
-			return CLIConfig{}, err
-		}
-		cLIConf.KubeConfigFilePath = "" // reset as we use in cluster configuration
+		return nil, "", err
+	}
+	err = LoadFromFile(configFilePath, &config)
+	if err != nil {
+		return nil, "", err
 	}
-	cLIConf.ClusterConfig = clusterConfig
-	return cLIConf, nil
+
+	err = LoadFromCLI(&config, flagSet)
+	if err != nil {
+		return nil, "", err
+	}
+
+	return &config, configFilePath, nil
 }
 
 // ValidateConfig validates the cli and file configs together.
-func ValidateConfig(config *Config, cliConfig *CLIConfig) error {
-	scrapeConfigsPresent := (config.Config != nil && len(config.Config.ScrapeConfigs) > 0)
-	if !(*cliConfig.PromCRWatcherConf.Enabled || scrapeConfigsPresent) {
+func ValidateConfig(config *Config) error {
+	scrapeConfigsPresent := (config.PromConfig != nil && len(config.PromConfig.ScrapeConfigs) > 0)
+	if !(config.PrometheusCR.Enabled || scrapeConfigsPresent) {
 		return fmt.Errorf("at least one scrape config must be defined, or Prometheus CR watching must be enabled")
 	}
 	return nil

cmd/otel-allocator/config/config_test.go

@@ -51,7 +51,7 @@ func TestLoad(t *testing.T) {
 				PrometheusCR: PrometheusCRConfig{
 					ScrapeInterval: model.Duration(time.Second * 60),
 				},
-				Config: &promconfig.Config{
+				PromConfig: &promconfig.Config{
 					GlobalConfig: promconfig.GlobalConfig{
 						ScrapeInterval:     model.Duration(60 * time.Second),
 						ScrapeTimeout:      model.Duration(10 * time.Second),
@@ -99,7 +99,7 @@ func TestLoad(t *testing.T) {
 			args: args{
 				file: "./testdata/no_config.yaml",
 			},
-			want:    createDefaultConfig(),
+			want:    CreateDefaultConfig(),
 			wantErr: assert.NoError,
 		},
 		{
@@ -115,7 +115,7 @@ func TestLoad(t *testing.T) {
 				PrometheusCR: PrometheusCRConfig{
 					ScrapeInterval: DefaultCRScrapeInterval,
 				},
-				Config: &promconfig.Config{
+				PromConfig: &promconfig.Config{
 					GlobalConfig: promconfig.GlobalConfig{
 						ScrapeInterval:     model.Duration(60 * time.Second),
 						ScrapeTimeout:      model.Duration(10 * time.Second),
@@ -163,7 +163,8 @@ func TestLoad(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := Load(tt.args.file)
+			got := CreateDefaultConfig()
+			err := LoadFromFile(tt.args.file, &got)
 			if !tt.wantErr(t, err, fmt.Sprintf("Load(%v)", tt.args.file)) {
 				return
 			}
@@ -173,53 +174,46 @@ func TestLoad(t *testing.T) {
 }
 
 func TestValidateConfig(t *testing.T) {
-	enabled := true
-	disabled := false
 	testCases := []struct {
 		name        string
-		cliConfig   CLIConfig
 		fileConfig  Config
 		expectedErr error
 	}{
 		{
 			name:        "promCR enabled, no Prometheus config",
-			cliConfig:   CLIConfig{PromCRWatcherConf: PrometheusCRWatcherConfig{Enabled: &enabled}},
-			fileConfig:  Config{Config: nil},
+			fileConfig:  Config{PromConfig: nil, PrometheusCR: PrometheusCRConfig{Enabled: true}},
 			expectedErr: nil,
 		},
 		{
 			name:        "promCR disabled, no Prometheus config",
-			cliConfig:   CLIConfig{PromCRWatcherConf: PrometheusCRWatcherConfig{Enabled: &disabled}},
-			fileConfig:  Config{Config: nil},
+			fileConfig:  Config{PromConfig: nil},
 			expectedErr: fmt.Errorf("at least one scrape config must be defined, or Prometheus CR watching must be enabled"),
 		},
 		{
 			name:        "promCR disabled, Prometheus config present, no scrapeConfigs",
-			cliConfig:   CLIConfig{PromCRWatcherConf: PrometheusCRWatcherConfig{Enabled: &disabled}},
-			fileConfig:  Config{Config: &promconfig.Config{}},
+			fileConfig:  Config{PromConfig: &promconfig.Config{}},
 			expectedErr: fmt.Errorf("at least one scrape config must be defined, or Prometheus CR watching must be enabled"),
 		},
 		{
-			name:      "promCR disabled, Prometheus config present, scrapeConfigs present",
-			cliConfig: CLIConfig{PromCRWatcherConf: PrometheusCRWatcherConfig{Enabled: &disabled}},
+			name: "promCR disabled, Prometheus config present, scrapeConfigs present",
 			fileConfig: Config{
-				Config: &promconfig.Config{ScrapeConfigs: []*promconfig.ScrapeConfig{{}}},
+				PromConfig: &promconfig.Config{ScrapeConfigs: []*promconfig.ScrapeConfig{{}}},
 			},
 			expectedErr: nil,
 		},
 		{
-			name:      "promCR enabled, Prometheus config present, scrapeConfigs present",
-			cliConfig: CLIConfig{PromCRWatcherConf: PrometheusCRWatcherConfig{Enabled: &enabled}},
+			name: "promCR enabled, Prometheus config present, scrapeConfigs present",
 			fileConfig: Config{
-				Config: &promconfig.Config{ScrapeConfigs: []*promconfig.ScrapeConfig{{}}},
+				PromConfig:   &promconfig.Config{ScrapeConfigs: []*promconfig.ScrapeConfig{{}}},
+				PrometheusCR: PrometheusCRConfig{Enabled: true},
 			},
 			expectedErr: nil,
 		},
 	}
 
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {
-			err := ValidateConfig(&tc.fileConfig, &tc.cliConfig)
+			err := ValidateConfig(&tc.fileConfig)
 			assert.Equal(t, tc.expectedErr, err)
 		})
 	}

cmd/otel-allocator/config/flags.go

@@ -0,0 +1,64 @@
+// Copyright The OpenTelemetry Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package config
+
+import (
+	"flag"
+	"path/filepath"
+
+	"github.com/spf13/pflag"
+	"k8s.io/client-go/util/homedir"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+)
+
+// Flag names.
+const (
+	targetAllocatorName         = "target-allocator"
+	configFilePathFlagName      = "config-file"
+	listenAddrFlagName          = "listen-addr"
+	prometheusCREnabledFlagName = "enable-prometheus-cr-watcher"
+	kubeConfigPathFlagName      = "kubeconfig-path"
+)
+
+// We can't bind this flag to our FlagSet, so we need to handle it separately.
+var zapCmdLineOpts zap.Options
+
+func getFlagSet(errorHandling pflag.ErrorHandling) *pflag.FlagSet {
+	flagSet := pflag.NewFlagSet(targetAllocatorName, errorHandling)
+	flagSet.String(configFilePathFlagName, DefaultConfigFilePath, "The path to the config file.")
+	flagSet.String(listenAddrFlagName, ":8080", "The address where this service serves.")
+	flagSet.Bool(prometheusCREnabledFlagName, false, "Enable Prometheus CRs as target sources")
+	flagSet.String(kubeConfigPathFlagName, filepath.Join(homedir.HomeDir(), ".kube", "config"), "absolute path to the KubeconfigPath file")
+	zapFlagSet := flag.NewFlagSet("", flag.ErrorHandling(errorHandling))
+	zapCmdLineOpts.BindFlags(zapFlagSet)
+	flagSet.AddGoFlagSet(zapFlagSet)
+	return flagSet
+}
+
+func getConfigFilePath(flagSet *pflag.FlagSet) (string, error) {
+	return flagSet.GetString(configFilePathFlagName)
+}
+
+func getKubeConfigFilePath(flagSet *pflag.FlagSet) (string, error) {
+	return flagSet.GetString(kubeConfigPathFlagName)
+}
+
+func getListenAddr(flagSet *pflag.FlagSet) (string, error) {
+	return flagSet.GetString(listenAddrFlagName)
+}
+
+func getPrometheusCREnabled(flagSet *pflag.FlagSet) (bool, error) {
+	return flagSet.GetBool(prometheusCREnabledFlagName)
+}