diff --git a/src/core/ext/xds/xds_api.cc b/src/core/ext/xds/xds_api.cc index fddd32e50b1..1d57802b90c 100644 --- a/src/core/ext/xds/xds_api.cc +++ b/src/core/ext/xds/xds_api.cc @@ -116,17 +116,6 @@ bool XdsAggregateAndLogicalDnsClusterEnabled() { return parse_succeeded && parsed_value; } -// TODO(yashykt): Check to see if xDS security is enabled. This will be -// removed once this feature is fully integration-tested and enabled by -// default. -bool XdsSecurityEnabled() { - char* value = gpr_getenv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT"); - bool parsed_value; - bool parse_succeeded = gpr_parse_bool_value(value, &parsed_value); - gpr_free(value); - return parse_succeeded && parsed_value; -} - // // XdsApi::Route::HashPolicy // @@ -2583,17 +2572,13 @@ grpc_error_handle FilterChainParse( } } } - // Get the DownstreamTlsContext for the filter chain - if (XdsSecurityEnabled()) { - auto* transport_socket = - envoy_config_listener_v3_FilterChain_transport_socket( - filter_chain_proto); - if (transport_socket != nullptr) { - grpc_error_handle error = DownstreamTlsContextParse( - context, transport_socket, - &filter_chain->filter_chain_data->downstream_tls_context); - if (error != GRPC_ERROR_NONE) errors.push_back(error); - } + auto* transport_socket = + envoy_config_listener_v3_FilterChain_transport_socket(filter_chain_proto); + if (transport_socket != nullptr) { + grpc_error_handle error = DownstreamTlsContextParse( + context, transport_socket, + &filter_chain->filter_chain_data->downstream_tls_context); + if (error != GRPC_ERROR_NONE) errors.push_back(error); } return GRPC_ERROR_CREATE_FROM_VECTOR("Error parsing FilterChain", &errors); } @@ -3138,18 +3123,16 @@ grpc_error_handle CdsResourceParse( errors.push_back( GRPC_ERROR_CREATE_FROM_STATIC_STRING("LB policy is not supported.")); } - if (XdsSecurityEnabled()) { - auto* transport_socket = - envoy_config_cluster_v3_Cluster_transport_socket(cluster); - if (transport_socket != nullptr) { - grpc_error_handle error = UpstreamTlsContextParse( - context, transport_socket, &cds_update->common_tls_context); - if (error != GRPC_ERROR_NONE) { - errors.push_back( - grpc_error_add_child(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "Error parsing security configuration"), - error)); - } + auto* transport_socket = + envoy_config_cluster_v3_Cluster_transport_socket(cluster); + if (transport_socket != nullptr) { + grpc_error_handle error = UpstreamTlsContextParse( + context, transport_socket, &cds_update->common_tls_context); + if (error != GRPC_ERROR_NONE) { + errors.push_back( + grpc_error_add_child(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "Error parsing security configuration"), + error)); } } // Record LRS server name (if any). diff --git a/src/core/ext/xds/xds_api.h b/src/core/ext/xds/xds_api.h index 355f3a6fa57..50d8b7ecda3 100644 --- a/src/core/ext/xds/xds_api.h +++ b/src/core/ext/xds/xds_api.h @@ -42,11 +42,6 @@ namespace grpc_core { -// TODO(yashykt): Check to see if xDS security is enabled. This will be -// removed once this feature is fully integration-tested and enabled by -// default. -bool XdsSecurityEnabled(); - class XdsClient; class XdsApi { diff --git a/src/core/ext/xds/xds_bootstrap.cc b/src/core/ext/xds/xds_bootstrap.cc index 3d227f54361..1f619d6d735 100644 --- a/src/core/ext/xds/xds_bootstrap.cc +++ b/src/core/ext/xds/xds_bootstrap.cc @@ -132,16 +132,14 @@ XdsBootstrap::XdsBootstrap(Json json, grpc_error_handle* error) { std::move(*it->second.mutable_string_value()); } } - if (XdsSecurityEnabled()) { - it = json.mutable_object()->find("certificate_providers"); - if (it != json.mutable_object()->end()) { - if (it->second.type() != Json::Type::OBJECT) { - error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( - "\"certificate_providers\" field is not an object")); - } else { - grpc_error_handle parse_error = ParseCertificateProviders(&it->second); - if (parse_error != GRPC_ERROR_NONE) error_list.push_back(parse_error); - } + it = json.mutable_object()->find("certificate_providers"); + if (it != json.mutable_object()->end()) { + if (it->second.type() != Json::Type::OBJECT) { + error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( + "\"certificate_providers\" field is not an object")); + } else { + grpc_error_handle parse_error = ParseCertificateProviders(&it->second); + if (parse_error != GRPC_ERROR_NONE) error_list.push_back(parse_error); } } *error = GRPC_ERROR_CREATE_FROM_VECTOR("errors parsing xds bootstrap file", diff --git a/test/core/xds/xds_bootstrap_test.cc b/test/core/xds/xds_bootstrap_test.cc index 55b94fd2f30..d007780b3fd 100644 --- a/test/core/xds/xds_bootstrap_test.cc +++ b/test/core/xds/xds_bootstrap_test.cc @@ -34,41 +34,9 @@ namespace grpc_core { namespace testing { +namespace { -class TestType { - public: - explicit TestType(bool parse_xds_certificate_providers) - : parse_xds_certificate_providers_(parse_xds_certificate_providers) {} - - bool parse_xds_certificate_providers() const { - return parse_xds_certificate_providers_; - } - - std::string AsString() const { - return parse_xds_certificate_providers_ - ? "WithCertificateProvidersParsing" - : "WithoutCertificateProvidersParsing"; - } - - private: - const bool parse_xds_certificate_providers_; -}; - -class XdsBootstrapTest : public ::testing::TestWithParam { - public: - XdsBootstrapTest() { - if (GetParam().parse_xds_certificate_providers()) { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT", "true"); - } else { - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT"); - } - grpc_init(); - } - - ~XdsBootstrapTest() override { grpc_shutdown_blocking(); } -}; - -TEST_P(XdsBootstrapTest, Basic) { +TEST(XdsBootstrapTest, Basic) { const char* json_str = "{" " \"xds_servers\": [" @@ -146,7 +114,7 @@ TEST_P(XdsBootstrapTest, Basic) { "example/resource"); } -TEST_P(XdsBootstrapTest, ValidWithoutNode) { +TEST(XdsBootstrapTest, ValidWithoutNode) { const char* json_str = "{" " \"xds_servers\": [" @@ -166,7 +134,7 @@ TEST_P(XdsBootstrapTest, ValidWithoutNode) { EXPECT_EQ(bootstrap.node(), nullptr); } -TEST_P(XdsBootstrapTest, InsecureCreds) { +TEST(XdsBootstrapTest, InsecureCreds) { const char* json_str = "{" " \"xds_servers\": [" @@ -186,7 +154,7 @@ TEST_P(XdsBootstrapTest, InsecureCreds) { EXPECT_EQ(bootstrap.node(), nullptr); } -TEST_P(XdsBootstrapTest, GoogleDefaultCreds) { +TEST(XdsBootstrapTest, GoogleDefaultCreds) { // Generate call creds file needed by GoogleDefaultCreds. const char token_str[] = "{ \"client_id\": \"32555999999.apps.googleusercontent.com\"," @@ -222,7 +190,7 @@ TEST_P(XdsBootstrapTest, GoogleDefaultCreds) { EXPECT_EQ(bootstrap.node(), nullptr); } -TEST_P(XdsBootstrapTest, MissingChannelCreds) { +TEST(XdsBootstrapTest, MissingChannelCreds) { const char* json_str = "{" " \"xds_servers\": [" @@ -240,7 +208,7 @@ TEST_P(XdsBootstrapTest, MissingChannelCreds) { GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, NoKnownChannelCreds) { +TEST(XdsBootstrapTest, NoKnownChannelCreds) { const char* json_str = "{" " \"xds_servers\": [" @@ -260,7 +228,7 @@ TEST_P(XdsBootstrapTest, NoKnownChannelCreds) { GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, MissingXdsServers) { +TEST(XdsBootstrapTest, MissingXdsServers) { grpc_error_handle error = GRPC_ERROR_NONE; Json json = Json::Parse("{}", &error); ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); @@ -270,7 +238,7 @@ TEST_P(XdsBootstrapTest, MissingXdsServers) { GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, TopFieldsWrongTypes) { +TEST(XdsBootstrapTest, TopFieldsWrongTypes) { const char* json_str = "{" " \"xds_servers\":1," @@ -287,15 +255,13 @@ TEST_P(XdsBootstrapTest, TopFieldsWrongTypes) { "\"node\" field is not an object.*" "\"server_listener_resource_name_" "template\" field is not a string.*")); - if (GetParam().parse_xds_certificate_providers()) { - EXPECT_THAT(grpc_error_std_string(error), - ::testing::ContainsRegex( - "\"certificate_providers\" field is not an object")); - } + EXPECT_THAT(grpc_error_std_string(error), + ::testing::ContainsRegex( + "\"certificate_providers\" field is not an object")); GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, XdsServerMissingServerUri) { +TEST(XdsBootstrapTest, XdsServerMissingServerUri) { const char* json_str = "{" " \"xds_servers\":[{}]" @@ -311,7 +277,7 @@ TEST_P(XdsBootstrapTest, XdsServerMissingServerUri) { GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, XdsServerUriAndCredsWrongTypes) { +TEST(XdsBootstrapTest, XdsServerUriAndCredsWrongTypes) { const char* json_str = "{" " \"xds_servers\":[" @@ -334,7 +300,7 @@ TEST_P(XdsBootstrapTest, XdsServerUriAndCredsWrongTypes) { GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, ChannelCredsFieldsWrongTypes) { +TEST(XdsBootstrapTest, ChannelCredsFieldsWrongTypes) { const char* json_str = "{" " \"xds_servers\":[" @@ -364,7 +330,7 @@ TEST_P(XdsBootstrapTest, ChannelCredsFieldsWrongTypes) { GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, NodeFieldsWrongTypes) { +TEST(XdsBootstrapTest, NodeFieldsWrongTypes) { const char* json_str = "{" " \"node\":{" @@ -387,7 +353,7 @@ TEST_P(XdsBootstrapTest, NodeFieldsWrongTypes) { GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, LocalityFieldsWrongType) { +TEST(XdsBootstrapTest, LocalityFieldsWrongType) { const char* json_str = "{" " \"node\":{" @@ -411,7 +377,7 @@ TEST_P(XdsBootstrapTest, LocalityFieldsWrongType) { GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, CertificateProvidersElementWrongType) { +TEST(XdsBootstrapTest, CertificateProvidersElementWrongType) { const char* json_str = "{" " \"xds_servers\": [" @@ -428,18 +394,14 @@ TEST_P(XdsBootstrapTest, CertificateProvidersElementWrongType) { Json json = Json::Parse(json_str, &error); ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); XdsBootstrap bootstrap(std::move(json), &error); - if (GetParam().parse_xds_certificate_providers()) { - EXPECT_THAT(grpc_error_std_string(error), - ::testing::ContainsRegex( - "errors parsing \"certificate_providers\" object.*" - "element \"plugin\" is not an object")); - } else { - EXPECT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); - } + EXPECT_THAT(grpc_error_std_string(error), + ::testing::ContainsRegex( + "errors parsing \"certificate_providers\" object.*" + "element \"plugin\" is not an object")); GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, CertificateProvidersPluginNameWrongType) { +TEST(XdsBootstrapTest, CertificateProvidersPluginNameWrongType) { const char* json_str = "{" " \"xds_servers\": [" @@ -458,19 +420,15 @@ TEST_P(XdsBootstrapTest, CertificateProvidersPluginNameWrongType) { Json json = Json::Parse(json_str, &error); ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); XdsBootstrap bootstrap(std::move(json), &error); - if (GetParam().parse_xds_certificate_providers()) { - EXPECT_THAT(grpc_error_std_string(error), - ::testing::ContainsRegex( - "errors parsing \"certificate_providers\" object.*" - "errors parsing element \"plugin\".*" - "\"plugin_name\" field is not a string")); - } else { - EXPECT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); - } + EXPECT_THAT(grpc_error_std_string(error), + ::testing::ContainsRegex( + "errors parsing \"certificate_providers\" object.*" + "errors parsing element \"plugin\".*" + "\"plugin_name\" field is not a string")); GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, CertificateProvidersUnrecognizedPluginName) { +TEST(XdsBootstrapTest, CertificateProvidersUnrecognizedPluginName) { const char* json_str = "{" " \"xds_servers\": [" @@ -489,15 +447,11 @@ TEST_P(XdsBootstrapTest, CertificateProvidersUnrecognizedPluginName) { Json json = Json::Parse(json_str, &error); ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); XdsBootstrap bootstrap(std::move(json), &error); - if (GetParam().parse_xds_certificate_providers()) { - EXPECT_THAT(grpc_error_std_string(error), - ::testing::ContainsRegex( - "errors parsing \"certificate_providers\" object.*" - "errors parsing element \"plugin\".*" - "Unrecognized plugin name: unknown")); - } else { - EXPECT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); - } + EXPECT_THAT(grpc_error_std_string(error), + ::testing::ContainsRegex( + "errors parsing \"certificate_providers\" object.*" + "errors parsing element \"plugin\".*" + "Unrecognized plugin name: unknown")); GRPC_ERROR_UNREF(error); } @@ -550,9 +504,7 @@ class FakeCertificateProviderFactory : public CertificateProviderFactory { } }; -TEST_P(XdsBootstrapTest, CertificateProvidersFakePluginParsingError) { - CertificateProviderRegistry::RegisterCertificateProviderFactory( - absl::make_unique()); +TEST(XdsBootstrapTest, CertificateProvidersFakePluginParsingError) { const char* json_str = "{" " \"xds_servers\": [" @@ -574,21 +526,15 @@ TEST_P(XdsBootstrapTest, CertificateProvidersFakePluginParsingError) { Json json = Json::Parse(json_str, &error); ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); XdsBootstrap bootstrap(std::move(json), &error); - if (GetParam().parse_xds_certificate_providers()) { - EXPECT_THAT(grpc_error_std_string(error), - ::testing::ContainsRegex( - "errors parsing \"certificate_providers\" object.*" - "errors parsing element \"fake_plugin\".*" - "field:config field:value not of type number")); - } else { - EXPECT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); - } + EXPECT_THAT(grpc_error_std_string(error), + ::testing::ContainsRegex( + "errors parsing \"certificate_providers\" object.*" + "errors parsing element \"fake_plugin\".*" + "field:config field:value not of type number")); GRPC_ERROR_UNREF(error); } -TEST_P(XdsBootstrapTest, CertificateProvidersFakePluginParsingSuccess) { - CertificateProviderRegistry::RegisterCertificateProviderFactory( - absl::make_unique()); +TEST(XdsBootstrapTest, CertificateProvidersFakePluginParsingSuccess) { const char* json_str = "{" " \"xds_servers\": [" @@ -611,24 +557,17 @@ TEST_P(XdsBootstrapTest, CertificateProvidersFakePluginParsingSuccess) { ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); XdsBootstrap bootstrap(std::move(json), &error); ASSERT_TRUE(error == GRPC_ERROR_NONE) << grpc_error_std_string(error); - if (GetParam().parse_xds_certificate_providers()) { - const CertificateProviderStore::PluginDefinition& fake_plugin = - bootstrap.certificate_providers().at("fake_plugin"); - ASSERT_EQ(fake_plugin.plugin_name, "fake"); - ASSERT_STREQ(fake_plugin.config->name(), "fake"); - ASSERT_EQ( - static_cast>( - fake_plugin.config) - ->value(), - 10); - } else { - EXPECT_TRUE(bootstrap.certificate_providers().empty()); - } + const CertificateProviderStore::PluginDefinition& fake_plugin = + bootstrap.certificate_providers().at("fake_plugin"); + ASSERT_EQ(fake_plugin.plugin_name, "fake"); + ASSERT_STREQ(fake_plugin.config->name(), "fake"); + ASSERT_EQ(static_cast>( + fake_plugin.config) + ->value(), + 10); } -TEST_P(XdsBootstrapTest, CertificateProvidersFakePluginEmptyConfig) { - CertificateProviderRegistry::RegisterCertificateProviderFactory( - absl::make_unique()); +TEST(XdsBootstrapTest, CertificateProvidersFakePluginEmptyConfig) { const char* json_str = "{" " \"xds_servers\": [" @@ -648,35 +587,27 @@ TEST_P(XdsBootstrapTest, CertificateProvidersFakePluginEmptyConfig) { ASSERT_EQ(error, GRPC_ERROR_NONE) << grpc_error_std_string(error); XdsBootstrap bootstrap(std::move(json), &error); ASSERT_TRUE(error == GRPC_ERROR_NONE) << grpc_error_std_string(error); - if (GetParam().parse_xds_certificate_providers()) { - const CertificateProviderStore::PluginDefinition& fake_plugin = - bootstrap.certificate_providers().at("fake_plugin"); - ASSERT_EQ(fake_plugin.plugin_name, "fake"); - ASSERT_STREQ(fake_plugin.config->name(), "fake"); - ASSERT_EQ( - static_cast>( - fake_plugin.config) - ->value(), - 0); - } else { - EXPECT_TRUE(bootstrap.certificate_providers().empty()); - } -} - -std::string TestTypeName(const ::testing::TestParamInfo& info) { - return info.param.AsString(); + const CertificateProviderStore::PluginDefinition& fake_plugin = + bootstrap.certificate_providers().at("fake_plugin"); + ASSERT_EQ(fake_plugin.plugin_name, "fake"); + ASSERT_STREQ(fake_plugin.config->name(), "fake"); + ASSERT_EQ(static_cast>( + fake_plugin.config) + ->value(), + 0); } -INSTANTIATE_TEST_SUITE_P(XdsBootstrap, XdsBootstrapTest, - ::testing::Values(TestType(false), TestType(true)), - &TestTypeName); - +} // namespace } // namespace testing } // namespace grpc_core int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv); grpc::testing::TestEnvironment env(argc, argv); + grpc_init(); + grpc_core::CertificateProviderRegistry::RegisterCertificateProviderFactory( + absl::make_unique()); int ret = RUN_ALL_TESTS(); + grpc_shutdown(); return ret; } diff --git a/test/cpp/end2end/xds_end2end_test.cc b/test/cpp/end2end/xds_end2end_test.cc index c4dc5172e5e..6bcffc3f34f 100644 --- a/test/cpp/end2end/xds_end2end_test.cc +++ b/test/cpp/end2end/xds_end2end_test.cc @@ -8190,16 +8190,6 @@ TEST_P(CdsTest, RingHashPolicyHasInvalidRingSizeMinGreaterThanMax) { class XdsSecurityTest : public BasicTest { protected: - static void SetUpTestCase() { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT", "true"); - BasicTest::SetUpTestCase(); - } - - static void TearDownTestCase() { - BasicTest::TearDownTestCase(); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT"); - } - void SetUp() override { BasicTest::SetUp(); root_cert_ = ReadFile(kCaCertPath); @@ -9347,16 +9337,6 @@ class XdsServerSecurityTest : public XdsEnd2endTest { XdsServerSecurityTest() : XdsEnd2endTest(1, 1, 100, true /* use_xds_enabled_server */) {} - static void SetUpTestCase() { - gpr_setenv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT", "true"); - XdsEnd2endTest::SetUpTestCase(); - } - - static void TearDownTestCase() { - XdsEnd2endTest::TearDownTestCase(); - gpr_unsetenv("GRPC_XDS_EXPERIMENTAL_SECURITY_SUPPORT"); - } - void SetUp() override { XdsEnd2endTest::SetUp(); root_cert_ = ReadFile(kCaCertPath);