ext_proc: attributes in first message to server#1
Conversation
…ath, not just with headers messages Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>
| // [#not-implemented-hide:] | ||
| // TODO(jbohanon) reserve field as part of rework detailed in https://github.com/envoyproxy/envoy/issues/32125 | ||
| // The values of properties selected by the ``request_attributes`` | ||
| // or ``response_attributes`` list in the configuration. Each entry | ||
| // in the list is populated | ||
| // from the standard :ref:`attributes <arch_overview_attributes>` | ||
| // supported across Envoy. | ||
| map<string, google.protobuf.Struct> attributes = 2; |
There was a problem hiding this comment.
Please don't remove this directly. Just keep it and mark it as deprecated.
Implementation LGTM overall. but I just do a quick check on the phone, so will check it on PC tomorrow.
There was a problem hiding this comment.
Given that it was not implemented, marked as not-implemented-hide, what is the risk in marking it reserved? Marking it deprecated and having it do nothing is more misleading than removing it in my opinion.
There was a problem hiding this comment.
Although specific to this field, I agree with you, there should be no risk and removing make the API more clean.
But our API policy tell:
Deprecations will still occur as an end-user indication that there is a preferred way to configure a particular feature, but no field will ever be removed nor will Envoy ever remove the implementation for any deprecated field.
The policy is desigend for common cases, it may not always be the best choice in every special case. But rule is the rule.
So, ...
There was a problem hiding this comment.
Sounds good. I went ahead and added it back with the [#not-implemented-hide:] and marked it deprecated.
Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>
|
@rshriram can you PTAL here and let me know if I should include this on the main PR? |
wbpcode
left a comment
wbpcode
left a comment
There was a problem hiding this comment.
LGTM overall. But you may need to update the tests to ensure:
- attributes are sent as expected with correct position and content.
- attributes are only sent once as expected.
| // This field is deprecated and not implemented. Attributes will be sent in | ||
| // the top-level :ref:`attributes <envoy_v3_api_field_service.ext_proc.v3.ProcessingRequest.attributes` | ||
| // field. | ||
| map<string, google.protobuf.Struct> attributes = 2; |
There was a problem hiding this comment.
Add the deprecated annotation to this field [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"]
|
|
||
| void sentAttributes(bool sent) { attributes_sent_ = sent; } |
There was a problem hiding this comment.
nit: setSentAttributes. But current style is also OK to me.
| return !attributes_sent_ && mgr.hasResponseExpr(); | ||
| } | ||
|
|
||
| const Http::RequestOrResponseHeaderMap* requestHeaders() const override { return nullptr; }; |
There was a problem hiding this comment.
I believe the request headers should be available in the response phase. At least you can add a TODO comment here and optimize it in the future.
There was a problem hiding this comment.
This is a good catch. I think it would be prudent to have separate fields in the base class for request and response header maps to accomplish this, even though on decoding it will always be nullptr for the response headers.
| bool end_stream) { | ||
| ENVOY_LOG(debug, "Sending a body chunk of {} bytes, end_stream {}", data.length(), end_stream); | ||
| ProcessingRequest req; | ||
| addAttributes(state, req); |
There was a problem hiding this comment.
Dont we need the same for the response bodies as well? or is this function called in the response body path as well?
There was a problem hiding this comment.
This is called from the onData helper method which is called on both decode and encode
|
@rshriram This special case result in special API. And it's not the only one in the Envoy. Here is another similar case envoyproxy#31507 I personally inclined to handle this API in same way with envoyproxy#31507. If you do want to remove it, we could discuss it as a sperated issue. |
|
Since we are on a time crunch for the implementation change, I am going to merge this PR with the API-only PR (envoyproxy#32176) and we can continue discussion over there. |
Signed-off-by: Jacob Bohanon <jacob.bohanon@solo.io>
3 participants
Commit Message: Send attributes with the first message to the server on the request and response paths. Previously the attributes were just sent with headers processing requests
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]