-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[forwarder] Allow configuration of multiple endpoints #352
Conversation
Also, adds some logging when the forwarder starts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added two quick comments, let me know what you think about them. I'm prepared to approve as is, though.
pkg/config/config.go
Outdated
return keysPerDomain, err | ||
} | ||
|
||
for domain, apiKeys := range additionalEndpoints { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if there is a domain without any keys? Should that throw an error or should we just be fine with it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should probably log an error and discard that domain, I'll update the PR
pkg/config/config.go
Outdated
|
||
for domain, apiKeys := range additionalEndpoints { | ||
if _, ok := keysPerDomain[domain]; ok { | ||
for _, apiKey := range apiKeys { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should dedupe API Keys. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, I'll do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @gmmeyer on his 2 points. Feel free to merge after addressing those points.
* dedupe api keys and remove empty ones * discard domains that don't have an api key
`go lint` complains otherwise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
What does this PR do?
Allows configuration of multiple endpoints (implements related RFC).
Also, adds some logging when the forwarder starts.
Motivation
Support multiple endpoints. Needed for the agents running on our staging and prod infras.
Additional Notes
The endpoints configuration is not documented in the example
datadog.yaml
since I don't think we want to advertise this feature. To be confirmed though.