[microsoft_dhcp] changes/fix to set values for source and the right values for host; #… by xtruthx · Pull Request #7633 · elastic/integrations

xtruthx · 2023-09-01T07:58:49Z

…7630

Type of change

Bug
Enhancement

What does this PR do?

Explainend in #7630

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.

Author's Checklist

Is the handling for the field object host complete and valid (not sure actually how they created normally)
Not sure about the test *-expected.json i adapt the non DHCPv6 as example like i think it should bee

Related issues

Closes [microsoft_dhcp] Wrong field name usage in processing conflict with the content host.name #7630

Pinging @elastic/integrations (Team:Integrations)

…7630

elasticmachine · 2023-09-01T08:02:03Z

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS

Expand to view the summary

Build stats

Start Time: 2023-11-02T07:04:25.120+0000
Duration: 16 min 17 sec

Test stats 🧪

Test	Results
Failed	0
Passed	6
Skipped	0
Total	6

🤖 GitHub comments

Expand to view the GitHub comments

To re-run your PR in the CI, just comment with:

/test : Re-trigger the build.

xtruthx · 2023-10-02T06:03:03Z

Can some one look into this please? If i am wrong with my thoughts and the thoughts behind the merge request please let me know that. Ping @elastic/security-external-integrations

efd6

This seems reasonable. Please resolve the conflict and I will run the test build.

…suggested; #7630

efd6 · 2023-10-03T21:37:59Z

Can you also resolve the conflict in the changelog (and ensure the manifest matches the resolution).

xtruthx · 2023-10-19T09:22:06Z

Is there still something to do? did i miss something?

efd6 · 2023-10-22T20:00:28Z

/test

efd6 · 2023-10-22T20:35:35Z

Please run elastic-package build.

xtruthx · 2023-10-23T06:42:22Z

i did a rebase with latest changes from main.
Ran the tests again and noticed that I just missed the failed tests for the pipeline The fields of the "host" object are filled by "observer", these are created by the agent. Do these also need to be in test-logs or where do I place these fields?

efd6 · 2023-10-23T22:06:20Z

/test

efd6 · 2023-10-23T22:56:39Z

/test

efd6 · 2023-10-23T23:54:23Z

/test

elasticmachine · 2023-10-24T00:11:37Z

🌐 Coverage report

Name	Metrics % (`covered/total`)	Diff
Packages	100.0% (`1/1`)	💚
Files	100.0% (`3/3`)	💚
Classes	100.0% (`3/3`)	💚
Methods	95.238% (`20/21`)	❕
Lines	97.585% (`687/704`)	❕
Conditionals	100.0% (`0/0`)	💚

efd6 · 2023-10-24T00:17:24Z

/test

efd6 · 2023-10-24T01:10:52Z

@xtruthx Please check that the changes that I've made match your expected semantics.

To answer your question, yes, the agent adds those fields in automatically. They don't need to be added into the tests, but the pipeline does need to be robust to their absence. These are the changes that I've made.

xtruthx · 2023-10-24T04:20:23Z

Thanks @efd6 . That looks great.
Ok I see, so it is better to work with "ignore_empty_value".
Also that the description of the fields must match with the ECS I got now.

Sorry for the stuttering in progress and thanks for the patience. In the next post I know now how to do it:) 😄

efd6 · 2023-10-26T20:13:03Z

/test

efd6 · 2023-10-26T20:50:41Z

The deconfliction needs to have an update to the manifest as well; this is an irritation with how merge conflict get resolved if you do it purely in a merge conflict editor.

… microsoft_dhcp

xtruthx · 2023-10-26T21:58:26Z

/test

xtruthx · 2023-10-26T21:59:22Z

The deconfliction needs to have an update to the manifest as well; this is an irritation with how merge conflict get resolved if you do it purely in a merge conflict editor.

Done and sorry i forgot the manifest. Now i hope it looks good. Time hits me now.

efd6 · 2023-10-26T23:27:09Z

/test

efd6

Thanks

efd6 · 2023-10-31T01:04:03Z

@xtruthx I'm just wondering whether client wouldn't be better than source; this is the terminology used in the MS documentation.

xtruthx · 2023-10-31T04:36:53Z

@efd6 I first thought about putting it into the ECS object 'client'. But then I thought it might be better to use 'source' so that it is also used correctly in the SIEM/Security in the Network overview. I am not sure if the field 'client.source' is also asked. I'll have a look at it again in a moment.

Additional:
This would be an example I think:
https://github.com/elastic/kibana/blob/main/x-pack/plugins/security_solution/public/common/components/visualization_actions/lens_attributes/hosts/kpi_unique_ips_source_metric.ts#L26

But yes from the terminology it would not be wrong to use 'client'.

efd6 · 2023-10-31T04:38:08Z

Thanks.

xtruthx · 2023-11-02T05:49:27Z

@efd6 as it thank last

Thanks.

@efd6 i think that means yes please do the change and i did so.

When building the integration I got the following validation error:

README.md file rendered: /home/daniel/git/elastic/integrations-fork/packages/microsoft_dhcp/docs/README.md
2023/11/02 06:23:30  INFO License text found in "/home/daniel/git/elastic/integrations-fork/LICENSE.txt" will be included in package
2023/11/02 06:23:30  INFO Skipped errors: found 1 validation error:
   1. conditions.kibana.version must be ^8.10.0 or greater to include saved object tags file: kibana/tags.yml (SVR00005)
Package built: /home/daniel/git/elastic/integrations-fork/build/packages/microsoft_dhcp-1.23.0.zip
Done

I think it makes sense to act accordingly and I have set the kibana version to ^8..10.0 only.

I hope now the merge is possible soon

efd6 · 2023-11-02T06:20:25Z

i think that means yes please do the change and i did so.

Oh, sorry. "Thanks" was for the explanation. If source is commonly used in the rules then it makes sense to use it. But rather than more churn, I'll discuss this with others. So don't make any change on that yet.

I think it makes sense to act accordingly and I have set the kibana version to ^8..10.0 only.

We'd rather this wasn't done since it cuts off users. The validation error really should be noted as a warning rather than an error as it is not something that blocks the build.

Please revert that line change.

efd6 · 2023-11-02T06:37:21Z

We've had a discussion; please revert the source to client change.

This reverts commit c8ae38e.

xtruthx · 2023-11-02T06:47:20Z

sorry reverted

efd6 · 2023-11-02T07:04:10Z

/test

efd6

Thanks

elasticmachine · 2023-11-02T23:10:35Z

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

changes/fix to set values for source and the right values for host; #…

b3aa61e

…7630

xtruthx requested a review from a team as a code owner September 1, 2023 07:58

xtruthx added 2 commits September 1, 2023 10:27

add a entry to changelog.yml;#7630

bf4325f

adapt test log expectation as example; #7630

453a84a

xtruthx changed the title ~~changes/fix to set values for source and the right values for host; #…~~ [microsoft_dhcp] changes/fix to set values for source and the right values for host; #… Sep 4, 2023

andrewkroh added Team:Security-External Integrations Integration:microsoft_dhcp Microsoft DHCP labels Oct 3, 2023

efd6 reviewed Oct 3, 2023

View reviewed changes

Comment thread packages/microsoft_dhcp/data_stream/log/_dev/test/pipeline/test-log.log-expected.json Outdated

Comment thread packages/microsoft_dhcp/data_stream/log/elasticsearch/ingest_pipeline/default.yml Outdated

Comment thread packages/microsoft_dhcp/data_stream/log/fields/fields.yml Outdated

resolve discussions, fix document, grok pattern and revert change as …

c214e28

…suggested; #7630

xtruthx and others added 2 commits October 4, 2023 07:43

resolve conflict with changelog and manifest; #7630

dea3a13

Merge branch 'main' into microsoft_dhcp

b486eab

Merge branch 'elastic:main' into microsoft_dhcp

59093ac

xtruthx and others added 2 commits October 23, 2023 08:20

Merge branch 'elastic:main' into microsoft_dhcp

1dc8e9a

build and fix readme;elastic#7630

432028e

fix up pipeline and build docs

206886c

efd6 force-pushed the microsoft_dhcp branch from c1bd744 to 206886c Compare October 24, 2023 00:10

efd6 reviewed Oct 24, 2023

View reviewed changes

Comment thread packages/microsoft_dhcp/data_stream/log/_dev/test/pipeline/test-log.log-expected.json Outdated

xtruthx and others added 2 commits October 26, 2023 09:31

resolve conflict changelog;elastic#7630

d6d78fc

Merge branch 'main' into microsoft_dhcp

b0731a4

xtruthx added 2 commits October 26, 2023 23:52

resolve conflict manifest;elastic#7630

1f127eb

Merge branch 'microsoft_dhcp' of github.com:xtruthx/integrations into…

d338439

… microsoft_dhcp

efd6 approved these changes Oct 31, 2023

View reviewed changes

xtruthx and others added 2 commits November 2, 2023 06:02

Merge branch 'elastic:main' into microsoft_dhcp

cff3b6e

change object field from source.* to client.* ; #7630

c8ae38e

Revert "change object field from source.* to client.* ; #7630"

7c3c9cd

This reverts commit c8ae38e.

efd6 approved these changes Nov 2, 2023

View reviewed changes

efd6 merged commit 31da033 into elastic:main Nov 2, 2023

xtruthx deleted the microsoft_dhcp branch November 3, 2023 06:35

Uh oh!

Conversation

xtruthx commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of change

What does this PR do?

Checklist

Author's Checklist

Related issues

Uh oh!

elasticmachine commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

🤖 GitHub comments

Uh oh!

xtruthx commented Oct 2, 2023

Uh oh!

efd6 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

efd6 commented Oct 3, 2023

Uh oh!

xtruthx commented Oct 19, 2023

Uh oh!

efd6 commented Oct 22, 2023

Uh oh!

efd6 commented Oct 22, 2023

Uh oh!

xtruthx commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efd6 commented Oct 23, 2023

Uh oh!

efd6 commented Oct 23, 2023

Uh oh!

efd6 commented Oct 23, 2023

Uh oh!

elasticmachine commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🌐 Coverage report

Uh oh!

efd6 commented Oct 24, 2023

Uh oh!

efd6 commented Oct 24, 2023

Uh oh!

xtruthx commented Oct 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

efd6 commented Oct 26, 2023

Uh oh!

efd6 commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xtruthx commented Oct 26, 2023

Uh oh!

xtruthx commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efd6 commented Oct 26, 2023

Uh oh!

efd6 left a comment

Choose a reason for hiding this comment

Uh oh!

efd6 commented Oct 31, 2023

Uh oh!

xtruthx commented Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efd6 commented Oct 31, 2023

Uh oh!

xtruthx commented Nov 2, 2023

Uh oh!

efd6 commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

xtruthx commented Sep 1, 2023 •

edited

Loading

elasticmachine commented Sep 1, 2023 •

edited

Loading

xtruthx commented Oct 23, 2023 •

edited

Loading

elasticmachine commented Oct 24, 2023 •

edited

Loading

xtruthx commented Oct 24, 2023 •

edited

Loading

efd6 commented Oct 26, 2023 •

edited

Loading

xtruthx commented Oct 26, 2023 •

edited

Loading

xtruthx commented Oct 31, 2023 •

edited

Loading

efd6 commented Nov 2, 2023 •

edited

Loading