Ensure the services status page renders as HTML #1265

simonswine · 2022-02-22T13:58:07Z

What this PR does

This removes the text/plain content type header from the services page

Which issue(s) this PR fixes or relates to

This ensures the services page of Mimir renders as HTML. I have noticed that when testing the v2.0.0-rc.0

Checklist

~~[ ] Tests updated~~
~~[ ] Documentation added~~
~~[ ] CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]~~

56quarters · 2022-02-22T14:00:04Z

What content type gets set instead? Or is the browser just guessing?

osg-grafana · 2022-02-22T14:16:47Z

In "https://github.com/grafana/mimir/blob/204a2a316708dba2e7f70736e65bc1241a82fabd/pkg/mimir/status.go#L22"
"Services Status", is it singular or plural. This needs to be sentence case, but could be written three ways:

Service’s status [only seeing status information about one service]
Services’ status [seeing status information about more than one service]
Service status [I would not recommend this version, because "Service" can look like a verb or a noun]

simonswine · 2022-02-23T10:18:30Z

Services’ status [seeing status information about more than one service]

This is the one

What content type gets set instead? Or is the browser just guessing?

The browser is guessing. There is also a chance that JSON is requested and returned. But I think text/plain is just always wrong.

pstibrany · 2022-02-23T12:06:56Z

Oh wait, we should not let browser guess. I thought we set the content type in util.RenderHTTPResponse. If we don't, can we do that? (RenderHTTPResponse actually sets application/json for JSON, but doesn't set text/html when writing HTML. If it cannot just set text/html, perhaps it needs extra parameter with correct content type in the case it's not rendering json.)

> Services’ status [seeing status information about more than one service]

simonswine · 2022-03-07T18:52:55Z

Oh wait, we should not let browser guess. I thought we set the content type in util.RenderHTTPResponse. If we don't, can we do that? (RenderHTTPResponse actually sets application/json for JSON, but doesn't set text/html when writing HTML. If it cannot just set text/html, perhaps it needs extra parameter with correct content type in the case it's not rendering json.)

Looking another time at this: I think the browser never guessed. If nothing is set something in our stack seems to set it to text/html; charset=utf-8. I have also observed that the json content-type never worked, as we call WriteHeader(200)
before we overwrite it.

So my proposed change is to remove WriteHeader(200), because that happens anyhow, when we first Write.

Looking at the content-types before this PR:

# HTML
$ curl -s -D - -o /dev/null http://127.0.0.1:8080/services | grep -i content-type:
Content-Type: text/html; charset=utf-8

# JSON
$ curl -s -H "accept: application/json" -D - -o /dev/null http://127.0.0.1:8080/services | grep -i content-type:                                                                                                                                                                                                                                                                      
Content-Type: text/plain; charset=utf-8

# This is how my firefox browser requests the page
$ curl 'http://127.0.0.1:8080/services' \
  -H 'Connection: keep-alive' \
  -H 'Cache-Control: max-age=0' \
  -H 'sec-ch-ua: " Not A;Brand";v="99", "Chromium";v="98"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "Linux"' \
  -H 'Upgrade-Insecure-Requests: 1' \
  -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/98.0.4758.102 Safari/537.36' \
  -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9' \
  -H 'Sec-Fetch-Site: same-origin' \
  -H 'Sec-Fetch-Mode: navigate' \
  -H 'Sec-Fetch-User: ?1' \
  -H 'Sec-Fetch-Dest: document' \
  -H 'Referer: http://127.0.0.1:8080/' \
  -H 'Accept-Language: en-US,en;q=0.9,de;q=0.8' \
  --compressed \
  -D - \
  -s \
  -o /dev/null | grep -i content-type
Content-Type: text/plain

And after:

# HTML
$ curl -s -D - -o /dev/null http://127.0.0.1:8080/services | grep -i content-type:
Content-Type: text/html; utf=8

# JSON
$ curl -s -H "accept: application/json" -D - -o /dev/null http://127.0.0.1:8080/services | grep -i content-type:
Content-Type: application/json

# This is how my firefox browser requests the page
$ curl 'http://127.0.0.1:8080/services' \                                                                                                                                                                                                                                                                                                                                               
  -H 'Connection: keep-alive' \
  -H 'Cache-Control: max-age=0' \
  -H 'sec-ch-ua: " Not A;Brand";v="99", "Chromium";v="98"' \
  -H 'sec-ch-ua-mobile: ?0' \
  -H 'sec-ch-ua-platform: "Linux"' \
  -H 'Upgrade-Insecure-Requests: 1' \
  -H 'User-Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/98.0.4758.102 Safari/537.36' \
  -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.9' \
  -H 'Sec-Fetch-Site: same-origin' \
  -H 'Sec-Fetch-Mode: navigate' \
  -H 'Sec-Fetch-User: ?1' \
  -H 'Sec-Fetch-Dest: document' \
  -H 'Referer: http://127.0.0.1:8080/' \
  -H 'Accept-Language: en-US,en;q=0.9,de;q=0.8' \
  --compressed \
  -D - \
  -s \
  -o /dev/null | grep -i content-type                                                                                                                                                                                                                                                                                                                                                            
Content-Type: text/html; utf=8

This seems to fix all of those cases.

colega · 2022-03-18T13:49:26Z

pkg/mimir/status.go

-	w.Header().Set("Content-Type", "text/plain")
+	w.Header().Set("Content-Type", "text/html; utf=8")


util.RenderHTTPResponse will try to set the content-type to application/json if that was requested, but we've already sent a content-type here. Why do we need this content-type setting at all?

RenderHTTPResponse only sets JSON content type. I think we should pass "non-JSON" content type to it, so that RenderHTTPResponse is always responsible for setting it.

I just checked the response for the /store-gateway/tenants, for example, which does not explicitly write the header, and the response contains the text/html content-type:

curl -I -XGET localhost:8080/store-gateway/tenants HTTP/1.1 200 OK Vary: Accept-Encoding Date: Tue, 22 Mar 2022 09:22:40 GMT Content-Type: text/html; charset=utf-8 Transfer-Encoding: chunked

So there's some kind of magic happening somewhere, I can recall that Golang detected the content type based on what the output looks like.

Yes, see doc for Write method of ResponseWriter: https://pkg.go.dev/net/http#ResponseWriter

If the Header does not contain a Content-Type line, Write adds a Content-Type set to the result of passing the initial 512 bytes of written data to DetectContentType.

I don't think we should rely on it.

I think it's happening here:

https://github.com/golang/go/blob/86c8075675ed74c1f404894242c26b99800f1639/src/net/http/server.go#L1408

I don't think we should rely on it.

I think it's not critical and we can expect it to work properly when we're sending HTML, but if not, sure, we can always specify it explicitly.

Anyway, my main comment was that we shouldn't set the content type here prematurely.

Anyway, my main comment was that we shouldn't set the content type here prematurely.

Agree.

colega · 2022-03-18T13:49:48Z

pkg/api/api.go

@@ -396,7 +396,7 @@ func (a *API) RegisterQueryScheduler(f *scheduler.Scheduler) {
 // TODO: Refactor this code to be accomplished using the services.ServiceManager
 // or a future module manager #2291
 func (a *API) RegisterServiceMapHandler(handler http.Handler) {
-	a.indexPage.AddLink(SectionAdminEndpoints, "/services", "Service Status")
+	a.indexPage.AddLink(SectionAdminEndpoints, "/services", "Service's status")


This needs to be rebased.

colega · 2022-03-30T15:55:07Z

I started from scratch here: #1575
I'll address the page title in a separate PR once #1572 is merged.

I think we can close this.

As Ursula pointed out, this page is listing the status information about more than one service, so this is the correct spelling. See: #1265 (comment) Signed-off-by: Oleg Zaytsev <[email protected]>

* Fix "Services' status" spelling As Ursula pointed out, this page is listing the status information about more than one service, so this is the correct spelling. See: #1265 (comment) Signed-off-by: Oleg Zaytsev <[email protected]> * Also fix the section header Signed-off-by: Oleg Zaytsev <[email protected]>

simonswine added bug Something isn't working type/bug labels Feb 22, 2022

pstibrany approved these changes Feb 22, 2022

View reviewed changes

osg-grafana approved these changes Feb 23, 2022

View reviewed changes

simonswine added 3 commits March 7, 2022 18:34

Ensure the services status page renders as HTML

c63e8a7

Fix spelling of services' status

0f6dfd6

> Services’ status [seeing status information about more than one service]

Set text/html type explicitly and do not call WriteHeader

f8588ac

simonswine force-pushed the 20220222_fix-content-type-status branch from 3b56223 to f8588ac Compare March 7, 2022 18:40

simonswine requested review from pstibrany and pracucci March 7, 2022 18:53

colega reviewed Mar 18, 2022

View reviewed changes

colega mentioned this pull request Mar 30, 2022

Fix "Services' status" page content type #1575

Merged

3 tasks

pracucci closed this Mar 31, 2022

colega deleted the 20220222_fix-content-type-status branch March 31, 2022 08:52

colega mentioned this pull request Mar 31, 2022

Fix "Services' status" spelling #1580

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure the services status page renders as HTML #1265

Ensure the services status page renders as HTML #1265

simonswine commented Feb 22, 2022

56quarters commented Feb 22, 2022

osg-grafana commented Feb 22, 2022

simonswine commented Feb 23, 2022

pstibrany commented Feb 23, 2022 •

edited

Loading

simonswine commented Mar 7, 2022

colega Mar 18, 2022 •

edited

Loading

pstibrany Mar 22, 2022

colega Mar 22, 2022

pstibrany Mar 22, 2022

colega Mar 22, 2022

colega Mar 22, 2022

pstibrany Mar 22, 2022

colega Mar 18, 2022

colega commented Mar 30, 2022

		w.Header().Set("Content-Type", "text/plain")
		w.Header().Set("Content-Type", "text/html; utf=8")

Ensure the services status page renders as HTML #1265

Ensure the services status page renders as HTML #1265

Conversation

simonswine commented Feb 22, 2022

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

56quarters commented Feb 22, 2022

osg-grafana commented Feb 22, 2022

simonswine commented Feb 23, 2022

pstibrany commented Feb 23, 2022 • edited Loading

simonswine commented Mar 7, 2022

colega Mar 18, 2022 • edited Loading

Choose a reason for hiding this comment

pstibrany Mar 22, 2022

Choose a reason for hiding this comment

colega Mar 22, 2022

Choose a reason for hiding this comment

pstibrany Mar 22, 2022

Choose a reason for hiding this comment

colega Mar 22, 2022

Choose a reason for hiding this comment

colega Mar 22, 2022

Choose a reason for hiding this comment

pstibrany Mar 22, 2022

Choose a reason for hiding this comment

colega Mar 18, 2022

Choose a reason for hiding this comment

colega commented Mar 30, 2022

pstibrany commented Feb 23, 2022 •

edited

Loading

colega Mar 18, 2022 •

edited

Loading