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

[xds] Relax the CHECK on factories for typed per-filter config #15770

Closed
stevenzzzz opened this issue Mar 30, 2021 · 12 comments · Fixed by #16119
Closed

[xds] Relax the CHECK on factories for typed per-filter config #15770

stevenzzzz opened this issue Mar 30, 2021 · 12 comments · Fixed by #16119
Labels

Comments

@stevenzzzz
Copy link
Contributor

Title: Do not reject the typed per-filter-config if a factory can't be found, remove binaries version coupling.

Description:
Right now for the typedConfig in route we demand the factory class to be linked in at Envoy, otherwise it rejects the whole DiscoveryResponse.
This adds coupling between management server and Envoy, which introduces complexity to rollout/backs of these binaries.

Let's be more tolerant to such config change. (with a runtime feature flag guard), similar to Protobuf upgrade.

Instead of throwing an exception, let's do logging and adding stats for such missing factories.

@stevenzzzz stevenzzzz added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Mar 30, 2021
@stevenzzzz
Copy link
Contributor Author

cc @htuch

@stevenzzzz
Copy link
Contributor Author

@stevenzzzz
Copy link
Contributor Author

cc @AndresGuedez

@mattklein123 mattklein123 added area/xds help wanted Needs help! and removed enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Mar 30, 2021
@mattklein123
Copy link
Member

I think it would be reasonable for this to be configurable, much like we do for other validation options. cc @dmitri-d

@dmitri-d
Copy link
Contributor

To clarify: is the idea to accept a "partial" route config update by ignoring per_filter_configs whose factories cannot be found?

@htuch
Copy link
Member

htuch commented Mar 31, 2021

CC @markdroth who introduced this. I agree it would be useful to make this configurable, probably fine to do it at bootstrap level.

@stevenzzzz
Copy link
Contributor Author

stevenzzzz commented Mar 31, 2021

some clarification: we want to break the management server and Envoy version coupling.
In some cases, it's fine to "ignore" some features for some routes, (esp for a multi-tenancy proxy).

I think we can add a switch for such factories: if mandatory, let's throw, if "optional", feel safe to ignore.

@markdroth
Copy link
Contributor

I think this would be best addressed by fixing #15025. That way, the control plane can explicitly indicate which filters should be optional and which are required.

Note that gRPC has already implemented support for the new is_optional fields.

@markdroth
Copy link
Contributor

Also, I don't think I introduced Envoy's current behavior -- I think it has always done this.

@soulxu
Copy link
Member

soulxu commented Apr 14, 2021

I think this would be best addressed by fixing #15025. That way, the control plane can explicitly indicate which filters should be optional and which are required.

Note that gRPC has already implemented support for the new is_optional fields.

emm....I missed this comments, looks like I fixed it as a wrong way.

@soulxu
Copy link
Member

soulxu commented Apr 15, 2021

I think this would be best addressed by fixing #15025. That way, the control plane can explicitly indicate which filters should be optional and which are required.
Note that gRPC has already implemented support for the new is_optional fields.

emm....I missed this comments, looks like I fixed it as a wrong way.

Close my PR. The optional filter is preferred fixed. @markdroth let me know if you need any volunteers to help on #15025.

@markdroth
Copy link
Contributor

As far as I know, no one is working on that issue, so you should feel free to send a PR for that.

We may want to just close this bug as a dup of #15025.

lizan pushed a commit that referenced this issue May 18, 2021
Enable `is_optional` field for `HttpFilter`. The default value is `false`, it will keep the same behavior with the current implementation. The envoy will reject the unsupported HTTP filter, also will reject the unsupported HTTP filter in typed per filter config. When the value is `true`, the unsupported HTTP filter will be ignored by the envoy, also will be ignored by typed per filter config, with a warning log.

Risk Level: low
Testing: unit tests and integration tests are added
Docs Changes: API doc is added
Release Notes: added as new feature
Fixes #15770 
Fixes #15025

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this issue Sep 30, 2021
Enable `is_optional` field for `HttpFilter`. The default value is `false`, it will keep the same behavior with the current implementation. The envoy will reject the unsupported HTTP filter, also will reject the unsupported HTTP filter in typed per filter config. When the value is `true`, the unsupported HTTP filter will be ignored by the envoy, also will be ignored by typed per filter config, with a warning log.

Risk Level: low
Testing: unit tests and integration tests are added
Docs Changes: API doc is added
Release Notes: added as new feature
Fixes envoyproxy#15770 
Fixes envoyproxy#15025

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants