Use local_info.node() instead of bootstrap.node() whenever possible #4120
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,6 +10,7 @@ | |
| #include "test/mocks/config/mocks.h" | ||
| #include "test/mocks/event/mocks.h" | ||
| #include "test/mocks/grpc/mocks.h" | ||
| #include "test/mocks/local_info/mocks.h" | ||
| #include "test/mocks/runtime/mocks.h" | ||
| #include "test/test_common/logging.h" | ||
| #include "test/test_common/utility.h" | ||
|
|
@@ -33,16 +34,17 @@ namespace { | |
| // is provided in [grpc_]subscription_impl_test.cc. | ||
| class GrpcMuxImplTest : public testing::Test { | ||
| public: | ||
| GrpcMuxImplTest() : async_client_(new Grpc::MockAsyncClient()), time_source_{} {} | ||
|
|
||
| void setup() { | ||
| EXPECT_CALL(dispatcher_, createTimer_(_)).WillOnce(Invoke([this](Event::TimerCb timer_cb) { | ||
| timer_cb_ = timer_cb; | ||
| timer_ = new Event::MockTimer(); | ||
|
There was a problem hiding this comment. Since the newly introduced tests do not call There was a problem hiding this comment. But yeah if we go with the other direction it will be reverted. |
||
| return timer_; | ||
| })); | ||
|
|
||
| grpc_mux_.reset(new GrpcMuxImpl( | ||
| local_info_, std::unique_ptr<Grpc::MockAsyncClient>(async_client_), dispatcher_, | ||
| *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( | ||
| "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), | ||
| random_, time_source_)); | ||
|
|
@@ -54,7 +56,7 @@ class GrpcMuxImplTest : public testing::Test { | |
| const Protobuf::int32 error_code = Grpc::Status::GrpcStatus::Ok, | ||
| const std::string& error_message = "") { | ||
| envoy::api::v2::DiscoveryRequest expected_request; | ||
| expected_request.mutable_node()->CopyFrom(local_info_.node()); | ||
| for (const auto& resource : resource_names) { | ||
| expected_request.add_resource_names(resource); | ||
| } | ||
|
|
@@ -71,7 +73,6 @@ class GrpcMuxImplTest : public testing::Test { | |
| EXPECT_CALL(async_stream_, sendMessage(ProtoEq(expected_request), false)); | ||
| } | ||
|
|
||
| NiceMock<Event::MockDispatcher> dispatcher_; | ||
| Runtime::MockRandomGenerator random_; | ||
| Grpc::MockAsyncClient* async_client_; | ||
|
|
@@ -81,11 +82,13 @@ class GrpcMuxImplTest : public testing::Test { | |
| std::unique_ptr<GrpcMuxImpl> grpc_mux_; | ||
| NiceMock<MockGrpcMuxCallbacks> callbacks_; | ||
| NiceMock<MockMonotonicTimeSource> time_source_; | ||
| NiceMock<LocalInfo::MockLocalInfo> local_info_; | ||
| }; | ||
|
|
||
| // Validate behavior when multiple type URL watches are maintained, watches are created/destroyed | ||
| // (via RAII). | ||
| TEST_F(GrpcMuxImplTest, MultipleTypeUrlStreams) { | ||
| setup(); | ||
| InSequence s; | ||
| auto foo_sub = grpc_mux_->subscribe("foo", {"x", "y"}, callbacks_); | ||
| auto bar_sub = grpc_mux_->subscribe("bar", {}, callbacks_); | ||
|
|
@@ -104,6 +107,7 @@ TEST_F(GrpcMuxImplTest, MultipleTypeUrlStreams) { | |
|
|
||
| // Validate behavior when multiple type URL watches are maintained and the stream is reset. | ||
| TEST_F(GrpcMuxImplTest, ResetStream) { | ||
| setup(); | ||
| InSequence s; | ||
| auto foo_sub = grpc_mux_->subscribe("foo", {"x", "y"}, callbacks_); | ||
| auto bar_sub = grpc_mux_->subscribe("bar", {}, callbacks_); | ||
|
|
@@ -129,6 +133,7 @@ TEST_F(GrpcMuxImplTest, ResetStream) { | |
|
|
||
| // Validate pause-resume behavior. | ||
| TEST_F(GrpcMuxImplTest, PauseResume) { | ||
| setup(); | ||
| InSequence s; | ||
| auto foo_sub = grpc_mux_->subscribe("foo", {"x", "y"}, callbacks_); | ||
| grpc_mux_->pause("foo"); | ||
|
|
@@ -149,6 +154,7 @@ TEST_F(GrpcMuxImplTest, PauseResume) { | |
|
|
||
| // Validate behavior when type URL mismatches occur. | ||
| TEST_F(GrpcMuxImplTest, TypeUrlMismatch) { | ||
| setup(); | ||
|
|
||
| std::unique_ptr<envoy::api::v2::DiscoveryResponse> invalid_response( | ||
| new envoy::api::v2::DiscoveryResponse()); | ||
|
|
@@ -184,6 +190,8 @@ TEST_F(GrpcMuxImplTest, TypeUrlMismatch) { | |
|
|
||
| // Validate behavior when watches has an unknown resource name. | ||
| TEST_F(GrpcMuxImplTest, WildcardWatch) { | ||
| setup(); | ||
|
|
||
| InSequence s; | ||
| const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; | ||
| auto foo_sub = grpc_mux_->subscribe(type_url, {}, callbacks_); | ||
|
|
@@ -215,6 +223,8 @@ TEST_F(GrpcMuxImplTest, WildcardWatch) { | |
|
|
||
| // Validate behavior when watches specify resources (potentially overlapping). | ||
| TEST_F(GrpcMuxImplTest, WatchDemux) { | ||
| setup(); | ||
|
|
||
| InSequence s; | ||
| const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; | ||
| NiceMock<MockGrpcMuxCallbacks> foo_callbacks; | ||
|
|
@@ -296,6 +306,8 @@ TEST_F(GrpcMuxImplTest, WatchDemux) { | |
|
|
||
| // Verifies that warning messages get logged when Envoy detects too many requests. | ||
| TEST_F(GrpcMuxImplTest, TooManyRequests) { | ||
| setup(); | ||
|
|
||
| EXPECT_CALL(async_stream_, sendMessage(_, false)).Times(AtLeast(100)); | ||
| EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); | ||
| EXPECT_CALL(time_source_, currentTime()) | ||
|
|
@@ -342,6 +354,8 @@ TEST_F(GrpcMuxImplTest, TooManyRequests) { | |
|
|
||
| // Verifies that a messsage with no resources is accepted. | ||
| TEST_F(GrpcMuxImplTest, UnwatchedTypeAcceptsEmptyResources) { | ||
| setup(); | ||
|
|
||
| EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); | ||
|
|
||
| const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; | ||
|
|
@@ -374,6 +388,8 @@ TEST_F(GrpcMuxImplTest, UnwatchedTypeAcceptsEmptyResources) { | |
|
|
||
| // Verifies that a messsage with some resources is rejected when there are no watches. | ||
| TEST_F(GrpcMuxImplTest, UnwatchedTypeRejectsResources) { | ||
| setup(); | ||
|
|
||
| EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); | ||
|
|
||
| const std::string& type_url = Config::TypeUrl::get().ClusterLoadAssignment; | ||
|
|
@@ -402,6 +418,28 @@ TEST_F(GrpcMuxImplTest, UnwatchedTypeRejectsResources) { | |
| grpc_mux_->onReceiveMessage(std::move(response))); | ||
| } | ||
|
|
||
| TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyClusterName) { | ||
| EXPECT_CALL(local_info_, clusterName()).WillOnce(Return("")); | ||
| EXPECT_THROW_WITH_MESSAGE( | ||
| GrpcMuxImpl( | ||
| local_info_, std::unique_ptr<Grpc::MockAsyncClient>(async_client_), dispatcher_, | ||
| *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( | ||
| "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), | ||
| random_, time_source_), | ||
| EnvoyException, "ads: setting --service-cluster and --service-node is required"); | ||
|
There was a problem hiding this comment. Unrelated: these error messages are misleading nowadays, since you can specify this same information in the bootstrap. There was a problem hiding this comment. I had some difficulties with crafting related test before, so I guess I need to read more on bootstrap initialization steps (server init). There was a problem hiding this comment. Think we should revisit the thrown error message from There was a problem hiding this comment. I have updated the message, not sure about its clarity and correctness. |
||
| } | ||
|
|
||
| TEST_F(GrpcMuxImplTest, BadLocalInfoEmptyNodeName) { | ||
| EXPECT_CALL(local_info_, nodeName()).WillOnce(Return("")); | ||
| EXPECT_THROW_WITH_MESSAGE( | ||
| GrpcMuxImpl( | ||
| local_info_, std::unique_ptr<Grpc::MockAsyncClient>(async_client_), dispatcher_, | ||
| *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( | ||
| "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), | ||
| random_, time_source_), | ||
| EnvoyException, "ads: setting --service-cluster and --service-node is required"); | ||
| } | ||
|
|
||
| } // namespace | ||
| } // namespace Config | ||
| } // namespace Envoy | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I have mixed opinions on this change. On the one hand, we make it more full proof, by forcing the
LocalInfotype. On the other, we are now passing a strict superset of what we were passing before, and the new information is not used. My main thoughts on makingLocalInfocanonical would be to just delete the node info from bootstrap very early in server init. But, this approach also works..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.
Interesting POV, will try to explore that too.
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.
It's not necessarily the correct PoV; just sharing where my thinking was originally :)