Fixing ResponseWriter to assert to http.Flusher in /sys/monitor endpoint #13200

hghaf099 · 2021-11-18T07:48:33Z

There are a number of interfaces that implement http.ResponseWriter. For that, wrapping http.ResponseWriter needs a lot of care.
StatusHeaderResponseWriter is instantiated in the http layer which wraps http.ResponseWriter. Then, HTTPResponseWriter wraps the already wrapped ResponseWriter. This causes issues as for example in the sys/monitor, the http.Flusher cannot be asserted and calling the endpoint returns stream not supported

…ResponseWriter issues

ncabatoff · 2021-11-18T15:47:25Z

http/handler.go

-					statusCode:  200,
-					headers:     listenerCustomHeaders.StatusCodeHeaderMap,
-				}
+				nw.SetHeaders(listenerCustomHeaders.StatusCodeHeaderMap)


Why introduce a method for this instead of just making it an argument to the constructor?

The new responseWriter nw needs to be instantiated even if the props.ListenerConfig is nil.

Sorry, I'm not following. What would break if we got rid of SetHeaders and instead assigned listenerCustomHeaders.StatusCodeHeaderMap to a local var here? Then passed that var as a 2nd arg to logical.NewStatusHeaderResponseWriter?

My primary goal here is to minimize mutable state in the type.

ncabatoff · 2021-11-18T15:53:53Z

http/logical.go

@@ -199,7 +199,7 @@ func buildLogicalRequestNoAuth(perfStandby bool, w http.ResponseWriter, r *http.
 		req.HTTPRequest = r
 	}
 	if responseWriter != nil {
-		req.ResponseWriter = logical.NewHTTPResponseWriter(responseWriter)
+		req.ResponseWriter = logical.NewStatusHeaderResponseWriter(responseWriter)


Why do we need to re-wrap? Why can't we just assign responseWriter directly?

So, responseWriter is of type http.ResponseWriter. if I assign that directly, then, where ever we would need extra functionality like ResponseWriter.Written(), we would need to assert the WrappingResponseWriter interface first. Although there are not many places right now for such functionality, it is easy to miss fixing those. It is not just the Written() function as well.

Is it though? I thought everything was wrapped by wrapGenericHandler, and it's always passing down a response writer of the new type.

hmm, I think so? The function signature of this function defines w http.ResponseWriter which is assigned to responseWriter. But, another thing is that of which type should the req.ResponseWriter be defined? should it be http.ResponseWriter, or logical.StatusHeaderResponseWriter?

I wasn't speaking in terms of function signature, I'm saying that since wrapGenericHandler is wrapping all request handlers, we shouldn't need to wrap the response handler again. And if we don't need to handle re-wrapping I think some of the code will be simplified.

I think we should probably stick with http.ResponseWriter, but that's mostly because I expect trying to switch it would cause problems. If it's possible to use StatusHeaderResponseWriter without it being really disruptive, that could make sense.

Sounds good! I will stick with http.ResponseWriter.

ncabatoff · 2021-11-18T15:56:01Z

sdk/logical/response.go

+	Wrapped() http.ResponseWriter
+	SetHeaders(h map[string][]*CustomHeader)
+	GetHeaders() map[string][]*CustomHeader
+	StatusCode() int


Is this actually used?

I found the issue in this PR while I was working at the logging requests feature. So, this is not needed now, but it is needed then.

You are right, this is not needed. Fixing it in both PRs. Thanks!

ncabatoff

Looking very close!

ncabatoff · 2021-11-23T14:02:44Z

sdk/logical/request.go

@@ -203,7 +203,7 @@ type Request struct {

 	// ResponseWriter if set can be used to stream a response value to the http
 	// request that generated this logical.Request object.
-	ResponseWriter *HTTPResponseWriter `json:"-" sentinel:""`
+	ResponseWriter *http.ResponseWriter `json:"-" sentinel:""`


I'd rather use the explicit type here than the interface, since there aren't any cases where we'll be using anything other than that type.

ncabatoff · 2021-11-23T14:05:52Z

vault/custom_response_headers_test.go

@@ -127,8 +128,9 @@ func TestCustomResponseHeadersConfigInteractUiConfig(t *testing.T) {
 		t.Fatalf("custom header config should be configured in core")
 	}

-	w := httptest.NewRecorder()
-	hw := logical.NewHTTPResponseWriter(w)
+	hw := func(w http.ResponseWriter) *http.ResponseWriter {


This is a rather complicated way to avoid defining a variable.

ncabatoff · 2021-11-23T14:15:41Z

http/handler.go

-					statusCode:  200,
-					headers:     listenerCustomHeaders.StatusCodeHeaderMap,
-				}
+				nw.SetHeaders(listenerCustomHeaders.StatusCodeHeaderMap)


Sorry, I'm not following. What would break if we got rid of SetHeaders and instead assigned listenerCustomHeaders.StatusCodeHeaderMap to a local var here? Then passed that var as a 2nd arg to logical.NewStatusHeaderResponseWriter?

My primary goal here is to minimize mutable state in the type.

ncabatoff · 2021-11-23T14:27:14Z

sdk/logical/response.go

+	written     *uint32
+}
+
+func NewStatusHeaderResponseWriter (w http.ResponseWriter) *StatusHeaderResponseWriter {


Suggested change

func NewStatusHeaderResponseWriter (w http.ResponseWriter) *StatusHeaderResponseWriter {

func NewStatusHeaderResponseWriter(w http.ResponseWriter) *StatusHeaderResponseWriter {

* Unify NewHTTPResponseWriter ant NewStatusHeaderResponseWriter to fix ResponseWriter issues * adding changelog * removing unnecessary function from the WrappingResponseWriter interface * changing logical requests responseWriter type * reverting change to HTTPResponseWriter

…ys/monitor endpoint (#13200) (#13260) * Unify HTTPResponseWriter and StatusHeaderResponseWriter (#13200) * Unify NewHTTPResponseWriter ant NewStatusHeaderResponseWriter to fix ResponseWriter issues * adding changelog * removing unnecessary function from the WrappingResponseWriter interface * changing logical requests responseWriter type * reverting change to HTTPResponseWriter * Update changelog/13200.txt * Update changelog/13200.txt

) * Unify NewHTTPResponseWriter ant NewStatusHeaderResponseWriter to fix ResponseWriter issues * adding changelog * removing unnecessary function from the WrappingResponseWriter interface * changing logical requests responseWriter type * reverting change to HTTPResponseWriter

Unify NewHTTPResponseWriter ant NewStatusHeaderResponseWriter to fix …

92b3e95

…ResponseWriter issues

hghaf099 changed the title ~~Unify NewHTTPResponseWriter ant NewStatusHeaderResponseWriter to fix …~~ Unify HTTPResponseWriter and StatusHeaderResponseWriter Nov 18, 2021

adding changelog

edb878a

vercel bot temporarily deployed to Preview – vault November 18, 2021 07:57 Inactive

vercel bot temporarily deployed to Preview – vault-storybook November 18, 2021 07:57 Inactive

hghaf099 mentioned this pull request Nov 18, 2021

VAULT-1564 report in-flight requests #13024

Merged

ncabatoff reviewed Nov 18, 2021

View reviewed changes

removing unnecessary function from the WrappingResponseWriter interface

89a5b47

vercel bot temporarily deployed to Preview – vault November 19, 2021 16:03 Inactive

vercel bot temporarily deployed to Preview – vault-storybook November 19, 2021 16:03 Inactive

changing logical requests responseWriter type

6ea1b90

vercel bot temporarily deployed to Preview – vault-storybook November 22, 2021 22:30 Inactive

vercel bot temporarily deployed to Preview – vault November 22, 2021 22:30 Inactive

ncabatoff reviewed Nov 23, 2021

View reviewed changes

reverting change to HTTPResponseWriter

b26a483

vercel bot temporarily deployed to Preview – vault November 23, 2021 18:30 Inactive

vercel bot temporarily deployed to Preview – vault-storybook November 23, 2021 18:30 Inactive

ncabatoff approved these changes Nov 23, 2021

View reviewed changes

hghaf099 marked this pull request as ready for review November 23, 2021 19:18

hghaf099 added the backport label Nov 23, 2021

hghaf099 merged commit 258193c into main Nov 23, 2021

hghaf099 added this to the 1.9.1 milestone Nov 23, 2021

hghaf099 changed the title ~~Unify HTTPResponseWriter and StatusHeaderResponseWriter~~ Fixing ResponseWriter to assert to http.Flusher in /sys/monitor endpoint Nov 23, 2021

thiskevinwang mentioned this pull request Apr 25, 2022

fix release/1.9.x deploy previews #15157

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing ResponseWriter to assert to http.Flusher in /sys/monitor endpoint #13200

Fixing ResponseWriter to assert to http.Flusher in /sys/monitor endpoint #13200

hghaf099 commented Nov 18, 2021 •

edited

ncabatoff Nov 18, 2021

hghaf099 Nov 18, 2021

ncabatoff Nov 23, 2021

ncabatoff Nov 18, 2021

hghaf099 Nov 18, 2021

ncabatoff Nov 18, 2021

hghaf099 Nov 18, 2021

ncabatoff Nov 22, 2021

hghaf099 Nov 22, 2021

ncabatoff Nov 18, 2021

hghaf099 Nov 18, 2021

hghaf099 Nov 18, 2021

ncabatoff left a comment

ncabatoff Nov 23, 2021

ncabatoff Nov 23, 2021

ncabatoff Nov 23, 2021

ncabatoff Nov 23, 2021

	func NewStatusHeaderResponseWriter (w http.ResponseWriter) *StatusHeaderResponseWriter {
	func NewStatusHeaderResponseWriter(w http.ResponseWriter) *StatusHeaderResponseWriter {

Fixing ResponseWriter to assert to http.Flusher in /sys/monitor endpoint #13200

Fixing ResponseWriter to assert to http.Flusher in /sys/monitor endpoint #13200

Conversation

hghaf099 commented Nov 18, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncabatoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hghaf099 commented Nov 18, 2021 •

edited