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

networking extensions #1849

Closed
wants to merge 1 commit into from
Closed

Conversation

kyessenov
Copy link
Contributor

Generalization of #1842 to support forward-thinking, proprietary extensions in the HTTP rules.

This roughly matches HTTPRouteFilter.

/assign @howardjohn
/assign @mandarjog
/assign @douglas-reid

Signed-off-by: Kuat Yessenov [email protected]

Signed-off-by: Kuat Yessenov <[email protected]>
@google-cla google-cla bot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Jan 26, 2021
@istio-testing istio-testing added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 26, 2021
@istio-testing
Copy link
Collaborator

@kyessenov: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
release-notes_api 2ba5cc4 link /test release-notes_api

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. I understand the commands that are listed here.

Copy link
Contributor

@mandarjog mandarjog left a comment

Choose a reason for hiding this comment

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

I think this is fine, but it only deal with client side extensions, which are only really used at gateways. We need something equivalent on the server side.

// HTTP route filter for custom extensions.
message HTTPRouteFilter {
// Extension category determining the type of a filter extension.
string category = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

ENUM ? or a list in the doc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to leave it open-ended just like in k8s.

Copy link
Contributor

@dgn dgn Jan 28, 2021

Choose a reason for hiding this comment

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

what kind of categories would these be? if we're referencing a generic Extension resource here, that resource will define its category itself, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, name is sufficient. This is trying to align GVK from k8s Gateway with extensions, so it could help to have an explicit "type" of an extension here.

string name = 2;

// Explicit extension configuration.
google.protobuf.Struct value = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

UpdaStruct, so Istio can verify it against some proto.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Istio API are only using proto for a schema, and I imagine the extension may chose to use k8s schema instead. UDPA struct is pure proto.

@kyessenov kyessenov added the release-notes-none Indicates a PR that does not require release notes. label Jan 26, 2021
@kyessenov
Copy link
Contributor Author

kyessenov commented Jan 26, 2021

@mandarjog Yes, I think for server-side extensions we can generalize Wasm extensions API to cover native extensions. This would be a brand new API. cc @dgn It might be OK to reference extensions API in a per-route extension reference. I just don't see how k8s gateway covers server-side use cases in principle.

@kyessenov
Copy link
Contributor Author

@dgn I think the back-reference from the Extension resource to the VirtualService route is also a viable approach. I do want to make sure that k8s Gateway can be translated to a set of Extension and VirtualService resources. I think there are a few requirements for this to be possible:

  1. k8s allows arbitrary order of filtesr, which is inconsistent with "phases" approach.
  2. We need an individual route match in the match context in Extension.
  3. Wasm filters can be applied per route via a generic typed_per_filter_config mechanism or something like that (cc @PiotrSikora )

@istio-testing istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Jan 30, 2021
@istio-testing
Copy link
Collaborator

@kyessenov: PR needs rebase.

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.

@douglas-reid
Copy link
Contributor

@kyessenov do we have a doc or more info on usage / alignment with other proposed APIs ?

@kyessenov kyessenov closed this Mar 15, 2021
@kyessenov
Copy link
Contributor Author

Daniel's proposal deals with server-side extensions. That is more restricted and simpler, so it should take priority. Ingress/client side can follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. needs-rebase Indicates a PR needs to be rebased before being merged release-notes-none Indicates a PR that does not require release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants