-
Notifications
You must be signed in to change notification settings - Fork 10
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
tflog+tfsdklog: Prevent slice/map leaks when setting LoggerOpts #132
Conversation
Reference: #126 Reference: #131 Since the `LoggerOpts` struct contains slice and map fields, it is important to ensure any modifications occur on copies of those slices and maps, otherwise the memory reference can wind up being shared. Consumers should always be able to create a new `context.Context` without worrying about shared data. This change introduces a `Copy()` method for `LoggerOpts` and implements it for option modifier functions which adjust a map or slice.
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.
LGTM 👍🏻
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.
LGTM 👍
tflog/provider_test.go
Outdated
|
||
tflog.Trace(originalCtx, "original should be masked") | ||
tflog.Trace(originalCtx, "new should be preserved") | ||
tflog.Trace(newCtx, "new should be masked") |
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.
Is it worth using different log levels for originalCtx
and newCtx
in order to be able to differentiate between them in the expectedOutput
?
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.
Sure thing -- will adjust
tflog/provider_test.go
Outdated
|
||
tflog.Trace(originalCtx, "original should be masked") | ||
tflog.Trace(originalCtx, "new should be preserved") | ||
tflog.Trace(newCtx, "new should be masked") |
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.
Is it worth using different log levels for originalCtx
and newCtx
in order to be able to differentiate between them in the expectedOutput
?
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions. |
Reference #126
Closes #131
Since the
LoggerOpts
struct contains slice and map fields, it is important to ensure any modifications occur on copies of those slices and maps, otherwise the memory reference can wind up being shared. Consumers should always be able to create a newcontext.Context
without worrying about shared data.This change introduces a
Copy()
method forLoggerOpts
and implements it for option modifier functions which adjust a map or slice.