Add TLS support for metric beat http server #11482

leopucci · 2019-03-26T20:51:50Z

No description provided.

elasticmachine · 2019-03-26T20:51:52Z

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

leopucci · 2019-03-26T20:59:44Z

This patch solves #11457

leopucci · 2019-03-26T21:03:12Z

How to use:
add ssl config on http.yml

- module: http
  metricsets:
    - server
  host: "localhost"
  port: "8080"
  enabled: true
  #ssl.certificate_authorities: ["/etc/pki/root/ca.pem"]
  ssl.certificate: "/etc/pki/client/cert.pem"
  ssl.key: "/etc/pki/client/cert.key"
  paths:
    - path: "/foo"
      namespace: "foo"
  #    fields: # added to the the response in root. overwrites existing fields
  #      key: "value"

leopucci · 2019-03-26T21:42:04Z

@andrewkroh can you help me understand what this means so I could solve it?

./helper/server/http/config.go
./helper/server/http/http.go
Code differs from goimports' style ^

leopucci · 2019-03-26T21:45:31Z

https://discuss.elastic.co/t/code-differs-from-goimports-style/84671

ruflin · 2019-03-27T14:46:06Z

@leopucci Run make fmt and it should solve your issue.

Any chance to get some tests added to this PR? It also needs a changelog entry.

leopucci · 2019-03-27T14:50:32Z

@ruflin the CI build is failing... i don't know how to make fmt on the CI build.
I will add some tests and add the changelog :)

ruflin · 2019-03-27T15:04:55Z

The CI build is failing because some linting rules are not passing. You need to run make fmt inside the Beats directory and then commit again.

leopucci · 2019-03-27T15:05:57Z

Thanks!

andrewkroh

Thanks for working on this. Can you also add an entry to the CHANGELOG.next.asciidoc file under the "Added" section for Metricbeat.

metricbeat/helper/server/http/http.go

leopucci · 2019-03-27T16:54:56Z

Thanks @andrewkroh for reviewing. Will change right away

metricbeat/helper/server/http/http_test.go

metricbeat/helper/server/http/http.go

leopucci · 2019-03-28T20:24:12Z

@andrewkroh Thanks for the revisions :)

ruflin · 2019-03-29T15:28:49Z

Thanks for all the work @leopucci I think this will also need an update in the docs about the new options: https://github.com/elastic/beats/blob/master/metricbeat/module/http/server/_meta/docs.asciidoc To make CI happy, you must run make fmt update again.

leopucci · 2019-03-29T15:44:51Z

@ruflin @andrewkroh Thanks for helping me to understand the whole process

odacremolbap

Hey @leopucci

awesome code here!
just a couple minor glitches at tests

Thanks a lot for the PR

CHANGELOG.next.asciidoc

metricbeat/helper/server/http/http.go

metricbeat/helper/server/http/http_test.go

leopucci · 2019-03-30T19:44:31Z

Hola @odacremolbap !
I have changed the tests to TDT....
GO is a powerful language. I'm excited with it.
Thanks for taking some time to review my code :)

odacremolbap · 2019-03-31T15:37:50Z

@leopucci Thanks a lot for going down this rabbit hole!

I think we are getting to a nice pattern here, let's improve it a bit more. Let me re-review.

leopucci · 2019-03-31T16:18:52Z

:) I have made some more changes trying to understand why Travis ci is failing.. Locally the tests happen OK!

odacremolbap

Looking very good, thanks for the effort, it is being worth it :-)

Check for requested changes and bring your own opinion on them

metricbeat/helper/server/http/http_test.go

leopucci · 2019-03-31T23:52:52Z

@odacremolbap Thanks for your help! I've tested and committed the new code!

metricbeat/module/http/server/_meta/docs.asciidoc

odacremolbap · 2019-04-01T12:55:38Z

The reason tests are not succeeding is https://github.com/elastic/beats/blob/master/metricbeat/tests/system/test_http.py#L58

I'll have time in some hours to take a deeper look, but you can probably go ahead and modify that string to match the prefix for the HTTP and HTTPS server start log line

leopucci · 2019-04-01T13:04:49Z

I will!

leopucci · 2019-04-01T13:27:43Z

Done :)

andrewkroh · 2019-04-01T14:20:40Z

metricbeat/helper/server/http/http.go

@@ -130,6 +148,11 @@ func (h *HttpServer) handleFunc(writer http.ResponseWriter, req *http.Request) {

 	case "GET":
 		writer.WriteHeader(http.StatusOK)
-		writer.Write([]byte("HTTP Server accepts data via POST"))
+		if h.server.TLSConfig != nil && h.server.TLSConfig.Certificates != nil {


Suggested change

if h.server.TLSConfig != nil && h.server.TLSConfig.Certificates != nil {

if h.server.TLSConfig != nil {

This can't be done. I've tested and for some reason, HTTP server creates a TLSConfig after calling ListenAndServe(), so the HTTP server will print the wrong message if we remove that, and HTTP tests will fail.

Oh, OK. Then I'm glad you tested it!

metricbeat/module/http/server/_meta/docs.asciidoc

leopucci · 2019-04-01T17:20:28Z

\o/!

odacremolbap · 2019-04-01T17:20:52Z

@leopucci thanks again for this contribution, this is looking superb 🍕
please, squash & merge

odacremolbap · 2019-04-01T17:22:44Z

... and rebase ... 😉

Update config.go Update http.go Update config.go Adding ssl tests, adjusting review requests

odacremolbap · 2019-04-01T18:42:10Z

@leopucci
I would say you can merge master at your branch, and keep your changes to solve the conflict.
https://github.com/elastic/beats/pull/11482/conflicts

Looks like it is only the coredns line conflicting there, so be sure to include that one at your branch and you should be good to go.

Ping me if I can help

leopucci · 2019-04-01T19:45:52Z

@odacremolbap Strange... my file already includes coredns line. Don't know if it is the `` around it. The only mod on this file is the new SSL support

odacremolbap · 2019-04-01T20:22:47Z

weird
maybe you can try to solve the conflict using GH UI, the Resolve conflicts button.

leopucci · 2019-04-02T12:16:56Z

@odacremolbap Only those with write access to this repository can merge pull requests.
I think i will not be able to resolve the conflict. On the UI the buttons are not enabled

kaiyan-sheng · 2019-04-02T15:39:54Z

metricbeat/helper/server/http/http.go

-		if err != nil && err != http.ErrServerClosed {
-			logp.Critical("Unable to start HTTP server due to error: %v", err)
+		if h.server.TLSConfig != nil {
+			logp.Info("Starting HTTPS server on %s", h.server.Addr)


Is it HTTP here?

Hi Kaiyan, it is HTTPS.

Hummmm... there is this logp.Critical that is not adjusted.. thanks i will change it now

leopucci · 2019-04-02T18:31:03Z

@andrewkroh @odacremolbap today was git branches learning day... recreated all pull requests...
#11611
Sorry for the mess!

odacremolbap · 2019-04-02T18:47:53Z

you are super welcomed, @leopucci
glad to see you try this hard

leopucci · 2019-04-02T21:52:17Z

=)

leopucci marked this pull request as ready for review March 26, 2019 21:02

leopucci requested a review from a team as a code owner March 26, 2019 21:02

ruflin added module review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Mar 27, 2019

andrewkroh requested changes Mar 27, 2019

View reviewed changes

metricbeat/helper/server/http/http.go Outdated Show resolved Hide resolved

metricbeat/helper/server/http/http.go Outdated Show resolved Hide resolved

metricbeat/helper/server/http/http.go Outdated Show resolved Hide resolved

houndci-bot reviewed Mar 27, 2019

View reviewed changes

metricbeat/helper/server/http/http_test.go Outdated Show resolved Hide resolved

andrewkroh reviewed Mar 28, 2019

View reviewed changes

metricbeat/helper/server/http/http.go Outdated Show resolved Hide resolved

metricbeat/helper/server/http/http.go Outdated Show resolved Hide resolved

leopucci requested a review from a team as a code owner March 29, 2019 19:23

odacremolbap suggested changes Mar 29, 2019

View reviewed changes

houndci-bot reviewed Mar 29, 2019

View reviewed changes

metricbeat/helper/server/http/http_test.go Outdated Show resolved Hide resolved

odacremolbap suggested changes Mar 31, 2019

View reviewed changes

odacremolbap reviewed Apr 1, 2019

View reviewed changes

metricbeat/module/http/server/_meta/docs.asciidoc Outdated Show resolved Hide resolved

alvarolobato added the [zube]: In Review label Apr 1, 2019

andrewkroh reviewed Apr 1, 2019

View reviewed changes

andrewkroh approved these changes Apr 1, 2019

View reviewed changes

odacremolbap approved these changes Apr 1, 2019

View reviewed changes

Add TLS support for metric beat http server

6652ea1

Update config.go Update http.go Update config.go Adding ssl tests, adjusting review requests

Update CHANGELOG.next.asciidoc

e53ea4e

Merge branch 'master' into patch-1

ba56779

kaiyan-sheng reviewed Apr 2, 2019

View reviewed changes

leopucci closed this Apr 2, 2019

zube bot added [zube]: Done and removed [zube]: In Review labels Apr 2, 2019

exekias removed the [zube]: Done label Apr 8, 2019

	if h.server.TLSConfig != nil && h.server.TLSConfig.Certificates != nil {
	if h.server.TLSConfig != nil {

Add TLS support for metric beat http server #11482

Add TLS support for metric beat http server #11482

Conversation

leopucci commented Mar 26, 2019 • edited by alvarolobato Loading

elasticmachine commented Mar 26, 2019

leopucci commented Mar 26, 2019

leopucci commented Mar 26, 2019

leopucci commented Mar 26, 2019

leopucci commented Mar 26, 2019

ruflin commented Mar 27, 2019

leopucci commented Mar 27, 2019

ruflin commented Mar 27, 2019

leopucci commented Mar 27, 2019

andrewkroh left a comment

Choose a reason for hiding this comment

leopucci commented Mar 27, 2019

leopucci commented Mar 28, 2019

ruflin commented Mar 29, 2019

leopucci commented Mar 29, 2019

odacremolbap left a comment

Choose a reason for hiding this comment

leopucci commented Mar 30, 2019

odacremolbap commented Mar 31, 2019

leopucci commented Mar 31, 2019

odacremolbap left a comment

Choose a reason for hiding this comment

leopucci commented Mar 31, 2019

odacremolbap commented Apr 1, 2019

leopucci commented Apr 1, 2019

leopucci commented Apr 1, 2019

andrewkroh Apr 1, 2019

Choose a reason for hiding this comment

leopucci Apr 1, 2019 • edited Loading

Choose a reason for hiding this comment

andrewkroh Apr 1, 2019

Choose a reason for hiding this comment

leopucci commented Apr 1, 2019

odacremolbap commented Apr 1, 2019

odacremolbap commented Apr 1, 2019

odacremolbap commented Apr 1, 2019

leopucci commented Apr 1, 2019

odacremolbap commented Apr 1, 2019

leopucci commented Apr 2, 2019

kaiyan-sheng Apr 2, 2019

Choose a reason for hiding this comment

leopucci Apr 2, 2019 • edited Loading

Choose a reason for hiding this comment

leopucci Apr 2, 2019

Choose a reason for hiding this comment

leopucci commented Apr 2, 2019

odacremolbap commented Apr 2, 2019

leopucci commented Apr 2, 2019

leopucci commented Mar 26, 2019 •

edited by alvarolobato

Loading

leopucci Apr 1, 2019 •

edited

Loading

leopucci Apr 2, 2019 •

edited

Loading