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

proofpoint_tap: improve clarity of agent config and fix pagination logic #11361

Merged
merged 5 commits into from
Oct 9, 2024

Conversation

efd6
Copy link
Contributor

@efd6 efd6 commented Oct 8, 2024

Proposed commit message

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.
There was no check for the pagination termination state, which for this API is
an empty array of events. Add that check after clarifying the HTTPJSON
configuration, including ensuring that all time ranges are valid queries.

Extend system test conditions to exercise pagination logic and termination,
and fix configuration of request tracing in system tests.

Author's Checklist

  • [ ]

How to test this PR locally

Related issues

Screenshots

@efd6 efd6 added enhancement New feature or request Integration:proofpoint_tap Proofpoint TAP Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations] labels Oct 8, 2024
@efd6 efd6 self-assigned this Oct 8, 2024
@efd6 efd6 force-pushed the s5180-proofpoint_tap branch from 9816952 to 16c8244 Compare October 8, 2024 03:39
There was no check for the pagination termination state, which for this
API is an empty array of events. Add that check.
@efd6 efd6 added the bugfix Pull request that fixes a bug issue label Oct 8, 2024
@efd6 efd6 marked this pull request as ready for review October 8, 2024 04:52
@efd6 efd6 requested a review from a team as a code owner October 8, 2024 04:52
@elasticmachine
Copy link

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

@efd6 efd6 changed the title proofpoint_tap: improve clarity of agent config proofpoint_tap: improve clarity of agent config and fix pagination logic Oct 8, 2024
kcreddy
kcreddy previously approved these changes Oct 8, 2024
Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Thanks for making the templates more readable.

I think the system test should be expanded to cover one pagination call so that we exercise more of the "code" paths.

default: |-
[[- $start := (now (parseDuration "-{{initial_interval}}")) -]]
[[- $hour := (parseDuration "1h") -]]
[[- $end := ($start .Add $hour) -]]
Copy link
Member

@andrewkroh andrewkroh Oct 8, 2024

Choose a reason for hiding this comment

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

This needs to use a prefix style call like .Add $start $hour.

If this type of problem always logs an message, then we should add it to https://github.com/elastic/elastic-package/blob/07fb3a2eab67291c015a63c8d50ffe42d8391550/internal/testrunner/runners/system/tester.go#L104-L124.

Copy link
Member

Choose a reason for hiding this comment

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

In the logs we have

{
  "@timestamp": "2024-10-08T14:30:31.642Z",
  "component": {
    "binary": "filebeat",
    "dataset": "elastic_agent.filebeat",
    "id": "httpjson-default",
    "type": "httpjson"
  },
  "ecs.version": "1.6.0",
  "error": {
    "message": "template: :3:13: executing \"\" at <$start>: can't give argument to non-function $start"
  },
  "id": "httpjson-proofpoint_tap.clicks_blocked-d1a045a5-2476-4a83-90c7-26b5e0cb5a95",
  "input_source": "http://svc-proofpoint_tap:8080/v2/siem/clicks/blocked",
  "input_url": "http://svc-proofpoint_tap:8080/v2/siem/clicks/blocked",
  "log": {
    "source": "httpjson-default"
  },
  "log.level": "debug",
  "log.logger": "input.httpjson-cursor",
  "log.origin": {
    "file.line": 111,
    "file.name": "httpjson/value_tpl.go",
    "function": "github.com/elastic/beats/v7/x-pack/filebeat/input/httpjson.(*valueTpl).Execute.func2"
  },
  "message": "template execution failed",
  "service.name": "filebeat",
  "target": "interval"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is surprising. The issue is not the call order, this is just not possible with this mechanism. I'll explore further. The key docs are

The name of a niladic method of the data, preceded by a period, such as .Method The result is the value of invoking the method with dot as the receiver, dot.Method(). Such a method must have one return value (of any type) or two return values, the second of which is an error. If it has two and the returned error is non-nil, execution terminates and an error is returned to the caller as the value of Execute. Method invocations may be chained and combined with fields and keys to any depth: .Field1.Key1.Method1.Field2.Key2.Method2 Methods can also be evaluated on variables, including chaining: $x.Method1.Field

The key word here is "niladic".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, this is harder than it really should be, https://go.dev/play/p/3wt50n_Fy2o.

@andrewkroh
Copy link
Member

This is not a new problem from this PR, but I noticed that tracer logs were not being enabled during testing because of this:

 input: httpjson
 service: proofpoint_tap
 vars:
+  enable_request_tracer: true
   url: http://{{Hostname}}:{{Port}}
   principal: xxxx
   secret: xxxx
 data_stream:
   vars:
     preserve_original_event: true
-    enable_request_tracer: true

@efd6
Copy link
Contributor Author

efd6 commented Oct 8, 2024

@andrewkroh I took a look at the system tests to see if there was a way to extend them at least over the pagination logic, but I don't see a way to do this with stream as it exists with any sane approach. I do have an approach though.

@elastic-vault-github-plugin-prod
Copy link

elastic-vault-github-plugin-prod bot commented Oct 8, 2024

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

@andrewkroh
Copy link
Member

LGTM. I would like to do a manual test tomorrow so I can peek at the agent log and tracer log.

Copy link
Member

@andrewkroh andrewkroh 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

💚 Build Succeeded

History

cc @efd6

@efd6 efd6 merged commit 1c07c84 into elastic:main Oct 9, 2024
5 checks passed
@elastic-vault-github-plugin-prod

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

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 enhancement New feature or request Integration:proofpoint_tap Proofpoint TAP Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Proofpoint TAP]: Requests continue after receiving an empty array
4 participants