Skip to content

Commit 9ac0dd8

Browse files
phlaxphlax
authored
Revert "Begin process of removing singleton use by StringMatcher (#32… (#32902)
Revert "Begin process of removing singleton use by StringMatcher (#32829)" This reverts commit b7818b0. Signed-off-by: Ryan Northey <[email protected]>
1 parent d5bfc06 commit 9ac0dd8

File tree

71 files changed

+236
-410
lines changed

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

71 files changed

+236
-410
lines changed

‎contrib/postgres_proxy/filters/network/test/postgres_integration_test.cc

+2-4
Original file line numberDiff line numberDiff line change
@@ -295,8 +295,7 @@ class UpstreamSSLBaseIntegrationTest : public PostgresBaseIntegrationTest {
295295
// The tls transport socket will be inserted into fake_upstream when
296296
// Envoy's upstream starttls transport socket is converted to secure mode.
297297
std::unique_ptr<Ssl::ContextManager> tls_context_manager =
298-
std::make_unique<Extensions::TransportSockets::Tls::ContextManagerImpl>(
299-
server_factory_context_);
298+
std::make_unique<Extensions::TransportSockets::Tls::ContextManagerImpl>(timeSystem());
300299

301300
envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext downstream_tls_context;
302301

@@ -528,8 +527,7 @@ class UpstreamAndDownstreamSSLIntegrationTest : public UpstreamSSLBaseIntegratio
528527
// The tls transport socket will be inserted into fake_upstream when
529528
// Envoy's upstream starttls transport socket is converted to secure mode.
530529
std::unique_ptr<Ssl::ContextManager> tls_context_manager =
531-
std::make_unique<Extensions::TransportSockets::Tls::ContextManagerImpl>(
532-
server_factory_context_);
530+
std::make_unique<Extensions::TransportSockets::Tls::ContextManagerImpl>(timeSystem());
533531

534532
envoy::extensions::transport_sockets::tls::v3::UpstreamTlsContext upstream_tls_context;
535533

‎envoy/server/factory_context.h

-10
Original file line numberDiff line numberDiff line change
@@ -36,11 +36,6 @@
3636
#include "source/common/protobuf/protobuf.h"
3737

3838
namespace Envoy {
39-
40-
namespace Regex {
41-
class Engine;
42-
}
43-
4439
namespace Server {
4540
namespace Configuration {
4641

@@ -134,11 +129,6 @@ class CommonFactoryContext {
134129
* @return ServerLifecycleNotifier& the lifecycle notifier for the server.
135130
*/
136131
virtual ServerLifecycleNotifier& lifecycleNotifier() PURE;
137-
138-
/**
139-
* @return the server regex engine.
140-
*/
141-
virtual Regex::Engine& regexEngine() PURE;
142132
};
143133

144134
/**

‎envoy/server/instance.h

-5
Original file line numberDiff line numberDiff line change
@@ -253,11 +253,6 @@ class Instance {
253253
*/
254254
virtual Configuration::StatsConfig& statsConfig() PURE;
255255

256-
/**
257-
* @return the server regex engine.
258-
*/
259-
virtual Regex::Engine& regexEngine() PURE;
260-
261256
/**
262257
* @return envoy::config::bootstrap::v3::Bootstrap& the servers bootstrap configuration.
263258
*/

‎envoy/ssl/context_manager.h

+1-9
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,6 @@
1010
#include "envoy/stats/scope.h"
1111

1212
namespace Envoy {
13-
14-
namespace Server {
15-
namespace Configuration {
16-
class CommonFactoryContext;
17-
} // namespace Configuration
18-
} // namespace Server
19-
2013
namespace Ssl {
2114

2215
// Opaque type defined and used by the ``ServerContext``.
@@ -80,8 +73,7 @@ using ContextManagerPtr = std::unique_ptr<ContextManager>;
8073
class ContextManagerFactory : public Config::UntypedFactory {
8174
public:
8275
~ContextManagerFactory() override = default;
83-
virtual ContextManagerPtr
84-
createContextManager(Server::Configuration::CommonFactoryContext& factory_context) PURE;
76+
virtual ContextManagerPtr createContextManager(TimeSource& time_source) PURE;
8577

8678
// There could be only one factory thus the name is static.
8779
std::string name() const override { return "ssl_context_manager"; }

‎mobile/library/common/extensions/cert_validator/platform_bridge/config.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ namespace Tls {
99

1010
CertValidatorPtr PlatformBridgeCertValidatorFactory::createCertValidator(
1111
const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
12-
Server::Configuration::CommonFactoryContext& /*context*/) {
12+
TimeSource& /*time_source*/) {
1313
return std::make_unique<PlatformBridgeCertValidator>(config, stats);
1414
}
1515

‎mobile/library/common/extensions/cert_validator/platform_bridge/config.h

+2-3
Original file line numberDiff line numberDiff line change
@@ -15,9 +15,8 @@ namespace Tls {
1515
class PlatformBridgeCertValidatorFactory : public CertValidatorFactory,
1616
public Config::TypedFactory {
1717
public:
18-
CertValidatorPtr
19-
createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
20-
Server::Configuration::CommonFactoryContext& context) override;
18+
CertValidatorPtr createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config,
19+
SslStats& stats, TimeSource& time_source) override;
2120

2221
std::string name() const override {
2322
return "envoy_mobile.cert_validator.platform_bridge_cert_validator";

‎mobile/test/common/integration/BUILD

-2
Original file line numberDiff line numberDiff line change
@@ -184,7 +184,6 @@ envoy_cc_test_library(
184184
"@envoy//source/exe:process_wide_lib",
185185
"@envoy//test/integration:autonomous_upstream_lib",
186186
"@envoy//test/integration:utility_lib",
187-
"@envoy//test/mocks/server:server_factory_context_mocks",
188187
"@envoy//test/mocks/server:transport_socket_factory_context_mocks",
189188
"@envoy//test/test_common:environment_lib",
190189
"@envoy_build_config//:extension_registry",
@@ -214,7 +213,6 @@ envoy_cc_test_library(
214213
"@envoy//source/exe:process_wide_lib",
215214
"@envoy//test/integration:autonomous_upstream_lib",
216215
"@envoy//test/integration:utility_lib",
217-
"@envoy//test/mocks/server:server_factory_context_mocks",
218216
"@envoy//test/mocks/server:transport_socket_factory_context_mocks",
219217
"@envoy//test/test_common:environment_lib",
220218
"@envoy_api//envoy/extensions/transport_sockets/tls/v3:pkg_cc_proto",

‎mobile/test/common/integration/test_server.cc

+1-1
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ namespace Envoy {
2525
Network::DownstreamTransportSocketFactoryPtr TestServer::createQuicUpstreamTlsContext(
2626
testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext>& factory_context) {
2727
envoy::extensions::transport_sockets::tls::v3::DownstreamTlsContext tls_context;
28-
Extensions::TransportSockets::Tls::ContextManagerImpl context_manager{server_factory_context_};
28+
Extensions::TransportSockets::Tls::ContextManagerImpl context_manager{time_system_};
2929
tls_context.mutable_common_tls_context()->add_alpn_protocols("h3");
3030
envoy::extensions::transport_sockets::tls::v3::TlsCertificate* certs =
3131
tls_context.mutable_common_tls_context()->add_tls_certificates();

‎mobile/test/common/integration/test_server.h

+1-3
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88

99
#include "envoy/extensions/transport_sockets/quic/v3/quic_transport.pb.h"
1010
#include "test/integration/autonomous_upstream.h"
11-
#include "test/mocks/server/server_factory_context.h"
1211
#include "test/mocks/server/transport_socket_factory_context.h"
1312
#include "test/integration/server.h"
1413

@@ -27,7 +26,6 @@ enum class TestServerType {
2726
class TestServer : public ListenerHooks {
2827
private:
2928
testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context_;
30-
testing::NiceMock<Server::Configuration::MockServerFactoryContext> server_factory_context_;
3129
Stats::IsolatedStoreImpl stats_store_;
3230
Event::GlobalTimeSystem time_system_;
3331
Api::ApiPtr api_;
@@ -37,7 +35,7 @@ class TestServer : public ListenerHooks {
3735
Thread::SkipAsserts skip_asserts_;
3836
ProcessWide process_wide;
3937
Thread::MutexBasicLockable lock;
40-
Extensions::TransportSockets::Tls::ContextManagerImpl context_manager_{server_factory_context_};
38+
Extensions::TransportSockets::Tls::ContextManagerImpl context_manager_{time_system_};
4139
std::unique_ptr<bazel::tools::cpp::runfiles::Runfiles> runfiles_;
4240

4341
// Either test_server_ will be set for test_server_type is a proxy, otherwise upstream_ will be

‎mobile/test/common/integration/xds_test_server.h

+1-3
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77

88
#include "test/integration/fake_upstream.h"
99
#include "test/integration/server.h"
10-
#include "test/mocks/server/server_factory_context.h"
1110
#include "test/mocks/server/transport_socket_factory_context.h"
1211
#include "test/test_common/test_time.h"
1312

@@ -37,7 +36,6 @@ class XdsTestServer {
3736

3837
private:
3938
testing::NiceMock<Server::Configuration::MockTransportSocketFactoryContext> factory_context_;
40-
testing::NiceMock<Server::Configuration::MockServerFactoryContext> server_factory_context_;
4139
Stats::IsolatedStoreImpl stats_store_;
4240
Event::GlobalTimeSystem time_system_;
4341
Api::ApiPtr api_;
@@ -46,7 +44,7 @@ class XdsTestServer {
4644
Event::DispatcherPtr dispatcher_;
4745
FakeUpstreamConfig upstream_config_;
4846
Thread::MutexBasicLockable lock_;
49-
Extensions::TransportSockets::Tls::ContextManagerImpl context_manager_{server_factory_context_};
47+
Extensions::TransportSockets::Tls::ContextManagerImpl context_manager_{time_system_};
5048
std::unique_ptr<bazel::tools::cpp::runfiles::Runfiles> runfiles_;
5149
std::unique_ptr<FakeUpstream> xds_upstream_;
5250
FakeHttpConnectionPtr xds_connection_;

‎mobile/test/common/integration/xds_test_server_interface.cc

-4
Original file line numberDiff line numberDiff line change
@@ -12,10 +12,6 @@ static std::weak_ptr<Envoy::XdsTestServer> weak_test_server_;
1212
static std::shared_ptr<Envoy::XdsTestServer> testServer() { return weak_test_server_.lock(); }
1313

1414
void initXdsServer() {
15-
// This is called via JNI from kotlin tests, and Envoy doesn't consider it a test thread
16-
// which triggers some failures of `ASSERT_IS_MAIN_OR_TEST_THREAD()`.
17-
Envoy::Thread::SkipAsserts skip;
18-
1915
Envoy::ExtensionRegistry::registerFactories();
2016
strong_test_server_ = std::make_shared<Envoy::XdsTestServer>();
2117
weak_test_server_ = strong_test_server_;

‎source/common/common/matchers.cc

+2-3
Original file line numberDiff line numberDiff line change
@@ -201,10 +201,9 @@ bool PathMatcher::match(const absl::string_view path) const {
201201
return matcher_.match(Http::PathUtil::removeQueryAndFragment(path));
202202
}
203203

204-
StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config,
205-
ThreadLocal::SlotAllocator& tls, Api::Api& api) {
204+
StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config) {
206205
auto factory = Config::Utility::getAndCheckFactory<StringMatcherExtensionFactory>(config, false);
207-
return factory->createStringMatcher(config.typed_config(), tls, api);
206+
return factory->createStringMatcher(config.typed_config());
208207
}
209208

210209
} // namespace Matchers

‎source/common/common/matchers.h

+9-48
Original file line numberDiff line numberDiff line change
@@ -86,36 +86,24 @@ class UniversalStringMatcher : public StringMatcher {
8686
bool match(absl::string_view) const override { return true; }
8787
};
8888

89-
StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config,
90-
ThreadLocal::SlotAllocator& tls, Api::Api& api);
89+
StringMatcherPtr getExtensionStringMatcher(const ::xds::core::v3::TypedExtensionConfig& config);
9190

9291
template <class StringMatcherType = envoy::type::matcher::v3::StringMatcher>
93-
class PrivateStringMatcherImpl : public ValueMatcher, public StringMatcher {
92+
class StringMatcherImpl : public ValueMatcher, public StringMatcher {
9493
public:
95-
// TODO(ggreenway): convert all but the first parameter into
96-
// `Server::Configuration::CommonFactoryContext`.
97-
explicit PrivateStringMatcherImpl(const StringMatcherType& matcher, Regex::Engine* regex_engine,
98-
ThreadLocal::SlotAllocator* tls, Api::Api* api)
99-
: matcher_(matcher) {
94+
explicit StringMatcherImpl(const StringMatcherType& matcher) : matcher_(matcher) {
10095
if (matcher.match_pattern_case() == StringMatcherType::MatchPatternCase::kSafeRegex) {
10196
if (matcher.ignore_case()) {
10297
ExceptionUtil::throwEnvoyException("ignore_case has no effect for safe_regex.");
10398
}
104-
if (regex_engine != nullptr) {
105-
regex_ = Regex::Utility::parseRegex(matcher_.safe_regex(), *regex_engine);
106-
} else {
107-
// TODO(ggreenway): remove this branch when we always have an engine. This is only
108-
// needed to make tests not complain about dereferencing a null pointer, even though
109-
// the reference isn't actually used.
110-
regex_ = Regex::Utility::parseRegex(matcher_.safe_regex());
111-
}
99+
regex_ = Regex::Utility::parseRegex(matcher_.safe_regex());
112100
} else if (matcher.match_pattern_case() == StringMatcherType::MatchPatternCase::kContains) {
113101
if (matcher_.ignore_case()) {
114102
// Cache the lowercase conversion of the Contains matcher for future use
115103
lowercase_contains_match_ = absl::AsciiStrToLower(matcher_.contains());
116104
}
117105
} else {
118-
initialize(matcher, tls, api);
106+
initialize(matcher);
119107
}
120108
}
121109

@@ -155,13 +143,11 @@ class PrivateStringMatcherImpl : public ValueMatcher, public StringMatcher {
155143
// overloading to only handle that case for type `envoy::type::matcher::v3::StringMatcher` to
156144
// prevent compilation errors on use of `kCustom`.
157145

158-
void initialize(const xds::type::matcher::v3::StringMatcher&, ThreadLocal::SlotAllocator*,
159-
Api::Api*) {}
146+
void initialize(const xds::type::matcher::v3::StringMatcher&) {}
160147

161-
void initialize(const envoy::type::matcher::v3::StringMatcher& matcher,
162-
ThreadLocal::SlotAllocator* tls, Api::Api* api) {
148+
void initialize(const envoy::type::matcher::v3::StringMatcher& matcher) {
163149
if (matcher.has_custom()) {
164-
custom_ = getExtensionStringMatcher(matcher.custom(), *tls, *api);
150+
custom_ = getExtensionStringMatcher(matcher.custom());
165151
}
166152
}
167153

@@ -207,34 +193,9 @@ class PrivateStringMatcherImpl : public ValueMatcher, public StringMatcher {
207193
StringMatcherPtr custom_;
208194
};
209195

210-
// Temporarily create two separate types with different constructors, inheriting from the same
211-
// implementation, to make it easier to find and replace all usage of the old one.
212-
// TODO(ggreenway): delete these two extra classes, make `PrivateStringMatcherImpl` back into
213-
// `StringMatcherImpl`.
214-
template <class StringMatcherType = envoy::type::matcher::v3::StringMatcher>
215-
class StringMatcherImplWithContext : public PrivateStringMatcherImpl<StringMatcherType> {
216-
public:
217-
explicit StringMatcherImplWithContext(const StringMatcherType& matcher,
218-
Server::Configuration::CommonFactoryContext& context)
219-
: PrivateStringMatcherImpl<StringMatcherType>(matcher, &context.regexEngine(),
220-
&context.threadLocal(), &context.api()) {}
221-
};
222-
223-
template <class StringMatcherType = envoy::type::matcher::v3::StringMatcher>
224-
class StringMatcherImpl : public PrivateStringMatcherImpl<StringMatcherType> {
225-
public:
226-
explicit StringMatcherImpl(const StringMatcherType& matcher)
227-
: PrivateStringMatcherImpl<StringMatcherType>(
228-
matcher, Regex::EngineSingleton::getExisting(),
229-
InjectableSingleton<ThreadLocal::SlotAllocator>::getExisting(),
230-
InjectableSingleton<Api::Api>::getExisting()) {}
231-
};
232-
233196
class StringMatcherExtensionFactory : public Config::TypedFactory {
234197
public:
235-
// TODO(ggreenway): Convert all but first parameter to `CommonFactoryContext`.
236-
virtual StringMatcherPtr createStringMatcher(const ProtobufWkt::Any& config,
237-
ThreadLocal::SlotAllocator& tls, Api::Api& api) PURE;
198+
virtual StringMatcherPtr createStringMatcher(const ProtobufWkt::Any& config) PURE;
238199

239200
std::string category() const override { return "envoy.string_matcher"; }
240201
};

‎source/common/common/regex.h

-10
Original file line numberDiff line numberDiff line change
@@ -79,16 +79,6 @@ class Utility {
7979

8080
return EngineSingleton::get().matcher(matcher.regex());
8181
}
82-
83-
template <class RegexMatcherType>
84-
static CompiledMatcherPtr parseRegex(const RegexMatcherType& matcher, Engine& engine) {
85-
// Fallback deprecated engine type in regex matcher.
86-
if (matcher.has_google_re2()) {
87-
return std::make_unique<CompiledGoogleReMatcher>(matcher);
88-
}
89-
90-
return engine.matcher(matcher.regex());
91-
}
9282
};
9383

9484
} // namespace Regex

‎source/common/tls/cert_validator/default_validator.cc

+8-9
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,8 @@ namespace Tls {
4444

4545
DefaultCertValidator::DefaultCertValidator(
4646
const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
47-
Server::Configuration::CommonFactoryContext& context)
48-
: config_(config), stats_(stats), context_(context) {
47+
TimeSource& time_source)
48+
: config_(config), stats_(stats), time_source_(time_source) {
4949
if (config_ != nullptr) {
5050
allow_untrusted_certificate_ = config_->trustChainVerification() ==
5151
envoy::extensions::transport_sockets::tls::v3::
@@ -155,7 +155,7 @@ int DefaultCertValidator::initializeSslContexts(std::vector<SSL_CTX*> contexts,
155155
if (!cert_validation_config->subjectAltNameMatchers().empty()) {
156156
for (const envoy::extensions::transport_sockets::tls::v3::SubjectAltNameMatcher& matcher :
157157
cert_validation_config->subjectAltNameMatchers()) {
158-
auto san_matcher = createStringSanMatcher(matcher, context_);
158+
auto san_matcher = createStringSanMatcher(matcher);
159159
if (san_matcher == nullptr) {
160160
throwEnvoyExceptionOrPanic(
161161
absl::StrCat("Failed to create string SAN matcher of type ", matcher.san_type()));
@@ -548,19 +548,18 @@ Envoy::Ssl::CertificateDetailsPtr DefaultCertValidator::getCaCertInformation() c
548548
if (ca_cert_ == nullptr) {
549549
return nullptr;
550550
}
551-
return Utility::certificateDetails(ca_cert_.get(), getCaFileName(), context_.timeSource());
551+
return Utility::certificateDetails(ca_cert_.get(), getCaFileName(), time_source_);
552552
}
553553

554554
absl::optional<uint32_t> DefaultCertValidator::daysUntilFirstCertExpires() const {
555-
return Utility::getDaysUntilExpiration(ca_cert_.get(), context_.timeSource());
555+
return Utility::getDaysUntilExpiration(ca_cert_.get(), time_source_);
556556
}
557557

558558
class DefaultCertValidatorFactory : public CertValidatorFactory {
559559
public:
560-
CertValidatorPtr
561-
createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
562-
Server::Configuration::CommonFactoryContext& context) override {
563-
return std::make_unique<DefaultCertValidator>(config, stats, context);
560+
CertValidatorPtr createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config,
561+
SslStats& stats, TimeSource& time_source) override {
562+
return std::make_unique<DefaultCertValidator>(config, stats, time_source);
564563
}
565564

566565
std::string name() const override { return "envoy.tls.cert_validator.default"; }

‎source/common/tls/cert_validator/default_validator.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ namespace Tls {
3535
class DefaultCertValidator : public CertValidator, Logger::Loggable<Logger::Id::connection> {
3636
public:
3737
DefaultCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config,
38-
SslStats& stats, Server::Configuration::CommonFactoryContext& context);
38+
SslStats& stats, TimeSource& time_source);
3939

4040
~DefaultCertValidator() override = default;
4141

@@ -110,7 +110,7 @@ class DefaultCertValidator : public CertValidator, Logger::Loggable<Logger::Id::
110110

111111
const Envoy::Ssl::CertificateValidationContextConfig* config_;
112112
SslStats& stats_;
113-
Server::Configuration::CommonFactoryContext& context_;
113+
TimeSource& time_source_;
114114

115115
bool allow_untrusted_certificate_{false};
116116
bssl::UniquePtr<X509> ca_cert_;

‎source/common/tls/cert_validator/factory.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ class CertValidatorFactory : public Config::UntypedFactory {
2121
public:
2222
virtual CertValidatorPtr
2323
createCertValidator(const Envoy::Ssl::CertificateValidationContextConfig* config, SslStats& stats,
24-
Server::Configuration::CommonFactoryContext& context) PURE;
24+
TimeSource& time_source) PURE;
2525

2626
std::string category() const override { return "envoy.tls.cert_validator"; }
2727
};

0 commit comments

Comments
 (0)