Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: ext-proc processing mode #3171

Merged
merged 12 commits into from
Apr 18, 2024
Merged

Conversation

guydc
Copy link
Contributor

@guydc guydc commented Apr 10, 2024

What this PR does / why we need it:
Allows users to define how request and response header/body are processed by exteral processing service.

By default, nothing is sent to the external processor. If an empty request/response struct is defined, headers of request/response are sent. If the body attribute is configured, the body is processed according to the defined processing mode (Stream, Buffer, BufferPartial).

Which issue(s) this PR fixes:
xref: #3170

@guydc guydc requested a review from a team as a code owner April 10, 2024 19:58
Signed-off-by: Guy Daich <guy.daich@sap.com>
@guydc guydc force-pushed the api-extproc-procmode branch from ae8c6b2 to 844558b Compare April 10, 2024 20:17
@guydc
Copy link
Contributor Author

guydc commented Apr 10, 2024

/retest

@guydc guydc added this to the v1.1.0-rc1 milestone Apr 10, 2024
@guydc
Copy link
Contributor Author

guydc commented Apr 10, 2024

/retest

1 similar comment
@guydc
Copy link
Contributor Author

guydc commented Apr 10, 2024

/retest

@zirain
Copy link
Contributor

zirain commented Apr 12, 2024

@guydc sorry, linter works as expected now, can you fix it?

1 similar comment
@zirain
Copy link
Contributor

zirain commented Apr 12, 2024

@guydc sorry, linter works as expected now, can you fix it?

guydc added 3 commits April 12, 2024 09:42
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
Signed-off-by: Guy Daich <guy.daich@sap.com>
@guydc
Copy link
Contributor Author

guydc commented Apr 12, 2024

/retest

guydc added 2 commits April 16, 2024 09:40
Signed-off-by: Guy Daich <guy.daich@sap.com>
@guydc
Copy link
Contributor Author

guydc commented Apr 16, 2024

/retest

@guydc guydc requested review from zirain and shawnh2 April 17, 2024 20:47
Copy link
Contributor

@zirain zirain left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

lgtm

type ExtProcBodyProcessingMode string

const (
StreamedExtProcBodyProcessingMode ExtProcBodyProcessingMode = "Streamed"
Copy link
Member

@zhaohuabing zhaohuabing Apr 18, 2024

Choose a reason for hiding this comment

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

Could we add comments for each mode? This way they can show in the generated API docs.

Signed-off-by: Guy Daich <guy.daich@sap.com>
@zhaohuabing
Copy link
Member

Minor/non-blocking comment on naming:

ProcessingMode is not a very clear name, but I can't think of a better name either.

Signed-off-by: Guy Daich <guy.daich@sap.com>
Copy link
Contributor

@arkodg arkodg 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 !

@arkodg
Copy link
Contributor

arkodg commented Apr 18, 2024

Minor/non-blocking comment on naming:

ProcessingMode is not a very clear name, but I can't think of a better name either.

yeah this API field is doing two things

  • what to send (body, header)
  • when to send it (stream, buffered)

can't think of a better name, so lets go ahead with the envoy specific name, since this feature is completely tied to envoy

@arkodg arkodg requested a review from zhaohuabing April 18, 2024 20:28
Copy link
Member

@zhaohuabing zhaohuabing 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.

@arkodg arkodg merged commit db41b16 into envoyproxy:main Apr 18, 2024
20 checks passed
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.

5 participants