Skip to content

Removes filter for sinks #261

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

Conversation

amandakarina
Copy link
Contributor

Hey folks

This pull request closes #258 issue.

Could you, please, take a look?

Thanks!

@amandakarina amandakarina marked this pull request as ready for review October 6, 2020 17:04
@amandakarina amandakarina requested a review from a team as a code owner October 6, 2020 17:04
@rjerrems
Copy link
Collaborator

rjerrems commented Oct 8, 2020

Hi @amandakarina - would be good to understand some extra context on what we are trying to achieve here, I have commented on the linked bug

@amandakarina
Copy link
Contributor Author

Hi @amandakarina - would be good to understand some extra context on what we are trying to achieve here, I have commented on the linked bug

Hey @rjerrems
The SHA scanner detected this finding after deploy CFT in a clean organization. The SHA Scanner checks if the sink has a empty filter, if so, no finding is generated, otherwise, a LOG_NOT_EXPORTED finding is created.

@rjerrems
Copy link
Collaborator

Yep - I suppose the question I have is: are we recommending that people export all logs? Are we confident the SHA finding is representative of best practice here? It doesn't seem to stack up with my experience where specific log types are usually targeted. It will also increase cost, particularly in our case where we are creating three copies of the exported logs to GCS, BigQuery & PubSub.

@amandakarina amandakarina marked this pull request as draft October 19, 2020 18:00
@amandakarina
Copy link
Contributor Author

Hey @rjerrems, I remove the filter only for bucket export. Could you, please take a look?

@amandakarina amandakarina marked this pull request as ready for review October 19, 2020 19:12
@rjerrems
Copy link
Collaborator

Thanks @amandakarina - can we add some brief docs that explain the difference, then we should be good to go.

Just to confirm also, that the SHA scanner result is fixed by this change?

@amandakarina
Copy link
Contributor Author

Thanks @amandakarina - can we add some brief docs that explain the difference, then we should be good to go.

Just to confirm also, that the SHA scanner result is fixed by this change?

Hey Rohan!

Based on my test, yes!
I inactivated all findings for LOG_NOT_EXPORTED and create a new project under my folder.
After SHA Scanner was executed no new finding was created and no finding for my folder was re-activated.

amandakarina and others added 4 commits October 22, 2020 10:14

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Copy link
Collaborator

@rjerrems rjerrems left a comment

Choose a reason for hiding this comment

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

LGTM

@rjerrems
Copy link
Collaborator

FYI Merging because test failure is due to name collision & healthy test run on prior commit

@rjerrems rjerrems merged commit cd42805 into terraform-google-modules:master Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[shascanner] LOG_NOT_EXPORTED detected
2 participants