[docs] Remove outdated statements

[derekargueta]

Description: v1 has been deprecated, remove statements about API methods being "optional until v1 is deprecated". Somewhat related to #2832.
Risk Level: none
Testing: N/A
Docs Changes: included
Release Notes: N/A

Signed-off-by: Derek Argueta [email protected]

[zuercher]

There's one more function marked "compulsory when v1 is deprecated" in that file (for createEmptyConfigProto).

That said, I think the way these methods actually become compulsory is to remove their default implementations (such that it becomes a compile error not to implement them in subclasses). I'm not sure if all the current filter factories would pass in that case, however.

@htuch are we ready to start requiring these functions to be implemented or is there another step that needs to happen first?

[derekargueta]

@zuercher agreed about true compulsory would be removing the default implementations. Looks like NamedListenerFilterConfigFactory::createEmptyConfigProto is already PURE so it's at least safe to call that one compulsory.

For the others, I believe the only thing that would prevent us from removing the default implementation for now is testing code that still uses the v1 API. For production filters, I think it's safe to call v2 "compulsory" since

1. production Envoys don't use v1 APIs for the actual runtime
2. If a factory subclassing NamedHttpFilterConfigFactory or NamedNetworkFilterConfigFactory doesn't implement the createEmptyConfigProto methods (which will return nullptr by default), this release assert gets triggered when using the v2 API (the only version supported for running Envoy).

So they currently are "compulsory" in a way for actually running Envoy, but not for unit tests (yet). For example, when updating the listener manager impl tests, I had to implement those methods to move those tests to v2 (here).

[htuch]

I agree, we should make these compulsory. The question is what is a sensible implementation for filters that don't require any configuration? It could just be google.protobuf.Empty. Maybe this should be the default?

I actually had an idea while looking at this and thinking about #6271. We could avoid having filters explicitly provide their semantic release version if they couple it with their config protobuf type URL. There are pros/cons here, but this was a useful reminder that this is possible.

[derekargueta]

I removed the default implementation for all these methods, making them PURE, and so far all builds are working fine. so far tested:

bazel build //source/exe:envoy-static
bazel test //test/common/...
bazel test //test/server/...

It could just be google.protobuf.Empty. Maybe this should be the default?

Yeah that seems the default practice right now, i.e. in EmptyHttpFilterConfig

envoy/source/extensions/filters/http/common/empty_http_filter_config.h

Line 37 in 6980091

ProtobufTypes::MessagePtr createEmptyConfigProto() override {

[zuercher]

So sounds like remove the default, aborting implementations of the createFilterFactoryFromProto methods and change the createEmptyConfigProto methods to return an empty config message.

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[stale]

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!