Skip to content

http: respecting configured added content encoding on local replies#20460

Merged
alyssawilk merged 2 commits intoenvoyproxy:mainfrom
alyssawilk:to_add
Mar 28, 2022
Merged

http: respecting configured added content encoding on local replies#20460
alyssawilk merged 2 commits intoenvoyproxy:mainfrom
alyssawilk:to_add

Conversation

@alyssawilk
Copy link
Copy Markdown
Contributor

Risk Level: Low
Testing: integration test
Docs Changes: n/a
Release Notes: inline
Runtime guard: yes
Fixes #20267

@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #20460 was opened by alyssawilk.

see: more, trace.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This part is hard to understand, "modify_headers_" function is used to modify response headers from local_reply_config. If it has "content_type" headers to add, response_headers should have content type, so it should be"

has_custom_content_type = (response_headers->ContentType() != nullptr; )

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oh yeah this was draft because it wasn't ready for review :-) I'd just had 11 unrelated tests failing locally and wanted to make sure it was my machine, not local reply changes :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This may not be correct.

How about applying "modify_headers" function after

response_headers->setReferenceContentType

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Which case is not correct?
I'm a bit wary of changing the body and then changing the headers.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was worrying response_headers may already has content_type before "modify_headers_" is called. Should we override it?

In reality, it seems we should not have the case. sendLocalReply() usually starts with a empty response_headers. it should not have content_type. Only "modify_headers_" call may add it.

So it is fine then.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think it's a valid point - tweaked things to that effect.

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
qiwzhang
qiwzhang previously approved these changes Mar 23, 2022
@alyssawilk alyssawilk marked this pull request as ready for review March 24, 2022 13:11
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk
Copy link
Copy Markdown
Contributor Author

sorted conflicts. @snowp PTAL?

Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@alyssawilk alyssawilk enabled auto-merge (squash) March 28, 2022 20:57
@alyssawilk alyssawilk merged commit 80c7e5b into envoyproxy:main Mar 28, 2022
ravenblackx pushed a commit to ravenblackx/envoy that referenced this pull request Jun 8, 2022
Risk Level: Low
Testing: integration test
Docs Changes: n/a
Release Notes: inline
Runtime guard: yes
Fixes envoyproxy#20267

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
@alyssawilk alyssawilk deleted the to_add branch August 4, 2022 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Local reply modification can't overwrite content-type header

3 participants