Skip to content

Virtual Service Direct Response #2407

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 9 commits into from
Jul 7, 2022
Merged

Virtual Service Direct Response #2407

merged 9 commits into from
Jul 7, 2022

Conversation

sschepens
Copy link
Contributor

@sschepens sschepens commented Jul 4, 2022 •
edited
Loading

@istio-policy-bot
Copy link

😊 Welcome @sschepens! This is either your first contribution to the Istio api repo, or it's been
awhile since you've been here.

You can learn more about the Istio working groups, code of conduct, and contributing guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test labels Jul 4, 2022
@istio-testing
Copy link
Collaborator

Hi @sschepens. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sschepens sschepens mentioned this pull request Jul 4, 2022
Merged
hzxuzhonghu
hzxuzhonghu reviewed Jul 5, 2022
View reviewed changes
@ericvn
Copy link

ericvn commented Jul 5, 2022

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Jul 5, 2022
@istio-policy-bot
Copy link

🤔 🐛 You appear to be fixing a bug in Go code, yet your PR doesn't include updates to any test files. Did you forget to add a test?

Courtesy of your friendly test nag.

@sschepens
Copy link
Contributor Author

@ericvn gencheck_api is failing because there seems to have been un update of protoc-gen-go i'm guessing nobody did a make gen after the last tools update?

@ericvn
Copy link

ericvn commented Jul 5, 2022

It could be that there is a difference in tooling used when running locally vs within the pipeline. I hope to fix that here shortly.

@sschepens
Copy link
Contributor Author

sschepens commented Jul 5, 2022 •
edited
Loading

@ericvn I pushed a commit using the previous image so that CI is happy.

Another thing, OpenApi generation is assigning format: binary to the bytes field which I think is not exactly correct, I think format: byte would be best, which is theoretically a base64 encoded bytes.
Is there any way to customize the type being assigned to a field? If there is no way, then I can just make it a string field and convert in istiod.

@ericvn ericvn mentioned this pull request Jul 5, 2022
Merged
linsun
linsun reviewed Jul 6, 2022
View reviewed changes
linsun
linsun reviewed Jul 6, 2022
View reviewed changes
Copy link
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

LGTM overall, would like an example on bytes

@sschepens
Revert "use previous tools"
86e39f7 
This reverts commit 293ec72.
@sschepens
Copy link
Contributor Author

sschepens commented Jul 6, 2022 •
edited
Loading

@linsun do you have any idea on the question I asked in my previous comment? when using bytes in protobuf it would seem that we're translating to a format: binary in OpenApi which would not be correct in this case, format: byte would be more appropiate. Is there any way to change the translated format?

@sschepens
Copy link
Contributor Author

sschepens commented Jul 6, 2022 •
edited
Loading

Added bytes example.

I think format binary is ok though, should be ready for merge.

@sschepens sschepens requested review from hzxuzhonghu and linsun July 7, 2022 14:22
@sschepens
Copy link
Contributor Author

/retest

linsun
linsun approved these changes Jul 7, 2022
View reviewed changes
Copy link
Member

@linsun linsun left a comment

Choose a reason for hiding this comment

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

LGTM

@louiscryan
Copy link
Contributor

Note that the envoy filter has an "unknown security posture" that makes me quite nervous about including this in the API

@louiscryan
Copy link
Contributor

See https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/network/direct_response/v3/config.proto

@sschepens
Copy link
Contributor Author

@louiscryan I think what you're seeing is the network filter (which is not used for HTTP) and not the direct_response of the router filter. The router filter has a known security posture.

@sschepens
Copy link
Contributor Author

This is what we will be using: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-msg-config-route-v3-directresponseaction

louiscryan
louiscryan reviewed Jul 7, 2022
View reviewed changes
networking/v1beta1/virtual_service.proto
// - match:
// - uri:
// exact: /v1/getProductRatings
// directResponse:
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned in the Envoy docs its good practice to recommend setting response headers, particularly content-type. Can you add that to one of the examples.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, i can add

@louiscryan
Copy link
Contributor

This is what we will be using: https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#envoy-v3-api-msg-config-route-v3-directresponseaction

Ah ok, thanks for clarifying

@istio-testing istio-testing added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 7, 2022
@sschepens
Copy link
Contributor Author

@louiscryan added an example with headers

louiscryan
louiscryan approved these changes Jul 7, 2022
View reviewed changes
Copy link
Contributor

@louiscryan louiscryan left a comment

Choose a reason for hiding this comment

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

LGTM

@istio-testing istio-testing merged commit de09801 into istio:master Jul 7, 2022
@sschepens sschepens deleted the direct-response branch July 7, 2022 22:35
hzxuzhonghu
hzxuzhonghu reviewed Jul 8, 2022
View reviewed changes
networking/v1beta1/virtual_service.proto
// directResponse:
// status: 503
// body:
// string: "unknown error"
Copy link
Member

Choose a reason for hiding this comment

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

is this right?

@ericvn ericvn added release-notes-none Indicates a PR that does not require release notes. and removed release-notes-none Indicates a PR that does not require release notes. labels Jul 8, 2022
@zirain zirain mentioned this pull request Sep 1, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu hzxuzhonghu left review comments

@linsun linsun linsun approved these changes

@louiscryan louiscryan louiscryan approved these changes

@nrjpoddar nrjpoddar Awaiting requested review from nrjpoddar

@howardjohn howardjohn Awaiting requested review from howardjohn

@ericvn ericvn Awaiting requested review from ericvn

Assignees
No one assigned
Labels
ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support direct response in VirtualService
7 participants
@sschepens @istio-policy-bot @istio-testing @ericvn @louiscryan @linsun @hzxuzhonghu