Skip to content

Commit d05c463

Browse files
StephenWakelyStephenWakely
authored
[AGTMETRICS-179]Validate all API keys given to the forwarder also come with a config path (#36193)
1 parent 9486953 commit d05c463

File tree

23 files changed

+271
-125
lines changed

23 files changed

+271
-125
lines changed

‎comp/aggregator/demultiplexer/demultiplexerimpl/test_agent_demultiplexer.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -183,7 +183,7 @@ func initTestAgentDemultiplexerWithFlushInterval(log log.Component, hostname hos
183183
opts.DontStartForwarders = true
184184
opts.EnableNoAggregationPipeline = true
185185

186-
sharedForwarderOptions := defaultforwarder.NewOptions(pkgconfigsetup.Datadog(), log, nil)
186+
sharedForwarderOptions, _ := defaultforwarder.NewOptions(pkgconfigsetup.Datadog(), log, nil)
187187
sharedForwarder := defaultforwarder.NewDefaultForwarder(pkgconfigsetup.Datadog(), log, sharedForwarderOptions)
188188

189189
orchestratorForwarder := option.New[defaultforwarder.Forwarder](defaultforwarder.NoopForwarder{})

‎comp/aggregator/diagnosesendermanager/diagnosesendermanagerimpl/sendermanager.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,11 @@ func (sender *diagnoseSenderManager) LazyGetSenderManager() (sender.SenderManage
7474
log := sender.deps.Log
7575
config := sender.deps.Config
7676
haAgent := sender.deps.HaAgent
77-
forwarder := defaultforwarder.NewDefaultForwarder(config, log, defaultforwarder.NewOptions(config, log, nil))
77+
options, err := defaultforwarder.NewOptions(config, log, nil)
78+
if err != nil {
79+
return nil, err
80+
}
81+
forwarder := defaultforwarder.NewDefaultForwarder(config, log, options)
7882
orchestratorForwarder := option.NewPtr[defaultforwarder.Forwarder](defaultforwarder.NoopForwarder{})
7983
eventPlatformForwarder := option.NewPtr[eventplatform.Forwarder](eventplatformimpl.NewNoopEventPlatformForwarder(sender.deps.Hostname, sender.deps.LogsCompressor))
8084
senderManager = aggregator.InitAndStartAgentDemultiplexer(

‎comp/forwarder/defaultforwarder/default_forwarder.go

+12-5
Original file line numberDiff line numberDiff line change
@@ -121,24 +121,31 @@ func ToggleFeature(features, flag Features) Features { return features ^ flag }
121121
func HasFeature(features, flag Features) bool { return features&flag != 0 }
122122

123123
// NewOptions creates new Options with default values
124-
func NewOptions(config config.Component, log log.Component, keysPerDomain map[string][]utils.APIKeys) *Options {
124+
func NewOptions(config config.Component, log log.Component, keysPerDomain map[string][]utils.APIKeys) (*Options, error) {
125+
126+
resolvers, err := pkgresolver.NewSingleDomainResolvers(keysPerDomain)
127+
if err != nil {
128+
return nil, err
129+
}
125130

126-
resolvers := pkgresolver.NewSingleDomainResolvers(keysPerDomain)
127131
vectorMetricsURL, err := pkgconfigsetup.GetObsPipelineURL(pkgconfigsetup.Metrics, config)
128132
if err != nil {
129133
log.Error("Misconfiguration of agent observability_pipelines_worker endpoint for metrics: ", err)
130134
}
131135
if r, ok := resolvers[utils.GetInfraEndpoint(config)]; ok && vectorMetricsURL != "" {
132136
log.Debugf("Configuring forwarder to send metrics to observability_pipelines_worker: %s", vectorMetricsURL)
133-
134-
resolvers[utils.GetInfraEndpoint(config)] = pkgresolver.NewDomainResolverWithMetricToVector(
137+
resolver, err := pkgresolver.NewDomainResolverWithMetricToVector(
135138
r.GetBaseDomain(),
136139
r.GetAPIKeysInfo(),
137140
vectorMetricsURL,
138141
)
142+
if err != nil {
143+
return nil, err
144+
}
145+
resolvers[utils.GetInfraEndpoint(config)] = resolver
139146
}
140147

141-
return NewOptionsWithResolvers(config, log, resolvers)
148+
return NewOptionsWithResolvers(config, log, resolvers), nil
142149
}
143150

144151
// NewOptionsWithResolvers creates new Options with default values

‎comp/forwarder/defaultforwarder/default_forwarder_test.go

+4-2
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,8 @@ func TestDefaultForwarderUpdateAPIKey(t *testing.T) {
4141
utils.NewAPIKeys("additional_endpoints", "api_key3"),
4242
},
4343
}
44-
forwarderOptions := NewOptions(mockConfig, log, keysPerDomains)
44+
forwarderOptions, err := NewOptions(mockConfig, log, keysPerDomains)
45+
require.NoError(t, err)
4546
forwarder := NewDefaultForwarder(mockConfig, log, forwarderOptions)
4647

4748
// API keys from the domain resolvers match
@@ -78,7 +79,8 @@ func TestDefaultForwarderUpdateAdditionalEndpointAPIKey(t *testing.T) {
7879
utils.NewAPIKeys("additional_endpoints", "api_key3"),
7980
},
8081
}
81-
forwarderOptions := NewOptions(mockConfig, log, keysPerDomains)
82+
forwarderOptions, err := NewOptions(mockConfig, log, keysPerDomains)
83+
require.NoError(t, err)
8284
forwarder := NewDefaultForwarder(mockConfig, log, forwarderOptions)
8385

8486
// API keys from the domain resolvers match

‎comp/forwarder/defaultforwarder/forwarder.go

+24-9
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package defaultforwarder
77

88
import (
99
"context"
10+
"fmt"
1011
"strings"
1112

1213
"go.uber.org/fx"
@@ -34,29 +35,42 @@ type provides struct {
3435
StatusProvider status.InformationProvider
3536
}
3637

37-
func newForwarder(dep dependencies) provides {
38+
func newForwarder(dep dependencies) (provides, error) {
3839
if dep.Params.useNoopForwarder {
3940
return provides{
4041
Comp: NoopForwarder{},
41-
}
42+
}, nil
4243
}
4344

44-
options := createOptions(dep.Params, dep.Config, dep.Log)
45+
options, err := createOptions(dep.Params, dep.Config, dep.Log)
46+
if err != nil {
47+
return provides{}, err
48+
}
4549

46-
return NewForwarder(dep.Config, dep.Log, dep.Lc, true, options)
50+
return NewForwarder(dep.Config, dep.Log, dep.Lc, true, options), nil
4751
}
4852

49-
func createOptions(params Params, config config.Component, log log.Component) *Options {
53+
func createOptions(params Params, config config.Component, log log.Component) (*Options, error) {
5054
var options *Options
5155
keysPerDomain, err := utils.GetMultipleEndpoints(config)
5256
if err != nil {
5357
log.Error("Misconfiguration of agent endpoints: ", err)
58+
return nil, fmt.Errorf("Misconfiguration of agent endpoints: %s", err)
5459
}
5560

5661
if !params.withResolver {
57-
options = NewOptions(config, log, keysPerDomain)
62+
options, err = NewOptions(config, log, keysPerDomain)
63+
if err != nil {
64+
log.Error("Error creating forwarder options: ", err)
65+
return nil, fmt.Errorf("Error creating forwarder options: %s", err)
66+
}
5867
} else {
59-
options = NewOptionsWithResolvers(config, log, resolver.NewSingleDomainResolvers(keysPerDomain))
68+
r, err := resolver.NewSingleDomainResolvers(keysPerDomain)
69+
if err != nil {
70+
log.Error("Error creating resolver: ", err)
71+
return nil, fmt.Errorf("Error creating resolver: %s", err)
72+
}
73+
options = NewOptionsWithResolvers(config, log, r)
6074
}
6175
// Override the DisableAPIKeyChecking only if WithFeatures was called
6276
if disableAPIKeyChecking, ok := params.disableAPIKeyCheckingOverride.Get(); ok {
@@ -72,7 +86,7 @@ func createOptions(params Params, config config.Component, log log.Component) *O
7286
}
7387
log.Infof("domain '%s' has %d keys: %s", resolver.GetBaseDomain(), len(scrubbedKeys), strings.Join(scrubbedKeys, ", "))
7488
}
75-
return options
89+
return options, nil
7690
}
7791

7892
// NewForwarder returns a new forwarder component.
@@ -98,7 +112,8 @@ func NewForwarder(config config.Component, log log.Component, lc fx.Lifecycle, i
98112
}
99113

100114
func newMockForwarder(config config.Component, log log.Component) provides {
115+
options, _ := NewOptions(config, log, nil)
101116
return provides{
102-
Comp: NewDefaultForwarder(config, log, NewOptions(config, log, nil)),
117+
Comp: NewDefaultForwarder(config, log, options),
103118
}
104119
}

‎comp/forwarder/defaultforwarder/forwarder_health_test.go

+26-19
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616
"testing"
1717

1818
"github.com/stretchr/testify/assert"
19+
"github.com/stretchr/testify/require"
1920

2021
"github.com/DataDog/datadog-agent/comp/core/config"
2122
logmock "github.com/DataDog/datadog-agent/comp/core/log/mock"
@@ -35,16 +36,18 @@ func TestCheckValidAPIKey(t *testing.T) {
3536

3637
keysPerDomains := map[string][]utils.APIKeys{
3738
ts1.URL: {
38-
utils.NewAPIKeys("", "api_key1"),
39-
utils.NewAPIKeys("", "api_key2"),
39+
utils.NewAPIKeys("path", "api_key1"),
40+
utils.NewAPIKeys("path", "api_key2"),
4041
},
4142
ts2.URL: {
42-
utils.NewAPIKeys("", "key3"),
43+
utils.NewAPIKeys("path", "key3"),
4344
},
4445
}
4546
log := logmock.New(t)
4647
cfg := config.NewMock(t)
47-
fh := forwarderHealth{log: log, config: cfg, domainResolvers: resolver.NewSingleDomainResolvers(keysPerDomains)}
48+
r, err := resolver.NewSingleDomainResolvers(keysPerDomains)
49+
require.NoError(t, err)
50+
fh := forwarderHealth{log: log, config: cfg, domainResolvers: r}
4851
fh.init()
4952
assert.True(t, fh.checkValidAPIKey())
5053

@@ -55,18 +58,18 @@ func TestCheckValidAPIKey(t *testing.T) {
5558

5659
func TestComputeDomainsURL(t *testing.T) {
5760
keysPerDomains := map[string][]utils.APIKeys{
58-
"https://app.datadoghq.com": {utils.NewAPIKeys("", "api_key1")},
59-
"https://custom.datadoghq.com": {utils.NewAPIKeys("", "api_key2")},
60-
"https://custom.agent.datadoghq.com": {utils.NewAPIKeys("", "api_key3")},
61-
"https://app.datadoghq.eu": {utils.NewAPIKeys("", "api_key4")},
62-
"https://app.us2.datadoghq.com": {utils.NewAPIKeys("", "api_key5")},
63-
"https://app.xx9.datadoghq.com": {utils.NewAPIKeys("", "api_key5")},
64-
"https://custom.agent.us2.datadoghq.com": {utils.NewAPIKeys("", "api_key6")},
61+
"https://app.datadoghq.com": {utils.NewAPIKeys("path", "api_key1")},
62+
"https://custom.datadoghq.com": {utils.NewAPIKeys("path", "api_key2")},
63+
"https://custom.agent.datadoghq.com": {utils.NewAPIKeys("path", "api_key3")},
64+
"https://app.datadoghq.eu": {utils.NewAPIKeys("path", "api_key4")},
65+
"https://app.us2.datadoghq.com": {utils.NewAPIKeys("path", "api_key5")},
66+
"https://app.xx9.datadoghq.com": {utils.NewAPIKeys("path", "api_key5")},
67+
"https://custom.agent.us2.datadoghq.com": {utils.NewAPIKeys("path", "api_key6")},
6568
// debatable whether the next one should be changed to `api.`, preserve pre-existing behavior for now
66-
"https://app.datadoghq.internal": {utils.NewAPIKeys("", "api_key7")},
67-
"https://app.myproxy.com": {utils.NewAPIKeys("", "api_key8")},
68-
"https://app.ddog-gov.com": {utils.NewAPIKeys("", "api_key9")},
69-
"https://custom.ddog-gov.com": {utils.NewAPIKeys("", "api_key10")},
69+
"https://app.datadoghq.internal": {utils.NewAPIKeys("path", "api_key7")},
70+
"https://app.myproxy.com": {utils.NewAPIKeys("path", "api_key8")},
71+
"https://app.ddog-gov.com": {utils.NewAPIKeys("path", "api_key9")},
72+
"https://custom.ddog-gov.com": {utils.NewAPIKeys("path", "api_key10")},
7073
}
7174

7275
expectedMap := map[string][]string{
@@ -84,7 +87,9 @@ func TestComputeDomainsURL(t *testing.T) {
8487
sort.Strings(keys)
8588
}
8689
log := logmock.New(t)
87-
fh := forwarderHealth{log: log, domainResolvers: resolver.NewSingleDomainResolvers(keysPerDomains)}
90+
r, err := resolver.NewSingleDomainResolvers(keysPerDomains)
91+
require.NoError(t, err)
92+
fh := forwarderHealth{log: log, domainResolvers: r}
8893
fh.init()
8994

9095
// lexicographical sort for assert
@@ -166,14 +171,16 @@ func TestUpdateAPIKey(t *testing.T) {
166171

167172
// starting API Keys, before the update
168173
keysPerDomains := map[string][]utils.APIKeys{
169-
ts1.URL: {utils.NewAPIKeys("", "api_key1"), utils.NewAPIKeys("", "api_key2")},
170-
ts2.URL: {utils.NewAPIKeys("", "api_key3")},
174+
ts1.URL: {utils.NewAPIKeys("path", "api_key1"), utils.NewAPIKeys("path", "api_key2")},
175+
ts2.URL: {utils.NewAPIKeys("path", "api_key3")},
171176
}
172177

173178
log := logmock.New(t)
174179
cfg := config.NewMock(t)
175180

176-
fh := forwarderHealth{log: log, config: cfg, domainResolvers: resolver.NewSingleDomainResolvers(keysPerDomains)}
181+
r, err := resolver.NewSingleDomainResolvers(keysPerDomains)
182+
require.NoError(t, err)
183+
fh := forwarderHealth{log: log, config: cfg, domainResolvers: r}
177184
fh.init()
178185
assert.True(t, fh.checkValidAPIKey())
179186

0 commit comments

Comments
 (0)