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

Added Sha3 as alternative #2480

Merged
merged 10 commits into from
Aug 13, 2020
Merged

Added Sha3 as alternative #2480

merged 10 commits into from
Aug 13, 2020

Conversation

wardbekker
Copy link
Member

@wardbekker wardbekker commented Aug 8, 2020

What this PR does / why we need it:

PR For discussion. Looking for a nice way to add Sha3_256 as a modern alternative to Sha2_256

Was looking how to I could implement template function syntax like:

Hash(salt, input) # -> defaulting to e.g. Sha2_256 
Hash(Sha3_256, salt, input) # -> explicit specification of hashing method 

But I couldn't find a way to implement function overloading in Go.

Hence, to don't go overboard on verbosity on the template function (e.g. Hash_Sha3_256(salt, input) ) I settled for Sha2(salt, input) and Sha3(salt, input).

I recommend to use only _256 result:

  • good enough (TM) hashing for the intended use case.
  • already a large number with 64 hex digits and high entropy numbers don't compress well, so _512 will add additional hex digits without any real benefit.

Checklist

  • Documentation added
  • Tests updated

@wardbekker wardbekker requested review from RichiH and sakjur and removed request for oddlittlebird August 9, 2020 19:34
@wardbekker
Copy link
Member Author

added commit and added more context to PR description

@codecov-commenter
Copy link

codecov-commenter commented Aug 9, 2020

Codecov Report

Merging #2480 into master will decrease coverage by 0.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2480      +/-   ##
==========================================
- Coverage   62.92%   62.90%   -0.03%     
==========================================
  Files         162      162              
  Lines       13981    13985       +4     
==========================================
- Hits         8798     8797       -1     
- Misses       4496     4498       +2     
- Partials      687      690       +3     
Impacted Files Coverage Δ
pkg/logentry/stages/template.go 83.87% <100.00%> (+1.11%) ⬆️
pkg/promtail/targets/file/tailer.go 73.86% <0.00%> (-4.55%) ⬇️
pkg/promtail/targets/file/filetarget.go 67.85% <0.00%> (-1.79%) ⬇️
pkg/querier/queryrange/downstreamer.go 97.93% <0.00%> (+2.06%) ⬆️

RichiH
RichiH previously requested changes Aug 10, 2020
Copy link
Member

@RichiH RichiH left a comment

Choose a reason for hiding this comment

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

I think we should be precise and explicit, i.e. call the hash function SHA3-256 (or Sha3-256). At the same time, we are applying a hash function, so that should be stated.
To lessen typing load, I would come with opinionated defaults. That's why I still think the default should be

replace: '*SSN*{{ .Value | Hash }}*'

which then offers SHA3-256 & a configured hash.

Copy link
Contributor

@oddlittlebird oddlittlebird left a comment

Choose a reason for hiding this comment

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

Edited docs

docs/sources/clients/promtail/stages/replace.md Outdated Show resolved Hide resolved
docs/sources/clients/promtail/stages/template.md Outdated Show resolved Hide resolved
Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

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

I think there's already been a lot of feedback here and I'm not opinionated enough/think bikeshedding further is worse than committing to something.

I like that we're exposing a Hash method designed to be kept strong over time. I dislike that we chose Sha2Hash over Sha256.

All in all I think this is a great PR I'm excited to have in Loki. Great work @wardbekker.

wardbekker and others added 2 commits August 13, 2020 16:09
Co-authored-by: Ed Welch <ed@oqqer.com>
Co-authored-by: Ed Welch <ed@oqqer.com>
@slim-bean slim-bean dismissed RichiH’s stale review August 13, 2020 14:30

changes were addressed

@slim-bean slim-bean merged commit 8ff8b42 into grafana:master Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants