http: check virtual host-specific typed per filter config by soulxu · Pull Request #15135 · envoyproxy/envoy

soulxu · 2021-02-22T13:15:23Z

Commit Message: http: check virtual host-specific typed per filter config
Additional Description:
When filter doesn't support virtual host-specific typed per filter config,
the envoy should reject any per filter config for that filter.

Also, adds new runtime config envoy.reloadable_features.check_unsupported_typed_per_filter_config
for backward-compatible. The default value is true, only when the value is true,
the check will be executed.

Risk Level: low
Testing: unit test added
Docs Changes: n/a
Release Notes: note new feature
Runtime guard: envoy.reloadable_features.check_unsupported_typed_per_filter_config
Fixes #15043

Signed-off-by: He Jie Xu hejie.xu@intel.com

When filter doesn't support virtual host-specific typed per filter config, the envoy should reject any per filter config for that filter. Also, adds new runtime config `envoy.reloadable_features.check_unsupported_typed_per_filter_config` for backward-compatible. The default value is false, only whe value is true, the check will be executed. Risk Level: low Testing: unit tests added Doc Changes: n/a Release Notes: note new feature Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only · 2021-02-22T13:15:26Z

Hi @soulxu, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #15135 was opened by soulxu.

see: more, trace.

source/extensions/filters/http/common/factory_base.h

test/extensions/filters/http/common/factory_base_test.cc

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

antoniovicente

Welcome to Envoy and thanks for the improvement to config validation. I'm not that familiar with all the interactions between config components and object factories, so hopefully @htuch can help fill in my gaps in understanding.

antoniovicente · 2021-02-23T01:14:21Z

tools/code_format/check_format.py

 # processing.
-EXCEPTION_ALLOWLIST = ("./source/common/config/utility.h")
+EXCEPTION_ALLOWLIST = ("./source/common/config/utility.h",
+                       "./source/extensions/filters/http/common/factory_base.h")


@ahedberg @htuch

We may have some problems here depending on the intent behind this check. It seems to me that headers in this list may need to be restricted so they are not included from other headers in other to prevent multiple definitions in builds that partially disable exceptions at a per-library basis. That said, there seems to be no restrictions in place that control where source/common/config/utility.h is included from or the use of those functions in header files.

This PR introduces an exception that is thrown from a base class method of a class that seems to be widely used. I think this change could break compilation of libraries that disable exceptions.

Another option is we can fix it here

envoy/source/common/router/config_impl.cc

Lines 1492 to 1502 in b23ee3b

for (const auto& it : typed_configs) {

// TODO(zuercher): canonicalization may be removed when deprecated filter names are removed

const auto& name =

Extensions::HttpFilters::Common::FilterNameUtil::canonicalFilterName(it.first);

auto object = createRouteSpecificFilterConfig(

name, it.second, ProtobufWkt::Struct::default_instance(), factory_context, validator);

if (object != nullptr) {

configs_[name] = std::move(object);

}

}

If we decide to have createRouteSpecificFilterConfigTyped throw an exception, the throw statement should be in the .cc file instead of the .h file. Then we don't need to add it to the exception allowlist.

FactoryBase is a template class, the throw can't be moved to a cc file (well, except possibly by adding a helper function)

Returning null and doing the validation in config_impl.cc seems attractive. Is that the only place where this factory method is called from?

Returning null and doing the validation in config_impl.cc seems attractive. Is that the only place where this factory method is called from?

Yes, that is the only place I found at least.

I think config_impl.cc is more robust in any case, since regardless of inheritance hierarchy, if there is no config, we can fire the exception.

It would be really helpful to some of us if exceptions were only thrown from cc files. Could you do the new config validation in config_impl.cc instead of the template factory class?

Yes, I will update, thanks for the feedback!

antoniovicente · 2021-02-23T01:15:35Z

source/common/runtime/runtime_features.cc

    // Sentinel and test flag.
    "envoy.reloadable_features.test_feature_false",
+    // Reject virtual host-specific configuration when filter doesn't support it.
+    "envoy.reloadable_features.check_unsupported_typed_per_filter_config",


My understanding is that runtime features should default to true. They provide a way for devs affected by a breaking change to temporarily rollback the behavior. Once we have given time to find and address all the breakages, we'll remove the runtime feature checks and only keep the new behavior.

Out of curiosity, are there any existing tests that fail when this runtime feature is enabled?

My understanding is that runtime features should default to true. They provide a way for devs affected by a breaking change to temporarily rollback the behavior. Once we have given time to find and address all the breakages, we'll remove the runtime feature checks and only keep the new behavior.

As https://github.com/envoyproxy/envoy/issues/15043#issuecomment-778703531 said, I thought we should set ti as false default, then it won't break existing user. But I may miss understanding that.

Out of curiosity, are there any existing tests that fail when this runtime feature is enabled?

I tested with the default value is true. In my env, only get one test failed 'IoSocketHandleImplIntegration.LastRoundTripIntegrationTest', I don't think it is related.

cc @htuch

Having the runtime flag so affected devs can disable the functionality while they fixed their config should be enough. I think that the default should be true.

And I agree that the 'IoSocketHandleImplIntegration.LastRoundTripIntegrationTest' failure is not related to this change. That's a non-hermetic test that does network communication. I think I'll file an issue about it.

I think default true is fine, as long as we have the runtime for now.

cc @htuch

Having the runtime flag so affected devs can disable the functionality while they fixed their config should be enough. I think that the default should be true.

And I agree that the 'IoSocketHandleImplIntegration.LastRoundTripIntegrationTest' failure is not related to this change. That's a non-hermetic test that does network communication. I think I'll file an issue about it.

let me change the default value as true, thanks.

antoniovicente · 2021-02-23T01:33:29Z

test/extensions/filters/http/common/factory_base_test.cc

+  ProtobufTypes::MessagePtr config = factory.createEmptyRouteConfigProto();
+
+  EXPECT_EQ(nullptr, factory.createRouteSpecificFilterConfig(
+                         *config, context, ProtobufMessage::getNullValidationVisitor()));


It would be good to have some e2e integration tests that cover this functionality. I don't know exactly what existing types would fail config load if you tried to provide per-route config for them. It seems that Envoy::Extensions::HttpFilters::HealthCheck::HealthCheckFilterConfig doesn't implement an override for createRouteSpecificFilterConfigTyped, perhaps that is a good extension to write a test against?

Yea, HealthCheckFilterConfig doesn't implement override, let me add integration tests.

antoniovicente · 2021-02-23T01:42:23Z

source/extensions/filters/http/common/factory_base.h

+              "The filter {} doesn't support virtual host-specific configurations. Set runtime "
+              "config `envoy.reloadable_features.check_unsupported_typed_per_filter_config` as "
+              "true to reject any invalid virtual-host specific configuration.",
+              name())


The continuous integration test run detected a build breakage on this line due to missing ; at the end of the line.

/wait

Build still looks broken. Did you implement a fix but didn't "git push" it out?

htuch

Thanks for taking this on!

htuch · 2021-02-24T02:29:19Z

source/common/runtime/runtime_features.cc

    // Sentinel and test flag.
    "envoy.reloadable_features.test_feature_false",
+    // Reject virtual host-specific configuration when filter doesn't support it.
+    "envoy.reloadable_features.check_unsupported_typed_per_filter_config",


I think default true is fine, as long as we have the runtime for now.

htuch · 2021-02-24T02:32:32Z

tools/code_format/check_format.py

 # processing.
-EXCEPTION_ALLOWLIST = ("./source/common/config/utility.h")
+EXCEPTION_ALLOWLIST = ("./source/common/config/utility.h",
+                       "./source/extensions/filters/http/common/factory_base.h")


I think config_impl.cc is more robust in any case, since regardless of inheritance hierarchy, if there is no config, we can fire the exception.

htuch · 2021-02-24T02:33:13Z

@markdroth does this match your intended behavior?

markdroth · 2021-02-26T17:10:17Z

Yes, this looks right to me. Thanks for doing this!

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

antoniovicente · 2021-03-01T22:31:56Z

/wait

Please look into the build breakages.

soulxu · 2021-03-02T00:07:22Z

thanks for all the review! let me work on the update.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

htuch

LGTM, thanks!

soulxu mentioned this pull request Feb 22, 2021

api: Envoy should NACK typed_per_filter_config entries for filters that don't support override configs #15043

Closed

junr03 assigned htuch and antoniovicente Feb 22, 2021

dmitri-d reviewed Feb 22, 2021

View reviewed changes

source/extensions/filters/http/common/factory_base.h Outdated Show resolved Hide resolved

dmitri-d reviewed Feb 22, 2021

View reviewed changes

test/extensions/filters/http/common/factory_base_test.cc Outdated Show resolved Hide resolved

address comments

7a92b45

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

antoniovicente reviewed Feb 23, 2021

View reviewed changes

htuch reviewed Feb 24, 2021

View reviewed changes

antoniovicente mentioned this pull request Feb 24, 2021

test: io_socket_handle_impl_integration_test.cc contains non-hermetic tests that depend on external network connectivity #15164

Closed

antoniovicente added the waiting label Feb 24, 2021

Revert factory base change

4085ba6

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

soulxu added 2 commits March 2, 2021 10:20

Add check at PerFilterConfig and add unittest

b1165d7

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

add integration test

a45cca1

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

repokitteh-read-only bot removed the waiting label Mar 2, 2021

update release note

7cd625e

Signed-off-by: He Jie Xu <hejie.xu@intel.com>

htuch approved these changes Mar 2, 2021

View reviewed changes

htuch merged commit 0ab78c7 into envoyproxy:main Mar 2, 2021

alyssawilk mentioned this pull request Oct 5, 2021

envoy.reloadable_features.check_unsupported_typed_per_filter_config deprecation #18433

Closed

	for (const auto& it : typed_configs) {
	// TODO(zuercher): canonicalization may be removed when deprecated filter names are removed
	const auto& name =
	Extensions::HttpFilters::Common::FilterNameUtil::canonicalFilterName(it.first);

	auto object = createRouteSpecificFilterConfig(
	name, it.second, ProtobufWkt::Struct::default_instance(), factory_context, validator);
	if (object != nullptr) {
	configs_[name] = std::move(object);
	}
	}

Conversation

soulxu commented Feb 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Feb 22, 2021

Uh oh!

Uh oh!

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Feb 24, 2021

Uh oh!

markdroth commented Feb 26, 2021

Uh oh!

antoniovicente commented Mar 1, 2021

Uh oh!

soulxu commented Mar 2, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

soulxu commented Feb 22, 2021 •

edited

Loading