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

lib: relax type hints on flatten function #72

Merged
merged 1 commit into from
Jul 19, 2024
Merged

lib: relax type hints on flatten function #72

merged 1 commit into from
Jul 19, 2024

Conversation

efd6
Copy link
Collaborator

@efd6 efd6 commented Jul 18, 2024

The documented type signature of the flatten function is <list<dyn>> -> <list<dyn>>, but the implementation was <list<typeV>> -> <list<typeV>>. This has an impact on uses of flatten where the input is a nested set of lists, the exact intended use-case. In this situation, the type checker is told that the returned value is a list of lists..., resulting in rejection of valid programs. There is a work-around that can be used, but instead, just make the implementation match the documentation and intention.

Please take a look.

@efd6 efd6 added bug Something isn't working Team:Security-Service Integrations labels Jul 18, 2024
@efd6 efd6 requested a review from a team July 18, 2024 20:49
@efd6 efd6 self-assigned this Jul 18, 2024
@efd6 efd6 marked this pull request as ready for review July 18, 2024 21:00
The documented type signature of the flatten function is <list<dyn>> ->
<list<dyn>>, but the implementation was <list<typeV>> -> <list<typeV>>.
This has an impact on uses of flatten where the input is a nested set of
lists, the exact intended use-case. In this situation, the type checker
is told that the returned value is a list of lists..., resulting in
rejection of valid programs. There is a work-around that can be used,
but instead, just make the implementation match the documentation and
intention.
@efd6
Copy link
Collaborator Author

efd6 commented Jul 18, 2024

While I'm confident that this change will be safe, I want to run some of the integrations that use flatten with this change in place to ensure that this does not break them.

@efd6
Copy link
Collaborator Author

efd6 commented Jul 18, 2024

Tested by build an x-pack/agentbeat with the following changes, and then injecting that into a running v8.14.1 elastic-stack managed stack.

diff --git a/go.mod b/go.mod
index d919df027d..a789fbdf33 100644
--- a/go.mod
+++ b/go.mod
@@ -199,7 +199,7 @@ require (
        github.com/elastic/elastic-agent-system-metrics v0.10.3
        github.com/elastic/go-elasticsearch/v8 v8.14.0
        github.com/elastic/go-sfdc v0.0.0-20240621062639-bcc8456508ff
-       github.com/elastic/mito v1.14.0
+       github.com/elastic/mito v1.14.1-0.20240718224105-890f1baa33f2
        github.com/elastic/tk-btf v0.1.0
        github.com/elastic/toutoumomoma v0.0.0-20221026030040-594ef30cb640
        github.com/foxcpp/go-mockdns v0.0.0-20201212160233-ede2f9158d15
@@ -290,6 +290,7 @@ require (
        github.com/go-stack/stack v1.8.0 // indirect
        github.com/gobuffalo/here v0.6.7 // indirect
        github.com/goccy/go-json v0.10.2 // indirect
+       github.com/goccy/go-yaml v1.11.0 // indirect
        github.com/godror/knownpb v0.1.0 // indirect
        github.com/gofrs/uuid/v5 v5.2.0 // indirect
        github.com/golang-jwt/jwt/v4 v4.5.0 // indirect
diff --git a/go.sum b/go.sum
index a628d95786..57e7673133 100644
--- a/go.sum
+++ b/go.sum
@@ -587,6 +587,8 @@ github.com/elastic/gosigar v0.14.3 h1:xwkKwPia+hSfg9GqrCUKYdId102m9qTJIIr7egmK/u
 github.com/elastic/gosigar v0.14.3/go.mod h1:iXRIGg2tLnu7LBdpqzyQfGDEidKCfWcCMS0WKyPWoMs=
 github.com/elastic/mito v1.14.0 h1:QbyqIuob+JXvygqB01C9KlqEThGx5p8n55HiFTSiMm8=
 github.com/elastic/mito v1.14.0/go.mod h1:J+wCf4HccW2YoSFmZMGu+d06gN+WmnIlj5ehBqine74=
+github.com/elastic/mito v1.14.1-0.20240718224105-890f1baa33f2 h1:mQLyEUGjJjkmWvNxZofc98zE4yAysYvIvGjPJjrOKfA=
+github.com/elastic/mito v1.14.1-0.20240718224105-890f1baa33f2/go.mod h1:J+wCf4HccW2YoSFmZMGu+d06gN+WmnIlj5ehBqine74=
 github.com/elastic/pkcs8 v1.0.0 h1:HhitlUKxhN288kcNcYkjW6/ouvuwJWd9ioxpjnD9jVA=
 github.com/elastic/pkcs8 v1.0.0/go.mod h1:ipsZToJfq1MxclVTwpG7U/bgeDtf+0HkUiOxebk95+0=
 github.com/elastic/ristretto v0.1.1-0.20220602190459-83b0895ca5b3 h1:ChPwRVv1RR4a0cxoGjKcyWjTEpxYfm5gydMIzo32cAw=
@@ -834,6 +836,8 @@ github.com/gocarina/gocsv v0.0.0-20170324095351-ffef3ffc77be/go.mod h1:/oj50ZdPq
 github.com/goccy/go-json v0.9.11/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I=
 github.com/goccy/go-json v0.10.2 h1:CrxCmQqYDkv1z7lO7Wbh2HN93uovUHgrECaO5ZrCXAU=
 github.com/goccy/go-json v0.10.2/go.mod h1:6MelG93GURQebXPDq3khkgXZkazVtN9CRI+MGFi0w8I=
+github.com/goccy/go-yaml v1.11.0 h1:n7Z+zx8S9f9KgzG6KtQKf+kwqXZlLNR2F6018Dgau54=
+github.com/goccy/go-yaml v1.11.0/go.mod h1:H+mJrWtjPTJAHvRbV09MCK9xYwODM+wRTVFFTWckfng=
 github.com/godbus/dbus v0.0.0-20190726142602-4481cbc300e2/go.mod h1:bBOAhwG1umN6/6ZUMtDFBMQR8jRg9O75tm9K00oMsK4=
 github.com/godbus/dbus/v5 v5.0.4/go.mod h1:xhWf0FNVPg57R7Z0UbKHbJfkEywrmjJnf7w5xrFpKfA=
 github.com/godbus/dbus/v5 v5.0.6 h1:mkgN1ofwASrYnJ5W6U/BxG15eXXXjirgZc7CLqkcaro=

Then run the following in the integrations packages directory.

for p in $(rg -g '*.yml.hbs' -F 'flatten()' -l | cut -d/ -f1,3); do (cd $(dirname $p) && elastic-package test system -d $(basename $p)); done
--- Test results for package: logstash - START ---
╭──────────┬─────────────┬───────────┬────────────────────────────────────┬────────┬─────────────────╮
│ PACKAGE  │ DATA STREAM │ TEST TYPE │ TEST NAME                          │ RESULT │    TIME ELAPSED │
├──────────┼─────────────┼───────────┼────────────────────────────────────┼────────┼─────────────────┤
│ logstash │ node_cel    │ system    │ default (variant: logstash_8.12.2) │ PASS   │ 3m15.316468241s │
╰──────────┴─────────────┴───────────┴────────────────────────────────────┴────────┴─────────────────╯
--- Test results for package: logstash - END   ---
--- Test results for package: logstash - START ---
╭──────────┬─────────────┬───────────┬────────────────────────────────────┬────────┬───────────────╮
│ PACKAGE  │ DATA STREAM │ TEST TYPE │ TEST NAME                          │ RESULT │  TIME ELAPSED │
├──────────┼─────────────┼───────────┼────────────────────────────────────┼────────┼───────────────┤
│ logstash │ plugins     │ system    │ default (variant: logstash_8.12.2) │ PASS   │ 38.080567102s │
╰──────────┴─────────────┴───────────┴────────────────────────────────────┴────────┴───────────────╯
--- Test results for package: logstash - END   ---
--- Test results for package: o365 - START ---
╭─────────┬─────────────┬───────────┬───────────┬────────┬───────────────╮
│ PACKAGE │ DATA STREAM │ TEST TYPE │ TEST NAME │ RESULT │  TIME ELAPSED │
├─────────┼─────────────┼───────────┼───────────┼────────┼───────────────┤
│ o365    │ audit       │ system    │ cel       │ PASS   │ 37.310610424s │
╰─────────┴─────────────┴───────────┴───────────┴────────┴───────────────╯
--- Test results for package: o365 - END   ---
--- Test results for package: qualys_vmdr - START ---
╭─────────────┬──────────────────────┬───────────┬───────────┬────────┬───────────────╮
│ PACKAGE     │ DATA STREAM          │ TEST TYPE │ TEST NAME │ RESULT │  TIME ELAPSED │
├─────────────┼──────────────────────┼───────────┼───────────┼────────┼───────────────┤
│ qualys_vmdr │ asset_host_detection │ system    │ default   │ PASS   │ 37.924890458s │
╰─────────────┴──────────────────────┴───────────┴───────────┴────────┴───────────────╯
--- Test results for package: qualys_vmdr - END   ---
--- Test results for package: snyk - START ---
╭─────────┬─────────────┬───────────┬───────────┬────────┬───────────────╮
│ PACKAGE │ DATA STREAM │ TEST TYPE │ TEST NAME │ RESULT │  TIME ELAPSED │
├─────────┼─────────────┼───────────┼───────────┼────────┼───────────────┤
│ snyk    │ audit_logs  │ system    │ default   │ PASS   │ 35.719636901s │
│ snyk    │ audit_logs  │ system    │ multi-org │ PASS   │ 38.747103654s │
╰─────────┴─────────────┴───────────┴───────────┴────────┴───────────────╯
--- Test results for package: snyk - END   ---
--- Test results for package: ti_eclecticiq - START ---
╭───────────────┬─────────────┬───────────┬───────────┬────────┬───────────────╮
│ PACKAGE       │ DATA STREAM │ TEST TYPE │ TEST NAME │ RESULT │  TIME ELAPSED │
├───────────────┼─────────────┼───────────┼───────────┼────────┼───────────────┤
│ ti_eclecticiq │ threat      │ system    │ default   │ PASS   │ 36.502266754s │
╰───────────────┴─────────────┴───────────┴───────────┴────────┴───────────────╯
--- Test results for package: ti_eclecticiq - END   ---
--- Test results for package: ti_otx - START ---
╭─────────┬───────────────────┬───────────┬───────────┬────────┬───────────────╮
│ PACKAGE │ DATA STREAM       │ TEST TYPE │ TEST NAME │ RESULT │  TIME ELAPSED │
├─────────┼───────────────────┼───────────┼───────────┼────────┼───────────────┤
│ ti_otx  │ pulses_subscribed │ system    │ default   │ PASS   │ 36.377400505s │
╰─────────┴───────────────────┴───────────┴───────────┴────────┴───────────────╯
--- Test results for package: ti_otx - END   ---
--- Test results for package: zscaler_zia - START ---
╭─────────────┬────────────────┬───────────┬───────────┬────────┬───────────────╮
│ PACKAGE     │ DATA STREAM    │ TEST TYPE │ TEST NAME │ RESULT │  TIME ELAPSED │
├─────────────┼────────────────┼───────────┼───────────┼────────┼───────────────┤
│ zscaler_zia │ sandbox_report │ system    │ default   │ PASS   │ 37.432331909s │
╰─────────────┴────────────────┴───────────┴───────────┴────────┴───────────────╯
--- Test results for package: zscaler_zia - END   ---

@efd6 efd6 merged commit f538a8d into dev Jul 19, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants