server: add a CLI option to disable extensions by jpeach · Pull Request #9125 · envoyproxy/envoy

jpeach · 2019-11-25T10:55:20Z

Description:

Add a new --disable-extensions flag that hides the named extensions
from their corresponding factory registry. The extensions are retained
in the registry factory, but the factory pointer is nulled so that
configuration attempts to use the extensions will fail.

Risk Level: Medium
Testing: Added unit tests, manual verification.
Docs Changes: Added to CLI reference.
Release Notes: Added release note.
Fixes: #9096

Add a new `--disable-extensions` flag that hides the named extensions from their corresponding factory registry. The extensions are retained in the registry factory, but the factory pointer is nulled so that configuration attempts to use the extensions will fail. This fixes envoyproxy#9096. Signed-off-by: James Peach <jpeach@apache.org>

repokitteh-read-only · 2019-11-25T10:55:25Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9125 was opened by jpeach.

see: more, trace.

Signed-off-by: James Peach <jpeach@apache.org>

jpeach · 2019-11-25T20:27:55Z

[ RUN      ] IpVersions/UdpProxyIntegrationTest.HelloWorldOnLoopback/IPv6
external/envoy/test/extensions/filters/udp/udp_proxy/udp_proxy_integration_test.cc:95: Failure
Expected equality of these values:
  6
  test_server_->counter("udp.foo.downstream_sess_tx_bytes")->value()
    Which is: 0
Stack trace:
  0x11ec808: Envoy::(anonymous namespace)::UdpProxyIntegrationTest::requestResponseWithListenerAddress()
  0x11eb4dd: Envoy::(anonymous namespace)::UdpProxyIntegrationTest_HelloWorldOnLoopback_Test::TestBody()
  0x2c7b4b4: testing::internal::HandleSehExceptionsInMethodIfSupported<>()
  0x2c6c9be: testing::internal::HandleExceptionsInMethodIfSupported<>()
  0x2c57233: testing::Test::Run()
  0x2c57e1d: testing::TestInfo::Run()
... Google Test internal frames ...

[  FAILED  ] IpVersions/UdpProxyIntegrationTest.HelloWorldOnLoopback/IPv6, where GetParam() = 4-byte object <01-00 00-00> (160 ms)

/retest

repokitteh-read-only · 2019-11-25T20:27:58Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #9125 (comment) was created by @jpeach.

see: more, trace.

jpeach · 2019-11-25T23:27:06Z

I'm also looking for feedback on how to fully-qualify extension names. Should we do access_loggers.envoy.tcp_grpc_access_log, or something like access_loggers/envoy.tcp_grpc_access_log?

lizan · 2019-11-26T01:08:38Z

I'm also looking for feedback on how to fully-qualify extension names. Should we do access_loggers.envoy.tcp_grpc_access_log, or something like access_loggers/envoy.tcp_grpc_access_log?

+1 to the latter.

lizan · 2019-11-26T01:09:09Z

cc @yanavlasov @htuch, this will affect the reporting client capabilities too.

lizan · 2019-11-26T02:05:21Z

@Shikugawa do you mind take a pass?

Shikugawa · 2019-11-26T04:03:16Z

@lizan It looks good for me. But I think that disableFactory must be fixed after #9048 was merged to deal with if passed factory name was deprecated. Instead factory should not be nullptr even if we passed deprecated name as disabled factory.

lizan · 2019-11-26T05:11:00Z

include/envoy/registry/registry.h

+    if (it == factories().end()) {
+      return false;
+    }
+    it->second = nullptr;


Can we just erase the name from factories()?

I avoided that because there's no locking around the lookup. That should not be a problem in practice with the current usage, but it makes me nervous :)

Changing the value should make you as nervous as erase :) because they are not thread safe either.

IIUC it's safe to change the pointer. It has no effect on concurrent lookups and and caller either gets the pointer or gets null. Since we don't deallocate the factory, either is OK. It's racy but not unsafe.

source/server/server.cc

source/server/server.h

jpeach · 2019-11-26T05:58:21Z

But I think that disableFactory must be fixed after #9048 was merged to deal with if passed factory name was deprecated.

IIUC, there's 2 options here.

When an extension is disabled, it is disabled for all its possible names
Only the given extension name is disabled, other names continue to resolve.

lizan · 2019-11-26T19:44:04Z

When an extension is disabled, it is disabled for all its possible names

Only the given extension name is disabled, other names continue to resolve.

I prefer 1.

jpeach · 2019-11-26T20:00:03Z

On Nov 27, 2019, at 6:44 AM, Lizan Zhou ***@***.***> wrote: When an extension is disabled, it is disabled for all its possible names Only the given extension name is disabled, other names continue to resolve. I prefer 1

Haha, I was leaning towards (2) so that you can disable just the deprecated names to test upgrade paths. Maybe a `—disable-extensions=deprecated”?

lizan · 2019-11-26T20:08:49Z

Those will be tested in DEPRECATED_FEATURE_TEST in the compile time options. It seems more confusing by doing 2 and that doesn't provide security protection either.

Signed-off-by: James Peach <jpeach@apache.org>

When an extension is disabled, use the deprecated name tracking to disable it by all its possible names. Signed-off-by: James Peach <jpeach@apache.org>

Signed-off-by: James Peach <jpeach@apache.org>

repokitteh-read-only · 2019-12-02T03:19:49Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #9125 was synchronize by jpeach.

see: more, trace.

jpeach · 2019-12-02T10:34:50Z

@lizan Updated with review feedback.

lizan

LGTM, just nits.

include/envoy/registry/registry.h

Signed-off-by: James Peach <jpeach@apache.org>

jpeach · 2019-12-11T23:52:13Z

@lizan WDYT? I'd prefer to leave the lambda and disable deletion as it, but happy to discuss further.

*Description:* Add a new `--disable-extensions` flag that hides the named extensions from their corresponding factory registry. The extensions are retained in the registry factory, but the factory pointer is nulled so that configuration attempts to use the extensions will fail. *Risk Level:* Medium *Testing:* Added unit tests, manual verification. *Docs Changes:* Added to CLI reference. *Release Notes:* Added release note. *Fixes:* envoyproxy#9096 Signed-off-by: James Peach <jpeach@apache.org> Signed-off-by: Prakhar <prakhar_au@yahoo.com>

repokitteh-read-only bot added the api label Nov 25, 2019

zuercher requested review from junr03 and removed request for junr03 November 25, 2019 18:08

zuercher self-assigned this Nov 25, 2019

Fix CI format checks.

ca80a4b

Signed-off-by: James Peach <jpeach@apache.org>

lizan suggested changes Nov 26, 2019

View reviewed changes

zuercher assigned lizan and unassigned zuercher Nov 26, 2019

jpeach added 4 commits December 2, 2019 09:22

Merge branch 'master' into disable-extensions-at-startup

6bd9c47

Signed-off-by: James Peach <jpeach@apache.org>

registry: disable all registered factory names

9a33e81

When an extension is disabled, use the deprecated name tracking to disable it by all its possible names. Signed-off-by: James Peach <jpeach@apache.org>

server: move extension disabling code to OptionsImpl

194b63d

Signed-off-by: James Peach <jpeach@apache.org>

server: change extensions category separator to '/'

7c2884f

Signed-off-by: James Peach <jpeach@apache.org>

yanavlasov previously approved these changes Dec 4, 2019

View reviewed changes

lizan reviewed Dec 4, 2019

View reviewed changes

include/envoy/registry/registry.h Outdated Show resolved Hide resolved

include/envoy/registry/registry.h Outdated Show resolved Hide resolved

Merge branch 'master' into disable-extensions-at-startup

834d85c

Signed-off-by: James Peach <jpeach@apache.org>

Fix review feedback.

21064a4

Signed-off-by: James Peach <jpeach@apache.org>

jpeach dismissed yanavlasov’s stale review via 21064a4 December 6, 2019 02:19

jpeach mentioned this pull request Dec 11, 2019

API: Add extension information the Node message. #9301

Merged

lizan approved these changes Dec 13, 2019

View reviewed changes

repokitteh-read-only bot removed the api label Dec 13, 2019

lizan merged commit 445d0ee into envoyproxy:master Dec 13, 2019

mattklein123 mentioned this pull request Dec 13, 2019

Add a flag to disable extensions at startup. #9096

Closed

jpeach mentioned this pull request Dec 17, 2019

Envoy emits a bogus "failed to disable extension" message at startup #9377

Closed

jmuia mentioned this pull request Jan 4, 2020

Multiple circuit breakers emit the same stat #9392

Closed

Conversation

jpeach commented Nov 25, 2019

Uh oh!

repokitteh-read-only bot commented Nov 25, 2019

Uh oh!

jpeach commented Nov 25, 2019

Uh oh!

repokitteh-read-only bot commented Nov 25, 2019

Uh oh!

jpeach commented Nov 25, 2019

Uh oh!

lizan commented Nov 26, 2019

Uh oh!

lizan commented Nov 26, 2019

Uh oh!

lizan commented Nov 26, 2019

Uh oh!

Shikugawa commented Nov 26, 2019

Uh oh!

lizan Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

jpeach Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

lizan Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

jpeach Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jpeach commented Nov 26, 2019

Uh oh!

lizan commented Nov 26, 2019

Uh oh!

jpeach commented Nov 26, 2019 via email

Uh oh!

lizan commented Nov 26, 2019

Uh oh!

repokitteh-read-only bot commented Dec 2, 2019

Uh oh!

jpeach commented Dec 2, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jpeach commented Dec 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants