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

Handle 1.12 contrib-image changes #35336

Closed
howardjohn opened this issue Sep 23, 2021 · 12 comments
Closed

Handle 1.12 contrib-image changes #35336

howardjohn opened this issue Sep 23, 2021 · 12 comments

Comments

@howardjohn
Copy link
Member

https://www.envoyproxy.io/docs/envoy/latest/start/install#install-contrib

In 1.20 envoy (1.12 Istio), some filters are removed from the core image. I believe this will impact our build, as we inherit this.

MySQL filter is used by Istio, albeit in alpha behind an env var flag.

We should consider how to handle this... some ideas:

  • Add them all back (as EnvoyFilter could use them)
  • Add only mysql back, since its the only 'first class' one
  • Keep them removed (and possibly even remove others), and remove first class mysql support
    • Possibly remove the other perma-alpha protocol filters as well

cc @zhaohuabing @kyessenov

@howardjohn howardjohn added this to the 1.12 milestone Sep 23, 2021
@howardjohn howardjohn added this to Release Blocker in Prioritization Sep 23, 2021
@zhaohuabing
Copy link
Member

zhaohuabing commented Sep 24, 2021

If Envoy project thinks some filters are not mature enough to be included in the official imagine, maybe we should follow this policy as well. Could it be possible that Istio proxy also provides two images: one only has official extensions and one includes the filters in the Envoy contrib directory? Then users can choose what images to use based on their use cases.

Another relevant but not direct issue about third-party protocols support is that Istio requires envoy filter proto go API to be compiled with its core repo. Otherwise validation webhook will complain it doesn't know the proto. So when I tried to support a new protocol in Istio via EnvoyFilter, I had to modify Istio source code, like this: https://github.com/aeraki-framework/istio/blob/270b62b9eff7dfb6439204c0d6be2ed876d82264/pkg/config/xds/filter_types.gen.go#L368

Can we eliminate this dependency somehow?

@poussa
Copy link

poussa commented Sep 24, 2021

I would vote for Istio proxy project to build two images (official and contrib). In order to move from contrib to official, the Envoy project requires users of the feature. If Istio proxy produces both images it makes much easier for the users to try out the contrib images, thus gaining the required users.

@durd07
Copy link

durd07 commented Sep 29, 2021

I also would vote for Istio project to build two images(official and contrib). There will be more and more envoy extensions contribute to contrib in the future. In order to support them in Istio, we can now only change the file filter_types.gen.go(maybe better solution in the future) but no more changes.

@howardjohn
Copy link
Member Author

In networking WG we decided for 1.12 we will just add them back to defer a decision. For future we may do another solution.

@lambdai
Copy link
Contributor

lambdai commented Oct 26, 2021

Another extension tcp_stats that should live in potential contrib image

This one is even trickier because it requires the build host has linux kernel >=4.6

#35742

@howardjohn
Copy link
Member Author

@lambdai I think we want all of the extensions that were in 1.11, not just mysql. Including kafka, etc.

The original agreement was:

Temporary fix for 1.12, permanent post 1.12.
Revert upstream Envoy change for 1.12 since it is so soon
Figure out better resolution post 1.12

@kyessenov
Copy link
Contributor

@zhaohuabing You can use TypedStruct to avoid a direct protobuf dependency. It's a typed wrapper with JSON inside.

The decision is about setting expectations. I don't think we can or should set the expectations that CVEs in contrib envoy extensions have to be addressed. Allowing users to use some untested features in Envoy binary is a dangerous precedent since it puts users into possibly insecure state without us even being aware of it.

As for MySQL/Kafka - these have been in alpha since 2016. I think it's time to either move them to beta or remove. It's not sustainable to have features in permanent alpha state.

@kyessenov
Copy link
Contributor

@howardjohn I think we need to support custom envoy builds regardless of contrib/core split. Control plane must be able to detect missing extensions and disable features gracefully. We already provide multiple different envoy builds with different sets of extensions. It is perfectly fine to use non-istio Envoy build. Telemetry won't work fully (which can be fixed with Wasm) but not everyone cares about it.

@lambdai
Copy link
Contributor

lambdai commented Oct 27, 2021

@lambdai I think we want all of the extensions that were in 1.11, not just mysql. Including kafka, etc.

yes, when it is backported to 1.12 we need an accurate list

@howardjohn
Copy link
Member Author

I think there are a few issues with upstream Envoy - but I would be extremely happy if they were mitigated. I think this is clearly the best path forward if feasible:

  • Telemetry, as you noted -> WASM can fix
  • Authn still uses custom filters, but only if you explicitly opt in. In older versions we always inserted them but not anymore -> I think someone was working on porting to WASM?
  • ALPN filter to set the istio-h2, istio-http/1.1 - Not sure how to address this. The filter is pretty tiny if we are able to just upstream it..

I think there is a lot of value in making it an opt in for contrib (ie you get contrib filters OR you get Istio filters, or build a custom image). And huge value to make it the default/only way we ship envoy - but that will require a lot more work since it will need to be zero-compromises.

@lambdai
Copy link
Contributor

lambdai commented Nov 4, 2021

envoy.filters.network: ..., envoy.filters.network.kafka_broker, envoy.filters.network.kafka_mesh, envoy.filters.network.local_ratelimit, envoy.filters.network.metadata_exchange, envoy.filters.network.mongo_proxy, envoy.filters.network.mysql_proxy,

envoy.filters.http: ..., istio.alpn, istio_authn, match-wrapper

@lambdai
Copy link
Contributor

lambdai commented Nov 4, 2021

1.12 issue is resolved.

ALPN filter will be deprecated with tunnel solution, but not now. Let's discuss in dedicated thread

@lambdai lambdai closed this as completed Nov 4, 2021
Prioritization automation moved this from Release Blocker to Done Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

6 participants