Skip to content

[panw] Allow / to be optional in url - #9688

Merged
gogochan merged 5 commits into
mainfrom
sdh4638
Apr 24, 2024
Merged

[panw] Allow / to be optional in url#9688
gogochan merged 5 commits into
mainfrom
sdh4638

Conversation

@gogochan

@gogochan gogochan commented Apr 23, 2024

Copy link
Copy Markdown
Contributor

Proposed commit message

Link to SDH

This PR allows url to not have /, allowing something like elastic.co. The painless script that handles url had a condition for a mandatory / in url field. Due to this condition, any urls not containing / were not processed by this painless script, resulting in missing field as noted in the SDH.

The logic was updated and the script was updated to handle both URL with or without /.

There was also another bug in port number parsing, making url.port string instead of long as defined HERE. Let me know if this fix breaks existing deployment because of this change.

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.

How to test this PR locally

elastic-package test

Related issues

Screenshots

@gogochan
gogochan marked this pull request as ready for review April 23, 2024 16:15
@gogochan
gogochan requested a review from a team as a code owner April 23, 2024 16:15
@gogochan gogochan added Integration:panw Palo Alto Next-Gen Firewall Team:Security-Deployment and Devices DEPRECATED Deployment and Devices Security team [elastic/sec-deployment-and-devices] labels Apr 23, 2024
@elasticmachine

Copy link
Copy Markdown

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

@gogochan gogochan added the bug Something isn't working, use only for issues label Apr 23, 2024

@taylor-swanson taylor-swanson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

title nit: The name of integration is panw, not panos (which is the data stream name).

@elasticmachine

Copy link
Copy Markdown

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@elasticmachine

Copy link
Copy Markdown

💚 Build Succeeded

History

@gogochan gogochan changed the title [panos] Allow / to be optional in url Apr 23, 2024
@gogochan
gogochan merged commit 122c0a1 into main Apr 24, 2024
@gogochan
gogochan deleted the sdh4638 branch April 24, 2024 12:31
@elasticmachine

Copy link
Copy Markdown

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

gizas pushed a commit that referenced this pull request Apr 26, 2024
qcorporation pushed a commit that referenced this pull request Feb 3, 2025
qcorporation pushed a commit that referenced this pull request Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working, use only for issues Integration:panw Palo Alto Next-Gen Firewall Team:Security-Deployment and Devices DEPRECATED Deployment and Devices Security team [elastic/sec-deployment-and-devices]

3 participants