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.

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

[exporter/datadog] add basic API key validation on startup #36510

Merged
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
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
27 changes: 27 additions & 0 deletions .chloggen/dd-config-api-key-fix.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
# Use this changelog template to create an entry for release notes.

# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix'
change_type: bug_fix

# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver)
component: connector/datadog, exporter/datadog, pkg/datadog

# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`).
note: throw error if datadog API key contains invalid characters

# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists.
issues: [36509]

# (Optional) One or more lines of additional information to render under the primary note.
# These lines will be padded with 2 spaces and then inserted directly into the document.
# Use pipe (|) for multiline entries.
subtext:

# If your change doesn't affect end users or the exported elements of any package,
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label.
# Optional: The change log or logs in which this entry should be included.
# e.g. '[user]' or '[user, api]'
# Include 'user' if the change is relevant to end users.
# Include 'api' if there is a change to a library API.
# Default: '[user]'
change_logs: [user]
2 changes: 1 addition & 1 deletion connector/datadogconnector/example_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (
)

func TestExamples(t *testing.T) {
t.Setenv("DD_API_KEY", "testvalue")
t.Setenv("DD_API_KEY", "aaaaaaaaa")
factories := newTestComponents(t)
const configFile = "./examples/config.yaml"
// https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/33594
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ data:
exporters:
datadog:
api:
key: <YOUR_API_KEY_HERE>
key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" # Change this to your Datadog API key
site: datadoghq.com # Change this to your site if not using the default
processors:
resourcedetection:
Expand Down
2 changes: 1 addition & 1 deletion exporter/datadogexporter/examples_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ func TestExamples(t *testing.T) {
continue
}
t.Run(filepath.Base(f.Name()), func(t *testing.T) {
t.Setenv("DD_API_KEY", "testvalue")
t.Setenv("DD_API_KEY", "aaaaaaaaa")
name := filepath.Join(folder, f.Name())
// https://github.com/open-telemetry/opentelemetry-collector-contrib/issues/33594
// nolint:staticcheck
Expand Down
2 changes: 1 addition & 1 deletion exporter/datadogexporter/factory_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -300,7 +300,7 @@ func TestOnlyMetadata(t *testing.T) {
BackOffConfig: configretry.NewDefaultBackOffConfig(),
QueueSettings: exporterhelper.NewDefaultQueueConfig(),

API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
Metrics: MetricsConfig{TCPAddrConfig: confignet.TCPAddrConfig{Endpoint: server.URL}},
Traces: TracesConfig{TCPAddrConfig: confignet.TCPAddrConfig{Endpoint: server.URL}},
OnlyMetadata: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ exporters:
verbosity: detailed
datadog:
api:
key: "key"
key: "aaa"
tls:
insecure_skip_verify: true
host_metadata:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ receivers:
exporters:
datadog:
api:
key: "key"
key: "aaa"
tls:
insecure_skip_verify: true
host_metadata:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ receivers:
exporters:
datadog:
api:
key: "key"
key: "aaa"
tls:
insecure_skip_verify: true
host_metadata:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ exporters:
verbosity: detailed
datadog:
api:
key: "key"
key: "aaa"
tls:
insecure_skip_verify: true
host_metadata:
Expand Down
4 changes: 2 additions & 2 deletions exporter/datadogexporter/metrics_exporter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ func TestNewExporter(t *testing.T) {

cfg := &Config{
API: APIConfig{
Key: "ddog_32_characters_long_api_key1",
Key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
},
Metrics: MetricsConfig{
TCPAddrConfig: confignet.TCPAddrConfig{
Expand Down Expand Up @@ -422,7 +422,7 @@ func TestNewExporter_Zorkian(t *testing.T) {

cfg := &Config{
API: APIConfig{
Key: "ddog_32_characters_long_api_key1",
Key: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
},
Metrics: MetricsConfig{
TCPAddrConfig: confignet.TCPAddrConfig{
Expand Down
13 changes: 10 additions & 3 deletions pkg/datadog/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package config // import "github.com/open-telemetry/opentelemetry-collector-cont
import (
"errors"
"fmt"
"regexp"
"strings"
"time"

Expand All @@ -27,6 +28,8 @@ var (
ErrNoMetadata = errors.New("only_metadata can't be enabled when host_metadata::enabled = false or host_metadata::hostname_source != first_resource")
// ErrInvalidHostname is returned when the hostname is invalid.
ErrEmptyEndpoint = errors.New("endpoint cannot be empty")
// ErrAPIKeyFormat is returned if API key contains invalid characters
ErrAPIKeyFormat = errors.New("api.key contains invalid characters")
jackgopack4 marked this conversation as resolved.
Show resolved Hide resolved
)

const (
Expand Down Expand Up @@ -120,10 +123,16 @@ func (c *Config) Validate() error {
return fmt.Errorf("hostname field is invalid: %w", err)
}

if c.API.Key == "" {
if string(c.API.Key) == "" {
return ErrUnsetAPIKey
}

nonHex := regexp.MustCompile("[^0-9a-fA-F]")
jackgopack4 marked this conversation as resolved.
Show resolved Hide resolved
jackgopack4 marked this conversation as resolved.
Show resolved Hide resolved
invalidAPIKeyChars := nonHex.FindAllString(string(c.API.Key), -1)
if len(invalidAPIKeyChars) > 0 {
return errors.Join(ErrAPIKeyFormat, fmt.Errorf("invalid characters: %s", strings.Join(invalidAPIKeyChars, ", ")))
jackgopack4 marked this conversation as resolved.
Show resolved Hide resolved
}

if err := c.Traces.Validate(); err != nil {
return err
}
Expand Down Expand Up @@ -250,8 +259,6 @@ func (c *Config) Unmarshal(configMap *confmap.Conf) error {
}
c.warnings = append(c.warnings, renamingWarnings...)

c.API.Key = configopaque.String(strings.TrimSpace(string(c.API.Key)))
mx-psi marked this conversation as resolved.
Show resolved Hide resolved

// If an endpoint is not explicitly set, override it based on the site.
if !configMap.IsSet("metrics::endpoint") {
c.Metrics.TCPAddrConfig.Endpoint = fmt.Sprintf("https://api.%s", c.API.Site)
Expand Down
35 changes: 21 additions & 14 deletions pkg/datadog/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,18 +42,25 @@ func TestValidate(t *testing.T) {
cfg: &Config{},
err: ErrUnsetAPIKey.Error(),
},
{
name: "invalid format api::key",
jackgopack4 marked this conversation as resolved.
Show resolved Hide resolved
cfg: &Config{
API: APIConfig{Key: "'aaaaaaa"},
},
err: ErrAPIKeyFormat.Error(),
},
{
name: "invalid hostname",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
TagsConfig: TagsConfig{Hostname: "invalid_host"},
},
err: "hostname field is invalid: invalid_host is not RFC1123 compliant",
},
{
name: "no metadata",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
OnlyMetadata: true,
HostMetadata: HostMetadataConfig{Enabled: false},
},
Expand All @@ -62,45 +69,45 @@ func TestValidate(t *testing.T) {
{
name: "span name remapping valid",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
Traces: TracesExporterConfig{TracesConfig: TracesConfig{SpanNameRemappings: map[string]string{"old.opentelemetryspan.name": "updated.name"}}},
},
},
{
name: "span name remapping empty val",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
Traces: TracesExporterConfig{TracesConfig: TracesConfig{SpanNameRemappings: map[string]string{"oldname": ""}}},
},
err: "'' is not valid value for span name remapping",
},
{
name: "span name remapping empty key",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
Traces: TracesExporterConfig{TracesConfig: TracesConfig{SpanNameRemappings: map[string]string{"": "newname"}}},
},
err: "'' is not valid key for span name remapping",
},
{
name: "ignore resources valid",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
Traces: TracesExporterConfig{TracesConfig: TracesConfig{IgnoreResources: []string{"[123]"}}},
},
},
{
name: "ignore resources missing bracket",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
Traces: TracesExporterConfig{TracesConfig: TracesConfig{IgnoreResources: []string{"[123"}}},
},
err: "'[123' is not valid resource filter regular expression",
},
{
name: "invalid histogram settings",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
Metrics: MetricsConfig{
HistConfig: HistogramConfig{
Mode: HistogramModeNoBuckets,
Expand All @@ -113,7 +120,7 @@ func TestValidate(t *testing.T) {
{
name: "TLS settings are valid",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
ClientConfig: confighttp.ClientConfig{
TLSSetting: configtls.ClientConfig{
InsecureSkipVerify: true,
Expand All @@ -124,14 +131,14 @@ func TestValidate(t *testing.T) {
{
name: "With trace_buffer",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
Traces: TracesExporterConfig{TraceBuffer: 10},
},
},
{
name: "With peer_tags",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
Traces: TracesExporterConfig{
TracesConfig: TracesConfig{
PeerTags: []string{"tag1", "tag2"},
Expand All @@ -142,7 +149,7 @@ func TestValidate(t *testing.T) {
{
name: "With confighttp client configs",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
ClientConfig: confighttp.ClientConfig{
ReadBufferSize: 100,
WriteBufferSize: 200,
Expand All @@ -160,7 +167,7 @@ func TestValidate(t *testing.T) {
{
name: "unsupported confighttp client configs",
cfg: &Config{
API: APIConfig{Key: "notnull"},
API: APIConfig{Key: "aaaaaaa"},
ClientConfig: confighttp.ClientConfig{
Endpoint: "endpoint",
Compression: "gzip",
Expand All @@ -177,7 +184,7 @@ func TestValidate(t *testing.T) {
t.Run(testInstance.name, func(t *testing.T) {
err := testInstance.cfg.Validate()
if testInstance.err != "" {
assert.EqualError(t, err, testInstance.err)
assert.ErrorContains(t, err, testInstance.err)
} else {
assert.NoError(t, err)
}
Expand Down