Skip to content

Commit

Permalink
feat: replace offline-mode filewatcher with polling (#317)
Browse files Browse the repository at this point in the history
We've encountered robustness issues with the current offline-mode
file-watcher implementation in Docker images that use shared volumes.
Additionally, we're seeing intermittent CI failures in the file-watcher
tests.

Overall, I believe the complexity introduced by file-watcher isn't worth
the payoff. It is supposed to unify all the different operating system
methods of notifying that files have changed, but it falls short.

The vast majority of the latency when using offline mode would be
downloading the actual archive from LaunchDarkly and the interval
between those downloads - which might be minutes/hours/days.

This commit replaces the file-watcher with a simple polling loop. Every
interval, it calls `stat()` and determines if the offline archive needs
to be reloaded.

The default interval is 1 second, and the minimum configurable is 100ms.
The minimum was chosen to protect the system in case of accidental
configuration of an extremely short interval (like 0).

In practice, most users may raise the interval - for example, if they're
fetching the archive every hour, there is no need to use a 1 second
interval.
  • Loading branch information
cwaldren-ld committed May 14, 2024
1 parent ccff11d commit 7bea824
Show file tree
Hide file tree
Showing 11 changed files with 143 additions and 158 deletions.
18 changes: 13 additions & 5 deletions config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,13 @@ var (
defaultRedisURL, _ = ct.NewOptURLAbsoluteFromString("redis://localhost:6379") //nolint:gochecknoglobals
)

const (
// This minimum value was chosen not as a recommendation, but more to protect the host system from thousands of syscalls +
// the CPU time it takes to read the archive over and over again. It is somewhat arbitrary.
// It likely doesn't make sense to use an interval this frequent in production use-cases.
minimumFileDataSourceMonitoringInterval = 100 * time.Millisecond
)

// DefaultLoggers is the default logging configuration used by Relay.
//
// Output goes to stdout, except Error level which goes to stderr. Debug level is disabled.
Expand Down Expand Up @@ -158,11 +165,12 @@ type AutoConfigConfig struct {

// OfflineModeConfig contains configuration parameters for the offline/file data source feature.
type OfflineModeConfig struct {
FileDataSource string `conf:"FILE_DATA_SOURCE"`
EnvDatastorePrefix string `conf:"ENV_DATASTORE_PREFIX"`
EnvDatastoreTableName string `conf:"ENV_DATASTORE_TABLE_NAME"`
EnvAllowedOrigin ct.OptStringList `conf:"ENV_ALLOWED_ORIGIN"`
EnvAllowedHeader ct.OptStringList `conf:"ENV_ALLOWED_HEADER"`
FileDataSource string `conf:"FILE_DATA_SOURCE"`
FileDataSourceMonitoringInterval ct.OptDuration `conf:"FILE_DATA_SOURCE_MONITORING_INTERVAL"`
EnvDatastorePrefix string `conf:"ENV_DATASTORE_PREFIX"`
EnvDatastoreTableName string `conf:"ENV_DATASTORE_TABLE_NAME"`
EnvAllowedOrigin ct.OptStringList `conf:"ENV_ALLOWED_ORIGIN"`
EnvAllowedHeader ct.OptStringList `conf:"ENV_ALLOWED_HEADER"`
}

// EventsConfig contains configuration parameters for proxying events.
Expand Down
24 changes: 18 additions & 6 deletions config/config_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,12 @@ var (
errOfflineModeWithEnvironments = errors.New("cannot configure specific environments if offline mode is enabled")
errAutoConfWithoutDBDisambig = errors.New(`when using auto-configuration with database storage, database prefix (or,` +
` if using DynamoDB, table name) must be specified and must contain "` + AutoConfigEnvironmentIDPlaceholder + `"`)
errRedisURLWithHostAndPort = errors.New("please specify Redis URL or host/port, but not both")
errRedisBadHostname = errors.New("invalid Redis hostname")
errConsulTokenAndTokenFile = errors.New("Consul token must be specified as either an inline value or a file, but not both") //nolint:stylecheck
errAutoConfWithFilters = errors.New("cannot configure filters if auto-configuration is enabled")
errMissingProjKey = errors.New("when filters are configured, all environments must specify a 'projKey'")
errRedisURLWithHostAndPort = errors.New("please specify Redis URL or host/port, but not both")
errRedisBadHostname = errors.New("invalid Redis hostname")
errConsulTokenAndTokenFile = errors.New("Consul token must be specified as either an inline value or a file, but not both") //nolint:stylecheck
errAutoConfWithFilters = errors.New("cannot configure filters if auto-configuration is enabled")
errMissingProjKey = errors.New("when filters are configured, all environments must specify a 'projKey'")
errInvalidFileDataSourceMonitoringInterval = fmt.Errorf("file data source monitoring interval must be >= %s", minimumFileDataSourceMonitoringInterval)
)

func errEnvironmentWithNoSDKKey(envName string) error {
Expand Down Expand Up @@ -76,6 +77,7 @@ func ValidateConfig(c *Config, loggers ldlog.Loggers) error {
validateConfigEnvironments(&result, c)
validateConfigDatabases(&result, c, loggers)
validateConfigFilters(&result, c)
validateOfflineMode(&result, c)

return result.GetError()
}
Expand Down Expand Up @@ -122,7 +124,7 @@ func validateConfigEnvironments(result *ct.ValidationResult, c *Config) {
}
if c.OfflineMode.FileDataSource == "" {
if c.OfflineMode.EnvDatastorePrefix != "" || c.OfflineMode.EnvDatastoreTableName != "" ||
len(c.OfflineMode.EnvAllowedOrigin.Values()) != 0 || len(c.OfflineMode.EnvAllowedHeader.Values()) != 0 {
len(c.OfflineMode.EnvAllowedOrigin.Values()) != 0 || len(c.OfflineMode.EnvAllowedHeader.Values()) != 0 || c.OfflineMode.FileDataSourceMonitoringInterval.IsDefined() {
result.AddError(nil, errOfflineModePropertiesWithNoFile)
}
} else {
Expand Down Expand Up @@ -184,6 +186,16 @@ func validateConfigFilters(result *ct.ValidationResult, c *Config) {
}
}
}

func validateOfflineMode(result *ct.ValidationResult, c *Config) {
if c.OfflineMode.FileDataSourceMonitoringInterval.IsDefined() {
interval := c.OfflineMode.FileDataSourceMonitoringInterval.GetOrElse(0)
if interval < minimumFileDataSourceMonitoringInterval {
result.AddError(nil, errInvalidFileDataSourceMonitoringInterval)
}
}
}

func validateConfigDatabases(result *ct.ValidationResult, c *Config, loggers ldlog.Loggers) {
normalizeRedisConfig(result, c)

Expand Down
15 changes: 15 additions & 0 deletions config/test_data_configs_invalid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ func makeInvalidConfigs() []testDataInvalidConfig {
makeInvalidConfigOfflineModeAllowedHeaderWithNoFile(),
makeInvalidConfigOfflineModePrefixWithNoFile(),
makeInvalidConfigOfflineModeTableNameWithNoFile(),
makeInvalidConfigOfflineModeWithMonitoringInterval("0s"),
makeInvalidConfigOfflineModeWithMonitoringInterval("-1s"),
makeInvalidConfigOfflineModeWithMonitoringInterval("99ms"),
makeInvalidConfigRedisInvalidHostname(),
makeInvalidConfigRedisInvalidDockerPort(),
makeInvalidConfigRedisConflictingParams(),
Expand Down Expand Up @@ -240,6 +243,18 @@ EnvDatastoreTableName = table
return c
}

func makeInvalidConfigOfflineModeWithMonitoringInterval(interval string) testDataInvalidConfig {
c := testDataInvalidConfig{name: "offline mode table name with no file"}
c.fileError = errInvalidFileDataSourceMonitoringInterval.Error()
c.fileContent = `
[OfflineMode]
fileDataSource = foo.tar.gz
fileDataSourceMonitoringInterval = ` + interval + `
`
return c
}

func makeInvalidConfigRedisInvalidHostname() testDataInvalidConfig {
c := testDataInvalidConfig{name: "Redis - invalid hostname"}
c.envVarsError = "invalid Redis hostname"
Expand Down
33 changes: 31 additions & 2 deletions config/test_data_configs_valid_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ func makeValidConfigs() []testDataValidConfig {
makeValidConfigExplicitOldDefaultBaseURI(),
makeValidConfigAutoConfig(),
makeValidConfigAutoConfigWithDatabase(),
makeValidConfigFileData(),
makeValidConfigOfflineModeMinimal(),
makeValidConfigOfflineModeWithMonitoringInterval("100ms"),
makeValidConfigOfflineModeWithMonitoringInterval("1s"),
makeValidConfigOfflineModeWithMonitoringInterval("5m"),
makeValidConfigRedisMinimal(),
makeValidConfigRedisAll(),
makeValidConfigRedisURL(),
Expand Down Expand Up @@ -335,7 +338,7 @@ Enabled = true
return c
}

func makeValidConfigFileData() testDataValidConfig {
func makeValidConfigOfflineModeMinimal() testDataValidConfig {
c := testDataValidConfig{name: "file data properties"}
c.makeConfig = func(c *Config) {
c.OfflineMode.FileDataSource = "my-file-path"
Expand All @@ -350,6 +353,32 @@ FileDataSource = my-file-path
return c
}

func MustOptDurationFromString(duration string) ct.OptDuration {
opt, err := ct.NewOptDurationFromString(duration)
if err != nil {
panic(err)
}
return opt
}

func makeValidConfigOfflineModeWithMonitoringInterval(interval string) testDataValidConfig {
c := testDataValidConfig{name: "file data properties"}
c.makeConfig = func(c *Config) {
c.OfflineMode.FileDataSource = "my-file-path"
c.OfflineMode.FileDataSourceMonitoringInterval = MustOptDurationFromString(interval)
}
c.envVars = map[string]string{
"FILE_DATA_SOURCE": "my-file-path",
"FILE_DATA_SOURCE_MONITORING_INTERVAL": interval,
}
c.fileContent = `
[OfflineMode]
FileDataSource = my-file-path
FileDataSourceMonitoringInterval = ` + interval + `
`
return c
}

func makeValidConfigRedisMinimal() testDataValidConfig {
c := testDataValidConfig{name: "Redis - minimal parameters"}
c.makeConfig = func(c *Config) {
Expand Down
15 changes: 8 additions & 7 deletions docs/configuration.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,13 +93,14 @@ _(6)_ When using a database store, if there are multiple environments, it is nec

This section is only applicable if [offline mode](https://docs.launchdarkly.com/home/advanced/relay-proxy-enterprise/offline) is enabled for your account.

| Property in file | Environment var | Type | Default | Description |
|--------------------------|----------------------------|:------:|:--------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `fileDataSource` | `FILE_DATA_SOURCE` | String | | Path to the offline mode data file that you have downloaded from LaunchDarkly. |
| `envDatastorePrefix` | `ENV_DATASTORE_PREFIX` | String | | If using a Redis, Consul, or DynamoDB store, this string will be added to all database keys to distinguish them from any other environments that are using the database. _(6)_ |
| `envDatastoreTableName ` | `ENV_DATASTORE_TABLE_NAME` | String | | If using a DynamoDB store, this specifies the table name. _(6)_ |
| `envAllowedOrigin` | `ENV_ALLOWED_ORIGIN` | URI | | If provided, adds CORS headers to prevent access from other domains. This variable can be provided multiple times per environment (if using the `ENV_ALLOWED_ORIGIN` variable, specify a comma-delimited list). |
| `envAllowedHeader` | `ENV_ALLOWED_HEADER` | String | | If provided, adds the specify headers to the list of accepted headers for CORS requests. This variable can be provided multiple times per environment (if using the `ENV_ALLOWED_HEADER` variable, specify a comma-delimited list). |
| Property in file | Environment var | Type | Default | Description |
|------------------------------------|----------------------------------------|:--------:|:--------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `fileDataSource` | `FILE_DATA_SOURCE` | String | | Path to the offline mode data file that you have downloaded from LaunchDarkly. |
| `fileDataSourceMonitoringInterval` | `FILE_DATA_SOURCE_MONITORING_INTERVAL` | Duration | `1s` | How often the file data source is checked for changes. Minimum is 100ms. To reduce computation and syscalls, raise the interval (for example, `5m` for every 5 minutes.) |
| `envDatastorePrefix` | `ENV_DATASTORE_PREFIX` | String | | If using a Redis, Consul, or DynamoDB store, this string will be added to all database keys to distinguish them from any other environments that are using the database. _(6)_ |
| `envDatastoreTableName ` | `ENV_DATASTORE_TABLE_NAME` | String | | If using a DynamoDB store, this specifies the table name. _(6)_ |
| `envAllowedOrigin` | `ENV_ALLOWED_ORIGIN` | URI | | If provided, adds CORS headers to prevent access from other domains. This variable can be provided multiple times per environment (if using the `ENV_ALLOWED_ORIGIN` variable, specify a comma-delimited list). |
| `envAllowedHeader` | `ENV_ALLOWED_HEADER` | String | | If provided, adds the specify headers to the list of accepted headers for CORS requests. This variable can be provided multiple times per environment (if using the `ENV_ALLOWED_HEADER` variable, specify a comma-delimited list). |

Note that the last three properties have the same meanings and the same environment variables names as the corresponding properties in the `[AutoConfig]` section described above. It is not possible to use `[OfflineMode]` and `[AutoConfig]` at the same time.

Expand Down

0 comments on commit 7bea824

Please sign in to comment.