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

[citrix_adc] Handle time zone parsing in sslvpn_and_aaatm_feature pipeline #10846

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

mjwolf
Copy link
Contributor

@mjwolf mjwolf commented Aug 21, 2024

Proposed commit message

This has a few pipeline improvements

  • Fail if sslvpn_and_aaatm_feature message data cannot be parsed. If this data is not parsed, most data provided by this pipeline is silently not populated. So I think overall its better to fail, so that users and developers are more aware that there is an error.
  • Improve parsing of the message to handle optional space between username and Group key. Both formats have been observed.
  • Handle the presence of time zone in the message timestamp.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

…eline

This has a few pipeline improvements

* Fail if sslvpn_and_aaatm_feature message data cannot be parsed. If this data
is not parsed, most data provided by this pipeline is silently not populated.
So I think overall its better to fail, so that users and developers are more aware
that there is an error.
* Improve parsing of the message to handle optional space between username and group.
Both formats have been observed.
* Handle the presence of time zone in the message timestamp.
@mjwolf mjwolf added bug Something isn't working, use only for issues Team:Security-Deployment and Devices Deployment and Devices Security team [elastic/sec-deployment-and-devices] Integration:citrix_adc Citrix ADC labels Aug 21, 2024
@mjwolf mjwolf self-assigned this Aug 21, 2024
@mjwolf mjwolf requested a review from a team as a code owner August 21, 2024 21:43
@elasticmachine
Copy link

Pinging @elastic/sec-deployment-and-devices (Team:Security-Deployment and Devices)

@andrewkroh andrewkroh added bugfix Pull request that fixes a bug issue Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team [elastic/obs-infraobs-integrations] and removed bug Something isn't working, use only for issues labels Aug 21, 2024
@mjwolf mjwolf requested a review from a team August 21, 2024 21:46
Copy link
Contributor

@pkoutsovasilis pkoutsovasilis left a comment

Choose a reason for hiding this comment

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

manifest needs to get updated as well? also this isn't a sec-deployment-and-devices integration?! or at least the .github/CODEOWNERS does not say so

@mjwolf
Copy link
Contributor Author

mjwolf commented Aug 21, 2024

manifest needs to get updated as well? also this isn't a sec-deployment-and-devices integration?! or at least the .github/CODEOWNERS does not say so

I've updated the manifest. And yeah, I guess we don't own this. It seems like an integration we'd own, and I didn't check before working on it

@pkoutsovasilis
Copy link
Contributor

pkoutsovasilis commented Aug 21, 2024

I've updated the manifest. And yeah, I guess we don't own this. It seems like an integration we'd own, and I didn't check before working on it

definitely no worries, I see no issue with that! just checking if my eyes were deceiving me. However this means even if I approve you know 🙂

Copy link
Contributor

@pkoutsovasilis pkoutsovasilis left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@mjwolf mjwolf enabled auto-merge (squash) August 28, 2024 17:29
@elasticmachine
Copy link

💚 Build Succeeded

History

cc @mjwolf

@shmsr
Copy link
Member

shmsr commented Aug 28, 2024

Changes look good, but I noticed a spelling error: "received" is misspelled as "recieved" in some places while reviewing. But it is unrelated to the change made here.

@mjwolf mjwolf merged commit 91399a8 into elastic:main Aug 28, 2024
5 checks passed
@mjwolf mjwolf deleted the citrix-timestamps branch August 28, 2024 20:09
@elasticmachine
Copy link

Package citrix_adc - 1.7.2 containing this change is available at https://epr.elastic.co/search?package=citrix_adc

@muthu-mps
Copy link
Contributor

@mjwolf - The citrix_waf integration also does processing the logs for citrix_adc. Do you consider validating the scenario with the WAF integration as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Pull request that fixes a bug issue Integration:citrix_adc Citrix ADC Team:Obs-InfraObs Label for the Observability Infrastructure Monitoring team [elastic/obs-infraobs-integrations] Team:Security-Deployment and Devices Deployment and Devices Security team [elastic/sec-deployment-and-devices]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants