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

Prometheus web doesn't respond on GET / HTTP/1.0 #3909

Closed
angry-cellophane opened this Issue Mar 4, 2018 · 3 comments

Comments

Projects
None yet
4 participants
@angry-cellophane
Copy link

angry-cellophane commented Mar 4, 2018

What did you do?
Let's say prometheus-web is running on localhost:9090

Send an http get request with no headers, version 1.0 to the root path.
e.g.

telnet localhost 9090
GET / HTTP/1.0<eol>
<eol>

where is is the end of line character

What did you expect to see?
an http response from prometheus

❯ telnet localhost 9090
Trying ::1...
Connected to localhost.
Escape character is '^]'.
GET / HTTP/1.0

HTTP/1.0 302 Found
Location: /graph
Date: Sun, 04 Mar 2018 21:17:41 GMT
Content-Length: 29
Content-Type: text/html; charset=utf-8

<a href="/graph">Found</a>.

Connection closed by foreign host.

What did you see instead? Under which circumstances?
no response, prometheus is waiting for more data from the client.

Originally, I found this issue when upgraded prometheus from version 1.7.1 to 2.1.0. I run prometheus in dcos. Dcos uses haproxy to load balance apps. HAProxy's default healthcheck doesn't work for prometheus 2.1.0 but it works for 1.7.1. The default healthcheck is GET / HTTP/1.0 with no headers.
If I send any http header prometheus sends the response immediately.
e.g.

❯ telnet localhost 9090
Trying ::1...
Connected to localhost.
Escape character is '^]'.
GET / HTTP/1.0
adsd: as

HTTP/1.0 302 Found
Location: /graph
Date: Sun, 04 Mar 2018 21:24:57 GMT
Content-Length: 29
Content-Type: text/html; charset=utf-8

<a href="/graph">Found</a>.

Connection closed by foreign host.

The issue with HAProxy was resolved by defining a custom health check with additional headers.

It looks like a regression bug.

Environment

  • System information:
    ❯ uname -srm
    Linux 4.10.10-200.fc25.x86_64 x86_64

  • Prometheus version:

❯ ./prometheus --version
prometheus, version 2.1.0 (branch: HEAD, revision: 85f23d8)
build user: root@6e784304d3ff
build date: 20180119-12:01:23
go version: go1.9.2

@simonpasquier

This comment has been minimized.

Copy link
Member

simonpasquier commented Mar 5, 2018

This is because Prometheus uses cockroach's cmux behind the scenes to serve gRPC and HTTP on the same listener. Not sure that there is nothing much to do about it.

@angry-cellophane

This comment has been minimized.

Copy link
Author

angry-cellophane commented Mar 5, 2018

thank you, @simonpasquier. Your comment helped me to find the cause of the issue and a workaround.
The solution is pretty simple but the issue is not so I need your opinions on this, guys.

Why it happens:

  1. Open https://github.com/prometheus/prometheus/blob/v2.1.0/web/web.go
  2. Go to line 398
    Two matchers are declared at this point.
	var (
		m       = cmux.New(listener)
		grpcl   = m.Match(cmux.HTTP2HeaderField("content-type", "application/grpc"))
		httpl   = m.Match(cmux.HTTP1Fast())
		grpcSrv = grpc.NewServer()
	)
  1. Let's look at HTTP2HeaderField's implementation:
    https://github.com/cockroachdb/cmux/blob/master/matchers.go
func HTTP2HeaderField(name, value string) Matcher {
	return func(r io.Reader) bool {
		return matchHTTP2Field(r, name, value)
	}
}


func matchHTTP2Field(r io.Reader, name, value string) (matched bool) {
	if !hasHTTP2Preface(r) {
		return false
	}
        //...
}

func hasHTTP2Preface(r io.Reader) bool {
	var b [len(http2.ClientPreface)]byte
	if _, err := io.ReadFull(r, b[:]); err != nil {
		return false
	}

	return string(b[:]) == http2.ClientPreface
}

So at this point the http2 matcher expects a certain amount of bytes to be passed. It doesn't conflict with the http2 spec - https://http2.github.io/http2-spec/#known-http
The problem is how cmux processes incoming requests: it iterates over a list of matchers and checks each one. Hence, if an http2 matcher goes first request processing blocks until it get the amount of bytes required by http2.

If you swap the matchers GET / HTTP/1.0 works fine, it's not blocked anymore.
I checked it with that simple example:

package main

import (
	"log"
	"net"
	"net/http"
	"github.com/cockroachdb/cmux"
	"google.golang.org/grpc"
)

func main() {
	listener, err := net.Listen("tcp", ":8080")
	if err != nil {
		log.Fatal(err)
	}

	m := cmux.New(listener)

	httpL := m.Match(cmux.HTTP1Fast())
	grpcl := m.Match(cmux.HTTP2HeaderField("content-type", "application/grpc"))

	http.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
		http.Redirect(w, r, "/graph", http.StatusFound)
	})

	httpSrv := &http.Server{}
	go httpSrv.Serve(httpL)

	grpcSrv := grpc.NewServer()
	go grpcSrv.Serve(grpcl)

	m.Serve()
}

Cmux has its own limitation and for me this issue looks like misuse of cmux rather than a bug in the lib.

What do you think?

@tkausl

This comment has been minimized.

Copy link

tkausl commented Jun 25, 2018

The other cmux repository seems to handle this better, see https://github.com/soheilhy/cmux/blob/master/matchers.go#L190

simonpasquier added a commit to simonpasquier/prometheus that referenced this issue Mar 15, 2019

*: bump gRPC and protobuf dependencies
The goal is to remove almost all references to the
golang.org/x/net/context package.

github.com/gogo/protobuf => v1.2.1
google.golang.org/grpc => v1.19.0
github.com/grpc-ecosystem/grpc-gateway => v1.18.5

It also replaces github.com/cockroachdb/cmux by github.com/soheilhy/cmux
because of [1] which fixes prometheus#3909 incidentally.

[1] grpc/grpc-go#2636

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier added a commit to simonpasquier/prometheus that referenced this issue Mar 21, 2019

*: bump gRPC and protobuf dependencies
The goal is to remove almost all references to the
golang.org/x/net/context package.

github.com/gogo/protobuf => v1.2.1
google.golang.org/grpc => v1.19.1
github.com/grpc-ecosystem/grpc-gateway => v1.18.5

It also replaces github.com/cockroachdb/cmux by github.com/soheilhy/cmux
because of [1] which fixes prometheus#3909 incidentally.

[1] grpc/grpc-go#2636

Signed-off-by: Simon Pasquier <spasquie@redhat.com>

simonpasquier added a commit that referenced this issue Apr 4, 2019

*: bump gRPC and protobuf dependencies (#5367)
The goal is to remove almost all references to the
golang.org/x/net/context package.

github.com/gogo/protobuf => v1.2.1
google.golang.org/grpc => v1.19.1
github.com/grpc-ecosystem/grpc-gateway => v1.18.5

It also replaces github.com/cockroachdb/cmux by github.com/soheilhy/cmux
because of [1] which fixes #3909 incidentally.

[1] grpc/grpc-go#2636

Signed-off-by: Simon Pasquier <spasquie@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.