feat: Direct Response #3977
feat: Direct Response #3977
Conversation
api/v1alpha1/virtualbackend_types.go
Outdated
| type VirtualBackendSpec struct { | ||
| Body Body `json:"body,omitempty" yaml:"body"` | ||
| StatusCode StatusCode `json:"statusCode,omitempty" yaml:"statusCode"` | ||
| ResponseHeadersToAdd []ResponseHeader `json:"responseHeadersToAdd,omitempty" yaml:"responseHeadersToAdd,omitempty"` |
There was a problem hiding this comment.
-
Maybe call the field
ResponseHeaders? -
Instead of a string array, maybe make this a
map[string]string? -
You might also consider adding CEL validation for the header names. According to RFC 7230:
Historically, HTTP has allowed field content with text in the
ISO-8859-1 charset [ISO-8859-1], supporting other charsets only
through use of [RFC2047] encoding. In practice, most HTTP header
field values use only a subset of the US-ASCII charset [USASCII].
Newly defined header fields SHOULD limit their field values to
US-ASCII octets. A recipient SHOULD treat other octets in field
content (obs-text) as opaque data.
There was a problem hiding this comment.
Maybe call the field ResponseHeaders?
Agree
Instead of a string array, maybe make this a map[string]string?
Hm, maybe make similar to ir.AddHeader?
type AddHeader struct {
Name string `json:"name" yaml:"name"`
Value string `json:"value" yaml:"value"`
Append bool `json:"append" yaml:"append"`
}
You might also consider adding CEL validation for the header names.
Yeah, I'll add validation when PR will be approved conceptually.
| Spec VirtualBackendSpec `json:"spec"` | ||
| } | ||
|
|
||
| type VirtualBackendSpec struct { |
There was a problem hiding this comment.
Add some CEL validations here, and also document what the defaults are if nothing is provided.
For consistency with other EG APIs, if the value is optional:
- It should be a pointer
- The JSON tag should specify "omitempty"
- There should be some documentation around what is used if the value was not specified.
This is especially important for the StatusCode field - there might not be a body but the direct response should always include a StatusCode - it shouldn't be possible to leave this empty.
seems not |
|
Should we add a new CRD to support one feature ? In a long run, will it cause the API maintaince issues ? |
@Xunzhuo Firstly, I thought about it but it will require so many changes and it won't look good by architect.
@Xunzhuo due to direct response gonna be part of Gateway API it's temporary solution for such functionality. But realization in GW API is unpredictable of time, like one guy took responsibility to add and since April no updates. |
|
we'll have to unfortunately have to wait until the API is available in Gateway API |
is there an issue on upstream? |
Signed-off-by: Karandashov Daniil <[email protected]>
… 'Body' to []byte Signed-off-by: Karandashov Daniil <[email protected]>
Signed-off-by: Karandashov Daniil <[email protected]>
Demacr
force-pushed
the
feature/add-direct-response
branch
from
017407e to
564eb48
Compare
| @@ -1212,6 +1212,8 @@ func (h AddHeader) Validate() error { | |||
| // DirectResponse holds the details for returning a body and status code for a route. | |||
| // +k8s:deepcopy-gen=true | |||
| type DirectResponse struct { | |||
| // Body will be used for the direct response's status code. | |||
| Body string `json:"body,omitempty" yaml:"body,omitempty"` | |||
There was a problem hiding this comment.
Body should be a []byte as well. If the direct response is a binary image, converting to a string might mess it up.
|
|
||
| if res.Body != "" { | ||
| directResponseDataSource = &corev3.DataSource{ | ||
| Specifier: &corev3.DataSource_InlineString{ |
There was a problem hiding this comment.
Use DataSource_InlineBytes instead of DataSource_InlineString.
| // +kubebuilder:resource:categories=envoy-gateway,shortName=vb | ||
| // | ||
| // VirtualBackend defines the configuration for direct response. | ||
| type VirtualBackend struct { |
|
ive raised #4120 to discuss how we can move forward for Direct Response, hoping to get some consensus on the issue, before we can move forward with the implementation |
|
@arkodg should we close this, and move to HTTPRouterFilter? |
yeah @zirain that makes sense, closing this PR, @Demacr if you're still interested on adding support for Direct Response, can you raise another PR to add the feature as a field to HTTPRouterFilter |
What this PR does / why we need it:
I decided to add Direct Response due to making endpoint stubs after deleting services. I found that #2714 issue has information that GW API still in development to add direct response and there is a way to workaround by
EnvoyPatchPolicy. So I've implemented new CRD which can be used asbackendRefinHTTPRoute.p.s.: this PR is a draft, I'll add documentation later. Will be glad to discuss about conception of this solution.
Fixed #2714