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.

Sign up for GitHub

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

Support env overrides for all inputs of the same type #6334

Merged
merged 1 commit into from
Apr 15, 2016
Merged

Support env overrides for all inputs of the same type #6334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- [#6228](https://github.com/influxdata/influxdb/pull/6228): Support for multiple listeners for collectd and OpenTSDB inputs.
- [#6292](https://github.com/influxdata/influxdb/issues/6292): Allow percentile to be used as a selector.
- [#5707](https://github.com/influxdata/influxdb/issues/5707): Return a deprecated message when IF NOT EXISTS is used.
- [#6334](https://github.com/influxdata/influxdb/pull/6334): Allow environment variables to be set per input type.

### Bugfixes

Expand Down
3 changes: 3 additions & 0 deletions cmd/influxd/run/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,9 @@ func (c *Config) applyEnvOverrides(prefix string, spec reflect.Value) error {
// e.g. GRAPHITE_0
if f.Kind() == reflect.Slice || f.Kind() == reflect.Array {
for i := 0; i < f.Len(); i++ {
if err := c.applyEnvOverrides(key, f.Index(i)); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code specifically seems to be wrong. This code is confusing so I'm not certain, but I think this would allow me to say INFLUXDB_0_BIND_ADDRESS to set all of the bind addresses in every section where that applies. If I'm reading this right, the unit test you have should work with the original code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The unit test doesn't work with the current code because inputs which allow multiple listeners don't get matched to environment variables like INFLUXDB_UDP_BIND_ADDRESS, only INFLUXDB_UDP_[i]_BIND_ADDRESS. This change allows both to be used to override the bind address for a section and only applies to sections which allow multiple instances, which are all input sections (UDP, graphite, etc).

Not sure how this would match a INFLUXDB_0_BIND_ADDRESS env var to all bind addresses. Maybe I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I think I misunderstood the f.Index(i) code. I thought that was an integer index that got returned.

return err
}
if err := c.applyEnvOverrides(fmt.Sprintf("%s_%d", key, i), f.Index(i)); err != nil {
return err
}
Expand Down
12 changes: 11 additions & 1 deletion cmd/influxd/run/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ bind-address = ":2010"
[[udp]]
bind-address = ":4444"

[[udp]]
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it be possible to set the second UDP listener with environment variables without needing an empty section here? I think that most people who would use environment variables to configure InfluxDB probably wouldn't use a configuration file, otherwise they would just use the configuration file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Environment variable overrides are applied by walking through the config struct to look for matching environment variable names. Since only one listener is added to the config for each section by default, multiple listeners are not possible with environment variables alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Allowing configuration of multiple listeners through env var overrides would require more substantial changes to applyEnvOverrides.


[monitoring]
enabled = true

Expand All @@ -154,6 +156,10 @@ enabled = true
t.Fatalf("failed to set env var: %v", err)
}

if err := os.Setenv("INFLUXDB_UDP_0_BIND_ADDRESS", ":5555"); err != nil {
t.Fatalf("failed to set env var: %v", err)
}

if err := os.Setenv("INFLUXDB_GRAPHITE_1_PROTOCOL", "udp"); err != nil {
t.Fatalf("failed to set env var: %v", err)
}
Expand All @@ -170,10 +176,14 @@ enabled = true
t.Fatalf("failed to apply env overrides: %v", err)
}

if c.UDPInputs[0].BindAddress != ":4444" {
if c.UDPInputs[0].BindAddress != ":5555" {
t.Fatalf("unexpected udp bind address: %s", c.UDPInputs[0].BindAddress)
}

if c.UDPInputs[1].BindAddress != ":1234" {
t.Fatalf("unexpected udp bind address: %s", c.UDPInputs[1].BindAddress)
}

if c.GraphiteInputs[1].Protocol != "udp" {
t.Fatalf("unexpected graphite protocol: %s", c.GraphiteInputs[1].Protocol)
}
Expand Down