Skip to content

Commit

Permalink
[Heartbeat] Produce error rather than panic on missing source (elasti…
Browse files Browse the repository at this point in the history
…c#24404) (elastic#24563)

Fixes elastic#24403.

With the changes to the heartbeat config syntax in 7.12 the `source`
field is now required. Our config validation code didn't actually check
for this field's presence, which caused an NPE.

This PR adds a validation checking for that config's presence. It also
adds tests for the validation code for config sub-fields. There were no
defects found in the validations for source.inline, or source.browser,
but a few tests were missing.

Instead of the panic seen in elastic#24403 users will now get the error seen
below.

```
2021-03-05T15:41:40.146-0600	ERROR	instance/beat.go:952	Exiting: could not create monitor: job err could not parse suite config: config 'source' must be specified for this monitor, if upgrading from a previous experimental version please see our new config docs accessing 'heartbeat.monitors.0' (source:'sample-synthetics-config/heartbeat.yml')
Exiting: could not create monitor: job err could not parse suite config: config 'source' must be specified for this monitor, if upgrading from a previous experimental version please see our new config docs accessing 'heartbeat.monitors.0' (source:'sample-synthetics-config/heartbeat.yml')
```

Co-authored-by: Blake Rouse <blake.rouse@elastic.co>
  • Loading branch information
andrewvc and blakerouse authored Mar 18, 2021
1 parent 685b241 commit 08e2048
Show file tree
Hide file tree
Showing 6 changed files with 148 additions and 2 deletions.
5 changes: 5 additions & 0 deletions x-pack/heartbeat/monitors/browser/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ type Config struct {

var ErrNameRequired = fmt.Errorf("config 'name' must be specified for this monitor")
var ErrIdRequired = fmt.Errorf("config 'id' must be specified for this monitor")
var ErrSourceRequired = fmt.Errorf("config 'source' must be specified for this monitor, if upgrading from a previous experimental version please see our new config docs")

func (c *Config) Validate() error {
if c.Name == "" {
Expand All @@ -33,5 +34,9 @@ func (c *Config) Validate() error {
return ErrIdRequired
}

if c.Source == nil {
return ErrSourceRequired
}

return nil
}
55 changes: 55 additions & 0 deletions x-pack/heartbeat/monitors/browser/config_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

package browser

import (
"testing"

"github.com/stretchr/testify/require"

"github.com/elastic/beats/v7/x-pack/heartbeat/monitors/browser/source"
)

func TestConfig_Validate(t *testing.T) {
testSource := source.Source{Inline: &source.InlineSource{Script: "//something"}}

tests := []struct {
name string
cfg *Config
wantErr error
}{
{
"no error",
&Config{Id: "myid", Name: "myname", Source: &testSource},
nil,
},
{
"no id",
&Config{Name: "myname", Source: &testSource},
ErrIdRequired,
},
{
"no name",
&Config{Id: "myid", Source: &testSource},
ErrNameRequired,
},
{
"no source",
&Config{Id: "myid", Name: "myname"},
ErrSourceRequired,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
err := tt.cfg.Validate()

if tt.wantErr != nil {
require.Equal(t, tt.wantErr, err)
} else {
require.NoError(t, err)
}
})
}
}
4 changes: 3 additions & 1 deletion x-pack/heartbeat/monitors/browser/source/inline.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,11 @@ type InlineSource struct {
BaseSource
}

var ErrNoInlineScript = fmt.Errorf("no 'script' value specified for inline source")

func (s *InlineSource) Validate() error {
if !regexp.MustCompile("\\S").MatchString(s.Script) {
return fmt.Errorf("no 'script' value specified for inline source")
return ErrNoInlineScript
}

return nil
Expand Down
42 changes: 42 additions & 0 deletions x-pack/heartbeat/monitors/browser/source/inline_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

package source

import (
"testing"

"github.com/stretchr/testify/require"
)

func TestInlineSourceValidation(t *testing.T) {
type testCase struct {
name string
source *InlineSource
wantErr error
}
testCases := []testCase{
{
"no error",
&InlineSource{Script: "a script"},
nil,
},
{
"no script",
&InlineSource{},
ErrNoInlineScript,
},
}

for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
err := tt.source.Validate()
if tt.wantErr != nil {
require.Equal(t, tt.wantErr, err)
} else {
require.NoError(t, err)
}
})
}
}
2 changes: 1 addition & 1 deletion x-pack/heartbeat/monitors/browser/source/source.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func (s *Source) Active() ISource {
return s.ActiveMemo
}

var ErrInvalidSource = fmt.Errorf("no or unknown source type specified for synthetic monitor")
var ErrInvalidSource = fmt.Errorf("no or unknown source type specified for synthetic monitor.")

func (s *Source) Validate() error {
if s.Active() == nil {
Expand Down
42 changes: 42 additions & 0 deletions x-pack/heartbeat/monitors/browser/source/source_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
// or more contributor license agreements. Licensed under the Elastic License;
// you may not use this file except in compliance with the Elastic License.

package source

import (
"testing"

"github.com/stretchr/testify/require"
)

func TestSourceValidation(t *testing.T) {
type testCase struct {
name string
source Source
wantErr error
}
testCases := []testCase{
{
"no error",
Source{Inline: &InlineSource{}},
nil,
},
{
"no concrete source",
Source{},
ErrInvalidSource,
},
}

for _, tt := range testCases {
t.Run(tt.name, func(t *testing.T) {
err := tt.source.Validate()
if tt.wantErr != nil {
require.Equal(t, tt.wantErr, err)
} else {
require.NoError(t, err)
}
})
}
}

0 comments on commit 08e2048

Please sign in to comment.