config: upgrade envoy.ip_tagging to v3alpha. #9150
Conversation
This is a manual pilot of the v3 API type upgade inside of Envoy. The IP tagging filter was picked as its API dependencies are relatively confined in scope, allowing a self-contained PoC to be written. This will be followed by automated upgrade via the API type database for the bulk of Envoy's internals. Part of envoyproxy#8082 Risk level: Low Testing: Additional unit and integration tests added (version_integration_test). Signed-off-by: Harvey Tuch <[email protected]>
htuch
requested review from
alyssawilk,
lizan,
mattklein123,
snowp and
zuercher
as code owners
|
@lizan @mattklein123 this should act as a litmus test for how the automated updates are going to look. |
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
| @@ -1,7 +1,8 @@ | |||
| #pragma once | |||
|
|
|||
| #include "envoy/config/filter/http/ip_tagging/v2/ip_tagging.pb.h" | |||
| #include "envoy/config/filter/http/ip_tagging/v2/ip_tagging.pb.validate.h" | |||
| #include "envoy/config/filter/http/ip_tagging/v2/ip_tagging.pb.h" // For proto descriptors only | |||
There was a problem hiding this comment.
Will we potentially run into linking dropping needed descriptors like has happened previously when we move to a fully automated/reflection version? Especially in library situations like Envoy Mobile? It would be nice to make sure we avoid this at a system level somehow and don't run into confusing runtime problems? (This is orthogonal to the other discussion about being able to completely disable conversion via compile time option potentially)
There was a problem hiding this comment.
Yeah, I want to take this on as a separate PR. Let's say we don't have the API type DB, and we embed annotations inside the v3 APIs to record some of the history. Do we want to be able to avoid including v2 for Envoy Mobile still? I.e. save the space of the proto descriptors.
There was a problem hiding this comment.
I think that's fine in that case, but I still worry about dropping descriptors that we need in confusing ways, but I think we can figure that out later.
There was a problem hiding this comment.
I think we might need to generate some dummy file to reference all of these. As we encounter this during the conversion work, I will add something to do this. Basically, I envisage a tool that is part of fix_format that uses the API type DB (which will be available at format time) that figures out the entire set of API build targets and headers, that generates some C++ file with lots of void casted references to things in these targets. Or we can try and solve the problem of the lost proto descriptors. Either way.
Signed-off-by: Harvey Tuch <[email protected]>
| #include "common/config/api_type_db.h" | ||
|
|
||
| // For proto descriptors only | ||
| #include "envoy/config/filter/http/ip_tagging/v2/ip_tagging.pb.h" |
There was a problem hiding this comment.
interesting, including header files will include descriptors? only linking isn't enough?
There was a problem hiding this comment.
This is due to how we are auto-generating the BUILD file deps for APIs based on headers. I will address in the next PR which is dealing with the general solution to type upgrade information.
There was a problem hiding this comment.
Also, for some proto descriptors, I think those with only service definitions, there are some C++ link issues. This largely shouldn't be an issue for us during API ingestion.
This is a manual pilot of the v3 API type upgade inside of Envoy. The IP
tagging filter was picked as its API dependencies are relatively
confined in scope, allowing a self-contained PoC to be written.
This will be followed by automated upgrade via the API type database for
the bulk of Envoy's internals.
Part of #8082
Risk level: Low
Testing: Additional unit and integration tests added
(version_integration_test).
Signed-off-by: Harvey Tuch [email protected]