Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add dynamodb backend #1158

Merged
merged 1 commit into from Mar 17, 2017
Merged

Conversation

tskinn
Copy link
Contributor

@tskinn tskinn commented Feb 13, 2017

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
Copy link
Collaborator

SantoDE commented Feb 20, 2017

Hey @tskinn ,

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

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

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two general remarks:

  1. 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?
  2. 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?

configuration.go Outdated
var defaultDynamoDB provider.DynamoDB
defaultDynamoDB.Constraints = types.Constraints{}
defaultDynamoDB.RefreshSeconds = 15
defaultDynamoDB.Region = "us-east-1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

docs/toml.md Outdated
- '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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo: it's type -> its type.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err should be enough.

}
provider := DynamoDB{}
client := provider.createClient()
if client == nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are totally right.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"github.com/containous/traefik/log"
"github.com/containous/traefik/safe"
"github.com/containous/traefik/types"
"time"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :-)

},
}

var testWithError = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, thanks. :-)

LGTM!

@emilevauge
Copy link
Member

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

@emilevauge
Copy link
Member

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

ID string `dynamodbav:"id"`
Name string `dynamodbav:"name"`
Frontend types.Frontend `dynamodbav:"frontend"`
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah we don't need that. Good catch

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not. I will adjust

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed.

configuration.go Outdated
var defaultDynamoDB provider.DynamoDB
defaultDynamoDB.Constraints = types.Constraints{}
defaultDynamoDB.RefreshSeconds = 15
defaultDynamoDB.Region = "us-east-1"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

@timoreimann timoreimann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor

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

@tskinn
Copy link
Contributor Author

tskinn commented Mar 9, 2017

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

@timoreimann
Copy link
Contributor

Perfect!

Done from my side. @emilevauge, WDYT?

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

configuration.go Outdated
var defaultDynamoDB provider.DynamoDB
defaultDynamoDB.Constraints = types.Constraints{}
defaultDynamoDB.RefreshSeconds = 15
defaultDynamoDB.Region = "us-east-1"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also add it in the README file ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @tskinn ^

Signed-off-by: Taylor Skinner <tskinn12@gmail.com>

add some comments

Signed-off-by: Taylor Skinner <tskinn12@gmail.com>

update readmes

make test runnable

Signed-off-by: Taylor Skinner <tskinn12@gmail.com>

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
Copy link
Contributor Author

tskinn commented Mar 16, 2017

@emilevauge rebased and ready to go

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work @tskinn
Thanks a lot!
LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants