Skip to content

x-pack/libbeat/common/cloudfoundry: Use xxhash instead of SHA1 #43964

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

Merged
merged 12 commits into from
May 28, 2025

Conversation

shmsr
Copy link
Member

@shmsr shmsr commented Apr 16, 2025

Proposed commit message

Drop usage of SHA1 and replace with a better algorithm xxhash for sanitizing the filenames.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 16, 2025
@shmsr shmsr self-assigned this Apr 16, 2025
Copy link
Contributor

mergify bot commented Apr 16, 2025

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @shmsr? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

@shmsr shmsr marked this pull request as ready for review April 17, 2025 07:15
@shmsr shmsr requested a review from a team as a code owner April 17, 2025 07:15
@shmsr shmsr requested review from belimawr and faec April 17, 2025 07:15
@shmsr shmsr changed the title x-pack/libbeat/common/cloudfoundry: Use SHA224 instead of SHA1 in FIPS mode x-pack/libbeat/common/cloudfoundry: Use xxhash instead of SHA1 in FIPS mode Apr 17, 2025
Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

I believe it ends up being just a variation of the same question asked by @kruskall: what happens if someone upgrades Beats to the FIPS version? Is this upgrade even supported?

@pierrehilbert pierrehilbert added the Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team label Apr 30, 2025
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Apr 30, 2025
@elasticmachine
Copy link
Collaborator

Pinging @elastic/elastic-agent-data-plane (Team:Elastic-Agent-Data-Plane)

@pierrehilbert pierrehilbert requested a review from rdner April 30, 2025 06:46
@shmsr
Copy link
Member Author

shmsr commented May 1, 2025

I believe it ends up being just a variation of the same question asked by @kruskall: what happens if someone upgrades Beats to the FIPS version? Is this upgrade even supported?

AFAIK it's a complete separate offering. So, switching from one mode to another, it is not even allowed. @kruskall can confirm in case if I am wrong here.

@shmsr shmsr changed the title x-pack/libbeat/common/cloudfoundry: Use xxhash instead of SHA1 in FIPS mode x-pack/libbeat/common/cloudfoundry: Use xxhash instead of SHA1 May 1, 2025
@rdner rdner removed their request for review May 6, 2025 07:50
@shmsr shmsr requested review from kruskall and belimawr May 6, 2025 20:05
@shmsr
Copy link
Member Author

shmsr commented May 6, 2025

I am thinking to merge it today/ tomorrow; could I please get approval if everything looks good?

@shmsr
Copy link
Member Author

shmsr commented May 7, 2025

@faec Could you please review this PR again? I am planning to merge it today if there are no further comments.

@shmsr shmsr requested a review from tommyers-elastic May 7, 2025 11:10
@pierrehilbert
Copy link
Collaborator

@shmsr Fae will be out until tomorrow so she won't be able to review it once again today.

@shmsr
Copy link
Member Author

shmsr commented May 7, 2025

@shmsr Fae will be out until tomorrow so she won't be able to review it once again today.

ah, ok. Then can someone else take a look? I just want to get it reviewed before everyone leaves for EAH.

Copy link
Contributor

@belimawr belimawr left a comment

Choose a reason for hiding this comment

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

LGTM, however I'm missing a changelog entry stating that upgrading to this version will cause the cache invalidation once.

@shmsr, do you have an idea (or is it possible to measure) what would be the performance impact upon upgrade?

I don't know much about this input, but invalidating the whole cache is similar to a cold start, so my understanding is that the performance impact would not cause problems, however it would be nice to confirm that.

@shmsr
Copy link
Member Author

shmsr commented May 23, 2025

LGTM, however I'm missing a changelog entry stating that upgrading to this version will cause the cache invalidation once.

I will add an entry.

@shmsr, do you have an idea (or is it possible to measure) what would be the performance impact upon upgrade?

From the code, it looks like it's being used for an inexpensive op. I checked the usage of methods of the cache, and it was being used to get CF app info (GetAppByGuid; here). So, I see a very minimal impact. And this also happens once per processor's Run (here) method.

I don't know much about this input, but invalidating the whole cache is similar to a cold start, so my understanding is that the performance impact would not cause problems, however it would be nice to confirm that.

Yes.

@shmsr shmsr requested a review from a team as a code owner May 27, 2025 20:19
@shmsr
Copy link
Member Author

shmsr commented May 27, 2025

@elastic/ingest-docs Can I get a review on this? Added a CL entry.

Copy link
Contributor

@vishaangelova vishaangelova left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@shmsr shmsr merged commit 367b201 into elastic:main May 28, 2025
85 checks passed
@shmsr shmsr added the backport-8.19 Automated backport to the 8.19 branch label Jun 3, 2025
mergify bot pushed a commit that referenced this pull request Jun 3, 2025
(cherry picked from commit 367b201)

# Conflicts:
#	docs/release-notes/index.md
pierrehilbert pushed a commit that referenced this pull request Jun 6, 2025
…h instead of SHA1 (#44621)

* x-pack/libbeat/common/cloudfoundry: Use xxhash instead of SHA1 (#43964)

(cherry picked from commit 367b201)

# Conflicts:
#	docs/release-notes/index.md

* Fix CL entry; move from *.md to *.asciidoc

---------

Co-authored-by: subham sarkar <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-8.19 Automated backport to the 8.19 branch Team:Elastic-Agent-Data-Plane Label for the Agent Data Plane team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants