Skip to content

proto: force link missing v2 protos #9615

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.

Sign up for GitHub

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

Merged
htuch merged 6 commits into from
Jan 10, 2020
Merged

proto: force link missing v2 protos #9615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d08f0aa
proto: link HDS v2
Jan 9, 2020
61a4b8b
update
Jan 9, 2020
35c5e26
fmt
Jan 9, 2020
57e4556
forced into v2 by formatter
Jan 9, 2020
01af73e
linking issue
Jan 9, 2020
b3a2c26
fix link
Jan 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions source/common/config/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,7 @@ envoy_cc_library(
hdrs = ["protobuf_link_hacks.h"],
deps = [
"@envoy_api//envoy/api/v2:pkg_cc_proto",
"@envoy_api//envoy/config/bootstrap/v2:pkg_cc_proto",
"@envoy_api//envoy/service/cluster/v3alpha:pkg_cc_proto",
"@envoy_api//envoy/service/discovery/v2:pkg_cc_proto",
"@envoy_api//envoy/service/discovery/v3alpha:pkg_cc_proto",
Expand Down
19 changes: 18 additions & 1 deletion source/common/config/protobuf_link_hacks.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
#include "envoy/api/v2/lds.pb.h"
#include "envoy/api/v2/rds.pb.h"
#include "envoy/api/v2/srds.pb.h"
#include "envoy/config/bootstrap/v2/bootstrap.pb.h"
#include "envoy/service/cluster/v3alpha/cds.pb.h"
#include "envoy/service/discovery/v2/ads.pb.h"
#include "envoy/service/discovery/v2/hds.pb.h"
#include "envoy/service/discovery/v2/rtds.pb.h"
#include "envoy/service/discovery/v2/sds.pb.h"
#include "envoy/service/discovery/v3alpha/ads.pb.h"
Expand Down Expand Up @@ -43,5 +45,20 @@ const envoy::service::listener::v3alpha::LdsDummy _lds_dummy_v3;
const envoy::service::route::v3alpha::RdsDummy _rds_dummy_v3;
const envoy::service::cluster::v3alpha::CdsDummy _cds_dummy_v3;
const envoy::service::endpoint::v3alpha::EdsDummy _eds_dummy_v3;
const envoy::service::route::v3alpha::SrdsDummy _srds_dummy_v4;
const envoy::service::route::v3alpha::SrdsDummy _srds_dummy_v3;

// With the v2 -> v3 migration there is another, related linking issue.
// Symbols for v2 protos which headers are not included in any file in the codebase are being
// dropped by the linker in some circumstances. For example, in the Envoy Mobile iOS build system.
// Even though all v2 packages are included as a dependency in their corresponding v3 package, and
// `always_link` is set for all proto bazel targets.
// Further proof of this can be seen by way of counter example with the envoy.api.v2.Cluster type,
// which is checked for by proto_descriptors.cc. This type **is** getting linked because its headers
// is still included in cds_api_impl.cc. On the other side because the v2 hds header is not included
// anywhere the v2 service type is getting dropped, and thus the descriptor is not present in the
// descriptor pool.
Copy link
Member

@htuch htuch Jan 9, 2020

Choose a reason for hiding this comment

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

Can you include a link to the tracking issue here?

Copy link
Member Author

@junr03 junr03 Jan 9, 2020

Choose a reason for hiding this comment

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

yep, was waiting to see what you thought. Will create and link now.

// https://github.com/envoyproxy/envoy/issues/9639
const envoy::config::bootstrap::v2::Bootstrap _bootstrap_dummy_v2;
const envoy::service::discovery::v2::Capability _hds_dummy_v2;

} // namespace Envoy
1 change: 1 addition & 0 deletions source/extensions/stat_sinks/metrics_service/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ envoy_cc_library(
deps = [
"//source/common/common:assert_lib",
"//source/common/protobuf",
"@envoy_api//envoy/service/metrics/v2:pkg_cc_proto",
],
)

Expand Down
5 changes: 5 additions & 0 deletions source/extensions/stat_sinks/metrics_service/grpc_metrics_proto_descriptors.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#include "extensions/stat_sinks/metrics_service/grpc_metrics_proto_descriptors.h"

#include "envoy/service/metrics/v2/metrics_service.pb.h"

#include "common/common/assert.h"
#include "common/common/fmt.h"
#include "common/protobuf/protobuf.h"
Expand All @@ -10,6 +12,9 @@ namespace StatSinks {
namespace MetricsService {

void validateProtoDescriptors() {
// https://github.com/envoyproxy/envoy/issues/9639
const envoy::service::metrics::v2::StreamMetricsMessage _dummy_v2;
Copy link
Member

@htuch htuch Jan 9, 2020

Choose a reason for hiding this comment

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

Can this be in protobuf_link_hacks? That way we can centralize this and more easily cleanup later.

Copy link
Member Author

@junr03 junr03 Jan 9, 2020

Choose a reason for hiding this comment

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

I don't know all the intricacies of the extensions build system but I thought that because this was an extension I would leave this under the extensions tree. I'll put a link to the tracking issue once I create it.

Copy link
Member

@htuch htuch Jan 9, 2020

Choose a reason for hiding this comment

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

SGTM


const auto method = "envoy.service.metrics.v2.MetricsService.StreamMetrics";

RELEASE_ASSERT(Protobuf::DescriptorPool::generated_pool()->FindMethodByName(method) != nullptr,
Expand Down