Add etcd backed storage #1108

dqminh · 2017-10-27T14:30:29Z

This adds a storage implementation backed by etcd. This should be useful mostly in cases where we dont want to depend on a separate SQL cluster ( and etcd is easier to operate ), or if we dont want to incur some overhead of using kubernetes CRD.

There are no documentation yet ! Once this looks good i can try to add them.

The previous test doesnt actually testing ListConnectors code. For example the following pseudocode will pass the test: ``` ListConnectors() { return nil, nil } ``` Instead change to actually fetch and compare list of connectors, ordering by name

ericchiang

Ran this locally and it passed all the conformance tests. Got a few lint errors but other than that it seems to run fine.

ericchiang · 2017-10-27T15:25:36Z

storage/etcd/config.go

+)
+
+type EtcdSSL struct {
+	ServerName string `json:"server_name" yaml:"server_name"`


nit: we use camel case for struct tags. json:"serverName"

Why don't we just use transport.TLSInfo https://github.com/coreos/etcd/blob/master/pkg/transport/listener.go#L59?

tlsInfo := transport.TLSInfo{ CertFile: "/tmp/test-certs/test-name-1.pem", KeyFile: "/tmp/test-certs/test-name-1-key.pem", TrustedCAFile: "/tmp/test-certs/trusted-ca.pem", } tlsConfig, err := tlsInfo.ClientConfig() ... clientv3.New(clientv3.Config{ ... TLS: tlsConfig, })

Ok, nvm. It's for json, yaml tagging.

ericchiang · 2017-10-27T15:25:48Z

storage/etcd/config.go

+	CertFile   string `json:"cert_file" yaml:"cert_file"`
+}
+
+type Etcd struct {


Allow a prefix

Hmm what do you mean ? Like setting a etcd namespace ? I think its usually done now with the help of the grpc-proxy, isnt it ?

Like setting a etcd namespace ?

Yeah an equivalent of etcd grpc-proxy start --namespace=<prefix>

I think its usually done now with the help of the grpc-proxy, isnt it ?

I think it depends on your setup. But yeah we can document the grpc-proxy solution then add native namespace functionality later.

Still need struct tags

Endpoints []string `yaml:"endpoints"`

ericchiang · 2017-10-27T15:27:56Z

storage/etcd/config.go

+
+type Etcd struct {
+	Endpoints        []string `json:"endpoints" yaml:"endpoints"`
+	DialTimeout      int      `json:"dial_timeout" yaml:"dial_timeout"`


We generally don't make everything configurable just because the client library does. Can we set DialTimeout and AutoSyncInterval to reasonable defaults and if there are compelling reasons to expose them later we can?

Hmm that works for me to. I can set them to the same default as etcdctl.

ericchiang · 2017-10-27T15:28:05Z

storage/etcd/config.go

+	Username         string   `json:"username" yaml:"username"`
+	Password         string   `json:"password" yaml:"password"`
+	SSL              EtcdSSL  `json:"ssl" yaml:"ssl"`
+	RejectOldCluster bool     `json:"reject-old-cluster" yaml:"reject-old-cluster"`


What does this do?

// RejectOldCluster when set will refuse to create a client against an outdated cluster. RejectOldCluster bool `json:"reject-old-cluster"`

as before, will also remove this knob for now.

ericchiang · 2017-10-27T15:31:06Z

storage/etcd/etcd.go

+	return c.txnUpdate(ctx, key, func(currentValue []byte) ([]byte, error) {
+		var current RefreshToken
+		if len(currentValue) > 0 {
+			if err := json.Unmarshal([]byte(currentValue), &current); err != nil {


Any thoughts on using a faster serialization format like protobuf? We use JSON for CRDs because CRDs only support JSON

I dont have any preference. Go's json stdlib is indeed slow-ish compared to other serialization format, but usually its not significant in practice i think.

ericchiang · 2017-10-27T15:31:56Z

storage/etcd/standup.sh

@@ -0,0 +1,109 @@
+#!/bin/bash
+
+if [ "$EUID" -ne 0 ]


Admittedly we've used these scripts less and less. I think instructions for downloading the binary releases would suffice https://github.com/coreos/etcd/releases

ericchiang · 2017-10-27T15:33:21Z

storage/etcd/etcd.go

+	return nil
+}
+
+func (c *conn) txnUpdate(ctx context.Context, key string, update func(current []byte) ([]byte, error)) error {


cc @xiang90 mind taking a look at this?

looks right to me.

we could do auto retry inside the txnUpdate by calling update func multiple times when there are conflicts (maybe it is already wrapped?).

I dont think auto-retry should be applied here. At least thats how the current test is structured ( also how sql transaction works ).
Although IIUC there are differences on the order of transaction between sql and non-sql storages now ( k8s or etcd ). In sql the first started transaction win vs the first finished transaction win in others.

@dqminh understood. thanks.

ericchiang · 2017-10-27T15:34:17Z

storage/etcd/etcd_test.go

+	}
+	newStorage := func() storage.Storage {
+		s := &Etcd{
+			Endpoints: []string{endpoints},


Weird that we call the var ENDPOINTS but only allow one.

Yah, in tests we only setup 1 endpoint so it still works 😛 I was lazy to do the transformation from env vars to list of endpoints.

ericchiang · 2017-10-27T15:38:52Z

Since adding a new storage is a big overhead to all future developers, if this is committed we need to be running these tests in travis.

Also just to address your commit message

kubernetes is not available yet, or if kubernetes depends on dex
to perform authentication and the operator would like to remove any
circular dependency if possible.

We intentionally build the OpenID Connect authenticator in Kubernetes to operate fine if dex isn't running. While the dependency looks circular, our experience of running dex on Tectonic would imply that that's a perfectly fine setup.

xiang90 · 2017-10-27T18:00:23Z

@dqminh

Can we handle timeout better? Or there is a future plan? Right now, we simply pass in context.TODO().

ericchiang · 2017-10-27T18:25:26Z

@xiang90 I think exposing a context is something we have to do on the dex end, unfortunately.

xiang90 · 2017-10-28T03:47:42Z

storage/etcd/etcd.go

+		return err
+	}
+	if !updateResp.Succeeded {
+		return fmt.Errorf("failed to update key=%q: updated not by us", key)


failed to update key = %q: concurrent conflicting update happened.

xiang90 · 2017-10-28T03:48:45Z

I only took a look at the txnUpdate func. Let me know if there is anything else I could help on.

dqminh · 2017-10-30T11:18:35Z

Can we handle timeout better? Or there is a future plan? Right now, we simply pass in context.TODO().

yah i think the storage interface needs to be changed so we can pass context downwards. Right now not sure what else can we do here:

using context.TODO()
enforce a hard-code timeout for all operations.

xiang90 · 2017-10-30T17:44:38Z

enforce a hard-code timeout for all operations.

this is what i usually do if the api layer does not expose context. it is safer. but i am not a dex expert, so i cannot speak for this exact case.

ericchiang · 2017-10-30T18:06:34Z

A hard-coded timeout sounds fine.

This patch adds etcd storage implementation. This should be useful in environments where - we dont want to depends on a separate, hard to maintain SQL cluster - we dont want to incur the overhead of talking to kubernetes apiservers - kubernetes is not available yet, or if kubernetes depends on dex to perform authentication and the operator would like to remove any circular dependency if possible.

This change vendors github.com/coreos/etcd related packages to support etcd storage implementation.

dqminh · 2017-10-31T14:45:02Z

added default timeout = 5s, and kv namespace support via Etcd.Namespace field.

ericchiang

One nit. Code lgtm

Needs docs and tests running in travis. I'd be happy with a follow up.

ericchiang · 2017-11-03T18:30:04Z

storage/etcd/config.go

+	CertFile   string `json:"cert_file" yaml:"cert_file"`
+}
+
+type Etcd struct {


Still need struct tags

Endpoints []string `yaml:"endpoints"`

dqminh · 2017-11-06T11:45:29Z

@ericchiang Thanks ! I will look into docs and travis change.

This patch uses docker to run an etcd container in travis CI so we can run storage/etcd conformance tests.

This adds references to etcd storage, including: - only supports etcd v3 - list of options and their meanings when connecting to etcd cluster

This explicitly adds struct tags for etcd storage instead of implicitly depends on yaml/json config serialization.

ericchiang

lgtm. Thanks for getting the conformance tests running against CI!

Add etcd backed storage

storage: fix list connector test

2b13bdd

The previous test doesnt actually testing ListConnectors code. For example the following pseudocode will pass the test: ``` ListConnectors() { return nil, nil } ``` Instead change to actually fetch and compare list of connectors, ordering by name

ericchiang reviewed Oct 27, 2017

View reviewed changes

xiang90 reviewed Oct 28, 2017

View reviewed changes

dqminh force-pushed the etcd-storage branch from 3bb8d10 to 8ce71d3 Compare October 30, 2017 11:36

dqminh added 2 commits October 31, 2017 14:43

vendor etcd dependencies for etcd storage

b410622

This change vendors github.com/coreos/etcd related packages to support etcd storage implementation.

dqminh force-pushed the etcd-storage branch from 8ce71d3 to b410622 Compare October 31, 2017 14:43

ericchiang reviewed Nov 3, 2017

View reviewed changes

dqminh added 3 commits November 6, 2017 14:23

add etcd to travis CI

fc1c60e

This patch uses docker to run an etcd container in travis CI so we can run storage/etcd conformance tests.

add documentation for etcd storage

a2188be

This adds references to etcd storage, including: - only supports etcd v3 - list of options and their meanings when connecting to etcd cluster

storage/etcd: document struct tag in code

e617197

This explicitly adds struct tags for etcd storage instead of implicitly depends on yaml/json config serialization.

ericchiang approved these changes Nov 6, 2017

View reviewed changes

ericchiang merged commit ccf85a7 into dexidp:master Nov 6, 2017

dqminh deleted the etcd-storage branch November 8, 2017 16:17

mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019

Merge pull request dexidp#1108 from dqminh/etcd-storage

fda4343

Add etcd backed storage

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add etcd backed storage #1108

Add etcd backed storage #1108

dqminh commented Oct 27, 2017

ericchiang left a comment

ericchiang Oct 27, 2017

gyuho Oct 27, 2017

gyuho Oct 27, 2017

ericchiang Oct 27, 2017

dqminh Oct 30, 2017

ericchiang Oct 30, 2017

ericchiang Nov 3, 2017

ericchiang Oct 27, 2017

dqminh Oct 30, 2017

ericchiang Oct 27, 2017

dqminh Oct 30, 2017

ericchiang Oct 27, 2017

dqminh Oct 31, 2017

ericchiang Oct 27, 2017

ericchiang Oct 27, 2017

xiang90 Oct 28, 2017

dqminh Oct 30, 2017

xiang90 Oct 30, 2017

ericchiang Oct 27, 2017

dqminh Oct 30, 2017

ericchiang commented Oct 27, 2017

xiang90 commented Oct 27, 2017

ericchiang commented Oct 27, 2017

xiang90 Oct 28, 2017

xiang90 commented Oct 28, 2017

dqminh commented Oct 30, 2017

xiang90 commented Oct 30, 2017

ericchiang commented Oct 30, 2017

dqminh commented Oct 31, 2017

ericchiang left a comment

ericchiang Nov 3, 2017

dqminh commented Nov 6, 2017

ericchiang left a comment

Add etcd backed storage #1108

Add etcd backed storage #1108

Conversation

dqminh commented Oct 27, 2017

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Oct 27, 2017

xiang90 commented Oct 27, 2017

ericchiang commented Oct 27, 2017

Choose a reason for hiding this comment

xiang90 commented Oct 28, 2017

dqminh commented Oct 30, 2017

xiang90 commented Oct 30, 2017

ericchiang commented Oct 30, 2017

dqminh commented Oct 31, 2017

ericchiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dqminh commented Nov 6, 2017

ericchiang left a comment

Choose a reason for hiding this comment