Override etcd suboptions independent of the datastore type

There was previously an issue with the etcd suboptions not being applied from the felix configuration if the etcd datastore type wasn't set the etcd in the felix configuration as well. This is a problem because the default datastore type is etcd, so users are bothering to explicitly set the datastore type to etcd. Since the etcd suboptions only have an effect if the datastore is etcd we can just overide the suboptions with what's in the felix configuration, which fixes this issue.

config/config_params.go

@@ -232,6 +232,8 @@ type Config struct {
 	SidecarAccelerationEnabled bool `config:"bool;false"`
 	XDPEnabled                 bool `config:"bool;false"`
 	GenericXDPEnabled          bool `config:"bool;false"`
+
+	loadClientConfigFromEnvironment func() (*apiconfig.CalicoAPIConfig, error)
 }
 
 type ProtoPort struct {
@@ -398,33 +400,50 @@ func (config *Config) DatastoreConfig() apiconfig.CalicoAPIConfig {
 	// To achieve that, first build a CalicoAPIConfig using libcalico-go's
 	// LoadClientConfigFromEnvironment - which means incorporating defaults and CALICO_XXX_YYY
 	// and XXX_YYY variables.
-	cfg, err := apiconfig.LoadClientConfigFromEnvironment()
+	cfg, err := config.loadClientConfigFromEnvironment()
 	if err != nil {
 		log.WithError(err).Panic("Failed to create datastore config")
 	}
 
 	// Now allow FELIX_XXXYYY variables or XxxYyy config file settings to override that, in the
-	// etcd case.
-	if config.setByConfigFileOrEnvironment("DatastoreType") && config.DatastoreType == "etcdv3" {
-		cfg.Spec.DatastoreType = apiconfig.EtcdV3
-		// Endpoints.
-		if config.setByConfigFileOrEnvironment("EtcdEndpoints") && len(config.EtcdEndpoints) > 0 {
-			cfg.Spec.EtcdEndpoints = strings.Join(config.EtcdEndpoints, ",")
-		} else if config.setByConfigFileOrEnvironment("EtcdAddr") {
-			cfg.Spec.EtcdEndpoints = config.EtcdScheme + "://" + config.EtcdAddr
-		}
-		// TLS.
-		if config.setByConfigFileOrEnvironment("EtcdKeyFile") {
-			cfg.Spec.EtcdKeyFile = config.EtcdKeyFile
-		}
-		if config.setByConfigFileOrEnvironment("EtcdCertFile") {
-			cfg.Spec.EtcdCertFile = config.EtcdCertFile
-		}
-		if config.setByConfigFileOrEnvironment("EtcdCaFile") {
-			cfg.Spec.EtcdCACertFile = config.EtcdCaFile
+	// etcd case. Note that that etcd options are set even if the DatastoreType isn't etcdv3.
+	// This allows the user to rely the default DatastoreType being etcdv3 and still being able
+	// to configure the other etcdv3 options. As of the time of this code change, the etcd options
+	// have no affect if the DatastoreType is not etcdv3.
+
+	// Datastore type, either etcdv3 or kubernetes
+	if config.setByConfigFileOrEnvironment("DatastoreType") {
+		log.Infof("Overriding DatastoreType from felix config to %s", config.DatastoreType)
+		if config.DatastoreType == string(apiconfig.EtcdV3) {
+			cfg.Spec.DatastoreType = apiconfig.EtcdV3
+		} else if config.DatastoreType == string(apiconfig.Kubernetes) {
+			cfg.Spec.DatastoreType = apiconfig.Kubernetes
 		}
 	}
 
+	// Endpoints.
+	if config.setByConfigFileOrEnvironment("EtcdEndpoints") && len(config.EtcdEndpoints) > 0 {
+		log.Infof("Overriding EtcdEndpoints from felix config to %s", config.EtcdEndpoints)
+		cfg.Spec.EtcdEndpoints = strings.Join(config.EtcdEndpoints, ",")
+	} else if config.setByConfigFileOrEnvironment("EtcdAddr") {
+		etcdEndpoints := config.EtcdScheme + "://" + config.EtcdAddr
+		log.Infof("Overriding EtcdEndpoints from felix config to %s", etcdEndpoints)
+		cfg.Spec.EtcdEndpoints = etcdEndpoints
+	}
+	// TLS.
+	if config.setByConfigFileOrEnvironment("EtcdKeyFile") {
+		log.Infof("Overriding EtcdKeyFile from felix config to %s", config.EtcdKeyFile)
+		cfg.Spec.EtcdKeyFile = config.EtcdKeyFile
+	}
+	if config.setByConfigFileOrEnvironment("EtcdCertFile") {
+		log.Infof("Overriding EtcdCertFile from felix config to %s", config.EtcdCertFile)
+		cfg.Spec.EtcdCertFile = config.EtcdCertFile
+	}
+	if config.setByConfigFileOrEnvironment("EtcdCaFile") {
+		log.Infof("Overriding EtcdCaFile from felix config to %s", config.EtcdCaFile)
+		cfg.Spec.EtcdCACertFile = config.EtcdCaFile
+	}
+
 	if !config.IpInIpEnabled && !config.VXLANEnabled {
 		// Polling k8s for node updates is expensive (because we get many superfluous
 		// updates) so disable if we don't need it.
@@ -619,6 +638,10 @@ func (config *Config) RawValues() map[string]string {
 	return config.rawValues
 }
 
+func (config *Config) SetLoadClientConfigFromEnvironmentFunction(fnc func() (*apiconfig.CalicoAPIConfig, error)) {
+	config.loadClientConfigFromEnvironment = fnc
+}
+
 func New() *Config {
 	if knownParams == nil {
 		loadParams()
@@ -637,6 +660,8 @@ func New() *Config {
 		hostname = strings.ToLower(os.Getenv("HOSTNAME"))
 	}
 	p.FelixHostname = hostname
+	p.loadClientConfigFromEnvironment = apiconfig.LoadClientConfigFromEnvironment
+
 	return p
 }
 

config/config_params_test.go

@@ -18,6 +18,8 @@ import (
 	"regexp"
 
 	. "github.com/projectcalico/felix/config"
+	"github.com/projectcalico/felix/testutils"
+	"github.com/projectcalico/libcalico-go/lib/apiconfig"
 
 	"net"
 	"reflect"
@@ -53,6 +55,7 @@ var _ = Describe("FelixConfig vs ConfigParams parity", func() {
 		"IpInIpTunnelAddr",
 		"IPv4VXLANTunnelAddr",
 		"VXLANTunnelMACAddr",
+		"loadClientConfigFromEnvironment",
 	}
 	cpFieldNameToFC := map[string]string{
 		"IpInIpEnabled":                      "IPIPEnabled",
@@ -427,6 +430,87 @@ var _ = Describe("DatastoreConfig tests", func() {
 			Expect(c.DatastoreConfig().Spec.K8sDisableNodePoll).To(BeTrue())
 		})
 	})
+
+	Describe("with the configuration set only from the common calico configuration", func() {
+		BeforeEach(func() {
+			c = New()
+			c.SetLoadClientConfigFromEnvironmentFunction(func() (*apiconfig.CalicoAPIConfig, error) {
+				return &apiconfig.CalicoAPIConfig{
+					Spec: apiconfig.CalicoAPIConfigSpec{
+						DatastoreType: apiconfig.EtcdV3,
+						EtcdConfig: apiconfig.EtcdConfig{
+							EtcdEndpoints:  "http://localhost:1234",
+							EtcdKeyFile:    testutils.TestDataFile("etcdkeyfile.key"),
+							EtcdCertFile:   testutils.TestDataFile("etcdcertfile.cert"),
+							EtcdCACertFile: testutils.TestDataFile("etcdcacertfile.cert"),
+						},
+					},
+				}, nil
+			})
+		})
+		It("sets the configuration options", func() {
+			spec := c.DatastoreConfig().Spec
+			Expect(spec.DatastoreType).To(Equal(apiconfig.EtcdV3))
+			Expect(spec.EtcdEndpoints).To(Equal("http://localhost:1234"))
+			Expect(spec.EtcdKeyFile).To(Equal(testutils.TestDataFile("etcdkeyfile.key")))
+			Expect(spec.EtcdCertFile).To(Equal(testutils.TestDataFile("etcdcertfile.cert")))
+			Expect(spec.EtcdCACertFile).To(Equal(testutils.TestDataFile("etcdcacertfile.cert")))
+		})
+	})
+	Describe("without setting the DatastoreType and setting the etcdv3 suboptions through the felix configuration", func() {
+		BeforeEach(func() {
+			c = New()
+			c.UpdateFrom(map[string]string{
+				"EtcdEndpoints": "http://localhost:1234",
+				"EtcdKeyFile":   testutils.TestDataFile("etcdkeyfile.key"),
+				"EtcdCertFile":  testutils.TestDataFile("etcdcertfile.cert"),
+				"EtcdCaFile":    testutils.TestDataFile("etcdcacertfile.cert"),
+			}, EnvironmentVariable)
+		})
+		It("sets the etcd suboptions", func() {
+			spec := c.DatastoreConfig().Spec
+			Expect(spec.DatastoreType).To(Equal(apiconfig.EtcdV3))
+			Expect(spec.EtcdEndpoints).To(Equal("http://localhost:1234/"))
+			Expect(spec.EtcdKeyFile).To(Equal(testutils.TestDataFile("etcdkeyfile.key")))
+			Expect(spec.EtcdCertFile).To(Equal(testutils.TestDataFile("etcdcertfile.cert")))
+			Expect(spec.EtcdCACertFile).To(Equal(testutils.TestDataFile("etcdcacertfile.cert")))
+		})
+	})
+	Describe("with the configuration set from the common calico configuration and the felix configuration", func() {
+		BeforeEach(func() {
+			c = New()
+
+			c.SetLoadClientConfigFromEnvironmentFunction(func() (*apiconfig.CalicoAPIConfig, error) {
+				return &apiconfig.CalicoAPIConfig{
+					Spec: apiconfig.CalicoAPIConfigSpec{
+						DatastoreType: apiconfig.Kubernetes,
+						EtcdConfig: apiconfig.EtcdConfig{
+							EtcdEndpoints:  "http://localhost:5432",
+							EtcdKeyFile:    testutils.TestDataFile("etcdkeyfileother.key"),
+							EtcdCertFile:   testutils.TestDataFile("etcdcertfileother.cert"),
+							EtcdCACertFile: testutils.TestDataFile("etcdcacertfileother.cert"),
+						},
+					},
+				}, nil
+			})
+
+			c.UpdateFrom(map[string]string{
+				"DatastoreType": "etcdv3",
+				"EtcdEndpoints": "http://localhost:1234",
+				"EtcdKeyFile":   testutils.TestDataFile("etcdkeyfile.key"),
+				"EtcdCertFile":  testutils.TestDataFile("etcdcertfile.cert"),
+				"EtcdCaFile":    testutils.TestDataFile("etcdcacertfile.cert"),
+			}, EnvironmentVariable)
+		})
+		It("sets the configuration to what the felix configuration is", func() {
+			spec := c.DatastoreConfig().Spec
+			Expect(spec.DatastoreType).To(Equal(apiconfig.EtcdV3))
+			Expect(spec.EtcdEndpoints).To(Equal("http://localhost:1234/"))
+			Expect(spec.EtcdKeyFile).To(Equal(testutils.TestDataFile("etcdkeyfile.key")))
+			Expect(spec.EtcdCertFile).To(Equal(testutils.TestDataFile("etcdcertfile.cert")))
+			Expect(spec.EtcdCACertFile).To(Equal(testutils.TestDataFile("etcdcacertfile.cert")))
+		})
+	})
 })
 
 var _ = DescribeTable("Config validation",

testutils/file.go

@@ -0,0 +1,12 @@
+package testutils
+
+import (
+	"os"
+	"path"
+)
+
+func TestDataFile(name string) string {
+	dir, _ := os.Getwd()
+
+	return path.Join(dir, "testdata", name)
+}