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.

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 log filtering functionality #71

Merged
merged 6 commits into from
Jul 14, 2022
Merged

Add log filtering functionality #71

merged 6 commits into from
Jul 14, 2022

Conversation

detro
Copy link
Contributor

@detro detro commented Jul 4, 2022

Closes #17

This is a general purpose solution, to enable provider developers to setup log filtering at runtime. Both log omission and log masking are supported.

Feature summary

For log omission (i.e. the log is never given to the underlying writer), it offers the following:

  • omit log by keys: if one or more given keys are met, the log is omitted from output
  • omit log matching regexp: if one of more of the given regular expressions are matched by the log message, the log is omitted from output
  • omit log matching string: simplified version of the regexp case

For log masking, it offers the following:

  • mask log value by keys: for keys that are matched within log arguments, the value is masked with ***
  • mask log matching regexp: every sub-string matching the regexp within the log message, are masked with ***
  • mask log matching string: simplified version of the regexp case

Feature coverage

The log filtering covers all the loggers in the package:

  • tflog provider root logger
  • tflog provider subsystems logger
  • tfsdklog provider logger
  • tfsdklog provider subsystems logger

@detro detro requested a review from a team as a code owner July 4, 2022 16:46
@detro detro added the enhancement New feature or request label Jul 4, 2022
@detro detro added this to the v0.5.0 milestone Jul 4, 2022
Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

LGTM. Couple of very minor comments.


return result
for i := 0; i < len(args); i += 2 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Although it looks like this should never happen, if len(args) is an odd/uneven number, then a key with no corresponding value could be appended to keys.

Perhaps for i := 0; i + 1 < len(args); i += 2 { could be used to avoid such a scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added tests for this as well

internal/hclogutils/args_test.go Show resolved Hide resolved
internal/logging/filtering.go Show resolved Hide resolved
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

Nice work so far -- a couple opening questions/comments before potentially diving deeper into things.

func omitOrMask(ctx context.Context, logger hclog.Logger, msg *string, additionalFields []map[string]interface{}) ([]interface{}, bool) {
tfLoggerOpts := logging.GetProviderRootTFLoggerOpts(ctx)
additionalArgs := hclogutils.MapsToArgs(additionalFields...)
impliedArgs := logger.ImpliedArgs()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we save a lot of slice trouble by keeping everything as maps with terraform-plugin-log? e.g. Keeping additionalFields as []map[string]interface{} (or better yet requiring the merged additional fields map) and writing the inverse of hclogutils.MapsToArgs() if we ever need to interrogate the underlying (Logger).ImpliedArgs()? That said, we might be able to avoid ImpliedArgs() completely, if we also saved the With() information similar to this filtering information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Doing the filtering on the maps, would force the logic to convert the ImpliedArgs to map, before it can be filtered.
So I did the opposite, given that the underlying library requires to go in the args = [][]interface{} direction.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but in my opinion it is more difficult to work with the slices correctly since it is an odd way to store paired data, when a map gives you direct access to paired keys and values. I think it would save a lot of code complexity and readability, such as not needing to loop through all slice elements to find a key to mask its value, to let the slice stuff be a detail we don't have to worry about except for converting to/from.

It would be nice to avoid ImpliedArgs() completely, but that change can certainly happen outside the scope of adding filters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that the ImpliedArgs() (that ultimately dictate this) are a sub-par thing from hclog. But I didn't want to expand my work to have to also alter With() so to always keep those key/value pairs stored in the context.

And writing the couple of i += 2 for loops was relatively easy.

We can refactor that later I guess.

Copy link
Contributor

@bflad bflad Jul 13, 2022

Choose a reason for hiding this comment

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

There was no need to refactor With() now (we could've introduced the slice to map converter temporarily), but like you say, it can be refactored later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll create a follow up ticket to do the refactoring and simplification: iterating over the keys of a map is definitely the most desirable way to go. Just focused on getting the functionality done and working on this one.

tflog/provider.go Outdated Show resolved Hide resolved
tflog/provider.go Outdated Show resolved Hide resolved
internal/logging/provider.go Outdated Show resolved Hide resolved
tflog/provider.go Outdated Show resolved Hide resolved
tflog/provider.go Outdated Show resolved Hide resolved
tflog/provider_test.go Outdated Show resolved Hide resolved
Ivan De Marino added 5 commits July 13, 2022 17:27
It takes `ImplicitArgs()` from `hclog.Logger`, and returns the keys of the k/v pairs slice
- tflog root logger
- tflog subsystem logger
- tfsdklog root logger
- tfsdklog subsystem logger
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

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

I don't agree with the readability and complexity of using slices throughout over maps, but the implementation works and that can be refactored later. Looks good to me, otherwise. 🚀

@detro
Copy link
Contributor Author

detro commented Jul 13, 2022

Will add a CHANGELOG entry for 0.5.0 and merge it

@detro
Copy link
Contributor Author

detro commented Jul 14, 2022

@bflad as agreed, here is an issue to track the work to refactor the whole use of implicit args #75

.changelog/71.txt Outdated Show resolved Hide resolved
.changelog/71.txt Outdated Show resolved Hide resolved
.changelog/71.txt Outdated Show resolved Hide resolved
.changelog/71.txt Outdated Show resolved Hide resolved
@detro detro merged commit 6c641bd into main Jul 14, 2022
@detro detro deleted the detro/17-log_filtering branch July 14, 2022 12:06
@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to remove sensitive content from logs
3 participants