add dynamodb backend #1158

tskinn · 2017-02-13T20:22:37Z

This PR is by probably not ready to merge but I would like some guidance on how to run tests and whatever else I need to add in order to get this PR ready for prime time.

What do you do when other tests are failing and the test never reaches your tests? I ended up deleting the other test files so I could verify my test was going through.

SantoDE · 2017-02-20T20:12:33Z

Hey @tskinn ,

best bet is to meet up on slack to get you up to lightspeed I guess :)

timoreimann

Two general remarks:

A number of log messages are missing a reference to DynamoDB (e.g., "Failed to scan table"). Can we add it accordingly ("Failed to scan DynamoDB table") so that users can easily spot in the logs that it's a part of DynamoDB which is failing?
This is more of a question to the other @containous/traefik maintainers: Since this is a new provider, should we also have an integration test?

timoreimann · 2017-03-01T20:35:06Z

configuration.go

+	var defaultDynamoDB provider.DynamoDB
+	defaultDynamoDB.Constraints = types.Constraints{}
+	defaultDynamoDB.RefreshSeconds = 15
+	defaultDynamoDB.Region = "us-east-1"


Is it reasonable to set a default region, or should we rather require the user to define one?

And if we want a default, is us-east-1 a good one? It has made quite some news during yesterday's S3 outage. :)

That's a good question. I think if we do have a default, us-east-1 is most reasonable besides the recent outage haha. In all my experiences with aws, us-east-1 is most deserving of 'default' status. But I would be happy to drop it if we decide a default region shouldn't be included.

I think it would be safer to avoid setting a default region. It not set, an error log should be printed. WDYT?

I'm in favor of making it mandatory. Users should make a conscious decision that region to pick IMHO.

@emilevauge any thoughts on this one?

timoreimann · 2017-03-01T20:37:58Z

docs/toml.md

+- 'name' : string
+    - The name is used as the name of the frontend or backend.
+- 'frontend' or 'backend' : map
+    - This attribute's structure matches exactly the structure of a Frontend or Backend type in traefik. See types/types.go for details. The presence or absence of this attribute determines it's type. So an item should never have both a 'frontend' and a 'backend' attribute. 


Typo: it's type -> its type.

timoreimann · 2017-03-01T21:47:19Z

provider/dynamodb.go

+				log.Debugf("Frontend unmarshalled successfully")
+			}
+		} else {
+			log.Warnf("Something is up! Following item doesn't follow compatible format:\n%v", item)


I'd suggest to drop the \n: Newlines in error messages often make it hard to do things like grepping for errors easily.

timoreimann · 2017-03-01T21:51:02Z

provider/dynamodb_test.go

+	}
+	loadedConfig, err := provider.loadDynamoConfig(dbiface)
+	if err != nil {
+		t.Fatalf("Expected noerror, got %s", err.Error())


err should be enough.

timoreimann · 2017-03-01T21:52:50Z

provider/dynamodb_test.go

+	}
+	provider := DynamoDB{}
+	client := provider.createClient()
+	if client == nil {


I don't think we need this check since we always return a non-nil value from createClient.

You are totally right.

timoreimann · 2017-03-01T21:53:30Z

provider/dynamodb_test.go

+		},
+	}
+	if !reflect.DeepEqual(loadedConfig, expectedConfig) {
+		t.Fatal("Configurations did not match")


Can we include the two mismatching configurations in the error message?

timoreimann · 2017-03-01T21:59:49Z

provider/dynamodb.go

+func (provider *DynamoDB) loadDynamoConfig(client *dynamoClient) (*types.Configuration, error) {
+	items, err := provider.scanTable(client)
+	if err != nil {
+		return nil, err


This error path isn't test-covered. Any chance we can do this?

I added a flag in the mock ScanPages to return an error and trigger this code.
https://github.com/containous/traefik/pull/1158/files#diff-57d4449dc0fc79752ce983c308204295R45

timoreimann · 2017-03-01T22:09:15Z

provider/dynamodb.go

+	"github.com/containous/traefik/log"
+	"github.com/containous/traefik/safe"
+	"github.com/containous/traefik/types"
+	"time"


The import should go below context further above, separating the following github.com imports by a blank line.

timoreimann

Another small design change request. :-)

Would you mind creating separate commits during the review instead of force-pushing over the existing one(s)? It makes for a more pleasant reviewing experience, thanks. :-)

timoreimann · 2017-03-02T18:00:17Z

provider/dynamodb_test.go

+	},
+}
+
+var testWithError = false


A problem with the approach of using a global variable is that it can make efforts to parallelize tests very difficult. It also complicates the current approach because TestLoadDynamoConfig now combines both states (testWithError equals true and false, respectively) into a single test, which makes it impossible to run the tests individually or see the outcome of the second before the first test.

Since we already have a testing struct, could you please move the variable into mockDynamoDBCLient and then set the value accordingly in two separate tests (e.g., TestLoadDynamoConfigSuccessful and TestLoadDynamoConfigFailure)?

timoreimann

Perfect, thanks. :-)

LGTM!

emilevauge · 2017-03-06T18:01:10Z

Waiting for integration tests ;) https://traefik.slack.com/archives/dev/p1488491697001137

emilevauge · 2017-03-08T16:46:38Z

@tskinn could you rebase your PR? Other than that looks good to me, thanks a lot for adding an integration test :)

timoreimann · 2017-03-07T23:20:08Z

integration/dynamodb_test.go

+	ID       string         `dynamodbav:"id"`
+	Name     string         `dynamodbav:"name"`
+	Frontend types.Frontend `dynamodbav:"frontend"`
+}


DynamoDBBackendItem and DynamoDBFrontendItem are equal except for the Backend / Frontend parts. You could move the common fields into a base struct and embed it.

Yeah I thought about that but I'm not sure how that works. In dynamo the attributes need to be named frontend or backend. If I leave the top level struct tag empty does it inherit the tag or something like that?

My understanding is that if you embed the struct, it should inherit that tags as well; functionally, it shouldn't be different from copying the struct.

So this is what I have in mind:

type DynamoDBItem struct { ID string `dynamodbav:"id"` Name string `dynamodbav:"name"` } type DynamoDBBackendItem struct { DynamoDBItem Backend types.Backend `dynamodbav:"backend"` } type DynamoDBFrontendItem struct { DynamoDBItem Frontend types.Frontend `dynamodbav:"frontend"` }

I'll leave it up to you if you want to give the embedded approach a try or stick to what we have right now. It's not critical for me.

timoreimann · 2017-03-07T23:48:40Z

integration/dynamodb_test.go

+func (s *DynamoDBSuite) SetUpSuite(c *check.C) {
+	s.createComposeProject(c, "dynamodb")
+	s.composeProject.Start(c)
+	time.Sleep(10 * time.Second)


Our integration test suites are already fairly flaky. I haven't looked too deep into the actual reasons yet, but believe that things like waiting a fixed amount of time and relying on systems to be up and running then is part of the problem.

Is there a way for us to tell when the project has started? Maybe by pinging a particular endpoint? We already have utils.Try and utils.TryRequest to do this in a looping fashion.

Yeah we don't need that. Good catch

timoreimann · 2017-03-07T23:49:36Z

integration/dynamodb_test.go

+		Endpoint:    aws.String(dynamoURL),
+	}
+	var sess *session.Session
+	err := utils.Try(300*time.Second, func() error {


Is it necessary to wait 5 minutes max? I thought this was a local dynamoDB instance?

Probably not. I will adjust

timoreimann · 2017-03-07T23:50:03Z

integration/dynamodb_test.go

+	time.Sleep(10 * time.Second)
+	dynamoURL := "http://" + s.composeProject.Container(c, "dynamo").NetworkSettings.IPAddress + ":8000"
+	config := &aws.Config{
+		Region:      aws.String("us-east-1"),


Are we actually using a real AWS region, or is this just to please the aws.Config?

Nope. We just have to please the config with some values.

timoreimann · 2017-03-07T23:51:53Z

integration/dynamodb_test.go

+		TableName: aws.String("traefik"),
+	}
+	_, err = svc.CreateTable(params)
+	c.Assert(err, checker.IsNil)


Should we return if the Assert fails to avoid finishing a possibly costly test whose output we won't need anymore?

timoreimann · 2017-03-08T17:13:33Z

configuration.go

+	var defaultDynamoDB provider.DynamoDB
+	defaultDynamoDB.Constraints = types.Constraints{}
+	defaultDynamoDB.RefreshSeconds = 15
+	defaultDynamoDB.Region = "us-east-1"


I'm in favor of making it mandatory. Users should make a conscious decision that region to pick IMHO.

@emilevauge any thoughts on this one?

timoreimann

@tskinn let me know if you want to do the embed thing or stick to what we have right now. :)

Either way, LGTM. :)

Excellent job, thanks for the contribution!

timoreimann · 2017-03-08T21:18:53Z

@tskinn there are some conflicts left over that you need to resolve once you rebase from master.

tskinn · 2017-03-09T01:57:01Z

@timoreimann thanks for helping get this ready. I went ahead and did the embed stuff you suggested.

timoreimann · 2017-03-09T04:05:25Z

Perfect!

Done from my side. @emilevauge, WDYT?

emilevauge

Could you rebase your PR and complete README file?
Other than that, LGTM :)

emilevauge · 2017-03-02T12:39:50Z

configuration.go

+	var defaultDynamoDB provider.DynamoDB
+	defaultDynamoDB.Constraints = types.Constraints{}
+	defaultDynamoDB.RefreshSeconds = 15
+	defaultDynamoDB.Region = "us-east-1"


I think it would be safer to avoid setting a default region. It not set, an error log should be printed. WDYT?

emilevauge · 2017-03-09T10:48:09Z

docs/index.md

@@ -11,7 +11,7 @@


 Træfɪk is a modern HTTP reverse proxy and load balancer made to deploy microservices with ease.
-It supports several backends ([Docker](https://www.docker.com/), [Swarm](https://docs.docker.com/swarm), [Mesos/Marathon](https://mesosphere.github.io/marathon/), [Consul](https://www.consul.io/), [Etcd](https://coreos.com/etcd/), [Zookeeper](https://zookeeper.apache.org), [BoltDB](https://github.com/boltdb/bolt), [Amazon ECS](https://aws.amazon.com/ecs/), Rest API, file...) to manage its configuration automatically and dynamically.
+It supports several backends ([Docker](https://www.docker.com/), [Swarm](https://docs.docker.com/swarm), [Mesos/Marathon](https://mesosphere.github.io/marathon/), [Consul](https://www.consul.io/), [Etcd](https://coreos.com/etcd/), [Zookeeper](https://zookeeper.apache.org), [BoltDB](https://github.com/boltdb/bolt), [Amazon ECS](https://aws.amazon.com/ecs/), [Amazon DynamoDB](https://aws.amazon.com/dynamodb/), Rest API, file...) to manage its configuration automatically and dynamically.


Could you also add it in the README file ?

Ping @tskinn ^

Signed-off-by: Taylor Skinner <[email protected]> add some comments Signed-off-by: Taylor Skinner <[email protected]> update readmes make test runnable Signed-off-by: Taylor Skinner <[email protected]> make test squash! add dynamo add glide.lock format imports gofmt update glide.lock fixes for review golint clean up and reorganize tests add dynamodb integration test remove default region. clean up tests. consistent docs forgot the region is required DRY make validate update readme and commit dependencies

tskinn · 2017-03-16T16:14:36Z

@emilevauge rebased and ready to go

emilevauge

Awesome work @tskinn
Thanks a lot!
LGTM

tskinn force-pushed the add-dynamodb-provider branch 2 times, most recently from 91b53d9 to baac6e2 Compare March 1, 2017 21:48

timoreimann suggested changes Mar 1, 2017

View reviewed changes

timoreimann reviewed Mar 1, 2017

View reviewed changes

tskinn force-pushed the add-dynamodb-provider branch 3 times, most recently from fd2a4fb to ac2e2a0 Compare March 1, 2017 23:59

emilevauge added status/1-needs-design-review status/2-needs-review status/3-docs-review and removed status/3-docs-review labels Mar 2, 2017

emilevauge added this to the 1.3 milestone Mar 2, 2017

timoreimann suggested changes Mar 2, 2017

View reviewed changes

timoreimann approved these changes Mar 2, 2017

View reviewed changes

timoreimann suggested changes Mar 8, 2017

View reviewed changes

timoreimann approved these changes Mar 8, 2017

View reviewed changes

tskinn force-pushed the add-dynamodb-provider branch from 74e9c16 to 77992e7 Compare March 9, 2017 01:54

tskinn force-pushed the add-dynamodb-provider branch from 77992e7 to 93cb7a3 Compare March 9, 2017 02:09

emilevauge approved these changes Mar 16, 2017

View reviewed changes

tskinn force-pushed the add-dynamodb-provider branch from 1bfcbc8 to b69c7e4 Compare March 16, 2017 16:11

tskinn force-pushed the add-dynamodb-provider branch from b69c7e4 to 72e35af Compare March 16, 2017 16:12

emilevauge approved these changes Mar 17, 2017

View reviewed changes

emilevauge added status/3-needs-merge and removed status/1-needs-design-review status/2-needs-review labels Mar 17, 2017

emilevauge merged commit 677899d into traefik:master Mar 17, 2017

ldez added the kind/enhancement a new or improved feature. label Apr 29, 2017

ldez added area/provider/dynamodb and removed status/3-needs-merge labels May 19, 2017

add dynamodb backend #1158

add dynamodb backend #1158

Conversation

tskinn commented Feb 13, 2017

SantoDE commented Feb 20, 2017

timoreimann 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

timoreimann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timoreimann left a comment

Choose a reason for hiding this comment

emilevauge commented Mar 6, 2017

emilevauge commented Mar 8, 2017

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

timoreimann left a comment • edited Loading

Choose a reason for hiding this comment

timoreimann commented Mar 8, 2017

tskinn commented Mar 9, 2017

timoreimann commented Mar 9, 2017

emilevauge 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

tskinn commented Mar 16, 2017

emilevauge left a comment

Choose a reason for hiding this comment

timoreimann left a comment •

edited

Loading