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

[Elastic Agent] Use http2 to connect to Fleet Server. #26474

Merged
merged 4 commits into from
Jun 28, 2021

Conversation

blakerouse
Copy link
Contributor

@blakerouse blakerouse commented Jun 24, 2021

What does this PR do?

Changes Elastic Agent to connect to Fleet Server using HTTP2.

Why is it important?

HTTP2 providers better performance and scale.

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

@blakerouse blakerouse added the Team:Elastic-Agent Label for the Agent team label Jun 24, 2021
@blakerouse blakerouse self-assigned this Jun 24, 2021
@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 Jun 24, 2021
@blakerouse blakerouse marked this pull request as ready for review June 24, 2021 13:23
@elasticmachine
Copy link
Collaborator

Pinging @elastic/agent (Team:Agent)

@elasticmachine
Copy link
Collaborator

elasticmachine commented Jun 24, 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

Expand to view the summary

Build stats

  • Build Cause: blakerouse commented: /test

  • Start Time: 2021-06-28T18:29:49.129+0000

  • Duration: 79 min 14 sec

  • Commit: 248747d

Test stats 🧪

Test Results
Failed 0
Passed 161
Skipped 25
Total 186

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 161
Skipped 25
Total 186

@blakerouse blakerouse added the backport-v7.14.0 Automated backport with mergify label Jun 28, 2021
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.

LGTM just small things i noticed

@@ -73,6 +73,63 @@ func TestTLSDialer(
}), nil
}

type DialerH2 interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

not an issue of this PR but it's pretty disturbing we don't provide Context Dialer

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 agree, I was trying to limit the change as much as possible. But we should look at switching all of it to context dailers.

var m sync.Mutex

return DialerFuncH2(func(network, address string, cfg *tls.Config) (net.Conn, error) {
switch network {
Copy link
Contributor

Choose a reason for hiding this comment

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

can this be extracted as it is the same thing as regular http with some modifier passed in changing nextProtos

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sadly no because the function has different parameters, h2 passes in the cfg *tls.Config where the normal http dialer does not

dialer := transport.NetDialer(timeout)
if scheme == "http" {
return &http.Transport{Dial: dialer.Dial}, nil
}
tlsConfig, err := tlscommon.LoadTLSConfig(tls)
Copy link
Contributor

Choose a reason for hiding this comment

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

this was first just so you dont end up instantiating dialer in case tls config is incorrect.

@blakerouse
Copy link
Contributor Author

/package

@blakerouse
Copy link
Contributor Author

/test

@blakerouse blakerouse merged commit 638b62d into elastic:master Jun 28, 2021
@blakerouse blakerouse deleted the agent-http2 branch June 28, 2021 19:57
mergify bot pushed a commit that referenced this pull request Jun 28, 2021
* Use http2 to connect to Fleet Server.

* Add changelog.

* Fix import formatting.

* Fix issue with tls and http2.

(cherry picked from commit 638b62d)
blakerouse added a commit that referenced this pull request Jun 28, 2021
* Use http2 to connect to Fleet Server.

* Add changelog.

* Fix import formatting.

* Fix issue with tls and http2.

(cherry picked from commit 638b62d)

Co-authored-by: Blake Rouse <blake.rouse@elastic.co>
@urso
Copy link

urso commented Jun 29, 2021

Reading the original issue, it sounds like we want to be able to downgrade to HTTP1 transparantly. Is that correct? Do we have tests for that? AFAIK the http2 package does not downgrade gracefully.

Why not http.Transport.ForceUpateHTTP2? This way we would rather try to upgrade a connection, and will continue to support different kind of proxies (which the http2 package does not support).

mdelapenya added a commit to mdelapenya/beats that referenced this pull request Jun 29, 2021
* master:
  Osquerybeat: set the raw index name to supress the timestamp suffix (elastic#26545)
  [Heartbeat] add screenshots config to synthetics (elastic#26455)
  [Elastic Agent] Use http2 to connect to Fleet Server. (elastic#26474)
  Remove all docs about  Beats central management (elastic#26399)
  update data.json for gcp billing (elastic#26506)
  Skip x-pack metricbeat tests (elastic#26537)
  [Elastic Agent] Fix issue with FLEET_CA not being used with Fleet Server in container (elastic#26529)
  Add changelog entry for  elastic#26224 (elastic#26531)
  Add inttests for the x-pack/metricbeat on a PR/branches basis (elastic#26526)
  Suppress too many errors (elastic#26224)
  Fix master's linting issue (elastic#26517)
  [libbeat] Fix encoding and file offset issues in the disk queue (elastic#26484)
  Add log_group_name_prefix config option for aws-cloudwatch input (elastic#26187)
  Update shared-deduplication.asciidoc (elastic#26492)
  Add Recorded Future support to threatintel module (elastic#26481)
v1v added a commit to v1v/beats that referenced this pull request Jun 29, 2021
…arwin-arm64

* upstream/master: (295 commits)
  Update urllib to 1.26.5. (elastic#26380)
  Update golang.org/x/crypto (elastic#26448)
  [Filebeat] Update Fortinet Ingest Pipeline (elastic#24816)
  Move parsers outside of filestream input so others can use them as well (elastic#26541)
  [Filebeat] Fix `threatintel.indicator.url.full` field not populating (elastic#26508)
  [Filebeat] Add network direction processor to Zeek and Suricata modules (elastic#24620)
  Logging code cleanup related to Nomad auto-discovery (elastic#26498)
  [Metricbeat] Add Couchbase's Sync Gateway module (elastic#25599)
  Refactor add_cloud_metadata to handle ECS fields easier (elastic#26438)
  [Elastic Agent] Improper casting of int64 (elastic#26520)
  [Elastic Agent] Enable configuring monitoring namespace (elastic#26439)
  [Heartbeat] configure permissions for synthetics config (elastic#26393)
  Osquerybeat: set the raw index name to supress the timestamp suffix (elastic#26545)
  [Heartbeat] add screenshots config to synthetics (elastic#26455)
  [Elastic Agent] Use http2 to connect to Fleet Server. (elastic#26474)
  Remove all docs about  Beats central management (elastic#26399)
  update data.json for gcp billing (elastic#26506)
  Skip x-pack metricbeat tests (elastic#26537)
  [Elastic Agent] Fix issue with FLEET_CA not being used with Fleet Server in container (elastic#26529)
  Add changelog entry for  elastic#26224 (elastic#26531)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.14.0 Automated backport with mergify Team:Elastic-Agent Label for the Agent team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Elastic Agent] Work todo for communication with new Fleet Server
4 participants