-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
rds: validate config in depth before update config dump #7956
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -493,6 +493,93 @@ TEST_F(RouteConfigProviderManagerImplTest, onConfigUpdateWrongSize) { | |
EnvoyException, "Unexpected RDS resource length: 2"); | ||
} | ||
|
||
// Regression test for https://github.com/envoyproxy/envoy/issues/7939 | ||
TEST_F(RouteConfigProviderManagerImplTest, ConfigDumpAfterConfigRejected) { | ||
auto message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["routes"](); | ||
const auto& route_config_dump = | ||
MessageUtil::downcastAndValidate<const envoy::admin::v2alpha::RoutesConfigDump&>( | ||
*message_ptr); | ||
|
||
// No routes at all, no last_updated timestamp | ||
envoy::admin::v2alpha::RoutesConfigDump expected_route_config_dump; | ||
TestUtility::loadFromYaml(R"EOF( | ||
static_route_configs: | ||
dynamic_route_configs: | ||
)EOF", | ||
expected_route_config_dump); | ||
EXPECT_EQ(expected_route_config_dump.DebugString(), route_config_dump.DebugString()); | ||
|
||
timeSystem().setSystemTime(std::chrono::milliseconds(1234567891234)); | ||
|
||
// dynamic. | ||
setup(); | ||
EXPECT_CALL(*factory_context_.cluster_manager_.subscription_factory_.subscription_, start(_)); | ||
factory_context_.init_manager_.initialize(init_watcher_); | ||
|
||
const std::string response1_json = R"EOF( | ||
{ | ||
"version_info": "1", | ||
"resources": [ | ||
{ | ||
"@type": "type.googleapis.com/envoy.api.v2.RouteConfiguration", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you use YAML for examples? it's more concise and the canonical text proto representation in Envoy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
"name": "foo_route_config", | ||
"virtual_hosts": [ | ||
{ | ||
"name": "integration", | ||
"domains": [ | ||
"*" | ||
], | ||
"routes": [ | ||
{ | ||
"match": { | ||
"prefix": "/foo" | ||
}, | ||
"route": { | ||
"cluster_header": ":authority" | ||
} | ||
} | ||
] | ||
}, | ||
{ | ||
"name": "duplicate", | ||
"domains": [ | ||
"*" | ||
], | ||
"routes": [ | ||
{ | ||
"match": { | ||
"prefix": "/foo" | ||
}, | ||
"route": { | ||
"cluster_header": ":authority" | ||
} | ||
} | ||
] | ||
} | ||
] | ||
} | ||
] | ||
} | ||
)EOF"; | ||
auto response1 = TestUtility::parseYaml<envoy::api::v2::DiscoveryResponse>(response1_json); | ||
|
||
EXPECT_CALL(init_watcher_, ready()); | ||
|
||
EXPECT_THROW(rds_callbacks_->onConfigUpdate(response1.resources(), response1.version_info()), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you explain why we should expect a throw here? I.e. what's throw-worthy in the config? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
EnvoyException); | ||
|
||
message_ptr = factory_context_.admin_.config_tracker_.config_tracker_callbacks_["routes"](); | ||
const auto& route_config_dump3 = | ||
MessageUtil::downcastAndValidate<const envoy::admin::v2alpha::RoutesConfigDump&>( | ||
*message_ptr); | ||
TestUtility::loadFromYaml(R"EOF( | ||
static_route_configs: | ||
dynamic_route_configs: | ||
)EOF", | ||
expected_route_config_dump); | ||
EXPECT_EQ(expected_route_config_dump.DebugString(), route_config_dump3.DebugString()); | ||
} | ||
|
||
} // namespace | ||
} // namespace Router | ||
} // namespace Envoy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenzzzz FYI, keep in mind we need to retain this behavior in the world of SRDS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG. This is another reason RDS should move to the config-provider-framework. Where RDSSubscription holds a single RDSProvider and this sort of validation(ConfigImpl instantiation) happens before update propagation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevenzzzz This is not the case because RDS is being used in multiple context and each context needs its own provider, that's what we address here. We may want to limit that flexibility but it will be hard to do so without breaking API compatibility.