diff --git a/docs/plan-unas-support.md b/docs/plan-unas-support.md index f8235d7b..2b7a6bf7 100644 --- a/docs/plan-unas-support.md +++ b/docs/plan-unas-support.md @@ -9,8 +9,8 @@ alexgreenbank in the package header and README. UNAS Pro is a standalone UniFi OS console with **no Network application**: no sites, no `/status`, its own credentials, its own host. `inputunifi`'s sites → devices → per-site flow has nothing to offer it, and folding it in would mean bolting a second credential set onto -`Controller`. So: a new `pkg/inputunas` plugin with its own `[unas]` config block, disabled -by default and a no-op when unconfigured. +`Controller`. So: a new `pkg/inputunas` plugin with its own `[unas]` config block, off by +default and a no-op when unconfigured. ### The one real blocker @@ -130,11 +130,13 @@ in all three example configs. `go test ./...` and `golangci-lint run` are clean. Refinements over the plan below: -1. **The opt-in mechanism is "no devices configured", not `disable`.** `Disable` zero-values - to false, so a field named `disable` cannot make a plugin default-off. `Initialize` returns - silently when the device list is empty, and unlike `inputunifi` it does **not** synthesize - a default device from `[unas.defaults]` — doing so would poll a host the operator never - named. `disable` remains as an explicit off switch. +1. **Opt-in is `enable`, defaulting to false — not `disable`.** A field named `disable` + zero-values to false, so it cannot make a plugin default-off and reads as a double + negative; `enable bool` defaults to off and says what it does. Two further guards: + `Initialize` returns silently when the device list is empty, and unlike `inputunifi` it + does **not** synthesize a default device from `[unas.defaults]` — doing so would poll a + host the operator never named. Devices configured while `enable` is false log one error, + since that combination is always a mistake. 2. **`Metrics` returns `(metrics, nil)` whenever any console was collected.** Not politeness: `poller.collectMetrics` uses `if result.err != nil { ... } else if result.metric != nil`, so returning both discards every metric in that cycle. One dead console out of three would @@ -156,7 +158,7 @@ Refinements over the plan below: ```toml [unas] - disable = false + enable = true [unas.defaults] user = "unpoller" pass = "" diff --git a/examples/up.conf.example b/examples/up.conf.example index 04eb2a88..7120125f 100644 --- a/examples/up.conf.example +++ b/examples/up.conf.example @@ -275,13 +275,13 @@ # A UNAS Pro is a standalone UniFi OS console with no Network application, so it is polled # by its own input plugin with its own credentials and host -- not as a UniFi controller. # -# This section is entirely optional and does nothing unless you add at least one -# [[unas.device]] below: with no devices configured the plugin stays silent and inert. +# This section is entirely optional. The plugin is off unless you set enable = true and +# add at least one [[unas.device]] below; otherwise it stays silent and inert. # Metrics land under the unifi_unas_ prefix in Prometheus, and in the unas_device, # unas_pool, unas_disk, and unas_share measurements in InfluxDB. # #[unas] -# disable = false +# enable = true # # Defaults applied to every device that does not set its own value. #[unas.defaults] diff --git a/examples/up.json.example b/examples/up.json.example index 8a3c941e..b167eb1f 100644 --- a/examples/up.json.example +++ b/examples/up.json.example @@ -87,8 +87,8 @@ }, "unas": { - "//": "UNAS Pro consoles. JSON cannot carry comments, so this section ships disabled: it shows the shape, and flipping disable to false with a real url turns it on.", - "disable": true, + "//": "UNAS Pro consoles. JSON cannot carry comments, so this section ships off: it shows the shape, and setting enable to true with a real url turns it on.", + "enable": false, "defaults": { "user": "unpoller", "pass": "", diff --git a/examples/up.yaml.example b/examples/up.yaml.example index 7767666b..67b3dfd2 100644 --- a/examples/up.yaml.example +++ b/examples/up.yaml.example @@ -91,7 +91,7 @@ unifi: # by its own input plugin with its own credentials and host -- not as a UniFi controller. # This section is optional: with no devices configured the plugin stays silent and inert. #unas: -# disable: false +# enable: true # defaults: # user: "unpoller" # pass: "" diff --git a/pkg/inputunas/README.md b/pkg/inputunas/README.md index 85c5cff3..c4039c25 100644 --- a/pkg/inputunas/README.md +++ b/pkg/inputunas/README.md @@ -2,9 +2,10 @@ Polls UniFi UNAS Pro storage consoles and hands their metrics to every configured output. -**This plugin is opt-in and does nothing until you configure at least one device.** With no -`[unas]` section, or a section with no devices, it stays silent and inert — it logs nothing -and polls nothing. +**This plugin is opt-in: `enable` defaults to `false`.** It does nothing until you set +`enable = true` *and* configure at least one device. With no `[unas]` section it stays silent +and inert — it logs nothing and polls nothing. If you configure devices but leave `enable` +unset, it logs one error saying so rather than failing quietly. ## Why a separate plugin? @@ -17,7 +18,7 @@ rather than as another UniFi controller. ```toml [unas] - disable = false + enable = true # Applied to any device that does not set its own value. [unas.defaults] @@ -44,7 +45,7 @@ tables. See `examples/up.{conf,json,yaml}.example`. | Variable | Meaning | |---|---| -| `UP_UNAS_DISABLE` | Disable the plugin outright. | +| `UP_UNAS_ENABLE` | Turn the plugin on. Defaults to `false`. | | `UP_UNAS_DEFAULT_USER` | Default username for every device. | | `UP_UNAS_DEFAULT_PASS` | Default password. | | `UP_UNAS_DEFAULT_VERIFY_SSL` | Default TLS verification. | diff --git a/pkg/inputunas/config.go b/pkg/inputunas/config.go index 76e45c6d..b60c8c91 100644 --- a/pkg/inputunas/config.go +++ b/pkg/inputunas/config.go @@ -46,9 +46,9 @@ type Device struct { // Config contains our configuration data. type Config struct { sync.RWMutex // locks a Device's client while it re-authenticates. - Default Device `json:"defaults" toml:"defaults" xml:"default" yaml:"defaults"` - Disable bool `json:"disable" toml:"disable" xml:"disable,attr" yaml:"disable"` - Devices []*Device `json:"devices" toml:"device" xml:"device" yaml:"devices"` + Default Device `json:"defaults" toml:"defaults" xml:"default" yaml:"defaults"` + Enable bool `json:"enable" toml:"enable" xml:"enable,attr" yaml:"enable"` + Devices []*Device `json:"devices" toml:"device" xml:"device" yaml:"devices"` } func init() { // nolint: gochecknoinits @@ -64,7 +64,7 @@ func init() { // nolint: gochecknoinits // setDefaults fills a device's unset fields from the defaults block, then from package // defaults. It never invents a URL: an empty URL is how "not configured" is expressed, and // synthesizing one (as inputunifi does with localhost:8443) would make the plugin poll a -// host the operator never named. That is what keeps UNAS support opt-in. +// host the operator never named. Opt-in rests on Config.Enable; this is the second guard. func (u *InputUNAS) setDefaults(d *Device) *Device { if d.User == "" { d.User = u.Default.User diff --git a/pkg/inputunas/input.go b/pkg/inputunas/input.go index 8897b56c..af7db324 100644 --- a/pkg/inputunas/input.go +++ b/pkg/inputunas/input.go @@ -21,18 +21,26 @@ var ErrNoDevices = errors.New("no UNAS devices configured") // Satisfies poller.Input interface. func (u *InputUNAS) Initialize(l poller.Logger) error { if u.Config == nil { - u.Config = &Config{Disable: true} + u.Config = &Config{} } - if u.Logger = l; u.Disable { + u.Logger = l + + // enable defaults to false, so the plugin is off until an operator asks for it. Warn when + // consoles are configured but the switch was never flipped -- that combination is always a + // mistake, and silence would leave the operator hunting for missing metrics. An operator + // who has never heard of UNAS has no devices either, and still sees nothing. + if !u.Enable { + if len(u.configuredDevices()) > 0 { + u.LogErrorf("UNAS devices are configured but unas.enable is false; not polling them") + } + return nil } u.Devices = u.configuredDevices() - // No [unas] block means no devices, which means nothing to say. Staying silent here is - // what makes the plugin opt-in: an operator who has never heard of UNAS should see no - // trace of it in the log. + // Enabled with nothing to poll: nothing to say, and nothing to do. if len(u.Devices) == 0 { return nil } @@ -96,7 +104,7 @@ func (u *InputUNAS) logDevice(d *Device) { // returning `(metrics, err)` after one console of three fails would throw away the two that // worked. Satisfies poller.Input interface. func (u *InputUNAS) Metrics(filter *poller.Filter) (*poller.Metrics, error) { - if u.Disable { + if !u.Enable { return nil, nil } @@ -234,7 +242,7 @@ func (u *InputUNAS) RawMetrics(filter *poller.Filter) ([]byte, error) { // DebugInput checks that every configured console can be reached and authenticated against. // Satisfies poller.Input interface. func (u *InputUNAS) DebugInput() (bool, error) { - if u == nil || u.Config == nil || u.Disable { + if u == nil || u.Config == nil || !u.Enable { return true, nil } @@ -294,7 +302,7 @@ func formatConfig(config *Config) *Config { CertPaths: config.Default.CertPaths, Timeout: config.Default.Timeout, }, - Disable: config.Disable, + Enable: config.Enable, Devices: devices, } } diff --git a/pkg/inputunas/input_test.go b/pkg/inputunas/input_test.go index b8f53024..252e1a5c 100644 --- a/pkg/inputunas/input_test.go +++ b/pkg/inputunas/input_test.go @@ -3,6 +3,8 @@ package inputunas_test import ( "net/http" "net/http/httptest" + "os" + "path/filepath" "sync/atomic" "testing" @@ -11,6 +13,8 @@ import ( "github.com/unpoller/unifi/v5" "github.com/unpoller/unpoller/pkg/inputunas" "github.com/unpoller/unpoller/pkg/poller" + "golift.io/cnfg" + "golift.io/cnfgfile" ) const ( @@ -76,7 +80,7 @@ func newInput(t *testing.T, urls ...string) *inputunas.InputUNAS { devices[i] = &inputunas.Device{URL: u, User: "unpoller", Pass: "secret"} } - return &inputunas.InputUNAS{Config: &inputunas.Config{Devices: devices}} + return &inputunas.InputUNAS{Config: &inputunas.Config{Enable: true, Devices: devices}} } func TestMetricsCollectsConsole(t *testing.T) { @@ -155,15 +159,25 @@ func TestMetricsPartialFailureStillReportsHealthyConsole(t *testing.T) { require.Len(t, m.UNASDevices, 1) } -func TestMetricsDisabled(t *testing.T) { +// enable defaults to false, so a config that names consoles but never sets it must not poll +// them. Configuring the devices here is the point: an empty config would pass even if the +// switch were ignored entirely. +func TestNotEnabled(t *testing.T) { t.Parallel() - u := &inputunas.InputUNAS{Config: &inputunas.Config{Disable: true}} + srv := (&unasServer{}).start(t) + u := &inputunas.InputUNAS{Config: &inputunas.Config{ + Devices: []*inputunas.Device{{URL: srv.URL, User: "unpoller", Pass: "secret"}}, + }} require.NoError(t, u.Initialize(nil)) m, err := u.Metrics(nil) require.NoError(t, err) - require.Nil(t, m) + require.Nil(t, m, "enable is false, so nothing is polled") + + ok, err := u.DebugInput() + require.NoError(t, err) + require.True(t, ok, "--debugio must not reach a console the operator has not enabled") } // With no [unas] block the plugin must be silent and inert: that is what makes UNAS support @@ -176,7 +190,7 @@ func TestUnconfiguredIsInert(t *testing.T) { m, err := u.Metrics(nil) require.NoError(t, err) - require.Nil(t, m, "an unconfigured plugin disables itself") + require.Nil(t, m, "an unconfigured plugin stays off") ok, err := u.DebugInput() require.NoError(t, err) @@ -186,7 +200,7 @@ func TestUnconfiguredIsInert(t *testing.T) { func TestDeviceWithNoURLIsIgnored(t *testing.T) { t.Parallel() - u := &inputunas.InputUNAS{Config: &inputunas.Config{Devices: []*inputunas.Device{{}, nil}}} + u := &inputunas.InputUNAS{Config: &inputunas.Config{Enable: true, Devices: []*inputunas.Device{{}, nil}}} require.NoError(t, u.Initialize(nil)) m, err := u.Metrics(nil) @@ -217,3 +231,70 @@ func TestRawMetrics(t *testing.T) { _, err = u.RawMetrics(&poller.Filter{Unit: 9}) require.ErrorIs(t, err, inputunas.ErrNoDevices) } + +// The enable switch has to survive four independent binding paths -- toml, json, yaml and the +// UP_ environment -- each driven by its own struct tag. A typo in any one tag leaves the +// plugin silently off for users of that format, which is the same class of invisible failure +// as a missing tag anywhere else in this config. Note the env name derives from the xml tag, +// not the json one. +func TestEnableBindsInEveryConfigFormat(t *testing.T) { + t.Parallel() + + write := func(t *testing.T, name, body string) string { + t.Helper() + + path := filepath.Join(t.TempDir(), name) + require.NoError(t, os.WriteFile(path, []byte(body), 0o600)) + + return path + } + + for _, tc := range []struct { + name string + file string + body string + }{ + {"toml", "up.conf", "[unas]\n enable = true\n[[unas.device]]\n url = \"https://nas\"\n"}, + {"json", "up.json", `{"unas":{"enable":true,"devices":[{"url":"https://nas"}]}}`}, + {"yaml", "up.yaml", "unas:\n enable: true\n devices:\n - url: \"https://nas\"\n"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + input := &inputunas.InputUNAS{Config: &inputunas.Config{}} + require.NoError(t, cnfgfile.Unmarshal(input, write(t, tc.file, tc.body))) + assert.True(t, input.Enable, "enable did not bind from %s", tc.name) + require.Len(t, input.Devices, 1) + assert.Equal(t, "https://nas", input.Devices[0].URL) + }) + } +} + +// Not parallel: t.Setenv is incompatible with a parallel test or parent. +// +// The env variable name comes from the xml tag, not the json one, which is why this is worth +// asserting rather than assuming from the toml/json/yaml cases above. +func TestEnableBindsFromEnvironment(t *testing.T) { + t.Setenv("UP_UNAS_ENABLE", "true") + + input := &inputunas.InputUNAS{Config: &inputunas.Config{}} + _, err := cnfg.UnmarshalENV(input, "UP") + require.NoError(t, err) + assert.True(t, input.Enable, "UP_UNAS_ENABLE did not bind") +} + +// The shipped examples must all default to off, so that installing unpoller never starts +// polling a storage console nobody asked for. +func TestShippedExamplesDoNotEnableUNAS(t *testing.T) { + t.Parallel() + + for _, name := range []string{"up.conf.example", "up.json.example", "up.yaml.example"} { + t.Run(name, func(t *testing.T) { + t.Parallel() + + input := &inputunas.InputUNAS{Config: &inputunas.Config{}} + require.NoError(t, cnfgfile.Unmarshal(input, filepath.Join("..", "..", "examples", name))) + assert.False(t, input.Enable, "%s ships with UNAS enabled", name) + }) + } +}