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

core: Add utilities for generating SHA256 hashes. #491

Merged
merged 58 commits into from
Jul 31, 2024

Conversation

haiqi96
Copy link
Contributor

@haiqi96 haiqi96 commented Jul 22, 2024

Description

The PR introduces a hash_utils.cpp/hpp to generate SHA256 hashes and two simple unit tests.
The PR also remove the CMakefile for Openssl. The file was originally added to fix a bug for Cmake with version < 3.16.0. since the minimum version of CMake we are using is 3.16.2, we can safely remove the file and bumb up the required version of cmake.

Validation performed

Generate a presigned url with s3 https url with the hash utils and confirmed that the presigned url works

@haiqi96 haiqi96 changed the title S3 ingestion core-clp: Add AWS signer class to CLP Jul 22, 2024
@haiqi96 haiqi96 changed the title core-clp: Add utilities to generate sha256 hash core-clp: Add support for generating sha256 hash. Jul 26, 2024
components/core/src/clp/hash_utils.hpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.hpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.hpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.hpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.hpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.cpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.cpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.cpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.cpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.cpp Outdated Show resolved Hide resolved
commit code review suggestions

Co-authored-by: kirkrodrigues <[email protected]>
components/core/src/clp/hash_utils.cpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.cpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.cpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.cpp Outdated Show resolved Hide resolved
Comment on lines 29 to 30
auto const openssl_err = ERR_get_error();
return {ERR_error_string(openssl_err, nullptr)};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
auto const openssl_err = ERR_get_error();
return {ERR_error_string(openssl_err, nullptr)};
auto const openssl_err = ERR_get_error();
auto* openssl_err_str = ERR_error_string(openssl_err, nullptr);
if (nullptr == openssl_err_str) {
return {};
}
return {openssl_err_str};

Because ERR_error_string may return nullptr and std::string{nullptr} triggers undefined behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for catching this. In the nullptr case, should we return some customzied error message so users don't get a "" which could be quite confusing?

Copy link
Member

Choose a reason for hiding this comment

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

Uh, we could. The reason I chose {} is because if you call this method and no error had occurred, then you would also get nullptr. So perhaps we should do, in order:

  • 0 == openssl_err => {}
  • nullptr == openssl_err_str => {"Unknown error"}
  • else => {openssl_err_str}

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, on no error, ERR_error_string does return a string. So I guess we can just do the last two bullets.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

never thought someone will try to call this when there's no error, lol. Sure, sounds fair

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps to be clearer, we can use "Error has no string representation" instead of "Unknown error".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about
"Error has no string representation, error_code: " + std::to_string(openssl_err)?

Copy link
Member

Choose a reason for hiding this comment

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

Sg

components/core/src/clp/hash_utils.hpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.hpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.hpp Outdated Show resolved Hide resolved
components/core/src/clp/hash_utils.hpp Show resolved Hide resolved
components/core/src/clp/hash_utils.hpp Outdated Show resolved Hide resolved
Copy link
Member

@kirkrodrigues kirkrodrigues left a comment

Choose a reason for hiding this comment

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

For the PR title, how about:

core: Add utilities for generating SHA256 hashes.

@haiqi96 haiqi96 changed the title core-clp: Add support for generating sha256 hash. core: Add utilities for generating SHA256 hashes. Jul 31, 2024
@haiqi96
Copy link
Contributor Author

haiqi96 commented Jul 31, 2024

For the PR title, how about:

core: Add utilities for generating SHA256 hashes.

@haiqi96 haiqi96 closed this Jul 31, 2024
@haiqi96
Copy link
Contributor Author

haiqi96 commented Jul 31, 2024

For the PR title, how about:

core: Add utilities for generating SHA256 hashes.

Sounds good

@haiqi96 haiqi96 reopened this Jul 31, 2024
@haiqi96 haiqi96 enabled auto-merge (squash) July 31, 2024 18:24
@kirkrodrigues kirkrodrigues disabled auto-merge July 31, 2024 19:22
@kirkrodrigues kirkrodrigues merged commit a6a2185 into y-scope:main Jul 31, 2024
26 checks passed
@haiqi96 haiqi96 deleted the s3_ingestion branch August 8, 2024 19:50
jackluo923 pushed a commit to jackluo923/clp that referenced this pull request Dec 4, 2024
Co-authored-by: wraymo <[email protected]>
Co-authored-by: Lin Zhihao <[email protected]>
Co-authored-by: kirkrodrigues <[email protected]>
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.

4 participants