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

[pkg/ottl] Added ottl functions for hashing strings #22968

Merged

Conversation

rnishtala-sumo
Copy link
Contributor

@rnishtala-sumo rnishtala-sumo commented May 30, 2023

Description:
Added 3 ottl functions for hashing strings

  • FNV
  • SHA1
  • SHA256

Link to tracking Issue: #22725

Testing:
Added the following unit tests

Documentation:

Copy link
Member

@TylerHelmuth TylerHelmuth left a comment

Choose a reason for hiding this comment

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

I didn't look at the details of the functions yet but the general structure of this PR is correct. Please add entries for each function in the ottlfuncs README.

@rnishtala-sumo rnishtala-sumo force-pushed the ottl_hash_functions branch 2 times, most recently from b996a55 to 117ee0b Compare May 31, 2023 20:25
@rnishtala-sumo rnishtala-sumo marked this pull request as ready for review May 31, 2023 21:09
@rnishtala-sumo rnishtala-sumo requested a review from a team May 31, 2023 21:09
pkg/ottl/ottlfuncs/func_fnv.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_sha1.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_fnv.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_sha1.go Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_sha256.go Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_sha256.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_sha1.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/README.md Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/README.md Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/README.md Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_fnv.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_fnv_test.go Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_sha1.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_sha1_test.go Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_sha256.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_sha256_test.go Show resolved Hide resolved
@rnishtala-sumo rnishtala-sumo force-pushed the ottl_hash_functions branch 2 times, most recently from 047f4e5 to faef564 Compare June 2, 2023 20:16
Copy link
Contributor

@evan-bradley evan-bradley left a comment

Choose a reason for hiding this comment

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

Thanks for adding these. Overall looks good to me, just one question.

pkg/ottl/ottlfuncs/func_fnv.go Outdated Show resolved Hide resolved
@kentquirk
Copy link
Member

I know I'm swooping in late, but could we please mention in the documentation that the SHA1 and SHA256 functions are not recommended to be used except where compatibility requires it? They're slow, insecure by modern standards, and really shouldn't be used by new applications.

@rnishtala-sumo
Copy link
Contributor Author

maybe a note like below?

Note: SHA1 is not a recommended hash function by the National Institute of Standards and Technology (NIST) due to the availability of more secure alternatives like SHA3.

@kentquirk
Copy link
Member

That's OK, but we don't include SHA3 as an available function. Maybe instead:

Note: According to the National Institute of Standards and Technology (NIST), SHA1 is no longer a recommended hash function. It should be avoided except when required for compatibility. New uses should prefer FNV whenever possible.

pkg/ottl/ottlfuncs/func_sha1.go Outdated Show resolved Hide resolved
pkg/ottl/ottlfuncs/func_sha256.go Outdated Show resolved Hide resolved
Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
@TylerHelmuth TylerHelmuth merged commit a8c03d0 into open-telemetry:main Jun 5, 2023
@github-actions github-actions bot added this to the next release milestone Jun 5, 2023
Caleb-Hurshman pushed a commit to observIQ/opentelemetry-collector-contrib that referenced this pull request Jul 6, 2023
…2968)

* [pkg/ottl] Added ottl functions for hashing strings

* [pkg/ottl] Return nil on errors

* Documentation and test changes for the hash functions

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>

---------

Co-authored-by: Tyler Helmuth <12352919+TylerHelmuth@users.noreply.github.com>
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.

5 participants