From 68c546358cfa543ca5baf35d64f2bba3e91040dd Mon Sep 17 00:00:00 2001 From: Wei Tie Date: Sun, 10 Dec 2017 19:34:42 -0800 Subject: [PATCH] Separate state store driver and address In the past, the state store driver address's schema part indicates the state store driver type, however, it limits https endpoints. This commit separates the driver type and address to make it align with updated netplugin and netmaster's CLI. Signed-off-by: Wei Tie --- common/test/utils.go | 11 +++++------ common/types/types.go | 3 ++- db/local_test.go | 18 ++++++++++++------ main.go | 10 +++++++++- scripts/systemtests.sh | 6 ++++-- scripts/systemtests_in_container.sh | 4 ++-- scripts/unittests_in_container.sh | 14 +++++++------- state/authz_test.go | 3 ++- state/consulstatedriver.go | 15 +++++++++++++-- state/consulstatedriver_test.go | 5 ++++- state/driverfactory.go | 17 ++++++----------- state/etcdstatedriver.go | 18 ++++++++++++++---- state/etcdstatedriver_test.go | 5 ++++- systemtests/init_test.go | 3 ++- 14 files changed, 86 insertions(+), 46 deletions(-) diff --git a/common/test/utils.go b/common/test/utils.go index 323a0140..b54ae94c 100644 --- a/common/test/utils.go +++ b/common/test/utils.go @@ -3,7 +3,6 @@ package test import ( "os" "os/exec" - "strings" log "github.com/Sirupsen/logrus" ) @@ -14,16 +13,16 @@ import ( // the datastore is already empty), so we'll use "|| true" to ignore failures. // params: // addr: address of the datastore, e.g., "etcd://w.x.y.z:2379" -func EmptyDatastore(addr string) { - switch { - case strings.HasPrefix(addr, "etcd://"): +func EmptyDatastore(driver string) { + switch driver { + case "etcd": cmd := "docker exec " + os.Getenv("ETCD_CONTAINER_NAME") + " /etcdctl rm --recursive /auth_proxy || true" log.Debugln("Emptying datastore:", cmd) if err := exec.Command("/bin/sh", "-c", cmd).Run(); err != nil { log.Fatalln("Failed to clear etcd: ", err) } - case strings.HasPrefix(addr, "consul://"): + case "consul": // NOTE: consul keys do not start with a / cmd := "docker exec " + os.Getenv("CONSUL_CONTAINER_NAME") + " consul kv delete -recurse auth_proxy || true" log.Debugln("Emptying datastore:", cmd) @@ -32,6 +31,6 @@ func EmptyDatastore(addr string) { log.Fatalln("Failed to clear consul: ", err) } default: - log.Fatalln("Unknown data store for address:", addr) + log.Fatalln("Unknown data store: %q", driver) } } diff --git a/common/types/types.go b/common/types/types.go index 77c7d522..f8fd7748 100644 --- a/common/types/types.go +++ b/common/types/types.go @@ -113,7 +113,8 @@ type LdapConfiguration struct { // StoreURL: URL of the key-value store // type KVStoreConfig struct { - StoreURL string `json:"kvstore-url"` + StoreURL string `json:"kvstore-url"` + StoreDriver string `json:"kvstore-driver"` } // diff --git a/db/local_test.go b/db/local_test.go index 225d2814..7538e5d6 100644 --- a/db/local_test.go +++ b/db/local_test.go @@ -42,24 +42,30 @@ var ( }, } - invalidUsers = []string{"xxx", "yyy", "zzz"} - builtInUsers = []string{types.Admin.String(), types.Ops.String()} - datastoreAddress = "" + invalidUsers = []string{"xxx", "yyy", "zzz"} + builtInUsers = []string{types.Admin.String(), types.Ops.String()} + datastoreDriver = "" ) // This runs before each test and guarantees the datastore is emptied out. func (s *dbSuite) SetUpTest(c *C) { - test.EmptyDatastore(datastoreAddress) + test.EmptyDatastore(datastoreDriver) } // SetUpSuite sets up the environment for tests. func (s *dbSuite) SetUpSuite(c *C) { - datastoreAddress = strings.TrimSpace(os.Getenv("DATASTORE_ADDRESS")) + datastoreAddress := strings.TrimSpace(os.Getenv("DATASTORE_ADDRESS")) + datastoreDriver = strings.TrimSpace(os.Getenv("DATASTORE_DRIVER")) + + if datastoreDriver != state.EtcdName && datastoreDriver != state.ConsulName { + log.Fatalln("you must provide a DATASTORE_DRIVER (options: [etcd, consul])") + } + if common.IsEmpty(datastoreAddress) { log.Fatalln("you must provide a DATASTORE_ADDRESS") } - if err := state.InitializeStateDriver(datastoreAddress); err != nil { + if err := state.InitializeStateDriver(datastoreDriver, datastoreAddress); err != nil { log.Fatalln(err) } diff --git a/main.go b/main.go index 9de757d2..2ef9107b 100644 --- a/main.go +++ b/main.go @@ -23,6 +23,7 @@ const ( var ( // flags + dataStoreDriver string // driver of the data store used by netmaster dataStoreAddress string // address of the data store used by netmaster debug bool // if set, log level is set to `debug` listenAddress string // address we listen on @@ -107,6 +108,13 @@ func processFlags() { "address of the state store used by netmaster", ) + flag.StringVar( + &dataStoreDriver, + "data-store-driver", + "", + "driver of the state store used by netmaster", + ) + flag.Parse() } @@ -211,7 +219,7 @@ func main() { } // Initialize data store - if err := state.InitializeStateDriver(dataStoreAddress); err != nil { + if err := state.InitializeStateDriver(dataStoreDriver, dataStoreAddress); err != nil { log.Fatalln(err) return } diff --git a/scripts/systemtests.sh b/scripts/systemtests.sh index 8c8f29ff..119bb27c 100755 --- a/scripts/systemtests.sh +++ b/scripts/systemtests.sh @@ -101,7 +101,8 @@ ETCD_PROXY_CONTAINER_ID=$( -e NO_NETMASTER_STARTUP_CHECK=true \ --network $NETWORK_NAME \ $PROXY_IMAGE \ - --data-store-address="etcd://$ETCD_CONTAINER_IP:2379" \ + --data-store-driver="etcd" \ + --data-store-address="http://$ETCD_CONTAINER_IP:2379" \ --tls-certificate=/local_certs/cert.pem \ --tls-key-file=/local_certs/local.key \ --listen-address=0.0.0.0:10000 \ @@ -120,7 +121,8 @@ CONSUL_PROXY_CONTAINER_ID=$( --network $NETWORK_NAME \ -e NO_NETMASTER_STARTUP_CHECK=true \ $PROXY_IMAGE \ - --data-store-address="consul://$CONSUL_CONTAINER_IP:8500" \ + --data-store-driver="consul" \ + --data-store-address="http://$CONSUL_CONTAINER_IP:8500" \ --tls-certificate=/local_certs/cert.pem \ --tls-key-file=/local_certs/local.key \ --listen-address=0.0.0.0:10001 \ diff --git a/scripts/systemtests_in_container.sh b/scripts/systemtests_in_container.sh index cde098c2..2027a9af 100755 --- a/scripts/systemtests_in_container.sh +++ b/scripts/systemtests_in_container.sh @@ -9,7 +9,7 @@ echo "Running systemtests against etcd" echo "" set -x -PROXY_ADDRESS=$1 DATASTORE_ADDRESS="etcd://$ETCD_CONTAINER_IP:2379" go test -v -timeout 5m ./systemtests -check.v +PROXY_ADDRESS=$1 DATASTORE_DRIVER=etcd DATASTORE_ADDRESS="http://$ETCD_CONTAINER_IP:2379" go test -v -timeout 5m ./systemtests -check.v EXIT_CODES+=($?) set +x @@ -18,7 +18,7 @@ echo "Running systemtests against consul" echo "" set -x -PROXY_ADDRESS=$2 DATASTORE_ADDRESS="consul://$CONSUL_CONTAINER_IP:8500" go test -v -timeout 5m ./systemtests -check.v +PROXY_ADDRESS=$2 DATASTORE_DRIVER=consul DATASTORE_ADDRESS="http://$CONSUL_CONTAINER_IP:8500" go test -v -timeout 5m ./systemtests -check.v EXIT_CODES+=($?) set +x diff --git a/scripts/unittests_in_container.sh b/scripts/unittests_in_container.sh index 2d27d569..2ea7704b 100755 --- a/scripts/unittests_in_container.sh +++ b/scripts/unittests_in_container.sh @@ -2,8 +2,8 @@ set -uo pipefail -ETCD_ADDRESS="etcd://$ETCD_CONTAINER_IP:2379" -CONSUL_ADDRESS="consul://$CONSUL_CONTAINER_IP:8500" +ETCD_ADDRESS="http://$ETCD_CONTAINER_IP:2379" +CONSUL_ADDRESS="http://$CONSUL_CONTAINER_IP:8500" EXIT_CODES=() @@ -13,13 +13,13 @@ echo "" echo "consul:" echo "" -DATASTORE_ADDRESS=$CONSUL_ADDRESS go test -v -timeout 1m ./db -check.v +DATASTORE_DRIVER=consul DATASTORE_ADDRESS=$CONSUL_ADDRESS go test -v -timeout 1m ./db -check.v EXIT_CODES+=($?) echo "" echo "etcd:" echo "" -DATASTORE_ADDRESS=$ETCD_ADDRESS go test -v -timeout 1m ./db -check.v +DATASTORE_DRIVER=etcd DATASTORE_ADDRESS=$ETCD_ADDRESS go test -v -timeout 1m ./db -check.v EXIT_CODES+=($?) echo "" @@ -29,15 +29,15 @@ echo "" echo "etcd:" echo "" -DATASTORE_ADDRESS=$ETCD_ADDRESS go test -run TestAuthZ* -v -timeout 1m ./state -check.v +DATASTORE_DRIVER=etcd DATASTORE_ADDRESS=$ETCD_ADDRESS go test -run TestAuthZ* -v -timeout 1m ./state -check.v EXIT_CODES+=($?) -DATASTORE_ADDRESS=$ETCD_ADDRESS go test -run TestEtcd* -v -timeout 1m ./state -check.v +DATASTORE_DRIVER=etcd DATASTORE_ADDRESS=$ETCD_ADDRESS go test -run TestEtcd* -v -timeout 1m ./state -check.v EXIT_CODES+=($?) echo "" echo "consul:" echo "" -DATASTORE_ADDRESS=$CONSUL_ADDRESS go test -run TestConsul* -v -timeout 1m ./state -check.v +DATASTORE_DRIVER=consul DATASTORE_ADDRESS=$CONSUL_ADDRESS go test -run TestConsul* -v -timeout 1m ./state -check.v EXIT_CODES+=($?) echo "" diff --git a/state/authz_test.go b/state/authz_test.go index 8c5a5312..7a317bed 100644 --- a/state/authz_test.go +++ b/state/authz_test.go @@ -19,7 +19,8 @@ var ( func TestAuthZInit(t *testing.T) { // create config for KV store config = types.KVStoreConfig{ - StoreURL: os.Getenv("DATASTORE_ADDRESS"), + StoreURL: os.Getenv("DATASTORE_ADDRESS"), + StoreDriver: os.Getenv("DATASTORE_Driver"), } // create a state driver diff --git a/state/consulstatedriver.go b/state/consulstatedriver.go index 4ef41f1a..2da8b972 100644 --- a/state/consulstatedriver.go +++ b/state/consulstatedriver.go @@ -3,6 +3,7 @@ package state import ( "errors" "fmt" + "net/url" "strings" "time" @@ -36,13 +37,23 @@ type ConsulStateDriver struct { // func (d *ConsulStateDriver) Init(config *types.KVStoreConfig) error { var err error + var endpoint *url.URL - if config == nil || !strings.Contains(config.StoreURL, "consul://") { + if config == nil { return errors.New("Invalid consul config") } + endpoint, err = url.Parse(config.StoreURL) + if err != nil { + return err + } + if endpoint.Scheme == "consul" { + endpoint.Scheme = "http" + } else if endpoint.Scheme != "http" && endpoint.Scheme != "https" { + return fmt.Errorf("invalid consul URL scheme %q", endpoint.Scheme) + } cfg := api.Config{ - Address: strings.TrimPrefix(config.StoreURL, "consul://"), + Address: endpoint.Host, } // create a consul client diff --git a/state/consulstatedriver_test.go b/state/consulstatedriver_test.go index b2d9e22f..20df85f2 100644 --- a/state/consulstatedriver_test.go +++ b/state/consulstatedriver_test.go @@ -8,7 +8,10 @@ import ( ) func setupConsulDriver(t *testing.T) *ConsulStateDriver { - config := types.KVStoreConfig{StoreURL: os.Getenv("DATASTORE_ADDRESS")} + config := types.KVStoreConfig{ + StoreURL: os.Getenv("DATASTORE_ADDRESS"), + StoreDriver: os.Getenv("DATASTORE_DRIVER"), + } driver := &ConsulStateDriver{} diff --git a/state/driverfactory.go b/state/driverfactory.go index e32638c1..d95c9c24 100644 --- a/state/driverfactory.go +++ b/state/driverfactory.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "reflect" - "strings" "github.com/contiv/auth_proxy/common" auth_errors "github.com/contiv/auth_proxy/common/errors" @@ -99,18 +98,14 @@ func GetStateDriver() (types.StateDriver, error) { // dataStoreAddress: address of the data store // return values: // returns any error as NewStateDriver() + validation errors -func InitializeStateDriver(dataStoreAddress string) error { +func InitializeStateDriver(dataStoreDriver, dataStoreAddress string) error { + if dataStoreDriver != EtcdName && dataStoreDriver != ConsulName { + return errors.New("Invalid data store driver, please set --data-store-driver (options: [etcd, consul])") + } if common.IsEmpty(dataStoreAddress) { return errors.New("Empty data store address, please set --data-store-address") } + _, err := NewStateDriver(dataStoreDriver, &types.KVStoreConfig{StoreURL: dataStoreAddress}) + return err - if strings.HasPrefix(dataStoreAddress, EtcdName+"://") { - _, err := NewStateDriver(EtcdName, &types.KVStoreConfig{StoreURL: dataStoreAddress}) - return err - } else if strings.HasPrefix(dataStoreAddress, ConsulName+"://") { - _, err := NewStateDriver(ConsulName, &types.KVStoreConfig{StoreURL: dataStoreAddress}) - return err - } - - return errors.New("Invalid data store address") } diff --git a/state/etcdstatedriver.go b/state/etcdstatedriver.go index 1cd5e8fb..bacd06fc 100644 --- a/state/etcdstatedriver.go +++ b/state/etcdstatedriver.go @@ -3,6 +3,7 @@ package state import ( "errors" "fmt" + "net/url" "reflect" "strings" "time" @@ -48,15 +49,24 @@ type EtcdStateDriver struct { // func (d *EtcdStateDriver) Init(config *types.KVStoreConfig) error { var err error + var endpoint *url.URL - if config == nil || !strings.Contains(config.StoreURL, "etcd://") { + if config == nil { return errors.New("Invalid etcd config") } - // configure etcd endpoints - etcdURL := strings.Replace(config.StoreURL, "etcd://", "http://", 1) + endpoint, err = url.Parse(config.StoreURL) + if err != nil { + return err + } + if endpoint.Scheme == "etcd" { + endpoint.Scheme = "http" + } else if endpoint.Scheme != "http" && endpoint.Scheme != "https" { + return fmt.Errorf("invalid etcd URL scheme %q", endpoint.Scheme) + } + etcdConfig := client.Config{ - Endpoints: []string{etcdURL}, + Endpoints: []string{endpoint.String()}, } // create etcd client diff --git a/state/etcdstatedriver_test.go b/state/etcdstatedriver_test.go index d2089325..d914526b 100644 --- a/state/etcdstatedriver_test.go +++ b/state/etcdstatedriver_test.go @@ -23,7 +23,10 @@ func Test(t *testing.T) { // Setup configuration needed to access local etcd func setupEtcdDriver(t *testing.T) *EtcdStateDriver { - config := types.KVStoreConfig{StoreURL: os.Getenv("DATASTORE_ADDRESS")} + config := types.KVStoreConfig{ + StoreURL: os.Getenv("DATASTORE_ADDRESS"), + StoreDriver: os.Getenv("DATASTORE_DRIVER"), + } driver := &EtcdStateDriver{} diff --git a/systemtests/init_test.go b/systemtests/init_test.go index ab43f43d..01a60e49 100644 --- a/systemtests/init_test.go +++ b/systemtests/init_test.go @@ -49,9 +49,10 @@ func Test(t *testing.T) { // DATASTORE_ADDRESS is set in ./scripts/systemtests_in_container.sh datastoreAddress := strings.TrimSpace(os.Getenv("DATASTORE_ADDRESS")) + datastoreDriver := strings.TrimSpace(os.Getenv("DATASTORE_DRIVER")) log.Info("Initializing datastore") - if err := state.InitializeStateDriver(datastoreAddress); err != nil { + if err := state.InitializeStateDriver(datastoreDriver, datastoreAddress); err != nil { log.Fatalln(err) }