Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rbac: add some debug logging. #3744

Merged
merged 4 commits into from
Jun 28, 2018
Merged

rbac: add some debug logging. #3744

merged 4 commits into from
Jun 28, 2018

Conversation

yangminzhu
Copy link
Contributor

Signed-off-by: Yangmin Zhu ymzhu@google.com

Description: Added some log for help debugging
Risk Level: Low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a

@lizan @mattklein123 @rodaine
Thank you!

@yangminzhu
rbac: add some debug logging.
e2dd078 
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
@yangminzhu yangminzhu mentioned this pull request Jun 27, 2018
Closed
17 tasks
@yangminzhu
fix coverate test
5695b34 
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
@yangminzhu
Copy link
Contributor Author

yangminzhu commented Jun 28, 2018 •
edited
Loading

It seems the coverage test is running at debug level which caused the test fail:

[ RUN      ] RoleBasedAccessControlFilterTest.Allowed
test/extensions/filters/http/rbac/rbac_filter_test.cc:51: Failure
Mock function called more times than expected - taking default action specified at:
test/mocks/network/mocks.cc:87:
    Function call: localAddress()
          Returns: @0x13ba6b08 16-byte object <00-00 00-00 00-00 00-00 00-00 00-00 00-00 00-00>
         Expected: to be called twice
           Actual: called 3 times - over-saturated and active

I changed the test to not to check exactly called times. The better way probably should to increase the expect_times by 1 when in debug mode, but not sure if this is a common way in Envoy.

mattklein123
mattklein123 reviewed Jun 28, 2018
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, some small comments.

test/extensions/filters/http/rbac/rbac_filter_test.cc Outdated
address_ = Envoy::Network::Utility::parseInternetAddress("1.2.3.4", port, false);
auto& expect = EXPECT_CALL(connection_, localAddress());
if (times > 0) {
expect.Times(times);
if (at_most_times > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't a useful expectation. Please just make this an ON_CALL(...).WillByDefault(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

source/extensions/filters/http/rbac/rbac_filter.cc Outdated
@@ -65,13 +65,30 @@ RoleBasedAccessControlRouteSpecificFilterConfig::RoleBasedAccessControlRouteSpec

Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::HeaderMap& headers,
bool) {
ENVOY_LOG(
debug,
"checking request:\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug lines in general should be single line. Please remove the newlines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

source/extensions/filters/http/rbac/rbac_filter.cc Outdated
@@ -65,13 +65,30 @@ RoleBasedAccessControlRouteSpecificFilterConfig::RoleBasedAccessControlRouteSpec

Http::FilterHeadersStatus RoleBasedAccessControlFilter::decodeHeaders(Http::HeaderMap& headers,
bool) {
ENVOY_LOG(
debug,
"checking request:\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"checking request" isn't a very descriptive message. Either make a new "rbac" log type, or prefix all of the messages you added with "rbac: " (or something similar).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, I added a new "rbac" log type.

@yangminzhu
address comments.
d2d29e9 
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
mattklein123
mattklein123 reviewed Jun 28, 2018
View reviewed changes
source/common/common/logger.h Outdated
@@ -45,7 +45,8 @@ namespace Logger {
FUNCTION(upstream) \
FUNCTION(grpc) \
FUNCTION(stats) \
FUNCTION(thrift)
FUNCTION(thrift) \
FUNCTION(rbac)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please alpha order

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@yangminzhu
address comment.
280fd98 
Signed-off-by: Yangmin Zhu <ymzhu@google.com>
mattklein123
mattklein123 approved these changes Jun 28, 2018
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks

@mattklein123 mattklein123 merged commit 4865c9c into envoyproxy:master Jun 28, 2018
@yangminzhu yangminzhu deleted the rbac_debug branch June 28, 2018 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@yangminzhu @mattklein123