[receiver/windowseventlogreceiver] Add remote log collection support #33601
Conversation
|
|
||
| // RemoteGroup is a group of remote servers. | ||
| type RemoteGroup struct { | ||
| Credentials RemoteCredentials `mapstructure:",squash"` |
There was a problem hiding this comment.
This may be better was a pointer so if omitted you could use the current user credentials by passing NULL for user, password, and domain - see https://learn.microsoft.com/en-us/windows/win32/wes/accessing-remote-computers
There was a problem hiding this comment.
It will be fine to do it in a separate PR.
There was a problem hiding this comment.
Will look more into this after applying initial feedback.
JonathanWamsley
force-pushed
the
windowseventlogreceiver-remote-collection
branch
2 times, most recently
from
4f70195 to
194da09
Compare
JonathanWamsley
force-pushed
the
windowseventlogreceiver-remote-collection
branch
2 times, most recently
from
e2f76ef to
c883a5e
Compare
There was a problem hiding this comment.
@JonathanWamsley could you please add a test? It will have to be skipped on CI but at least one could run it locally when updating the feature. It could check for some env var, TEST_REMOTE_WINDOWS_EVENTLOG_* where one could put the server and credentials. When these are not defined than the test is skipped.
| #### Remote Configuration | ||
|
|
||
| If collection of the local event log is desired, a separate receiver needs to be created. | ||
|
|
There was a problem hiding this comment.
We need also to include a mention of the requirements on the remote server:
the remote computer must enable the "Remote Event Log Management" Windows Firewall exception; otherwise, when you try to use the session handle, the call will error with RPC_S_SERVER_UNAVAILABLE. The computer to which you are connecting must be running Windows Vista or later.
Source: https://learn.microsoft.com/en-us/windows/win32/wes/accessing-remote-computers
There was a problem hiding this comment.
Updated in readme @pjanotti
JonathanWamsley
force-pushed
the
windowseventlogreceiver-remote-collection
branch
2 times, most recently
from
574734d to
c87ec8a
Compare
JonathanWamsley
force-pushed
the
windowseventlogreceiver-remote-collection
branch
from
1cc9d3c to
631954a
Compare
|
Hey @djaglowski , I believe every time I update and run the CI tests, a different test fails with the exception of 2 of them in the last couple iterations. I don't think they are specific to the changes I made. I have not seen the tests fail from what I have created. Is there something else that needs to be done? Thanks. |
|
We seems to be struggling with flaky tests. I believe some have been unrelated, but there is at least one WEL test which failed the most recent run: https://github.com/open-telemetry/opentelemetry-collector-contrib/actions/runs/10145674230/job/28051942532?pr=33601#step:6:245 |
| @@ -31,6 +31,7 @@ type EventXML struct { | |||
| Security *Security `xml:"System>Security"` | |||
| Execution *Execution `xml:"System>Execution"` | |||
| EventData EventData `xml:"EventData"` | |||
| RemoteServer string `xml:"RemoteServer,omitempty"` | |||
There was a problem hiding this comment.
@JonathanWamsley @pjanotti Do we actually ever expect this field to be present in the XML? Looking at this more closely I think maybe we just added it to the struct so it could be passed along conveniently, but we're always setting it based on the config value. Does that seem right?
There was a problem hiding this comment.
Good point @djaglowski - the same information is already on the event XML returned by the event APIs, see https://learn.microsoft.com/en-us/windows/win32/wes/eventschema-computer-systempropertiestype-element
We don't need this field.
There was a problem hiding this comment.
I went ahead and moved the remote server info to an attribute in #34720, but if it's always the same value then I suppose we don't even need the attribute. Any idea how to assess if these might sometimes be different in a meaningful way?
There was a problem hiding this comment.
Per doc above, Computer records where the event happened. This sounds to me that it can be different than the remote computer that we are targeting. @JonathanWamsley can you help us by testing that?
…pen-telemetry#33601) **Description:** <Describe what has changed.> <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> This PR adds remote log collection for the windows event log receiver. A config supports a single server configuration, multiple servers with multiple credentials configuration and multiple servers with single credentials configuration. **Link to tracking Issue:** <Issue number if applicable> open-telemetry#33100 **Testing:** <Describe what testing was performed and which tests were added.> Added relevant test to the test files that existed. This was tested on a single remote and with a valid / invalid remote to ensure successful collection of logs. Gather local logs was also tested to make sure old functionality is consistent too. **Documentation:** <Describe the documentation added.> Updated Read me documentation --------- Co-authored-by: Daniel Jaglowski <[email protected]>
Description:
This PR adds remote log collection for the windows event log receiver.
A config supports a single server configuration, multiple servers with multiple credentials configuration and multiple servers with single credentials configuration.
Link to tracking Issue:
#33100
Testing:
Added relevant test to the test files that existed.
This was tested on a single remote and with a valid / invalid remote to ensure successful collection of logs. Gather local logs was also tested to make sure old functionality is consistent too.
Documentation:
Updated Read me documentation