-
Notifications
You must be signed in to change notification settings - Fork 898
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
Add custom Azure logger #17228
Merged
Merged
Add custom Azure logger #17228
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
module Vmdb::Loggers | ||
class AzureLogger < VMDBLogger | ||
def initialize(*loggers) | ||
super | ||
|
||
# pulled from Ruby's `Logger::Formatter`, which is what it defaults to when it is `nil` | ||
@datetime_format = "%Y-%m-%dT%H:%M:%S.%6N " | ||
@formatter = Vmdb::Loggers::AzureLogger::Formatter.new | ||
end | ||
|
||
def <<(msg) | ||
msg = msg.strip | ||
log(level, msg) | ||
msg.size | ||
end | ||
|
||
class Formatter < VMDBLogger::Formatter | ||
def call(severity, datetime, progname, msg) | ||
msg = msg.sub(/Bearer(.*?)\"/, 'Bearer [FILTERED] "') | ||
msg = msg.sub(/SharedKey(.*?)\"/, 'SharedKey [FILTERED] "') | ||
msg = msg.sub(/client_secret=(.*?)&/, "client_secret=[FILTERED]&") | ||
super(severity, datetime, progname, msg) | ||
end | ||
end | ||
end | ||
end |
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 |
---|---|---|
@@ -0,0 +1,26 @@ | ||
describe Vmdb::Loggers::AzureLogger do | ||
before do | ||
@log_stream = StringIO.new | ||
@log = described_class.new(@log_stream) | ||
end | ||
|
||
context "azure" do | ||
it "filters out bearer tokens" do | ||
@log.log(@log.level, 'Bearer abcd1234 "stuff"') | ||
@log_stream.rewind | ||
expect(@log_stream.read).to match(Regexp.quote('Bearer [FILTERED] "stuff"')) | ||
end | ||
|
||
it "filters out sharedkey tokens" do | ||
@log.log(@log.level, 'SharedKey xxx123 "stuff"') | ||
@log_stream.rewind | ||
expect(@log_stream.read).to match(Regexp.quote('SharedKey [FILTERED] "stuff"')) | ||
end | ||
|
||
it "filters out client secret tokens" do | ||
@log.log(@log.level, 'client_secret=abc123&management=yadayada') | ||
@log_stream.rewind | ||
expect(@log_stream.read).to match(Regexp.quote('client_secret=[FILTERED]&management=yadayada')) | ||
end | ||
end | ||
end |
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.
Why is this necessary? A VMDBLogger is a Logger, which already has the
<<
method defined as you have done here.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.
Not sure why, but it won't work without it. I assume it's a rest-client issue.
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.
What kind of error? I just tried << on a VMDBLogger and it 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.
Ohhh, I see...
<<
"works", but doesn't write out the timestamp, which is the problem in the first place. I'm wondering if this should just be implemented on the underlying VMDBLogger class instead. @NickLaMuro @bdunne thoughts?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 don't know, seems like a bad idea. The whole point of
Logger#<<
is to print without formatting:https://github.com/ruby/ruby/blob/trunk/lib/logger.rb#L477-L483
I think it is stupid for
rest-client
to log via<<
, so I think doing this is a targeted class makes more sense. We only want this with loggers where this is a problem (i.e., ones that have to hook intorest-client
), and not for every logger in MIQ, because the original behavior of#<<
might be desired in some other circumstance.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.
@bdunne Yes, we have. Take a look at the issue tracker for rest-client. Unfortunately, the project is in poor health.
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 been looking into replacing
rest-client
where we can (since a couple of dependencies for it our ones we own). That said, it is more a labor of love, and not something on the "official agenda".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.
@NickLaMuro I'm desperately trying to get azure-armrest switched over to Excon, but it's a lot of work. I should probably make a separate port where I don't try to refactor anything as I go, and just do a straight port to get it working.
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.
@NickLaMuro I actually thinking moving off of rest-client should start getting prioritized a bit higher the longer that rest-client languishes. I think Excon and Faraday are the two best options at this point.
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 was just going to use
net/http
where the additions of provided by other libs aren't really necessary.