Allow file provider to load service config from files in a directory. #1672

rjshep · 2017-05-26T13:53:56Z

Currently the file provider reads service config either from the global config, or from a single file specified in global config.

In some deployments it would be nice to load from multiple files - perhaps because each service is defined in a single file and is maintained separately from the others. This PR extends the file provider to allow this.

Also added a unit test for the file provider and fixed bug in integration test which didn't check err response of GetRequest.

timoreimann

I optimistically went ahead and left a few comments. However, we still need to formally approve the PR's design and purpose. @emilevauge, WDYT?

timoreimann · 2017-05-26T22:33:27Z

integration/fixtures/file/dir/simple1.toml

@@ -0,0 +1,30 @@
+# rules


Can we trim down the configuration file to the bare minimum we need in order for the test to succeed?

Same for simple2.toml.

timoreimann · 2017-05-26T22:52:36Z

provider/file/file.go

+				return
+			case evt := <-watcher.Events:
+				event = &evt
+			case <-time.After(debouncePeriod):


The timer will be reset every time one of the other events comes in (because we will move to the next for-loop iteration and have line 74 create a new fresh time.After timer).

I think what you want to do instead is create a new ticker using time.NewTicker before the loop starts and reference it's C channel in the case statement.

(While doing so, you should also defer-execute Stop() on the created ticker to avoid resource leakage.)

Having the timer reset each time an event comes in is what I wanted to achieve. This lets things settle - for example if an external process is re-writing the files - before sending the updated configuration via the configuration channel. What I saw during testing was that multiple events were fired, even for a single edit, and Traefik was then trying to reload config more than needed. To be fair, the existing logic which checks if the provided config has changed kicked in and therefore nothing was actually done with it, but I felt it better just to put the delay in at the point where we know multiple events can occur in quick succession.

I see your point, it's a fair one. On the other hand, that external process could be so busy updating the file that the actual event processing case could be "starving", leaving the configuration out-of-date for possibly more than two seconds.

Admittedly, I don't know how realistic that scenario actually is. My other argument would be the deviation in behavior from the other provider watches.

I can see arguments for both directions, so I'll let @emilevauge chime in on this one.

@rjshep I'm not sure it's really needed knowing that indeed we have a similar global mechanism in server.go. I think we should simplify this kind of filtering and keep it one place only :)

timoreimann · 2017-05-26T22:55:31Z

provider/file/file.go

 	configuration := new(types.Configuration)
 	if _, err := toml.DecodeFile(filename, configuration); err != nil {
-		log.Error("Error reading file:", err)
-		return nil
+		return nil, fmt.Errorf("error reading file: %s", err)


Can we say "configuration file"?

timoreimann · 2017-05-26T22:58:38Z

provider/file/file.go

+		}
+
+		for k, v := range c.Backends {
+			configuration.Backends[k] = v


Should we check here whether a given backend key has already been configured, and if so, log a warning and skip the current backend?

Same question for frontends below.

timoreimann · 2017-05-26T23:01:21Z

provider/file/file_test.go

+)
+
+const (
+	frontends = `


Can we reduce the test definitions?

I think, you can reduce test definitions like that:

file_test.go

package file import ( "context" "fmt" "io/ioutil" "os" "path" "testing" "time" "github.com/containous/traefik/provider" "github.com/containous/traefik/safe" "github.com/containous/traefik/types" "github.com/stretchr/testify/assert" ) func TestProvideSingleFile(t *testing.T) { tempDir := createTempDir(t, "testfile") defer os.RemoveAll(tempDir) expectedNumFrontends := 2 expectedNumBackends := 2 tempFile := createFile(t, tempDir, "simple.toml", createFrontendConfiguration(expectedNumFrontends), createBackendConfiguration(expectedNumBackends)) configurationChan, signal := createConfigurationRoutine(t, &expectedNumFrontends, &expectedNumBackends) fileProvider := Provider{ BaseProvider: provider.BaseProvider{ Filename: tempFile.Name(), Watch: true, }, } fileProvider.Provide(configurationChan, safe.NewPool(context.Background()), nil) // Wait for initial message to be tested waitForSignal(t, signal, 2*time.Second, "initial config") // Now test again with single frontend and backend expectedNumFrontends = 1 expectedNumBackends = 1 tempFile = createFile(t, tempDir, "simple.toml", createFrontendConfiguration(expectedNumFrontends), createBackendConfiguration(expectedNumBackends)) waitForSignal(t, signal, 2*time.Second, "single frontend and backend") } func TestProvideDirectory(t *testing.T) { tempDir := createTempDir(t, "testdir") defer os.RemoveAll(tempDir) expectedNumFrontends := 2 expectedNumBackends := 2 tempFile1 := createRandomFile(t, tempDir, createFrontendConfiguration(expectedNumFrontends)) tempFile2 := createRandomFile(t, tempDir, createBackendConfiguration(expectedNumBackends)) configurationChan, signal := createConfigurationRoutine(t, &expectedNumFrontends, &expectedNumBackends) fileProvider := Provider{ BaseProvider: provider.BaseProvider{ Watch: true, }, Directory: tempDir, } fileProvider.Provide(configurationChan, safe.NewPool(context.Background()), nil) // Wait for initial config message to be tested waitForSignal(t, signal, 2*time.Second, "initial config") // Now remove the backends file expectedNumFrontends = 2 expectedNumBackends = 0 os.Remove(tempFile2.Name()) waitForSignal(t, signal, 2*time.Second, "remove the backends file") // Now remove the frontends file expectedNumFrontends = 0 expectedNumBackends = 0 os.Remove(tempFile1.Name()) waitForSignal(t, signal, 2*time.Second, "remove the frontends file") } func createConfigurationRoutine(t *testing.T, expectedNumFrontends *int, expectedNumBackends *int) (chan types.ConfigMessage, chan interface{}) { configurationChan := make(chan types.ConfigMessage) signal := make(chan interface{}) go func() { for { data := <-configurationChan assert.Equal(t, "file", data.ProviderName) assert.Len(t, data.Configuration.Frontends, *expectedNumFrontends) assert.Len(t, data.Configuration.Backends, *expectedNumBackends) signal <- nil } }() return configurationChan, signal } func waitForSignal(t *testing.T, signal chan interface{}, timeout time.Duration, caseName string) { timer := time.NewTimer(timeout) defer timer.Stop() select { case <-signal: case <-timer.C: t.Fatalf("Timed out waiting for assertions to be tested: %s", caseName) } } // createRandomFile Helper func createRandomFile(t *testing.T, tempDir string, contents ...string) *os.File { return createFile(t, tempDir, fmt.Sprintf("temp%d.toml", time.Now().UnixNano()), contents...) } // createFile Helper func createFile(t *testing.T, tempDir string, name string, contents ...string) *os.File { fileName := path.Join(tempDir, name) tempFile, err := os.Create(fileName) if err != nil { t.Fatal(err) } for _, content := range contents { _, err := tempFile.WriteString(content) if err != nil { t.Fatal(err) } } err = tempFile.Close() if err != nil { t.Fatal(err) } return tempFile } // createTempDir Helper func createTempDir(t *testing.T, dir string) string { d, err := ioutil.TempDir("", dir) if err != nil { t.Fatal(err) } return d } // createFrontendConfiguration Helper func createFrontendConfiguration(n int) string { conf := "[frontends]\n" for i := 1; i <= n; i++ { conf += fmt.Sprintf(` [frontends.frontend%[1]d] backend = "backend%[1]d" `, i) } return conf } // createBackendConfiguration Helper func createBackendConfiguration(n int) string { conf := "[backends]\n" for i := 1; i <= n; i++ { conf += fmt.Sprintf(` [backends.backend%[1]d] [backends.backend%[1]d.servers.server1] url = "http://172.17.0.%[1]d:80" `, i) } return conf }

After reflections, I think you can add 2 tests (when not watch) and simplify the provide section:

file_test.go

package file import ( "context" "fmt" "io/ioutil" "os" "path" "testing" "time" "github.com/containous/traefik/safe" "github.com/containous/traefik/types" "github.com/stretchr/testify/assert" ) func TestProvideSingleFileAndWatch(t *testing.T) { tempDir := createTempDir(t, "testfile") defer os.RemoveAll(tempDir) expectedNumFrontends := 2 expectedNumBackends := 2 tempFile := createFile(t, tempDir, "simple.toml", createFrontendConfiguration(expectedNumFrontends), createBackendConfiguration(expectedNumBackends)) configurationChan, signal := createConfigurationRoutine(t, &expectedNumFrontends, &expectedNumBackends) provide(configurationChan, watch, withFile(tempFile)) // Wait for initial message to be tested err := waitForSignal(signal, 2*time.Second, "initial config") assert.NoError(t, err) // Now test again with single frontend and backend expectedNumFrontends = 1 expectedNumBackends = 1 tempFile = createFile(t, tempDir, "simple.toml", createFrontendConfiguration(expectedNumFrontends), createBackendConfiguration(expectedNumBackends)) // Must fail because we don't watch the change err = waitForSignal(signal, 2*time.Second, "single frontend and backend") assert.NoError(t, err) } func TestProvideSingleFileAndNotWatch(t *testing.T) { tempDir := createTempDir(t, "testfile") defer os.RemoveAll(tempDir) expectedNumFrontends := 2 expectedNumBackends := 2 tempFile := createFile(t, tempDir, "simple.toml", createFrontendConfiguration(expectedNumFrontends), createBackendConfiguration(expectedNumBackends)) configurationChan, signal := createConfigurationRoutine(t, &expectedNumFrontends, &expectedNumBackends) provide(configurationChan, withFile(tempFile)) // Wait for initial message to be tested err := waitForSignal(signal, 2*time.Second, "initial config") assert.NoError(t, err) // Now test again with single frontend and backend expectedNumFrontends = 1 expectedNumBackends = 1 tempFile = createFile(t, tempDir, "simple.toml", createFrontendConfiguration(expectedNumFrontends), createBackendConfiguration(expectedNumBackends)) // Must fail because we don't watch the changes err = waitForSignal(signal, 2*time.Second, "single frontend and backend") assert.Error(t, err) } func TestProvideDirectoryAndWatch(t *testing.T) { tempDir := createTempDir(t, "testdir") defer os.RemoveAll(tempDir) expectedNumFrontends := 2 expectedNumBackends := 2 tempFile1 := createRandomFile(t, tempDir, createFrontendConfiguration(expectedNumFrontends)) tempFile2 := createRandomFile(t, tempDir, createBackendConfiguration(expectedNumBackends)) configurationChan, signal := createConfigurationRoutine(t, &expectedNumFrontends, &expectedNumBackends) provide(configurationChan, watch, withDirectory(tempDir)) // Wait for initial config message to be tested err := waitForSignal(signal, 2*time.Second, "initial config") assert.NoError(t, err) // Now remove the backends file expectedNumFrontends = 2 expectedNumBackends = 0 os.Remove(tempFile2.Name()) err = waitForSignal(signal, 2*time.Second, "remove the backends file") assert.NoError(t, err) // Now remove the frontends file expectedNumFrontends = 0 expectedNumBackends = 0 os.Remove(tempFile1.Name()) err = waitForSignal(signal, 2*time.Second, "remove the frontends file") assert.NoError(t, err) } func TestProvideDirectoryAndNotWatch(t *testing.T) { tempDir := createTempDir(t, "testdir") defer os.RemoveAll(tempDir) expectedNumFrontends := 2 expectedNumBackends := 2 createRandomFile(t, tempDir, createFrontendConfiguration(expectedNumFrontends)) tempFile2 := createRandomFile(t, tempDir, createBackendConfiguration(expectedNumBackends)) configurationChan, signal := createConfigurationRoutine(t, &expectedNumFrontends, &expectedNumBackends) provide(configurationChan, withDirectory(tempDir)) // Wait for initial config message to be tested err := waitForSignal(signal, 2*time.Second, "initial config") assert.NoError(t, err) // Now remove the backends file expectedNumFrontends = 2 expectedNumBackends = 0 os.Remove(tempFile2.Name()) // Must fail because we don't watch the changes err = waitForSignal(signal, 2*time.Second, "remove the backends file") assert.Error(t, err) } func createConfigurationRoutine(t *testing.T, expectedNumFrontends *int, expectedNumBackends *int) (chan types.ConfigMessage, chan interface{}) { configurationChan := make(chan types.ConfigMessage) signal := make(chan interface{}) go func() { for { data := <-configurationChan assert.Equal(t, "file", data.ProviderName) assert.Len(t, data.Configuration.Frontends, *expectedNumFrontends) assert.Len(t, data.Configuration.Backends, *expectedNumBackends) signal <- nil } }() return configurationChan, signal } func waitForSignal(signal chan interface{}, timeout time.Duration, caseName string) error { timer := time.NewTimer(timeout) defer timer.Stop() select { case <-signal: case <-timer.C: return fmt.Errorf("Timed out waiting for assertions to be tested: %s", caseName) } return nil } func provide(configurationChan chan types.ConfigMessage, builders ...func(p *Provider)) { pvd := &Provider{} for _, builder := range builders { builder(pvd) } pvd.Provide(configurationChan, safe.NewPool(context.Background()), nil) } func watch(pvd *Provider) { pvd.Watch = true } func withDirectory(name string) func(*Provider) { return func(pvd *Provider) { pvd.Directory = name } } func withFile(tempFile *os.File) func(*Provider) { return func(p *Provider) { p.Filename = tempFile.Name() } } // createRandomFile Helper func createRandomFile(t *testing.T, tempDir string, contents ...string) *os.File { return createFile(t, tempDir, fmt.Sprintf("temp%d.toml", time.Now().UnixNano()), contents...) } // createFile Helper func createFile(t *testing.T, tempDir string, name string, contents ...string) *os.File { fileName := path.Join(tempDir, name) tempFile, err := os.Create(fileName) if err != nil { t.Fatal(err) } for _, content := range contents { _, err := tempFile.WriteString(content) if err != nil { t.Fatal(err) } } err = tempFile.Close() if err != nil { t.Fatal(err) } return tempFile } // createTempDir Helper func createTempDir(t *testing.T, dir string) string { d, err := ioutil.TempDir("", dir) if err != nil { t.Fatal(err) } return d } // createFrontendConfiguration Helper func createFrontendConfiguration(n int) string { conf := "[frontends]\n" for i := 1; i <= n; i++ { conf += fmt.Sprintf(` [frontends.frontend%[1]d] backend = "backend%[1]d" `, i) } return conf } // createBackendConfiguration Helper func createBackendConfiguration(n int) string { conf := "[backends]\n" for i := 1; i <= n; i++ { conf += fmt.Sprintf(` [backends.backend%[1]d] [backends.backend%[1]d.servers.server1] url = "http://172.17.0.%[1]d:80" `, i) } return conf }

timoreimann · 2017-05-26T23:12:50Z

provider/file/file_test.go

+	provider.Provide(c, safe.NewPool(context.Background()), nil)
+
+	// Wait for initial message to be tested
+	wg.Wait()


We should define an upper bound after which we stop waiting for the asserting goroutine to complete. Otherwise, our test could lock up if we make a mistake or otherwise fail to send an event down the ConfigMessage channel.

The usual way to achieve this is to define a time.Timer of, say, 500 ms or so, and have the test t.Fail if it fires. If you also replace the wait group by a done channel that's being closed by the asserting goroutine as soon as it finishes, you can have a simple select statements over the timeout and done channels.

Similar for other tests below.

timoreimann · 2017-05-26T23:15:12Z

provider/file/file_test.go

+func createFile(t *testing.T, file string) *os.File {
+	f, err := os.Create(file)
+	if err != nil {
+		t.Error(err)


Should likely be a t.Fatal.

timoreimann · 2017-05-26T23:15:17Z

provider/file/file_test.go

+func createTempDir(t *testing.T, dir string) string {
+	d, err := ioutil.TempDir("", dir)
+	if err != nil {
+		t.Error(err)


Should likely be a t.Fatal.

timoreimann · 2017-05-26T23:17:02Z

provider/file/file_test.go

+	tempFile.WriteString(frontends + backends)
+	tempFile.Close()
+
+	provider := new(Provider)


How about using the &Provider{...} notation and inline the field settings from lines 107-108 directly?

timoreimann · 2017-05-26T23:19:19Z

provider/file/file.go

+	if p.Directory != "" {
+		watchDir = p.Directory
+		configuration, err = loadFileConfigFromDirectory(p.Directory)
+	} else {


If we have both a single filename and a directory defined, the directory wins. I wonder if that's desirable or if we should at least log a warning, if not let Traefik fail to bootstrap during a configuration sanity check. WDYT?

And whatever we decide, we should document how Traefik deals with this situation.

Ideally, yes, we'd notify if both a single file and directory are specified. However, file is always specified, as it's then name of the main Traefik config file, even if the filename isn't set in the [file] config section. I didn't see a straightforward way to address this.

True, that's a tricky. I'd just amend the documentation then by noting that the directory setting takes precedence.

rjshep · 2017-05-30T13:46:42Z

Thanks, as always, for the comments @timoreimann. I've made the changes requested and look forward to @emilevauge 's views too.

ldez · 2017-06-21T14:05:00Z

provider/file/file_test.go

+		for {
+			data := <-c
+			assert.Equal(t, "file", data.ProviderName)
+			assert.Equal(t, numFrontends, len(data.Configuration.Frontends))


assert.Len(t, data.Configuration.Frontends, numFrontends) assert.Len(t, data.Configuration.Backends, numBackends)

ldez · 2017-06-21T14:06:55Z

provider/file/file.go

+	var fileList []os.FileInfo
+	var err error
+
+	if fileList, err = ioutil.ReadDir(directory); err != nil {


you can simplify:

fileList, err := ioutil.ReadDir(directory) if err != nil { // ...

ldez · 2017-06-21T14:07:57Z

provider/file/file.go

 	}
-	return configuration
+
+	configuration := &types.Configuration{Frontends: make(map[string]*types.Frontend),


you can change the format to:

configuration := &types.Configuration{ Frontends: make(map[string]*types.Frontend), Backends: make(map[string]*types.Backend), }

ldez · 2017-06-21T14:09:52Z

provider/file/file.go

+		}
+
+		var c *types.Configuration
+		if c, err = loadFileConfig(path.Join(directory, file.Name())); err != nil {


this can be simplify:

c, err := loadFileConfig(path.Join(directory, file.Name())) if err != nil { // ...

ldez · 2017-06-21T14:11:12Z

provider/file/file.go

+			} else {
+				configuration.Frontends[k] = v
+			}
+


could you remove blank line ?

ldez · 2017-06-21T14:12:44Z

provider/file/file.go

+			return nil, err
+		}
+
+		for k, v := range c.Backends {


maybe replace k, v by backendName, backend

ldez · 2017-06-21T14:13:10Z

provider/file/file.go

+			}
+		}
+
+		for k, v := range c.Frontends {


maybe replace k, v by frontendName, frontend

ldez · 2017-06-21T14:13:45Z

provider/file/file_test.go

+			data := <-c
+			assert.Equal(t, "file", data.ProviderName)
+			assert.Equal(t, numFrontends, len(data.Configuration.Frontends))
+			assert.Equal(t, numBackends, len(data.Configuration.Backends))


assert.Len(t, data.Configuration.Frontends, numFrontends) assert.Len(t, data.Configuration.Backends, numBackends)

ldez · 2017-06-22T14:49:40Z

provider/file/file.go

+	var fileList []os.FileInfo
+	var err error
+
+	fileList, err = ioutil.ReadDir(directory)


you could remove:

var fileList []os.FileInfo var err error

and change:

fileList, err = ioutil.ReadDir(directory) // by fileList, err := ioutil.ReadDir(directory)

ldez · 2017-06-22T15:26:22Z

provider/file/file.go

+	} else {
+		watchDir = filepath.Dir(p.Filename)
+		configuration, err = loadFileConfig(p.Filename)
+	}


I think you can replace this duplicate block (here and in the watcherCallback) by this method:

func (p *Provider) loadConfig() (*types.Configuration, error) { if p.Directory != "" { return loadFileConfigFromDirectory(p.Directory) } else { return loadFileConfig(p.Filename) } }

ldez

I think this is my last review 😸

ldez · 2017-06-24T11:02:38Z

provider/file/file.go

-	file, err := os.Open(p.Filename)
-	if err != nil {
-		log.Error("Error opening file", err)
+	if err := p.addWatcher(pool, watchDir, configurationChan, p.watcherCallback); err != nil {


you need to use p.Watch condition:

if p.Watch { var watchDir string if p.Directory != "" { watchDir = p.Directory } else { watchDir = filepath.Dir(p.Filename) } if err := p.addWatcher(pool, watchDir, configurationChan, p.watcherCallback); err != nil { return err } }

ldez · 2017-06-24T11:24:08Z

provider/file/file_test.go

+)
+
+const (
+	frontends = `


I think, you can reduce test definitions like that:

file_test.go

package file import ( "context" "fmt" "io/ioutil" "os" "path" "testing" "time" "github.com/containous/traefik/provider" "github.com/containous/traefik/safe" "github.com/containous/traefik/types" "github.com/stretchr/testify/assert" ) func TestProvideSingleFile(t *testing.T) { tempDir := createTempDir(t, "testfile") defer os.RemoveAll(tempDir) expectedNumFrontends := 2 expectedNumBackends := 2 tempFile := createFile(t, tempDir, "simple.toml", createFrontendConfiguration(expectedNumFrontends), createBackendConfiguration(expectedNumBackends)) configurationChan, signal := createConfigurationRoutine(t, &expectedNumFrontends, &expectedNumBackends) fileProvider := Provider{ BaseProvider: provider.BaseProvider{ Filename: tempFile.Name(), Watch: true, }, } fileProvider.Provide(configurationChan, safe.NewPool(context.Background()), nil) // Wait for initial message to be tested waitForSignal(t, signal, 2*time.Second, "initial config") // Now test again with single frontend and backend expectedNumFrontends = 1 expectedNumBackends = 1 tempFile = createFile(t, tempDir, "simple.toml", createFrontendConfiguration(expectedNumFrontends), createBackendConfiguration(expectedNumBackends)) waitForSignal(t, signal, 2*time.Second, "single frontend and backend") } func TestProvideDirectory(t *testing.T) { tempDir := createTempDir(t, "testdir") defer os.RemoveAll(tempDir) expectedNumFrontends := 2 expectedNumBackends := 2 tempFile1 := createRandomFile(t, tempDir, createFrontendConfiguration(expectedNumFrontends)) tempFile2 := createRandomFile(t, tempDir, createBackendConfiguration(expectedNumBackends)) configurationChan, signal := createConfigurationRoutine(t, &expectedNumFrontends, &expectedNumBackends) fileProvider := Provider{ BaseProvider: provider.BaseProvider{ Watch: true, }, Directory: tempDir, } fileProvider.Provide(configurationChan, safe.NewPool(context.Background()), nil) // Wait for initial config message to be tested waitForSignal(t, signal, 2*time.Second, "initial config") // Now remove the backends file expectedNumFrontends = 2 expectedNumBackends = 0 os.Remove(tempFile2.Name()) waitForSignal(t, signal, 2*time.Second, "remove the backends file") // Now remove the frontends file expectedNumFrontends = 0 expectedNumBackends = 0 os.Remove(tempFile1.Name()) waitForSignal(t, signal, 2*time.Second, "remove the frontends file") } func createConfigurationRoutine(t *testing.T, expectedNumFrontends *int, expectedNumBackends *int) (chan types.ConfigMessage, chan interface{}) { configurationChan := make(chan types.ConfigMessage) signal := make(chan interface{}) go func() { for { data := <-configurationChan assert.Equal(t, "file", data.ProviderName) assert.Len(t, data.Configuration.Frontends, *expectedNumFrontends) assert.Len(t, data.Configuration.Backends, *expectedNumBackends) signal <- nil } }() return configurationChan, signal } func waitForSignal(t *testing.T, signal chan interface{}, timeout time.Duration, caseName string) { timer := time.NewTimer(timeout) defer timer.Stop() select { case <-signal: case <-timer.C: t.Fatalf("Timed out waiting for assertions to be tested: %s", caseName) } } // createRandomFile Helper func createRandomFile(t *testing.T, tempDir string, contents ...string) *os.File { return createFile(t, tempDir, fmt.Sprintf("temp%d.toml", time.Now().UnixNano()), contents...) } // createFile Helper func createFile(t *testing.T, tempDir string, name string, contents ...string) *os.File { fileName := path.Join(tempDir, name) tempFile, err := os.Create(fileName) if err != nil { t.Fatal(err) } for _, content := range contents { _, err := tempFile.WriteString(content) if err != nil { t.Fatal(err) } } err = tempFile.Close() if err != nil { t.Fatal(err) } return tempFile } // createTempDir Helper func createTempDir(t *testing.T, dir string) string { d, err := ioutil.TempDir("", dir) if err != nil { t.Fatal(err) } return d } // createFrontendConfiguration Helper func createFrontendConfiguration(n int) string { conf := "[frontends]\n" for i := 1; i <= n; i++ { conf += fmt.Sprintf(` [frontends.frontend%[1]d] backend = "backend%[1]d" `, i) } return conf } // createBackendConfiguration Helper func createBackendConfiguration(n int) string { conf := "[backends]\n" for i := 1; i <= n; i++ { conf += fmt.Sprintf(` [backends.backend%[1]d] [backends.backend%[1]d.servers.server1] url = "http://172.17.0.%[1]d:80" `, i) } return conf }

ldez

GJ ! 👍

LGTM

nmengin · 2017-06-27T10:08:37Z

provider/file/file.go

+		if p.Directory != "" {
+			watchDir = p.Directory
+		} else {
+			watchDir = filepath.Dir(p.Filename)


By adding watcher to the directory even if you just want to wach a file, you are going to watch all the directory events.
For example, if it exists 10 toml files into your directory, you are going to reload the configuration when all of these files will be modified.

That's why I suggest you to just add a watcher for the configuration file : watchDir = p.Filename and, maybe rename watchDir into watchFile (dir is a file ;) ) or watchItem...

If you just want to watch a file, then you'd just specify the Filename and not use Directory at all. So that Watch applies only to the active setting Filename or Directory - they are mutually exclusive.

@rjshep Of course, but when user specifies FileName, the watcher is added on its parent directory (filepath.Dir(p.Filename)). It was the mechanism before your PR.

However, before the modification the callback checked if the events received by the watcher were sent by the configuration file as described below :

case event := <-watcher.Events: if strings.Contains(event.Name, file.Name())

It would have been better to add directly the watcher on the file but, thanks to this code, the configuration was reloaded only when the Filename was modified.
But now, the configuration will be reloaded if one of all the toml file contained in the same directory than the Filename.
There will have no problem because only the Filename configuration will be reloaded but, it should be better to take advantage of you PR to improve the Filename watching by replacing watchDir = filepath.Dir(p.Filename) by watchDir = p.Filename.

Then, the configuration will only be reloaded when the Filename will change.

Sorry @nmengin, completely missed your original point. Yes that is a sensible change to make.

nmengin · 2017-06-27T12:57:16Z

integration/fixtures/file/directory.toml

+  [entryPoints.http]
+  address = ":8000"
+
+logLevel = "DEBUG"


The logLevel should to be defined before the entrypoints otherwise the logger will not work correctly.

ldez · 2017-06-27T14:02:49Z

@rjshep could you squash and rebase (for semaphoreCI)?

nmengin

@rjshep Thanks for the great PR which allow new behavior and improves the current!

LGTM

nmengin

As I said previously, LGTM

ldez added kind/enhancement a new or improved feature. status/1-needs-design-review labels May 26, 2017

timoreimann suggested changes May 26, 2017

View reviewed changes

ldez added status/2-needs-review and removed status/1-needs-design-review labels May 30, 2017

ldez self-requested a review May 30, 2017 22:49

yannh mentioned this pull request Jun 13, 2017

File backend should support multiple files / configuration directory(ies) #1739

Closed

ldez added this to the 1.4 milestone Jun 13, 2017

ldez suggested changes Jun 21, 2017

View reviewed changes

ldez suggested changes Jun 22, 2017

View reviewed changes

ldez added contributor/waiting-for-corrections and removed contributor/waiting-for-corrections labels Jun 22, 2017

ldez suggested changes Jun 24, 2017

View reviewed changes

ldez approved these changes Jun 26, 2017

View reviewed changes

nmengin reviewed Jun 27, 2017

View reviewed changes

nmengin approved these changes Jun 27, 2017

View reviewed changes

rjshep force-pushed the directory-config branch from 8a7527d to c3e540c Compare June 27, 2017 14:09

timoreimann approved these changes Jun 27, 2017

View reviewed changes

ldez force-pushed the directory-config branch from c3e540c to 0e0b86a Compare June 27, 2017 14:21

Allow file provider to load config from files in a directory.

430cdbd

rjshep force-pushed the directory-config branch from 0e0b86a to 430cdbd Compare June 27, 2017 14:24

ldez merged commit 4128c1a into traefik:master Jun 27, 2017

ldez removed the status/2-needs-review label Jun 27, 2017

ldez mentioned this pull request Oct 1, 2017

multiple toml file not recognised for [file] backend #2194

Closed

ldez added the area/provider/file label Oct 1, 2017

altovedo deleted the directory-config branch September 12, 2018 08:59

Allow file provider to load service config from files in a directory. #1672

Allow file provider to load service config from files in a directory. #1672

Conversation

rjshep commented May 26, 2017 • edited by ldez Loading

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

ldez Jun 24, 2017 • edited Loading

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 May 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjshep commented May 30, 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

ldez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldez Jun 24, 2017 • edited Loading

Choose a reason for hiding this comment

ldez 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

ldez commented Jun 27, 2017 • edited Loading

nmengin left a comment • edited Loading

Choose a reason for hiding this comment

nmengin left a comment

Choose a reason for hiding this comment

rjshep commented May 26, 2017 •

edited by ldez

Loading

ldez Jun 24, 2017 •

edited

Loading

timoreimann May 26, 2017 •

edited

Loading

ldez Jun 24, 2017 •

edited

Loading

ldez commented Jun 27, 2017 •

edited

Loading

nmengin left a comment •

edited

Loading