add support for CWS on Windows #907

paulcacheux · 2024-01-24T16:18:19Z

What does this PR do?

This PR adds support for CWS on Windows. It also slightly cleans up the system probe recipe and adds a rspec test for this new support.

Motivation

Windows agent builders use chef to install the agent, since we want to enable CWS there, we need this new support.

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Use:

datadog:
  security_agent:
    cws:
      enabled: true

and check that CWS is actually enabled (this requires a custom agent build until 7.52 is released)

Reviewer's Checklist

derekwbrown · 2024-01-24T19:26:18Z

recipes/system-probe.rb

@@ -22,7 +22,7 @@
 # Set the correct agent startup action
 cws_enabled = node['datadog']['security_agent']['cws']['enabled']
 sysprobe_enabled = if is_windows


Suggested change

sysprobe_enabled = if is_windows

cws_enabled = node['datadog']['security_agent']['cws']['enabled']

npm_enabled = node['datadog']['system_probe']['network_enabled']

usm_enabled = node['datadog']['system_probe']['service_monitoring_enabled']

sysprobe_enabled = node['datadog']['system_probe']['enabled']

sysprobe_enabled = sysprobe_enabled || cws_enabled || npm_enabled || usm_enabled

my suggestion didn't work out as clearly as I'd hoped.
I'd replace the whole sysprobe_enabled = if is_windows... block

I did something similar to what you suggested, I think it makes things easier anyway even if we still have a small if is_windows

derekwbrown · 2024-01-24T19:52:34Z

recipes/system-probe.rb

-                   end
+
+sysprobe_enabled ||= npm_enabled || cws_enabled
+unless is_windows


Windows has USM; so on windows system probe should be enabled if usm is enabled.

(this is also somewhat outside the scope of what you're doing, because it's clear the USM support for windows is lacking in the chef recipe). So, I'm fine either way. But there's no underlying reason for the conditional at all.

Ah didn't know that, yeah the USM is checked here but not plugged anywhere after that. I will simplify further then

chouetz · 2024-01-26T08:48:46Z

recipes/security-agent.rb

@@ -20,11 +20,17 @@
 is_windows = platform_family?('windows')

 # Set the correct agent startup action
-security_agent_enabled = !is_windows && node['datadog']['security_agent']['cws']['enabled'] || node['datadog']['security_agent']['cspm']['enabled']
+security_agent_enabled = node['datadog']['security_agent']['cws']['enabled'] || (!is_windows && node['datadog']['security_agent']['cspm']['enabled'])


If you enable cws on windows, why do you change the condition on cspm? Was it initially incorrect?

I think the original code was wrong, CSPM does not support windows for now. So I'm moving the is windows check to only apply to the CSPM part

chouetz · 2024-01-26T10:09:15Z

recipes/security-agent.rb

@@ -65,14 +73,18 @@
 # Common configuration
 service_provider = Chef::Datadog.service_provider(node)

-service_name = 'datadog-agent-security'
+service_name = is_windows ? 'datadog-security-agent' : 'datadog-agent-security'


This is probably not related to this PR, but why???

I have no idea.. I'm just copying what has been done for system probe

is the name of the service on linux datadog-security-agent or datadog-agent-security?
Right now, the name of the service (on windows) is datadog-security-agent. Since we haven't shipped, we can make it datadog-agent-security without breaking anything, and then it will match linux if that's desirable.

chouetz · 2024-01-26T10:14:45Z

spec/system-probe_spec.rb

+        solo.resource_collection.insert(
+          Chef::Resource::Service.new('datadog-agent', solo.run_context))
+        solo.resource_collection.insert(
+          Chef::Resource::Service.new('datadog-agent-security', solo.run_context))


If I believe line 76 in recipes/security-agent.rb in windows the name changes, correct?

Suggested change

Chef::Resource::Service.new('datadog-agent-security', solo.run_context))

Chef::Resource::Service.new('datadog-security-agent', solo.run_context))

I think there is a difference between the "public name" of the service which changes on windows, and the name of the chef service which is common on both platforms

derekwbrown

Approved as is; but noted there are some options we might want to take since we haven't shipped anything yet.

derekwbrown · 2024-01-31T23:36:33Z

recipes/security-agent.rb


 #
 # Configures security-agent agent
-security_agent_config_file = '/etc/datadog-agent/security-agent.yaml'
+security_agent_config_file =
+  if is_windows


this is probably OK for now, and is probably how we do it for system-probe.yaml, too.
But the root config dir is configurable on windows.
So, c:\programdata... is the correct default. But in theory it could be changed. it's not clear if the chef recipe as a whole supports that.

We don't support changing the PROJECTLOCATION nor the APPLICATIONDATADIRECTORY with Chef on Windows.

thanks. that's even better.

derekwbrown · 2024-01-31T23:37:56Z

recipes/security-agent.rb

@@ -65,14 +73,18 @@
 # Common configuration
 service_provider = Chef::Datadog.service_provider(node)

-service_name = 'datadog-agent-security'
+service_name = is_windows ? 'datadog-security-agent' : 'datadog-agent-security'


is the name of the service on linux datadog-security-agent or datadog-agent-security?
Right now, the name of the service (on windows) is datadog-security-agent. Since we haven't shipped, we can make it datadog-agent-security without breaking anything, and then it will match linux if that's desirable.

This reverts commit 9c2c583.

paulcacheux added 3 commits January 24, 2024 16:54

run security agent specs on windows

3cc3713

add security agent test

4e6920f

add support for security agent service on windows

8b275af

derekwbrown approved these changes Jan 24, 2024

View reviewed changes

paulcacheux force-pushed the paulcacheux/cws-windows branch from a92f7c4 to a233d8f Compare January 24, 2024 19:46

derekwbrown reviewed Jan 24, 2024

View reviewed changes

paulcacheux force-pushed the paulcacheux/cws-windows branch from a233d8f to a654c73 Compare January 24, 2024 20:00

small code cleanup

4a73c82

paulcacheux force-pushed the paulcacheux/cws-windows branch from a654c73 to 4a73c82 Compare January 24, 2024 20:14

paulcacheux marked this pull request as ready for review January 24, 2024 20:32

paulcacheux requested a review from a team as a code owner January 24, 2024 20:32

paulcacheux force-pushed the paulcacheux/cws-windows branch 3 times, most recently from 4628528 to 6233eab Compare January 25, 2024 08:02

add rspec test for windows on CWS

0617920

paulcacheux force-pushed the paulcacheux/cws-windows branch from 6233eab to 0617920 Compare January 25, 2024 08:06

chouetz approved these changes Jan 26, 2024

View reviewed changes

lebauce requested a review from derekwbrown January 31, 2024 22:47

derekwbrown approved these changes Jan 31, 2024

View reviewed changes

paulcacheux merged commit 9c2c583 into main Feb 12, 2024
11 of 13 checks passed

paulcacheux deleted the paulcacheux/cws-windows branch February 12, 2024 10:08

paulcacheux added a commit that referenced this pull request Feb 15, 2024

Revert "add support for CWS on Windows (#907)"

7cc1812

This reverts commit 9c2c583.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for CWS on Windows #907

add support for CWS on Windows #907

paulcacheux commented Jan 24, 2024 •

edited

Loading

derekwbrown Jan 24, 2024

derekwbrown Jan 24, 2024

paulcacheux Jan 24, 2024

derekwbrown Jan 24, 2024

paulcacheux Jan 24, 2024

chouetz Jan 26, 2024

paulcacheux Jan 26, 2024

chouetz Jan 26, 2024

paulcacheux Jan 26, 2024

derekwbrown Jan 31, 2024

chouetz Jan 26, 2024

paulcacheux Jan 26, 2024

derekwbrown left a comment

derekwbrown Jan 31, 2024

julien-lebot Feb 1, 2024

derekwbrown Feb 1, 2024

derekwbrown Jan 31, 2024

-sysprobe_enabled = if is_windows
+cws_enabled = node['datadog']['security_agent']['cws']['enabled']
+npm_enabled = node['datadog']['system_probe']['network_enabled']
+usm_enabled = node['datadog']['system_probe']['service_monitoring_enabled']
+sysprobe_enabled = node['datadog']['system_probe']['enabled']
+sysprobe_enabled = sysprobe_enabled || cws_enabled || npm_enabled || usm_enabled

	Chef::Resource::Service.new('datadog-agent-security', solo.run_context))
	Chef::Resource::Service.new('datadog-security-agent', solo.run_context))

add support for CWS on Windows #907

add support for CWS on Windows #907

Conversation

paulcacheux commented Jan 24, 2024 • edited Loading

What does this PR do?

Motivation

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Reviewer's Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwbrown left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulcacheux commented Jan 24, 2024 •

edited

Loading