Add Content-Type directive to body factory .body_factory_info#12947
Add Content-Type directive to body factory .body_factory_info#12947bryancall wants to merge 3 commits intoapache:masterfrom
Conversation
The body factory previously only supported Content-Language and Content-Charset directives in .body_factory_info. This adds a Content-Type directive so operators can control the MIME type of error responses (e.g. text/plain instead of the default text/html). Also fixes HttpSM::setup_internal_transfer which was unconditionally overwriting Content-Type to text/html, masking any type set by the body factory. Now uses the presence bit check to only set the default when Content-Type is not already present in the response.
The :file: role with nitpicky mode causes an unresolved reference warning for .body_factory_info. Use literal markup instead.
There was a problem hiding this comment.
Pull request overview
This PR extends Apache Traffic Server’s body factory error-page customization to support a configurable Content-Type via .body_factory_info, and prevents HttpSM from overwriting an already-set Content-Type during internal transfers.
Changes:
- Add parsing/storage of a new
Content-Typedirective in.body_factory_infoand use it when generating body-factory responses. - Update
HttpSM::setup_internal_transfer()to only apply thetext/htmldefault whenContent-Typeis not already present. - Add documentation and a gold test covering both default and custom
Content-Typebehavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/gold_tests/body_factory/body_factory_content_type.test.py | New autest validating default (text/html) vs custom (text/plain) body-factory Content-Type. |
| src/proxy/http/HttpSM.cc | Avoids unconditional Content-Type: text/html overwrite by checking the presence bit. |
| src/proxy/http/HttpBodyFactory.cc | Parses Content-Type from .body_factory_info and incorporates it into generated response headers. |
| include/proxy/http/HttpBodyFactory.h | Extends internal fabricate() signature and body set metadata to include content_type. |
| doc/admin-guide/monitoring/error-messages.en.rst | Documents .body_factory_info directives, including the new Content-Type. |
| configs/body_factory/default/.body_factory_info | Updates shipped template metadata comments to include the new directive. |
You can also share your feedback on Copilot code review. Take the survey.
- Rename type_ptr to content_type_ptr to avoid confusion with the type parameter (error template name) - Fix docs: .body_factory_info is required for a template set to load, not optional. Absent file causes the directory to be skipped. - Use literal markup for .body_factory_info to fix Sphinx nitpick warning
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
| const char *mime_type = content_type_ptr ? content_type_ptr : "text/html"; | ||
| snprintf(content_type_out_buf, content_type_buf_size, "%s; charset=%s", mime_type, charset_ptr); |
There was a problem hiding this comment.
Content-Type from .body_factory_info is appended with ; charset=... unconditionally. If an operator configures Content-Type with parameters (e.g. application/json; charset=utf-16), this will generate an invalid header with duplicate charset parameters. Consider either (a) treating .body_factory_info's Content-Type as the full header value and not appending charset, or (b) parsing/stripping parameters (or at least detecting an existing charset=) before appending.
| tr.Processes.Default.Streams.stdout += Testers.ContainsExpression( | ||
| 'Content-Type: text/html; charset=utf-8', 'Default body factory should produce text/html with charset') |
There was a problem hiding this comment.
This assertion is potentially brittle if the server emits a different charset casing (e.g. UTF-8) or slightly different formatting/spacing. If ContainsExpression is regex-based, consider using a case-insensitive pattern and/or a more flexible match for the charset portion to reduce test flakiness across platforms/builds.
|
|
||
| To describe Korean error pages encoded in the ``iso-2022-kr`` character set:: | ||
|
|
||
| Content-Language: kr |
There was a problem hiding this comment.
Content-Language: kr is not a standard language tag for Korean (commonly ko / ko-KR per BCP 47). Since this section is newly documenting metadata directives, consider updating the example to use a standard tag to avoid propagating incorrect configuration patterns.
| Content-Language: kr | |
| Content-Language: ko-KR |
Problem
Body factory error responses are always served with
Content-Type: text/htmland there is no way to change this. The.body_factory_infometadata file supportsContent-LanguageandContent-Charsetdirectives but has noContent-Typedirective. Operators who serve non-HTML error pages (plain text, JSON, etc.) have no mechanism to set the correct MIME type.Additionally,
HttpSM::setup_internal_transferunconditionally overwritesContent-Typetotext/htmlwheninternal_msg_buffer_typeis NULL, which would mask any type set earlier in the response pipeline.Summary
Add
Content-Typedirective to.body_factory_info-- Operators can now set the MIME type for body factory error responses (e.g.Content-Type: text/plain) instead of the hardcodedtext/htmldefault. The directive is parsed alongside the existingContent-LanguageandContent-Charsetdirectives.Fix Content-Type overwrite in
HttpSM::setup_internal_transfer-- Now uses theMIME_PRESENCE_CONTENT_TYPEpresence bit to only apply thetext/htmldefault when Content-Type is not already present in the response.Add documentation for
.body_factory_infometadata file -- The admin guide's error messages page now documents all three supported directives (Content-Language,Content-Charset,Content-Type) with examples.Add autest -- New
body_factory_content_type.test.pyverifies both default (text/html) and custom (text/plain) Content-Type behavior.Testing
Fixes: #10893