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: standalone HECEventwriter #380

Merged
merged 2 commits into from
Jul 22, 2024

Conversation

zugf
Copy link
Contributor

@zugf zugf commented Jul 9, 2024

When create_from_token() is invoked, it triggers the HECEventWriter constructor with scheme, host, and port set to None. This leads to a get_splunkd_access_info() call, which throws an exception if the HEC URI doesn’t reference the local machine. As this call is unnecessary in such cases, its invocation has been relocated within the if-statement

@zugf zugf requested a review from a team as a code owner July 9, 2024 13:16
Copy link

github-actions bot commented Jul 9, 2024

CLA Assistant Lite bot All contributors have signed the COC ✍️ ✅
Posted by the CLA Assistant Lite bot.

@zugf
Copy link
Contributor Author

zugf commented Jul 9, 2024

Where can I find the CLA document and how do I sign it?

@artemrys
Copy link
Member

artemrys commented Jul 9, 2024

@zugf thanks for the proposed fix!

you can find CLA here - CLA.md and Code of Conduct here - CODE_OF_CONDUCT.md.

in order to sign CLA and CoC - please post a comment "I have read the CLA Document and I hereby accept the CLA" and then another comment "I have read the Code of Conduct and I hereby accept the Terms"

@zugf
Copy link
Contributor Author

zugf commented Jul 9, 2024

I have read the Code of Conduct and I hereby accept the Terms

@zugf
Copy link
Contributor Author

zugf commented Jul 9, 2024

I have read the CLA Document and I hereby accept the CLA

@artemrys artemrys requested a review from hetangmodi-crest July 9, 2024 18:05
@hetangmodi-crest
Copy link
Contributor

hmm, the update makes sense. Can you please add unit test case for it as well so we can capture this behaviour?

@artemrys
Copy link
Member

@zugf we can help you with tests if you don't feel comfortable with approaching them yourself :)

@zugf
Copy link
Contributor Author

zugf commented Jul 15, 2024

To avoid code duplication, I’ve refactored test_hec_event_writer into a parametrized test.

The last 5 lines of the code have been extracted into a separate unit test.

The behaviour will be tested by the last parameter set (create_hec_event_writer__create_from_token__external_host, False).

This simulates a machine without a Splunk installation that uses the HEC to push events to a different machine. Existing monkey patch functionality is used to unset SPLUNK_HOME.

@hetangmodi-crest
Copy link
Contributor

hetangmodi-crest commented Jul 18, 2024

@zugf , can you please sign the commits, and we can proceed to merge the changes (signing commits guide).

@zugf zugf force-pushed the zugf/FixHECEventWriter branch from 3f1549e to 764c7b4 Compare July 19, 2024 18:49
zugf added 2 commits July 19, 2024 21:33
if create_from_token() is used, it will call the
HECEventWriter constructor with scheme, host and port set to None.
This will result in a call to get_splunkd_access_info() which will
throw an exception if the hec uri does not point to the local machine.
This call is not needed in that case,
so the invocation has been moved inside of the if-statement.
refactor existing hec  event writer unit tests
add test parameter for external hec event writer
@zugf zugf force-pushed the zugf/FixHECEventWriter branch from 764c7b4 to 93a105f Compare July 19, 2024 19:49
@zugf
Copy link
Contributor Author

zugf commented Jul 19, 2024

Commits are signed. Have a good weekend
Florian

@hetangmodi-crest hetangmodi-crest merged commit 4e961b2 into splunk:main Jul 22, 2024
13 of 14 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 22, 2024
@srv-rr-github-token
Copy link
Contributor

🎉 This issue has been resolved in version 5.1.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants