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

Add a header round tripper option to httpcommon #27509

Merged

Conversation

michel-laterman
Copy link
Contributor

What does this PR do?

Add a new TransportOption to add a set of headers to each HTTP request
through a custom http.RoundTripper. It will set the passed headers to
each request if the header key is not present. Use the new transport
option to add User-Agent headers to heartbeat, metricbeat, and filebeat.

Why is it important?

Adds the User-Agent header to Metricbeat. Consolidate the beats to use the
same approach when injecting the User-Agent header where possible.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [] I have made corresponding changes to the documentation
  • [] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

Add a new TransportOption to add a set of headers to each HTTP request
through a custom http.RoundTripper. It will set the passed headers to
each request if the header key is not present. Use the new transport
option to add User-Agent headers to heartbeat, metricbeat, and filebeat.
@elasticmachine
Copy link
Collaborator

Pinging @elastic/agent (Team:Agent)

@botelastic botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Aug 19, 2021
@elasticmachine
Copy link
Collaborator

elasticmachine commented Aug 19, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2021-09-07T16:47:02.587+0000

  • Duration: 151 min 49 sec

  • Commit: 4212819

Test stats 🧪

Test Results
Failed 0
Passed 53890
Skipped 5324
Total 59214

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 53890
Skipped 5324
Total 59214

@andrewkroh
Copy link
Member

andrewkroh commented Aug 19, 2021

Can you please check that the eslegclient includes the user-agent too.

And the Kibana client.

@michel-laterman
Copy link
Contributor Author

Hey @andrewkroh,

I can pretty easily add this to the kibana config here:

rt, err := config.Transport.Client()

However, I'm not sure what I should set the user-agent to? Is there any way to get the beat name from the Kibana client implentation?

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM WRT heartbeat code changes! Nice cleanup!

@@ -674,3 +675,28 @@ func mustParseURL(t *testing.T, url string) *url.URL {
}
return parsed
}

func TestUserAgentInject(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing this excellent test!

Add the User-Agent header with the RoundTripper to requests made by the
Kibana and eslegclient. The value for the User-Agent will be constructed
from what is returned by the os executable name.
@@ -110,6 +113,14 @@ func NewConnection(s ConnectionSettings) (*Connection, error) {
}
}

name, err := os.Executable()
Copy link
Member

@andrewkroh andrewkroh Aug 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps UserAgent could be a setting within ConnectionSettings that is passed in by the caller. That seems like something that it could pass in when creating the ES client. Like this location where the Beat name is known

ConnectionSettings: eslegclient.ConnectionSettings{

Get user-agent values for the Kibana and eslegclient from existing
config settings that get passed into the clients.
@@ -77,7 +77,7 @@ func main() {
Path: u.Path,
SpaceID: *spaceID,
Transport: transport,
})
}, "beat")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where I can get the beatname for this command.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't have a beat name since it's a development tool. It's not too important that this thing set a user-agent, but if it must have one then maybe it can identify itself as Beat Development Tools.

@mergify
Copy link
Contributor

mergify bot commented Aug 26, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b http-useragent-headers upstream/http-useragent-headers
git merge upstream/master
git push upstream http-useragent-headers

@@ -77,7 +77,7 @@ func main() {
Path: u.Path,
SpaceID: *spaceID,
Transport: transport,
})
}, "beat")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It won't have a beat name since it's a development tool. It's not too important that this thing set a user-agent, but if it must have one then maybe it can identify itself as Beat Development Tools.

@@ -57,7 +58,8 @@ type Connection struct {

// ConnectionSettings are the settings needed for a Connection
type ConnectionSettings struct {
URL string
URL string
Beatname string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this client useful for things other than a Beat? Having a parameter that is the beat name sort of implies that this is only usable by a Beat. An alternative would be to accept the user agent string as a parameter or call it "product name". Even if it were called "product name" the code would still assume it's a beat since it creates the user-agent string based on the beat version info. (I'm not saying you need to change it.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to expose this one? this is also not serialized so it hiding it should not matter, wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewkroh, I think that Beatname is OK; i'm not aware of it being used in other locations
@michalpristas the outputs/elasticsearch and monitoring/report/elasticsearch both construct ConnectionSettings structs when creating clients. Having this exposed allows them to do them to fill it in

@@ -474,7 +474,7 @@ func kibanaClient(cfg kibanaConfig, headers map[string]string) (*kibana.Client,
IgnoreVersion: true,
Transport: transport,
Headers: headers,
}, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should probably use Elastic-Agent or do you have a list, enum... other set of values you agreed on with anybody?

Copy link
Contributor

@michalpristas michalpristas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, couple of nits and Qs along the way

@@ -531,7 +531,7 @@ func (b *Beat) Setup(settings Settings, bt beat.Creator, setup SetupSettings) er
if outCfg.Name() != "elasticsearch" {
return fmt.Errorf("Index management requested but the Elasticsearch output is not configured/enabled")
}
esClient, err := eslegclient.NewConnectedClient(outCfg.Config())
esClient, err := eslegclient.NewConnectedClient(outCfg.Config(), b.Info.Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be set to hostname in some cases, is that ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's not really what we want, is there a way we can get the *beat name (metricebeat, heartbet, etc)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this one will be correct most of the time but i've seen some initialization with hostname. would be good to get some input from somebody who knows this a bit better

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The beatname is under b.Info.Beat. b.Info.Name is sometimes the hostname, or whatever the user has set in name in the config file.

@@ -57,7 +58,8 @@ type Connection struct {

// ConnectionSettings are the settings needed for a Connection
type ConnectionSettings struct {
URL string
URL string
Beatname string
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to expose this one? this is also not serialized so it hiding it should not matter, wdyt?

@@ -153,7 +155,8 @@ func NewClientWithConfigDefault(config *ClientConfig, defaultPort int) (*Client,
headers.Set(k, v)
}

rt, err := config.Transport.Client()
userAgent := useragent.UserAgent(beatname)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if beatname is empty? we should not try to add any header and risk overriding already valid value with nothing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a default Libbeat value

@michel-laterman michel-laterman merged commit 8a5dac6 into elastic:master Sep 7, 2021
@michel-laterman michel-laterman deleted the http-useragent-headers branch September 7, 2021 21:50
@michel-laterman michel-laterman added the backport-v7.16.0 Automated backport with mergify label Sep 7, 2021
mergify bot pushed a commit that referenced this pull request Sep 7, 2021
* Add a header round tripper option to httpcommon

Add a new TransportOption to add a set of headers to each HTTP request
through a custom http.RoundTripper. It will set the passed headers to
each request if the header key is not present. Use the new transport
option to add User-Agent headers to heartbeat, metricbeat, and filebeat.

* Fix useragent injection in heartbeat IPMonitor jobs

* Add User-Agent header to kibana and eslegclient

Add the User-Agent header with the RoundTripper to requests made by the
Kibana and eslegclient. The value for the User-Agent will be constructed
from what is returned by the os executable name.

* Fix syntax errors

* Get user agent from existing ingo

Get user-agent values for the Kibana and eslegclient from existing
config settings that get passed into the clients.

* change from settings.Name to b.Info.Name

* Fix missing param

* Rename export dashboard useragent value

* Review feedback

* Change beat.Info.Name to beat.Info.Beat

* Fix typo

(cherry picked from commit 8a5dac6)

# Conflicts:
#	heartbeat/monitors/active/http/task.go
#	heartbeat/monitors/active/http/task_test.go
#	libbeat/cmd/export/dashboard.go
michel-laterman added a commit that referenced this pull request Sep 8, 2021
#27788)

* Add a header round tripper option to httpcommon (#27509)

* Add a header round tripper option to httpcommon

Add a new TransportOption to add a set of headers to each HTTP request
through a custom http.RoundTripper. It will set the passed headers to
each request if the header key is not present. Use the new transport
option to add User-Agent headers to heartbeat, metricbeat, and filebeat.

* Fix useragent injection in heartbeat IPMonitor jobs

* Add User-Agent header to kibana and eslegclient

Add the User-Agent header with the RoundTripper to requests made by the
Kibana and eslegclient. The value for the User-Agent will be constructed
from what is returned by the os executable name.

* Fix syntax errors

* Get user agent from existing ingo

Get user-agent values for the Kibana and eslegclient from existing
config settings that get passed into the clients.

* change from settings.Name to b.Info.Name

* Fix missing param

* Rename export dashboard useragent value

* Review feedback

* Change beat.Info.Name to beat.Info.Beat

* Fix typo

(cherry picked from commit 8a5dac6)

# Conflicts:
#	heartbeat/monitors/active/http/task.go
#	heartbeat/monitors/active/http/task_test.go
#	libbeat/cmd/export/dashboard.go

* Cleanup merge

* Fix for Filebeat

Co-authored-by: Michel Laterman <[email protected]>
Co-authored-by: michel-laterman <[email protected]>
mdelapenya added a commit to mdelapenya/beats that referenced this pull request Sep 9, 2021
* master: (39 commits)
  [Heartbeat] Move JSON tests from python->go (elastic#27816)
  docs: simplify permissions for Dockerfile COPY (elastic#27754)
  Osquerybeat: Fix osquery logger plugin severy levels mapping (elastic#27789)
  [Filebeat] Update compatibility function to remove processor description on ES < 7.9.0 (elastic#27774)
  warn log entry and no validation failure when both queue_url and buck… (elastic#27612)
  libbeat/cmd/instance: ensure test config file has appropriate permissions (elastic#27178)
  [Heartbeat] Add httpcommon options to ZipURL (elastic#27699)
  Add a header round tripper option to httpcommon (elastic#27509)
  [Elastic Agent] Add validation to ensure certificate paths are absolute. (elastic#27779)
  Rename dashboards according to module.yml files for master (elastic#27749)
  Refactor vagrantfile, add scripts for provisioning with docker/kind (elastic#27726)
  Accept syslog dates with leading 0 (elastic#27775)
  [Filebeat] Add timezone config option to decode_cef and syslog input (elastic#27727)
  [Filebeat] Threatintel compatibility updates (elastic#27323)
  Add support for ephemeral containers in elastic agent dynamic provider (elastic#27707)
  [Filebeat] Integration tests in CI for AWS-S3 input (elastic#27491)
  Fix flakyness of TestFilestreamEmptyLine (elastic#27705)
  [Filebeat] kafka v2 using parsers (elastic#27335)
  Update Kafka version parsing / supported range (elastic#27720)
  Update Sarama to 1.29.1 (elastic#27717)
  ...
Icedroid pushed a commit to Icedroid/beats that referenced this pull request Nov 1, 2021
* Add a header round tripper option to httpcommon

Add a new TransportOption to add a set of headers to each HTTP request
through a custom http.RoundTripper. It will set the passed headers to
each request if the header key is not present. Use the new transport
option to add User-Agent headers to heartbeat, metricbeat, and filebeat.

* Fix useragent injection in heartbeat IPMonitor jobs

* Add User-Agent header to kibana and eslegclient

Add the User-Agent header with the RoundTripper to requests made by the
Kibana and eslegclient. The value for the User-Agent will be constructed
from what is returned by the os executable name.

* Fix syntax errors

* Get user agent from existing ingo

Get user-agent values for the Kibana and eslegclient from existing
config settings that get passed into the clients.

* change from settings.Name to b.Info.Name

* Fix missing param

* Rename export dashboard useragent value

* Review feedback

* Change beat.Info.Name to beat.Info.Beat

* Fix typo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.16-candidate backport-v7.16.0 Automated backport with mergify enhancement Team:Elastic-Agent Label for the Agent team v7.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[All beats] HTTP request should include the beat name and the version as part of the user-agent
6 participants