ext_proc: revise service api for attributes

[jbohanon]

Commit Message: Revise service API to reflect changes discussed in #32125
Additional Description:
Risk Level: Low - feature merged very recently and has not been released
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:]

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #32176 was opened by jbohanon.

see: more, trace.

[jbohanon]

cc @htuch @rshriram

[wbpcode]

#30747 #31090

[wbpcode]

Thanks for you contribution. I taken a look to the dicussion and you previous PR. (But it's a little complex so I am not sure I get it correctly, feel free to point it out if I am wrong.)

I think if you want to revert (or partly revert)previous implementation, you should add the [#not-implemented-hide:] back to the request_attributes and response_attributes because you may want to change the behavior in the implementation.

Actually, if we haven't figured out a final solution or API, we should just revert previous two implementation commits directly. It would be more clean. cc @htuch

[rshriram]

@wbpcode I dont think we need to revert any changes from previous PRs. If we agree on just moving the attributes outside the HttpHeaders message, then this API change becomes much simpler and the implementation should require only minor changes (populate ProcessingRequest.attributes instead of ProcessingRequest.request|response_headers.attributes) on top of the code in #31090.

[rshriram]

@htuch , per discussion in the issue thread, shall we just keep this field as it is, maintaining a clear separation between attributes derived from the connection/request/resp vs those pushed through config or internal state?

[wbpcode]

@wbpcode I dont think we need to revert any changes from previous PRs. If we agree on just moving the attributes outside the HttpHeaders message, then this API change becomes much simpler and the implementation should require only minor changes (populate ProcessingRequest.attributes instead of ProcessingRequest.request|response_headers.attributes) on top of the code in #31090.

Yeah. But the premise is we can agree on with all the API changes (attributes and metadata context) and complete the code and test updates in the windows. (Today may the last day?)

And from my perspective, this PR seems more like a partly reverting rather than refactoring, for now.

Considering the time windows, I still suggest you revert the previous commits and we can merge the reverting directly. Then you can create a new PR for these features immediately.

Or we must act very quickly.

[wbpcode]

/assign @htuch

[wbpcode]

Another solution is defer the metadata_contexr related changes to another PR, because it was merged at one week ago and still has one week time to update it.

Then this PR could only update the attributes implementation (like the @rshriram 's suggestion) and API which would LGTM to me.

[jbohanon]

I have updated this to be more tightly-scoped to the attributes fields. I agree that reverting the committed code is not necessary. The discussion around the field metadata_context is still ongoing in the issue thread, but moving attributes up to be a first-class field in the ProcessingRequest message is not controversial.

[wbpcode]

/retest

[wbpcode]

@jbohanon LGTM to the API, and please update the implementation.

Here is the timeline:

2024/01/24: merged the feature of the request_attributes and response_attributes. This attributes will be add to the attributes in the headers field.

Now, if you want to move the attributes to the ProcessingRequest, you should complete all refactoring (include API and implementation) before 2024/02/07.

[jbohanon]

Now, if you want to move the attributes to the ProcessingRequest, you should complete all refactoring (include API and implementation) before 2024/02/07.

Sounds good. For my education, where is the two-week contract specified?

[jbohanon]

Possible implementation put up here: jbohanon#1

I can pull those changes into this PR if desired, but I still need to work on the tests

[wbpcode]

Now, if you want to move the attributes to the ProcessingRequest, you should complete all refactoring (include API and implementation) before 2024/02/07.

Sounds good. For my education, where is the two-week contract specified?

https://github.com/envoyproxy/envoy/blob/main/api%2FAPI_VERSIONING.md

From this API guiding, we can only do a break change to exist API in two weeks after it is merged or it's marked as WIP.

Now you want to abort the attributes in the header message directly, IMO, it is like a break change. 🤔 So the two weeks windows should be adaptive to this case.

[wbpcode]

I also noticed that the attributes is existing for a long time and still be marked as not-implement-hide. (But in fact it is be used in your previous PR.)
So, I am not sure if we can treat this as special case. 🤔

[rshriram]

@wbpcode that is precisely the reason we are doing this change now and trying to remove the HttpHeaders.attributes because that field was never implemented till now. So we have the ability to make this breaking change.

[rshriram]

LGTM
Thank you so much for acting quickly.

[htuch]

LGTM. To be clear, the headers attributes field was never marked not [#not-implemented-hide:] (it's unchanged on line 206 above). So, from a stable API perspective it was never a stable field :)

[jbohanon]

Thanks @htuch. I am working through the integration test updates now and will ping again once everything is green on CI.

There was an outstanding question on jbohanon#1 re: simply reserving this field that was never marked as stable/implemented. What do you think of that?

[wbpcode]

LGTM to the code. Could you check the integration tests? Seems that the error may is related to this PR.

[jbohanon]

@htuch @wbpcode Thanks for the reviews. I have fixed up the integration test and am ready for another 👀

[wbpcode]

LGTM. Thanks.

[rshriram]

@jbohanon one question. Our API says request_attributes and response_attributes. What about the connection.* attributes documented here (https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes) ? Does the code reject those?

[rshriram]

Put another way, should we just have a generic attributes repeated field in ext_proc filter instead of request and response attributes that does not seem to add any real value, as the wire proto has all attributes in one bag.

[jbohanon]

@jbohanon one question. Our API says request_attributes and response_attributes. What about the connection.* attributes documented here (https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes) ? Does the code reject those?

@rshriram the qualifier in front of _attributes specifies when that list is sent, not what attribute prefixes are accepted. It is the case that connection.* attributes are available on the request path, but it is not the case that response.* attributes are available on the request path. Keeping them separated allows users to list sensible attributes, and just those which they need, for the different request and response processing requests.

[rshriram]

so if I set BOTH request_attributes and response attributes in ext_proc filter to the following,
request.host, source.address, response.code

what happens during request headers/body events and response headers/body events?

Can I assume that there will be exactly ONe copy of request.host, source.address in ProcessingRequest.attributes for request headers/body calls and exactly ONE copy of request.host, source.address, response.code in ProcessingRequest.attributes for response headers/body calls?

[jbohanon]

so if I set BOTH request_attributes and response attributes in ext_proc filter to the following, request.host, source.address, response.code

what happens during request headers/body events and response headers/body events?

Can I assume that there will be exactly ONe copy of request.host, source.address in ProcessingRequest.attributes for request headers/body calls and exactly ONE copy of request.host, source.address, response.code in ProcessingRequest.attributes for response headers/body calls?

Correct, given:

ExternalProcessor:
  request_attributes:
    - request.host
    - source.address
    - response.code
  response_attributes:
    - request.host
    - source.address
    - response.code

There will be exactly ONE copy of request.host and source.address, and exactly ZERO copies of response.code in the ProcessingRequest.attributes for request headers/body/trailers processing calls. There will be exactly ONE copy of request.host and source.address, and response.code in the ProcessingRequest.attributes for response headers/body/trailers processing calls.

I added a test to validate that the unavailable attribute is not included.