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

Fix community_id processor so that ports greater than 65535 aren't valid #25409

Merged

Conversation

legoguy1000
Copy link
Contributor

@legoguy1000 legoguy1000 commented Apr 28, 2021

What does this PR do?

This changes the community_id processor so that it doesn't accept ports > 65535 or < 1. Based off of how the ES processor works, https://github.com/elastic/elasticsearch/blob/master/modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/CommunityIdProcessor.java#L175

Why is it important?

Currently the processor only checks for if the port != 0 but allows negative numbers and numbers > 65535 which is invalid.

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.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Use cases

Screenshots

image
This is from #24620 when I moved the community_id processor from filebeat to the ingest pipeline, the ES processor does the additional checks that filebeat currently is not.

Logs

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 28, 2021
@legoguy1000 legoguy1000 force-pushed the communityid-processor-high-ports branch from f575d83 to efdee99 Compare April 28, 2021 20:53
@legoguy1000
Copy link
Contributor Author

@andrewkroh Found a bug in the community_id processor. You were the last to make changes to it so I figured I'd ping you for any comments or feedback.

@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 28, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: marc-gr commented: /test

  • Start Time: 2021-05-05T10:55:54.126+0000

  • Duration: 102 min 58 sec

  • Commit: ef2ca1b

Test stats 🧪

Test Results
Failed 0
Passed 29774
Skipped 4010
Total 33784

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 29774
Skipped 4010
Total 33784

@kvch kvch requested a review from andrewkroh April 29, 2021 10:44
@kvch
Copy link
Contributor

kvch commented Apr 29, 2021

jenkins run tests

@legoguy1000
Copy link
Contributor Author

legoguy1000 commented Apr 29, 2021

I see why the tests are failing, because of the unit16 conversion function. For numbers larger than a int16, its returning the remainder instead of just failing.

@legoguy1000
Copy link
Contributor Author

I'm testing the tryToUint16 function and no matter what it only seems to hit the int case of the switch so the all the extra cases may be unecessary.

@legoguy1000
Copy link
Contributor Author

I updated the function forcing the unit16 conversion and now all the tests pass. IDK if its the best way of doing it.

@legoguy1000 legoguy1000 marked this pull request as ready for review April 29, 2021 14:44
@elasticmachine
Copy link
Collaborator

Pinging @elastic/security-external-integrations (Team:Security-External Integrations)

@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label May 3, 2021
@marc-gr
Copy link
Contributor

marc-gr commented May 3, 2021

/test

@marc-gr marc-gr self-assigned this May 3, 2021
@legoguy1000 legoguy1000 force-pushed the communityid-processor-high-ports branch from 21b3b1e to b018d2c Compare May 3, 2021 15:25
@legoguy1000
Copy link
Contributor Author

IDK why the community IDs are being removed from these documents when all teh pieces are valid, https://beats-ci.elastic.co/blue/organizations/jenkins/Beats%2Fbeats/detail/PR-25409/5/pipeline#step-7529-log-174. The Go tests pass just fine.

@mergify
Copy link
Contributor

mergify bot commented May 3, 2021

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b communityid-processor-high-ports upstream/communityid-processor-high-ports
git merge upstream/master
git push upstream communityid-processor-high-ports

@legoguy1000 legoguy1000 force-pushed the communityid-processor-high-ports branch from b018d2c to c5b7cc5 Compare May 3, 2021 18:48
Copy link
Contributor

@marc-gr marc-gr 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 the types you removed are the ones causing the missing community_ids, since they are going to the default case. (Do not merge my suggestion as I am quite sure they are not correctly formatted)

@marc-gr
Copy link
Contributor

marc-gr commented May 5, 2021

/test

@marc-gr marc-gr added backport-v7.12.0 Automated backport with mergify backport-v7.13.0 Automated backport with mergify backport-v7.14.0 Automated backport with mergify bug labels May 5, 2021
@marc-gr
Copy link
Contributor

marc-gr commented May 5, 2021

/test

@marc-gr
Copy link
Contributor

marc-gr commented May 5, 2021

Thanks for the work!

@marc-gr marc-gr merged commit 07d0cd8 into elastic:master May 5, 2021
@legoguy1000 legoguy1000 deleted the communityid-processor-high-ports branch May 5, 2021 13:45
mergify bot pushed a commit that referenced this pull request May 5, 2021
…valid (#25409)

* initial commit

* update function

* update function per comments

(cherry picked from commit 07d0cd8)
mergify bot pushed a commit that referenced this pull request May 5, 2021
…valid (#25409)

* initial commit

* update function

* update function per comments

(cherry picked from commit 07d0cd8)
mergify bot pushed a commit that referenced this pull request May 5, 2021
…valid (#25409)

* initial commit

* update function

* update function per comments

(cherry picked from commit 07d0cd8)
marc-gr pushed a commit that referenced this pull request May 6, 2021
…valid (#25409) (#25558)

* initial commit

* update function

* update function per comments

(cherry picked from commit 07d0cd8)

Co-authored-by: Alex Resnick <[email protected]>
marc-gr pushed a commit that referenced this pull request May 6, 2021
…valid (#25409)

* initial commit

* update function

* update function per comments

(cherry picked from commit 07d0cd8)
marc-gr pushed a commit that referenced this pull request May 6, 2021
…valid (#25409) (#25557)

* initial commit

* update function

* update function per comments

(cherry picked from commit 07d0cd8)

Co-authored-by: Alex Resnick <[email protected]>
marc-gr pushed a commit that referenced this pull request May 6, 2021
…valid (#25409) (#25559)

* initial commit

* update function

* update function per comments

(cherry picked from commit 07d0cd8)

Co-authored-by: Alex Resnick <[email protected]>
leweafan pushed a commit to leweafan/beats that referenced this pull request Apr 28, 2023
…valid (elastic#25409) (elastic#25559)

* initial commit

* update function

* update function per comments

(cherry picked from commit df5f409)

Co-authored-by: Alex Resnick <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-v7.12.0 Automated backport with mergify backport-v7.13.0 Automated backport with mergify backport-v7.14.0 Automated backport with mergify bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants